OCPBUGS-49691: Include named cert CAs in root-ca ConfigMap#9064
OCPBUGS-49691: Include named cert CAs in root-ca ConfigMap#9064hypershift-jira-solve-ci[bot] wants to merge 2 commits into
Conversation
When named certificates are configured for KAS via spec.configuration.apiServer.servingCerts.namedCertificates, the CA certificates that signed them were not included in the root-ca ConfigMap. This caused TLS verification failures for in-cluster clients connecting to the KAS external endpoint using the in-cluster CA bundle (e.g., ArgoCD running with a service account). ReconcileRootCAConfigMap now accepts the named certificate secrets and appends their tls.crt content to the CA bundle. The reconcilePKI method delegates trust source fetching to a new getRootCATrustSources helper that retrieves both the observed default ingress cert and named certificate secrets. The previously dead includeServingCertificates function, which was written for this purpose but never wired in, is removed since the logic is now properly integrated into the reconciliation flow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add error wrapping in reconcilePKI for getRootCATrustSources error - Add empty-name guard for named certificates (matching OAuth path) - Add warning log when skipping NotFound named cert secrets - Add t.Parallel() to TestReconcileRootCAConfigMap and subtests - Switch test assertions to Gomega for pki package consistency - Add test case for named cert secret with no tls.crt key Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-moderate
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-49691, 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. |
openshift-ci-robot
added
the
jira/invalid-bug
📝 WalkthroughWalkthroughThe controller now collects the observed default ingress certificate and configured named certificate Secrets for PKI reconciliation. Sequence Diagram(s)sequenceDiagram
participant HostedControlPlaneReconciler
participant KubernetesAPI
participant ReconcileRootCAConfigMap
participant RootCAConfigMap
HostedControlPlaneReconciler->>KubernetesAPI: Retrieve ingress ConfigMap and named certificate Secrets
HostedControlPlaneReconciler->>ReconcileRootCAConfigMap: Provide trust sources
ReconcileRootCAConfigMap->>RootCAConfigMap: Build and append CA certificates
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hypershift-jira-solve-ci[bot] 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 |
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
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/pki/ca.go (1)
118-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the aggregate-CA error with context.
The new branch propagates the error bare, which loses the failing reconciliation step for callers other than the current controller. Use
%wwhen rethrowing.Based on coding guidelines, “Wrap errors with context when rethrowing.”
Proposed fix
if err := reconcileAggregateCA(cm, ownerRef, sources...); err != nil { - return err + return fmt.Errorf("failed to reconcile aggregate CA: %w", err) }🤖 Prompt for 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. In `@control-plane-operator/controllers/hostedcontrolplane/pki/ca.go` around lines 118 - 120, Update the error return in the reconcileAggregateCA call to wrap the underlying error with context using %w, identifying the aggregate-CA reconciliation step while preserving error unwrapping for callers.Source: Coding guidelines
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 1904-1918: The named certificate Secret lookup in the
HostedControlPlane reconciliation flow does not establish watches for
user-managed Secrets. Update the controller setup and the named-certificate
handling around GetNamedCertificates to watch referenced Secrets and enqueue the
owning HostedControlPlane when those Secrets are created, updated, or deleted,
while preserving the existing not-found and error behavior.
---
Nitpick comments:
In `@control-plane-operator/controllers/hostedcontrolplane/pki/ca.go`:
- Around line 118-120: Update the error return in the reconcileAggregateCA call
to wrap the underlying error with context using %w, identifying the aggregate-CA
reconciliation step while preserving error unwrapping for callers.
🪄 Autofix (Beta)
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: Enterprise
Run ID: eddfa1fc-40be-4f29-a7d6-94085e518a0d
📒 Files selected for processing (4)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/pki/ca.gocontrol-plane-operator/controllers/hostedcontrolplane/pki/ca_test.go
💤 Files with no reviewable changes (1)
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
| var namedCertSecrets []*corev1.Secret | ||
| for _, namedCert := range hcp.Spec.Configuration.GetNamedCertificates() { | ||
| if namedCert.ServingCertificate.Name == "" { | ||
| continue | ||
| } | ||
| secret := &corev1.Secret{} | ||
| if err := r.Get(ctx, client.ObjectKey{Namespace: hcp.Namespace, Name: namedCert.ServingCertificate.Name}, secret); err != nil { | ||
| if !apierrors.IsNotFound(err) { | ||
| return nil, nil, fmt.Errorf("failed to get named certificate secret %s: %w", namedCert.ServingCertificate.Name, err) | ||
| } | ||
| log := ctrl.LoggerFrom(ctx) | ||
| log.Info("Named certificate secret not found, skipping for root CA trust bundle", "secret", namedCert.ServingCertificate.Name) | ||
| continue | ||
| } | ||
| namedCertSecrets = append(namedCertSecrets, secret) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'SetupWithManager|Watches\(|Owns\(|EnqueueRequestsFromMapFunc|NamedCertificates|ServingCertificate' control-plane-operator/controllers/hostedcontrolplaneRepository: openshift/hypershift
Length of output: 33300
Watch the named certificate Secrets here. This code only reads them during HCP reconcile, and the Secret watch in this controller only enqueues owner refs. If these are user-managed Secrets, create/update/delete events won’t trigger a reconcile, so root-ca can stay stale until some unrelated HCP change.
🤖 Prompt for 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.
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`
around lines 1904 - 1918, The named certificate Secret lookup in the
HostedControlPlane reconciliation flow does not establish watches for
user-managed Secrets. Update the controller setup and the named-certificate
handling around GetNamedCertificates to watch referenced Secrets and enqueue the
owning HostedControlPlane when those Secrets are created, updated, or deleted,
while preserving the existing not-found and error behavior.
|
@hypershift-jira-solve-ci[bot]: 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9064 +/- ##
=======================================
Coverage 44.50% 44.51%
=======================================
Files 774 774
Lines 96980 96988 +8
=======================================
+ Hits 43164 43175 +11
+ Misses 50828 50821 -7
- Partials 2988 2992 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1 participant
What this PR does / why we need it:
When named certificates are configured for KAS via
spec.configuration.apiServer.servingCerts.namedCertificates, the CAcertificates that signed them were not included in the
root-caConfigMap. This caused TLS verification failures for in-cluster
clients connecting to the KAS external endpoint using the in-cluster
CA bundle (e.g., ArgoCD running with a service account).
This PR fixes the issue by:
ReconcileRootCAConfigMapto accept the named certificate secretsand append their
tls.crtcontent to the CA bundle.getRootCATrustSourceshelper toreconcilePKIthat retrieves boththe observed default ingress cert and named certificate secrets.
includeServingCertificatesfunction, which waswritten for this purpose but never wired in, since the logic is now properly
integrated into the reconciliation flow.
t.Parallel()and Gomega assertions for consistency.Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/OCPBUGS-49691
Special notes for your reviewer:
The
includeServingCertificatesfunction that was removed was dead code — it existed but was never called. The new implementation integrates the same logic directly into the reconciliation flow.Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via openshift-developer plugin
Summary by CodeRabbit
Enhancements
Bug Fixes