Etcd Certificates are not generated when adding nodes to an existing cluster with scale.yml

[kartsank]

What type of PR is this?
/kind bug

What this PR does / why we need it:
if will add new host or missing node etcd cert host to gen_node_certs_True group as part of check_certs and use to create certs using gen_certs task.

Which issue(s) this PR fixes:
Fixes #12117

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
None

NONE

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: kartsank / name: Karthik S (5e650c3, a3ccbe2, c33c394)

[k8s-ci-robot]

Welcome @kartsank!

It looks like this is your first PR to kubernetes-sigs/kubespray 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubespray has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @kartsank. 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-sigs/prow repository.

[chadswen]

/ok-to-test

[chadswen]

/retest

[VannTen]

You're duplicating the group definition in the task above (with group_by). Even if add_host is the right approach, we should only keep one group generation.

[kartsank]

You're duplicating the group definition in the task above (with group_by). Even if add_host is the right approach, we should only keep one group generation.

An existing group was created using inventory_hostname (etcd, control-plane). If the etcd play is executed without the kube_node or new_host inventory group, the node certificate will not be generated on the first etcd node. To address this, I am re-evaluating all hosts in the k8s_cluster instead of just the inventory_hostname hosts and adding any missing hosts to the existing group.

[VannTen]

I see where the problem is. This was fixed for cluster.yml and upgrade-cluster.yml in #10769 , but scale.yml does not use the common playbook install-etcd.yml which has the correct behavior (dynamically add only the needed hosts, aka nodes using a direct etcd client). I don't think there is anything preventing us from using the same thing in scale.yml, which should fix the problem.

[kartsank]

I see where the problem is. This was fixed for cluster.yml and upgrade-cluster.yml in #10769 , but scale.yml does not use the common playbook install-etcd.yml which has the correct behavior (dynamically add only the needed hosts, aka nodes using a direct etcd client). I don't think there is anything preventing us from using the same thing in scale.yml, which should fix the problem.

Initially, I considered including install-etcd.yml, but it adds all kube_node to the groups rather than just the missing nodes. My goal is to create the node certificate only for the missing or new hosts, rather than re-creating the node certificate each time.

https://github.com/kubernetes-sigs/kubespray/blob/master/playbooks/install_etcd.yml#L1C1-L15C17

If force_etcd_cert_refresh is set to true, we can add all hosts. Otherwise, there is no need to re-create the node certificate for every execution of scale.yml. Additionally, the installation of etcd is not required for scale.yml unless etcd_cluster_setup is enabled for new nodes.

https://github.com/kubernetes-sigs/kubespray/blob/master/playbooks/install_etcd.yml#L17-L29

[VannTen]

Initially, I considered including install-etcd.yml, but it adds all kube_node to the groups rather than just the missing nodes. My goal is to create the node certificate only for the missing or new hosts, rather than re-creating the node certificate each time.

group_by still consider --limit, or does it not ? So even with force_etcd_refresh + scale.yml with --limit (which is the intended usage), I don't see the advantage.

[kartsank]

Initially, I considered including install-etcd.yml, but it adds all kube_node to the groups rather than just the missing nodes. My goal is to create the node certificate only for the missing or new hosts, rather than re-creating the node certificate each time.

group_by still consider --limit, or does it not ? So even with force_etcd_refresh + scale.yml with --limit (which is the intended usage), I don't see the advantage.

I believe the limit is not considered by group_by. Therefore, we have two options: we can either include install-etcd.yml within scale.yml, or we can add the kube_node group to the existing ETCD play in scale.yml.

• name: Generate the etcd certificates beforehand
  hosts: etcd:kube_control_plane---------> add :kube_node
  gather_facts: false
  any_errors_fatal: "{{ any_errors_fatal | default(true) }}"
  environment: "{{ proxy_disable_env }}"
  roles:
  • { role: kubespray-defaults }
  • role: etcd
    tags: etcd
    vars:
    etcd_cluster_setup: false
    etcd_events_cluster_setup: false
    when:
    • etcd_deployment_type != "kubeadm"
    • kube_network_plugin in ["calico", "flannel", "canal", "cilium"] or cilium_deploy_additionally | default(false) | bool
    • kube_network_plugin != "calico" or calico_datastore == "etcd"

https://github.com/kubernetes-sigs/kubespray/blob/master/playbooks/scale.yml#L9

[VannTen]

I don't think so limit is not consider by group_by.

It does, I just tested.

[kartsank]

I tested the install_etcd.yml in scale.yml, and all tests were successful. Therefore, I will include install_etcd.yml in scale.yml to resolve the add_node certificate issue.

Thank you for the feedback. I will update this pull request to include install_etcd.yml.

@VannTen Please review and provider your comments

[kartsank]

@VannTen Please review and provider your comments. updated scale.yml with install_etcd playbook.

[VannTen]

The only slight concern I have is this, which differs in install-etcd.yml.

However this only affect 'etcd', so it would only be a problem for using scale.yml with etcd nodes, which is not supported. I wonder if we have an easy way to fail early if something like this is attempted 🤔

Maybe that's out of scope for this though.

@tico88612 @ant31 thoughts ?

[ant31]

can we set the variables in the import? or with set fact before ?

[kartsank]

The default value for etcd_cluster_setup is set to true. However, this can be changed to false by configuring the variables in scale.yaml.

https://github.com/kubernetes-sigs/kubespray/blob/master/roles/etcd/defaults/main.yml#L6

@VannTen and @ant31 please review my changes.

[VannTen]

Oh right, I don't why I thought we could not set vars on import_playbook.

In that case we'd define in both cases at import_playbook level and be done with it.

[kartsank]

updated.

[VannTen]

You'll need to update cluster.yml and upgrade-cluster.yml as well.

[VannTen]

Oh right, I don't why I thought we could not set vars on import_playbook.

In that case we'd define in both cases at import_playbook level and be done with it.

[VannTen]

So the whole section should be scraped and handled at import_playbook level on both use sites.

[kartsank]

@VannTen updated on cluster.yml and upgrade_cluster.yml. please review. Thanks

[kartsank]

You'll need to update cluster.yml and upgrade-cluster.yml as well.

updated

[VannTen]

Thanks for your patience !
That should do it
/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kartsank, VannTen

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:

• OWNERS [VannTen]

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

[kartsank]

Thanks for your patience ! That should do it /approve /lgtm

Thanks for your review support and approval.