search

kubernetes-retired / cluster-api-provider-nested

Cluster API Provider for Nested Clusters
Apache License 2.0
301 stars 67 forks source link

🐛Fix Pod Mutator ordering #319

Closed m-messiah closed 2 years ago

m-messiah commented 2 years ago

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

wondywang commented 2 years 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?

christopherhein commented 2 years ago

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

Fei-Guo commented 2 years ago

If plugin order matters, can we somehow add a test to check the order?

m-messiah commented 2 years ago

If plugin order matters, can we somehow add a test to check the order?

simple test added

m-messiah commented 2 years ago

/retest-required

Fei-Guo commented 2 years ago

/approve /lgtm

k8s-ci-robot commented 2 years ago

[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

Needs approval from an approver in each of these files: - ~~[virtualcluster/OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-nested/blob/main/virtualcluster/OWNERS)~~ [Fei-Guo] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment