nokubeconfig controller: use listers, filter informer events, avoid applies #1889
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrutkovs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
vrutkovs
force-pushed
the
nodekubeconfig-lister
branch
from
749cf43 to
686a4ea
Compare
|
/test e2e-short-cert-rotation |
vrutkovs
force-pushed
the
nodekubeconfig-lister
branch
3 times, most recently
from
338d30a to
a82359c
Compare
|
/test e2e-short-cert-rotation |
pkg/operator/nodekubeconfigcontroller/nodekubeconfigcontroller.go
Outdated
Show resolved
Hide resolved
…necessary applies
vrutkovs
force-pushed
the
nodekubeconfig-lister
branch
from
a82359c to
d2115d0
Compare
|
@vrutkovs: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
WalkthroughThis pull request refactors the NodeKubeconfigController to use filtered informers with selective event handling for specific ConfigMaps and Secrets. It introduces resource name constants, adds a pre-apply validation check comparing existing secrets against desired state using deep equality, and includes a new test case for annotation-only updates. Additionally, a parameter name typo is corrected. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
♻️ Duplicate comments (1)
pkg/operator/nodekubeconfigcontroller/nodekubeconfigcontroller.go (1)
183-193: Critical: Optimization is broken—StringData not converted to Data before comparison.The code populates only
requiredSecret.StringData(lines 157-169) but comparesrequiredSecret.DataagainstactualSecret.Data(line 188). SincerequiredSecret.Datais never initialized, it remains nil whileactualSecret.Data(fetched from the API lister) contains the actual decoded secret data. This causes the comparison to always fail, bypassing the optimization and triggering unnecessary applies every reconciliation.Convert
StringDatatoDatabefore the comparison:requiredSecret.Annotations[certrotation.CertificateNotAfterAnnotation] = systemAdminCredsSecret.Annotations[certrotation.CertificateNotAfterAnnotation] } + // Convert StringData to Data for comparison + if requiredSecret.Data == nil { + requiredSecret.Data = make(map[string][]byte) + } + for k, v := range requiredSecret.StringData { + requiredSecret.Data[k] = []byte(v) + } + requiredSecret.StringData = nil + actualSecret, err := secretLister.Secrets(requiredSecret.Namespace).Get(requiredSecret.Name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/operator/nodekubeconfigcontroller/nodekubeconfigcontroller.go(5 hunks)pkg/operator/nodekubeconfigcontroller/nodekubeconfigcontroller_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/operator/nodekubeconfigcontroller/nodekubeconfigcontroller.gopkg/operator/nodekubeconfigcontroller/nodekubeconfigcontroller_test.go
🔇 Additional comments (5)
pkg/operator/nodekubeconfigcontroller/nodekubeconfigcontroller.go (5)
7-7: LGTM: Import additions support new functionality.The
reflectimport enables the deep equality check for the pre-apply optimization, andapierrorsis used to distinguish NotFound errors from other errors.Also applies to: 24-24
30-34: LGTM: Constants improve maintainability.Extracting resource names into constants eliminates magic strings and makes the code more maintainable.
49-49: LGTM: Parameter name typo corrected.Fixed typo:
infrastuctureInformer→infrastructureInformer.
60-75: Verify the filter passes through non-ConfigMap/Secret events as intended.The filter logic returns
truefor all events except specific ConfigMap and Secret resources. This means:
- ConfigMaps: only
kube-apiserver-server-cain target namespace triggers sync- Secrets: only
node-system-admin-clientin operator namespace triggers sync- All other informer events (Infrastructure, OperatorClient) pass through
Ensure this is the intended behavior, as line 74 allows all non-ConfigMap/Secret events to trigger reconciliation.
121-121: LGTM: Improved error messages with constants and namespace/name format.Error messages now use constants and follow the
namespace/nameformat for better clarity.Also applies to: 128-128, 132-132, 135-135, 141-141
| { | ||
| name: "annotations only update", | ||
| existingObjects: []runtime.Object{ | ||
| &corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: "openshift-kube-apiserver", | ||
| Name: "kube-apiserver-server-ca", | ||
| }, | ||
| Data: map[string]string{ | ||
| "ca-bundle.crt": "kube-apiserver-server-ca certificate", | ||
| }, | ||
| }, | ||
| &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: "openshift-kube-apiserver-operator", | ||
| Name: "node-system-admin-client", | ||
| Annotations: map[string]string{ | ||
| certrotation.CertificateNotBeforeAnnotation: certNotBefore, | ||
| certrotation.CertificateNotAfterAnnotation: certNotAfter, | ||
| }, | ||
| }, | ||
| Data: map[string][]byte{ | ||
| "tls.crt": []byte(publicKey), | ||
| "tls.key": []byte(privateKey), | ||
| }, | ||
| }, | ||
| &corev1.Secret{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| APIVersion: "v1", | ||
| Kind: "Secret", | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: "openshift-kube-apiserver", | ||
| Name: "node-kubeconfigs", | ||
| Annotations: map[string]string{ | ||
| annotations.OpenShiftComponent: "kube-apiserver", | ||
| certrotation.CertificateNotBeforeAnnotation: "some-old-not-before", | ||
| certrotation.CertificateNotAfterAnnotation: "some-old-not-after", | ||
| }, | ||
| }, | ||
| Data: map[string][]byte{ | ||
| "localhost.kubeconfig": generateKubeConfig("localhost", "https://localhost:6443"), | ||
| "localhost-recovery.kubeconfig": generateKubeConfig("localhost-recovery", "https://localhost:6443"), | ||
| "lb-ext.kubeconfig": generateKubeConfig("lb-ext", lbExtServer), | ||
| "lb-int.kubeconfig": generateKubeConfig("lb-int", lbIntServer), | ||
| }, | ||
| }, | ||
| }, | ||
| infrastructure: &configv1.Infrastructure{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: "", | ||
| Name: "cluster", | ||
| }, | ||
| Status: configv1.InfrastructureStatus{ | ||
| APIServerURL: lbExtServer, | ||
| APIServerInternalURL: lbIntServer, | ||
| }, | ||
| }, | ||
| expectedErr: nil, | ||
| expectedActions: []clienttesting.Action{ | ||
| clienttesting.DeleteActionImpl{ | ||
| ActionImpl: clienttesting.ActionImpl{ | ||
| Namespace: "openshift-kube-apiserver", | ||
| Verb: "delete", | ||
| Resource: corev1.SchemeGroupVersion.WithResource("secrets"), | ||
| }, | ||
| Name: "node-kubeconfigs", | ||
| }, | ||
| clienttesting.CreateActionImpl{ | ||
| ActionImpl: clienttesting.ActionImpl{ | ||
| Namespace: "openshift-kube-apiserver", | ||
| Verb: "create", | ||
| Resource: corev1.SchemeGroupVersion.WithResource("secrets"), | ||
| }, | ||
| Object: &corev1.Secret{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| APIVersion: "v1", | ||
| Kind: "Secret", | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: "openshift-kube-apiserver", | ||
| Name: "node-kubeconfigs", | ||
| Labels: map[string]string{}, | ||
| OwnerReferences: []metav1.OwnerReference{}, | ||
| Annotations: map[string]string{ | ||
| annotations.OpenShiftComponent: "kube-apiserver", | ||
| certrotation.CertificateNotBeforeAnnotation: certNotBefore, | ||
| certrotation.CertificateNotAfterAnnotation: certNotAfter, | ||
| }, | ||
| }, | ||
| Data: map[string][]byte{ | ||
| "localhost.kubeconfig": generateKubeConfig("localhost", "https://localhost:6443"), | ||
| "localhost-recovery.kubeconfig": generateKubeConfig("localhost-recovery", "https://localhost:6443"), | ||
| "lb-ext.kubeconfig": generateKubeConfig("lb-ext", lbExtServer), | ||
| "lb-int.kubeconfig": generateKubeConfig("lb-int", lbIntServer), | ||
| }, | ||
| Type: corev1.SecretTypeOpaque, | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Good test coverage for annotation updates, but missing no-op test case.
The new "annotations only update" test validates that annotation changes trigger a delete+recreate, which is correct.
However, to properly validate the pre-apply optimization introduced in this PR (lines 183-191 in nodekubeconfigcontroller.go), you should add a test case where the existing secret matches the desired secret exactly (same Data AND Annotations). In this case, the expected actions should be empty (no delete, no create, no update).
Add this test case after the current one:
{
name: "no-op when secret matches exactly",
existingObjects: []runtime.Object{
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: "openshift-kube-apiserver",
Name: "kube-apiserver-server-ca",
},
Data: map[string]string{
"ca-bundle.crt": "kube-apiserver-server-ca certificate",
},
},
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: "openshift-kube-apiserver-operator",
Name: "node-system-admin-client",
Annotations: map[string]string{
certrotation.CertificateNotBeforeAnnotation: certNotBefore,
certrotation.CertificateNotAfterAnnotation: certNotAfter,
},
},
Data: map[string][]byte{
"tls.crt": []byte(publicKey),
"tls.key": []byte(privateKey),
},
},
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: "openshift-kube-apiserver",
Name: "node-kubeconfigs",
Annotations: map[string]string{
annotations.OpenShiftComponent: "kube-apiserver",
certrotation.CertificateNotBeforeAnnotation: certNotBefore,
certrotation.CertificateNotAfterAnnotation: certNotAfter,
},
},
Data: map[string][]byte{
"localhost.kubeconfig": generateKubeConfig("localhost", "https://localhost:6443"),
"localhost-recovery.kubeconfig": generateKubeConfig("localhost-recovery", "https://localhost:6443"),
"lb-ext.kubeconfig": generateKubeConfig("lb-ext", lbExtServer),
"lb-int.kubeconfig": generateKubeConfig("lb-int", lbIntServer),
},
},
},
infrastructure: &configv1.Infrastructure{
ObjectMeta: metav1.ObjectMeta{
Name: "cluster",
},
Status: configv1.InfrastructureStatus{
APIServerURL: lbExtServer,
APIServerInternalURL: lbIntServer,
},
},
expectedErr: nil,
expectedActions: []clienttesting.Action{}, // No actions expected
},🤖 Prompt for AI Agents
In pkg/operator/nodekubeconfigcontroller/nodekubeconfigcontroller_test.go around
lines 368 to 468, add a new table-driven test case immediately after the
"annotations only update" case named "no-op when secret matches exactly" that
supplies existingObjects containing the same ConfigMap, the same
node-system-admin-client Secret, and a node-kubeconfigs Secret whose Data and
Annotations exactly match the desired output (use certNotBefore/certNotAfter for
annotations and the same generateKubeConfig outputs and lbExt/lbInt values),
with the same infrastructure object as the other tests; set expectedErr to nil
and expectedActions to an empty slice so the test asserts no
delete/create/update occurs.
3 participants
In order to make controller sync less often this commit reworks it: