ESO-266:Adds an enhancement proposal to add override configuration feilds in external-secrets-operator api #1898
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
siddhibhor-56
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@siddhibhor-56: This pull request references ESO-266 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.21.0" version, but no target version was set. 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. |
|
@siddhibhor-56: This pull request references ESO-266 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.21.0" version, but no target version was set. 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. |
|
@siddhibhor-56: This pull request references ESO-266 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.21.0" version, but no target version was set. 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. |
|
@siddhibhor-56: all tests passed! 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. |
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] Architectural Review - OpenShift Perspective
After reviewing the existing API (CommonConfigs in ApplicationConfig) and this proposal, here are key observations:
What's Already Covered Globally (via ApplicationConfig.CommonConfigs):
- ✅ Resources (requests/limits) - line 550 of existing API
- ✅ LogLevel - line 544 of existing API
- ✅ Tolerations - line 564 of existing API
- ✅ NodeSelector - line 573 of existing API
- ✅ Affinity - line 555 of existing API
What This PR Adds:
- Component-specific deployment overrides (RevisionHistoryLimit via overrideArgs)
- Global annotations for all components
- Component-specific environment variables (overrideEnv)
Critical Gaps & Concerns:
See inline comments for detailed architectural feedback on upgradeability, maintainability, and scalability.
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] Critical: Missing SCC/SecurityContext customization
The customer requirement explicitly states: 'remove runAsUser as we have more restrictive SCCs'. This is not addressed in the proposal.
OpenShift SCCs are a critical security boundary. Administrators need to customize pod security contexts (runAsUser, fsGroup, seccompProfile, etc.) to comply with custom SCCs.
Recommendation: Add a securityContext field to ComponentConfig or document why this is out of scope and how users should handle SCC compatibility.
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] Architectural: overrideArgs extensibility model is fragile
The string-based "Key:Value" parsing (e.g., "RevisionHistoryLimit:5") has significant scalability and maintainability issues:
- Regex updates required for each new override: The XValidation regex must be updated for every new override type, requiring API changes
- No support for complex values: Cannot handle values containing colons, commas, or structured data
- Poor discoverability: Users must consult documentation to know valid keys
- Error-prone: Typos like "RevisionHistorylimit:5" (lowercase 'l') are accepted but fail silently at runtime
Recommendation: Use a structured approach with typed fields instead of string parsing. This provides type safety, better validation, and easier extension.
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] Upgradeability: Missing conflict resolution strategy
When the operator upgrades, new defaults might conflict with user-configured overrides. The proposal doesn't specify:
- Precedence rules: If operator v2 changes a default that conflicts with user's overrideEnv, which wins?
- Breaking changes: How are users notified if their overrides become invalid or deprecated?
- Status feedback: No mechanism to report which overrides were applied vs rejected vs overridden
Recommendation:
- Add explicit precedence rules: "user overrides always take precedence over operator defaults"
- Add status conditions to ExternalSecretsConfig reporting override application status
- Document the upgrade contract in the "Upgrade / Downgrade Strategy" section
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] Security: Insufficient specification of protected env vars/annotations
The mitigation states "The operator will protect certain critical environment variables" and "The operator will protect certain critical arguments" but this is too vague for production.
Required specifications:
- Explicit list of protected env vars: e.g., KUBERNETES_SERVICE_HOST, POD_NAME, POD_NAMESPACE, SERVICE_ACCOUNT, etc.
- Explicit list of protected annotations: e.g., openshift.io/, operator.openshift.io/, kubernetes.io/*
- Behavior when user tries to override protected items: Reject at admission time? Accept but ignore? Log warning?
- Extensibility: How are new protected items added as the operator evolves?
Recommendation:
- Document the complete allow/deny model in this enhancement
- Consider rejecting protected overrides at API validation time rather than logging warnings (fail-fast principle)
- Add integration tests verifying protection mechanisms work
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] API Design: Annotation validation underspecified
The proposal states "validates that annotation keys and values conform to Kubernetes annotation constraints" but doesn't address OpenShift-specific concerns:
- Reserved annotation prefixes: Should the operator block openshift.io/, operator.openshift.io/, kubernetes.io/* annotations?
- Operator-managed annotations: What happens if users try to set annotations the operator itself manages?
- Annotation conflicts: User annotation overwrites operator default - is this allowed? Documented?
Example scenario: User sets operator.openshift.io/managed-by: custom which could interfere with platform management.
Recommendation:
- Add validation to reject reserved annotation prefixes at admission time
- Document which annotations are reserved/protected in the API docs
- Add XValidation rule to enforce annotation prefix restrictions
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] Maintainability: RevisionHistoryLimit validation should enforce minimum
The risk mitigation recommends values between 3-5, but the XValidation only checks for non-negative integers.
Problem: Setting RevisionHistoryLimit:0 is accepted by validation but eliminates all rollback capability, which is almost never correct and severely impacts recoverability.
Recommendation: Add minimum value validation in the API. The regex should ensure at least value 1, or better yet, use a structured field with proper integer validation and minimum/maximum constraints.
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] Test Coverage: Missing upgrade/downgrade tests
The test plan doesn't include tests for the critical upgrade/downgrade scenarios mentioned in the Upgrade/Downgrade Strategy section.
Required test scenarios:
- Upgrade with existing user configs - verify configs are preserved and still applied
- Upgrade where new operator defaults conflict with user overrides - verify user overrides win
- Downgrade removes unsupported fields - verify graceful degradation
- Upgrade adds new override types - verify backward compatibility
- Verify status conditions accurately report override application (success/failure/warning)
These tests are essential for production confidence in the upgrade path.
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] Scalability: MaxItems=50 seems arbitrary and excessive
Why allow 50 override args when only RevisionHistoryLimit is currently supported?
Concerns:
- Invitation to abuse: High limit makes it easier for users to add excessive overrides
- Validation complexity: More overrides = harder to validate and debug
- Future maintenance burden: Harder to change once users depend on the limit
Recommendation: Set a conservative limit based on expected near-term usage (e.g., 5-10 overrides). This can be increased in future versions as new override types are added, but decreasing a limit is a breaking change.
The limit should reflect actual anticipated usage, not theoretical maximum.
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] Operational: Missing observability for override application
The support procedures don't provide operators with visibility into override application status. This is critical for troubleshooting production issues.
Required observability:
- Status conditions: Which overrides were successfully applied
- Warnings/rejections: Which overrides were rejected due to protection rules
- Conflicts: Which user overrides overwrote operator defaults
- Validation failures: Which overrides failed runtime validation
Recommendation: Add status conditions to ExternalSecretsConfig that report which overrides were applied, rejected, or resulted in warnings. This enables support engineers to quickly diagnose misconfigurations without needing to inspect pod specs manually.
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] Documentation: Add AWS STS environment variable example
The customer requirement specifically mentions setting AWS-specific environment variables for STS.
Recommendation: Add a concrete example showing AWS STS configuration with variables like AWS_ROLE_ARN, AWS_WEB_IDENTITY_TOKEN_FILE, and AWS_REGION. This validates that the design meets the stated customer requirement and provides a practical reference for users migrating from helm charts.
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] API Versioning: Consider v1alpha1 instead of direct GA
The proposal states this will go GA directly, but several aspects are underspecified:
Undefined behaviors:
- Which env vars/annotations are protected (vague "certain critical" language)
- How conflicts between user overrides and operator defaults are resolved
- Status feedback mechanism for override application
- Security boundary enforcement details
Recommendation: Consider v1alpha1 to allow API refinement based on real-world usage patterns before committing to API stability guarantees. Once the implementation proves stable and usage patterns are established, promote to v1beta1 or v1. Alternatively, if GA is required, document all undefined behaviors explicitly and add status feedback mechanisms to avoid breaking changes later.
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] Architecture: Clarify component-specific vs global configuration strategy
The existing API (ApplicationConfig.CommonConfigs) provides global configuration for resources, logLevel, tolerations, nodeSelector, and affinity. This PR adds component-specific overrideEnv.
Question: Why is environment variable configuration component-specific while resources/tolerations/nodeSelector are global-only?
Potential scenarios requiring component-specific resources/tolerations:
- Controller needs more CPU/memory than Webhook
- BitwardenSDKServer requires different node placement
- CertController needs different tolerations for compliance nodes
Recommendation: Document the rationale for why global-only is sufficient for most settings and component-specific env vars are a special case. The current mixed model (some global, some component-specific) should be intentional and documented, not accidental.
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
[claude-generated] Consistency: ComponentName enum values need clarification
The proposal mentions four components (Controller, Webhook, CertController, BitwardenSDKServer) but the enum comment at line 82 says "Valid values: Controller, Webhook, CertController, Bitwarden" while using different actual enum values (ExternalSecretsCoreController, Webhook, CertController, BitwardenSDKServer).
Recommendation: Ensure ComponentName enum values are clearly documented and match across the enhancement and the actual API code. All four components should be explicitly defined if they're all supported.
|
|
||
| ### Goals | ||
|
|
||
| - Provide a declarative API for specifying deployment lifecycle overrides for each component via `componentConfig`. |
There was a problem hiding this comment.
[claude-generated] Critical: Missing SCC/SecurityContext customization
The customer requirement explicitly states: 'remove runAsUser as we have more restrictive SCCs'. This is not addressed in the proposal.
OpenShift SCCs are a critical security boundary. Administrators need to customize pod security contexts (runAsUser, fsGroup, seccompProfile, etc.) to comply with custom SCCs.
Recommendation: Add a securityContext field to ComponentConfig or document why this is out of scope and how users should handle SCC compatibility.
There was a problem hiding this comment.
already covered in TP version
| // - RevisionHistoryLimit:<int> - Number of old ReplicaSets to retain (e.g., "RevisionHistoryLimit:12") | ||
| // | ||
| // +listType=atomic | ||
| // +kubebuilder:validation:MinItems:=0 |
There was a problem hiding this comment.
[claude-generated] Architectural: overrideArgs extensibility model is fragile
The string-based "Key:Value" parsing (e.g., "RevisionHistoryLimit:5") has significant scalability and maintainability issues:
- Regex updates required for each new override: The XValidation regex must be updated for every new override type, requiring API changes
- No support for complex values: Cannot handle values containing colons, commas, or structured data
- Poor discoverability: Users must consult documentation to know valid keys
- Error-prone: Typos like "RevisionHistorylimit:5" (lowercase 'l') are accepted but fail silently at runtime
Recommendation: Use a structured approach with typed fields instead of string parsing. This provides type safety, better validation, and easier extension.
|
|
||
| * **Upgrade:** On upgrade, the new `annotations` and `componentConfig` fields (including `overrideArgs` and `overrideEnv`) will be available. Existing installations without these configurations will continue to work with default settings. Users can optionally add annotations, deployment overrides, and custom environment variables after upgrade. | ||
|
|
||
| * **Downgrade:** If a user downgrades to a version that doesn't support `annotations` or `componentConfig`, these fields will be ignored by the older operator. Deployments will revert to default configurations, custom annotations will be removed, and custom environment variables will be reset. Users should be aware that custom configurations will be lost on downgrade. |
There was a problem hiding this comment.
[claude-generated] Upgradeability: Missing conflict resolution strategy
When the operator upgrades, new defaults might conflict with user-configured overrides. The proposal doesn't specify:
- Precedence rules: If operator v2 changes a default that conflicts with user's overrideEnv, which wins?
- Breaking changes: How are users notified if their overrides become invalid or deprecated?
- Status feedback: No mechanism to report which overrides were applied vs rejected vs overridden
Recommendation:
- Add explicit precedence rules: "user overrides always take precedence over operator defaults"
- Add status conditions to ExternalSecretsConfig reporting override application status
- Document the upgrade contract in the "Upgrade / Downgrade Strategy" section
| * **Risk:** Users may accidentally override critical arguments required for proper operation. | ||
| * **Mitigation:** The operator will protect certain critical arguments from being overridden and will log warnings if users attempt to do so. | ||
|
|
||
| * **Risk:** Users may override critical environment variables required for proper component operation. |
There was a problem hiding this comment.
[claude-generated] Security: Insufficient specification of protected env vars/annotations
The mitigation states "The operator will protect certain critical environment variables" and "The operator will protect certain critical arguments" but this is too vague for production.
Required specifications:
- Explicit list of protected env vars: e.g., KUBERNETES_SERVICE_HOST, POD_NAME, POD_NAMESPACE, SERVICE_ACCOUNT, etc.
- Explicit list of protected annotations: e.g., openshift.io/, operator.openshift.io/, kubernetes.io/*
- Behavior when user tries to override protected items: Reject at admission time? Accept but ignore? Log warning?
- Extensibility: How are new protected items added as the operator evolves?
Recommendation:
- Document the complete allow/deny model in this enhancement
- Consider rejecting protected overrides at API validation time rather than logging warnings (fail-fast principle)
- Add integration tests verifying protection mechanisms work
|
|
||
| **For Global Annotations:** | ||
|
|
||
| 1. **User Configuration:** Administrator updates the `ExternalSecretsConfig` CR with the `controllerConfig.annotations` field containing custom key-value pairs. |
There was a problem hiding this comment.
[claude-generated] API Design: Annotation validation underspecified
The proposal states "validates that annotation keys and values conform to Kubernetes annotation constraints" but doesn't address OpenShift-specific concerns:
- Reserved annotation prefixes: Should the operator block openshift.io/, operator.openshift.io/, kubernetes.io/* annotations?
- Operator-managed annotations: What happens if users try to set annotations the operator itself manages?
- Annotation conflicts: User annotation overwrites operator default - is this allowed? Documented?
Example scenario: User sets `operator.openshift.io/managed-by: custom` which could interfere with platform management.
Recommendation:
- Add validation to reject reserved annotation prefixes at admission time
- Document which annotations are reserved/protected in the API docs
- Add XValidation rule to enforce annotation prefix restrictions
| // Currently supported deployment-level overrides: | ||
| // - RevisionHistoryLimit:<int> - Number of old ReplicaSets to retain (e.g., "RevisionHistoryLimit:12") | ||
| // | ||
| // +listType=atomic |
There was a problem hiding this comment.
[claude-generated] Maintainability: RevisionHistoryLimit validation should enforce minimum
The risk mitigation recommends values between 3-5, but the XValidation only checks for non-negative integers.
Problem: Setting RevisionHistoryLimit:0 is accepted by validation but eliminates all rollback capability, which is almost never correct and severely impacts recoverability.
Recommendation: Add minimum value validation in the API. The regex should ensure at least value 1, or better yet, use a structured field with proper integer validation and minimum/maximum constraints.
| * **Integration Tests:** | ||
| 1. Deploy the operator and create an `ExternalSecretsConfig` with component configuration. | ||
| 2. Verify that "RevisionHistoryLimit:X" is correctly applied to the deployment's `spec.revisionHistoryLimit`. | ||
| 3. Verify that specified annotations appear on both Deployment and Pod template. |
There was a problem hiding this comment.
[claude-generated] Test Coverage: Missing upgrade/downgrade tests
The test plan doesn't include tests for the critical upgrade/downgrade scenarios mentioned in the Upgrade/Downgrade Strategy section.
Required test scenarios:
- Upgrade with existing user configs - verify configs are preserved and still applied
- Upgrade where new operator defaults conflict with user overrides - verify user overrides win
- Downgrade removes unsupported fields - verify graceful degradation
- Upgrade adds new override types - verify backward compatibility
- Verify status conditions accurately report override application (success/failure/warning)
These tests are essential for production confidence in the upgrade path.
| // | ||
| // Currently supported deployment-level overrides: | ||
| // - RevisionHistoryLimit:<int> - Number of old ReplicaSets to retain (e.g., "RevisionHistoryLimit:12") | ||
| // |
There was a problem hiding this comment.
[claude-generated] Scalability: MaxItems=50 seems arbitrary and excessive
Why allow 50 override args when only RevisionHistoryLimit is currently supported?
Concerns:
- Invitation to abuse: High limit makes it easier for users to add excessive overrides
- Validation complexity: More overrides = harder to validate and debug
- Future maintenance burden: Harder to change once users depend on the limit
Recommendation: Set a conservative limit based on expected near-term usage (e.g., 5-10 overrides). This can be increased in future versions as new override types are added, but decreasing a limit is a breaking change.
The limit should reflect actual anticipated usage, not theoretical maximum.
|
|
||
| ## Support Procedures | ||
|
|
||
| Support personnel debugging configuration issues should: |
There was a problem hiding this comment.
[claude-generated] Operational: Missing observability for override application
The support procedures don't provide operators with visibility into override application status. This is critical for troubleshooting production issues.
Required observability:
- Status conditions: Which overrides were successfully applied
- Warnings/rejections: Which overrides were rejected due to protection rules
- Conflicts: Which user overrides overwrote operator defaults
- Validation failures: Which overrides failed runtime validation
Recommendation: Add status conditions to ExternalSecretsConfig that report which overrides were applied, rejected, or resulted in warnings. This enables support engineers to quickly diagnose misconfigurations without needing to inspect pod specs manually.
|
|
||
| ```yaml | ||
| apiVersion: operator.openshift.io/v1alpha1 | ||
| kind: ExternalSecretsConfig |
There was a problem hiding this comment.
[claude-generated] Documentation: Add AWS STS environment variable example
The customer requirement specifically mentions setting AWS-specific environment variables for STS.
Recommendation: Add a concrete example showing AWS STS configuration with variables like AWS_ROLE_ARN, AWS_WEB_IDENTITY_TOKEN_FILE, and AWS_REGION. This validates that the design meets the stated customer requirement and provides a practical reference for users migrating from helm charts.
| * Argument merging logic is complete. | ||
| * Annotation merging logic is complete and applies to both Deployment and Pod template. | ||
| * Environment variable merging logic is complete and applies to the container spec. | ||
| * All tests outlined in the Test Plan are passing. |
There was a problem hiding this comment.
[claude-generated] API Versioning: Consider v1alpha1 instead of direct GA
The proposal states this will go GA directly, but several aspects are underspecified:
Undefined behaviors:
- Which env vars/annotations are protected (vague "certain critical" language)
- How conflicts between user overrides and operator defaults are resolved
- Status feedback mechanism for override application
- Security boundary enforcement details
Recommendation: Consider v1alpha1 to allow API refinement based on real-world usage patterns before committing to API stability guarantees. Once the implementation proves stable and usage patterns are established, promote to v1beta1 or v1. Alternatively, if GA is required, document all undefined behaviors explicitly and add status feedback mechanisms to avoid breaking changes later.
| - Provide a declarative API for specifying deployment lifecycle overrides for each component via `componentConfig`. | ||
| - Provide a declarative API for adding custom annotations globally to all component Deployments and Pod templates via `controllerConfig.annotations`. | ||
| - Provide a declarative API for specifying custom environment variables for each component via `componentConfig.overrideEnv`. | ||
| - Support all four operand components: Controller, Webhook, CertController, and BitwardenSDKServer. |
There was a problem hiding this comment.
[claude-generated] Architecture: Clarify component-specific vs global configuration strategy
The existing API (ApplicationConfig.CommonConfigs) provides global configuration for resources, logLevel, tolerations, nodeSelector, and affinity. This PR adds component-specific overrideEnv.
Question: Why is environment variable configuration component-specific while resources/tolerations/nodeSelector are global-only?
Potential scenarios requiring component-specific resources/tolerations:
- Controller needs more CPU/memory than Webhook
- BitwardenSDKServer requires different node placement
- CertController needs different tolerations for compliance nodes
Recommendation: Document the rationale for why global-only is sufficient for most settings and component-specific env vars are a special case. The current mixed model (some global, some component-specific) should be intentional and documented, not accidental.
| // ComponentConfig represents component-specific configuration overrides. | ||
| type ComponentConfig struct { | ||
| // componentName specifies which deployment component this configuration applies to. | ||
| // Valid values: Controller, Webhook, CertController, Bitwarden |
There was a problem hiding this comment.
[claude-generated] Consistency: ComponentName enum values need clarification
The proposal mentions four components (Controller, Webhook, CertController, BitwardenSDKServer) but the enum comment at line 82 says "Valid values: Controller, Webhook, CertController, Bitwarden" while using different actual enum values (ExternalSecretsCoreController, Webhook, CertController, BitwardenSDKServer).
Recommendation: Ensure ComponentName enum values are clearly documented and match across the enhancement and the actual API code. All four components should be explicitly defined if they're all supported.
|
|
||
| ## Motivation | ||
|
|
||
| Administrators often need to control deployment lifecycle settings and add custom annotations without directly modifying the underlying operator-managed Deployment resources. |
There was a problem hiding this comment.
i think the motivation is for the operator to be able to derive the operand deployment manifests in a consistent way using the CR while allowing the user finer control of resource management and operational parameters.
Otherwise, we can also exclude annotation and revision history checks from being considered as a change on the deployment spec and let the user directly update those fields on the operand deployment, right?
| // +kubebuilder:validation:XValidation:rule="self.all(x, x.matches('^RevisionHistoryLimit:\\d+$'))",message="Only deployment-level overrides for RevisionHistoryLimit are supported, which must be followed by a non-negative integer (e.g., RevisionHistoryLimit:5)." | ||
| // +kubebuilder:validation:Optional | ||
| // +optional | ||
| OverrideArgs []string `json:"overrideArgs,omitempty"` |
There was a problem hiding this comment.
As these are part of DeploymentSpec, can the name reflect the same instead of OverrideArgs ?
bharath-b-rh
left a comment
bharath-b-rh
left a comment
There was a problem hiding this comment.
My suggestions are mainly around keeping it generic, so that in future other such override are asked, same EP can be extended. Feel free to discard any which may not be required.
| creation-date: 2025-12-1 | ||
| last-updated: 2025-12-5 | ||
| tracking-link: | ||
| - https://issues.redhat.com/browse/ESO-266 |
There was a problem hiding this comment.
| - https://issues.redhat.com/browse/ESO-266 | |
| - https://issues.redhat.com/browse/RFE-7842 | |
| - https://issues.redhat.com/browse/OCPSTRAT-2419 | |
| - https://issues.redhat.com/browse/ESO-266 |
|
|
||
| ## Summary | ||
|
|
||
| This document proposes an enhancement to the `ExternalSecretsConfig` API by introducing a `ComponentConfig` extension and global annotations support through `Annotations` field. This allows administrators to specify component-specific overrides for deployment lifecycle settings, custom environment variables, and global custom annotations for external-secrets components (Controller, Webhook, CertController, BitwardenSDKServer). This change offers administrators greater control over the resource management and operational parameters of components. |
There was a problem hiding this comment.
| This document proposes an enhancement to the `ExternalSecretsConfig` API by introducing a `ComponentConfig` extension and global annotations support through `Annotations` field. This allows administrators to specify component-specific overrides for deployment lifecycle settings, custom environment variables, and global custom annotations for external-secrets components (Controller, Webhook, CertController, BitwardenSDKServer). This change offers administrators greater control over the resource management and operational parameters of components. | |
| The External Secrets Operator for Red Hat OpenShift provides limited configuration options via its `ExternalSecretsConfig` API, constraining user customization. This enhancement proposes extending the `ExternalSecretsConfig` API to allow comprehensive customization of the external-secrets deployment. The extended configuration options—including annotations, environment variables, and deployment/pod specifications will be available for all core components (Controller, Webhook, CertController, BitwardenSDKServer). This change provides administrators with greater control over the resource management and operational parameters of each component. |
|
|
||
| ## Motivation | ||
|
|
||
| Administrators often need to control deployment lifecycle settings and add custom annotations without directly modifying the underlying operator-managed Deployment resources. |
There was a problem hiding this comment.
| Administrators often need to control deployment lifecycle settings and add custom annotations without directly modifying the underlying operator-managed Deployment resources. | |
| Administrators often need to control core operational parameters, lifecycle settings, and custom metadata without directly modifying the underlying operator-managed resources. Currently, any manual changes made directly to the operand resources or other component specifications are immediately overwritten by the operator, making persistent customization impossible. This hardening forces users to accept default settings that may not be optimal for their workloads. This proposal resolves this issue by providing a dedicated, supported configuration path through `ExternalSecretsConfig`, granting administrators the necessary flexibility to fine-tune essential specifications like revisionHistoryLimit, add crucial environment variables, and apply additional metadata for seamless and efficient integration into complex cluster environments. |
|
|
||
| ### User Stories | ||
|
|
||
| - As an administrator, I want to customize deployment lifecycle properties for `external-secrets` operand components to manage their resource consumption and rollback behavior. |
There was a problem hiding this comment.
| - As an administrator, I want to customize deployment lifecycle properties for `external-secrets` operand components to manage their resource consumption and rollback behavior. | |
| - As an OpenShift administrator, I want to configure the deployment lifecycle properties (e.g., revisionHistoryLimit) for external-secrets operand components using the `ExternalSecretsConfig` API so that I can control their rollback behavior and optimize cluster resource consumption. |
| ### User Stories | ||
|
|
||
| - As an administrator, I want to customize deployment lifecycle properties for `external-secrets` operand components to manage their resource consumption and rollback behavior. | ||
| - As an `external-secrets` user, I want to be able to set specific deployment override values independently for each component via ComponentConfig to meet unique operational requirements. |
There was a problem hiding this comment.
| - As an `external-secrets` user, I want to be able to set specific deployment override values independently for each component via ComponentConfig to meet unique operational requirements. | |
| - As an OpenShift Administrator, I need to apply unique configuration overrides (e.g., revisionHistoryLimit) to individual external-secrets components (Controller, Webhook, etc.) so that I can meet the specific operational and resource requirements of each component efficiently. |
|
|
||
| **For Component Configuration:** | ||
|
|
||
| 1. **User Configuration:** Administrator updates the `ExternalSecretsConfig` CR, utilizing the new `componentConfig` list to specify configuration entries for a component (Controller, Webhook, etc.). This includes deployment-level overrides via `overrideArgs` and custom environment variables via `overrideEnv`. |
There was a problem hiding this comment.
| 1. **User Configuration:** Administrator updates the `ExternalSecretsConfig` CR, utilizing the new `componentConfig` list to specify configuration entries for a component (Controller, Webhook, etc.). This includes deployment-level overrides via `overrideArgs` and custom environment variables via `overrideEnv`. | |
| 1. **User Configuration:** Administrator updates the `ExternalSecretsConfig` CR, utilizing the new `componentConfig` list to specify configuration entries for a component (Controller, Webhook, etc.). This includes deployment-level overrides via `overrideDeploymentConfig` and custom environment variables via `overrideEnv`. |
| **For Component Configuration:** | ||
|
|
||
| 1. **User Configuration:** Administrator updates the `ExternalSecretsConfig` CR, utilizing the new `componentConfig` list to specify configuration entries for a component (Controller, Webhook, etc.). This includes deployment-level overrides via `overrideArgs` and custom environment variables via `overrideEnv`. | ||
| 2. **Validation:** It verifies the `componentName` against the allowed enum values and enforces uniqueness across the list. It strictly validates the `OverrideArgs` field using the provided `XValidation` rule, ensuring every entry uses the specified format. For `overrideEnv`, it validates that environment variable names and values conform to Kubernetes conventions. |
There was a problem hiding this comment.
| 2. **Validation:** It verifies the `componentName` against the allowed enum values and enforces uniqueness across the list. It strictly validates the `OverrideArgs` field using the provided `XValidation` rule, ensuring every entry uses the specified format. For `overrideEnv`, it validates that environment variable names and values conform to Kubernetes conventions. | |
| 2. **Validation:** It verifies the `componentName` against the allowed enum values and enforces uniqueness across the list. It strictly validates the `overrideDeploymentConfig` field using the provided Kubernetes CEL validation rules, ensuring every entry uses the specified format. For `overrideEnv`, it validates that environment variable names and values conform to Kubernetes conventions. |
|
|
||
| 1. **User Configuration:** Administrator updates the `ExternalSecretsConfig` CR, utilizing the new `componentConfig` list to specify configuration entries for a component (Controller, Webhook, etc.). This includes deployment-level overrides via `overrideArgs` and custom environment variables via `overrideEnv`. | ||
| 2. **Validation:** It verifies the `componentName` against the allowed enum values and enforces uniqueness across the list. It strictly validates the `OverrideArgs` field using the provided `XValidation` rule, ensuring every entry uses the specified format. For `overrideEnv`, it validates that environment variable names and values conform to Kubernetes conventions. | ||
| 3. **Reconciliation:** It parses the `OverrideArgs` field to identify and extract the deployment override key and its corresponding value. It updates the component's underlying Kubernetes Deployment resource by setting the parsed override value in the appropriate `.spec` field. For `overrideEnv`, the operator merges user-specified environment variables with default variables, with user values taking precedence in case of conflicts. |
There was a problem hiding this comment.
| 3. **Reconciliation:** It parses the `OverrideArgs` field to identify and extract the deployment override key and its corresponding value. It updates the component's underlying Kubernetes Deployment resource by setting the parsed override value in the appropriate `.spec` field. For `overrideEnv`, the operator merges user-specified environment variables with default variables, with user values taking precedence in case of conflicts. | |
| 3. **Reconciliation:** It parses the `overrideDeploymentConfig` field to identify and extract the deployment override key and its corresponding value. It updates the component's underlying Kubernetes Deployment resource by setting the parsed override value in the appropriate `.spec` field. For `overrideEnv`, the operator merges user-specified environment variables with default variables, with user values taking precedence in case of conflicts. |
| componentConfig: | ||
| - componentName: ExternalSecretsCoreController | ||
| overrideEnv: | ||
| - name: HTTP_PROXY |
There was a problem hiding this comment.
Since we have dedicated config for proxy, I would use different for example.
| * **Mitigation:** The operator will protect certain critical arguments from being overridden and will log warnings if users attempt to do so. | ||
|
|
||
| * **Risk:** Users may override critical environment variables required for proper component operation. | ||
| * **Mitigation:** The operator will protect certain critical environment variables from being overridden and will log warnings if users attempt to do so. |
There was a problem hiding this comment.
| * **Mitigation:** The operator will protect certain critical environment variables from being overridden and will log warnings if users attempt to do so. | |
| * **Mitigation:** The operator can protect certain critical environment variables from being overridden and will log warnings if users attempt to do so. |
4 participants
This PR introduces the
componentConfigextension to theExternalSecretsConfigAPI andAnnotationsfeild allowing administrators to configure specific, component-level deployment settings and global custom annotations. This enhancement adds user-configurable settings for critical deployment parameters