OCPBUGS-84254: Redeploy console pods upon cert rotation#1142
OCPBUGS-84254: Redeploy console pods upon cert rotation#1142jhadvig wants to merge 1 commit intoopenshift:release-4.20from
Conversation
openshift-ci-robot
added
jira/severity-moderate
WalkthroughThe changes add support for tracking a console serving certificate secret across the operator. The operator now watches the certificate resource, retrieves it during sync, and annotates the deployment with its resource version for change detection. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@jhadvig: This pull request references Jira Issue OCPBUGS-63502, 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
added
the
approved
jhadvig
changed the title
|
@jhadvig: This pull request references Jira Issue OCPBUGS-84254, 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. |
|
/retest |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 the current code and only fix it if needed.
Inline comments:
In `@pkg/console/subresource/deployment/deployment_test.go`:
- Around line 535-547: The test's args struct declares an unused field
authnConfig; remove the authnConfig field from the type args declaration and any
test case literals that set authnConfig so the struct and its usages no longer
reference it (look for the type args declaration and test case constructions in
deployment_test.go, e.g., where an args literal is created) to satisfy
golangci-lint's unused-field complaint.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cd6dfe95-1fff-4b4a-8665-b7f90add88f0
📒 Files selected for processing (4)
pkg/console/operator/operator.gopkg/console/operator/sync_v400.gopkg/console/subresource/deployment/deployment.gopkg/console/subresource/deployment/deployment_test.go
| type args struct { | ||
| deployment *appsv1.Deployment | ||
| consoleConfigMap *corev1.ConfigMap | ||
| serviceCAConfigMap *corev1.ConfigMap | ||
| authServerCAConfigMap *corev1.ConfigMap | ||
| trustedCAConfigMap *corev1.ConfigMap | ||
| oAuthClientSecret *corev1.Secret | ||
| sessionSecret *corev1.Secret | ||
| proxyConfig *configv1.Proxy | ||
| infrastructureConfig *configv1.Infrastructure | ||
| authnConfig *configv1.Authentication | ||
| deployment *appsv1.Deployment | ||
| consoleConfigMap *corev1.ConfigMap | ||
| serviceCAConfigMap *corev1.ConfigMap | ||
| authServerCAConfigMap *corev1.ConfigMap | ||
| trustedCAConfigMap *corev1.ConfigMap | ||
| oAuthClientSecret *corev1.Secret | ||
| sessionSecret *corev1.Secret | ||
| consoleServingCertSecret *corev1.Secret | ||
| proxyConfig *configv1.Proxy | ||
| infrastructureConfig *configv1.Infrastructure | ||
| authnConfig *configv1.Authentication | ||
| } |
There was a problem hiding this comment.
Remove unused authnConfig field from test args.
authnConfig (Line 546) is never referenced and is flagged by golangci-lint as unused.
🧹 Proposed fix
type args struct {
deployment *appsv1.Deployment
consoleConfigMap *corev1.ConfigMap
serviceCAConfigMap *corev1.ConfigMap
authServerCAConfigMap *corev1.ConfigMap
trustedCAConfigMap *corev1.ConfigMap
oAuthClientSecret *corev1.Secret
sessionSecret *corev1.Secret
consoleServingCertSecret *corev1.Secret
proxyConfig *configv1.Proxy
infrastructureConfig *configv1.Infrastructure
- authnConfig *configv1.Authentication
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type args struct { | |
| deployment *appsv1.Deployment | |
| consoleConfigMap *corev1.ConfigMap | |
| serviceCAConfigMap *corev1.ConfigMap | |
| authServerCAConfigMap *corev1.ConfigMap | |
| trustedCAConfigMap *corev1.ConfigMap | |
| oAuthClientSecret *corev1.Secret | |
| sessionSecret *corev1.Secret | |
| proxyConfig *configv1.Proxy | |
| infrastructureConfig *configv1.Infrastructure | |
| authnConfig *configv1.Authentication | |
| deployment *appsv1.Deployment | |
| consoleConfigMap *corev1.ConfigMap | |
| serviceCAConfigMap *corev1.ConfigMap | |
| authServerCAConfigMap *corev1.ConfigMap | |
| trustedCAConfigMap *corev1.ConfigMap | |
| oAuthClientSecret *corev1.Secret | |
| sessionSecret *corev1.Secret | |
| consoleServingCertSecret *corev1.Secret | |
| proxyConfig *configv1.Proxy | |
| infrastructureConfig *configv1.Infrastructure | |
| authnConfig *configv1.Authentication | |
| } | |
| type args struct { | |
| deployment *appsv1.Deployment | |
| consoleConfigMap *corev1.ConfigMap | |
| serviceCAConfigMap *corev1.ConfigMap | |
| authServerCAConfigMap *corev1.ConfigMap | |
| trustedCAConfigMap *corev1.ConfigMap | |
| oAuthClientSecret *corev1.Secret | |
| sessionSecret *corev1.Secret | |
| consoleServingCertSecret *corev1.Secret | |
| proxyConfig *configv1.Proxy | |
| infrastructureConfig *configv1.Infrastructure | |
| } |
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 546-546: field authnConfig is unused
(unused)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/console/subresource/deployment/deployment_test.go` around lines 535 -
547, The test's args struct declares an unused field authnConfig; remove the
authnConfig field from the type args declaration and any test case literals that
set authnConfig so the struct and its usages no longer reference it (look for
the type args declaration and test case constructions in deployment_test.go,
e.g., where an args literal is created) to satisfy golangci-lint's unused-field
complaint.
There was a problem hiding this comment.
nit, and it is in the test file, not a big deal tho
There was a problem hiding this comment.
@Leo6Leo, agreed — it's a minor cosmetic issue in a test file with no functional impact. Feel free to leave it as-is if it's not worth the churn.
|
The tests seem flaky. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, Leo6Leo 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 |
|
/retest |
|
@jhadvig: The following test 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. |
3 participants
Root cause
When the
console-serving-certsecret is rotated (by service-ca operator), the console-operator does not detect the change. The console pods continuerunning with the old certificate mounted at
/var/serving-cert, leading to TLS errors and console unavailability until pods are manually restarted.Solution
Register an informer for the
console-serving-certsecret so the operator watches it for changes. When the secret'sresourceVersionchanges, theoperator stamps the new value as a
console.openshift.io/serving-cert-secret-versionannotation on the console Deployment, which triggers anautomatic pod rollout with the new certificate.
Backport of #1093
Test cases
Scenario 1: Verify secret informer registration
$ oc get secret console-serving-cert -n openshift-console -o jsonpath='{.metadata.resourceVersion}'
36546
$ oc get deployment console -n openshift-console -o jsonpath='{.spec.template.metadata.annotations.console.openshift.io/serving-cert-secret-version}'
36546
Expected: The deployment annotation matches the secret's
resourceVersion.Scenario 2: Automatic pod restart on certificate rotation
Delete the serving cert to simulate rotation
$ oc delete secret console-serving-cert -n openshift-console
Wait for service-ca to regenerate it, then verify
$ oc get secret console-serving-cert -n openshift-console -o jsonpath='{.metadata.resourceVersion}'
38672
$ oc get deployment console -n openshift-console -o jsonpath='{.spec.template.metadata.annotations.console.openshift.io/serving-cert-secret-version}'
38672
$ oc get pods -n openshift-console
NAME READY STATUS RESTARTS AGE
console-75d8bfcdf6-6w2wf 1/1 Running 0 55s <-- redeployed
console-75d8bfcdf6-9zn7z 1/1 Running 0 55s <-- redeployed
downloads-6d94488dc7-rnqts 1/1 Running 0 52m
downloads-6d94488dc7-tb4bz 1/1 Running 0 52m
Expected: The annotation updates to the new
resourceVersionand console pods are automatically redeployed. Downloads pods remain untouched./assign @Leo6Leo
Summary by CodeRabbit
Release Notes