From 1ebbfa176d5cae0c27e90377d412551aac566130 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Wed, 19 Aug 2026 09:27:00 +0400 Subject: [PATCH 1/3] fix(gcp): harden Artifact Registry cleanup policies, and test the fix 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 --- docs/docs/reference/supported-resources.md | 28 ++ pkg/api/tests/refapp_gke_autopilot.go | 6 - pkg/clouds/gcloud/artifactregistry.go | 21 +- pkg/clouds/gcloud/resources_test.go | 71 +++ pkg/clouds/pulumi/gcp/artifactregistry.go | 164 ++++++- .../gcp/artifactregistry_cleanup_test.go | 440 ++++++++++++++---- 6 files changed, 610 insertions(+), 120 deletions(-) diff --git a/docs/docs/reference/supported-resources.md b/docs/docs/reference/supported-resources.md index f8877829..4341ed35 100644 --- a/docs/docs/reference/supported-resources.md +++ b/docs/docs/reference/supported-resources.md @@ -857,6 +857,34 @@ resources: password: "${env:REGISTRY_PASSWORD}" ``` +##### Image retention (`cleanupPolicies`) + +Artifact Registry keeps every image version forever unless a cleanup policy says otherwise, and storage is billed per GB. Retention can be declared here so it is reviewed as code. + +```yaml + cleanupPolicies: + - name: delete-untagged-older-30d + action: DELETE + condition: + tagState: UNTAGGED + olderThan: 30d # or "2592000s"; both are accepted + - name: keep-most-recent-20 + action: KEEP # KEEP wins over a matching DELETE + mostRecentVersions: + keepCount: 20 + cleanupPolicyDryRun: true # report only; set false to delete +``` + +Three behaviours are worth knowing before you use this. + +**Omitting `cleanupPolicies` means Simple Container does not manage retention.** Any policy set outside SC — through `gcloud` or the console — is left alone. This is the default and it is deliberate: the field is authoritative in the provider, so a resource that declares nothing would otherwise *delete* whatever is configured. + +**Declaring it makes Simple Container authoritative.** Policies set outside SC are then replaced by the declared list on the next provision. An explicitly empty list (`cleanupPolicies: []`) means "managed, and I want none", which is how retention is removed; deleting the block entirely returns the repository to unmanaged and leaves the live policies in place. + +**`cleanupPolicyDryRun` defaults to `true`.** Nothing is deleted until it is explicitly set to `false`. Dry run evaluates the policies and reports what they would remove, so run it first and read the result: deleting an image that is still deployed makes the next node reschedule fail to pull, and no provision can restore a deleted layer. + +Configuration is validated before provisioning, and a policy is rejected if it would match far more than it appears to — an empty `condition`, a `DELETE` narrowed only by `tagState` or `newerThan`, a `KEEP` with no `keepCount`, an empty prefix, or a non-positive duration. Note that an unrecognised key in `server.yaml` is silently ignored rather than rejected, so a mistyped condition field would otherwise produce a policy matching every version. + ### **Database Resources** #### **Cloud SQL PostgreSQL** (`gcp-cloudsql-postgres`) diff --git a/pkg/api/tests/refapp_gke_autopilot.go b/pkg/api/tests/refapp_gke_autopilot.go index c8050001..696883bc 100644 --- a/pkg/api/tests/refapp_gke_autopilot.go +++ b/pkg/api/tests/refapp_gke_autopilot.go @@ -91,12 +91,6 @@ var ResolvedRefappGkeAutopilotServerResources = map[string]api.ResourceDescripto Config: &gcloud.ArtifactRegistryConfig{ Credentials: ResolvedCommonGcpCredentials, Location: "europe-west3", - // Empty rather than nil because this fixture is post-resolution: - // the placeholder resolver deep-copies by reflection and calls - // reflect.MakeSlice for every slice kind, so an omitted list - // arrives as an empty non-nil slice. Not managed either way, - // which ManagesCleanupPolicies asserts. - CleanupPolicies: []gcloud.ArtifactRegistryCleanupPolicy{}, }, }, Inherit: api.Inherit{}, diff --git a/pkg/clouds/gcloud/artifactregistry.go b/pkg/clouds/gcloud/artifactregistry.go index ca9395a9..a250f3fb 100644 --- a/pkg/clouds/gcloud/artifactregistry.go +++ b/pkg/clouds/gcloud/artifactregistry.go @@ -26,7 +26,14 @@ type ArtifactRegistryConfig struct { // next provision. Leaving this empty therefore means "SC does not manage // retention", and SC preserves whatever is configured out of band rather // than deleting it. See ManagesCleanupPolicies. - CleanupPolicies []ArtifactRegistryCleanupPolicy `json:"cleanupPolicies,omitempty" yaml:"cleanupPolicies,omitempty"` + // A POINTER so that "absent" and "declared empty" stay distinguishable. + // The placeholder resolver deep-copies configs by reflection and calls + // reflect.MakeSlice for every slice kind, so a nil SLICE arrives as an empty + // one and the difference is destroyed; its pointer branch returns early on + // nil, so a nil POINTER survives. Without that, `cleanupPolicies: []` could + // not mean "managed, and I want none", and removing policies from config + // would silently leave the live ones in place forever. + CleanupPolicies *[]ArtifactRegistryCleanupPolicy `json:"cleanupPolicies,omitempty" yaml:"cleanupPolicies,omitempty"` // CleanupPolicyDryRun evaluates the policies and reports what they would // delete without deleting it. Only meaningful alongside CleanupPolicies. @@ -37,7 +44,17 @@ type ArtifactRegistryConfig struct { // not, the caller must tell Pulumi to ignore the field rather than send an empty // value, which is the difference between "not managed" and "delete the policy". func (c *ArtifactRegistryConfig) ManagesCleanupPolicies() bool { - return len(c.CleanupPolicies) > 0 + return c.CleanupPolicies != nil +} + +// DeclaredCleanupPolicies returns the declared retention, or nil when SC does +// not manage it. An explicitly empty list is "managed, and empty", which is how +// retention is removed. +func (c *ArtifactRegistryConfig) DeclaredCleanupPolicies() []ArtifactRegistryCleanupPolicy { + if c.CleanupPolicies == nil { + return nil + } + return *c.CleanupPolicies } // ArtifactRegistryCleanupPolicy mirrors a single Artifact Registry cleanup diff --git a/pkg/clouds/gcloud/resources_test.go b/pkg/clouds/gcloud/resources_test.go index 44ff1a70..4511244b 100644 --- a/pkg/clouds/gcloud/resources_test.go +++ b/pkg/clouds/gcloud/resources_test.go @@ -476,3 +476,74 @@ func TestReadGkeAutopilotResourceConfig(t *testing.T) { Expect(err).To(HaveOccurred()) }) } + +// The cleanup-policy fields are only ever reachable from YAML, and every other +// test for them constructs Go structs directly. A wrong struct tag would ship +// green while the declared retention silently became "not managed" — which is +// the failure this whole feature exists to prevent. +func TestArtifactRegistryConfigReadsCleanupPoliciesFromYAML(t *testing.T) { + RegisterTestingT(t) + + cfg := &api.Config{Config: map[string]any{ + "projectId": "p", + "location": "europe-west3", + "cleanupPolicies": []any{ + map[string]any{ + "name": "delete-untagged-older-30d", + "action": "DELETE", + "condition": map[string]any{"tagState": "UNTAGGED", "olderThan": "2592000s"}, + }, + map[string]any{ + "name": "keep-most-recent-20", + "action": "KEEP", + "mostRecentVersions": map[string]any{"keepCount": 20}, + }, + }, + "cleanupPolicyDryRun": true, + }} + out, err := ArtifactRegistryConfigReadConfig(cfg) + Expect(err).To(BeNil()) + ar, ok := out.Config.(*ArtifactRegistryConfig) + Expect(ok).To(BeTrue()) + + Expect(ar.ManagesCleanupPolicies()).To(BeTrue()) + got := ar.DeclaredCleanupPolicies() + Expect(got).To(HaveLen(2)) + Expect(got[0].Name).To(Equal("delete-untagged-older-30d")) + Expect(got[0].Action).To(Equal("DELETE")) + Expect(got[0].Condition).NotTo(BeNil()) + Expect(got[0].Condition.TagState).To(Equal("UNTAGGED")) + Expect(got[0].Condition.OlderThan).To(Equal("2592000s")) + Expect(got[1].MostRecentVersions).NotTo(BeNil()) + Expect(*got[1].MostRecentVersions.KeepCount).To(Equal(20)) + Expect(ar.CleanupPolicyDryRun).NotTo(BeNil()) + Expect(*ar.CleanupPolicyDryRun).To(BeTrue()) +} + +// Absent must stay absent through the decode: it is what makes SC preserve +// out-of-band retention instead of deleting it. +func TestArtifactRegistryConfigAbsentCleanupPoliciesStaysUnmanaged(t *testing.T) { + RegisterTestingT(t) + + out, err := ArtifactRegistryConfigReadConfig(&api.Config{Config: map[string]any{ + "projectId": "p", "location": "europe-west3", + }}) + Expect(err).To(BeNil()) + ar := out.Config.(*ArtifactRegistryConfig) + Expect(ar.CleanupPolicies).To(BeNil()) + Expect(ar.ManagesCleanupPolicies()).To(BeFalse()) +} + +// An explicitly empty YAML list must survive as managed-and-empty. +func TestArtifactRegistryConfigEmptyListIsManaged(t *testing.T) { + RegisterTestingT(t) + + out, err := ArtifactRegistryConfigReadConfig(&api.Config{Config: map[string]any{ + "projectId": "p", "location": "europe-west3", "cleanupPolicies": []any{}, + }}) + Expect(err).To(BeNil()) + ar := out.Config.(*ArtifactRegistryConfig) + Expect(ar.CleanupPolicies).NotTo(BeNil(), "an explicit empty list is how retention is removed") + Expect(ar.ManagesCleanupPolicies()).To(BeTrue()) + Expect(ar.DeclaredCleanupPolicies()).To(BeEmpty()) +} diff --git a/pkg/clouds/pulumi/gcp/artifactregistry.go b/pkg/clouds/pulumi/gcp/artifactregistry.go index 4c72e981..bf6025bc 100644 --- a/pkg/clouds/pulumi/gcp/artifactregistry.go +++ b/pkg/clouds/pulumi/gcp/artifactregistry.go @@ -5,6 +5,8 @@ package gcp import ( "fmt" + "regexp" + "strconv" "strings" "github.com/pkg/errors" @@ -83,19 +85,51 @@ func ArtifactRegistry(ctx *sdk.Context, stack api.Stack, input api.ResourceInput // that omits it sends an update clearing whatever is there. So either // declare it, or tell the engine to leave it alone. Sending nothing is the // one option that silently deletes another tool's retention policy. + // Scoped to this resource only. opts is shared with the IAM policy, both + // service accounts and their keys, none of which has a cleanupPolicies + // property, so appending in place attaches a meaningless option to eight + // resources and persists it in their state. + repoOpts := opts if arCfg.ManagesCleanupPolicies() { - policies, err := cleanupPolicyArgs(arCfg.CleanupPolicies) + declared := arCfg.DeclaredCleanupPolicies() + policies, err := cleanupPolicyArgs(declared) if err != nil { - return nil, err + return nil, errors.Wrapf(err, "invalid cleanup policies for artifact registry %q in %q", + artifactRegistryName, input.StackParams.Environment) + } + // Unset means dry run. The failure modes are not symmetric: dry-run when + // you wanted enforcement costs storage and is undone by flipping one + // boolean, while enforcement when you wanted dry run destroys image + // layers that no provision can restore. Enforcing is therefore an + // explicit, reviewable act. + dryRun := true + if arCfg.CleanupPolicyDryRun != nil { + dryRun = *arCfg.CleanupPolicyDryRun } repoArgs.CleanupPolicies = policies - repoArgs.CleanupPolicyDryRun = sdk.Bool(lo.FromPtr(arCfg.CleanupPolicyDryRun)) + repoArgs.CleanupPolicyDryRun = sdk.Bool(dryRun) + if dryRun { + params.Log.Info(ctx.Context(), "artifact registry %q: SC manages %d cleanup policies in DRY RUN; "+ + "policies set outside SC will be replaced, nothing is deleted until cleanupPolicyDryRun is false", + artifactRegistryName, len(declared)) + } else { + params.Log.Warn(ctx.Context(), "artifact registry %q: cleanup policies are ENFORCING (%d policies); "+ + "versions matching a DELETE policy will be permanently removed", + artifactRegistryName, len(declared)) + } } else { - opts = append(opts, sdk.IgnoreChanges(cleanupPolicyFields)) + if arCfg.CleanupPolicyDryRun != nil { + return nil, errors.Errorf("artifact registry %q in %q: cleanupPolicyDryRun is set but cleanupPolicies is not declared, "+ + "so it would have no effect; declare cleanupPolicies or remove cleanupPolicyDryRun", + artifactRegistryName, input.StackParams.Environment) + } + params.Log.Info(ctx.Context(), "artifact registry %q: cleanup policies not declared, leaving any out-of-band retention untouched", + artifactRegistryName) + repoOpts = append(append([]sdk.ResourceOption{}, opts...), sdk.IgnoreChanges(cleanupPolicyFields)) } params.Log.Info(ctx.Context(), "configure artifact registry repository %q", artifactRegistryName) - repo, err := artifactregistry.NewRepository(ctx, artifactRegistryName, &repoArgs, opts...) + repo, err := artifactregistry.NewRepository(ctx, artifactRegistryName, &repoArgs, repoOpts...) if err != nil { return nil, errors.Wrapf(err, "failed to create artifact registry") } @@ -273,12 +307,50 @@ func toRegistryServiceAccountEmailExport(input api.ResourceInput, saType string, // would let a provision flip an out-of-band dry-run repository into enforcing. var cleanupPolicyFields = []string{"cleanupPolicies", "cleanupPolicyDryRun"} +// maxCleanupPolicies is the Artifact Registry limit. Exceeding it fails at +// apply, after other resources in the stack have already been mutated. +const maxCleanupPolicies = 10 + +// cleanupDurationRe accepts the duration forms the provider accepts. Its own +// acceptance tests use the day form ("30d", "7d"), while the REST API reports +// seconds ("2592000s"); DurationDiffSuppress treats them as equivalent, so +// rejecting either would reject valid configuration. +var cleanupDurationRe = regexp.MustCompile(`^([0-9]+(?:\.[0-9]+)?)(s|m|h|d)$`) + +// validateCleanupDuration rejects shapes the provider would reject at apply, +// and zero, which is not a syntax error but a semantic one: "olderThan: 0s" on +// a DELETE policy matches every version in the repository. +func validateCleanupDuration(policy, field, v string) error { + m := cleanupDurationRe.FindStringSubmatch(v) + if m == nil { + return errors.Errorf("cleanup policy %q: %s must be a positive duration such as %q or %q, got %q", + policy, field, "30d", "2592000s", v) + } + if n, err := strconv.ParseFloat(m[1], 64); err != nil || n <= 0 { + return errors.Errorf("cleanup policy %q: %s must be greater than zero, got %q", policy, field, v) + } + return nil +} + +// hasEmpty reports whether a prefix list contains an empty entry. An empty +// prefix matches everything, and it is the shape an unresolved template +// placeholder collapses to, so it is reachable without anyone typing "". +func hasEmpty(vals []string) bool { + return lo.Contains(vals, "") +} + // cleanupPolicyArgs converts the declared retention into provider inputs. // -// Validation is deliberate rather than passing strings through: the provider -// rejects an unknown action or tagState at APPLY, and an Artifact Registry -// misconfiguration is measured in deleted images. +// Validation is deliberate rather than passing strings through. Two classes of +// mistake matter here and neither is caught by the provider in time to help: +// shapes the API rejects fail at APPLY, halfway through a provision; and +// shapes the API ACCEPTS but which match far more than the author intended +// delete images that are still deployed. The guards below are ordered so the +// second class is impossible to express, not merely discouraged. func cleanupPolicyArgs(policies []gcloud.ArtifactRegistryCleanupPolicy) (artifactregistry.RepositoryCleanupPolicyArray, error) { + if len(policies) > maxCleanupPolicies { + return nil, errors.Errorf("artifact registry accepts at most %d cleanup policies, got %d", maxCleanupPolicies, len(policies)) + } out := make(artifactregistry.RepositoryCleanupPolicyArray, 0, len(policies)) seen := make(map[string]bool, len(policies)) for _, p := range policies { @@ -297,6 +369,10 @@ func cleanupPolicyArgs(policies []gcloud.ArtifactRegistryCleanupPolicy) (artifac if p.MostRecentVersions != nil && action != "KEEP" { return nil, errors.Errorf("cleanup policy %q: mostRecentVersions is only valid with a KEEP action", p.Name) } + // condition and mostRecentVersions are a union field in the API. + if p.Condition != nil && p.MostRecentVersions != nil { + return nil, errors.Errorf("cleanup policy %q: condition and mostRecentVersions are mutually exclusive", p.Name) + } if p.Condition == nil && p.MostRecentVersions == nil { return nil, errors.Errorf("cleanup policy %q: needs a condition or mostRecentVersions", p.Name) } @@ -312,15 +388,57 @@ func cleanupPolicyArgs(policies []gcloud.ArtifactRegistryCleanupPolicy) (artifac default: return nil, errors.Errorf("cleanup policy %q: tagState must be TAGGED, UNTAGGED or ANY, got %q", p.Name, c.TagState) } - for field, v := range map[string]string{"olderThan": c.OlderThan, "newerThan": c.NewerThan} { - if v != "" && !strings.HasSuffix(v, "s") { - return nil, errors.Errorf("cleanup policy %q: %s must be a duration in seconds with an 's' suffix, e.g. \"2592000s\", got %q", p.Name, field, v) + // A condition whose every field is empty is not a narrow policy, it + // is every version in the repository: tagState defaults to ANY + // server-side and nothing else constrains it. Config decoding is + // non-strict, so a mistyped key ("olderThen") produces exactly this. + if tagState == "" && c.OlderThan == "" && c.NewerThan == "" && + len(c.TagPrefixes)+len(c.PackageNamePrefixes)+len(c.VersionNamePrefixes) == 0 { + return nil, errors.Errorf("cleanup policy %q: condition has no criteria and would match every version; "+ + "note that an unrecognised key is silently ignored, so check for a typo", p.Name) + } + for _, d := range []struct{ field, value string }{ + {"olderThan", c.OlderThan}, + {"newerThan", c.NewerThan}, + } { + if d.value == "" { + continue + } + if err := validateCleanupDuration(p.Name, d.field, d.value); err != nil { + return nil, err + } + } + // Tag prefixes only mean anything against tagged versions; the API + // rejects the combination rather than ignoring it. + if len(c.TagPrefixes) > 0 && tagState != "TAGGED" { + return nil, errors.Errorf("cleanup policy %q: tagPrefixes requires tagState TAGGED, got %q", p.Name, c.TagState) + } + for field, vals := range map[string][]string{ + "tagPrefixes": c.TagPrefixes, + "packageNamePrefixes": c.PackageNamePrefixes, + "versionNamePrefixes": c.VersionNamePrefixes, + } { + if hasEmpty(vals) { + return nil, errors.Errorf("cleanup policy %q: %s contains an empty prefix, which matches everything", p.Name, field) } } - cond := &artifactregistry.RepositoryCleanupPolicyConditionArgs{ - TagPrefixes: sdk.ToStringArray(c.TagPrefixes), - PackageNamePrefixes: sdk.ToStringArray(c.PackageNamePrefixes), - VersionNamePrefixes: sdk.ToStringArray(c.VersionNamePrefixes), + narrowed := c.OlderThan != "" || len(c.TagPrefixes)+len(c.PackageNamePrefixes)+len(c.VersionNamePrefixes) > 0 + if action == "DELETE" && !narrowed { + // Left here, the policy is either "everything" (tagState only) + // or "everything pushed recently" (newerThan only), and the + // latter is precisely the images currently running. + return nil, errors.Errorf("cleanup policy %q: a DELETE condition must narrow by olderThan or a prefix list; "+ + "tagState or newerThan alone targets versions that are still deployed", p.Name) + } + cond := &artifactregistry.RepositoryCleanupPolicyConditionArgs{} + if len(c.TagPrefixes) > 0 { + cond.TagPrefixes = sdk.ToStringArray(c.TagPrefixes) + } + if len(c.PackageNamePrefixes) > 0 { + cond.PackageNamePrefixes = sdk.ToStringArray(c.PackageNamePrefixes) + } + if len(c.VersionNamePrefixes) > 0 { + cond.VersionNamePrefixes = sdk.ToStringArray(c.VersionNamePrefixes) } if tagState != "" { cond.TagState = sdk.StringPtr(tagState) @@ -334,14 +452,20 @@ func cleanupPolicyArgs(policies []gcloud.ArtifactRegistryCleanupPolicy) (artifac args.Condition = cond } if m := p.MostRecentVersions; m != nil { - if lo.FromPtr(m.KeepCount) < 0 { - return nil, errors.Errorf("cleanup policy %q: keepCount cannot be negative", p.Name) + // keepCount nil or 0 sends most_recent_versions {} with no count, + // which GCP treats as keeping nothing. Since KEEP is what outranks a + // companion DELETE, that reads as a safety net while being none. + if m.KeepCount == nil || *m.KeepCount < 1 { + return nil, errors.Errorf("cleanup policy %q: mostRecentVersions requires keepCount >= 1", p.Name) + } + if hasEmpty(m.PackageNamePrefixes) { + return nil, errors.Errorf("cleanup policy %q: packageNamePrefixes contains an empty prefix, which matches everything", p.Name) } mrv := &artifactregistry.RepositoryCleanupPolicyMostRecentVersionsArgs{ - PackageNamePrefixes: sdk.ToStringArray(m.PackageNamePrefixes), + KeepCount: sdk.IntPtr(*m.KeepCount), } - if m.KeepCount != nil { - mrv.KeepCount = sdk.IntPtr(*m.KeepCount) + if len(m.PackageNamePrefixes) > 0 { + mrv.PackageNamePrefixes = sdk.ToStringArray(m.PackageNamePrefixes) } args.MostRecentVersions = mrv } diff --git a/pkg/clouds/pulumi/gcp/artifactregistry_cleanup_test.go b/pkg/clouds/pulumi/gcp/artifactregistry_cleanup_test.go index fded1323..edc942c9 100644 --- a/pkg/clouds/pulumi/gcp/artifactregistry_cleanup_test.go +++ b/pkg/clouds/pulumi/gcp/artifactregistry_cleanup_test.go @@ -9,157 +9,413 @@ import ( . "github.com/onsi/gomega" "github.com/samber/lo" + "github.com/pulumi/pulumi/sdk/v3/go/common/resource" + sdk "github.com/pulumi/pulumi/sdk/v3/go/pulumi" + + gcpsdk "github.com/pulumi/pulumi-gcp/sdk/v8/go/gcp" + + "github.com/simple-container-com/api/pkg/api" + "github.com/simple-container-com/api/pkg/api/logger" "github.com/simple-container-com/api/pkg/clouds/gcloud" + pApi "github.com/simple-container-com/api/pkg/clouds/pulumi/api" ) -// The provider treats cleanupPolicies as authoritative. A Repository resource -// that omits it sends an update CLEARING whatever is configured, which is how a -// separately-managed retention policy gets deleted on the next provision. -// Observed on payspace-475408 2026-08-12: a policy set at 13:35:10 was cleared -// by pulumi-gcp/v8.41.1 at 13:37:31, in a request whose cleanupPolicies was -// empty. Refresh had loaded the live policy into outputs, the program declared -// no input, and the diff resolved as "delete it". +// arCapture records what ArtifactRegistry() actually asks Pulumi to register. // -// So the contract is: declaring nothing must mean "not managed", never -// "delete". That is what ManagesCleanupPolicies gates. -func TestManagesCleanupPolicies(t *testing.T) { - RegisterTestingT(t) +// Testing the helpers in isolation is what let the original change ship with +// its own fix untested: deleting the IgnoreChanges branch entirely — reverting +// to the behaviour that silently deleted retention policies — left the suite +// green, because nothing observed the ResourceOption that IS the fix. +// MockResourceArgs.RegisterRPC exposes both the inputs and the ignore list, so +// the property can be asserted where it lives. +type arCapture struct { + inputs resource.PropertyMap + ignoreChanges []string + seen bool +} - Expect((&gcloud.ArtifactRegistryConfig{}).ManagesCleanupPolicies()).To(BeFalse(), - "no declared policies must mean 'SC does not manage retention', so the field is ignored rather than emptied") +func (c *arCapture) NewResource(a sdk.MockResourceArgs) (string, resource.PropertyMap, error) { + if a.TypeToken == "gcp:artifactregistry/repository:Repository" { + c.seen, c.inputs = true, a.Inputs + if a.RegisterRPC != nil { + c.ignoreChanges = a.RegisterRPC.IgnoreChanges + } + } + out := a.Inputs.Mappable() + out["email"] = "sa@test-project.iam.gserviceaccount.com" + out["privateKey"] = "x" + return a.Name + "-id", resource.NewPropertyMapFromMap(out), nil +} - Expect((&gcloud.ArtifactRegistryConfig{ - CleanupPolicies: []gcloud.ArtifactRegistryCleanupPolicy{{Name: "x", Action: "DELETE"}}, - }).ManagesCleanupPolicies()).To(BeTrue()) +func (c *arCapture) Call(sdk.MockCallArgs) (resource.PropertyMap, error) { + return resource.PropertyMap{}, nil +} - // An explicitly empty slice is still "not managed". Treating it as "delete - // everything declared" would make an empty YAML list destructive. - Expect((&gcloud.ArtifactRegistryConfig{ - CleanupPolicies: []gcloud.ArtifactRegistryCleanupPolicy{}, - }).ManagesCleanupPolicies()).To(BeFalse()) +// renderARRepo runs the real provisioning function under mocks and returns what +// reached the Repository resource. err is the provisioning error, if any. +func renderARRepo(cfg *gcloud.ArtifactRegistryConfig) (*arCapture, error) { + setGlobalServicesAPIClient(newMockServicesAPIClient()) + defer resetGlobalServicesAPIClient() + + cfg.Docker = &gcloud.DockerConfig{ImmutableTags: lo.ToPtr(false)} + cfg.Location = "europe-west3" + cfg.ProjectId = "test-project" + + capture := &arCapture{} + err := sdk.RunErr(func(ctx *sdk.Context) error { + prov, err := gcpsdk.NewProvider(ctx, "test-provider", &gcpsdk.ProviderArgs{ + Project: sdk.String("test-project"), + }) + if err != nil { + return err + } + _, err = ArtifactRegistry(ctx, api.Stack{}, api.ResourceInput{ + Descriptor: &api.ResourceDescriptor{ + Name: "registry", + Type: gcloud.ResourceTypeArtifactRegistry, + Config: api.Config{Config: cfg}, + }, + StackParams: &api.StackParams{Environment: "test", StackName: "s"}, + }, pApi.ProvisionParams{Log: logger.New(), Provider: prov}) + return err + }, sdk.WithMocks("test", "test", capture)) + return capture, err } -// Both fields must be ignored together. Ignoring only cleanupPolicies would let -// a provision flip a repository that another tool put in dry-run into -// enforcing, which turns a reporting run into real deletions. -func TestCleanupPolicyFieldsCoverDryRun(t *testing.T) { +// The fix itself: with no retention declared, SC must ask the engine to leave +// the field alone and must not send an empty value, which is what deleted the +// policies. Removing the IgnoreChanges branch fails this test. +func TestUndeclaredRetentionIsIgnoredNotEmptied(t *testing.T) { RegisterTestingT(t) - Expect(cleanupPolicyFields).To(ConsistOf("cleanupPolicies", "cleanupPolicyDryRun")) + c, err := renderARRepo(&gcloud.ArtifactRegistryConfig{}) + Expect(err).To(BeNil()) + Expect(c.seen).To(BeTrue()) + + Expect(c.ignoreChanges).To(ConsistOf("cleanupPolicies", "cleanupPolicyDryRun"), + "undeclared retention must be ignored, including dryRun, or a provision can flip an "+ + "out-of-band dry-run repository into enforcing") + Expect(c.inputs).NotTo(HaveKey(resource.PropertyKey("cleanupPolicies")), + "sending an empty value is what deleted the policies") + Expect(c.inputs).NotTo(HaveKey(resource.PropertyKey("cleanupPolicyDryRun"))) } -func TestCleanupPolicyArgsAcceptsRealPolicies(t *testing.T) { +// The ignore list must not leak onto the IAM policy, service accounts or keys, +// none of which has a cleanupPolicies property. +func TestIgnoreChangesIsScopedToTheRepository(t *testing.T) { RegisterTestingT(t) - got, err := cleanupPolicyArgs([]gcloud.ArtifactRegistryCleanupPolicy{ - { - Name: "delete-untagged-older-30d", - Action: "DELETE", - Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{TagState: "UNTAGGED", OlderThan: "2592000s"}, - }, - { - Name: "keep-most-recent-20", - Action: "KEEP", - MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{KeepCount: lo.ToPtr(20)}, + var others []string + leakCheck := &arLeakCapture{other: &others} + setGlobalServicesAPIClient(newMockServicesAPIClient()) + defer resetGlobalServicesAPIClient() + cfg := &gcloud.ArtifactRegistryConfig{ + Docker: &gcloud.DockerConfig{ImmutableTags: lo.ToPtr(false)}, Location: "europe-west3", + } + cfg.ProjectId = "test-project" + Expect(sdk.RunErr(func(ctx *sdk.Context) error { + prov, err := gcpsdk.NewProvider(ctx, "p", &gcpsdk.ProviderArgs{Project: sdk.String("test-project")}) + if err != nil { + return err + } + _, err = ArtifactRegistry(ctx, api.Stack{}, api.ResourceInput{ + Descriptor: &api.ResourceDescriptor{Name: "registry", Type: gcloud.ResourceTypeArtifactRegistry, Config: api.Config{Config: cfg}}, + StackParams: &api.StackParams{Environment: "test", StackName: "s"}, + }, pApi.ProvisionParams{Log: logger.New(), Provider: prov}) + return err + }, sdk.WithMocks("test", "test", leakCheck))).To(BeNil()) + + Expect(others).To(BeEmpty(), "cleanup ignore list leaked onto non-repository resources: %v", others) +} + +type arLeakCapture struct{ other *[]string } + +func (c *arLeakCapture) NewResource(a sdk.MockResourceArgs) (string, resource.PropertyMap, error) { + if a.TypeToken != "gcp:artifactregistry/repository:Repository" && a.RegisterRPC != nil { + for _, ic := range a.RegisterRPC.IgnoreChanges { + if ic == "cleanupPolicies" || ic == "cleanupPolicyDryRun" { + *c.other = append(*c.other, a.TypeToken+":"+ic) + } + } + } + out := a.Inputs.Mappable() + out["email"] = "sa@test-project.iam.gserviceaccount.com" + out["privateKey"] = "x" + return a.Name + "-id", resource.NewPropertyMapFromMap(out), nil +} +func (c *arLeakCapture) Call(sdk.MockCallArgs) (resource.PropertyMap, error) { + return resource.PropertyMap{}, nil +} + +// Declared retention must reach the resource intact. Every field asserted here +// survives a mutant that drops or hardcodes it. +func TestDeclaredRetentionReachesRepoArgs(t *testing.T) { + RegisterTestingT(t) + + c, err := renderARRepo(&gcloud.ArtifactRegistryConfig{ + CleanupPolicies: &[]gcloud.ArtifactRegistryCleanupPolicy{ + { + Name: "delete-untagged-older-30d", Action: "delete", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{TagState: "untagged", OlderThan: "2592000s"}, + }, + { + Name: "keep-most-recent-20", Action: "KEEP", + MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{KeepCount: lo.ToPtr(20)}, + }, }, + CleanupPolicyDryRun: lo.ToPtr(true), + }) + Expect(err).To(BeNil()) + Expect(c.ignoreChanges).To(BeEmpty(), "declared retention is managed, so nothing may be ignored") + Expect(c.inputs["cleanupPolicyDryRun"].BoolValue()).To(BeTrue()) + + arr := c.inputs["cleanupPolicies"].ArrayValue() + Expect(arr).To(HaveLen(2)) + + p0 := arr[0].ObjectValue() + Expect(p0["id"].StringValue()).To(Equal("delete-untagged-older-30d")) + Expect(p0["action"].StringValue()).To(Equal("DELETE"), "action must be upper-cased for the API") + Expect(p0["condition"].ObjectValue()["tagState"].StringValue()).To(Equal("UNTAGGED")) + Expect(p0["condition"].ObjectValue()["olderThan"].StringValue()).To(Equal("2592000s")) + + p1 := arr[1].ObjectValue() + Expect(p1["action"].StringValue()).To(Equal("KEEP")) + Expect(p1["mostRecentVersions"].ObjectValue()["keepCount"].NumberValue()).To(BeEquivalentTo(20)) +} + +// Unset dryRun must mean dry run. Enforcement destroys image layers no +// provision can restore; dry-run costs storage and is undone by one boolean. +func TestUnsetDryRunDefaultsToDryRun(t *testing.T) { + RegisterTestingT(t) + + c, err := renderARRepo(&gcloud.ArtifactRegistryConfig{ + CleanupPolicies: &[]gcloud.ArtifactRegistryCleanupPolicy{{ + Name: "p", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{TagState: "UNTAGGED", OlderThan: "30d"}, + }}, + }) + Expect(err).To(BeNil()) + Expect(c.inputs["cleanupPolicyDryRun"].BoolValue()).To(BeTrue(), + "omitting cleanupPolicyDryRun must not enforce deletion on the first provision") +} + +func TestEnforcingRequiresAnExplicitFalse(t *testing.T) { + RegisterTestingT(t) + + c, err := renderARRepo(&gcloud.ArtifactRegistryConfig{ + CleanupPolicies: &[]gcloud.ArtifactRegistryCleanupPolicy{{ + Name: "p", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{TagState: "UNTAGGED", OlderThan: "30d"}, + }}, + CleanupPolicyDryRun: lo.ToPtr(false), }) Expect(err).To(BeNil()) - Expect(got).To(HaveLen(2)) + Expect(c.inputs["cleanupPolicyDryRun"].BoolValue()).To(BeFalse()) +} + +// An explicitly empty list is "managed, and I want none" — the only way to +// remove retention through config. +func TestEmptyListIsManagedAndEmpty(t *testing.T) { + RegisterTestingT(t) + + c, err := renderARRepo(&gcloud.ArtifactRegistryConfig{ + CleanupPolicies: &[]gcloud.ArtifactRegistryCleanupPolicy{}, + }) + Expect(err).To(BeNil()) + Expect(c.ignoreChanges).To(BeEmpty(), + "an explicit empty list means SC owns the field; ignoring it would make retention impossible to remove") + Expect(c.inputs).To(HaveKey(resource.PropertyKey("cleanupPolicies"))) + Expect(c.inputs["cleanupPolicies"].ArrayValue()).To(BeEmpty()) +} + +func TestDryRunWithoutPoliciesIsRejected(t *testing.T) { + RegisterTestingT(t) + + _, err := renderARRepo(&gcloud.ArtifactRegistryConfig{CleanupPolicyDryRun: lo.ToPtr(true)}) + Expect(err).NotTo(BeNil(), "config that silently does nothing must fail loudly") + Expect(err.Error()).To(ContainSubstring("cleanupPolicyDryRun is set but cleanupPolicies is not declared")) +} + +func TestArtifactRegistryRejectsInvalidCleanupPolicy(t *testing.T) { + RegisterTestingT(t) + + _, err := renderARRepo(&gcloud.ArtifactRegistryConfig{ + CleanupPolicies: &[]gcloud.ArtifactRegistryCleanupPolicy{{Name: "p", Action: "PURGE"}}, + }) + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(ContainSubstring("invalid cleanup policies for artifact registry"), + "the error must name the registry and environment like every other failure in this function") +} + +func TestManagesCleanupPolicies(t *testing.T) { + RegisterTestingT(t) + + Expect((&gcloud.ArtifactRegistryConfig{}).ManagesCleanupPolicies()).To(BeFalse(), + "absent means not managed, so out-of-band retention is preserved rather than deleted") + Expect((&gcloud.ArtifactRegistryConfig{ + CleanupPolicies: &[]gcloud.ArtifactRegistryCleanupPolicy{}, + }).ManagesCleanupPolicies()).To(BeTrue(), + "an explicit empty list is managed-and-empty, which is how retention is removed") + Expect((&gcloud.ArtifactRegistryConfig{ + CleanupPolicies: &[]gcloud.ArtifactRegistryCleanupPolicy{{Name: "x"}}, + }).ManagesCleanupPolicies()).To(BeTrue()) } func TestCleanupPolicyArgsRejectsBadInput(t *testing.T) { RegisterTestingT(t) + ok := func(p gcloud.ArtifactRegistryCleanupPolicy) []gcloud.ArtifactRegistryCleanupPolicy { + return []gcloud.ArtifactRegistryCleanupPolicy{p} + } for _, tc := range []struct { name string - policy gcloud.ArtifactRegistryCleanupPolicy + in []gcloud.ArtifactRegistryCleanupPolicy wantErr string why string }{ { - name: "unknown action", - policy: gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "PURGE"}, - wantErr: "action must be DELETE or KEEP", - why: "the provider rejects this at apply, by which point the config is already merged", + name: "empty condition matches every version", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{}}), + wantErr: "condition has no criteria", + why: "config decoding is non-strict, so a mistyped key produces exactly this and would delete the whole repository", }, { - name: "mostRecentVersions with DELETE", - policy: gcloud.ArtifactRegistryCleanupPolicy{ - Name: "p", Action: "DELETE", - MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{KeepCount: lo.ToPtr(5)}, - }, - wantErr: "only valid with a KEEP action", - why: "a keep-count under a DELETE action reads as protective while deleting", + name: "tagState alone does not narrow a DELETE", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{TagState: "UNTAGGED"}}), + wantErr: "must narrow by olderThan or a prefix list", + why: "every untagged version, regardless of age, includes ones just pushed", }, { - name: "no condition and no mostRecentVersions", - policy: gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE"}, - wantErr: "needs a condition or mostRecentVersions", - why: "an unconditional DELETE policy matches every version in the repository", + name: "newerThan alone targets running images", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{NewerThan: "7d"}}), + wantErr: "must narrow by olderThan or a prefix list", + why: "deleting the most recently pushed images is the shortest path to CannotPull", }, { - name: "duration without a seconds suffix", - policy: gcloud.ArtifactRegistryCleanupPolicy{ - Name: "p", Action: "DELETE", - Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{OlderThan: "30d"}, - }, - wantErr: "'s' suffix", - why: "gcloud docs use 30d style durations but the API wants seconds; silently wrong retention otherwise", + name: "condition and mostRecentVersions together", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "KEEP", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{OlderThan: "30d"}, + MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{KeepCount: lo.ToPtr(2)}}), + wantErr: "mutually exclusive", + why: "they are a union field; the API rejects it mid-provision", }, { - name: "unknown tagState", - policy: gcloud.ArtifactRegistryCleanupPolicy{ - Name: "p", Action: "DELETE", - Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{TagState: "RELEASED", OlderThan: "1s"}, - }, - wantErr: "tagState must be", - why: "a typo'd tagState defaults to ANY server-side, widening the delete set", + name: "tagPrefixes without TAGGED", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{TagState: "UNTAGGED", TagPrefixes: []string{"v"}}}), + wantErr: "requires tagState TAGGED", + }, + { + name: "empty prefix matches everything", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{TagState: "TAGGED", TagPrefixes: []string{""}}}), + wantErr: "empty prefix", + why: "an unresolved template placeholder collapses to an empty string", + }, + { + name: "keepCount unset protects nothing", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "KEEP", + MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{}}), + wantErr: "keepCount >= 1", + why: "GCP treats a missing count as zero, so the KEEP reads as protective while protecting nothing", + }, + { + name: "keepCount zero", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "KEEP", + MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{KeepCount: lo.ToPtr(0)}}), + wantErr: "keepCount >= 1", + }, + { + name: "olderThan zero matches every version", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{OlderThan: "0s"}}), + wantErr: "greater than zero", + }, + { + name: "garbage duration", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{OlderThan: "30days"}}), + wantErr: "positive duration", + }, + { + name: "negative duration", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{OlderThan: "-5s"}}), + wantErr: "positive duration", + }, + { + name: "newerThan is validated too", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{NewerThan: "abcs", TagPrefixes: []string{"v"}, TagState: "TAGGED"}}), + wantErr: "positive duration", + why: "the original only validated olderThan", + }, + { + name: "unknown action", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "PURGE"}), + wantErr: "action must be DELETE or KEEP", + }, + { + name: "mostRecentVersions under DELETE", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", + MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{KeepCount: lo.ToPtr(5)}}), + wantErr: "only valid with a KEEP action", + }, + { + name: "neither condition nor mostRecentVersions", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE"}), + wantErr: "needs a condition or mostRecentVersions", }, { name: "missing name", - policy: gcloud.ArtifactRegistryCleanupPolicy{Action: "KEEP", MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{KeepCount: lo.ToPtr(1)}}, + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Action: "KEEP", MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{KeepCount: lo.ToPtr(1)}}), wantErr: "missing a name", }, { - name: "negative keepCount", - policy: gcloud.ArtifactRegistryCleanupPolicy{ - Name: "p", Action: "KEEP", - MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{KeepCount: lo.ToPtr(-1)}, + name: "duplicate names", + in: []gcloud.ArtifactRegistryCleanupPolicy{ + {Name: "dup", Action: "DELETE", Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{OlderThan: "30d"}}, + {Name: "dup", Action: "KEEP", MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{KeepCount: lo.ToPtr(1)}}, }, - wantErr: "cannot be negative", + wantErr: "duplicate cleanup policy name", + why: "Artifact Registry keys policies by name, so a duplicate silently drops one", }, } { t.Run(tc.name, func(t *testing.T) { RegisterTestingT(t) - _, err := cleanupPolicyArgs([]gcloud.ArtifactRegistryCleanupPolicy{tc.policy}) + _, err := cleanupPolicyArgs(tc.in) Expect(err).NotTo(BeNil(), tc.why) Expect(err.Error()).To(ContainSubstring(tc.wantErr)) }) } } -func TestCleanupPolicyArgsRejectsDuplicateNames(t *testing.T) { +func TestCleanupPolicyArgsRejectsTooManyPolicies(t *testing.T) { RegisterTestingT(t) - // Artifact Registry keys policies by name, so a duplicate silently drops one - // of them and the repository ends up with retention nobody reviewed. - _, err := cleanupPolicyArgs([]gcloud.ArtifactRegistryCleanupPolicy{ - {Name: "dup", Action: "DELETE", Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{OlderThan: "1s"}}, - {Name: "dup", Action: "KEEP", MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{KeepCount: lo.ToPtr(1)}}, - }) + many := make([]gcloud.ArtifactRegistryCleanupPolicy, 0, maxCleanupPolicies+1) + for i := 0; i <= maxCleanupPolicies; i++ { + many = append(many, gcloud.ArtifactRegistryCleanupPolicy{ + Name: string(rune('a' + i)), Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{OlderThan: "30d"}, + }) + } + _, err := cleanupPolicyArgs(many) Expect(err).NotTo(BeNil()) - Expect(err.Error()).To(ContainSubstring("duplicate cleanup policy name")) + Expect(err.Error()).To(ContainSubstring("at most 10 cleanup policies")) } -// Case-insensitivity matters because the committed JSON in at least one fleet -// uses "Delete"/"Keep" while the API enum is upper-case. -func TestCleanupPolicyArgsNormalisesCase(t *testing.T) { +// The provider's own acceptance tests use the day form, while the REST API +// reports seconds. Rejecting either would reject valid configuration. +func TestCleanupDurationAcceptsBothProviderForms(t *testing.T) { RegisterTestingT(t) - got, err := cleanupPolicyArgs([]gcloud.ArtifactRegistryCleanupPolicy{{ - Name: "p", Action: "Delete", - Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{TagState: "untagged", OlderThan: "1s"}, - }}) - Expect(err).To(BeNil()) - Expect(got).To(HaveLen(1)) + for _, d := range []string{"30d", "7d", "2592000s", "720h", "45m", "1.5h"} { + Expect(validateCleanupDuration("p", "olderThan", d)).To(BeNil(), "%q must be accepted", d) + } + for _, d := range []string{"30days", "abcs", "s", "-5s", "0s", "0d", "30 s", ""} { + Expect(validateCleanupDuration("p", "olderThan", d)).NotTo(BeNil(), "%q must be rejected", d) + } } From fad04c9fccac555c041e492bc070722daef39417 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Wed, 19 Aug 2026 22:53:40 +0400 Subject: [PATCH 2/3] fix(deps): bump go directive to 1.26.6 to clear reachable stdlib advisories 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 --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 02687d27..25210066 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/simple-container-com/api -go 1.26.5 +go 1.26.6 require ( cloud.google.com/go/storage v1.62.3 From 05a08f8eb115b677d3e0441beae70de8c18425ce Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Thu, 20 Aug 2026 09:45:29 +0400 Subject: [PATCH 3/3] fix(gcp): narrowing rule was backwards, and cover the paths this PR added 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 --- docs/docs/reference/supported-resources.md | 10 +- pkg/clouds/gcloud/resources_test.go | 24 +++- pkg/clouds/pulumi/gcp/artifactregistry.go | 87 +++++++++---- .../gcp/artifactregistry_cleanup_test.go | 114 +++++++++++++++--- 4 files changed, 182 insertions(+), 53 deletions(-) diff --git a/docs/docs/reference/supported-resources.md b/docs/docs/reference/supported-resources.md index 4341ed35..424940fe 100644 --- a/docs/docs/reference/supported-resources.md +++ b/docs/docs/reference/supported-resources.md @@ -867,7 +867,7 @@ Artifact Registry keeps every image version forever unless a cleanup policy says action: DELETE condition: tagState: UNTAGGED - olderThan: 30d # or "2592000s"; both are accepted + olderThan: 30d # or "2592000s"; both accepted, integers only - name: keep-most-recent-20 action: KEEP # KEEP wins over a matching DELETE mostRecentVersions: @@ -879,11 +879,15 @@ Three behaviours are worth knowing before you use this. **Omitting `cleanupPolicies` means Simple Container does not manage retention.** Any policy set outside SC — through `gcloud` or the console — is left alone. This is the default and it is deliberate: the field is authoritative in the provider, so a resource that declares nothing would otherwise *delete* whatever is configured. -**Declaring it makes Simple Container authoritative.** Policies set outside SC are then replaced by the declared list on the next provision. An explicitly empty list (`cleanupPolicies: []`) means "managed, and I want none", which is how retention is removed; deleting the block entirely returns the repository to unmanaged and leaves the live policies in place. +**Declaring it makes Simple Container authoritative.** Policies set outside SC are then replaced by the declared list on the next provision. An explicitly empty list (`cleanupPolicies: []`) means "managed, and I want none", which is how retention is removed. + +Note two asymmetries. Writing the key with no value (`cleanupPolicies:` alone) decodes as *absent*, i.e. unmanaged — visually almost identical to `[]`, which removes every policy. And once SC has managed the field, **deleting the block does not return the repository to out-of-band control**: Pulumi's `ignoreChanges` carries the previous value forward from state, so retention freezes at the last declared list and later console edits are reverted on each provision. Genuinely handing the field back requires removing the property from stack state. **`cleanupPolicyDryRun` defaults to `true`.** Nothing is deleted until it is explicitly set to `false`. Dry run evaluates the policies and reports what they would remove, so run it first and read the result: deleting an image that is still deployed makes the next node reschedule fail to pull, and no provision can restore a deleted layer. -Configuration is validated before provisioning, and a policy is rejected if it would match far more than it appears to — an empty `condition`, a `DELETE` narrowed only by `tagState` or `newerThan`, a `KEEP` with no `keepCount`, an empty prefix, or a non-positive duration. Note that an unrecognised key in `server.yaml` is silently ignored rather than rejected, so a mistyped condition field would otherwise produce a policy matching every version. +Policies are validated while the Pulumi program is evaluated, and one is rejected if it would match far more than it appears to: an empty `condition`, a `DELETE` that does not set `olderThan` or target `tagState: UNTAGGED`, a `KEEP` with no `keepCount`, an empty prefix, or a non-positive duration. Prefixes are deliberately **not** accepted as narrowing a `DELETE` — they select which packages a policy covers, not which ages, so `packageNamePrefixes` alone would delete the running version of that package. + +An unrecognised key in `server.yaml` is silently ignored rather than rejected, so a mistyped condition field would otherwise produce a policy matching every version, which is what these checks are for. ### **Database Resources** diff --git a/pkg/clouds/gcloud/resources_test.go b/pkg/clouds/gcloud/resources_test.go index 4511244b..23a6e665 100644 --- a/pkg/clouds/gcloud/resources_test.go +++ b/pkg/clouds/gcloud/resources_test.go @@ -489,14 +489,19 @@ func TestArtifactRegistryConfigReadsCleanupPoliciesFromYAML(t *testing.T) { "location": "europe-west3", "cleanupPolicies": []any{ map[string]any{ - "name": "delete-untagged-older-30d", - "action": "DELETE", - "condition": map[string]any{"tagState": "UNTAGGED", "olderThan": "2592000s"}, + "name": "delete-old-feature-tags", + "action": "DELETE", + "condition": map[string]any{ + "tagState": "TAGGED", "olderThan": "2592000s", "newerThan": "3600s", + "tagPrefixes": []any{"feature-"}, + "packageNamePrefixes": []any{"svc/"}, + "versionNamePrefixes": []any{"sha256:"}, + }, }, map[string]any{ "name": "keep-most-recent-20", "action": "KEEP", - "mostRecentVersions": map[string]any{"keepCount": 20}, + "mostRecentVersions": map[string]any{"keepCount": 20, "packageNamePrefixes": []any{"api/"}}, }, }, "cleanupPolicyDryRun": true, @@ -509,13 +514,20 @@ func TestArtifactRegistryConfigReadsCleanupPoliciesFromYAML(t *testing.T) { Expect(ar.ManagesCleanupPolicies()).To(BeTrue()) got := ar.DeclaredCleanupPolicies() Expect(got).To(HaveLen(2)) - Expect(got[0].Name).To(Equal("delete-untagged-older-30d")) + Expect(got[0].Name).To(Equal("delete-old-feature-tags")) Expect(got[0].Action).To(Equal("DELETE")) Expect(got[0].Condition).NotTo(BeNil()) - Expect(got[0].Condition.TagState).To(Equal("UNTAGGED")) + // Every key, because a dead struct tag on any prefix field silently drops + // the narrowing and widens the DELETE to the whole repository, with no error. + Expect(got[0].Condition.TagState).To(Equal("TAGGED")) Expect(got[0].Condition.OlderThan).To(Equal("2592000s")) + Expect(got[0].Condition.NewerThan).To(Equal("3600s")) + Expect(got[0].Condition.TagPrefixes).To(ConsistOf("feature-")) + Expect(got[0].Condition.PackageNamePrefixes).To(ConsistOf("svc/")) + Expect(got[0].Condition.VersionNamePrefixes).To(ConsistOf("sha256:")) Expect(got[1].MostRecentVersions).NotTo(BeNil()) Expect(*got[1].MostRecentVersions.KeepCount).To(Equal(20)) + Expect(got[1].MostRecentVersions.PackageNamePrefixes).To(ConsistOf("api/")) Expect(ar.CleanupPolicyDryRun).NotTo(BeNil()) Expect(*ar.CleanupPolicyDryRun).To(BeTrue()) } diff --git a/pkg/clouds/pulumi/gcp/artifactregistry.go b/pkg/clouds/pulumi/gcp/artifactregistry.go index bf6025bc..a3035c93 100644 --- a/pkg/clouds/pulumi/gcp/artifactregistry.go +++ b/pkg/clouds/pulumi/gcp/artifactregistry.go @@ -108,7 +108,14 @@ func ArtifactRegistry(ctx *sdk.Context, stack api.Stack, input api.ResourceInput } repoArgs.CleanupPolicies = policies repoArgs.CleanupPolicyDryRun = sdk.Bool(dryRun) - if dryRun { + if len(declared) == 0 { + // dryRun gates deletion of VERSIONS by policies, not removal of the + // policies themselves, so it offers no protection on this path. + params.Log.Warn(ctx.Context(), "artifact registry %q: cleanupPolicies is declared empty, so ALL retention "+ + "policies on this repository will be removed, including any set outside SC; "+ + "remove the cleanupPolicies block instead if you meant to leave retention alone", + artifactRegistryName) + } else if dryRun { params.Log.Info(ctx.Context(), "artifact registry %q: SC manages %d cleanup policies in DRY RUN; "+ "policies set outside SC will be replaced, nothing is deleted until cleanupPolicyDryRun is false", artifactRegistryName, len(declared)) @@ -311,11 +318,18 @@ var cleanupPolicyFields = []string{"cleanupPolicies", "cleanupPolicyDryRun"} // apply, after other resources in the stack have already been mutated. const maxCleanupPolicies = 10 +// maxCleanupPolicyNameLen mirrors the provider's documented limit on the policy +// id; over it the apply fails. +const maxCleanupPolicyNameLen = 128 + // cleanupDurationRe accepts the duration forms the provider accepts. Its own // acceptance tests use the day form ("30d", "7d"), while the REST API reports // seconds ("2592000s"); DurationDiffSuppress treats them as equivalent, so // rejecting either would reject valid configuration. -var cleanupDurationRe = regexp.MustCompile(`^([0-9]+(?:\.[0-9]+)?)(s|m|h|d)$`) +// The quantity must be an INTEGER: the provider expands the m/h/d forms with +// strconv.Atoi before converting to seconds, so a fractional value such as +// "1.5h" passes any regex that allows it and then fails at registration. +var cleanupDurationRe = regexp.MustCompile(`^([0-9]+)(s|m|h|d)$`) // validateCleanupDuration rejects shapes the provider would reject at apply, // and zero, which is not a syntax error but a semantic one: "olderThan: 0s" on @@ -326,16 +340,24 @@ func validateCleanupDuration(policy, field, v string) error { return errors.Errorf("cleanup policy %q: %s must be a positive duration such as %q or %q, got %q", policy, field, "30d", "2592000s", v) } - if n, err := strconv.ParseFloat(m[1], 64); err != nil || n <= 0 { + // m[1] is [0-9]+ by construction, so only the range error is reachable. + n, err := strconv.Atoi(m[1]) + if err != nil { + return errors.Errorf("cleanup policy %q: %s is out of range, got %q", policy, field, v) + } + if n <= 0 { return errors.Errorf("cleanup policy %q: %s must be greater than zero, got %q", policy, field, v) } return nil } -// hasEmpty reports whether a prefix list contains an empty entry. An empty -// prefix matches everything, and it is the shape an unresolved template -// placeholder collapses to, so it is reachable without anyone typing "". -func hasEmpty(vals []string) bool { +// hasEmptyPrefix reports whether a prefix list contains an empty entry, which +// matches everything. +// +// Reachable without anyone typing "": an ${env:VAR} whose variable is unset +// resolves to the empty string. (An UNRESOLVED placeholder stays literal +// "${...}" instead, so that is not the path.) +func hasEmptyPrefix(vals []string) bool { return lo.Contains(vals, "") } @@ -353,9 +375,12 @@ func cleanupPolicyArgs(policies []gcloud.ArtifactRegistryCleanupPolicy) (artifac } out := make(artifactregistry.RepositoryCleanupPolicyArray, 0, len(policies)) seen := make(map[string]bool, len(policies)) - for _, p := range policies { + for i, p := range policies { if p.Name == "" { - return nil, errors.Errorf("cleanup policy is missing a name") + return nil, errors.Errorf("cleanup policy #%d is missing a name", i+1) + } + if len(p.Name) >= maxCleanupPolicyNameLen { + return nil, errors.Errorf("cleanup policy %q: name must be under %d characters", p.Name, maxCleanupPolicyNameLen) } if seen[p.Name] { return nil, errors.Errorf("duplicate cleanup policy name %q", p.Name) @@ -411,24 +436,38 @@ func cleanupPolicyArgs(policies []gcloud.ArtifactRegistryCleanupPolicy) (artifac // Tag prefixes only mean anything against tagged versions; the API // rejects the combination rather than ignoring it. if len(c.TagPrefixes) > 0 && tagState != "TAGGED" { - return nil, errors.Errorf("cleanup policy %q: tagPrefixes requires tagState TAGGED, got %q", p.Name, c.TagState) + got := c.TagState + if got == "" { + got = `"" (defaults to ANY)` + } else { + got = strconv.Quote(got) + } + return nil, errors.Errorf("cleanup policy %q: tagPrefixes requires tagState TAGGED, got %s", p.Name, got) } - for field, vals := range map[string][]string{ - "tagPrefixes": c.TagPrefixes, - "packageNamePrefixes": c.PackageNamePrefixes, - "versionNamePrefixes": c.VersionNamePrefixes, + for _, pl := range []struct { + field string + vals []string + }{ + {"condition.tagPrefixes", c.TagPrefixes}, + {"condition.packageNamePrefixes", c.PackageNamePrefixes}, + {"condition.versionNamePrefixes", c.VersionNamePrefixes}, } { - if hasEmpty(vals) { - return nil, errors.Errorf("cleanup policy %q: %s contains an empty prefix, which matches everything", p.Name, field) + if hasEmptyPrefix(pl.vals) { + return nil, errors.Errorf("cleanup policy %q: %s contains an empty prefix, which matches everything", p.Name, pl.field) } } - narrowed := c.OlderThan != "" || len(c.TagPrefixes)+len(c.PackageNamePrefixes)+len(c.VersionNamePrefixes) > 0 + // Only AGE, or restricting to untagged versions, separates "old" from + // "still running". Prefixes select which PACKAGES a policy covers, + // not which ages, so `DELETE` + packageNamePrefixes deletes every + // version of that package including the deployed digest, and for a + // Docker repository versionNamePrefixes is the digest itself. + // An earlier revision of this guard accepted prefixes as narrowing + // and named them in the error, which steered anyone blocked on the + // safe shape towards the destructive one. + narrowed := c.OlderThan != "" || tagState == "UNTAGGED" if action == "DELETE" && !narrowed { - // Left here, the policy is either "everything" (tagState only) - // or "everything pushed recently" (newerThan only), and the - // latter is precisely the images currently running. - return nil, errors.Errorf("cleanup policy %q: a DELETE condition must narrow by olderThan or a prefix list; "+ - "tagState or newerThan alone targets versions that are still deployed", p.Name) + return nil, errors.Errorf("cleanup policy %q: a DELETE condition must set olderThan, or target tagState UNTAGGED; "+ + "prefixes select which packages are covered, not which ages, so a prefix alone deletes the running version", p.Name) } cond := &artifactregistry.RepositoryCleanupPolicyConditionArgs{} if len(c.TagPrefixes) > 0 { @@ -458,8 +497,8 @@ func cleanupPolicyArgs(policies []gcloud.ArtifactRegistryCleanupPolicy) (artifac if m.KeepCount == nil || *m.KeepCount < 1 { return nil, errors.Errorf("cleanup policy %q: mostRecentVersions requires keepCount >= 1", p.Name) } - if hasEmpty(m.PackageNamePrefixes) { - return nil, errors.Errorf("cleanup policy %q: packageNamePrefixes contains an empty prefix, which matches everything", p.Name) + if hasEmptyPrefix(m.PackageNamePrefixes) { + return nil, errors.Errorf("cleanup policy %q: mostRecentVersions.packageNamePrefixes contains an empty prefix, which matches everything", p.Name) } mrv := &artifactregistry.RepositoryCleanupPolicyMostRecentVersionsArgs{ KeepCount: sdk.IntPtr(*m.KeepCount), diff --git a/pkg/clouds/pulumi/gcp/artifactregistry_cleanup_test.go b/pkg/clouds/pulumi/gcp/artifactregistry_cleanup_test.go index edc942c9..4f2e0172 100644 --- a/pkg/clouds/pulumi/gcp/artifactregistry_cleanup_test.go +++ b/pkg/clouds/pulumi/gcp/artifactregistry_cleanup_test.go @@ -4,6 +4,7 @@ package gcp import ( + "strings" "testing" . "github.com/onsi/gomega" @@ -162,6 +163,21 @@ func TestDeclaredRetentionReachesRepoArgs(t *testing.T) { Name: "keep-most-recent-20", Action: "KEEP", MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{KeepCount: lo.ToPtr(20)}, }, + { + Name: "delete-old-feature-tags", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{ + TagState: "TAGGED", OlderThan: "30d", NewerThan: "1s", + TagPrefixes: []string{"feature-"}, + PackageNamePrefixes: []string{"svc/"}, + VersionNamePrefixes: []string{"sha256:"}, + }, + }, + { + Name: "keep-api-20", Action: "KEEP", + MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{ + KeepCount: lo.ToPtr(20), PackageNamePrefixes: []string{"api/"}, + }, + }, }, CleanupPolicyDryRun: lo.ToPtr(true), }) @@ -170,17 +186,36 @@ func TestDeclaredRetentionReachesRepoArgs(t *testing.T) { Expect(c.inputs["cleanupPolicyDryRun"].BoolValue()).To(BeTrue()) arr := c.inputs["cleanupPolicies"].ArrayValue() - Expect(arr).To(HaveLen(2)) + Expect(arr).To(HaveLen(4)) p0 := arr[0].ObjectValue() Expect(p0["id"].StringValue()).To(Equal("delete-untagged-older-30d")) Expect(p0["action"].StringValue()).To(Equal("DELETE"), "action must be upper-cased for the API") - Expect(p0["condition"].ObjectValue()["tagState"].StringValue()).To(Equal("UNTAGGED")) - Expect(p0["condition"].ObjectValue()["olderThan"].StringValue()).To(Equal("2592000s")) + c0 := p0["condition"].ObjectValue() + Expect(c0["tagState"].StringValue()).To(Equal("UNTAGGED")) + Expect(c0["olderThan"].StringValue()).To(Equal("2592000s")) + // The absence side is the whole reason the builder uses len()>0 guards: + // sending an empty list is not the same as not constraining on it. + for _, k := range []resource.PropertyKey{"tagPrefixes", "packageNamePrefixes", "versionNamePrefixes", "newerThan"} { + Expect(c0).NotTo(HaveKey(k), "undeclared condition field %q must not be sent", k) + } p1 := arr[1].ObjectValue() Expect(p1["action"].StringValue()).To(Equal("KEEP")) Expect(p1["mostRecentVersions"].ObjectValue()["keepCount"].NumberValue()).To(BeEquivalentTo(20)) + Expect(p1["mostRecentVersions"].ObjectValue()).NotTo(HaveKey(resource.PropertyKey("packageNamePrefixes"))) + + // Every prefix and newerThan path must survive the trip. Losing a prefix + // silently widens a DELETE from one package to the whole repository. + c2 := arr[2].ObjectValue()["condition"].ObjectValue() + Expect(c2["tagPrefixes"].ArrayValue()[0].StringValue()).To(Equal("feature-")) + Expect(c2["packageNamePrefixes"].ArrayValue()[0].StringValue()).To(Equal("svc/")) + Expect(c2["versionNamePrefixes"].ArrayValue()[0].StringValue()).To(Equal("sha256:")) + Expect(c2["newerThan"].StringValue()).To(Equal("1s")) + Expect(c2["olderThan"].StringValue()).To(Equal("30d")) + + mrv3 := arr[3].ObjectValue()["mostRecentVersions"].ObjectValue() + Expect(mrv3["packageNamePrefixes"].ArrayValue()[0].StringValue()).To(Equal("api/")) } // Unset dryRun must mean dry run. Enforcement destroys image layers no @@ -233,7 +268,8 @@ func TestDryRunWithoutPoliciesIsRejected(t *testing.T) { _, err := renderARRepo(&gcloud.ArtifactRegistryConfig{CleanupPolicyDryRun: lo.ToPtr(true)}) Expect(err).NotTo(BeNil(), "config that silently does nothing must fail loudly") - Expect(err.Error()).To(ContainSubstring("cleanupPolicyDryRun is set but cleanupPolicies is not declared")) + Expect(err.Error()).To(ContainSubstring(`artifact registry "registry--test" in "test": cleanupPolicyDryRun is set`), + "the error must identify which registry and environment") } func TestArtifactRegistryRejectsInvalidCleanupPolicy(t *testing.T) { @@ -243,7 +279,7 @@ func TestArtifactRegistryRejectsInvalidCleanupPolicy(t *testing.T) { CleanupPolicies: &[]gcloud.ArtifactRegistryCleanupPolicy{{Name: "p", Action: "PURGE"}}, }) Expect(err).NotTo(BeNil()) - Expect(err.Error()).To(ContainSubstring("invalid cleanup policies for artifact registry"), + Expect(err.Error()).To(ContainSubstring(`invalid cleanup policies for artifact registry "registry--test" in "test"`), "the error must name the registry and environment like every other failure in this function") } @@ -279,18 +315,11 @@ func TestCleanupPolicyArgsRejectsBadInput(t *testing.T) { wantErr: "condition has no criteria", why: "config decoding is non-strict, so a mistyped key produces exactly this and would delete the whole repository", }, - { - name: "tagState alone does not narrow a DELETE", - in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", - Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{TagState: "UNTAGGED"}}), - wantErr: "must narrow by olderThan or a prefix list", - why: "every untagged version, regardless of age, includes ones just pushed", - }, { name: "newerThan alone targets running images", in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{NewerThan: "7d"}}), - wantErr: "must narrow by olderThan or a prefix list", + wantErr: "must set olderThan, or target tagState UNTAGGED", why: "deleting the most recently pushed images is the shortest path to CannotPull", }, { @@ -350,7 +379,42 @@ func TestCleanupPolicyArgsRejectsBadInput(t *testing.T) { in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{NewerThan: "abcs", TagPrefixes: []string{"v"}, TagState: "TAGGED"}}), wantErr: "positive duration", - why: "the original only validated olderThan", + why: "newerThan must be validated too, not just olderThan", + }, + { + name: "unknown tagState", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{TagState: "RELEASED", OlderThan: "30d"}}), + wantErr: "tagState must be", + why: "a typo'd tagState defaults to ANY server-side, widening the delete set", + }, + { + name: "keep with an empty package prefix", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "KEEP", + MostRecentVersions: &gcloud.ArtifactRegistryCleanupMostRecentVersions{ + KeepCount: lo.ToPtr(1), PackageNamePrefixes: []string{""}}}), + wantErr: "mostRecentVersions.packageNamePrefixes contains an empty prefix", + why: "a KEEP matching every package silently changes what the companion DELETE spares", + }, + { + name: "prefix alone does not narrow a DELETE by age", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{PackageNamePrefixes: []string{"api"}}}), + wantErr: "must set olderThan, or target tagState UNTAGGED", + why: "this deletes every version of that package including the deployed digest", + }, + { + name: "digest prefix alone covers the whole docker repository", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: "p", Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{VersionNamePrefixes: []string{"sha256:"}}}), + wantErr: "must set olderThan, or target tagState UNTAGGED", + why: "for a Docker repo the version name IS the digest", + }, + { + name: "policy name over the provider limit", + in: ok(gcloud.ArtifactRegistryCleanupPolicy{Name: strings.Repeat("x", 128), Action: "DELETE", + Condition: &gcloud.ArtifactRegistryCleanupPolicyCondition{OlderThan: "30d"}}), + wantErr: "must be under 128 characters", }, { name: "unknown action", @@ -407,15 +471,25 @@ func TestCleanupPolicyArgsRejectsTooManyPolicies(t *testing.T) { Expect(err.Error()).To(ContainSubstring("at most 10 cleanup policies")) } -// The provider's own acceptance tests use the day form, while the REST API -// reports seconds. Rejecting either would reject valid configuration. -func TestCleanupDurationAcceptsBothProviderForms(t *testing.T) { +// Accepted forms are pinned to evidence, not to the regex under test. +// +// "30d"/"7d" appear verbatim in terraform-provider-google's generated +// acceptance test for google_artifact_registry_repository (older_than = "30d", +// newer_than = "7d"), and the REST API reports the same field in seconds. So +// both must be accepted. The quantity must be an integer, because the provider +// expands the m/h/d forms with strconv.Atoi: "1.5h" would pass validation here +// and then fail at resource registration. +func TestCleanupDurationAcceptsTheFormsTheProviderAccepts(t *testing.T) { RegisterTestingT(t) - for _, d := range []string{"30d", "7d", "2592000s", "720h", "45m", "1.5h"} { - Expect(validateCleanupDuration("p", "olderThan", d)).To(BeNil(), "%q must be accepted", d) + for _, d := range []string{"30d", "7d", "2592000s", "720h", "45m", "1s"} { + Expect(validateCleanupDuration("p", "olderThan", d)).To(BeNil(), "%q is used by the provider and must be accepted", d) } - for _, d := range []string{"30days", "abcs", "s", "-5s", "0s", "0d", "30 s", ""} { + for _, d := range []string{ + "1.5h", "0.5d", // fractional: the provider expands with strconv.Atoi + "30days", "abcs", "s", "-5s", "30 s", "", // malformed + "0s", "0d", // zero matches every version + } { Expect(validateCleanupDuration("p", "olderThan", d)).NotTo(BeNil(), "%q must be rejected", d) } }