ESO-237: Fix reconciliation blocked when managed labels are externally modified#153
Conversation
Signed-off-by: chiragkyal <ckyal@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@chiragkyal: This pull request references ESO-237 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. 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. |
|
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:
WalkthroughIntroduces a ChangesManaged Label Restoration
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over SetupWithManager,predicate.Funcs: Label removal detection
SetupWithManager->>predicate.Funcs: configure Update predicate
Note right of predicate.Funcs: allow if old OR new object has managed label
end
rect rgba(144, 238, 144, 0.5)
Note over Reconciler,UncachedClient: createWithFallback on AlreadyExists
Reconciler->>CachedClient: Create(desired)
CachedClient-->>Reconciler: AlreadyExists
Reconciler->>Reconciler: resolve GVK, remove obsolete annotations
Reconciler->>UncachedClient: UpdateWithRetry(desired with restored labels)
UncachedClient-->>Reconciler: nil
Reconciler->>eventRecorder: emit "restored" Reconciled event
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/external_secrets/configmap.go`:
- Around line 75-79: The fallback recovery path in the desiredConfigMap update
block does not preserve external annotations from the live ConfigMap. While the
code copies Data and BinaryData from actualConfigMap and removes obsolete
annotations via RemoveObsoleteAnnotations, it fails to merge back any external
annotations that exist on the live ConfigMap before calling UpdateWithRetry.
Merge the annotations from actualConfigMap into desiredConfigMap (preserving
external annotations that aren't managed by the controller) before the
UpdateWithRetry call. Additionally, extend the regression test in
pkg/controller/external_secrets/configmap_test.go to assert that external
annotations are preserved when the AlreadyExists recovery update path is
triggered.
🪄 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: 2ed66a5d-9c8d-4e45-a044-2d82020f48f7
📒 Files selected for processing (20)
pkg/controller/external_secrets/certificate.gopkg/controller/external_secrets/certificate_test.gopkg/controller/external_secrets/configmap.gopkg/controller/external_secrets/configmap_test.gopkg/controller/external_secrets/controller.gopkg/controller/external_secrets/deployments.gopkg/controller/external_secrets/networkpolicy.gopkg/controller/external_secrets/networkpolicy_test.gopkg/controller/external_secrets/rbacs.gopkg/controller/external_secrets/rbacs_test.gopkg/controller/external_secrets/secret.gopkg/controller/external_secrets/secret_test.gopkg/controller/external_secrets/service_test.gopkg/controller/external_secrets/serviceaccounts.gopkg/controller/external_secrets/serviceaccounts_test.gopkg/controller/external_secrets/services.gopkg/controller/external_secrets/utils.gopkg/controller/external_secrets/utils_test.gopkg/controller/external_secrets/validatingwebhook.gopkg/controller/external_secrets/validatingwebhook_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #153 +/- ##
==========================================
- Coverage 46.69% 46.67% -0.02%
==========================================
Files 29 29
Lines 4420 4486 +66
==========================================
+ Hits 2064 2094 +30
- Misses 2054 2089 +35
- Partials 302 303 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bharath-b-rh
left a comment
bharath-b-rh
left a comment
There was a problem hiding this comment.
LGTM. Just few suggestions.
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/controller/external_secrets/configmap.go (2)
72-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve external annotations before patching metadata (AlreadyExists recovery path).
The current code patches metadata with only the desired annotations from
desiredConfigMap. Since MergePatch replaces the entire annotations map (RFC 7386), any external annotations added by other controllers or users are removed. Fetch the existing ConfigMap and merge its annotations before callingpatchConfigMapMetadata.Suggested fix
r.log.V(1).Info("trusted CA bundle ConfigMap exists on API server but absent from label-filtered cache, patching metadata", "name", configMapName) + actualConfigMap := &corev1.ConfigMap{} + if getErr := r.UncachedClient.Get(r.ctx, client.ObjectKeyFromObject(desiredConfigMap), actualConfigMap); getErr != nil { + return common.FromClientError(getErr, "failed to fetch existing %s trusted CA bundle ConfigMap", configMapName) + } + mergedAnnotations := maps.Clone(actualConfigMap.GetAnnotations()) + if mergedAnnotations == nil { + mergedAnnotations = map[string]string{} + } + for k, v := range desiredConfigMap.GetAnnotations() { + mergedAnnotations[k] = v + } + desiredConfigMap.SetAnnotations(mergedAnnotations) common.RemoveObsoleteAnnotations(desiredConfigMap, resourceMetadata) if patchErr := r.patchConfigMapMetadata(desiredConfigMap, r.UncachedClient); patchErr != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/external_secrets/configmap.go` around lines 72 - 76, Before calling patchConfigMapMetadata in the AlreadyExists recovery path, fetch the existing ConfigMap from the API server to retrieve its current annotations. Merge the existing ConfigMap's annotations with the desired annotations in desiredConfigMap to preserve any external annotations added by other controllers or users, then pass the merged ConfigMap to patchConfigMapMetadata. This prevents MergePatch (RFC 7386) from replacing the entire annotations map and losing external annotations.
87-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve external annotations before patching metadata (exists + metadata-modified path).
Similar to the AlreadyExists path, this code replaces the entire annotations map when patching. External annotations from
existingConfigMapshould be merged intodesiredConfigMapbefore patching to avoid losing metadata managed by other controllers.Suggested fix
if exist && common.ObjectMetadataModified(desiredConfigMap, existingConfigMap, &resourceMetadata) { r.log.V(1).Info("trusted CA bundle ConfigMap has been modified, patching metadata to desired state", "name", configMapName) + mergedAnnotations := maps.Clone(existingConfigMap.GetAnnotations()) + if mergedAnnotations == nil { + mergedAnnotations = map[string]string{} + } + for k, v := range desiredConfigMap.GetAnnotations() { + mergedAnnotations[k] = v + } + desiredConfigMap.SetAnnotations(mergedAnnotations) common.RemoveObsoleteAnnotations(desiredConfigMap, resourceMetadata) if err := r.patchConfigMapMetadata(desiredConfigMap, r.CtrlClient); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/external_secrets/configmap.go` around lines 87 - 91, When patching the ConfigMap metadata in the metadata-modified path, external annotations from the existingConfigMap are being lost. Before calling common.RemoveObsoleteAnnotations on desiredConfigMap, merge the annotations from existingConfigMap into desiredConfigMap to preserve annotations managed by other controllers. This ensures that only obsolete annotations are removed while external annotations are retained, similar to the handling in the AlreadyExists path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@pkg/controller/external_secrets/configmap.go`:
- Around line 72-76: Before calling patchConfigMapMetadata in the AlreadyExists
recovery path, fetch the existing ConfigMap from the API server to retrieve its
current annotations. Merge the existing ConfigMap's annotations with the desired
annotations in desiredConfigMap to preserve any external annotations added by
other controllers or users, then pass the merged ConfigMap to
patchConfigMapMetadata. This prevents MergePatch (RFC 7386) from replacing the
entire annotations map and losing external annotations.
- Around line 87-91: When patching the ConfigMap metadata in the
metadata-modified path, external annotations from the existingConfigMap are
being lost. Before calling common.RemoveObsoleteAnnotations on desiredConfigMap,
merge the annotations from existingConfigMap into desiredConfigMap to preserve
annotations managed by other controllers. This ensures that only obsolete
annotations are removed while external annotations are retained, similar to the
handling in the AlreadyExists path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5cec64ab-709d-46c7-8a97-89df9e9e6c42
📒 Files selected for processing (13)
pkg/controller/external_secrets/certificate_test.gopkg/controller/external_secrets/configmap.gopkg/controller/external_secrets/configmap_test.gopkg/controller/external_secrets/controller.gopkg/controller/external_secrets/networkpolicy_test.gopkg/controller/external_secrets/rbacs_test.gopkg/controller/external_secrets/secret_test.gopkg/controller/external_secrets/service_test.gopkg/controller/external_secrets/serviceaccounts_test.gopkg/controller/external_secrets/test_utils.gopkg/controller/external_secrets/utils.gopkg/controller/external_secrets/utils_test.gopkg/controller/external_secrets/validatingwebhook_test.go
✅ Files skipped from review due to trivial changes (4)
- pkg/controller/external_secrets/certificate_test.go
- pkg/controller/external_secrets/networkpolicy_test.go
- pkg/controller/external_secrets/rbacs_test.go
- pkg/controller/external_secrets/secret_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/controller/external_secrets/service_test.go
- pkg/controller/external_secrets/utils.go
- pkg/controller/external_secrets/utils_test.go
- pkg/controller/external_secrets/controller.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/e2e_test.go`:
- Around line 1069-1152: Add meaningful failure messages to all assertions
within the Eventually blocks across the three restoration test cases
(ServiceAccount, Role, and Deployment). For each error assertion lacking a
message (g.Expect(err).NotTo(HaveOccurred())), add a diagnostic message like
"failed to get ServiceAccount" or "failed to get Role". For each label assertion
lacking a message (g.Expect(resource.Labels).To(HaveKeyWithValue(...))), add a
message like "operator should restore app=external-secrets on ServiceAccount
saName" or similar for Role and Deployment. Apply these messages consistently
across all three test functions to ensure CI failures are easily triageable.
🪄 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: 6c9b66ef-791e-4c5a-a8b5-de4d4b65bf7a
📒 Files selected for processing (1)
test/e2e/e2e_test.go
| Eventually(func(g Gomega) { | ||
| sa, err := clientset.CoreV1().ServiceAccounts(operandNamespace).Get(ctx, saName, metav1.GetOptions{}) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(sa.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue)) | ||
| }, time.Minute, 5*time.Second).Should(Succeed()) | ||
|
|
||
| By("Removing the managed label from the ServiceAccount") | ||
| Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
| sa, err := clientset.CoreV1().ServiceAccounts(operandNamespace).Get(ctx, saName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| delete(sa.Labels, managedLabelKey) | ||
| _, err = clientset.CoreV1().ServiceAccounts(operandNamespace).Update(ctx, sa, metav1.UpdateOptions{}) | ||
| return err | ||
| })).To(Succeed(), "should remove the managed label") | ||
|
|
||
| By("Waiting for operator to restore the managed label") | ||
| Eventually(func(g Gomega) { | ||
| sa, err := clientset.CoreV1().ServiceAccounts(operandNamespace).Get(ctx, saName, metav1.GetOptions{}) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(sa.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue), | ||
| "operator should restore app=external-secrets on ServiceAccount %s", saName) | ||
| }, 2*time.Minute, 5*time.Second).Should(Succeed()) | ||
| }) | ||
|
|
||
| It("should restore the app=external-secrets label on a Role after external removal", func() { | ||
| roleName := "external-secrets" | ||
|
|
||
| By("Verifying Role has the managed label initially") | ||
| Eventually(func(g Gomega) { | ||
| role, err := clientset.RbacV1().Roles(operandNamespace).Get(ctx, roleName, metav1.GetOptions{}) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(role.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue)) | ||
| }, time.Minute, 5*time.Second).Should(Succeed()) | ||
|
|
||
| By("Removing the managed label from the Role") | ||
| Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
| role, err := clientset.RbacV1().Roles(operandNamespace).Get(ctx, roleName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| delete(role.Labels, managedLabelKey) | ||
| _, err = clientset.RbacV1().Roles(operandNamespace).Update(ctx, role, metav1.UpdateOptions{}) | ||
| return err | ||
| })).To(Succeed(), "should remove the managed label") | ||
|
|
||
| By("Waiting for operator to restore the managed label") | ||
| Eventually(func(g Gomega) { | ||
| role, err := clientset.RbacV1().Roles(operandNamespace).Get(ctx, roleName, metav1.GetOptions{}) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(role.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue), | ||
| "operator should restore app=external-secrets on Role %s", roleName) | ||
| }, 2*time.Minute, 5*time.Second).Should(Succeed()) | ||
| }) | ||
|
|
||
| It("should restore the app=external-secrets label on a Deployment after external removal", func() { | ||
| depName := "external-secrets" | ||
|
|
||
| By("Verifying Deployment has the managed label initially") | ||
| Eventually(func(g Gomega) { | ||
| dep, err := clientset.AppsV1().Deployments(operandNamespace).Get(ctx, depName, metav1.GetOptions{}) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(dep.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue)) | ||
| }, time.Minute, 5*time.Second).Should(Succeed()) | ||
|
|
||
| By("Removing the managed label from the Deployment") | ||
| Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
| dep, err := clientset.AppsV1().Deployments(operandNamespace).Get(ctx, depName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| delete(dep.Labels, managedLabelKey) | ||
| _, err = clientset.AppsV1().Deployments(operandNamespace).Update(ctx, dep, metav1.UpdateOptions{}) | ||
| return err | ||
| })).To(Succeed(), "should remove the managed label") | ||
|
|
||
| By("Waiting for operator to restore the managed label") | ||
| Eventually(func(g Gomega) { | ||
| dep, err := clientset.AppsV1().Deployments(operandNamespace).Get(ctx, depName, metav1.GetOptions{}) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(dep.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue), | ||
| "operator should restore app=external-secrets on Deployment %s", depName) | ||
| }, 2*time.Minute, 5*time.Second).Should(Succeed()) |
There was a problem hiding this comment.
Add meaningful failure messages to assertions in the new restoration specs.
Several new assertions omit diagnostic messages (for example g.Expect(err).NotTo(HaveOccurred())), which makes e2e failures harder to triage in CI. Please add explicit messages to the error and label assertions in these Eventually blocks.
As per coding guidelines, “Flag Ginkgo test assertions that lack meaningful failure messages. Assertions should include messages like 'failed to create test pod' to help diagnose what went wrong.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/e2e_test.go` around lines 1069 - 1152, Add meaningful failure
messages to all assertions within the Eventually blocks across the three
restoration test cases (ServiceAccount, Role, and Deployment). For each error
assertion lacking a message (g.Expect(err).NotTo(HaveOccurred())), add a
diagnostic message like "failed to get ServiceAccount" or "failed to get Role".
For each label assertion lacking a message
(g.Expect(resource.Labels).To(HaveKeyWithValue(...))), add a message like
"operator should restore app=external-secrets on ServiceAccount saName" or
similar for Role and Deployment. Apply these messages consistently across all
three test functions to ensure CI failures are easily triageable.
Source: Coding guidelines
| if !exist { | ||
| if err := r.Create(r.ctx, desired); err != nil { | ||
| return common.FromClientError(err, "failed to create %s secret resource", secretName) | ||
| if err := r.createWithFallback(desired, resourceMetadata, secretName); err != nil { |
There was a problem hiding this comment.
We will need to use the same logic as that of CNO co-managed ConfigMap for Secrets too.
There was a problem hiding this comment.
Could you please share your thought process behind this? The ConfigMap uses a metadata-only MergePatch because CNO co-manages its Data. Whereas the webhook TLS Secret has no such co-manager; its Data comes entirely from a static asset owned by this operator.
Could you clarify what specific scenario you have in mind? Is there an external controller that writes to the Secret's Data that we should be aware of? or I'm missing something
There was a problem hiding this comment.
The operator creates an empty secret when cert-manager is not configured, during this scenario, the cert-controller component of external-secrets will inject the TLS content to it which will be used by the webhook. The cert-controller expects the secret to be present and has only updates it.
There was a problem hiding this comment.
Thanks for the clarification. Updated in the latest commit.
| isManagedResource := func(object client.Object) bool { | ||
| return object.GetLabels() != nil && object.GetLabels()[requestEnqueueLabelKey] == requestEnqueueLabelValue | ||
| }) | ||
| } |
There was a problem hiding this comment.
| isManagedResource := func(object client.Object) bool { | |
| return object.GetLabels() != nil && object.GetLabels()[requestEnqueueLabelKey] == requestEnqueueLabelValue | |
| }) | |
| } | |
| isManagedResource := func(object client.Object) bool { | |
| admit := object.GetLabels() != nil && object.GetLabels()[requestEnqueueLabelKey] == requestEnqueueLabelValue | |
| if !admit { | |
| logIgnored(object) | |
| } | |
| return admit | |
| }) |
There was a problem hiding this comment.
I think it would cause confusion for UpdateFunc. If logging is inside isManagedResource, and ObjectOld doesn't match but ObjectNew does, we'd log "not of interest" for ObjectOld, then accept the event anyway. That's misleading.
There was a problem hiding this comment.
Yeah agree and we could improve it with an optional arg, whether log or not. My intention is to avoid repetitive code which will be flagged by linter. Or we could use the suggestion made on trust-manager as well, where we were passing objects as vargs and check if present in any of it.
Ref: openshift/cert-manager-operator#379 (comment)
There was a problem hiding this comment.
Yeah we can do like that also; thanks. Updated.
There was a problem hiding this comment.
Is the change missing from the commit or are we not making the change?
There was a problem hiding this comment.
It's already pushed I think, could you please check the latest changes.
| if tt.wantCreate && cached.CreateCallCount() == 0 { | ||
| t.Error("expected Create to be called, but it wasn't") | ||
| } | ||
| if tt.wantPatch && cached.PatchCallCount() == 0 && uncached.PatchCallCount() == 0 { |
There was a problem hiding this comment.
Should this be?
| if tt.wantPatch && cached.PatchCallCount() == 0 && uncached.PatchCallCount() == 0 { | |
| if tt.wantPatch && (cached.PatchCallCount() == 0 || uncached.PatchCallCount() == 0) { |
There was a problem hiding this comment.
Modified as per the latest changes.
|
cc: @snarayan-redhat for docs approval. Please help add release notes for the changes. |
|
/label docs-approved |
openshift-ci
Bot
added
the
docs-approved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controller/external_secrets/configmap_test.go (1)
248-255: ⚡ Quick winAdd a symmetric negative assertion for unexpected
Createcalls.The table asserts
Createwhen expected, but it doesn’t fail whenCreateis called unexpectedly in non-create scenarios.Suggested test hardening
if tt.wantCreate && cached.CreateCallCount() == 0 { t.Error("expected Create to be called, but it wasn't") } + if !tt.wantCreate && cached.CreateCallCount() > 0 { + t.Error("expected no Create call, but one was made") + } if tt.wantPatch && cached.PatchCallCount() == 0 { t.Error("expected Patch to be called, but it wasn't") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/external_secrets/configmap_test.go` around lines 248 - 255, The test currently has a negative assertion to verify that Patch is not called when unexpected (using !tt.wantPatch && cached.PatchCallCount() > 0), but lacks a symmetric negative assertion for Create calls. Add a similar negative assertion check after the positive Create assertion that verifies when tt.wantCreate is false and cached.CreateCallCount() is greater than zero, the test fails with an appropriate error message indicating that an unexpected Create call was made. This ensures symmetry in test validation for both Create and Patch operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 216-218: The E2E_GINKGO_LABEL_FILTER variable is currently running
tests labeled with Platform:Generic that also include Proxy:HTTP labels, which
require cluster-wide proxy configuration but are not gated on whether proxy is
actually configured. Modify the E2E_GINKGO_LABEL_FILTER value to exclude tests
with the Proxy:HTTP label from the default run, ensuring that proxy-dependent
tests are only executed when proxy configuration is explicitly provided or
enabled. Update the filter expression to use Ginkgo's label syntax to exclude or
restrict tests that have the Proxy:HTTP label requirement.
---
Nitpick comments:
In `@pkg/controller/external_secrets/configmap_test.go`:
- Around line 248-255: The test currently has a negative assertion to verify
that Patch is not called when unexpected (using !tt.wantPatch &&
cached.PatchCallCount() > 0), but lacks a symmetric negative assertion for
Create calls. Add a similar negative assertion check after the positive Create
assertion that verifies when tt.wantCreate is false and cached.CreateCallCount()
is greater than zero, the test fails with an appropriate error message
indicating that an unexpected Create call was made. This ensures symmetry in
test validation for both Create and Patch operations.
🪄 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: 7e8912d3-42f2-4836-b44c-4d4da948ec22
📒 Files selected for processing (14)
Makefilepkg/controller/external_secrets/certificate.gopkg/controller/external_secrets/configmap.gopkg/controller/external_secrets/configmap_test.gopkg/controller/external_secrets/deployments.gopkg/controller/external_secrets/networkpolicy.gopkg/controller/external_secrets/rbacs.gopkg/controller/external_secrets/secret.gopkg/controller/external_secrets/serviceaccounts.gopkg/controller/external_secrets/services.gopkg/controller/external_secrets/utils.gopkg/controller/external_secrets/utils_test.gopkg/controller/external_secrets/validatingwebhook.gotest/e2e/e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/controller/external_secrets/secret.go
- pkg/controller/external_secrets/rbacs.go
- pkg/controller/external_secrets/utils.go
Signed-off-by: chiragkyal <ckyal@redhat.com>
…d TLS data Signed-off-by: chiragkyal <ckyal@redhat.com>
|
/retest |
|
/test e2e-operator |
|
/lgtm /label px-approved Find the list of pre-merge tests performed for the changes here |
openshift-ci
Bot
added
px-approved
|
/hold |
openshift-ci
Bot
added
do-not-merge/hold
| // Resources intentionally excluded: | ||
| // - Deployment: Kubernetes adds deployment.kubernetes.io/revision on every rollout, | ||
| // so annotation events are suppressed to avoid infinite reconcile loops. | ||
| Context("Managed Annotation Restoration", Ordered, Label("Platform:AWS"), func() { |
There was a problem hiding this comment.
| Context("Managed Annotation Restoration", Ordered, Label("Platform:AWS"), func() { | |
| Context("Managed Annotation Restoration", Ordered, Label("Platform:Generic"), func() { |
There was a problem hiding this comment.
Do we have Platfrom:Generic filter applied? Otherwise the tests won't run.
…urces Signed-off-by: chiragkyal <ckyal@redhat.com>
|
@chiragkyal: 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. |
|
/lgtm |
|
/approve |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh, chiragkyal The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
5 participants
Description
When the
app=external-secretslabel on a managed resource is changedexternally, the resource disappears from the label-filtered informer
cache. Subsequent reconciliation attempts to create the resource,
receives AlreadyExists, and gets stuck in a permanent error loop.
Changes
createWithFallbackhelper that handles AlreadyExists by falling back to an uncached UpdateWithRetry, restoring the managed labels and annotations.Summary by CodeRabbit
New Features
Bug Fixes
Tests