From eecaa34a693138b38c44dee02f6a00cf1b2d5cc9 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Thu, 13 Aug 2026 18:53:21 +0400 Subject: [PATCH 1/2] fix(gcp): stop clearing Artifact Registry cleanup policies SC does not 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 --- docs/schemas/gcp/artifactregistryconfig.json | 72 ++++++++ pkg/clouds/gcloud/artifactregistry.go | 49 ++++++ pkg/clouds/pulumi/gcp/artifactregistry.go | 97 ++++++++++ .../gcp/artifactregistry_cleanup_test.go | 165 ++++++++++++++++++ 4 files changed, 383 insertions(+) create mode 100644 pkg/clouds/pulumi/gcp/artifactregistry_cleanup_test.go diff --git a/docs/schemas/gcp/artifactregistryconfig.json b/docs/schemas/gcp/artifactregistryconfig.json index 87b95e1c..da00ddcd 100644 --- a/docs/schemas/gcp/artifactregistryconfig.json +++ b/docs/schemas/gcp/artifactregistryconfig.json @@ -47,6 +47,78 @@ ], "type": "object" }, + "cleanupPolicies": { + "items": { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "properties": { + "action": { + "type": "string" + }, + "condition": { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "properties": { + "newerThan": { + "type": "string" + }, + "olderThan": { + "type": "string" + }, + "packageNamePrefixes": { + "items": { + "type": "string" + }, + "type": "array" + }, + "tagPrefixes": { + "items": { + "type": "string" + }, + "type": "array" + }, + "tagState": { + "type": "string" + }, + "versionNamePrefixes": { + "items": { + "type": "string" + }, + "type": "array" + } + }, + "required": [], + "type": "object" + }, + "mostRecentVersions": { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "properties": { + "keepCount": { + "type": "integer" + }, + "packageNamePrefixes": { + "items": { + "type": "string" + }, + "type": "array" + } + }, + "required": [], + "type": "object" + }, + "name": { + "type": "string" + } + }, + "required": [ + "action", + "name" + ], + "type": "object" + }, + "type": "array" + }, + "cleanupPolicyDryRun": { + "type": "boolean" + }, "docker": { "$schema": "https://json-schema.org/draft/2020-12/schema", "properties": { diff --git a/pkg/clouds/gcloud/artifactregistry.go b/pkg/clouds/gcloud/artifactregistry.go index 57bbc4c2..ca9395a9 100644 --- a/pkg/clouds/gcloud/artifactregistry.go +++ b/pkg/clouds/gcloud/artifactregistry.go @@ -17,6 +17,55 @@ type ArtifactRegistryConfig struct { Docker *DockerConfig `json:"docker,omitempty" yaml:"docker,omitempty"` Domain *string `json:"domain" yaml:"domain"` BasicAuth *RegistryBasicAuth `json:"basicAuth,omitempty" yaml:"basicAuth,omitempty"` + + // CleanupPolicies declares image retention for the repository. + // + // The provider treats cleanupPolicies as authoritative (unlike labels, whose + // schema documents itself as non-authoritative), so a repository resource + // that does not declare it will CLEAR any policy set by another tool on the + // 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"` + + // CleanupPolicyDryRun evaluates the policies and reports what they would + // delete without deleting it. Only meaningful alongside CleanupPolicies. + CleanupPolicyDryRun *bool `json:"cleanupPolicyDryRun,omitempty" yaml:"cleanupPolicyDryRun,omitempty"` +} + +// ManagesCleanupPolicies reports whether retention is declared here. When it is +// 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 +} + +// ArtifactRegistryCleanupPolicy mirrors a single Artifact Registry cleanup +// policy. Durations are the API's second-suffixed form, e.g. "2592000s". +type ArtifactRegistryCleanupPolicy struct { + // Name identifies the policy within the repository. + Name string `json:"name" yaml:"name"` + // Action is DELETE or KEEP (case-insensitive). + Action string `json:"action" yaml:"action"` + Condition *ArtifactRegistryCleanupPolicyCondition `json:"condition,omitempty" yaml:"condition,omitempty"` + // MostRecentVersions retains a minimum number of versions. KEEP only. + MostRecentVersions *ArtifactRegistryCleanupMostRecentVersions `json:"mostRecentVersions,omitempty" yaml:"mostRecentVersions,omitempty"` +} + +type ArtifactRegistryCleanupPolicyCondition struct { + // TagState is TAGGED, UNTAGGED or ANY. + TagState string `json:"tagState,omitempty" yaml:"tagState,omitempty"` + // OlderThan / NewerThan are durations, e.g. "2592000s" for 30 days. + OlderThan string `json:"olderThan,omitempty" yaml:"olderThan,omitempty"` + NewerThan string `json:"newerThan,omitempty" yaml:"newerThan,omitempty"` + TagPrefixes []string `json:"tagPrefixes,omitempty" yaml:"tagPrefixes,omitempty"` + PackageNamePrefixes []string `json:"packageNamePrefixes,omitempty" yaml:"packageNamePrefixes,omitempty"` + VersionNamePrefixes []string `json:"versionNamePrefixes,omitempty" yaml:"versionNamePrefixes,omitempty"` +} + +type ArtifactRegistryCleanupMostRecentVersions struct { + KeepCount *int `json:"keepCount,omitempty" yaml:"keepCount,omitempty"` + PackageNamePrefixes []string `json:"packageNamePrefixes,omitempty" yaml:"packageNamePrefixes,omitempty"` } type RegistryBasicAuth struct { diff --git a/pkg/clouds/pulumi/gcp/artifactregistry.go b/pkg/clouds/pulumi/gcp/artifactregistry.go index 5e250e8f..4c72e981 100644 --- a/pkg/clouds/pulumi/gcp/artifactregistry.go +++ b/pkg/clouds/pulumi/gcp/artifactregistry.go @@ -79,6 +79,21 @@ func ArtifactRegistry(ctx *sdk.Context, stack api.Stack, input api.ResourceInput return nil, errors.Errorf("registry format is not supported") } + // cleanupPolicies is authoritative in the provider: a Repository resource + // 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. + if arCfg.ManagesCleanupPolicies() { + policies, err := cleanupPolicyArgs(arCfg.CleanupPolicies) + if err != nil { + return nil, err + } + repoArgs.CleanupPolicies = policies + repoArgs.CleanupPolicyDryRun = sdk.Bool(lo.FromPtr(arCfg.CleanupPolicyDryRun)) + } else { + opts = append(opts, sdk.IgnoreChanges(cleanupPolicyFields)) + } + params.Log.Info(ctx.Context(), "configure artifact registry repository %q", artifactRegistryName) repo, err := artifactregistry.NewRepository(ctx, artifactRegistryName, &repoArgs, opts...) if err != nil { @@ -252,3 +267,85 @@ func toRegistryServiceAccountKeyExport(input api.ResourceInput, saType string, r func toRegistryServiceAccountEmailExport(input api.ResourceInput, saType string, registryName string) string { return input.ToResName(fmt.Sprintf("%s-%s-sa", saType, registryName)) } + +// cleanupPolicyFields are the property paths SC declines to manage when no +// retention is configured. Both are needed: leaving cleanupPolicyDryRun out +// would let a provision flip an out-of-band dry-run repository into enforcing. +var cleanupPolicyFields = []string{"cleanupPolicies", "cleanupPolicyDryRun"} + +// 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. +func cleanupPolicyArgs(policies []gcloud.ArtifactRegistryCleanupPolicy) (artifactregistry.RepositoryCleanupPolicyArray, error) { + out := make(artifactregistry.RepositoryCleanupPolicyArray, 0, len(policies)) + seen := make(map[string]bool, len(policies)) + for _, p := range policies { + if p.Name == "" { + return nil, errors.Errorf("cleanup policy is missing a name") + } + if seen[p.Name] { + return nil, errors.Errorf("duplicate cleanup policy name %q", p.Name) + } + seen[p.Name] = true + + action := strings.ToUpper(p.Action) + if action != "DELETE" && action != "KEEP" { + return nil, errors.Errorf("cleanup policy %q: action must be DELETE or KEEP, got %q", p.Name, p.Action) + } + if p.MostRecentVersions != nil && action != "KEEP" { + return nil, errors.Errorf("cleanup policy %q: mostRecentVersions is only valid with a KEEP action", p.Name) + } + if p.Condition == nil && p.MostRecentVersions == nil { + return nil, errors.Errorf("cleanup policy %q: needs a condition or mostRecentVersions", p.Name) + } + + args := &artifactregistry.RepositoryCleanupPolicyArgs{ + Id: sdk.String(p.Name), + Action: sdk.String(action), + } + if c := p.Condition; c != nil { + tagState := strings.ToUpper(c.TagState) + switch tagState { + case "", "TAGGED", "UNTAGGED", "ANY": + 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) + } + } + cond := &artifactregistry.RepositoryCleanupPolicyConditionArgs{ + TagPrefixes: sdk.ToStringArray(c.TagPrefixes), + PackageNamePrefixes: sdk.ToStringArray(c.PackageNamePrefixes), + VersionNamePrefixes: sdk.ToStringArray(c.VersionNamePrefixes), + } + if tagState != "" { + cond.TagState = sdk.StringPtr(tagState) + } + if c.OlderThan != "" { + cond.OlderThan = sdk.StringPtr(c.OlderThan) + } + if c.NewerThan != "" { + cond.NewerThan = sdk.StringPtr(c.NewerThan) + } + 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) + } + mrv := &artifactregistry.RepositoryCleanupPolicyMostRecentVersionsArgs{ + PackageNamePrefixes: sdk.ToStringArray(m.PackageNamePrefixes), + } + if m.KeepCount != nil { + mrv.KeepCount = sdk.IntPtr(*m.KeepCount) + } + args.MostRecentVersions = mrv + } + out = append(out, args) + } + return out, nil +} diff --git a/pkg/clouds/pulumi/gcp/artifactregistry_cleanup_test.go b/pkg/clouds/pulumi/gcp/artifactregistry_cleanup_test.go new file mode 100644 index 00000000..fded1323 --- /dev/null +++ b/pkg/clouds/pulumi/gcp/artifactregistry_cleanup_test.go @@ -0,0 +1,165 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) Simple Container + +package gcp + +import ( + "testing" + + . "github.com/onsi/gomega" + "github.com/samber/lo" + + "github.com/simple-container-com/api/pkg/clouds/gcloud" +) + +// 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". +// +// So the contract is: declaring nothing must mean "not managed", never +// "delete". That is what ManagesCleanupPolicies gates. +func TestManagesCleanupPolicies(t *testing.T) { + RegisterTestingT(t) + + Expect((&gcloud.ArtifactRegistryConfig{}).ManagesCleanupPolicies()).To(BeFalse(), + "no declared policies must mean 'SC does not manage retention', so the field is ignored rather than emptied") + + Expect((&gcloud.ArtifactRegistryConfig{ + CleanupPolicies: []gcloud.ArtifactRegistryCleanupPolicy{{Name: "x", Action: "DELETE"}}, + }).ManagesCleanupPolicies()).To(BeTrue()) + + // 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()) +} + +// 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) { + RegisterTestingT(t) + + Expect(cleanupPolicyFields).To(ConsistOf("cleanupPolicies", "cleanupPolicyDryRun")) +} + +func TestCleanupPolicyArgsAcceptsRealPolicies(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)}, + }, + }) + Expect(err).To(BeNil()) + Expect(got).To(HaveLen(2)) +} + +func TestCleanupPolicyArgsRejectsBadInput(t *testing.T) { + RegisterTestingT(t) + + for _, tc := range []struct { + name string + policy 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: "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: "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: "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: "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: "missing name", + policy: 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)}, + }, + wantErr: "cannot be negative", + }, + } { + t.Run(tc.name, func(t *testing.T) { + RegisterTestingT(t) + _, err := cleanupPolicyArgs([]gcloud.ArtifactRegistryCleanupPolicy{tc.policy}) + Expect(err).NotTo(BeNil(), tc.why) + Expect(err.Error()).To(ContainSubstring(tc.wantErr)) + }) + } +} + +func TestCleanupPolicyArgsRejectsDuplicateNames(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)}}, + }) + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(ContainSubstring("duplicate cleanup policy name")) +} + +// 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) { + 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)) +} From 74109e6a2d7be9eefe7297ec5c0d550265f2ee61 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Thu, 13 Aug 2026 19:25:15 +0400 Subject: [PATCH 2/2] fix(gcp): update post-resolution fixture for the new cleanup-policy fields 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 --- pkg/api/tests/refapp_gke_autopilot.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/api/tests/refapp_gke_autopilot.go b/pkg/api/tests/refapp_gke_autopilot.go index 696883bc..c8050001 100644 --- a/pkg/api/tests/refapp_gke_autopilot.go +++ b/pkg/api/tests/refapp_gke_autopilot.go @@ -91,6 +91,12 @@ 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{},