ESO-396: Mount user configured trustedCABundle on external-secrets core controller#149
Open
bharath-b-rh wants to merge 2 commits into
Open
ESO-396: Mount user configured trustedCABundle on external-secrets core controller#149bharath-b-rh wants to merge 2 commits into
bharath-b-rh wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: This pull request references ESO-396 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 story 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (60)
📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (11)
WalkthroughAdds validated trusted CA bundle support, refactors controller cache/watch and reconcile flow, introduces a user-trusted-CA error kind, rewires deployment proxy and CA wiring, fixes certificate issuer lookup, and expands unit and e2e tests, docs, and constants. ChangesTrusted CA Bundle Feature Implementation
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRsSuggested labelsdocs-approved, lgtm Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh 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
…re controller Signed-off-by: Bharath B <bhb@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/external_secrets/controller.go (1)
314-322:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix watch registration: pointer-literal
switchcases never match
Inpkg/controller/external_secrets/controller.go,controllerManagedResourcesis a[]client.Objectholding pointer instances (e.g.,&appsv1.Deployment{},&corev1.Secret{}). Theswitch res { case &appsv1.Deployment{}: ... }compares pointer identities, so those case literals never match the slice elements and thedefaultbranch always runs.Suggested fix
for _, res := range controllerManagedResources { - switch res { - case &appsv1.Deployment{}: + switch res.(type) { + case *appsv1.Deployment: mgrBuilder.Watches(res, handler.EnqueueRequestsFromMapFunc(mapFunc), withIgnoreStatusUpdatePredicates) - case &corev1.Secret{}: + case *corev1.Secret: mgrBuilder.WatchesMetadata(res, handler.EnqueueRequestsFromMapFunc(mapFunc), builder.WithPredicates(predicate.LabelChangedPredicate{})) default: mgrBuilder.Watches(res, handler.EnqueueRequestsFromMapFunc(mapFunc), managedResourcePredicate) } }🤖 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/controller.go` around lines 314 - 322, The switch in controller.go is comparing pointer literal identities so cases like "&appsv1.Deployment{}" never match elements from controllerManagedResources; change the logic to type-switch or compare concrete types instead: iterate controllerManagedResources and use a type switch on the element (or reflect.TypeOf(res)) to detect *appsv1.Deployment and *corev1.Secret and then call mgrBuilder.Watches/mgrBuilder.WatchesMetadata with the appropriate predicates (preserving handler.EnqueueRequestsFromMapFunc(mapFunc), withIgnoreStatusUpdatePredicates, builder.WithPredicates(predicate.LabelChangedPredicate{}), and managedResourcePredicate).
🧹 Nitpick comments (2)
test/e2e/e2e_test.go (1)
401-403: ⚡ Quick winAvoid replacing the entire
ControllerConfigwhen toggling env overrides.These assignments wipe unrelated
ControllerConfigfields and can create cross-test drift. Update onlyComponentConfigson the copied CR.♻️ Proposed fix
- updatedCR.Spec.ControllerConfig = operatorv1alpha1.ControllerConfig{ - ComponentConfigs: applicableEnvConfigs, - } + updatedCR.Spec.ControllerConfig.ComponentConfigs = applicableEnvConfigs- updatedCR.Spec.ControllerConfig = operatorv1alpha1.ControllerConfig{ - ComponentConfigs: nil, - } + updatedCR.Spec.ControllerConfig.ComponentConfigs = nilAlso applies to: 451-454
🤖 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 401 - 403, The test currently replaces the whole updatedCR.Spec.ControllerConfig with a new ControllerConfig which wipes other fields; instead assign only ComponentConfigs to preserve unrelated fields: ensure updatedCR.Spec.ControllerConfig is non-nil (create a zero value if needed) and set updatedCR.Spec.ControllerConfig.ComponentConfigs = applicableEnvConfigs; apply the same change to the other similar assignment later in the file where ControllerConfig is being replaced.pkg/controller/external_secrets/deployments_test.go (1)
2027-2033: ⚡ Quick winAvoid branching test setup by test-name string.
Line 2031 uses a literal test title to select behavior. This is brittle and can silently break setup on rename; use an explicit per-case flag instead.
Refactor to explicit case metadata
@@ tests := []struct { name string esc *v1alpha1.ExternalSecretsConfig cm *corev1.ConfigMap + cmNotFound bool deployment *appsv1.Deployment @@ { name: "removes user CA config when ConfigMap is missing", esc: baseESC, + cmNotFound: true, @@ - case tt.name == "removes user CA config when ConfigMap is missing": + case tt.cmNotFound: cached = &fakes.FakeCtrlClient{} uncached = &fakes.FakeCtrlClient{}🤖 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/deployments_test.go` around lines 2027 - 2033, The test currently branches setup on the literal test title (tt.name == "removes user CA config when ConfigMap is missing"); add an explicit per-case flag to the test case struct (e.g., a bool like noConfigMapClients or needsEmptyClients), update the affected test cases to set that flag, and replace the name-based branch with a check of that flag in the switch that assigns cached and uncached; keep the existing behavior by instantiating &fakes.FakeCtrlClient{} for cached and uncached when the flag is set, otherwise call setupConfigMapClients(t, tt.cm) when tt.cm != nil.
🤖 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/certificate.go`:
- Line 196: The error formatting uses types.NamespacedName variable key directly
with %q which yields noisy diagnostics; update the fmt.Errorf call to format the
key as a string (e.g., use key.String()) and pass that string to the formatter
(for example use "%s" with key.String() or "%q" with key.String()) in the return
statement that currently reads fmt.Errorf("failed to fetch %q issuer: %w", key,
err), referencing the key variable (types.NamespacedName) in the certificate
fetch logic.
In `@pkg/controller/external_secrets/controller.go`:
- Around line 205-206: The code ignores the error returned by
labels.NewRequirement when building managedResourceLabelReqSelector and uses a
value switch that compares pointer identity for elements from
controllerManagedResources, causing watches for Deployments/Secrets not to
register; fix by checking and returning/logging the error from
labels.NewRequirement when creating managedResourceLabelReq (used to build
managedResourceLabelReqSelector) instead of discarding it, and change the switch
over res in the controllerManagedResources loop to a type switch (e.g., switch
res.(type) { case *appsv1.Deployment: ... case *corev1.Secret: ... }) so the
cases match the actual pointer types and the special watches for
appsv1.Deployment and corev1.Secret are correctly registered.
In `@pkg/controller/external_secrets/deployments_test.go`:
- Around line 1874-1899: Enhance the tests to assert the SSL_CERT_DIR env var is
present only on the core controller container and absent elsewhere: in the
assertDeploy closure, after locating controller via findContainer(deployment,
OperandCoreControllerContainer) assert controller.Env contains a variable named
"SSL_CERT_DIR" (and optionally its expected value), then after locating the
sidecar via findContainer(deployment, "sidecar") assert sidecar.Env does NOT
contain "SSL_CERT_DIR"; apply the same absence assertion to the nil-config
cleanup test block (the other test around lines 1934-1950) so both the
controller-only and cleanup paths explicitly check SSL_CERT_DIR
presence/absence.
In `@pkg/controller/external_secrets/deployments.go`:
- Around line 128-136: The code returns early when applyUserCABundleConfig
returns a NotFound error, which prevents subsequent deployment assembly steps
(e.g., updateResourceRequirement, affinity/tolerations/nodeSelector, user
overrides, proxy wiring) from running; change the error handling in the block
that calls applyUserCABundleConfig so that if
common.IsUserTrustedCABundleNotFound(err) you do NOT return but instead record
or wrap the error and continue assembling the Deployment (i.e., proceed to call
updateResourceRequirement and the remaining wiring functions), while preserving
the existing behavior of returning nil, wrapped for all other errors from
applyUserCABundleConfig.
- Around line 520-543: mergeContainerEnvVars currently iterates the envVars map
in unpredictable order which can reorder container.Env and cause spurious diffs;
modify mergeContainerEnvVars to collect and sort the keys of envVars (e.g., into
a []string, sort.Strings(keys)), then iterate the sorted keys to append managed
env vars, and finally append existing container.Env entries that are not present
in envVars so the resulting container.Env order is deterministic.
In `@pkg/controller/external_secrets/trusted_ca_bundle.go`:
- Around line 97-139: parsePEMCABundle currently ignores any non-PEM trailing
bytes left in the rest buffer after the pem.Decode loop; after the loop (before
the caCount==0 check) validate that rest is empty or only contains
whitespace/newlines and if not return a trustedCABundleValidationError (use
eventReason trustedCABundleEventInvalidPEM) with a message indicating malformed
trailing data in the trusted CA bundle; reference the rest variable, the
pem.Decode loop, parsePEMCABundle function and trustedCABundleValidationError to
locate where to add this post-loop validation.
In `@test/e2e/trusted_ca_bundle_test.go`:
- Around line 118-125: The cleanup call that currently discards the result of
retry.RetryOnConflict must check and assert the error instead of ignoring it:
capture the returned error from the retry.RetryOnConflict call that modifies
ExternalSecretsConfig (the anonymous func that gets esc and sets
esc.Spec.ControllerConfig.TrustedCABundle = nil and calls
suiteRuntimeClient.Update), then assert the result is nil or allowable (e.g.
treat apierrors.IsNotFound(err) as success) so teardown failures aren’t
swallowed; update the AfterEach to assign the retry result to a variable and
perform the appropriate assertion/conditional handling around that variable.
---
Outside diff comments:
In `@pkg/controller/external_secrets/controller.go`:
- Around line 314-322: The switch in controller.go is comparing pointer literal
identities so cases like "&appsv1.Deployment{}" never match elements from
controllerManagedResources; change the logic to type-switch or compare concrete
types instead: iterate controllerManagedResources and use a type switch on the
element (or reflect.TypeOf(res)) to detect *appsv1.Deployment and *corev1.Secret
and then call mgrBuilder.Watches/mgrBuilder.WatchesMetadata with the appropriate
predicates (preserving handler.EnqueueRequestsFromMapFunc(mapFunc),
withIgnoreStatusUpdatePredicates,
builder.WithPredicates(predicate.LabelChangedPredicate{}), and
managedResourcePredicate).
---
Nitpick comments:
In `@pkg/controller/external_secrets/deployments_test.go`:
- Around line 2027-2033: The test currently branches setup on the literal test
title (tt.name == "removes user CA config when ConfigMap is missing"); add an
explicit per-case flag to the test case struct (e.g., a bool like
noConfigMapClients or needsEmptyClients), update the affected test cases to set
that flag, and replace the name-based branch with a check of that flag in the
switch that assigns cached and uncached; keep the existing behavior by
instantiating &fakes.FakeCtrlClient{} for cached and uncached when the flag is
set, otherwise call setupConfigMapClients(t, tt.cm) when tt.cm != nil.
In `@test/e2e/e2e_test.go`:
- Around line 401-403: The test currently replaces the whole
updatedCR.Spec.ControllerConfig with a new ControllerConfig which wipes other
fields; instead assign only ComponentConfigs to preserve unrelated fields:
ensure updatedCR.Spec.ControllerConfig is non-nil (create a zero value if
needed) and set updatedCR.Spec.ControllerConfig.ComponentConfigs =
applicableEnvConfigs; apply the same change to the other similar assignment
later in the file where ControllerConfig is being replaced.
🪄 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: 5749c0c6-c704-4267-92d5-fa8b2db8d24b
📒 Files selected for processing (36)
api/v1alpha1/external_secrets_config_types.goapi/v1alpha1/groupversion_info.goapi/v1alpha1/meta.gobundle/manifests/openshift-external-secrets-operator.clusterserviceversion.yamlbundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlbundle/manifests/operator.openshift.io_externalsecretsmanagers.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsmanagers.yamldocs/api_reference.mdpkg/controller/common/errors.gopkg/controller/common/errors_test.gopkg/controller/commontest/utils.gopkg/controller/external_secrets/certificate.gopkg/controller/external_secrets/certificate_test.gopkg/controller/external_secrets/configmap.gopkg/controller/external_secrets/constants.gopkg/controller/external_secrets/controller.gopkg/controller/external_secrets/controller_failure_test.gopkg/controller/external_secrets/controller_test.gopkg/controller/external_secrets/deployments.gopkg/controller/external_secrets/deployments_test.gopkg/controller/external_secrets/networkpolicy_test.gopkg/controller/external_secrets/test_utils.gopkg/controller/external_secrets/trusted_ca_bundle.gopkg/controller/external_secrets/trusted_ca_bundle_test.gopkg/controller/external_secrets/utils.gopkg/controller/external_secrets/utils_test.gopkg/version/version.gotest/e2e/bitwarden_es_test.gotest/e2e/e2e_test.gotest/e2e/helpers_test.gotest/e2e/trusted_ca_bundle_test.gotest/utils/bitwarden.gotest/utils/cleanup.gotest/utils/conditions.gotest/utils/external_secrets_config.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/go.mod`:
- Around line 73-74: The go.mod pins "go.opentelemetry.io/otel v1.38.0" and
"go.opentelemetry.io/otel/trace v1.38.0", which are within the vulnerable range
for GHSA-mh2q-q3fh-2475; update both module versions to at least v1.41.0 (e.g.,
replace those entries with v1.41.0 or run go get
go.opentelemetry.io/otel@v1.41.0 and go get
go.opentelemetry.io/otel/trace@v1.41.0), then run go mod tidy to lock the
updates and verify builds/tests pass.
🪄 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: 21dd620e-35ed-4eba-924d-05d44774bc9a
⛔ Files ignored due to path filters (1)
test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
api/v1alpha1/external_secrets_config_types.goapi/v1alpha1/groupversion_info.goapi/v1alpha1/meta.gobundle/manifests/openshift-external-secrets-operator.clusterserviceversion.yamlbundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlbundle/manifests/operator.openshift.io_externalsecretsmanagers.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsmanagers.yamldocs/api_reference.mdpkg/controller/common/errors.gopkg/controller/common/errors_test.gopkg/controller/commontest/utils.gopkg/controller/external_secrets/certificate.gopkg/controller/external_secrets/certificate_test.gopkg/controller/external_secrets/configmap.gopkg/controller/external_secrets/constants.gopkg/controller/external_secrets/controller.gopkg/controller/external_secrets/controller_failure_test.gopkg/controller/external_secrets/controller_test.gopkg/controller/external_secrets/deployments.gopkg/controller/external_secrets/deployments_test.gopkg/controller/external_secrets/networkpolicy_test.gopkg/controller/external_secrets/test_utils.gopkg/controller/external_secrets/trusted_ca_bundle.gopkg/controller/external_secrets/trusted_ca_bundle_test.gopkg/controller/external_secrets/utils.gopkg/controller/external_secrets/utils_test.gopkg/version/version.gotest/e2e/bitwarden_es_test.gotest/e2e/e2e_test.gotest/e2e/helpers_test.gotest/e2e/trusted_ca_bundle_test.gotest/go.modtest/utils/bitwarden.gotest/utils/cleanup.gotest/utils/conditions.gotest/utils/external_secrets_config.go
✅ Files skipped from review due to trivial changes (11)
- bundle/manifests/operator.openshift.io_externalsecretsmanagers.yaml
- docs/api_reference.md
- pkg/version/version.go
- api/v1alpha1/meta.go
- api/v1alpha1/groupversion_info.go
- config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml
- test/utils/bitwarden.go
- config/crd/bases/operator.openshift.io_externalsecretsmanagers.yaml
- bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml
- bundle/manifests/openshift-external-secrets-operator.clusterserviceversion.yaml
- api/v1alpha1/external_secrets_config_types.go
🚧 Files skipped from review as they are similar to previous changes (25)
- pkg/controller/external_secrets/networkpolicy_test.go
- pkg/controller/external_secrets/controller_test.go
- pkg/controller/commontest/utils.go
- pkg/controller/common/errors.go
- pkg/controller/external_secrets/certificate_test.go
- pkg/controller/external_secrets/utils_test.go
- test/e2e/helpers_test.go
- test/utils/cleanup.go
- pkg/controller/external_secrets/trusted_ca_bundle_test.go
- pkg/controller/external_secrets/certificate.go
- test/utils/conditions.go
- pkg/controller/common/errors_test.go
- pkg/controller/external_secrets/trusted_ca_bundle.go
- pkg/controller/external_secrets/configmap.go
- pkg/controller/external_secrets/test_utils.go
- pkg/controller/external_secrets/utils.go
- test/utils/external_secrets_config.go
- test/e2e/trusted_ca_bundle_test.go
- pkg/controller/external_secrets/controller_failure_test.go
- test/e2e/bitwarden_es_test.go
- pkg/controller/external_secrets/controller.go
- pkg/controller/external_secrets/deployments_test.go
- test/e2e/e2e_test.go
- pkg/controller/external_secrets/deployments.go
- pkg/controller/external_secrets/constants.go
Signed-off-by: Bharath B <bhb@redhat.com>
Contributor
Author
|
/test e2e-operator |
|
@bharath-b-rh: 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #149 +/- ##
==========================================
- Coverage 47.94% 46.27% -1.68%
==========================================
Files 29 31 +2
Lines 4207 5005 +798
==========================================
+ Hits 2017 2316 +299
- Misses 1902 2391 +489
- Partials 288 298 +10
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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
make test-unitmake test-apisSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests