OCPBUGS-82192: fix(webhookcerts): handle upgrade from service-ca managed certs - #8189
Conversation
On existing OpenShift clusters upgrading to self-managed webhook certs, the service-ca operator may still have its annotations on the operator Service and may have created the serving cert secret. This causes a CA mismatch: the webhook configs contain the self-managed CA bundle but the serving cert is signed by service-ca's CA, resulting in "x509: certificate signed by unknown authority" errors. Add removeServiceCAResources() to the WebhookCertReconciler following the same pattern as removeServiceCAAnnotationAndSecret in the CPO. It removes the service-ca annotations from the Service and deletes the serving cert secret if it was created by service-ca, allowing the reconciler to recreate it signed by the self-managed CA. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
enxebre
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@enxebre: This pull request references CNTRLPLANE-2207 which is a valid jira issue. 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. |
📝 WalkthroughWalkthroughThe webhook certificates controller is updated to support migration away from service-ca-managed serving certificates. New constants define annotation keys used by service-ca. The reconciliation flow now includes a preliminary step that removes service-ca-related state: it fetches the operator Service and removes service-ca annotations, then fetches and deletes the serving-cert Secret if it is service-ca-managed. Detection of service-ca management is performed by checking for originating-service annotation keys on the Secret. After cleanup, normal self-signed CA reconciliation proceeds. Sequence DiagramsequenceDiagram
participant Reconciler as Webhook Cert Reconciler
participant K8sAPI as Kubernetes API
participant Service as Operator Service
participant Secret as Manager Serving Cert Secret
Reconciler->>Reconciler: Reconcile called
Reconciler->>K8sAPI: removeServiceCAResources()
K8sAPI->>Service: Fetch operator Service
Service-->>K8sAPI: Service object
K8sAPI-->>Reconciler: Service retrieved
rect rgba(255, 107, 107, 0.5)
Reconciler->>Reconciler: Check for service-ca annotations
Reconciler->>K8sAPI: Remove annotations from Service
K8sAPI->>Service: Update Service (annotations removed)
Service-->>K8sAPI: Service updated
K8sAPI-->>Reconciler: Update confirmed
end
K8sAPI->>Secret: Fetch manager-serving-cert Secret
Secret-->>K8sAPI: Secret object (if exists)
K8sAPI-->>Reconciler: Secret retrieved
rect rgba(255, 107, 107, 0.5)
Reconciler->>Reconciler: isServiceCAManaged(Secret)
Reconciler->>Reconciler: Check originating-service annotations
alt Secret is service-ca managed
Reconciler->>K8sAPI: Delete Secret
K8sAPI->>Secret: Delete Secret
Secret-->>K8sAPI: Deleted
else Secret is self-managed
Reconciler->>Reconciler: Skip deletion
end
end
Reconciler->>Reconciler: Reconcile self-signed CA (original flow)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/hypershift-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre 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 |
openshift-ci
Bot
added
the
approved
|
@enxebre: This pull request references CNTRLPLANE-2207 which is a valid jira issue. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go (1)
233-258: Add explicit alpha-annotation coverage and reuse controller constants in tests.The new tests only exercise beta keys via string literals. Since controller logic handles both alpha and beta keys, add an alpha case and reference
serviceCABetaAnnotation/serviceCAAlphaAnnotationandoriginatingServiceBetaAnnotation/originatingServiceAlphaAnnotationto avoid drift.As per coding guidelines,
**: Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.Also applies to: 260-315, 317-335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go` around lines 233 - 258, Test currently only asserts removal of the beta service-ca annotation using string literals; add an equivalent test case that covers the alpha annotation too and replace any hard-coded annotation keys in these tests with the controller constants serviceCABetaAnnotation, serviceCAAlphaAnnotation, originatingServiceBetaAnnotation and originatingServiceAlphaAnnotation so the tests exercise both alpha and beta paths and stay in sync with controller logic (update the test in the t.Run block and the other referenced blocks around lines 260-335 to use those constants and add the extra alpha-case assertion).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@hypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go`:
- Around line 233-258: Test currently only asserts removal of the beta
service-ca annotation using string literals; add an equivalent test case that
covers the alpha annotation too and replace any hard-coded annotation keys in
these tests with the controller constants serviceCABetaAnnotation,
serviceCAAlphaAnnotation, originatingServiceBetaAnnotation and
originatingServiceAlphaAnnotation so the tests exercise both alpha and beta
paths and stay in sync with controller logic (update the test in the t.Run block
and the other referenced blocks around lines 260-335 to use those constants and
add the extra alpha-case assertion).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c8bcac3a-25cb-4462-bbdb-c9f4f9107193
📒 Files selected for processing (2)
hypershift-operator/controllers/webhookcerts/webhookcerts_controller.gohypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go
sjenning
changed the title
openshift-ci-robot
added
the
jira/invalid-bug
|
@enxebre: This pull request references Jira Issue OCPBUGS-82192, 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@sjenning: This pull request references Jira Issue OCPBUGS-82192, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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 @@
## main #8189 +/- ##
==========================================
+ Coverage 33.11% 33.13% +0.01%
==========================================
Files 768 768
Lines 93116 93159 +43
==========================================
+ Hits 30840 30865 +25
- Misses 59665 59677 +12
- Partials 2611 2617 +6
🚀 New features to boost your workflow:
|
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
|
/test e2e-aws-4-21 |
|
/verified by e2e and @sjenning |
openshift-ci-robot
added
the
verified
|
@clebs: 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. |
|
@enxebre: 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. |
|
@enxebre: Jira Issue Verification Checks: Jira Issue OCPBUGS-82192 Jira Issue OCPBUGS-82192 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
Fix included in release 4.22.0-0.nightly-2026-04-10-145151 |
6 participants
Summary
x509: certificate signed by unknown authority.removeServiceCAResources()toWebhookCertReconcilerfollowing the same pattern asremoveServiceCAAnnotationAndSecretin the CPO. It removes service-ca annotations from the Service and deletes the serving cert secret if it was created by service-ca, so the reconciler recreates it signed by the self-managed CA.Test plan
go test ./hypershift-operator/controllers/webhookcerts/...)make verifypasses🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests