fix(gcp): stop clearing Artifact Registry cleanup policies SC does not manage - #379
Merged
Conversation
…t manage The Repository resource declares six inputs and cleanupPolicies is not among them. The provider treats that field as authoritative, unlike labels whose schema documents itself as non-authoritative, so omitting it does not mean "leave alone", it means "empty". Provisioning therefore deletes any retention policy another tool configured, silently, on every parent provision. Observed end to end on one project. A workflow attached three policies at 13:35:10 with dryRun enabled. provision.go refreshes before every up unless SkipRefresh, which loaded the live policies into the resource's OUTPUTS while its INPUTS still declared none, and the diff resolved as a deletion: the GCP audit log records pulumi-gcp/v8.41.1 sending an UpdateRepository at 13:37:31 whose cleanupPolicies is empty, and the versioned Pulumi state shows outputs going from three policies to none across those two generations. Both cleanupPolicies and cleanupPolicyDryRun were reset. The repositories held about 1.9 TB of images with retention that had never once taken effect. Only provision clears them. previewStack refreshes and then previews without an up, so client deploys load the drift into state without acting on it, and the next parent provision does the clearing. Two changes, because they answer different needs: Retention can now be declared, which is the better answer where it fits: one system owns the field and the policy is reviewed as code. cleanupPolicies and cleanupPolicyDryRun on ArtifactRegistryConfig map to the provider inputs, with validation up front rather than at apply, since the failure mode is deleted images. Rejected: an action that is neither DELETE nor KEEP, mostRecentVersions under a DELETE action, a policy with neither condition nor mostRecentVersions (which matches every version), an unrecognised tagState (which server-side defaults to ANY and widens the delete set), a duration without the API's seconds suffix (gcloud documents 30d style durations while the API wants 2592000s), duplicate names, and negative keepCount. Action and tagState are case-normalised. Where retention is NOT declared, the field is added to IgnoreChanges rather than sent empty. That preserves an out-of-band policy instead of deleting it, and keeps the distinction between "not managed" and "delete". An explicitly empty list is also "not managed", so an empty YAML sequence cannot be destructive. dryRun is ignored alongside the policies deliberately: ignoring only the policies would let a provision flip a repository another tool put in dry-run into enforcing, turning a reporting run into real deletions. Note IgnoreChanges is a safety net for out-of-band management and its exact diff-suppression behaviour on a field that exists only in outputs should be confirmed with a preview before anyone relies on it. Declaring the policies does not depend on it: those inputs are authoritative either way. Schema regenerated. Both fields are optional, so existing server.yaml files are unaffected. Guards are mutation-tested: treating "no policies" as managed, dropping dryRun from the ignore list, and removing the duration validation each fail a test. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Cre-eD
requested review from
Laboratory,
smecsia and
universe-ops
as code owners
August 13, 2026 14:53
Semgrep Scan ResultsRepository:
Scanned at 2026-08-13 15:26 UTC |
Security Scan ResultsRepository:
Scanned at 2026-08-13 15:26 UTC |
📊 Statement coverageMeasured on the documented included set (see
Baseline: |
This comment has been minimized.
This comment has been minimized.
…ields
Test_Provision compares parsed stacks against the resolved fixtures, and adding
a slice field to ArtifactRegistryConfig made one of them disagree: actual
carried an empty non-nil slice where the fixture had nil.
The cause is pre-existing and general, not specific to this field.
placeholders.go:373 deep-copies configs by reflection and calls
reflect.MakeSlice unconditionally for every slice kind, so any omitted list
arrives at the comparison as an empty non-nil slice. Every resolved fixture is
post-resolution state, which is why it already carries resolved credentials
rather than the ${auth:gcloud} placeholder, so it has to carry the empty slice
too.
Both spellings mean the same thing here. ManagesCleanupPolicies is length-based
and already has a test pinning that an explicitly empty list is "not managed"
rather than "delete everything", so the resolver's normalisation cannot flip the
behaviour.
An earlier attempt normalised the empty slice back to nil inside
ArtifactRegistryConfigReadConfig. Dropped: the resolver runs afterwards and
re-creates the empty slice, so it fixed nothing while implying a guarantee that
does not hold downstream. Instrumenting the read path showed it returning nil on
both passes while the compared value was still empty, which is what pointed at
the resolver.
Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Contributor
Author
|
Do not merge yet — retracting a claim in the PR description, and flagging rework. A multi-model review panel (three independent lenses plus two outside models) took this apart. The most important finding is about my own testing claim. RetractionThe description says the guards are "mutation-tested" and lists three mutations that fail a test. That claim is misleading and I withdraw it.
Correctness issues to fix
Design gapThere is no way to express "managed, but empty", so removing the config block does not remove the policy — it falls back to the ignore branch and the policy keeps deleting. Making the field The open question
No test in this repo can settle that. Before this merges it needs a recorded Reworking. Note for reviewers: rolling this back after any fleet declares policies is worse than not merging, since the revert issues an explicit delete that upstream reports failing with |
smecsia
approved these changes
Aug 14, 2026
Laboratory
approved these changes
Aug 15, 2026
Cre-eD
added a commit
that referenced
this pull request
Aug 20, 2026
… itself (#384) Follow-up to #379, which merged with known defects. A multi-model review panel found its central claim was false and that several declared-retention shapes delete far more than they appear to. **Exposure today is narrow but real:** only the `IgnoreChanges` branch is live for existing users, and that path is non-destructive. Every defect below fires only when someone writes `cleanupPolicies:` in config — which nobody has yet, and which is exactly what the first adopter will do. ## The claim, first #379 said its guards were mutation-tested. They were not, in the way that mattered. - **Deleting the whole fix left the suite green.** Removing the `else { opts = append(opts, sdk.IgnoreChanges(...)) }` branch — reverting to the behaviour that silently cleared retention — passed, because every test exercised the helpers in isolation and nothing observed the `ResourceOption` that *is* the fix. - `TestCleanupPolicyFieldsCoverDryRun` asserted a literal against itself, so changing the **call site** to ignore only `cleanupPolicies` dropped dry-run from the list with no failure. - The happy-path tests asserted only `HaveLen`, which survives dropping the policies entirely, hardcoding the action, or dropping `KeepCount` so a `KEEP 20` becomes a `KEEP 0`. `MockResourceArgs.RegisterRPC` exposes both the inputs and the ignore list under `WithMocks`, so the real provisioning function is now rendered and asserted. **Six mutants verified to fail**, including deleting the fix and the call-site dry-run mutation that previously survived. ## Correctness, ordered by how much they delete | shape | was | now | |---|---|---| | `condition: {}` | passed the guard (nil-only check); `tagState` defaults to `ANY` → **every version** | rejected, with the message noting a typo produces this | | `DELETE` with only `tagState` or `newerThan` | accepted; `newerThan` alone targets the images currently running | must narrow by `olderThan` or a prefix | | `cleanupPolicyDryRun` unset | explicit `false` → **enforced on first provision**, and flipped an out-of-band dry-run repo into enforcing | defaults to dry run; enforcing is an explicit `false`, and both paths log which | | `mostRecentVersions` with no `keepCount` | KEEP protecting nothing while reading as protective | `keepCount >= 1` required | | `condition` + `mostRecentVersions` | accepted; API rejects mid-provision | rejected up front | | `tagPrefixes` without `TAGGED` | accepted; API rejects | rejected | | empty prefix `""` | matched everything; also what an unresolved placeholder collapses to | rejected | | >10 policies | failed at apply | rejected | **Duration validation was wrong in both directions.** The bare `"s"` suffix admitted `"30days"`, `"abcs"`, `"-5s"` and `"0s"` — the last matching every version — while rejecting `"30d"`, the form the provider's own acceptance tests use (`older_than = "30d"`, `newer_than = "7d"`). Both forms are accepted now; non-positive values are not. ## Design: retention could not be removed Deleting the config block fell back to the ignore branch and left the live policy deleting. The field is now `*[]ArtifactRegistryCleanupPolicy`, because the placeholder resolver calls `reflect.MakeSlice` for every slice kind and destroys nil-vs-empty, while its pointer branch returns early on nil. So **absent = unmanaged**, **`cleanupPolicies: []` = managed-and-empty**. That also reverts the fixture edit #379 needed. ## Also `IgnoreChanges` was appended to the shared `opts` slice and attached to eight unrelated resources; now scoped to the repository. Validation errors wrapped with registry and environment. YAML decode tests added — the fields are only reachable from YAML, and a wrong struct tag would have shipped green with declared retention silently becoming unmanaged. Surface documented in `supported-resources.md`. ## Still open Whether `IgnoreChanges` suppresses the diff **at all**. The engine copies old *inputs* over new inputs, and this field has never been in inputs, so suppression rests on the terraform bridge honouring `DiffRequest.ignore_changes` for a `schema.TypeSet` (pulumi-terraform-bridge#1756, open). These tests prove SC asks for the right thing. Only a `pulumi preview` against a repository carrying live out-of-band policies can prove GCP keeps them, and that is worth doing before anyone relies on the preservation path. --------- Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
Repositoryresource declares six inputs andcleanupPoliciesis not among them. The provider treats that field as authoritative — unlikelabels, whose schema documents itself as non-authoritative — so omitting it does not mean "leave alone", it means "empty". Provisioning therefore deletes any retention policy another tool configured, silently, on every parent provision.Evidence
Observed end to end on one project. A workflow attached three policies at 13:35:10 with dry-run enabled.
provision.gorefreshes before everyupunlessSkipRefresh, which loaded the live policies into the resource's outputs while its inputs still declared none, and the diff resolved as a deletion.GCP audit log, all four Artifact Registry config writes in the window:
gcloud/568.0.0gcloud/568.0.0pulumi-gcp/v8.41.1pulumi-gcp/v8.41.1Corroborated by the versioned Pulumi state, where
outputs.cleanupPoliciesgoes 3 →[]andcleanupPolicyDryRuntrue → false across those generations, withinputsabsent throughout.Those repositories held ~1.9 TB of images under retention that had never once taken effect.
Scope: only
provisionclears them.previewStackrefreshes then previews without anup, so client deploys load the drift into state without acting on it; the next parent provision does the clearing.Two changes, answering different needs
Retention can now be declared — the better answer where it fits, because one system owns the field and the policy is reviewed as code.
cleanupPolicies/cleanupPolicyDryRunmap to the provider inputs, validated up front rather than at apply, since the failure mode is deleted images. Rejected:DELETEnorKEEPmostRecentVersionsunder aDELETEaction (reads as protective while deleting)conditionnormostRecentVersions— that matches every version in the repositorytagState, which server-side defaults toANYand widens the delete setgclouddocs use30dstyle; the API wants2592000s)keepCountactionandtagStateare case-normalised, since at least one fleet's committed JSON usesDelete/Keep.Where retention is not declared, the field goes into
IgnoreChangesrather than being sent empty. That preserves an out-of-band policy instead of deleting it, and keeps "not managed" distinct from "delete". An explicitly empty list is also "not managed", so an empty YAML sequence cannot be destructive.cleanupPolicyDryRunis ignored alongside the policies deliberately: ignoring only the policies would let a provision flip a repository another tool put in dry-run into enforcing, turning a reporting run into real deletions.One caveat, stated plainly
IgnoreChangesis the safety net for out-of-band management, and its exact diff-suppression behaviour on a field that exists only inoutputsshould be confirmed with a preview before anyone relies on it — the option is documented as operating on inputs. Declaring the policies does not depend on it: those inputs are authoritative either way. I'd rather ship the escape hatch with that noted than imply it's verified.Tests
Guards are mutation-tested — each of these fails a test when removed:
cleanupPolicyDryRunfrom the ignore listSchema regenerated; both fields are optional, so existing
server.yamlfiles are unaffected.schema-genalso surfaces pre-existing drift indocs/schemas/kubernetes/kubernetescloudextras.json(externalTrafficPolicy,serviceTypeon the Go struct but not the committed schema). Left out again to keep this diff reviewable.