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

External changes to labels and annotations aren't reverted with server side apply. #2331

Closed
bernhardloos opened this issue Mar 6, 2023 · 6 comments · Fixed by #2332
Closed
Assignees
Labels
area/server-side-apply kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed

Comments

@bernhardloos
Copy link

What happened?

I manually removed a label or annotations from a kubernetes resource managed pulumi. Pulumi notices it in refresh, but then it doesn't repair the missing annotation or label on up and doesn't indicate any differences on subsequent runs.
A changed value gets repaired as expected.
Also, I didn't try this without server side apply.

Expected Behavior

Pulumi should revert all external changes and restore resources to the configured state.

Steps to reproduce

  1. set enableServerSideApply to true for the provider
  2. create a resource with some labels and/or annotations (I used a Secret for my tests)
  3. manually delete a label or annotation from the resource with kubectl or some other tool
  4. run pulumi refresh, notice that the resource is marked as changed, the removed annotation is in the details
  5. run pulumi up
  6. the removed annotation/label is not restored

Output of pulumi about

CLI
Version 3.56.0
Go Version go1.20.1
Go Compiler gc

Plugins
NAME VERSION
kubernetes 3.24.1
kubernetes_crds 0.0.0
python unknown
tls 4.10.0

Host
OS fedora
Version 37
Arch x86_64

This project is written in python: executable='/usr/bin/python3' version='3.11.1'

Dependencies:
NAME VERSION
pip 22.2.2
pulumi-kubernetes 3.24.1
pulumi-kubernetes-crds 0.0.0
pulumi-tls 4.10.0
setuptools 62.6.0

Additional context

No response

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).

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

Hi @bernhardloos and thanks for reporting this.

At the moment, this is expected behaviour since with SSA enabled, we do not force an override for any conflicts with an existing version of the resource. To enable this, you'll need to append the "pulumi.com/patchForce": "true" annotation to the k8s resource you are managing with Pulumi.

Additional materials:

Please refer to our documentation about upserting a resource for more information:
https://www.pulumi.com/registry/packages/kubernetes/how-to-guides/managing-resources-with-server-side-apply/#upsert-a-resource

#2011 also documents the design decisions behind SSA implementation within our provider.

#2280 is an enhancement request to enable patchForce on a stack basis, rather than per-resource basis.

@rquitales rquitales added resolution/wont-fix This issue won't be fixed and removed needs-triage Needs attention from the triage team labels Mar 7, 2023
@bernhardloos
Copy link
Author

As far as I can tell, patchForce deals with conflicts via managedFields, but this is not the case here. The annotation or label is simply gone, no conflict. Unsurprisingly, setting the annotation patchForce didn't change the behavior in any way. Also, the missing annotation or label will get recreated without any problems if I make any change to the resource (even unrelated parts) in pulumi.
Either way, I would consider this a serious bug in the provider, because your actual configuration can now differ from the pulumi configuration without any indication that something is wrong.

@rquitales would you please consider reopening this?

@rquitales
Copy link
Member

rquitales commented Mar 7, 2023

Ah! When I tried to reproduce this locally, I mistakenly combined adding "pulumi.com/patchForce": "true" after removing the annotation with kubectl causing me to miss this bug, since making changes to the resource in other parts will cause the annotation to be recreated.

Yes, this does appear to be an issue we need to address.

@rquitales rquitales reopened this Mar 7, 2023
@rquitales rquitales added area/server-side-apply and removed resolution/wont-fix This issue won't be fixed labels Mar 7, 2023
@rquitales rquitales self-assigned this Mar 8, 2023
@rquitales
Copy link
Member

Tracked down this issue to 2 bugs, one within our codebase, the other is an upstream issue. Firstly, when we merge our client-side patch with server-side patch, we need to ensure that empty values are also removed when merging. Secondly, there is a bug in upstream imdario/mergo that does not merge maps properly.

I have filled a bug issue and PRs to address these.

rquitales added a commit that referenced this issue Mar 8, 2023
When merging client-side and server-side patches for diffing, we need to remove
any empty/removed keys from the server-side patch in the client-side patch as well.

Fixes: #2331
rquitales added a commit that referenced this issue Mar 8, 2023
When merging client-side and server-side patches for diffing, we need to remove
any empty/removed keys from the server-side patch in the client-side patch as well.

Fixes: #2331
rquitales added a commit that referenced this issue Mar 16, 2023
When merging client-side and server-side patches for diffing, we need to remove
any empty/removed keys from the server-side patch in the client-side patch as well.

Fixes: #2331
rquitales added a commit that referenced this issue Mar 16, 2023
When merging client-side and server-side patches for diffing, we need to remove
any empty/removed keys from the server-side patch in the client-side patch as well.

Fixes: #2331
rquitales added a commit that referenced this issue Mar 16, 2023
When merging client-side and server-side patches for diffing, we need to remove
any empty/removed keys from the server-side patch in the client-side patch as well.

Fixes: #2331
rquitales added a commit that referenced this issue Mar 16, 2023
* fix: Remove empty keys when merging unstructured resources

When merging client-side and server-side patches for diffing, we need to remove
any empty/removed keys from the server-side patch in the client-side patch as well.

Fixes: #2331

* test: Add integration test for SSA updating empty labels/annotations
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Mar 16, 2023
@colinbankier
Copy link

@rquitales could you clarify what the expected behaviour is when enableServerSideApply is false on the provider?
I'm seeing similar behaviour to the above even when just updating a value on a k8s resource. Enabling SSA does fix it, but it was surprising to me that the diff was not picked up without it. e.g. pulumi refresh does show a change, but the state is not updated so the value is never set upon a subsequent pulumi up.

@rquitales
Copy link
Member

@colinbankier The behaviour should be the same either CSA or SSA is used. It looks like this is also an issue for non-server side apply as well. Could you file a new issue for this and we'll work on resolving this issue. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants