OCPBUGS-77781: fix(certs): normalize IP SANs to stop dual-stack KAS cert churn - #9286
OCPBUGS-77781: fix(certs): normalize IP SANs to stop dual-stack KAS cert churn#9286RamLavi wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@RamLavi: This pull request references Jira Issue OCPBUGS-77781, 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
RamLavi
changed the title
openshift-ci
Bot
added
area/control-plane-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@support/certs/tls_test.go`:
- Around line 198-201: Update the “ipv4-only” test case in the test table to
remove the IPv6 loopback address 0:0:0:0:0:0:0:1, leaving only IPv4 addresses so
the case provides true IPv4-only coverage; keep IPv6 coverage in the existing
dual-stack case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a207c001-b99f-4245-bbd4-ee75c377cce9
📒 Files selected for processing (2)
support/certs/tls.gosupport/certs/tls_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9286 +/- ##
==========================================
+ Coverage 45.67% 45.69% +0.01%
==========================================
Files 781 781
Lines 97726 97736 +10
==========================================
+ Hits 44641 44660 +19
+ Misses 50019 50010 -9
Partials 3066 3066
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
OCPBUGS-77781: ValidateKeyPair compared cert IP SANs with raw byte slices. After X.509 round-trip, IPv4 SANs are 4 bytes while desired IPs from net.ParseIP are 16-byte IPv4-mapped, so dual-stack SANs mis-sorted and falsely failed validation. CPO then resigned kas-server-crt every reconcile, churning the kube-apiserver Deployment. Normalize IPs with net.IP.To16() before compare. Add a unit test that reconciles twice for ipv4-only and dual-stack and asserts the cert is not regenerated. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ram Lavi <ralavi@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
RamLavi
changed the title
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@RamLavi: This pull request references Jira Issue OCPBUGS-77781, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (yli2@redhat.com), skipping review request. 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. |
maiqueb
left a comment
maiqueb
left a comment
There was a problem hiding this comment.
Just left a recommendation to improve the unit test's readability.
Feel free to leave it as is.
| if err := certs.ReconcileSignedCert( | ||
| secret, | ||
| ca, | ||
| "kubernetes", | ||
| []string{"kubernetes"}, | ||
| pki.X509UsageServerAuth, | ||
| corev1.TLSCertKey, | ||
| corev1.TLSPrivateKeyKey, | ||
| "", | ||
| dnsNames, | ||
| tc.ips, | ||
| ); err != nil { | ||
| t.Fatalf("ReconcileSignedCert iteration %d failed: %v", i, err) | ||
| } | ||
|
|
||
| cert := secret.Data[corev1.TLSCertKey] | ||
| key := secret.Data[corev1.TLSPrivateKeyKey] | ||
| if len(cert) == 0 || len(key) == 0 { | ||
| t.Fatalf("iteration %d did not populate tls.crt/tls.key", i) | ||
| } | ||
|
|
There was a problem hiding this comment.
This test is very nice, but I think we could improve its readability.
How about we move this code into an helper (let's assume it would be called reconcileCert), and invoke it twice from the test ?
Then the test would read something like:
secret := &corev1.Secret{Type: corev1.SecretTypeTLS}
initialCert, initialKey, err := reconcileCert(secret, ca, tc.ips)
if err != nil {
t.Fatalf("initial certificate generation failed: %w", err)
return
}
laterCert, laterKey, err := reconcileCert(secret, ca, tc.ips)
if err != nil {
t.Fatalf("subsequent certificate generation failed: %w", err)
return
}
if !bytes.Equal(firstCert, laterCert) {
t.Fatalf("tls.crt was regenerated for %s (revalidation should have been a no-op)", i, tc.name)
}
if !bytes.Equal(firstKey, laterKey) {
t.Fatalf("tls.key was regenerated for %s (revalidation should have been a no-op)", i, tc.name)
}There was a problem hiding this comment.
will leave as is as you suggest, if that OK.
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: maiqueb, nunnatsa, RamLavi The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
|
@RamLavi: The following tests failed, say
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. |
6 participants
What this PR does / why we need it
On dual-stack HostedClusters,
ValidateKeyPaircompared certificate IP SANs withcmp.Diff+SortSlices(bytes.Compare)on rawnet.IPbyte slices.After an X.509 round-trip, IPv4 SANs are stored as 4-byte values, while desired IPs from
net.ParseIPare 16-byte IPv4-mapped. Mixed lengths change sort order for some IPv6 service CIDRs (e.g.2001:db8:…), so validation falsely reports that IP addresses differ even when the logical set is unchanged.CPO then resigns
kas-server-crtevery reconcile → kube-apiserver Deploymentconfig-hash/ generation churn → ReplicaSet pile-up.Fix: normalize both sides with
net.IP.To16()before comparing.Test: unit test reconciles twice for ipv4-only and dual-stack IPs and asserts
tls.crtis not regenerated.Which issue(s) this PR fixes
Fixes OCPBUGS-77781
Special notes for your reviewer
Manual verification (kubevirt HCP, stock CPO → fixed CPO)
Dual-stack HC with
serviceNetworkincluding2001:db8:2::/112(ULAfd02::/112alone often does not trigger the sort mismatch).Before (stock CPO): between two samples, RS count and Deployment generation climb while SANs stay identical:
After (CPO image with this fix): over 60s, metrics stay flat:
Checklist
Summary by CodeRabbit
Bug Fixes
Tests