From 53f0563ecc4df9c185222c75bebda179f5b743ec Mon Sep 17 00:00:00 2001 From: Ramon Quitales Date: Thu, 29 Feb 2024 10:33:59 -0800 Subject: [PATCH] Enhance SSA ignoreChanges by having better field manager path comparisons (#2828) ### Proposed changes This PR enhances the field manager checks to ensure we do not take control of fields manager by other owners during a Patch Resource update. Previously, our set of managed fields only contains that the child node of a path. We now add the parent fields into the set as well. #### Example: Previously: Set containing currently managed path(s): `.spec.metadata.template.labels` Path we want to check if present in set: `.spec.metadata.template` Using `set.Has(path)` will return `false`. However, this is not wanted since adding `.spec.metadata.template` as an ignoreChanges path means we don't care about children fields as well. Now: `set.Has(path)` will now return `true`, which indicates to our provider that we need to ignore `spec.metadata.template` ### Related issues (optional) Fixes: #2714 --- CHANGELOG.md | 2 ++ provider/pkg/await/await.go | 19 ++++++++++++++++- .../step1/index.ts | 5 +---- .../step2/index.ts | 5 +---- tests/sdk/nodejs/nodejs_test.go | 21 +++++++++++++++++++ 5 files changed, 43 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d50b9f0c2d..63fbd441a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## Unreleased +- Fix SSA ignoreChanges by enhancing field manager path comparisons (https://github.com/pulumi/pulumi-kubernetes/pull/2828) + ## 4.8.1 (February 22, 2024) - skip normalization in preview w/ computed fields (https://github.com/pulumi/pulumi-kubernetes/pull/2846) diff --git a/provider/pkg/await/await.go b/provider/pkg/await/await.go index c503863a83..94c44def3e 100644 --- a/provider/pkg/await/await.go +++ b/provider/pkg/await/await.go @@ -568,7 +568,6 @@ func handleSSAIgnoreFields(c *UpdateConfig, liveOldObj *unstructured.Unstructure managedFields := liveOldObj.GetManagedFields() // Keep track of fields that are managed by the current field manager, and fields that are managed by other field managers. theirFields, ourFields := new(fieldpath.Set), new(fieldpath.Set) - fieldpath.MakePathOrDie() for _, f := range managedFields { s, err := fluxssa.FieldsToSet(*f.FieldsV1) @@ -584,6 +583,10 @@ func handleSSAIgnoreFields(c *UpdateConfig, liveOldObj *unstructured.Unstructure } } + // Add all parent paths of the fields to the set. + theirFields = ensureFieldsAreMembers(theirFields) + ourFields = ensureFieldsAreMembers(ourFields) + for _, ignorePath := range c.IgnoreChanges { ipParsed, err := resource.ParsePropertyPath(ignorePath) if err != nil { @@ -645,6 +648,20 @@ func makeInterfaceSlice[T any](inputs []T) []interface{} { return s } +// ensureFieldsAreMembers adds all parent paths of a given fieldpath.Set back to the set. The fieldpath.Set.Insert method specifically mentions that it does not +// add parent paths, so we need to do this manually. We do this by iterating from the start of the path to the end, and +// adding each level of the path tree to the set. +func ensureFieldsAreMembers(s *fieldpath.Set) *fieldpath.Set { + newSet := new(fieldpath.Set) + s.Iterate(func(p fieldpath.Path) { + for i := range p { + newSet.Insert(p[:i+1]) + } + }) + + return newSet +} + // fixCSAFieldManagers patches the field managers for an existing resource that was managed using client-side apply. // The new server-side apply field manager takes ownership of all these fields to avoid conflicts. func fixCSAFieldManagers(c *UpdateConfig, liveOldObj *unstructured.Unstructured, client dynamic.ResourceInterface) (*unstructured.Unstructured, error) { diff --git a/tests/sdk/nodejs/field-manager-patch-resources/step1/index.ts b/tests/sdk/nodejs/field-manager-patch-resources/step1/index.ts index 5dc6d4f7bd..d02da8ea3f 100644 --- a/tests/sdk/nodejs/field-manager-patch-resources/step1/index.ts +++ b/tests/sdk/nodejs/field-manager-patch-resources/step1/index.ts @@ -33,9 +33,6 @@ const depPatch = new k8s.apps.v1.DeploymentPatch( }, spec: { template: { - metadata: { - labels: undefined, - }, spec: { containers: [ { @@ -46,4 +43,4 @@ const depPatch = new k8s.apps.v1.DeploymentPatch( }, } }, -}, { provider: provider, retainOnDelete: true }); \ No newline at end of file +}, { provider: provider, retainOnDelete: true, ignoreChanges: ["spec.template.metadata", "spec.selector"]}); \ No newline at end of file diff --git a/tests/sdk/nodejs/field-manager-patch-resources/step2/index.ts b/tests/sdk/nodejs/field-manager-patch-resources/step2/index.ts index 7cc5029c71..dec7eb2b6e 100644 --- a/tests/sdk/nodejs/field-manager-patch-resources/step2/index.ts +++ b/tests/sdk/nodejs/field-manager-patch-resources/step2/index.ts @@ -33,9 +33,6 @@ const depPatch = new k8s.apps.v1.DeploymentPatch( }, spec: { template: { - metadata: { - labels: undefined, - }, spec: { containers: [ { @@ -46,4 +43,4 @@ const depPatch = new k8s.apps.v1.DeploymentPatch( }, } }, -}, { provider: provider, retainOnDelete: true }); \ No newline at end of file +}, { provider: provider, retainOnDelete: true, ignoreChanges: ["spec.template.metadata", "spec.selector"]}); \ No newline at end of file diff --git a/tests/sdk/nodejs/nodejs_test.go b/tests/sdk/nodejs/nodejs_test.go index ea07920da0..68c5298890 100644 --- a/tests/sdk/nodejs/nodejs_test.go +++ b/tests/sdk/nodejs/nodejs_test.go @@ -2325,6 +2325,27 @@ func TestFieldManagerPatchResources(t *testing.T) { depReplicas, err := tests.Kubectl("get deployment -o=jsonpath={.spec.replicas} -n", namespace, "test-mgr-nginx") assert.NoError(t, err) assert.Equal(t, "2", string(depReplicas)) + + // Ensure that we don't inadvertently share ownership of nested fields that we specify in ignoreChanges. + // See: https://github.com/pulumi/pulumi-kubernetes/issues/2714. + liveObj, err := tests.Kubectl("get deployment -o yaml --show-managed-fields -n", namespace, "test-mgr-nginx") + assert.NoError(t, err) + wantString := ` - apiVersion: apps/v1 + fieldsType: FieldsV1 + fieldsV1: + f:metadata: + f:annotations: + f:pulumi.com/patchForce: {} + f:spec: + f:template: + f:spec: + f:containers: + k:{"name":"nginx"}: + .: {} + f:image: {} + f:name: {} + manager: pulumi-kubernetes-` + assert.Contains(t, string(liveObj), wantString) }, }, },