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.

Sign up for GitHub

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

Jump to bottom

Bug: tags, nodeLabels, and nodeTaints cannot be completely removed from ManagedCluster, ManagedClustersAgentPool #3522

Closed
nojnhuh opened this issue Nov 6, 2023 · 5 comments · Fixed by #3540
Closed

Bug: tags, nodeLabels, and nodeTaints cannot be completely removed from ManagedCluster, ManagedClustersAgentPool #3522

nojnhuh opened this issue Nov 6, 2023 · 5 comments · Fixed by #3540
Assignees
@matthchr
Labels
bug 🪲 Something isn't working capz Required for CAPZ ASO adoption high-priority Issues we intend to prioritize (security, outage, blocking bug)
Milestone
v2.4.0

Comments

@nojnhuh
Copy link
Member

nojnhuh commented Nov 6, 2023

Version of Azure Service Operator

6657652, built from source

Describe the bug
When setting any of the spec fields tags, nodeLabels, and nodeTaints in ManagedClustersAgentPool to a non-null value, then to a null value, none of the corresponding configuration is reset in Azure. ManagedCluster spec.tags is also affected.

To Reproduce
Steps to reproduce the behavior:

  • Create a ManagedCluster with at least one key/value pair in spec.tags
  • After the Managed Cluster is Ready, remove all of spec.tags
  • The ManagedCluster goes to Reconciling and then to Ready, but the tags still exist according to Azure

Expected behavior
All of the tags are deleted in Azure.

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Add any other context about the problem here.
@nojnhuh
Copy link
Member Author

nojnhuh commented Nov 6, 2023

related to #3367, #3407

@theunrepentantgeek
Copy link
Member

theunrepentantgeek commented Nov 6, 2023 •
edited
Loading

Is this not fixed by #3407? If it's not, we may have a bug in our generated code.

(While #3407 was added to address spec.tags, we modified the behaviour for all containerservice collections.)

@matthchr
Copy link
Member

matthchr commented Nov 6, 2023

Yeah I was going to say, this should've been fixed by #3407. We should be able to reproduce with a test on our end though.

@nojnhuh what were you doing in the SDK to make this work previously? Were you setting it to null or to an empty collection?

@nojnhuh
Copy link
Member Author

nojnhuh commented Nov 6, 2023

I'm pretty sure I built everything from source right and I can see the changes from #3407 in my local copy.

CAPZ sets these to empty, non-nil values to delete all, so like map[string]string{} or SliceType{}.

@matthchr matthchr added bug 🪲 Something isn't working and removed needs-triage 🔍 labels Nov 6, 2023
@matthchr matthchr added this to the v2.4.0 milestone Nov 6, 2023
@matthchr
Copy link
Member

matthchr commented Nov 7, 2023

We did some digging into this and determine that the issue is that AKS RP treats null the same as omission, so while our test ensures that we pass null it does not ensure that null actually does what we want (which is clear the value). We'll fix that.

There are two possible fixes:

  1. Pass empty collection instead of null for these collections automatically.
  2. Allow user specified empty collection to propagate to AKS RP.

We'll need to think a bit about which option makes the most sense.

@matthchr matthchr added capz Required for CAPZ ASO adoption high-priority Issues we intend to prioritize (security, outage, blocking bug) labels Nov 7, 2023
matthchr added a commit to matthchr/azure-service-operator that referenced this issue Nov 10, 2023
@matthchr
Add support for explicitEmptyCollections payloadType
90232c5 
This fixes Azure#3522.

* Use this new payloadType for AKS resources, because the AKS RP doesn't
  treat "myProperty": null any different than omission of "myProperty"
  entirely, which meaans that it doesn't correctly clear properties that
  it should.
* Update test recordings.
@matthchr matthchr mentioned this issue Nov 10, 2023
Merged
2 tasks
matthchr added a commit to matthchr/azure-service-operator that referenced this issue Nov 11, 2023
@matthchr
Add support for explicitEmptyCollections payloadType
b8d786f 
This fixes Azure#3522.

* Use this new payloadType for AKS resources, because the AKS RP doesn't
  treat "myProperty": null any different than omission of "myProperty"
  entirely, which meaans that it doesn't correctly clear properties that
  it should.
* Update test recordings.
matthchr added a commit to matthchr/azure-service-operator that referenced this issue Nov 11, 2023
@matthchr
Add support for explicitEmptyCollections payloadType
f57b138 
This fixes Azure#3522.

* Use this new payloadType for AKS resources, because the AKS RP doesn't
  treat "myProperty": null any different than omission of "myProperty"
  entirely, which meaans that it doesn't correctly clear properties that
  it should.
* Update test recordings.
matthchr added a commit to matthchr/azure-service-operator that referenced this issue Nov 12, 2023
@matthchr
Add support for explicitEmptyCollections payloadType
a5401ec 
This fixes Azure#3522.

* Use this new payloadType for AKS resources, because the AKS RP doesn't
  treat "myProperty": null any different than omission of "myProperty"
  entirely, which meaans that it doesn't correctly clear properties that
  it should.
* Update test recordings.
github-merge-queue bot pushed a commit that referenced this issue Nov 12, 2023
@matthchr
Add support for explicitEmptyCollections payloadType (#3540)
e864652 
This fixes #3522.

* Use this new payloadType for AKS resources, because the AKS RP doesn't
  treat "myProperty": null any different than omission of "myProperty"
  entirely, which meaans that it doesn't correctly clear properties that
  it should.
* Update test recordings.
@github-project-automation github-project-automation bot moved this from Backlog to Recently Completed in Azure Service Operator Roadmap Nov 12, 2023
@matthchr matthchr self-assigned this Nov 13, 2023
@matthchr matthchr moved this from Recently Completed to Ready for Release in Azure Service Operator Roadmap Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@matthchr matthchr

Labels
bug 🪲 Something isn't working capz Required for CAPZ ASO adoption high-priority Issues we intend to prioritize (security, outage, blocking bug)
Projects
Azure Service Operator Roadmap
Archived in project
Milestone
v2.4.0
Development

Successfully merging a pull request may close this issue.

Add support for explicitEmptyCollections payloadType matthchr/azure-service-operator
3 participants
@theunrepentantgeek @matthchr @nojnhuh