Adds PodNodeSelector to the list of plugins to be configured

[titansmc]

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:

Those are the defaults but if I want to add another plugin that needs configuration to inventory/ops_k1/group_vars/k8s_cluster/k8s-cluster.yml it won't work, because the variables defined in main.yaml will take preference over what is written in the inventory. So the only way I have is to run kubespray like this, from now on:

Which issue(s) this PR fixes:

Fixes #10588

Adds PodNodeSelector to the list of plugins to be configured

[k8s-ci-robot]

Hi @titansmc. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[VannTen]

So, I've taken a closer look at what we do with that variable, and IMO we should not have it in defaults to be modified:
The admission_plugin which needs a config file are a bounded set (as they are compiled in the api-server), so we should rather list all the concerned plugins than expose a configuration knob which could be misused.

Could you add the plugin you care about in the list instead ? (that would be nice if you added all of the plugins which needs configuration, but that's not a requirements to merge)

Also, please follow the PR template and add a proper release note.

/ok-to-test

[titansmc]

I see... @VannTen Let me know how it looks now

[VannTen]

Just one more thing and I think we'll be good: I think with that change you can remove the task "Kubeadm | Configure default cluster podnodeslector" introduced in #10607 since I think it will be handled by the generic one just above.

[yankay]

Hi @titansmc

Follow https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#which-plugins-are-enabled-by-default. The kubespray may not have a solid reason to add the PodNodeSelector as the default plugins, But it's configurable.

Best Regards :-)

[VannTen]

@yankay In that case it's not about defaults, but about which admission plugins needs a config file (which we use to conditionally push one)

[yankay]

@yankay In that case it's not about defaults, but about which admission plugins needs a config file (which we use to conditionally push one)

Thanks @VannTen

I got it. :-)

Follow the https://blog.opstree.com/2020/03/24/ansible-directory-structure-default-vs-vars/,

• defaults mean “default variables for the roles,” and vars mean “other variables for the role.”
• The priority of the vars is higher than that of defaults.

How about changing the kube_apiserver_admission_plugins_needs_configuration to roles/kubernetes/control-plane/defaults/main/main.yml?

[VannTen]

Precisely, this is the approach I think we should **not** use. We already have a variable defining which admission plugins are enabled: kube_apiserver_enable_admission_plugins See: - name: Kubeadm | Push admission control config files template: src: "{{ item | lower }}.yaml.j2" dest: "{{ kube_config_dir }}/admission-controls/{{ item | lower }}.yaml" mode: 0640 when: - kube_apiserver_admission_control_config_file - item in kube_apiserver_admission_plugins_needs_configuration loop: "{{ kube_apiserver_enable_admission_plugins }}" That means that we only use kube_apiserver_admission_plugins_needs_configuration as the set of admission plugins for which we will push a dedicated configuration file (one per admission plugin). That set of admission plugins should not be configurable, because the existing admission plugins are fixed (compiled in the apiserver), not user-supplied. Ultimately we should list all admission plugins which needs a config file here (ref: https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#what-does-each-admission-controller-do). That does not mean they will be enabled, just that when an user adds them in kube_apiserver_enable_admission_plugins, we correctly push the corresponding configuration file.

[VannTen]

Just one more thing and I think we'll be good: I think with that change you can remove the task "Kubeadm | Configure default cluster podnodeslector" introduced in #10607 since I think it will be handled by the generic one just above.

@titansmc

[VannTen]

@titansmc any chance you can address the previous comment ?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: titansmc
Once this PR has been reviewed and has the lgtm label, please assign liupeng0518 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[titansmc]

@VannTen should be fine now

[VannTen]

Yeah, that's fine this way ! 👍
Can you just squash your commit (at least the two first) and modify the commit message ? Currently it does not reflect what it's actually modifying ^^.

[linux-foundation-easycla]

• ✅ login: titansmc / name: jandres - moscardo (6734371, 646cf06, 2a91cc7, cdd6874)
• ❌ The email address for the commit (e0d1fec, 680a692) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

[titansmc]

I think I managed to screw it up even more....not sure how to follow from here

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-ci-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.