GCP rule hardening - Part 1

cleancloud/providers/gcp/rules/disk_unattached.py

@@ -1,3 +1,42 @@
+"""
+Rule: gcp.compute.disk.unattached
+
+    (spec — docs/specs/gcp/disk_unattached.md)
+
+Intent:
+    Detect Compute Engine persistent disks that are currently unattached to
+    any VM and still bill for storage so they can be reviewed as conservative
+    cleanup candidates.
+
+Exclusions:
+    - disk record malformed or name absent/empty (spec 8.1)
+    - aggregated scope key unsupported or unresolvable (spec 8.2)
+    - disk status not exactly "READY" (spec 8.4)
+    - disk users field unresolvable / not an explicit empty list (spec 8.5)
+    - disk users non-empty (spec 8.6)
+
+Detection:
+    - disk status == "READY"
+    - users[] is explicitly an empty list (no current VM attachment)
+    - covers zonal (zones/ZONE) and regional (regions/REGION) scopes
+
+Confidence (spec 9.4):
+    - Zonal, detached < 24h: LOW
+    - Zonal, detached 24h–7d: MEDIUM
+    - Zonal, detached >= 7d (or never): HIGH
+    - Regional, detached < 24h: LOW
+    - Regional, otherwise: MEDIUM
+
+Cost model (spec 9.5):
+    - estimated_monthly_cost_usd = None
+    - No flat region-reference price table; pricing varies by type, region, and currency.
+    - State only that unattached disks continue to incur storage charges.
+
+APIs:
+    - compute.disks.list (via disks.aggregatedList)
+"""
+
+import warnings
 from datetime import datetime, timezone
 from typing import List, Optional
 
@@ -9,28 +48,6 @@
 from cleancloud.core.finding import Finding
 from cleancloud.core.risk import RiskLevel
 
-# GCP Persistent Disk pricing ($/GB/month, us-central1 reference).
-# Source: https://cloud.google.com/compute/disks-image-pricing
-#
-# Notes:
-# - pd-extreme also bills for provisioned IOPS separately (not estimable from listing).
-# - Hyperdisk types bill for capacity + provisioned IOPS and/or throughput separately.
-#   Only the capacity component can be estimated here; actual cost is typically higher.
-#   hyperdisk-balanced: 3,000 IOPS and 140 MiB/s free baseline, additional usage billed.
-#   hyperdisk-extreme: all provisioned IOPS billable, no free baseline.
-#   hyperdisk-throughput: all provisioned throughput billable.
-#   Using pd-standard rate as conservative capacity-only floor for all hyperdisk types.
-_DISK_TYPE_COST_PER_GB: dict = {
-    "pd-standard": 0.04,
-    "pd-balanced": 0.10,
-    "pd-ssd": 0.17,
-    "pd-extreme": 0.125,  # capacity only; provisioned IOPS billed separately
-    "hyperdisk-balanced": 0.04,  # capacity only; IOPS + throughput billed separately
-    "hyperdisk-extreme": 0.04,  # capacity only; all IOPS billed separately
-    "hyperdisk-throughput": 0.04,  # capacity only; throughput billed separately
-}
-_DEFAULT_COST_PER_GB = 0.04  # pd-standard as conservative fallback
-
 _HYPERDISK_TYPES = frozenset({"hyperdisk-balanced", "hyperdisk-extreme", "hyperdisk-throughput"})
 
 
@@ -46,17 +63,6 @@ def find_unattached_disks(
     Persistent disks bill regardless of attachment status. Orphaned disks are
     commonly left behind after VM deletion — a high-volume, zero-utility cost source.
 
-    Detection logic:
-    - Disk status == READY (exists, not being created or deleted)
-    - Disk users list is empty (not attached to any instance)
-    - Covers both zonal disks (zones/ZONE) and regional disks (regions/REGION)
-
-    Confidence:
-    - Zonal disk, unattached, detached > 7 days ago (or never detached): HIGH
-    - Zonal disk, detached 24h–7d ago: MEDIUM — may still be in a deletion pipeline
-    - Either type detached < 24h ago: LOW — very likely mid-pipeline
-    - Regional disk, unattached: MEDIUM — may be intentionally kept for HA failover
-
     IAM permissions required:
     - compute.disks.list (included in roles/compute.viewer)
     """
@@ -71,17 +77,39 @@ def find_unattached_disks(
     # See: https://cloud.google.com/compute/docs/reference/rest/v1/disks/aggregatedList
     try:
         for scope_key, scope_disks in disks_client.aggregated_list(project=project_id):
+            # spec 9.6 / 9.1.8-9: surface partial-success warnings so callers know
+            # that zero findings cannot be interpreted as a clean project.
+            _scope_warning = getattr(scope_disks, "warning", None)
+            if _scope_warning and getattr(_scope_warning, "code", ""):
+                warnings.warn(
+                    f"gcp.compute.disk.unattached: aggregated inventory returned partial "
+                    f"coverage for scope '{scope_key}' "
+                    f"(code: {_scope_warning.code}) — findings from this scope may be incomplete",
+                    UserWarning,
+                    stacklevel=2,
+                )
+
             if not scope_disks.disks:
                 continue
 
-            # scope_key is "zones/us-central1-a" or "regions/us-central1"
+            # --- spec 8.2: scope key must be exactly "zones/ZONE" or "regions/REGION" ---
             scope_parts = scope_key.split("/")
+            if len(scope_parts) != 2:
+                continue  # too few or too many segments — skip
+
             scope_type = scope_parts[0]  # "zones" or "regions"
             location = scope_parts[1]  # zone name or region name
 
             if scope_type == "zones":
                 zone_name = location
-                region = zone_name.rsplit("-", 1)[0]  # "us-central1-a" -> "us-central1"
+                # spec 7 / 9.1.5: derive region by stripping the trailing zone letter.
+                # GCP zone names always end in a single alphabetic letter (e.g. "-a").
+                # A scope like zones/us-central1 (no zone letter) must skip — rsplit
+                # alone would silently derive "us" from "us-central1", which is wrong.
+                zone_parts = zone_name.rsplit("-", 1)
+                if len(zone_parts) < 2 or len(zone_parts[1]) != 1 or not zone_parts[1].isalpha():
+                    continue  # spec 8.2 / 7: zone lacks a single-letter suffix → skip
+                region = zone_parts[0]
                 is_regional = False
             elif scope_type == "regions":
                 zone_name = None
@@ -94,46 +122,55 @@ def find_unattached_disks(
                 continue
 
             for disk in scope_disks.disks:
+                # spec 8.1: skip malformed records with absent / empty name
+                if not disk.name:
+                    continue
+
+                # spec 8.4: only READY disks are eligible
                 if disk.status != "READY":
                     continue
-                if disk.users:  # non-empty = attached to one or more VMs
+
+                # spec 8.5: users must be an explicit list — any other type
+                # (None, str, dict, tuple, …) means attachment state is unresolvable.
+                # An empty list is the only value that safely means "unattached".
+                if not isinstance(disk.users, list):
+                    continue
+                # spec 8.6: any current user entry means attached → skip
+                if disk.users:
                     continue
 
-                # Extract short disk type from full resource URL
+                # Extract short disk type from full resource URL.
                 # e.g. "zones/us-central1-a/diskTypes/pd-ssd" -> "pd-ssd"
+                # spec 7: fallback to "unknown" (not a guessed default type)
                 disk_type_url = disk.type_ or ""
-                disk_type = disk_type_url.split("/")[-1] if disk_type_url else "pd-standard"
+                disk_type = disk_type_url.split("/")[-1] if disk_type_url else "unknown"
 
-                size_gb = int(disk.size_gb) if disk.size_gb is not None else 0
-                cost_per_gb = _DISK_TYPE_COST_PER_GB.get(disk_type, _DEFAULT_COST_PER_GB)
-                monthly_cost = round(size_gb * cost_per_gb, 2)
+                # spec 7: parse size as non-negative int; use 0 on malformed values
+                try:
+                    size_gb = int(disk.size_gb) if disk.size_gb is not None else 0
+                except (ValueError, TypeError):
+                    size_gb = 0
 
                 labels = dict(disk.labels) if disk.labels else {}
 
                 # Regional disks use a different resource path than zonal disks.
-                # lastAttachTimestamp / lastDetachTimestamp are [Output Only] fields
-                # confirmed in GCP Disk API:
-                # https://cloud.google.com/compute/docs/reference/rest/v1/disks
                 if is_regional:
                     resource_id = f"projects/{project_id}/regions/{region}/disks/{disk.name}"
                     report_location = region
                 else:
                     resource_id = f"projects/{project_id}/zones/{zone_name}/disks/{disk.name}"
                     report_location = zone_name
 
-                # Confidence: regional disks are often intentionally provisioned for HA
-                # (replicated across two zones); an unattached regional disk is more
-                # ambiguous than an unattached zonal disk.
+                # spec 9.4: confidence baseline
                 confidence = ConfidenceLevel.MEDIUM if is_regional else ConfidenceLevel.HIGH
 
                 # Modulate confidence by time since last detach.
-                # A disk detached < 24h ago may be mid-pipeline (VM deleted, disk
-                # deletion pending). After 7 days the disk is almost certainly orphaned.
-                last_detach_str = disk.last_detach_timestamp or ""
+                # spec 7: treat non-string timestamps as unknown rather than crashing.
+                raw_ts = disk.last_detach_timestamp
+                last_detach_str = raw_ts if isinstance(raw_ts, str) else ""
                 hours_since_detach: Optional[float] = None
                 if last_detach_str:
                     try:
-                        # GCP uses RFC3339; handle both "+HH:MM" and "Z" offsets
                         ts = last_detach_str.replace("Z", "+00:00")
                         last_detach = datetime.fromisoformat(ts)
                         if last_detach.tzinfo is None:
@@ -142,17 +179,17 @@ def find_unattached_disks(
                         if hours_since_detach < 24:
                             confidence = ConfidenceLevel.LOW
                         elif hours_since_detach < 7 * 24 and not is_regional:
-                            # Zonal disk detached 24h–7d ago: still plausibly in a pipeline.
-                            # Regional disks stay at their MEDIUM base regardless.
+                            # Zonal disk detached 24h–7d: plausibly still mid-pipeline.
+                            # Regional disks remain at MEDIUM baseline.
                             confidence = ConfidenceLevel.MEDIUM
-                    except ValueError:
-                        pass
+                    except (ValueError, AttributeError):
+                        pass  # unparseable timestamp — keep baseline confidence
 
                 signals_used = [
                     "Disk status: READY",
                     "No VM users (users list empty)",
-                    f"Disk type: {disk_type} (~${cost_per_gb}/GB/month storage)",
-                    f"Size: {size_gb} GB -> ~${monthly_cost}/month (estimated, region-dependent)",
+                    f"Disk type: {disk_type}",
+                    f"Size: {size_gb} GB",
                 ]
                 if is_regional:
                     signals_used.append(
@@ -161,15 +198,16 @@ def find_unattached_disks(
                 if hours_since_detach is not None:
                     signals_used.append(f"Last detached: {hours_since_detach:.0f}h ago")
 
+                # spec 9.5 / 10.2: never claim a specific dollar cost — pricing varies
                 signals_not_checked = [
+                    "Exact monthly cost (varies by disk type, region, currency, and provisioned performance — see GCP billing)",
                     "Disk reserved for imminent VM recreation",
-                    "Snapshot-only workflow (intentional detachment)",
+                    "Snapshot, image, or template restore dependency (disk may be intentionally detached)",
                     "Cross-project disk sharing",
                 ]
                 if disk_type in _HYPERDISK_TYPES:
                     signals_not_checked.append(
-                        f"Hyperdisk IOPS and throughput charges are billed separately from "
-                        f"capacity — actual monthly cost is likely higher than ~${monthly_cost}"
+                        "Hyperdisk IOPS and throughput charges are billed separately from capacity cost"
                     )
                 if disk_type == "pd-extreme":
                     signals_not_checked.append(
@@ -201,8 +239,7 @@ def find_unattached_disks(
                         summary=(
                             f"Persistent disk '{disk.name}' ({size_gb} GB, {disk_type}) "
                             f"in {'region' if is_regional else 'zone'} '{report_location}' "
-                            f"is not attached to any VM but continues to incur storage "
-                            f"charges (~${monthly_cost}/month, estimated, region-dependent)."
+                            f"is not attached to any VM but continues to incur storage charges."
                         ),
                         reason="Disk has no attached VM (users list is empty)",
                         risk=RiskLevel.MEDIUM,
@@ -214,7 +251,8 @@ def find_unattached_disks(
                             time_window=None,
                         ),
                         details=details,
-                        estimated_monthly_cost_usd=(monthly_cost if monthly_cost > 0 else None),
+                        # spec 9.5.1: cost model is None — pricing varies by type/region/currency
+                        estimated_monthly_cost_usd=None,
                     )
                 )