OCPBUGS-62177: verify cert revocation against all KAS pods - #8263
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR changes the certificate revocation controller to verify API server certificates by connecting to each non-terminating, ready KAS pod instead of a single service endpoint. It lists pods labeled Sequence Diagram(s)sequenceDiagram
participant Controller as CertificateRevocationController
participant Informer as Pod Informer / Cache
participant Pod as KAS Pod
participant K8sAPI as Kubernetes API Server
Controller->>Informer: list pods (label=app=kube-apiserver)
Informer-->>Controller: pod list
Controller->>Controller: filter non-terminating, check isPodReady & PodIP
alt no ready pods
Controller->>Controller: requeue
else ready pods exist
loop for each ready pod
Controller->>Controller: resolve container port (named or default)
Controller->>Pod: HTTPS SSR to https://<PodIP>:<port> (TLS SNI = KAS service)
Pod->>K8sAPI: forward SSR to kube-apiserver instance
K8sAPI-->>Pod: SSR response (authorized/unauthorized)
Pod-->>Controller: SSR result
end
Controller->>Controller: require all pods to accept/reject as gate condition
end
Controller->>K8sAPI: update certificate revocation status
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go`:
- Around line 1320-1349: The test currently only asserts the aggregate boolean
result for verifyCertificateAgainstAllKASPods; update the case so the testFunc
used in the scenario records/counts invocations (e.g., increment a counter
closed over by the test) and assert that the counter equals the number of ready,
non-terminating pods (two in this case) in addition to checking expectedResult;
modify the test case's testFunc and add an assertion after calling
verifyCertificateAgainstAllKASPods to enforce one callback invocation per ready
pod.
In
`@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go`:
- Around line 603-612: The code currently forces pod-level TLS verification off
by setting podCfg.TLSClientConfig.Insecure = true and clearing CAData/CAFile;
instead remove that line and restore CA verification by copying the CA from the
original admin config into the pod config (e.g., set
podCfg.TLSClientConfig.CAData = adminCfg.TLSClientConfig.CAData and
podCfg.TLSClientConfig.CAFile = adminCfg.TLSClientConfig.CAFile) while keeping
podCfg.TLSClientConfig.ServerName = hcpmanifests.KubeAPIServerServiceName so
SNI/hostname verification works for the pod connection in
certificaterevocationcontroller (use the podCfg and adminCfg symbols to locate
the change).
🪄 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: Pro Plus
Run ID: 9b0040c3-97c3-458f-a2ee-f91dc0478343
⛔ Files ignored due to path filters (5)
control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yamlis excluded by!**/testdata/**
📒 Files selected for processing (3)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/role.yamlcontrol-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.gocontrol-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8263 +/- ##
==========================================
+ Coverage 37.53% 37.64% +0.10%
==========================================
Files 751 751
Lines 92026 92160 +134
==========================================
+ Hits 34544 34691 +147
+ Misses 54841 54812 -29
- Partials 2641 2657 +16
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go (1)
99-100: Wire the Pods informer into the controller dependency chain.Pod readiness and IP now gate revocation progress. Calling
Pods().Informer()for a side effect (lines 99-100) does not register the informer with the controller factory, so the pod cache won't participate in initial sync and pod transitions won't trigger re-enqueuing of CRR work. Pass the informer to the controller factory to make pod updates drive reconciliation immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go` around lines 99 - 100, The Pods informer is only being created for side effects (kubeInformersForNamespaces.InformersFor(hostedControlPlane.Namespace).Core().V1().Pods().Informer()) but not registered with the controller factory, so pod events won't drive reconciliation; change the code to capture the informer (e.g., podsInformer := kubeInformersForNamespaces.InformersFor(hostedControlPlane.Namespace).Core().V1().Pods().Informer()) and pass that informer into the controller factory/constructor used to build the CertificateRevocation controller (the controller factory or NewController/WithInformer call you use to wire controllers) so the pod cache participates in initial sync and pod updates enqueue CRR work immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go`:
- Around line 603-617: The per-pod client probes currently inherit adminCfg with
no timeout, so create a bounded timeout for each pod: set podCfg.Timeout =
<reasonable duration> (e.g., 10s) after building podCfg (the struct from
rest.AnonymousClientConfig) to ensure the generated http client has a request
timeout, and also wrap the testFunc call with a child context with the same
timeout (use context.WithTimeout(ctx, <same duration>) and defer cancel) before
calling testFunc(ctxWithTimeout, podClient) to avoid hanging reconcile workers;
update references around podCfg, podClient, and testFunc accordingly.
---
Nitpick comments:
In
`@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go`:
- Around line 99-100: The Pods informer is only being created for side effects
(kubeInformersForNamespaces.InformersFor(hostedControlPlane.Namespace).Core().V1().Pods().Informer())
but not registered with the controller factory, so pod events won't drive
reconciliation; change the code to capture the informer (e.g., podsInformer :=
kubeInformersForNamespaces.InformersFor(hostedControlPlane.Namespace).Core().V1().Pods().Informer())
and pass that informer into the controller factory/constructor used to build the
CertificateRevocation controller (the controller factory or
NewController/WithInformer call you use to wire controllers) so the pod cache
participates in initial sync and pod updates enqueue CRR work immediately.
🪄 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: Pro Plus
Run ID: e75e2dca-ee10-4fe8-951e-575e5dcc6fc0
📒 Files selected for processing (1)
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go (2)
1376-1378: Strengthen thelistPodsmock contract.Line 1376 currently ignores
namespaceandselector, so selector/namespace regressions won’t be caught by this test.Suggested diff
t.Run(testCase.name, func(t *testing.T) { + expectedSelector := labels.SelectorFromSet(labels.Set{"app": "kube-apiserver"}) c := &CertificateRevocationController{ listPods: func(namespace string, selector labels.Selector) ([]*corev1.Pod, error) { + if namespace != "test-ns" { + t.Fatalf("unexpected namespace: %q", namespace) + } + if selector.String() != expectedSelector.String() { + t.Fatalf("unexpected selector: %q", selector.String()) + } return testCase.pods, nil }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go` around lines 1376 - 1378, The test's listPods mock currently ignores its namespace and selector parameters; update the mock used in the test case for listPods to validate that the incoming namespace and selector match the expected values for that test (e.g., compare against testCase.expectedNamespace and testCase.expectedSelector or derive from testCase.namespace/testCase.selector) and return an error if they differ, otherwise return testCase.pods; this will ensure regressions in namespace/selector handling are caught by the test.
1246-1373: Add explicit cases for missingPodIPand error propagation branches.The table currently misses the
PodIP == ""requeue path and error-return branches (listPodserror andtestFuncerror). Covering these will harden regression detection in this critical flow.Suggested additions (table-driven cases)
for _, testCase := range []struct { name string pods []*corev1.Pod testFunc func(ctx context.Context, client kubernetes.Interface) (bool, error) + listPodsErr error expectedResult bool expectedErr bool expectedCalls int }{ + { + name: "When a ready pod has empty PodIP it should requeue", + pods: []*corev1.Pod{{ + ObjectMeta: metav1.ObjectMeta{Name: "kas-1", Namespace: "test-ns"}, + Spec: kasPodSpec(), + Status: corev1.PodStatus{ + PodIP: "", + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, + }, + }}, + expectedResult: false, + expectedCalls: 0, + }, + { + name: "When listing pods fails it should return an error", + listPodsErr: assert.AnError, + expectedErr: true, + expectedCalls: 0, + }, + { + name: "When testFunc returns an error it should return an error", + pods: []*corev1.Pod{{ + ObjectMeta: metav1.ObjectMeta{Name: "kas-1", Namespace: "test-ns"}, + Spec: kasPodSpec(), + Status: corev1.PodStatus{ + PodIP: "10.0.0.1", + Conditions: []corev1.PodCondition{{Type: corev1.PodReady, Status: corev1.ConditionTrue}}, + }, + }}, + testFunc: func(_ context.Context, _ kubernetes.Interface) (bool, error) { + return false, assert.AnError + }, + expectedErr: true, + expectedCalls: 1, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go` around lines 1246 - 1373, Add three table-driven test cases to exercise the missing branches: (1) a "pod with empty PodIP should requeue" case where pods contains a Pod whose Status.PodIP == "" and expectedResult is false; (2) a "listPods returns error" case that simulates listPods failing (set test setup to return an error when listing pods) and set expectedErr true; and (3) a "testFunc returns error" case where testFunc returns (false, error) and expectedErr true. Place these entries alongside the existing cases in the same test table (the loop over testCase) so the existing test runner uses them; reference the existing fields testFunc and expectedErr to validate behavior and the listPods path to simulate the list failure. Ensure the case names are descriptive and expectedResult/expectedCalls are set appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go`:
- Around line 1376-1378: The test's listPods mock currently ignores its
namespace and selector parameters; update the mock used in the test case for
listPods to validate that the incoming namespace and selector match the expected
values for that test (e.g., compare against testCase.expectedNamespace and
testCase.expectedSelector or derive from testCase.namespace/testCase.selector)
and return an error if they differ, otherwise return testCase.pods; this will
ensure regressions in namespace/selector handling are caught by the test.
- Around line 1246-1373: Add three table-driven test cases to exercise the
missing branches: (1) a "pod with empty PodIP should requeue" case where pods
contains a Pod whose Status.PodIP == "" and expectedResult is false; (2) a
"listPods returns error" case that simulates listPods failing (set test setup to
return an error when listing pods) and set expectedErr true; and (3) a "testFunc
returns error" case where testFunc returns (false, error) and expectedErr true.
Place these entries alongside the existing cases in the same test table (the
loop over testCase) so the existing test runner uses them; reference the
existing fields testFunc and expectedErr to validate behavior and the listPods
path to simulate the list failure. Ensure the case names are descriptive and
expectedResult/expectedCalls are set appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 40f42242-fce3-4416-beb2-928fa3c63249
📒 Files selected for processing (1)
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go
0e87b3d to
3ac77d1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go (1)
601-622:⚠️ Potential issue | 🟠 MajorAdd explicit timeout to prevent hanging on unresponsive pods.
The per-pod client probes inherit
Timeout=0fromadminCfg(meaning no timeout). A blackholed or half-open PodIP can hang the reconcile worker indefinitely sincerest.Config.Timeout=0means "no timeout" in client-go.🛡️ Proposed fix
+ const perPodTimeout = 10 * time.Second + for _, pod := range readyPods { port := containerPort(pod, "client", config.KASPodDefaultPort) podCfg := rest.AnonymousClientConfig(adminCfg) + podCfg.Timeout = perPodTimeout podCfg.TLSClientConfig.CertData = certPEM podCfg.TLSClientConfig.KeyData = keyPEM podCfg.Host = fmt.Sprintf("https://%s", net.JoinHostPort(pod.Status.PodIP, strconv.Itoa(int(port)))) // We're connecting to the PodIP, but the serving cert is still issued for the KAS service // name. Keep CA verification enabled and override ServerName for SNI + hostname validation. podCfg.TLSClientConfig.ServerName = hcpmanifests.KubeAPIServerServiceName podClient, err := kubernetes.NewForConfig(podCfg) if err != nil { return false, fmt.Errorf("couldn't create client for KAS pod %s/%s: %w", pod.Namespace, pod.Name, err) } - passed, err := testFunc(ctx, podClient) + podCtx, cancel := context.WithTimeout(ctx, perPodTimeout) + passed, err := testFunc(podCtx, podClient) + cancel() // explicit cleanup instead of defer inside loop if err != nil { return false, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go` around lines 601 - 622, The pod-specific REST client inherits a zero (no) timeout from rest.AnonymousClientConfig(adminCfg), which can hang on blackholed PodIPs; set an explicit timeout on the returned rest.Config (podCfg.Timeout) before calling kubernetes.NewForConfig — e.g. use a sane default like 15–30s or copy adminCfg.Timeout when non-zero — and ensure you import time if using a time.Duration literal; update the code around rest.AnonymousClientConfig(adminCfg) / podCfg and before kubernetes.NewForConfig to assign podCfg.Timeout.
🧹 Nitpick comments (1)
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go (1)
584-599: Consider adding debug logging for requeue reasons.When requeuing because a pod isn't ready or no pods are available, it may be helpful for debugging to log which pod or condition triggered the requeue. This is optional since the behavior is correct.
💡 Optional: Add debug logging
var readyPods []*corev1.Pod for _, pod := range pods { if pod.DeletionTimestamp != nil { continue } if !isPodReady(pod) || pod.Status.PodIP == "" { // a non-terminating pod that's not ready: we can't check it yet, requeue + klog.V(4).Infof("KAS pod %s/%s not ready for verification (ready=%v, podIP=%q), requeueing", + pod.Namespace, pod.Name, isPodReady(pod), pod.Status.PodIP) return false, nil } readyPods = append(readyPods, pod) } if len(readyPods) == 0 { // no pods to check yet, requeue + klog.V(4).Infof("No ready KAS pods found in namespace %s, requeueing", namespace) return false, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go` around lines 584 - 599, Add debug logs before the two early returns to record why we're requeuing: inside the loop, when skipping a pod because !isPodReady(pod) or pod.Status.PodIP == "" log the pod's name/namespace and the specific condition that triggered the requeue (use pod.Name, pod.Namespace, isPodReady result and pod.Status.PodIP); and after the loop, when len(readyPods) == 0 log that no non-terminating ready pods were found (include the original pods list size or selector info if available). Use the controller's existing logger (e.g., r.Log or reqLogger) to emit these debug messages right before the respective return false, nil statements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go`:
- Around line 601-622: The pod-specific REST client inherits a zero (no) timeout
from rest.AnonymousClientConfig(adminCfg), which can hang on blackholed PodIPs;
set an explicit timeout on the returned rest.Config (podCfg.Timeout) before
calling kubernetes.NewForConfig — e.g. use a sane default like 15–30s or copy
adminCfg.Timeout when non-zero — and ensure you import time if using a
time.Duration literal; update the code around
rest.AnonymousClientConfig(adminCfg) / podCfg and before kubernetes.NewForConfig
to assign podCfg.Timeout.
---
Nitpick comments:
In
`@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go`:
- Around line 584-599: Add debug logs before the two early returns to record why
we're requeuing: inside the loop, when skipping a pod because !isPodReady(pod)
or pod.Status.PodIP == "" log the pod's name/namespace and the specific
condition that triggered the requeue (use pod.Name, pod.Namespace, isPodReady
result and pod.Status.PodIP); and after the loop, when len(readyPods) == 0 log
that no non-terminating ready pods were found (include the original pods list
size or selector info if available). Use the controller's existing logger (e.g.,
r.Log or reqLogger) to emit these debug messages right before the respective
return false, nil statements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a2b128b1-8046-4168-956c-a837c093fb68
⛔ Files ignored due to path filters (5)
control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yamlis excluded by!**/testdata/**
📒 Files selected for processing (3)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/role.yamlcontrol-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.gocontrol-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go
✅ Files skipped from review due to trivial changes (1)
- control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/role.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go
sdminonne
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@sdminonne: This pull request references Jira Issue OCPBUGS-62177, 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
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go (1)
99-100: Wire pod updates into the queue, not just the cache.The new gate now depends on pod readiness and
PodIP, but this informer is only being initialized for lister access. That means CRRs still converge on those transitions only viaSyntheticRequeueError. Hooking pod events intoenqueueAll(...)would make rollouts react faster and cut needless requeues while KAS pods are coming up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go` around lines 99 - 100, The Pod informer is only created for lister access but not wired to enqueue changes, so Pod readiness/PodIP transitions don't trigger immediate reconciles; update the code around kubeInformersForNamespaces.InformersFor(hostedControlPlane.Namespace).Core().V1().Pods().Informer() to add event handlers that call enqueueAll(...) on Pod Add/Update/Delete (use cache.ResourceEventHandlerFuncs with AddFunc, UpdateFunc that compares old/new and always call enqueueAll for relevant changes, and DeleteFunc) so Pod events drive the controller rather than relying on SyntheticRequeueError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go`:
- Around line 99-100: The Pod informer is only created for lister access but not
wired to enqueue changes, so Pod readiness/PodIP transitions don't trigger
immediate reconciles; update the code around
kubeInformersForNamespaces.InformersFor(hostedControlPlane.Namespace).Core().V1().Pods().Informer()
to add event handlers that call enqueueAll(...) on Pod Add/Update/Delete (use
cache.ResourceEventHandlerFuncs with AddFunc, UpdateFunc that compares old/new
and always call enqueueAll for relevant changes, and DeleteFunc) so Pod events
drive the controller rather than relying on SyntheticRequeueError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b0d6c3c1-3ba0-403f-bbf5-2f676ef56184
📒 Files selected for processing (2)
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.gocontrol-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go
146e31a to
5b7f272
Compare
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@sdminonne: This pull request references Jira Issue OCPBUGS-62177, 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 (jiezhao@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. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
/test e2e-aks-4-22 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
Test Failure Analysis:
|
| # | Test | Duration | Direct Error |
|---|---|---|---|
| 1 | EnsureGlobalPullSecret/When_management-cluster_hostedCluster.Spec.PullSecret_is_updated_in-place… |
1215s | context deadline exceeded waiting for DaemonSet |
| 2 | EnsureGlobalPullSecret/Check_if_the_config.json_is_correct_in_all_of_the_nodes |
0.02s | 409: daemonsets.apps "kubelet-config-verifier" already exists |
| 3 | EnsureGlobalPullSecret |
— | Parent cascade |
| 4 | Main |
— | Parent cascade |
| 5 | TestCreateCluster |
— | Parent cascade |
Recommendations
-
Move cleanup to
deferinVerifyKubeletConfigWithDaemonSet()(globalps.go:210-240). The DaemonSet deletion and pull-secret cleanup must execute regardless of whetherwaitForDaemonSetReadysucceeds:ds, err := CreateKubeletConfigVerifierDaemonSet(...) g.Expect(err).ToNot(HaveOccurred()) defer func() { _ = client.AppsV1().DaemonSets(ds.Namespace).Delete(ctx, ds.Name, metav1.DeleteOptions{}) _ = client.CoreV1().Secrets(ds.Namespace).Delete(ctx, "pull-secret", metav1.DeleteOptions{}) }()
-
Handle
AlreadyExistsfor the DaemonSet inCreateKubeletConfigVerifierDaemonSet()(line 200). The function already handlesAlreadyExistsfor the pull-secret (lines 47-50) but not for the DaemonSet itself. Add the same pattern: delete-and-recreate or return the existing object. -
Investigate the 2/3 DaemonSet readiness. The
replicas: 1NodePool with 3 actual nodes suggests either autoscaling or a stale NodePool spec. The DaemonSet should tolerate node count mismatches or the test should pin expected pod count to the actual schedulable node count. -
This PR is safe to merge from a test perspective — the failures are a known flake in
EnsureGlobalPullSecretunrelated to cert revocation changes.
Evidence
build-log.txt — DaemonSet stuck at 2/3 ready (~360 poll iterations):
waiting for DaemonSet kubelet-config-verifier to be ready, 2/3 are ready
waiting for DaemonSet kubelet-config-verifier to be ready, 2/3 are ready
...
(repeats for ~20 minutes)
build-log.txt — Timeout error (globalps.go:220):
failed to wait for DaemonSet kubelet-config-verifier to be ready: context deadline exceeded
build-log.txt — Cascade error (globalps.go:211):
daemonsets.apps "kubelet-config-verifier" already exists
junit.xml — Failure 1 (timeout):
<testcase name="When_management-cluster_hostedCluster.Spec.PullSecret_is_updated_in-place..."
classname="TestCreateCluster/Main/EnsureGlobalPullSecret" time="1215.12">
<failure>failed to wait for DaemonSet kubelet-config-verifier to be ready: context deadline exceeded</failure>
</testcase>junit.xml — Failure 2 (stale resource):
<testcase name="Check_if_the_config.json_is_correct_in_all_of_the_nodes"
classname="TestCreateCluster/Main/EnsureGlobalPullSecret" time="0.02">
<failure>daemonsets.apps "kubelet-config-verifier" already exists</failure>
</testcase>globalps.go:220 — Non-deferred assertion (root of cascade):
g.Expect(waitForDaemonSetReady(ctx, client, ds)).To(Succeed())
// Lines 223-239: cleanup code UNREACHABLE on failurePR tests — All passed:
TestCreateClusterHABreakGlassCredentials— PASSED (1911.95s)- All break-glass-credentials subtests — PASSED
|
/retest-required |
|
|
|
/verified by e2e |
openshift-ci-robot
added
the
verified
|
@sdminonne: 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. |
| } | ||
| hasWorkerNodes = len(nodeList.Items) > 0 | ||
| } else { | ||
| // Private clusters are not reachable from the test runner; |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/test-required |
|
@sdminonne: 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. |
|
@sdminonne: Jira Issue Verification Checks: Jira Issue OCPBUGS-62177 Jira Issue OCPBUGS-62177 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 5.0.0-0.nightly-2026-05-12-025124 |
|
/cherry-pick release-4.20 |
|
@sdminonne: #8263 failed to apply on top of branch "release-4.20": 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 kubernetes-sigs/prow repository. |
|
/cherry-pick release-4.22 |
|
@sdminonne: #8263 failed to apply on top of branch "release-4.22": 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 kubernetes-sigs/prow repository. |
9 participants
What this PR does / why we need it:
Updates the
CertificateRevocationControllerto verify certificate trust and revocation against every individual KAS pod rather than through the service load balancer. In HA deployments with 3 KAS replicas, the service endpoint load-balances to a single pod, so the check could hit a pod that had loaded the updated trust bundle while others hadn't, causing premature state transitions in the revocation flow.Changes
control-plane-pki-operator (
CertificateRevocationController):verifyCertificateAgainstAllKASPodswhich enumerates ready KAS pods via label selector and connects to each pod IP directly (with SNI set to the KAS service name for TLS validation)ensureNewSignerCertificatePropagated, verify the old signer is also still trusted (detects mid-reload); inensureOldSignerCertificateRevoked, verify the new signer cert is still trusted before declaring the old one revoked, preventing false positives when a pod restart temporarily rejects all certsverifyCertificateTrustedandverifyCertificateRevokedhelpers for per-pod SelfSubjectReview checkscontrol-plane-operator (RBAC):
list/watchon pods andgeton deployments to thecontrol-plane-pki-operatorrole so it can enumerate KAS pods and verify replica counts (update role asset + 5 test fixtures)support/podspec:
IsPodReadyandContainerPorthelpers for pod readiness checks and named port lookupe2e / integration:
TestCreateClusterHABreakGlassCredentialsexercising the break-glass credential flow on a HighlyAvailable control plane (3 KAS replicas)hasWorkerNodesdetection for private clusters by checking NodePool replica counts instead of assuming workers existContext
This was previously fixed in PRs #7405 + #7744 but reverted in #7784 because the e2e test added alongside it had a ~50% flake rate. The controller fix itself was sound — the flakiness was in the e2e test wiring. This PR re-applies the controller logic with comprehensive unit tests and a different, more targeted HA e2e test.
Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/OCPBUGS-62177
Special notes for your reviewer:
The dual-cert cross-checks are the key addition beyond the original fix. In
ensureOldSignerCertificateRevoked, verifying the new cert is trusted before declaring the old one revoked prevents false positives when a KAS pod restart during revocation causes the old cert to become unreachable (connection refused), which looks like revocation but is actually the pod being down. Similarly, inensureNewSignerCertificatePropagated, verifying the old cert is also still trusted prevents declaring propagation complete when a pod is mid-reload and happens to accept the new cert but hasn't finished loading the full trust bundle.Checklist: