OCPBUGS-98220: fix(cpo,hcco): prevent premature KAS convergence during KMS key rotation [release-4.22]#8972
OCPBUGS-98220: fix(cpo,hcco): prevent premature KAS convergence during KMS key rotation [release-4.22]#8972csrwng wants to merge 3 commits into
Conversation
Kubernetes deployment status fields exclude terminating pods, so IsDeploymentReady returns true while old KAS pods are still running. This causes ShouldPromoteTargetKey to fire prematurely during KMS key rotation, triggering overlapping rollouts with multiple KAS pods running different encryption configurations simultaneously. Two fixes applied to KASDeploymentConvergedWithEncryptionConfig: 1. Add an encryption-config-hash annotation to the KAS pod template so both CPO and HCCO can verify the deployment has rolled out with the specific encryption config containing the target key, rather than observing readiness from a previous rollout. 2. Add a hasTerminatingPods check that lists pods matching the deployment selector and returns false if any have DeletionTimestamp set, ensuring convergence is only reported after old pods have fully terminated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Log errors before failing closed in hasTerminatingPods so operators can diagnose unexpected failures in the terminating pod check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/jira cherry-pick OCPBUGS-98086 |
|
@csrwng: Jira Issue OCPBUGS-98086 has been cloned as Jira Issue OCPBUGS-98220. Will retitle bug to link to clone. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@csrwng: This pull request references Jira Issue OCPBUGS-98220, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-4.22 #8972 +/- ##
================================================
+ Coverage 35.85% 35.86% +0.01%
================================================
Files 774 774
Lines 94736 94791 +55
================================================
+ Hits 33969 33999 +30
- Misses 57986 58007 +21
- Partials 2781 2785 +4
🚀 New features to boost your workflow:
|
Fix gci import group ordering. Regenerate KAS deployment fixture to include the new encryption-config-hash annotation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2ca875c to
250dd74
Compare
|
/verified by ahitacat I have verify this test using the following annotation: To verify this works I have update twice the key version (on completion of the first one): Create a cluster With this we can see it succeeded of re-encrypt the second time with a new version. And cluster is healthy. |
|
@ahitacat: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@jparrill: This pull request references Jira Issue OCPBUGS-98220, which is invalid:
Comment DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
/hold |
|
/test e2e-aws |
|
@csrwng: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
What this PR does / why we need it:
KMS key rotation fails because
IsDeploymentReadyreturns true while old KAS pods are still terminating. Kubernetes deployment status fields explicitly exclude terminating pods ("non-terminating"), so all status counters look healthy while pods running the old encryption config are still serving requests. This causesShouldPromoteTargetKeyto fire prematurely, rewriting the encryption config and triggering another KAS rollout — resulting in 3 KAS pods with different encryption configs running simultaneously.This PR:
hasTerminatingPodscheck toKASDeploymentConvergedWithEncryptionConfigthat lists pods matching the deployment's selector and returns false if any haveDeletionTimestampsetencryption-config-hashannotation to the KAS pod template so convergence can verify the deployment rolled out with the correct configclient.Readerto the convergence function from both the CPO and HCCO callersThe fix fails closed — if listing pods errors, convergence is not reported.
This is the release-4.22 backport of #8970.
Which issue(s) this PR fixes:
Fixes OCPBUGS-98086
Special notes for your reviewer:
hasTerminatingPodshelper uses the deployment's ownSpec.Selectorto find its pods — no hardcoded labelsChecklist: