Skip to content

Commit

Permalink
Enhance SSA ignoreChanges by having better field manager path compari…
Browse files Browse the repository at this point in the history
…sons (#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
  • Loading branch information
rquitales committed Feb 29, 2024
1 parent 2edaad8 commit 53f0563
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 9 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
19 changes: 18 additions & 1 deletion provider/pkg/await/await.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@ const depPatch = new k8s.apps.v1.DeploymentPatch(
},
spec: {
template: {
metadata: {
labels: undefined,
},
spec: {
containers: [
{
Expand All @@ -46,4 +43,4 @@ const depPatch = new k8s.apps.v1.DeploymentPatch(
},
}
},
}, { provider: provider, retainOnDelete: true });
}, { provider: provider, retainOnDelete: true, ignoreChanges: ["spec.template.metadata", "spec.selector"]});
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@ const depPatch = new k8s.apps.v1.DeploymentPatch(
},
spec: {
template: {
metadata: {
labels: undefined,
},
spec: {
containers: [
{
Expand All @@ -46,4 +43,4 @@ const depPatch = new k8s.apps.v1.DeploymentPatch(
},
}
},
}, { provider: provider, retainOnDelete: true });
}, { provider: provider, retainOnDelete: true, ignoreChanges: ["spec.template.metadata", "spec.selector"]});
21 changes: 21 additions & 0 deletions tests/sdk/nodejs/nodejs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
},
},
},
Expand Down

0 comments on commit 53f0563

Please sign in to comment.