search

cybozu-go / accurate

Kubernetes controller for multi-tenancy. It propagates resources between namespaces accurately and allows tenant users to create/delete sub-namespaces.
https://cybozu-go.github.io/accurate/
Apache License 2.0
34 stars 5 forks source link

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

Closed erikgb closed 4 months ago

erikgb commented 4 months ago

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 https://github.com/kubernetes/kubernetes/pull/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 https://github.com/cybozu-go/accurate/issues/120

erikgb commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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

@zoetrope @yamatcha Could you please check it?

yamatcha commented 4 months ago

@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. https://github.com/cybozu-go/accurate/blob/36bb411afb82b1f56ad0767379c5bd3c130619c9/e2e/e2e_test.go#L174-L177

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

erikgb commented 4 months ago

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

erikgb commented 4 months ago

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:

https://github.com/cybozu-go/accurate/commit/e899d8797aba131a4b5a398490d134ee202f129d#diff-33975f8c70294bffa37f57ef00ebd5656b884bad2006be7e261ef8cca1011161R122-R123

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