|
| 1 | +# SKY-1100 follow-up precision plan |
| 2 | + |
| 3 | +Task: continue PR #1067 by implementing the high-priority precision fixes identified by the orphan ConfigMap/Secret competitor comparison and product review. |
| 4 | + |
| 5 | +Premise and scope: |
| 6 | + |
| 7 | +- This is detector precision work for the `orphanConfigMapSecret` audit check. The dangerous failure mode is a false-positive cleanup recommendation for a ConfigMap/Secret that is actually consumed by convention or platform/controller behavior. |
| 8 | +- Keep this follow-up intentionally narrow. Implement the three live-supported, high-value suppressions: |
| 9 | + 1. Dynamic-loader ConfigMaps labeled for Grafana/Prometheus/fluentd/K8sGPT dynamic consumption. |
| 10 | + 2. ConfigMap leader-election state with the canonical `control-plane.alpha.kubernetes.io/leader` annotation. |
| 11 | + 3. Narrow GKE/GMP managed add-on Secret suppression for the sampled `gmp-system/webhook-tls` class. |
| 12 | +- Do not build low-evidence/niche handlers in this PR: SecretProviderClass, Volcano command-line refs, Argo Workflows/CNPG controller config, or cert-manager `Certificate.status.nextPrivateKeySecretName`. Those were either source-derived only, 1-2 sampled objects, or likely label-covered in real cert-manager Secrets. |
| 13 | + |
| 14 | +Current-code evidence: |
| 15 | + |
| 16 | +- `pkg/audit/checks.go:808-829` emits orphan ConfigMap findings after checking referenced refs, `kube-root-ca`, `isKnownPlatformConfigMap`, and controller owner references. |
| 17 | +- `pkg/audit/checks.go:831-858` emits orphan Secret findings after checking references, service-account tokens, Helm release Secrets, cert-manager certificate labels, `isKnownPlatformSecret`, and owner refs. |
| 18 | +- `pkg/audit/checks.go:864-935` contains the ConfigMap platform suppressor. It has narrow GKE/GMP ConfigMap names and controller platform cases, but no dynamic-loader label or leader-election annotation handling. |
| 19 | +- `pkg/audit/checks.go:937-975` contains the Secret platform suppressor. It has SealedSecrets, Argo CD, cert-manager webhook/account-key, `gmp-public/alertmanager`, and Crossplane root CA suppressions, but no `gmp-system/webhook-tls` managed add-on case. |
| 20 | +- `pkg/audit/checks_test.go:1287-1480` is the existing platform-artifact positive/negative corpus for `orphanConfigMapSecret`; this is the right place to add the new suppressions and same-looking negative cases. |
| 21 | +- `pkg/audit/checks_test.go:1523-1802` already pins precision paths for workload refs, terminal jobs/pods, ServiceAccount imagePullSecrets, ephemeral containers, and CRD refs. |
| 22 | +- The comparison report confirmed the live evidence: |
| 23 | + - `/private/tmp/radar-orphan-compare/reports/final-orphan-tool-comparison-report-v2.md:104-121` identifies 31 dashboard-looking Radar findings, `datadog-operator-lock`, and `gmp-system/webhook-tls` as concrete precision gaps. |
| 24 | + - `/private/tmp/radar-orphan-compare/reports/final-orphan-tool-comparison-report-v2.md:146-148` ranks dynamic-loader labels, leader-election annotations, and narrow GKE/GMP managed add-on suppression as the first recommended changes. |
| 25 | + - `/private/tmp/radar-orphan-compare/reports/source-logic-report.md:150` names K8sGPT's dynamic-loader labels: `grafana_dashboard`, `grafana_datasource`, `prometheus_rule`, `fluentd_config`, and `k8sgpt.ai/dynamically-loaded=true`. |
| 26 | + |
| 27 | +Certain: |
| 28 | + |
| 29 | +- Add a small helper in `pkg/audit/checks.go`, near `labelsValue`, for label/annotation predicates rather than scattering string checks through the suppressors. |
| 30 | +- For ConfigMaps, extend `isKnownPlatformConfigMap` to suppress: |
| 31 | + - label keys `grafana_dashboard`, `grafana_datasource`, `prometheus_rule`, `fluentd_config` when present with a non-empty value other than clear false values (`false`, `0`, `no`). |
| 32 | + - label `k8sgpt.ai/dynamically-loaded` only when the value is truthy (`true`, `1`, `yes`), because that label is explicit state rather than a generic loader selector. |
| 33 | + - annotation `control-plane.alpha.kubernetes.io/leader` when non-empty. |
| 34 | +- For Secrets, extend `isKnownPlatformSecret` to suppress only the GKE/GMP sampled shape: namespace `gmp-system`, name `webhook-tls`, and strong managed add-on evidence via either `addonmanager.kubernetes.io/mode=Reconcile` or a `components.gke.io/` metadata key. Do not suppress every `webhook-tls` or every `gmp-system` Secret. |
| 35 | +- Add tests to `TestOrphanConfigMapSecretSkipsKnownPlatformArtifacts` for representative dynamic-loader labels, leader-election annotation, and the GKE/GMP `webhook-tls` Secret. |
| 36 | +- Add negative tests to `TestOrphanConfigMapSecretKnownPlatformArtifactNegativeCases` for: |
| 37 | + - a dashboard-looking name with no dynamic-loader label (e.g. `koala-grafana-dashboards`) remains flagged. |
| 38 | + - a `grafana_dashboard=false` ConfigMap remains flagged. |
| 39 | + - a `control-plane.alpha.kubernetes.io/leader` annotation with an empty value remains flagged. |
| 40 | + - a `gmp-system/webhook-tls` Secret without managed add-on evidence remains flagged. |
| 41 | + |
| 42 | +Confident: |
| 43 | + |
| 44 | +- No registry copy change is needed. The current copy says "supported controller configuration" (`pkg/audit/registry.go:285-291`); these convention suppressions are part of the supported platform/controller carve-out, not a new user-facing concept. |
| 45 | +- No visual-test is needed. This is backend audit logic only; the UI surfaces fewer findings but no rendered behavior/layout changes. |
| 46 | +- No live mutation is needed. Live validation can be done by building/running the PR binary against already-authenticated clusters and comparing the orphan check output count/names before/after, but unit tests are the main preservation proof. |
| 47 | + |
| 48 | +Needs-input: |
| 49 | + |
| 50 | +- None expected. The product decision to keep this precise and not add broad generic scanning was already settled in the product review. If Claude disagrees and argues for adding a wider mechanism or a user-facing opt-out annotation now, treat that as Discuss and stop. |
| 51 | + |
| 52 | +Implementation sequence: |
| 53 | + |
| 54 | +1. Edit `pkg/audit/checks.go`: |
| 55 | + - Add metadata helpers: `metadataValue`, `hasMetadataKeyPrefix`, `metadataValueEqualFold`, and a compact dynamic-loader predicate. Keep names local and explicit. |
| 56 | + - Update `isKnownPlatformConfigMap` with dynamic-loader and leader-election checks after nil/explicit name checks but before controller-specific name allowlists. |
| 57 | + - Update `isKnownPlatformSecret` with the narrow `gmp-system/webhook-tls` managed add-on check. |
| 58 | +2. Edit `pkg/audit/checks_test.go`: |
| 59 | + - Extend the positive platform fixture list and expected suppressed keys. |
| 60 | + - Extend negative cases for false-looking labels and insufficient GKE/GMP evidence. |
| 61 | +3. Run focused tests: |
| 62 | + - `cd pkg && go test ./audit -run 'TestOrphanConfigMapSecret'` |
| 63 | +4. Run broader repo checks before PR update: |
| 64 | + - `make test` |
| 65 | + - `make build` |
| 66 | + - `make tsc` is redundant if `make build` includes frontend build, but run it if the repo command expects it separately in the QA ledger. |
| 67 | + - `visual-test: skipped (backend audit logic only)`. |
| 68 | +5. Optional live validation after tests: |
| 69 | + - Build/run PR binary and query `/api/audit?raw=true` on a GKE cluster that previously had dashboard and `gmp-system/webhook-tls` findings. |
| 70 | + - Verify the specific dynamic-loader/leader/GMP findings are absent and ordinary unlabeled dashboard-ish rows remain if present. |
| 71 | + |
| 72 | +Risk / blast radius: |
| 73 | + |
| 74 | +- Affected surface: only the `orphanConfigMapSecret` audit check. |
| 75 | +- Main risk: over-suppression, especially if a user labels an actually-unused ConfigMap with a dynamic-loader key by accident. Mitigation: require precise known keys and false-value negative tests; do not suppress by name pattern alone. |
| 76 | +- Residual risk: unlabeled dynamic dashboard ConfigMaps remain possible false positives; this is intentional because suppressing by name would hide real stale ConfigMaps. |
0 commit comments