Fix etcd certificate lifecycle: prevent unnecessary regeneration and ensure restart on cert changes

[AdeshDeshmukh]

What type of PR is this?

/kind bug

What this PR does / why we need it

Fixes three compounding bugs in the etcd certificate lifecycle that cause certificates to regenerate on every playbook run and etcd to never restart after regeneration, leading to in-memory/on-disk TLS context drift.

Bug 1 — force_etcd_cert_refresh: true forces regeneration every run

roles/etcd_defaults/defaults/main.yml:18

Changed default to false. This flag was introduced in PR #7219 as a band-aid that bypassed the cert-existence checking logic. Users can still opt in with force_etcd_cert_refresh: true if needed.

Bug 2 — Handler timing prevents etcd restart

roles/etcd/tasks/main.yml:81-87

The etcd_secret_changed fact was set by a handler (Set etcd_secret_changed) which fires at end-of-play, but was checked in a when condition during the play — before the handler fired. Fixed by having gen_certs_script.yml directly notify Restart etcd and Restart etcd-events alongside the existing Set etcd_secret_changed notification, then removing the dead gate tasks.

Bug 3 — Wrong handler name for etcd-events

roles/etcd/tasks/main.yml:91

Copy-paste bug: "Restart etcd-events if certs changed" notified Restart etcd instead of Restart etcd-events. The Restart etcd-events handler also lacked the etcd_events_cluster_setup guard, which has been added.

Additional changes

• patterns in check_certs.yml:5 changed from comma-separated string to YAML list (idiomatic correctness)
• Dead gate tasks at main.yml:81-95 removed (16 lines deleted)
• Added etcd_events_cluster_setup | default(false) guard to Restart etcd-events handler

Why direct notify instead of flush_handlers?

The flush_handlers approach would cause a double restart when both config and certs change. Direct notify avoids this entirely because Ansible automatically deduplicates handler notifications — only one restart fires per play regardless of how many tasks notify.

Which issue(s) this PR fixes

Fixes #13353

Special notes for your reviewer

Notification flow after fix:
gen_certs_script.yml tasks that generate or sync certs now directly notify Restart etcd and Restart etcd-events alongside the existing Set etcd_secret_changed. Ansible deduplication ensures exactly one restart per play.

Guard moved to handler: etcd_events_cluster_setup moved from the deleted gate task to the handler itself — evaluated at the correct time (end-of-play).

Cross-role preserved: etcd_secret_changed in roles/kubernetes/control-plane/tasks/pre-upgrade.yml:9 continues to work because that runs in a separate play (handlers fire between plays).

Does this PR introduce a user-facing change?

Fixes etcd certificate lifecycle: certificates no longer regenerate on every playbook run, and etcd now restarts when certificates change. Previously, in-memory and on-disk TLS context could diverge, potentially causing cluster-wide TLS handshake failures on etcd restart.

Checklist

• Issue filed (etcd certs regenerate on every playbook run, etcd never restarted — in-memory and on-disk TLS context drifts #13353)
• Pre-commit checks pass (local pre-commit hook blocked by Python 3.13 env issue, CI will run)
• Idempotency verified (second run should not regenerate certs)
• Cert restart verified (cert delete -> re-run -> etcd restarts)
• DCO sign-off included

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

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

[kubernetes-prow]

Hi @AdeshDeshmukh. 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.

Details

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.

[yankay]

/ok-to-test

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[AdeshDeshmukh]

@yankay Could you please review? CI is all green. 🙏

[tico88612]

I don't really understand why you wrote it this way. Could you explain?

[AdeshDeshmukh]

@tico88612 .
The handler now fires on non-etcd nodes after the direct-notify change , but etcd_cluster_is_healthy is only defined on etcd members.

The default(dict(rc=1)) just prevents a crash on nodes where the variable doesn't exist... it evaluates to "unhealthy" and safely skips the backup there.

I kept the original condition intact rather than adding a separate group check because the health check already implies it.

[Payback159]

Related, not conflicting in intent: I opened #13406, which removes the duplicate execution of the etcd cert tasks within a single run (every step of the role exists twice since #9173, and hosts that are in both kube_control_plane and k8s_cluster run both copies whenever the network plugin needs etcd client certs).

This PR fixes how often certificates are regenerated across runs and the missing restart afterwards, #13406 fixes how often the tasks execute within one run — so the two are complementary rather than alternatives.

They touch four files in common, but a test merge only conflicts in roles/etcd/tasks/gen_certs_script.yml; check_certs.yml, main.yml and etcd_defaults/defaults/main.yml merge cleanly. Resolving it means putting the notify: block from this PR on the single merged cert generation task instead of two. Happy to rebase #13406 on top of whichever lands first.

[Payback159]

While testing #13406 on a live Vagrant cluster (3 dedicated etcd, 2 control plane, 1 worker, calico with calico_datastore: etcd) I ran into something that looks directly relevant to Bug 1 of this PR.

In roles/etcd/tasks/check_certs.yml the existing_certs variable is never templated — there are no {{ }} around it:

- name: "Check_certs | Set 'gen_*_certs' groups to track which nodes needs to have certs generated on first etcd node"
  vars:
    existing_certs: etcdcert_master.files | map(attribute='path')
  ansible.builtin.group_by:
    key: "gen_{{ item.node_type }}_certs_{{ force_etcd_cert_refresh or item.certs is not subset(existing_certs) }}"

So existing_certs is the literal string etcdcert_master.files | map(attribute='path') (type_debug reports AnsibleUnicode), and item.certs is not subset(<a string>) is always true. The group key is therefore always gen_*_certs_True, whether or not the certificate actually exists on the first etcd node.

Reproduced against real cluster state, running the task above verbatim next to a copy that only differs by adding the {{ }}, with force_etcd_cert_refresh: false and all member certs present:

found 19 cert files on etcdt-1
member-etcdt-1.pem present: True
as-is  groups: ['gen_master_certs_True', 'gen_node_certs_True']
fixed  groups: ['FIXED_gen_master_certs_False', 'FIXED_gen_node_certs_True']

Same data, same flag — the only difference is the templating, and the master group flips from True to the correct False.

Since sync_certs is derived from 'gen_master_certs_True' in group_names or 'gen_node_certs_True' in group_names, this feeds straight into how much work a run does after force_etcd_cert_refresh becomes false by default.

To be precise about what I did not verify: I cannot claim this alone defeats the purpose of flipping the default. In my run the corrected variant still yielded gen_node_certs_True, because dedicated etcd members legitimately have no node-<host>.pem while cert_files.node is defined for every host — a separate question, and possibly topology specific. I only want to flag the untemplated variable itself, which is unambiguous.

Not something #13406 touches or needs; raising it here because it sits right next to Bug 1. Happy to open a separate issue if you would rather keep this PR focused.