Closed m-messiah closed 1 year ago
LGTM. But, I am not quite sure if the change of ResourceRegister List is suitable.
@Fei-Guo can you take a look as well?
/lgtm
But I will let Fei approve/comment. Needing ordering is a lot like how the admission registration works for api servers, based on naming to denote ordering.
If plugin order matters, can we somehow add a test to check the order?
If plugin order matters, can we somehow add a test to check the order?
simple test added
/retest-required
/approve /lgtm
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: Fei-Guo, m-messiah
The full list of commands accepted by this bot can be found here.
The pull request process is described here
What this PR does / why we need it: PodServiceLinkMutatorPlugin should be executed before the Default PodMutator (as well as PodMountServiceAccountTokenMutator). The first assumption in https://github.com/kubernetes-sigs/cluster-api-provider-nested/pull/314 was incorrect and we need to invoke Default after all mutations, rather than before. In the ideal way, we should have Default mutator in the same list of ordering, being able to execute some plugins before and some after the default, but Default Mutator uses too much controller fields (as serviceLister and secretLister), so it is hard to migrate it somewhere else without changing PodMutateCtx having all controller things. We should consider doing it later when it will be really needed. The PR also adds the missing test for serviceLinks
Which issue(s) this PR fixes: Fixes #318