Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DaemonSetPatch taking ownership of fields unexpectedly upon an update operation with ignoreChanges set #2714

Closed
zbuchheit opened this issue Dec 13, 2023 · 2 comments · Fixed by #2828
Assignees
Labels
area/patch area/server-side-apply kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Milestone

Comments

@zbuchheit
Copy link

zbuchheit commented Dec 13, 2023

What happened?

While using the Pulumi DaemonSetPatch resource, I encountered a bug where having ignoreChanges set caused unexpected behavior. Specifically, when this parameter was set, the Pulumi process took unexpected ownership of resources during an update operation. It did not occur on the initial resource creation, but on an update operation once a field was changed.

I had ignoreChanges set to spec.template.metadata and spec.selector and witnessed the pulumi-kubernetes manager take ownership of spec.template.metadata but not spec.selector

Example

Code Example

		daemonSet, err := v1.NewDaemonSetPatch(ctx, "k8s-daemonset-patch", &v1.DaemonSetPatchArgs{
					Metadata: metav1.ObjectMetaPatchArgs{
						Name:      pulumi.String("kube-proxy"),
						Namespace: pulumi.String("kube-system"),
						Annotations: pulumi.StringMap{
							"pulumi.com/patchForce":        pulumi.String("true"),
						},
					},
					Spec: v1.DaemonSetSpecPatchArgs{
						Template: corev1.PodTemplateSpecPatchArgs{
							Spec: corev1.PodSpecPatchArgs{
								Containers: corev1.ContainerPatchArray{
									corev1.ContainerPatchArgs{
										Name:  pulumi.String("kube-proxy"),
										Image: pulumi.String("602401143452.dkr.ecr.us-west-2.amazonaws.com/eks/kube-proxy:v1.24.17-minimal-eksbuild.2"), //Change this to for update
									},
								},
							},
						},
					},
				}, pulumi.Provider(kubeProvider),
				pulumi.IgnoreChanges([]string{"spec.template.metadata", "spec.selector"}), 
				pulumi.RetainOnDelete(true), 
			)

Resulting Config Information

The kube-proxy config on resource creation.
The kube-proxy config after an update with ignoreChanges set.

Output of pulumi about

CLI          
Version      3.92.0
Go Version   go1.21.3
Go Compiler  gc

Plugins
NAME        VERSION
go          unknown
kubernetes  4.5.5

Host     
OS       darwin
Version  14.0
Arch     arm64

This project is written in go: executable='/opt/homebrew/bin/go' version='go version go1.21.0 darwin/arm64'

Current Stack: zbuchheit-pulumi-corp/go-daemon-patch/dev

TYPE                               URN
pulumi:pulumi:Stack                urn:pulumi:dev::go-daemon-patch::pulumi:pulumi:Stack::go-daemon-patch-dev
pulumi:providers:pulumi            urn:pulumi:dev::go-daemon-patch::pulumi:providers:pulumi::default
pulumi:pulumi:StackReference       urn:pulumi:dev::go-daemon-patch::pulumi:pulumi:StackReference::zbuchheit-pulumi-corp/aws-ts-eks/dev
pulumi:providers:kubernetes        urn:pulumi:dev::go-daemon-patch::pulumi:providers:kubernetes::eks-kubernetes
kubernetes:apps/v1:DaemonSetPatch  urn:pulumi:dev::go-daemon-patch::kubernetes:apps/v1:DaemonSetPatch::k8s-daemonset-patch


Found no pending operations associated with dev

Backend        
Name           pulumi.com
URL            https://app.pulumi.com/zbuchheit-pulumi-corp
User           zbuchheit-pulumi-corp
Organizations  zbuchheit-pulumi-corp
Token type     personal

Dependencies:
NAME                                        VERSION
github.com/pulumi/pulumi-kubernetes/sdk/v4  4.5.5
github.com/pulumi/pulumi/sdk/v3             3.92.0

Pulumi locates its logs in /var/folders/lh/l71cdh810xb33t0jc7qmt5_80000gn/T/ by default

Additional context

I believe it might be slightly related to the described behavior here: #2639 (comment)

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@zbuchheit zbuchheit added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Dec 13, 2023
@rquitales
Copy link
Member

Thanks for the detailed logs. It looks like we might need to harden our fieldManager path traversal/matching logic.

@rquitales rquitales added area/server-side-apply and removed needs-triage Needs attention from the triage team labels Dec 14, 2023
@arwilczek90
Copy link

I can also confirm that this happens on the image field for almost any workload which is a very dangerous bug if you are not using pulumi to deploy image changes.

@mjeffryes mjeffryes added this to the 0.100 milestone Jan 29, 2024
rquitales added a commit that referenced this issue Feb 29, 2024
rquitales added a commit that referenced this issue Feb 29, 2024
rquitales added a commit that referenced this issue Feb 29, 2024
…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
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/patch area/server-side-apply kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants