fix(gcp): harden Artifact Registry cleanup policies, and test the fix itself - #384
Conversation
… itself Follow-up to #379. A multi-model review found that its central claim was false and that several declared-retention shapes delete far more than they appear to. The claim first, because it is the reason the rest shipped. #379 said its guards were mutation-tested. They were not, in the way that mattered: deleting the whole IgnoreChanges branch, reverting to the behaviour that silently cleared retention policies, left the suite green, 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 ignore 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 are verified to fail it, including deleting the fix and the call-site dry-run mutation that previously survived. Correctness, in rough order of how much they delete: An empty condition passed the unconditional-DELETE guard, which only checked for nil. tagState defaults to ANY server-side, so `condition: {}` matched every version in the repository. Config decoding is non-strict yaml.Unmarshal, so a mistyped key produces exactly that shape and the error message now says so. A DELETE narrowed only by tagState or newerThan is rejected for the same reason: newerThan alone targets the most recently pushed images, which are the ones currently running. cleanupPolicyDryRun unset became an explicit false, so declaring policies enforced deletion on the first provision and silently flipped a repository someone had put in dry-run out of band. Unset now means dry run; enforcing is an explicit false, and both paths log which one they took. mostRecentVersions with keepCount unset or zero built a KEEP that protects nothing while reading as protective, and KEEP is what outranks a companion DELETE. keepCount >= 1 is now required. condition and mostRecentVersions are a union field; setting both was accepted here and rejected by the API mid-provision. tagPrefixes requires tagState TAGGED. Empty prefixes match everything and are what an unresolved placeholder collapses to. The 10-policy limit is enforced before apply. Duration validation was wrong in both directions: a bare "s" suffix admitted "30days", "abcs", "-5s" and "0s" (which matches every version), while rejecting "30d", the form the provider's own acceptance tests use. Both the day and seconds forms are accepted now and non-positive values are not. Design: retention could not be removed through configuration. Deleting the 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 means unmanaged, and an explicit empty list means managed-and-empty. That also reverts the fixture edit #379 needed. IgnoreChanges was appended to the shared opts slice and attached to eight unrelated resources; it is now scoped to the repository. Validation errors are wrapped with the registry and environment like every other failure here. Adds the YAML decode tests the fields never had. They are only reachable from YAML, and a wrong struct tag would have shipped green with declared retention silently becoming unmanaged. Documents the surface in supported-resources.md, including that omitting the field preserves out-of-band retention, that an empty list is how retention is removed, and that dry run is the default. Still open, and not answerable from this repo: 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 TypeSet. These tests prove SC asks for the right thing; only a preview against a repository carrying live out-of-band policies can prove GCP keeps them. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Semgrep Scan ResultsRepository:
Scanned at 2026-08-20 05:46 UTC |
Security Scan ResultsRepository:
Scanned at 2026-08-20 05:46 UTC |
📊 Statement coverageMeasured on the documented included set (see
Baseline: |
…sories govulncheck reads the stdlib version from the `go` directive in go.mod and the job runs with GOTOOLCHAIN=local, so staying on 1.26.5 reported seven reachable stdlib vulnerabilities: GO-2026-6218 (net/url quadratic resolvePath), GO-2026-6091 (html/template JS regexp context), GO-2026-6090 (crypto/tls post-handshake message limit), GO-2026-6089 (net/http H2 ReadHeaderTimeout), GO-2026-6088 (encoding/xml recursion depth), GO-2026-5972 (encoding/asn1 recursion depth) and GO-2026-5026 (idna ASCII-only Punycode labels). All seven are fixed in go1.26.6. No source change: the advisories landed in the vuln DB on 2026-08-18 and hit every branch whose go directive predates 1.26.6. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
…dded
Second review round on this branch. Two classes of problem, both mine.
The narrowing guard permitted the destructive shape and rejected the safe one.
It accepted a prefix list as narrowing a DELETE, but a prefix selects which
PACKAGES a policy covers, not which ages. So DELETE with
packageNamePrefixes: ["web-app"] deleted every version of that package
including the deployed digest, and for a Docker repository versionNamePrefixes
is the digest itself, so ["sha256:"] matched the whole repository. Both passed.
Meanwhile DELETE with tagState UNTAGGED, which cannot touch a tagged deployed
image, was rejected, and the error text named "a prefix list" as the fix, so
anyone blocked on the safe shape was told in-band to switch to the unsafe one.
That also rejected the provider's own reference configuration, whose first
policy is delete-untagged with tag_state = "UNTAGGED"; a user who deleted the
failing policy to get past validation would be left keep-only, with retention
that is declared, green and deletes nothing.
Now a DELETE condition must set olderThan or target UNTAGGED, and prefix-only
shapes are rejected with an error that says why.
Durations are integer-only. The provider expands the m/h/d forms with
strconv.Atoi, so "1.5h" passed validation and failed at registration, and the
test asserted it as valid.
The rest were coverage gaps in code this PR introduced. The five
prefix/newerThan passthrough branches had no test and each survived deletion,
which means a DELETE could silently lose its narrowing and widen to the whole
repository. The YAML decode test covered 5 of 11 keys, so a dead struct tag on
any prefix field dropped it with no error at all, and both the empty-condition
and narrowing guards still passed. Rewriting the table also deleted the
"unknown tagState" case that existed before, leaving that enum guard unkilled;
restored. The new mostRecentVersions empty-prefix guard had no test either.
Two assertions could not fail for what they claimed. The error-context one
survived dropping both the registry name and the environment, and
TestCleanupDurationAcceptsBothProviderForms re-derived the regex under test
against itself, so it could not have caught the fractional-duration bug. Both
now assert the real values, and the duration test is pinned to evidence: the
day form appears verbatim in terraform-provider-google's generated acceptance
test, the API reports seconds, and fractional quantities are rejected because
of the Atoi expansion.
Fixed four comments and two doc claims that were wrong. hasEmpty said an
unresolved placeholder collapses to ""; it stays literal, and the reachable
path is an ${env:VAR} with the variable unset. The docs said config is
validated before provisioning, but validation runs after the services API has
already been enabled. The docs said deleting the block returns a repository to
unmanaged and leaves live policies in place; once SC has managed the field,
ignoreChanges carries the previous value forward from state, so retention
freezes at the last declared list. Also documented that the bare key
"cleanupPolicies:" decodes as absent while "[]" removes every policy, which
look nearly identical in review.
The empty-list path logged the most reassuring message on the most destructive
branch: it removes every live policy and said "DRY RUN, nothing is deleted",
but dryRun gates deletion of versions by policies, not removal of the policies
themselves. It now warns.
Adds a policy-name length check, prefix-error messages that name which block
they came from, deterministic ordering so two empty lists cannot produce a
random field name, and an index for a nameless policy.
Eleven mutants verified to fail: the four passthrough branches, the tagState
enum widened to accept RELEASED, the mostRecentVersions empty-prefix guard, the
error wrap, and four YAML struct tags. Full suite green apart from two
pre-existing failures unrelated to this change: TestDetectRootDir fails at HEAD
in this worktree, and the OIDC retry test is timing-flaky and passes 3/3 on
re-run.
Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
|
Second review round applied ( The narrowing rule was backwardsIt accepted a prefix list as narrowing a
It also rejected the provider's own reference configuration, whose first policy is A Durations are integer-onlyThe provider expands the Coverage gaps in code this PR addedThe five prefix/ Two assertions could not fail for what they claimedThe error-context assertion survived dropping both the registry name and the environment. Wrong statements correctedFour comments and two doc claims. And the most destructive branch carried the most reassuring log: VerificationEleven mutants verified to fail: the four passthrough branches, the Two suite failures are unrelated and pre-existing: Upgrade note for the release description
Still open and not answerable from this repo: whether |
3 participants
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
IgnoreChangesbranch is live for existing users, and that path is non-destructive. Every defect below fires only when someone writescleanupPolicies: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.
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 theResourceOptionthat is the fix.TestCleanupPolicyFieldsCoverDryRunasserted a literal against itself, so changing the call site to ignore onlycleanupPoliciesdropped dry-run from the list with no failure.HaveLen, which survives dropping the policies entirely, hardcoding the action, or droppingKeepCountso aKEEP 20becomes aKEEP 0.MockResourceArgs.RegisterRPCexposes both the inputs and the ignore list underWithMocks, 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
condition: {}tagStatedefaults toANY→ every versionDELETEwith onlytagStateornewerThannewerThanalone targets the images currently runningolderThanor a prefixcleanupPolicyDryRununsetfalse→ enforced on first provision, and flipped an out-of-band dry-run repo into enforcingfalse, and both paths log whichmostRecentVersionswith nokeepCountkeepCount >= 1requiredcondition+mostRecentVersionstagPrefixeswithoutTAGGED""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 callsreflect.MakeSlicefor 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
IgnoreChangeswas appended to the sharedoptsslice 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 insupported-resources.md.Still open
Whether
IgnoreChangessuppresses 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 honouringDiffRequest.ignore_changesfor aschema.TypeSet(pulumi-terraform-bridge#1756, open).These tests prove SC asks for the right thing. Only a
pulumi previewagainst 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.