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

fix: upgrade managed fields in controller resources from CSA to SSA #121

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

erikgb
Copy link
Contributor

@erikgb erikgb commented Feb 23, 2024

This PR reuses the upstream code that kubectl uses to upgrade managedFields from CSA to SSA. It appears like subresources are not supported yet, but I am working on it in kubernetes/kubernetes#123484. The PR allowing us to upgrade managed fields for subresources is now merged and should be released with the upcoming K8s 1.30. I will create a follow-up PR when the improved helper is available, but this PR fixes the main issue.

Fixes #120

@erikgb
Copy link
Contributor Author

erikgb commented Feb 23, 2024

@ymmt2005, do you have any suggestions on how to test this properly? Some of the owned resources are created with a CREATE request, which will set the managedFields to Update (CSA). I could test by forcing an update of the owned resource with SSA - which should also upgrade the resource from CSA to SSA. But I was hoping to replace the initial CREATE with an SSA to avoid wasting api-server resources, so this will only be a temporary test-solution. 🤔 What I want to do, is to pre-create a resource simulating a resource that existed before migrating to SSA. Could it be an option to do it in the controller's test-suite setup - before the controllers/manager are bootstrapped? WDYT?

@ymmt2005
Copy link
Member

@erikgb
Hello,
Appreciate your continued contribution!

What I want to do, is to pre-create a resource simulating a resource that existed before migrating to SSA. Could it be an option to do it in the controller's test-suite setup - before the controllers/manager are bootstrapped?

I'm OK with this.

@erikgb
Copy link
Contributor Author

erikgb commented Feb 26, 2024

@ymmt2005 Do you know what's wrong with the CI? Seems unrelated to the change here, and I cannot reproduce it locally. It appears to complain about some aqua "policies".

@ymmt2005
Copy link
Member

hmm... I'm not familiar with Aqua.

@zoetrope @yamatcha
Could you please check it?

@yamatcha
Copy link
Contributor

yamatcha commented Feb 27, 2024

@erikgb
Most of this error log is unimportant what is important is this the following part.
https://github.com/cybozu-go/accurate/actions/runs/8030385773/job/21937711586?pr=121#step:6:38
I think CI failed because my secret was not found here.

accurate/e2e/e2e_test.go

Lines 174 to 177 in 36bb411

Eventually(func() error {
_, err := kubectl(nil, "get", "-n", "sub1", "secrets", "mysecret")
return err
}).Should(Succeed())

The warning log by aqua may confuse developers, so I'll fix it in another PR.

@erikgb
Copy link
Contributor Author

erikgb commented Feb 27, 2024

Thanks for the feedback @yamatcha! I will take another look!

@erikgb erikgb changed the title fix: upgrade controller resources from CSA to SSA fix: upgrade managed fields in controller resources from CSA to SSA Mar 3, 2024
@erikgb erikgb marked this pull request as ready for review March 3, 2024 20:14
@erikgb
Copy link
Contributor Author

erikgb commented Mar 3, 2024

I finally found the reason for the failing test. When using SSA you need to set ALL fields the manager cares for. These two lines were missing:

e899d87#diff-33975f8c70294bffa37f57ef00ebd5656b884bad2006be7e261ef8cca1011161R122-R123

No idea why the envtests didn't catch this. But the PR should be ready for review now! PTAL!

@yamatcha yamatcha self-assigned this Mar 4, 2024
@yamatcha yamatcha removed their assignment Mar 13, 2024
@yamatcha yamatcha self-requested a review March 13, 2024 02:03
@yamatcha yamatcha merged commit a823423 into cybozu-go:main Mar 14, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre-existing resources should be upgraded to SSA
3 participants