OCPBUGS-60997: targetconfigcontroller: make sure extension-apiserver-authentication has necessary annotations #1909
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@vrutkovs: This pull request references Jira Issue OCPBUGS-60997, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an asset manifest for kube-system/extension-apiserver-authentication and updates the target config controller to load that asset, compare required annotations with the live ConfigMap, and reconcile annotations when they differ; missing ConfigMap is treated as a no-op. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TCC as TargetConfigController
participant Assets as AssetLoader
participant KAPI as Kubernetes API (ConfigMaps)
Note over Assets,TCC: New asset: extension-apiserver-authentication-cm.yaml
TCC->>Assets: Load asset → required annotations
Assets-->>TCC: Required annotations
TCC->>KAPI: GET ConfigMap "extension-apiserver-authentication" (kube-system)
alt NotFound
KAPI-->>TCC: 404 NotFound
TCC->>TCC: Return (no-op)
else Found
KAPI-->>TCC: Existing ConfigMap (with annotations)
alt Annotations differ
TCC->>KAPI: UPDATE ConfigMap (apply required annotations)
KAPI-->>TCC: Update result / Error
opt Update error
TCC->>TCC: Record error (configmap/extension-apiserver-authentication)
end
else Annotations match
TCC->>TCC: No-op
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[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: This pull request references Jira Issue OCPBUGS-60997, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml (1)
6-8: Tighten description wording.Minor clarity improvement; no behavioral impact.
- "openshift.io/description": "CA holding the root certificate bundle to use to verify client certificates on incoming requests" + "openshift.io/description": "CA bundle used to verify client certificates on incoming requests"pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
8-8: Drop unused reflect import after targeted merge refactor.- "reflect"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml(1 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (2)
vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/core.go (1)
ReadConfigMapV1OrDie(20-26)bindata/assets.go (1)
MustAsset(17-24)
🔇 Additional comments (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
241-244: Integration looks good; errors aggregated under a clear key.
bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml
Outdated
Show resolved
Hide resolved
vrutkovs
force-pushed
the
extension-apiserver-authentication-ownership
branch
2 times, most recently
from
35fa693 to
da6a38e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml (2)
8-8: Fix minor grammar in description annotation.Tighten wording and remove “to use to”.
- "openshift.io/description": "CA holding the root certificate bundle to use to verify client certificates on incoming requests" + "openshift.io/description": "CA holding the root certificate bundle used to verify client certificates on incoming requests"
1-1: Optional: add a header comment to document purpose (avoids accidental apply).Clarify this asset is a spec for annotations only and not meant to create/replace the CM data.
+# NOTE: This asset defines the required annotations for the live +# kube-system/extension-apiserver-authentication ConfigMap. It is not +# applied directly; the operator reads these annotations and reconciles +# them on the existing ConfigMap created by kube-apiserver. apiVersion: v1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml(1 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/targetconfigcontroller/targetconfigcontroller.go
🔇 Additional comments (1)
bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml (1)
4-5: Namespace/name now correct for the live ConfigMap.Matches kube-system/extension-apiserver-authentication.
vrutkovs
force-pushed
the
extension-apiserver-authentication-ownership
branch
from
da6a38e to
6272754
Compare
| cm.Annotations = required.Annotations | ||
| updatedCM, err := cmCLient.Update(ctx, cm, metav1.UpdateOptions{}) | ||
| resourcehelper.ReportUpdateEvent(recorder, updatedCM, err) | ||
| return err |
There was a problem hiding this comment.
this line 534 return err maybe redundant as line 537 can return this err too
There was a problem hiding this comment.
Wait, not this would be a different err as it would be shadowed
There was a problem hiding this comment.
yes, it might be shadowed when using the same err, my bad
vrutkovs
force-pushed
the
extension-apiserver-authentication-ownership
branch
4 times, most recently
from
a836b12 to
85260ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
515-549: Merge-only annotations and tolerate NotFound: implemented as requested; minor nitsThe helper correctly preserves unrelated annotations and returns nil on NotFound. Two small cleanups:
- Avoid marking modified just for initializing the map; let key updates drive the change.
- Use cmClient for consistent naming and return nil explicitly at the end for readability.
Apply:
func ensureKubeAPIServerExtensionAuthenticationCA(ctx context.Context, client coreclientv1.CoreV1Interface, recorder events.Recorder) error { required := resourceread.ReadConfigMapV1OrDie(bindata.MustAsset("assets/kube-apiserver/extension-apiserver-authentication-cm.yaml")) - cmCLient := client.ConfigMaps("kube-system") + cmClient := client.ConfigMaps("kube-system") - cm, err := cmCLient.Get(ctx, "extension-apiserver-authentication", metav1.GetOptions{}) + cm, err := cmClient.Get(ctx, "extension-apiserver-authentication", metav1.GetOptions{}) if err != nil { // kube-apiserver creates this CM; don't degrade while waiting. if apierrors.IsNotFound(err) { return nil } return err } // Ensure that the config map is updated with the required annotations modified := false if cm.Annotations == nil { cm.Annotations = make(map[string]string) - modified = true } for key, expected := range required.Annotations { if actual, ok := cm.Annotations[key]; !ok || actual != expected { cm.Annotations[key] = expected modified = true } } if modified { - updatedCM, err := cmCLient.Update(ctx, cm, metav1.UpdateOptions{}) + updatedCM, err := cmClient.Update(ctx, cm, metav1.UpdateOptions{}) resourcehelper.ReportUpdateEvent(recorder, updatedCM, err) return err } - return err + return nil }
🧹 Nitpick comments (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
542-545: Optional: include event details about what changedPassing a short detail string improves observability.
- updatedCM, err := cmClient.Update(ctx, cm, metav1.UpdateOptions{}) - resourcehelper.ReportUpdateEvent(recorder, updatedCM, err) + updatedCM, err := cmClient.Update(ctx, cm, metav1.UpdateOptions{}) + resourcehelper.ReportUpdateEvent(recorder, updatedCM, err, "reconciled required annotations")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml(1 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (3)
vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/core.go (1)
ReadConfigMapV1OrDie(20-26)bindata/assets.go (1)
MustAsset(17-24)vendor/github.com/openshift/library-go/pkg/operator/resource/resourcehelper/event_helpers.go (1)
ReportUpdateEvent(21-31)
🔇 Additional comments (2)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (2)
240-244: Add reconciliation for extension-apiserver-authentication: LGTMProperly wired into createTargetConfig with error aggregation consistent with existing patterns.
516-517: Asset presence and annotations verified
bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml includes the requiredopenshift.io/owning-componentandopenshift.io/descriptionannotations.
vrutkovs
force-pushed
the
extension-apiserver-authentication-ownership
branch
2 times, most recently
from
3401378 to
c4aaf92
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| errors = append(errors, fmt.Errorf("%q: %v", "configmap/trusted-ca-bundle", err)) | ||
| } | ||
|
|
||
| err = ensureKubeAPIServerExtensionAuthenticationCA(ctx, c.kubeClient.CoreV1(), recorder) |
There was a problem hiding this comment.
The new logic is not covered by unit tests. Adding a unit test for ensureKubeAPIServerExtensionAuthenticationCA would be beneficial to verify its behavior under different conditions
Example Test Cases to Add:
- ConfigMap does not exist: The function should do nothing and return no error.
- ConfigMap exists, no annotations: The function should add the required annotations.
- ConfigMap exists, partial/unrelated annotations: The function should add the required annotations without removing existing ones.
- ConfigMap exists, incorrect annotation value: The function should correct the value of the annotation.
- ConfigMap exists, all annotations correct: The function should do nothing.
There was a problem hiding this comment.
- This already exists -
configmap not found (Get error) - Done -
configmap exists but missing annotations, update succeeds - Added
unrelated annotations are not removed - There is one -
configmap exists but missing annotations, update succeeds- which is almost matching, but addedconfigmap exists with incorrect OpenShiftComponent annotation, update succeedsjust to be sure - This exists already -
configmap exists with correct annotations, no update needed
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
vrutkovs
force-pushed
the
extension-apiserver-authentication-ownership
branch
2 times, most recently
from
e22fa30 to
d74958d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (2)
538-544: Emit an update event and return the Update error directly.This keeps behavior consistent with other helpers (e.g., CombineCABundle* callers) and improves observability.
Apply:
- if modified { - if _, err := cmClient.Update(ctx, cm, metav1.UpdateOptions{}); err != nil { - recorder.Warningf("AnnotationUpdateFailed", "Failed to update annotations on configmap kube-system/extension-apiserver-authentication: %v", err) - return err - } - return nil - } + if modified { + updatedCM, err := cmClient.Update(ctx, cm, metav1.UpdateOptions{}) + resourcehelper.ReportUpdateEvent(recorder, updatedCM, err) + return err + }
519-523: Don’t swallow non-NotFound errors from Get; return real failures.Unconditionally returning nil on any Get error hides auth/permission and transient API failures, preventing degradation and retries. Only tolerate NotFound as non-degrading.
Apply:
- cm, err := cmClient.Get(ctx, "extension-apiserver-authentication", metav1.GetOptions{}) - if err != nil { - // kube-apiserver creates this CM; don't degrade while waiting. - return nil - } + cm, err := cmClient.Get(ctx, "extension-apiserver-authentication", metav1.GetOptions{}) + if err != nil { + // kube-apiserver creates this CM; don't degrade while waiting. + if apierrors.IsNotFound(err) { + return nil + } + return err + }
🧹 Nitpick comments (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
517-517: Use the canonical namespace constant.Tiny polish: prefer metav1.NamespaceSystem over a raw "kube-system" literal.
Apply:
- cmClient := client.ConfigMaps("kube-system") + cmClient := client.ConfigMaps(metav1.NamespaceSystem)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml(1 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller.go(2 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go (6)
vendor/github.com/openshift/library-go/pkg/operator/events/recorder_in_memory.go (1)
NewInMemoryRecorder(36-42)vendor/k8s.io/client-go/kubernetes/fake/clientset_generated.go (1)
NewSimpleClientset(150-176)vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/core.go (1)
ReadConfigMapV1OrDie(20-26)bindata/assets.go (1)
MustAsset(17-24)vendor/k8s.io/client-go/testing/actions.go (1)
GetAction(536-539)vendor/k8s.io/client-go/testing/fake.go (1)
Fake(32-47)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (2)
vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/core.go (1)
ReadConfigMapV1OrDie(20-26)bindata/assets.go (1)
MustAsset(17-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
240-243: Wire-up looks good.The new reconciliation step is properly added to the target-config flow and participates in degradation aggregation.
pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go (1)
32-39: Imports look correct.bindata/resourceread are needed for asset-driven expectations; client-go testing import is used for action assertions and reactors.
vrutkovs
force-pushed
the
extension-apiserver-authentication-ownership
branch
from
d74958d to
76c75bd
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go (3)
1225-1229: Add a subtest for non-NotFound Get error propagation.Covers the missing error path suggested earlier; ensures Get failures (not IsNotFound) bubble up.
Apply:
func TestEnsureKubeAPIServerExtensionAuthenticationCA(t *testing.T) { ctx := context.Background() recorder := events.NewInMemoryRecorder("test", clock.RealClock{}) + t.Run("get failure (non-NotFound) returns error", func(t *testing.T) { + client := fake.NewSimpleClientset() + client.Fake.PrependReactor("get", "configmaps", func(action clientgotesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("boom") + }) + err := ensureKubeAPIServerExtensionAuthenticationCA(ctx, client.CoreV1(), recorder) + if err == nil || !strings.Contains(err.Error(), "boom") { + t.Fatalf("expected non-NotFound get error to propagate, got: %v", err) + } + })
1239-1260: Assert all required annotations, not just owning-component.Strengthen this test to validate every required annotation from the asset (e.g., owning-component and description).
updatedCM, err := client.CoreV1().ConfigMaps("kube-system").Get(ctx, "extension-apiserver-authentication", metav1.GetOptions{}) if err != nil { t.Fatalf("failed to get updated configmap: %v", err) } - if updatedCM.Annotations == nil || updatedCM.Annotations[annotations.OpenShiftComponent] != "kube-apiserver" { - t.Fatalf("expected annotation not set, got: %v", updatedCM.Annotations) - } + required := resourceread.ReadConfigMapV1OrDie(bindata.MustAsset("assets/kube-apiserver/extension-apiserver-authentication-cm.yaml")) + for k, v := range required.Annotations { + if updatedCM.Annotations[k] != v { + t.Fatalf("expected %q=%q, got: %v", k, v, updatedCM.Annotations) + } + }
1355-1383: Also assert all required annotations after correcting incorrect owning-component.Ensure description (and any future required keys) is reconciled too.
- // Verify the OpenShiftComponent annotation is updated to the correct value - if updatedCM.Annotations == nil || updatedCM.Annotations[annotations.OpenShiftComponent] != "kube-apiserver" { - t.Errorf("expected annotation %s=kube-apiserver, got: %v", annotations.OpenShiftComponent, updatedCM.Annotations) - } + required := resourceread.ReadConfigMapV1OrDie(bindata.MustAsset("assets/kube-apiserver/extension-apiserver-authentication-cm.yaml")) + for k, v := range required.Annotations { + if updatedCM.Annotations[k] != v { + t.Fatalf("expected %q=%q, got: %v", k, v, updatedCM.Annotations) + } + }
🧹 Nitpick comments (2)
pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go (2)
1278-1278: Nit: fix comment typo.- // Check that client only did one action) + // Check that client only did one action
1316-1353: Avoid mutating the asset’s annotations map when building expectations.Clone before merging unrelated keys to prevent accidental side effects.
- expectedAnnotations := required.Annotations - for k, v := range unrelatedAnnotations { - expectedAnnotations[k] = v - } + expectedAnnotations := make(map[string]string, len(required.Annotations)+len(unrelatedAnnotations)) + for k, v := range required.Annotations { + expectedAnnotations[k] = v + } + for k, v := range unrelatedAnnotations { + expectedAnnotations[k] = v + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml(1 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller.go(2 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml
- pkg/operator/targetconfigcontroller/targetconfigcontroller.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go (7)
vendor/github.com/openshift/library-go/pkg/operator/events/recorder_in_memory.go (1)
NewInMemoryRecorder(36-42)vendor/k8s.io/client-go/kubernetes/fake/clientset_generated.go (1)
NewSimpleClientset(150-176)vendor/github.com/openshift/api/annotations/annotations.go (1)
OpenShiftComponent(33-33)vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/core.go (1)
ReadConfigMapV1OrDie(20-26)bindata/assets.go (1)
MustAsset(17-24)vendor/k8s.io/client-go/testing/actions.go (1)
GetAction(536-539)vendor/k8s.io/client-go/testing/fake.go (1)
Fake(32-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go (4)
28-40: Imports LGTM.New imports (cmp, bindata, resourceread, clientgotesting) are used appropriately.
1230-1238: NotFound path test LGTM.Correctly asserts nil error when the ConfigMap is absent.
1261-1293: Idempotency + action count LGTM.Good that you assert no update when annotations match and verify a single Get action.
1295-1314: Update failure propagation LGTM.The reactor-based failure path is covered correctly.
vrutkovs
force-pushed
the
extension-apiserver-authentication-ownership
branch
from
76c75bd to
f06e6e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go (1)
1316-1356: Good: avoid mutating source annotations map by deep-copying.This resolves the prior aliasing bug reported by cursor[bot]; using a new map prevents unintended side effects.
🧹 Nitpick comments (3)
pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go (3)
1230-1237: Add test: non-NotFound Get error should propagate.You cover NotFound (nil) and Update error, but not Get errors other than NotFound. Add a subtest to ensure errors bubble up.
Apply:
func TestEnsureKubeAPIServerExtensionAuthenticationCA(t *testing.T) { ctx := context.Background() recorder := events.NewInMemoryRecorder("test", clock.RealClock{}) + t.Run("get failure (non-NotFound) returns error", func(t *testing.T) { + client := fake.NewSimpleClientset() + client.Fake.PrependReactor("get", "configmaps", func(action clientgotesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("boom") + }) + err := ensureKubeAPIServerExtensionAuthenticationCA(ctx, client.CoreV1(), recorder) + if err == nil || !strings.Contains(err.Error(), "boom") { + t.Fatalf("expected non-NotFound get error to propagate, got: %v", err) + } + })
1239-1259: Consider asserting all required annotations, not just owning-component.You already validate full annotation set in another subtest; for consistency, reuse the same check here via a small helper.
You could add:
func assertHasRequiredAnnotations(t *testing.T, got map[string]string) { t.Helper() required := resourceread.ReadConfigMapV1OrDie(bindata.MustAsset("assets/kube-apiserver/extension-apiserver-authentication-cm.yaml")) if diff := cmp.Diff(required.Annotations, filterMap(got, required.Annotations)); diff != "" { t.Fatalf("required annotations mismatch (-want +got):\n%s", diff) } } func filterMap(got, required map[string]string) map[string]string { out := map[string]string{} for k := range required { out[k] = got[k] } return out }Then call
assertHasRequiredAnnotations(t, updatedCM.Annotations).
1278-1278: Fix trailing parenthesis in comment.Minor polish.
- // Check that client only did one action) + // Check that client only did one action
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml(1 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller.go(2 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml
- pkg/operator/targetconfigcontroller/targetconfigcontroller.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go (6)
vendor/github.com/openshift/library-go/pkg/operator/events/recorder_in_memory.go (1)
NewInMemoryRecorder(36-42)vendor/k8s.io/client-go/kubernetes/fake/clientset_generated.go (1)
NewSimpleClientset(150-176)vendor/github.com/openshift/api/annotations/annotations.go (1)
OpenShiftComponent(33-33)vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/core.go (1)
ReadConfigMapV1OrDie(20-26)bindata/assets.go (1)
MustAsset(17-24)vendor/k8s.io/client-go/testing/actions.go (1)
GetAction(536-539)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go (2)
29-40: Imports look correct and necessary.New imports are used (cmp for diffs, bindata/resourceread for assets, clientgotesting for reactors). No issues.
1358-1385: LGTM: incorrect owning-component is reconciled to kube-apiserver.Behavior matches intent; update path is exercised and verified.
vrutkovs
force-pushed
the
extension-apiserver-authentication-ownership
branch
from
f06e6e6 to
c2e5d73
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go (2)
1250-1271: Strengthen assertion to validate all required annotations, not just owning-component.
You already verify the full set in another subtest; mirroring that here keeps each case self-contained.- if updatedCM.Annotations == nil || updatedCM.Annotations[annotations.OpenShiftComponent] != "kube-apiserver" { - t.Fatalf("expected annotation not set, got: %v", updatedCM.Annotations) - } + required := resourceread.ReadConfigMapV1OrDie(bindata.MustAsset("assets/kube-apiserver/extension-apiserver-authentication-cm.yaml")) + for k, v := range required.Annotations { + if updatedCM.Annotations[k] != v { + t.Fatalf("expected %q=%q, got: %v", k, v, updatedCM.Annotations) + } + }
1369-1397: Optional: add a parallel case for incorrect description annotation.
Covers drift on the description key explicitly.@@ t.Run("configmap exists with incorrect OpenShiftComponent annotation, update succeeds", func(t *testing.T) { @@ }) + + t.Run("configmap exists with incorrect description annotation, update succeeds", func(t *testing.T) { + required := resourceread.ReadConfigMapV1OrDie(bindata.MustAsset("assets/kube-apiserver/extension-apiserver-authentication-cm.yaml")) + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "extension-apiserver-authentication", + Namespace: "kube-system", + Annotations: map[string]string{ + "openshift.io/description": "wrong", + }, + }, + } + client := fake.NewSimpleClientset(cm) + err := ensureKubeAPIServerExtensionAuthenticationCA(ctx, client.CoreV1(), recorder) + if err != nil { + t.Fatalf("expected nil error, got: %v", err) + } + updatedCM, err := client.CoreV1().ConfigMaps("kube-system").Get(ctx, "extension-apiserver-authentication", metav1.GetOptions{}) + if err != nil { + t.Fatalf("failed to get updated configmap: %v", err) + } + if updatedCM.Annotations["openshift.io/description"] != required.Annotations["openshift.io/description"] { + t.Fatalf("expected description to be reconciled to %q, got: %v", required.Annotations["openshift.io/description"], updatedCM.Annotations) + } + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml(1 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller.go(2 hunks)pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- bindata/assets/kube-apiserver/extension-apiserver-authentication-cm.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/targetconfigcontroller/targetconfigcontroller.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go (6)
vendor/github.com/openshift/library-go/pkg/operator/events/recorder_in_memory.go (1)
NewInMemoryRecorder(36-42)vendor/k8s.io/client-go/kubernetes/fake/clientset_generated.go (1)
NewSimpleClientset(150-176)vendor/github.com/openshift/api/annotations/annotations.go (1)
OpenShiftComponent(33-33)vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/core.go (1)
ReadConfigMapV1OrDie(20-26)bindata/assets.go (1)
MustAsset(17-24)vendor/k8s.io/client-go/testing/actions.go (1)
GetAction(536-539)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go (6)
29-40: LGTM: new imports are all used and appropriate.
1230-1238: Good: NotFound path returns nil.
Covers the no-op reconcile case cleanly.
1239-1249: Good: non-NotFound Get error propagates.
Matches desired controller behavior.
1272-1304: Nice: no-op path asserts only a single Get action.
Prevents accidental writes.
1306-1325: Good: update error path is exercised via reactor.
Ensures surface of upstream failure.
1327-1367: Well done: unrelated annotations preserved and required map is copied, not mutated.
This avoids the prior pitfall of aliasing required.Annotations.
…has necessary annotations configmap kube-system/extension-apiserver-authentication is created by kube-apiserver, but it doesn't have ownership metadata. This commit updates target config controller to set necessary metadata (ownership and description)
vrutkovs
force-pushed
the
extension-apiserver-authentication-ownership
branch
from
c2e5d73 to
f7d6cfd
Compare
|
LGTM |
|
/retest-required |
openshift-ci-robot
added
jira/invalid-bug
|
@vrutkovs: This pull request references Jira Issue OCPBUGS-60997, which is invalid:
Comment In 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
|
@vrutkovs: This pull request references Jira Issue OCPBUGS-60997, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In 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. |
|
@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. |
configmap kube-system/extension-apiserver-authentication is created by kube-apiserver, but it doesn't have ownership metadata. This commit updates target config controller to set necessary metadata (ownership and description)
Now this CA has ownership and description set:
Summary by CodeRabbit
Bug Fixes
Chores
Tests