ESO-279:Adds implementation logic for revisionLimitHistory to support customizations at install time#94
Conversation
WalkthroughApplies per-component Deployment.RevisionHistoryLimit from ExternalSecretsConfig to generated Deployments, adds asset-to-component mapping, treats RevisionHistoryLimit differences as deployment spec changes, and adds unit and e2e tests plus testdata for revisionHistoryLimit scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)/bin/bash: line 1: golangci-lint: command not found Comment |
siddhibhor-56
force-pushed
the
ESO-278
branch
from
4baefed to
bed7f63
Compare
![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
🧹 Nitpick comments (1)
docs/api_reference.md (1)
148-148: Inconsistency in ComponentName enum documentation.The
ComponentConfig.componentNamefield (line 148) allowsWebhookandCertControlleras valid values, but theComponentNameenum section (lines 167-168) only documentsExternalSecretsCoreControllerandBitwardenSDKServer.Additionally, the description text on line 148 references "Bitwarden" but the enum shows "BitwardenSDKServer".
Please ensure the ComponentName section documents all valid component names, or verify that the ComponentConfig field should only permit a subset of component types. Update terminology for consistency.
Also applies to: 167-168
📜 Review details
Configuration used: Organization 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 (6)
api/v1alpha1/external_secrets_config_types.goapi/v1alpha1/zz_generated.deepcopy.gobundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.mdpkg/controller/external_secrets/deployments.go
🧰 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:
api/v1alpha1/external_secrets_config_types.goapi/v1alpha1/zz_generated.deepcopy.gopkg/controller/external_secrets/deployments.gobundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.md
🧬 Code graph analysis (2)
api/v1alpha1/zz_generated.deepcopy.go (1)
api/v1alpha1/external_secrets_config_types.go (2)
ComponentConfig(140-151)DeploymentConfig(152-160)
pkg/controller/external_secrets/deployments.go (2)
pkg/controller/external_secrets/controller.go (1)
Reconciler(85-94)api/v1alpha1/external_secrets_config_types.go (7)
ExternalSecretsConfig(43-55)ControllerConfig(103-138)ComponentName(243-243)CoreController(247-247)Webhook(250-250)CertController(253-253)BitwardenSDKServer(256-256)
🔇 Additional comments (5)
api/v1alpha1/external_secrets_config_types.go (2)
131-159: ComponentConfig/DeploymentConfig shape and wiring look correctThe
ComponentConfigsfield,ComponentConfigstruct, andDeploymentConfig.RevisionHistoryLimit(pointer withMinimum=1) line up cleanly with the intended per‑component override semantics and the CRD schema; omitempty behavior with the nested pointer is also correct. I don’t see issues here.
246-254: Extending ComponentName enum for Webhook/CertController is safeAdding
WebhookandCertControllertoComponentNameis consistent with their use inComponentConfigand the controller logic; no behavioral or API‑compat concerns stand out.api/v1alpha1/zz_generated.deepcopy.go (1)
165-179: Deep‑copy implementation matches new API typesThe generated deep‑copy methods for
ComponentConfig,DeploymentConfig, and the updatedControllerConfig.DeepCopyIntocorrectly handle nested structs and pointer fields and align with the new spec types.Also applies to: 240-246, 279-297
config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml (1)
1266-1299: Base CRD schema for componentConfig matches Go typesThe
controllerConfig.componentConfigdefinition (enum values,revisionHistoryLimitconstraints, listMapKey, maxItems) is consistent with theControllerConfig/ComponentConfig/DeploymentConfigGo types and should serialize/validate correctly.pkg/controller/external_secrets/deployments.go (1)
151-154: RevisionHistoryLimit application logic is soundMapping assets to
ComponentNameand applyingDeploymentConfigs.RevisionHistoryLimitonly when set is nil‑safe, keeps defaults intact when unset, and cleanly scopes overrides per component. The helper switch on the known asset constants is straightforward and future‑proof enough for the current set of deployments.Also applies to: 669-709
siddhibhor-56
force-pushed
the
ESO-278
branch
from
c452c0b to
c386c82
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization 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 (3)
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.mdpkg/controller/external_secrets/deployments_test.go
🧰 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:
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.mdpkg/controller/external_secrets/deployments_test.go
🧬 Code graph analysis (1)
pkg/controller/external_secrets/deployments_test.go (2)
api/v1alpha1/external_secrets_config_types.go (4)
ComponentConfig(140-151)ComponentName(243-243)CoreController(247-247)DeploymentConfig(152-160)pkg/controller/commontest/utils.go (1)
TestExternalSecretsImageName(16-16)
🔇 Additional comments (5)
docs/api_reference.md (3)
117-132: LGTM!The ComponentConfig documentation is clear, complete, and properly structured with appropriate validation constraints and cross-references.
225-239: LGTM!The DeploymentConfig documentation is clear and properly explains the purpose of the revisionHistoryLimit field with appropriate validation constraints.
134-152: LGTM!The ComponentName enum documentation properly reflects the expanded set of component values (Webhook, CertController) and clarifies the CoreController description.
pkg/controller/external_secrets/deployments_test.go (1)
565-604: LGTM!The test case correctly validates that per-component
revisionHistoryLimitconfiguration is applied to deployments. The test setup, configuration, and assertions are appropriate for the create path.bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
1266-1299: LGTM!The
componentConfigfield definition correctly matches the Go types:
- ComponentConfig with required
componentNameand optionaldeploymentConfigs- DeploymentConfig with optional
revisionHistoryLimit(int32, min: 1)- Proper list validation (maxItems: 4, map-keyed by componentName)
- Correct enum values including BitwardenSDKServer
The implementation aligns with the base CRD and API types.
siddhibhor-56
force-pushed
the
ESO-278
branch
from
c386c82 to
43e5105
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/api_reference.md (1)
204-204: Add description for componentConfig field.The
componentConfigfield in the ControllerConfig table is missing a description. Based on the context, it should explain that this field allows specifying per-component deployment configuration overrides.🔎 Suggested documentation update
| Field | Description | Default | Validation | | --- | --- | --- | --- | | `certProvider` _[CertProvidersConfig](#certprovidersconfig)_ | certProvider is for defining the configuration for certificate providers used to manage TLS certificates for webhook and plugins. | | Optional: \{\} <br /> | | `labels` _object (keys:string, values:string)_ | labels to apply to all resources created for the external-secrets operand deployment.<br />This field can have a maximum of 20 entries. | | MaxProperties: 20 <br />MinProperties: 0 <br />Optional: \{\} <br /> | | `networkPolicies` _[NetworkPolicy](#networkpolicy) array_ | networkPolicies specifies the list of network policy configurations<br />to be applied to external-secrets pods.<br />Each entry allows specifying a name for the generated NetworkPolicy object,<br />along with its full Kubernetes NetworkPolicy definition.<br />If this field is not provided, external-secrets components will be isolated<br />with deny-all network policies, which will prevent proper operation. | | MaxItems: 50 <br />MinItems: 0 <br />Optional: \{\} <br /> | -| `componentConfig` _[ComponentConfig](#componentconfig) array_ | | | MaxItems: 4 <br />MinItems: 0 <br />Optional: \{\} <br /> | +| `componentConfig` _[ComponentConfig](#componentconfig) array_ | componentConfig allows specifying per-component deployment configuration overrides.<br />Each entry targets a specific component by name and can override deployment-level settings. | | MaxItems: 4 <br />MinItems: 0 <br />Optional: \{\} <br /> |
📜 Review details
Configuration used: Organization 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 (3)
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.mdpkg/controller/external_secrets/deployments_test.go
🧰 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/controller/external_secrets/deployments_test.godocs/api_reference.mdbundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml
🧬 Code graph analysis (1)
pkg/controller/external_secrets/deployments_test.go (4)
pkg/controller/crd_annotator/controller.go (1)
Reconciler(65-69)pkg/controller/client/fakes/fake_ctrl_client.go (1)
FakeCtrlClient(12-135)api/v1alpha1/external_secrets_config_types.go (6)
ExternalSecretsConfig(43-55)ControllerConfig(103-138)ComponentConfig(140-151)ComponentName(243-243)CoreController(247-247)DeploymentConfig(152-160)pkg/controller/commontest/utils.go (1)
TestExternalSecretsImageName(16-16)
🔇 Additional comments (3)
docs/api_reference.md (1)
134-151: LGTM!The ComponentName enum documentation is properly updated to include Webhook and CertController components. The descriptions are clear and consistent.
pkg/controller/external_secrets/deployments_test.go (1)
565-607: LGTM!The test case properly validates the new per-component revisionHistoryLimit feature. The test setup, configuration, and validation logic are correct and follow established patterns in the test suite.
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
1266-1299: LGTM!The componentConfig CRD definition is well-structured with appropriate validations:
- Proper enum constraints for componentName
- Correct minimum validation (1) for revisionHistoryLimit to ensure rollback capability
- List type properly configured as map with componentName as key to enforce uniqueness
- Array size constraints (maxItems: 4) align with the number of supported components
The definition is consistent with the API types and documentation.
siddhibhor-56
changed the title
|
@siddhibhor-56: This pull request references ESO-279 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 "4.22.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. |
openshift-ci-robot
added
the
jira/valid-reference
| } | ||
|
|
||
| // Apply component-specific configurations (RevisionHistoryLimit) | ||
| if err := r.applyRevisionHistoryLimit(deployment, esc, assetName); err != nil { |
There was a problem hiding this comment.
I would suggest to keep it generic.
| if err := r.applyRevisionHistoryLimit(deployment, esc, assetName); err != nil { | |
| if err := r.applyUserDeploymentConfigs(deployment, esc, assetName); err != nil { |
There was a problem hiding this comment.
Done, Renamed
| componentName := getComponentNameFromAsset(assetName) | ||
| if componentName == "" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This should be an error. componentName is from the enum, so must have a valid value. And if any issues in the code, must be logged.
siddhibhor-56
force-pushed
the
ESO-278
branch
2 times, most recently
from
4a53df0 to
0cca990
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/e2e_test.go (1)
232-272: Well-structured test with good coverage.The test properly:
- Updates configuration and restores it in defer
- Waits for pods before verifying
- Uses
Eventuallywith appropriate timeouts to handle reconciliation timing- Verifies all three components with distinct expected values
Consider extracting the repeated deployment verification into a helper function to reduce code duplication:
♻️ Optional: Extract helper to reduce repetition
verifyRevisionHistoryLimit := func(deploymentName string, expectedLimit int32) { Eventually(func(g Gomega) { deployment, err := clientset.AppsV1().Deployments(operandNamespace).Get(ctx, deploymentName, metav1.GetOptions{}) g.Expect(err).NotTo(HaveOccurred(), "should get %s deployment", deploymentName) g.Expect(deployment.Spec.RevisionHistoryLimit).NotTo(BeNil(), "revisionHistoryLimit should be set") g.Expect(*deployment.Spec.RevisionHistoryLimit).To(Equal(expectedLimit), "revisionHistoryLimit should be %d for %s", expectedLimit, deploymentName) }, time.Minute, 5*time.Second).Should(Succeed()) } By("Verifying revisionHistoryLimit for all deployments") verifyRevisionHistoryLimit("external-secrets", int32(3)) verifyRevisionHistoryLimit("external-secrets-webhook", int32(5)) verifyRevisionHistoryLimit("external-secrets-cert-controller", int32(2))
📜 Review details
Configuration used: Organization 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)
test/e2e/e2e_test.gotest/e2e/testdata/external_secret_with_revision_limit.yaml
🧰 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:
test/e2e/e2e_test.gotest/e2e/testdata/external_secret_with_revision_limit.yaml
🧬 Code graph analysis (1)
test/e2e/e2e_test.go (1)
test/utils/conditions.go (1)
VerifyPodsReadyByPrefix(53-81)
🔇 Additional comments (2)
test/e2e/testdata/external_secret_with_revision_limit.yaml (1)
1-34: LGTM!The test fixture is well-structured with distinct
revisionHistoryLimitvalues (3, 5, 2) for each component, making it easy to verify that per-component configuration is applied correctly. The networkPolicy section provides additional realism to the test configuration.test/e2e/e2e_test.go (1)
44-49: LGTM!The new constant is appropriately named and follows the existing naming convention.
| } | ||
|
|
||
| // Map asset name to component name | ||
| componentName := getComponentNameFromAsset(assetName) |
There was a problem hiding this comment.
nit: why not return error from getComponentNameFromAsset() instead of creating it in the caller.
test/e2e/e2e_test.go
Outdated
| deployment, err := clientset.AppsV1().Deployments(operandNamespace).Get(ctx, "external-secrets", metav1.GetOptions{}) | ||
| g.Expect(err).NotTo(HaveOccurred(), "should get external-secrets deployment") | ||
| g.Expect(deployment.Spec.RevisionHistoryLimit).NotTo(BeNil(), "revisionHistoryLimit should be set") | ||
| g.Expect(*deployment.Spec.RevisionHistoryLimit).To(Equal(int32(3)), "revisionHistoryLimit should be 3 for controller") |
There was a problem hiding this comment.
Please add the same validation in the existing test case where revisionHistoryLimit is not set, or don't set it for one of the components.
There was a problem hiding this comment.
updated the testcase
There was a problem hiding this comment.
I think the current commit doesn't contain the change. Could you check once.
siddhibhor-56
force-pushed
the
ESO-278
branch
3 times, most recently
from
41177da to
2025a67
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/controller/external_secrets/deployments.go`:
- Around line 679-687: The loop over esc.Spec.ControllerConfig.ComponentConfigs
may dereference a nil DeploymentConfigs; before reading
i.DeploymentConfigs.RevisionHistoryLimit check that i.DeploymentConfigs != nil
(and that RevisionHistoryLimit != nil) and only then assign to
deployment.Spec.RevisionHistoryLimit; update the block in the loop that matches
componentName (the code referencing i.DeploymentConfigs.RevisionHistoryLimit and
deployment.Spec.RevisionHistoryLimit) to guard against nil DeploymentConfigs and
avoid the panic.
♻️ Duplicate comments (3)
docs/api_reference.md (3)
117-132: Missing description for ComponentConfig type.The ComponentConfig section still lacks a description explaining its purpose. This was previously flagged.
204-204: Missing description for componentConfig field.The
componentConfigfield in the ControllerConfig table still has an empty description. This was previously flagged.
225-239: Missing description for DeploymentConfig type.The DeploymentConfig section still lacks a description explaining its purpose. This was previously flagged.
🧹 Nitpick comments (1)
api/v1alpha1/external_secrets_config_types.go (1)
242-254: ClarifyComponentNamedocstring now that it covers more than network policies.
The comment still implies network-policy-only usage after adding Webhook/CertController.💡 Suggested doc update
-// ComponentName represents the different external-secrets components that can have network policies applied. +// ComponentName represents the different external-secrets components that can be configured per-component +// (e.g., network policies and deployment overrides).
📜 Review details
Configuration used: Organization 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 (10)
api/v1alpha1/external_secrets_config_types.goapi/v1alpha1/zz_generated.deepcopy.gobundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.mdpkg/controller/common/utils.gopkg/controller/external_secrets/deployments.gopkg/controller/external_secrets/deployments_test.gotest/e2e/e2e_test.gotest/e2e/testdata/external_secret_with_revision_limit.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/controller/common/utils.go
- api/v1alpha1/zz_generated.deepcopy.go
- pkg/controller/external_secrets/deployments_test.go
- config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml
🧰 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:
test/e2e/testdata/external_secret_with_revision_limit.yamltest/e2e/e2e_test.goapi/v1alpha1/external_secrets_config_types.godocs/api_reference.mdpkg/controller/external_secrets/deployments.gobundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml
🧬 Code graph analysis (2)
test/e2e/e2e_test.go (1)
test/utils/conditions.go (1)
VerifyPodsReadyByPrefix(53-81)
pkg/controller/external_secrets/deployments.go (1)
api/v1alpha1/external_secrets_config_types.go (7)
ExternalSecretsConfig(43-55)ControllerConfig(103-138)ComponentName(243-243)CoreController(247-247)Webhook(250-250)CertController(253-253)BitwardenSDKServer(256-256)
🔇 Additional comments (8)
docs/api_reference.md (1)
143-151: LGTM!The ComponentName enum is correctly updated with
WebhookandCertControllervalues, with clear descriptions for each component.test/e2e/testdata/external_secret_with_revision_limit.yaml (1)
1-34: LGTM!Well-structured test fixture that exercises per-component revisionHistoryLimit with distinct values (3, 5, 2) for easy verification. The omission of BitwardenSDKServer is appropriate since it's an optional plugin.
test/e2e/e2e_test.go (2)
46-48: LGTM!Constants are properly defined and follow the existing naming convention.
232-273: LGTM!Well-structured E2E test that:
- Properly updates and restores the ExternalSecretsConfig
- Waits for pod readiness before verification
- Uses
Eventuallywith appropriate timeout/interval for async deployment updates- Verifies all three component deployments with their expected revisionHistoryLimit values
pkg/controller/external_secrets/deployments.go (2)
150-152: LGTM!The integration point is clean -
applyUserDeploymentConfigsis called at the right place after other deployment configurations are applied.
692-706: LGTM!Clean mapping function with comprehensive error handling. All four deployment asset names are correctly mapped to their corresponding ComponentName enum values.
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
1258-1291: LGTM!The CRD schema for
componentConfigis well-defined:
- Proper enum validation for
componentNamewith all four component valuesrevisionHistoryLimithas correctminimum: 1constraint- List semantics (
x-kubernetes-list-type: mapwithcomponentNamekey) ensure uniqueness per componentmaxItems: 4correctly limits entries to match the number of supported componentsapi/v1alpha1/external_secrets_config_types.go (1)
132-159: Per-component schema and validation look solid.
Good use of map semantics andrevisionHistoryLimitvalidation to keep overrides consistent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@siddhibhor-56: This pull request references ESO-279 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 either version "4.22." or "openshift-4.22.", but it targets "eso-1.1" instead. 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. |
siddhibhor-56
force-pushed
the
ESO-278
branch
from
1e491a8 to
a07bbd0
Compare
# Conflicts: # api/v1alpha1/external_secrets_config_types.go # api/v1alpha1/zz_generated.deepcopy.go # bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml # config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml # docs/api_reference.md
siddhibhor-56
force-pushed
the
ESO-278
branch
from
a07bbd0 to
d5c3f78
Compare
bharath-b-rh
left a comment
bharath-b-rh
left a comment
There was a problem hiding this comment.
Just some nitpicks, otherwise ready to be merged.
| return nil, fmt.Errorf("failed to update proxy configuration: %w", err) | ||
| } | ||
| if err := r.applyUserDeploymentConfigs(deployment, esc, assetName); err != nil { | ||
| return nil, fmt.Errorf("failed to apply revision history limit: %w", err) |
There was a problem hiding this comment.
Please make the error message generic, something like
| return nil, fmt.Errorf("failed to apply revision history limit: %w", err) | |
| return nil, fmt.Errorf("failed to apply user deployment configurations: %w", err) |
There was a problem hiding this comment.
Corrected the logs
| container.VolumeMounts = filteredVolumeMounts | ||
| } | ||
|
|
||
| // applyUserDeploymentConfigs sets the revisionHistoryLimit from ComponentConfig if specified. |
There was a problem hiding this comment.
| // applyUserDeploymentConfigs sets the revisionHistoryLimit from ComponentConfig if specified. | |
| // applyUserDeploymentConfigs updates the deployment resource spec with user specified configurations. |
There was a problem hiding this comment.
Corrected the logs
| case bitwardenDeploymentAssetName: | ||
| return operatorv1alpha1.BitwardenSDKServer, nil | ||
| default: | ||
| return "", fmt.Errorf("invalid or unknown deployment asset name %q: ensure the asset is mapped to a valid ComponentName enum value (CoreController, Webhook, CertController, or BitwardenSDKServer)", assetName) |
There was a problem hiding this comment.
| return "", fmt.Errorf("invalid or unknown deployment asset name %q: ensure the asset is mapped to a valid ComponentName enum value (CoreController, Webhook, CertController, or BitwardenSDKServer)", assetName) | |
| return "", fmt.Errorf("unknown deployment asset name: %s", assetName) |
There was a problem hiding this comment.
Corrected the logs
|
|
||
| componentName, err := getComponentNameFromAsset(assetName) | ||
| if err != nil { | ||
| r.log.Error(err, "failed to apply revision history limit due to unrecognized asset name", "assetName", assetName, "deployment", deployment.GetName()) |
There was a problem hiding this comment.
| r.log.Error(err, "failed to apply revision history limit due to unrecognized asset name", "assetName", assetName, "deployment", deployment.GetName()) | |
| r.log.Error(err, "failed to resolve deployment name for updating configurations") |
There was a problem hiding this comment.
Corrected the logs
test/e2e/e2e_test.go
Outdated
| deployment, err := clientset.AppsV1().Deployments(operandNamespace).Get(ctx, "external-secrets", metav1.GetOptions{}) | ||
| g.Expect(err).NotTo(HaveOccurred(), "should get external-secrets deployment") | ||
| g.Expect(deployment.Spec.RevisionHistoryLimit).NotTo(BeNil(), "revisionHistoryLimit should be set") | ||
| g.Expect(*deployment.Spec.RevisionHistoryLimit).To(Equal(int32(3)), "revisionHistoryLimit should be 3 for controller") |
There was a problem hiding this comment.
I think the current commit doesn't contain the change. Could you check once.
siddhibhor-56
force-pushed
the
ESO-278
branch
from
d5c3f78 to
fd74749
Compare
|
/lgtm @mytreya-rh for additional reviews. @snarayan-redhat for docs-approved label. We will need to cover the feature overview in RN and will also require adding a section with examples on the usage. Thank you! |
openshift-ci
bot
added
the
px-approved
|
/label docs-approved |
openshift-ci
bot
added
the
docs-approved
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
Overall good! Few comments, mostly suggesting some restructuring for improved cohesion and terseness.
| if len(esc.Spec.ControllerConfig.ComponentConfigs) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
As getComponentNameFromAsset is a trivial mapping function, i feel this early return is not needed.
There was a problem hiding this comment.
True, updated
| m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { | ||
| return false, nil | ||
| }) |
There was a problem hiding this comment.
There are about 5 or more of these. Can we not have just on function DoesNotExist and plugit into m.ExistsCalls on all occasions when we need it to return false?
| m.CreateCalls(func(ctx context.Context, obj client.Object, _ ...client.CreateOption) error { | ||
| switch o := obj.(type) { | ||
| case *appsv1.Deployment: | ||
| capturedDeployments[o.Name] = o.DeepCopy() |
There was a problem hiding this comment.
whats the use of this map capturedDeployments ? Unable to see its usage anywhere
There was a problem hiding this comment.
removed the unused code
| validateDeployment: func(t *testing.T, deployment *appsv1.Deployment) { | ||
| if deployment == nil { | ||
| t.Error("deployment should not be nil") | ||
| return | ||
| } | ||
| if deployment.Spec.RevisionHistoryLimit == nil { | ||
| t.Error("revisionHistoryLimit should be set") | ||
| return | ||
| } | ||
| if *deployment.Spec.RevisionHistoryLimit != 5 { | ||
| t.Errorf("revisionHistoryLimit = %d, want 5", *deployment.Spec.RevisionHistoryLimit) | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
can we pull this code up into a validateRevisionHistory function so that we can instantiate it with the required revision history as the argument, so that it can be re-used in this and subsequent tests?
| m.CreateCalls(func(ctx context.Context, obj client.Object, _ ...client.CreateOption) error { | ||
| switch o := obj.(type) { | ||
| case *appsv1.Deployment: | ||
| if o.Name == "external-secrets" { | ||
| *capturedDeployment = o.DeepCopy() | ||
| } | ||
| } | ||
| return nil | ||
| }) | ||
| }, |
There was a problem hiding this comment.
this looks exactly similar to what's in the test "deployment without revisionHistoryLimit should use default"
There was a problem hiding this comment.
Can we refactor the unit test so that it gets terse?
For example, like in this yet to be merged PR :-)
https://github.com/kubernetes-sigs/secrets-store-csi-driver/pull/1841/changes#diff-6d438949f97677c98d673fc22e1a3c43e2f1c4cc7a14e4973555395b8c9c1acb
Most other comments in this file are made in the same vein.
There was a problem hiding this comment.
Updated the unit test as per the shared example
There was a problem hiding this comment.
Instead of one more yaml file, can we template the existing external_secret.yaml (with componentConfigs wrapped in an if)?
That way we need not hardcode the values in e2e_test.go
The template can be instantiated with the desired revision limit, and the same can be verified on the resulting deployments.
There was a problem hiding this comment.
PTAL updated the e2e
siddhibhor-56
force-pushed
the
ESO-278
branch
from
e6e82bf to
2499240
Compare
test/e2e/e2e_test.go
Outdated
| componentConfigsYAML := fmt.Sprintf(` componentConfigs: | ||
| - componentName: ExternalSecretsCoreController | ||
| deploymentConfigs: | ||
| revisionHistoryLimit: %d | ||
| - componentName: Webhook | ||
| deploymentConfigs: | ||
| revisionHistoryLimit: %d | ||
| - componentName: CertController | ||
| deploymentConfigs: | ||
| revisionHistoryLimit: %d | ||
| `, controllerLimit, webhookLimit, certControllerLimit) |
There was a problem hiding this comment.
Actually, what i had in mind was to extend external_secret.yaml as below with go template:
{{- if .ComponentConfigs }}
componentConfigs:
{{- range .ComponentConfigs }}
- componentName: {{ .ComponentName }}
{{- if .DeploymentConfigs }}
deploymentConfigs:
{{- if .DeploymentConfigs.RevisionHistoryLimit }}
revisionHistoryLimit: {{ .DeploymentConfigs.RevisionHistoryLimit }}
{{- end }}
{{- end }}
{{- end }}
{{- end }}
And populate it using text/template package in the e2e tests.
But now, i am thinking, would it be of much more programmatic convenience if we get rid of external_secret.yaml file and generate the yaml directly from the go struct as below:
// Helper function to create base ExternalSecretsConfig
func createBaseExternalSecretsConfig() *v1alpha1.ExternalSecretsConfig {
tcp := corev1.ProtocolTCP
udp := corev1.ProtocolUDP
port6443 := int32(6443)
port443 := int32(443)
port5353 := int32(5353)
return &v1alpha1.ExternalSecretsConfig{
TypeMeta: metav1.TypeMeta{
APIVersion: "operator.openshift.io/v1alpha1",
Kind: "ExternalSecretsConfig",
},
ObjectMeta: metav1.ObjectMeta{
Name: "cluster",
Labels: map[string]string{
"app.kubernetes.io/name": "cluster",
"app.kubernetes.io/managed-by": "external-secrets-operator-e2e",
},
},
Spec: v1alpha1.ExternalSecretsConfigSpec{
ControllerConfig: v1alpha1.ControllerConfig{
NetworkPolicies: []v1alpha1.NetworkPolicy{
{
Name: "allow-external-secrets-egress",
ComponentName: v1alpha1.CoreController,
Egress: []networkingv1.NetworkPolicyEgressRule{
{
To: []networkingv1.NetworkPolicyPeer{},
Ports: []networkingv1.NetworkPolicyPort{
{Protocol: &tcp, Port: &port6443}, // Kubernetes API
{Protocol: &tcp, Port: &port443}, // HTTPS (AWS Secrets Manager)
{Protocol: &tcp, Port: &port5353}, // DNS
{Protocol: &udp, Port: &port5353}, // DNS
},
},
},
},
},
},
},
}
}
// Helper function to create ExternalSecretsConfig YAML without componentConfigs
// This matches the signature expected by CreateFromFile/DeleteFromFile
func createExternalSecretsConfigBase(filename string) ([]byte, error) {
config := createBaseExternalSecretsConfig()
return yaml.Marshal(config)
}
// Helper function to create ExternalSecretsConfig with custom revision history limits
// Returns a function matching the signature expected by CreateFromFile/DeleteFromFile
func createExternalSecretsConfigWithRevisionLimits(controllerLimit, webhookLimit, certControllerLimit int32) func(string) ([]byte, error) {
return func(filename string) ([]byte, error) {
config := createBaseExternalSecretsConfig()
// Add componentConfigs with revision history limits
config.Spec.ControllerConfig.ComponentConfigs = []v1alpha1.ComponentConfig{
{
ComponentName: v1alpha1.CoreController,
DeploymentConfigs: &v1alpha1.DeploymentConfig{
RevisionHistoryLimit: &controllerLimit,
},
},
{
ComponentName: v1alpha1.Webhook,
DeploymentConfigs: &v1alpha1.DeploymentConfig{
RevisionHistoryLimit: &webhookLimit,
},
},
{
ComponentName: v1alpha1.CertController,
DeploymentConfigs: &v1alpha1.DeploymentConfig{
RevisionHistoryLimit: &certControllerLimit,
},
},
}
return yaml.Marshal(config)
}
}
@bharath-b-rh @siddhibhor-56 What do you think?
siddhibhor-56
force-pushed
the
ESO-278
branch
from
2499240 to
c8cc767
Compare
|
@siddhibhor-56: 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 |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh, mytreya-rh, siddhibhor-56 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
6 participants
revisionHistoryLimiton a per-component basis for external-secrets operand deployments.