ESO-540/ESO-550: Adds concurrent flag, replicas, and experimentalOverrides to ExternalSecretsConfig - #2068
Open
siddhibhor-56 wants to merge 5 commits into
Open
ESO-540/ESO-550: Adds concurrent flag, replicas, and experimentalOverrides to ExternalSecretsConfig#2068siddhibhor-56 wants to merge 5 commits into
siddhibhor-56 wants to merge 5 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@siddhibhor-56: This pull request references ESO-533 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 epic 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. |
siddhibhor-56
marked this pull request as draft
July 27, 2026 10:42
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe proposal expands ChangesExternal Secrets controller configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
siddhibhor-56
changed the title
siddhibhor-56
changed the title
siddhibhor-56
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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 `@enhancements/external-secrets-operator/external-secrets-component-config.md`:
- Line 22: Update the Jira metadata for ESO-533 so its target version is 5.0.0,
or document the intentional exception in the external-secrets component
configuration if the target version should remain unset.
- Around line 191-198: Update the workflow and support sections around the
visible forbidden-path references, including the diagram’s “Reject forbidden
nested lists” step, to consistently include containers, initContainers, volumes,
ephemeralContainers, volumeMounts, and volumeDevices. Preserve the existing
forbidden-path handling and ensure every mention uses this complete normative
list.
- Around line 105-115: Update the trustedCABundle mount decision in the External
Secrets controller reconciliation to skip the user CA volume only when the
referenced ConfigMap is confirmed to be the known CNO/proxy-managed bundle
already mounted in the controller pod. Do not skip mounting solely because the
ConfigMap has the injection label; mount arbitrary labeled references normally,
while preserving existing optional and invalid-PEM handling.
- Around line 471-479: Update the required podAntiAffinity selectors in both
examples around the affinity configuration to use the exact labels emitted on
the operator-rendered core-controller pods instead of hard-coded
app.kubernetes.io/name: external-secrets. Verify the selector matches those pods
and add or update the relevant test to enforce that match.
- Around line 140-152: The experimentalOverrides validation described in the
component configuration must enforce an explicit allowlist rather than only
rejecting forbidden paths. Update the override validation or application logic
associated with the external secrets component to permit only the documented
scheduling paths under Deployment.spec, such as template.spec.affinity,
tolerations, nodeSelector, and topologySpreadConstraints, and reject every other
path—including currently unlisted fields—by setting Degraded and skipping the
patch.
- Around line 680-685: Clarify the Downgrade section to document the supported
downgrade procedure: distinguish operator-image rollback from the active CRD
schema transition, specify the required CRD/version change, and describe backup
and reapply expectations when fields are rejected or pruned. State the exact
supported downgrade path before describing the resulting behavior or
availability risks.
- Around line 696-700: The upgrade discussion must not describe OLM bundle
upgrades as atomic or imply there is no version skew. Update the section around
the ExternalSecretsConfig fields to document the supported upgrade order,
acknowledge that operator and operand Deployments may temporarily run different
revisions, and specify the behavior when ExternalSecretsConfig is changed before
the new operator finishes rolling out.
In `@enhancements/installer/gcp-sovereign-cloud.md`:
- Around line 320-362: Update validateGCPServices to probe all six required
services listed in requiredServices, rather than validating only Compute. Add
bounded API-access checks for Cloud Resource Manager, DNS, IAM, IAM Credentials,
Service Usage, and Compute; classify any non-required capabilities separately as
optional, and return a clear service-specific error immediately when a required
probe is unavailable so provisioning does not begin.
- Around line 654-692: The GCD installer documentation does not specify
enforceable defaults or preflight checks for unsupported resources. Update the
installer configuration and provisioning flow to use supported C3/M3/A3 Edge
machine types and Hyperdisk Balanced disks, and validate load balancers, DNS
zones, and SSL certificates before infrastructure creation, failing fast with
actionable errors for unsupported global, public, or managed options.
- Around line 340-352: Make the snippets around validateGCPServices and the
related resource-manager example self-contained: define or pass projectID
explicitly, expose client.projectID if it is intended, and provide the
GetResourceManagerService helper before use. If these examples are intentionally
illustrative, label them clearly as pseudocode instead of leaving undefined
references.
- Around line 373-400: Update validateRegion to follow the nextPageToken
returned by computeSvc.Regions.List(client.projectID).Do(), fetching and
scanning every regions page before concluding the region is unavailable.
Accumulate region names from all pages so both validation and the
available-regions error message include the complete result set.
- Around line 62-69: Unify the GCP configuration contract across
enhancements/installer/gcp-sovereign-cloud.md:62-69 by defining universeDomain
as the canonical source, its precedence over cloudEnvironment and other inputs,
defaults, and mismatch behavior; update
enhancements/installer/gcp-sovereign-cloud.md:591-603 so the GCD example uses
only documented install-config fields; and update
enhancements/installer/gcp-sovereign-cloud.md:791-805 so version-skew handling
derives from the same source of truth used for component discovery.
- Around line 535-547: Update the credential-loading flow before
compute.NewService and the other GCP client constructors to use the typed
authentication loader instead of deprecated option.WithCredentialsJSON. Validate
the parsed credential type and universe/endpoint fields, rejecting unexpected
values before creating any client, while preserving automatic universe-domain
endpoint and authentication selection for supported credentials.
- Around line 218-240: Align the authentication guidance in
enhancements/installer/gcp-sovereign-cloud.md lines 218-240 and 471-508 to one
consistent current Go client-library contract: update the examples to use
option.WithAuthCredentialsJSON with option.ServiceAccount and credentials JSON,
and describe self-signed JWT as being derived by the SDK from the
service-account key rather than embedded in the JSON. Ensure both sections
consistently explain whether WithUniverseDomain is required, without mixing
incompatible authentication paths.
🪄 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: dce47c73-3036-4784-bc38-611c4f38ee6c
📒 Files selected for processing (2)
enhancements/external-secrets-operator/external-secrets-component-config.mdenhancements/installer/gcp-sovereign-cloud.md
Comment on lines
+105
to
+115
| `**ConfigMap` with the CNO injection label:** If the `ConfigMap` referenced by `trustedCABundle` is labeled with `config.openshift.io/inject-trusted-cabundle: "true"`, operator **skips** mounting that reference for `trustedCABundle`. | ||
|
|
||
| **Interaction with `overrideEnv`:** The operator owns `**SSL_CERT_DIR`** (and, when applicable, `**SSL_CERT_FILE**`) on the **External Secrets core controller** for proxy/CNO trust and for `**trustedCABundle`** injection. `**overrideEnv**` therefore **must not** set `**SSL_CERT_DIR`** or `**SSL_CERT_FILE**` on **any** operand component: the `**ExternalSecretsConfig`** CRD extends the existing `**overrideEnv**` CEL rule so the API server **rejects** those names up front (same pattern as reserved prefixes such as `KUBERNETES_`). No runtime “ignore vs reject” choice is required for a valid CR. Concurrency is **not** configurable via `overrideEnv`; use the first-class `**concurrent`** field (the operator builds container `args`, not an env var, for `--concurrent`). | ||
|
|
||
|
|
||
| | Situation | Expected behaviour | | ||
| | ------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `optional: false` (default) and missing `ConfigMap` or key | **Degraded**; do not patch the controller `Deployment` until valid. | | ||
| | `optional: true` and missing `ConfigMap` or key | Skip user bundle; no error for the missing reference alone. | | ||
| | Present key with **invalid PEM** | **Degraded** regardless of `optional`. | | ||
| | Referenced `ConfigMap` has `**config.openshift.io/inject-trusted-cabundle: "true"`** | A `ConfigMap` is already created, when proxy is configured, and its contents are mounted at `/etc/pki/tls/certs` path. Mounting it again under `/etc/pki/tls/user-certs` would be a duplicate. The operator **skips** the trustedCABundle volume mount. | |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not skip the user CA mount based only on the injection label.
A config.openshift.io/inject-trusted-cabundle: "true" label does not by itself prove that this referenced ConfigMap is the CNO-managed bundle already mounted in the controller pod. Skipping the mount for an arbitrary labeled ConfigMap can leave the controller without the requested trust roots. Restrict this optimization to the known CNO/proxy-mounted bundle and otherwise mount the referenced ConfigMap.
🤖 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 `@enhancements/external-secrets-operator/external-secrets-component-config.md`
around lines 105 - 115, Update the trustedCABundle mount decision in the
External Secrets controller reconciliation to skip the user CA volume only when
the referenced ConfigMap is confirmed to be the known CNO/proxy-managed bundle
already mounted in the controller pod. Do not skip mounting solely because the
ConfigMap has the injection label; mount arbitrary labeled references normally,
while preserving existing optional and invalid-PEM handling.
Contributor
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
enhancements/external-secrets-operator/external-secrets-component-config.md (1)
291-291: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
=for these Kubebuilder validation markers.
Enum:=,Minimum:=,Maximum:=,MinItems:=,MinLength:=,MaxLength:=, andPattern:=should all be=here, otherwise the generated CRD will miss the intended constraints.🤖 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 `@enhancements/external-secrets-operator/external-secrets-component-config.md` at line 291, Update the Kubebuilder validation marker near the component enum to use the supported equals syntax, changing the Enum marker from `Enum:=` to `Enum=` so the generated CRD preserves the constraint.
🤖 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.
Outside diff comments:
In `@enhancements/external-secrets-operator/external-secrets-component-config.md`:
- Line 291: Update the Kubebuilder validation marker near the component enum to
use the supported equals syntax, changing the Enum marker from `Enum:=` to
`Enum=` so the generated CRD preserves the constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 48b40099-9e37-4461-830e-e17b62e43ede
📒 Files selected for processing (1)
enhancements/external-secrets-operator/external-secrets-component-config.md
siddhibhor-56
changed the title
siddhibhor-56
marked this pull request as ready for review
July 29, 2026 18:44
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@enhancements/external-secrets-operator/external-secrets-component-config.md`:
- Around line 75-82: Update the Non-Goals wording around component-level
scheduling configuration to clarify that affinity, anti-affinity, tolerations,
and node selectors are excluded only as first-class API fields. Explicitly state
that these settings remain available through the documented
experimentalOverrides escape hatch, while preserving the existing restrictions
on operator-owned containers, initContainers, and volumes.
- Around line 635-637: Update the test plan around experimentalOverrides to
separate rejected protected-field patches from successful allowed scheduling
patches. Verify patches targeting image, --enable-leader-election, concurrent,
replicas, containers, initContainers, or volumes produce Degraded and leave the
Deployment unchanged; separately verify allowed scheduling patches succeed while
preserving the operator baseline.
- Around line 135-150: Remove the statement that webhook, cert-controller, and
Bitwarden replica counts can be configured through experimentalOverrides, since
the forbidden-path rules reject replicas patches. Update the surrounding
experimentalOverrides documentation to state that non-core replica counts are
unsupported while preserving the existing replicas prohibition.
🪄 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: 3acfa3ce-91fb-4fab-8f7a-9aa10d486786
📒 Files selected for processing (1)
enhancements/external-secrets-operator/external-secrets-component-config.md
siddhibhor-56
changed the title
|
@siddhibhor-56: This pull request references ESO-550 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. |
siddhibhor-56
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
enhancements/external-secrets-operator/external-secrets-component-config.md (1)
13-22: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSet the target version for ESO-550.
OpenShift CI confirms that ESO-550 is valid but has no target version, while this branch expects
5.0.0. Update the Jira metadata or document the intentional exception before merge.🤖 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 `@enhancements/external-secrets-operator/external-secrets-component-config.md` around lines 13 - 22, Update the Jira metadata associated with ESO-550 in the tracking-link section to set its target version to 5.0.0. If that metadata cannot be changed, document the intentional target-version exception in this component configuration instead.
🤖 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 `@enhancements/external-secrets-operator/external-secrets-component-config.md`:
- Around line 629-635: The test plan for experimentalOverrides must explicitly
verify that forbidden patches both set the component to Degraded and leave the
Deployment unchanged. Add parameterized rejection cases covering image,
args/command, trust-related environment variables, concurrent, replicas,
ephemeralContainers, volumeMounts, and volumeDevices, while keeping successful
scheduling-only patches in a separate test group.
- Line 188: Update the forbidden nested-list guidance in the workflow and
support references, including the diagram at the visible Op->>Op step and the
corresponding sections, to consistently name containers, initContainers,
ephemeralContainers, volumes, volumeMounts, and volumeDevices. Replace
abbreviated “mounts” wording with the exact normative field names while
preserving the existing validation guidance.
- Around line 139-153: Update the override contract documentation to define an
explicit allowlist: only the listed scheduling and placement paths may be
patched, and every other Deployment.spec path must be rejected. Resolve the
non-core replicas inconsistency by either removing its support or documenting it
as an explicit validated exception, ensuring the stated scaling contract and
validation behavior agree.
---
Outside diff comments:
In `@enhancements/external-secrets-operator/external-secrets-component-config.md`:
- Around line 13-22: Update the Jira metadata associated with ESO-550 in the
tracking-link section to set its target version to 5.0.0. If that metadata
cannot be changed, document the intentional target-version exception in this
component configuration instead.
🪄 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: 87fadc52-3755-4962-9a08-8e27f779755c
📒 Files selected for processing (1)
enhancements/external-secrets-operator/external-secrets-component-config.md
|
|
||
| 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. | ||
|
|
||
| This enhancement further adds first-class fields for **core controller concurrency** (`concurrent`) and **core controller replica count** (`replicas`) so GitOps-managed clusters can tune throughput and high availability without unsupported workarounds. It also introduces an **`experimentalOverrides`** escape hatch (`runtime.RawExtension`) on per-component configuration that applies a strategic merge patch to the component `Deployment` `spec`, covering knobs that are not yet first-class (for example pod anti-affinity or topology spread constraints) while making clear that incorrect patches degrade the operand. |
Contributor
There was a problem hiding this comment.
I think experimentalOverrides makes sound like enabling an preview feature, instead let us rename it to advancedOverrides, with a warning note like DO NOT USE UNLESS YOU KNOW EXACTLY WHAT YOU ARE DOING.
Suggested change
| This enhancement further adds first-class fields for **core controller concurrency** (`concurrent`) and **core controller replica count** (`replicas`) so GitOps-managed clusters can tune throughput and high availability without unsupported workarounds. It also introduces an **`experimentalOverrides`** escape hatch (`runtime.RawExtension`) on per-component configuration that applies a strategic merge patch to the component `Deployment` `spec`, covering knobs that are not yet first-class (for example pod anti-affinity or topology spread constraints) while making clear that incorrect patches degrade the operand. | |
| This enhancement further adds first-class fields for **core controller concurrency** (`concurrent`) and **core controller replica count** (`replicas`) so GitOps-managed clusters can tune throughput and high availability without unsupported workarounds. It also introduces an **`advancedOverrides`** escape hatch (`runtime.RawExtension`) on per-component configuration that applies a strategic merge patch to the component `Deployment` `spec`, covering knobs that are not yet first-class (for example pod anti-affinity or topology spread constraints) while making clear that incorrect patches degrade the operand. |
|
|
||
| Administrators also frequently run external secret management systems (for example IBM Secret Server, Thycotic, HashiCorp Vault) that use certificates signed by external PKI; the CA certificates must be available to the `external-secrets` controller for **TLS server certificate verification** on outbound HTTPS. On OpenShift, the Cluster Network Operator (CNO) injects the merged trusted CA bundle into `ConfigMaps` that carry the label **`config.openshift.io/inject-trusted-cabundle: "true"`**. That mechanism is wired to the cluster **`Proxy`** object: administrators distribute user-configured CA certificates cluster-wide by setting `Proxy.spec.trustedCA` (and related proxy fields when they use an HTTP/HTTPS proxy). Asking administrators to edit the `Proxy` CR solely to attach a CA bundle when they do not use an HTTP/HTTPS proxy is a poor fit for clusters that otherwise do not operate `Proxy`. Some `external-secrets` providers expose per-store CA options, but not all do, and repeating configuration across many stores increases maintenance overhead. This enhancement extends `ExternalSecretsConfig` with an operator-local `trustedCABundle` for controller-wide trust when that model is appropriate. | ||
|
|
||
| Large-scale deployments also need higher reconcile parallelism. The operand supports `--concurrent`, but the operator hardcodes `--concurrent=1` in container args; `overrideEnv` cannot change it, and any manual Deployment patch is reverted on reconcile, blocking GitOps workflows such as Argo CD. A single controller replica also means downtime during node failures or disruptions, so a supported **`replicas`** field with proper **leader election** is needed for HA. Finally, until first-class API fields cover every Deployment knob (affinity, topology spread, etc.), an **`experimentalOverrides`** escape hatch lets administrators apply a strategic merge patch to a component Deployment spec under explicit unsupported/Degraded semantics. |
Contributor
There was a problem hiding this comment.
Let us also not make --concurrent as first class field, since there is a new request to add more fields around it. And also since we can't benchmark the values for the fields, let it be allowed via advancedOverrides.
| - Applying the user configured CA bundle to webhook or unrelated sidecars unless a follow-up explicitly requires it. | ||
| - Automatic CA certificate rotation or lifecycle management. The operator mounts the ConfigMap as-is; certificate updates are the cluster administrator's responsibility. | ||
| - Supporting ConfigMaps from namespaces other than the `external-secrets` operand namespace (`external-secrets`), as Kubernetes does not allow pods to mount ConfigMaps from other namespaces. | ||
| - Guaranteeing that `experimentalOverrides` patches produce a supportable configuration. The field is unsupported; the administrator is responsible for providing correct patch data. |
Contributor
There was a problem hiding this comment.
Suggested change
| - Guaranteeing that `experimentalOverrides` patches produce a supportable configuration. The field is unsupported; the administrator is responsible for providing correct patch data. | |
| - Guaranteeing that `experimentalOverrides` patches produce a supportable configuration. The administrator is responsible for providing correct patch data. |
| - Exhaustive validation of individual configured values (e.g., validating that an environment variable value is semantically correct). Users should consult upstream documentation. Only basic structural validation (non-empty strings, list length limits) will be performed. | ||
| - Ability to set resource limits (CPU, memory requests/limits), replica counts, pod affinity/anti-affinity, tolerations, or node selectors on a per-component or individual deployment basis. These component-level overrides are out of scope for this proposal (except for revisionHistoryLimit, which is specifically introduced here) | ||
| - Exhaustive validation of individual configured values (e.g., validating that an environment variable value is semantically correct). Users should consult upstream documentation. Only basic structural validation (non-empty strings, list length limits, numeric bounds on `concurrent` / `replicas`) will be performed. | ||
| - Setting resource limits (CPU/memory requests/limits) as dedicated API fields on a per-component basis. Resource limits are out of scope for this proposal. |
Contributor
There was a problem hiding this comment.
We can update this non-goal to say, the same can be achieved using advancedOverrides, but are not available as first class fields?
| - `template.spec.tolerations` | ||
| - `template.spec.nodeSelector` | ||
| - `template.spec.topologySpreadConstraints` | ||
| - **Forbidden paths** — the operator rejects patches targeting these and sets **Degraded**: |
Contributor
There was a problem hiding this comment.
we may need to add metadata, automountServiceAccountToken, securityContext, serviceAccountName, selector as well to the list. And status will be set Degraded with UserConfigurationError.
Comment on lines
+303
to
+310
| // experimentalOverrides is an unsupported escape hatch: a strategic merge patch applied to this | ||
| // component's Deployment.spec after first-class fields are rendered. Intended for scheduling | ||
| // fields (affinity, tolerations, nodeSelector, topologySpreadConstraints). Patches that touch | ||
| // operator-owned nested lists — containers, initContainers, ephemeralContainers, volumes, or | ||
| // volumeMounts/volumeDevices — are rejected and cause Degraded. Invalid or unusable patch data | ||
| // also causes Degraded. Prefer first-class fields (concurrent, replicas, revisionHistoryLimit, | ||
| // overrideEnv, trustedCABundle) when available. | ||
| // +optional |
Contributor
There was a problem hiding this comment.
Please add an improved version of below. This is just a suggestion.
Suggested change
| // experimentalOverrides is an unsupported escape hatch: a strategic merge patch applied to this | |
| // component's Deployment.spec after first-class fields are rendered. Intended for scheduling | |
| // fields (affinity, tolerations, nodeSelector, topologySpreadConstraints). Patches that touch | |
| // operator-owned nested lists — containers, initContainers, ephemeralContainers, volumes, or | |
| // volumeMounts/volumeDevices — are rejected and cause Degraded. Invalid or unusable patch data | |
| // also causes Degraded. Prefer first-class fields (concurrent, replicas, revisionHistoryLimit, | |
| // overrideEnv, trustedCABundle) when available. | |
| // +optional | |
| // advancedOverrides applies raw patches on top of the final operator generated Deployment spec. | |
| // WARNING: DO NOT USE UNLESS YOU KNOW EXACTLY WHAT YOU ARE DOING. | |
| // This field can overwrite your own first-class CRD settings. You must NOT use this | |
| // field to add or modify containers, initContainers, or ports, as doing so breaks | |
| // the structural integrity of the operand and will fail deployment reconciliation. | |
| // +kubebuilder:validation:Optional | |
| // +kubebuilder:pruning:PreserveUnknownFields |
| // replicas sets the desired replica count for the external-secrets core controller Deployment. | ||
| // When omitted, defaults to 1. When greater than 1, leader election keeps a single active reconciler. | ||
| // +kubebuilder:validation:Minimum:=1 | ||
| // +kubebuilder:validation:Maximum:=10 |
Contributor
There was a problem hiding this comment.
10 is a reasonable limit or should we keep it at 50. They could still use advancedOverrides to set higher value. I think 10 should be fine too.
Comment on lines
+468
to
+477
| experimentalOverrides: | ||
| template: | ||
| spec: | ||
| affinity: | ||
| podAntiAffinity: | ||
| requiredDuringSchedulingIgnoredDuringExecution: | ||
| - labelSelector: | ||
| matchLabels: | ||
| app.kubernetes.io/name: external-secrets | ||
| topologyKey: kubernetes.io/hostname |
Contributor
There was a problem hiding this comment.
Does the sample config work?
Suggested change
| experimentalOverrides: | |
| template: | |
| spec: | |
| affinity: | |
| podAntiAffinity: | |
| requiredDuringSchedulingIgnoredDuringExecution: | |
| - labelSelector: | |
| matchLabels: | |
| app.kubernetes.io/name: external-secrets | |
| topologyKey: kubernetes.io/hostname | |
| experimentalOverrides: | |
| spec: | |
| template: | |
| spec: | |
| affinity: | |
| podAntiAffinity: | |
| requiredDuringSchedulingIgnoredDuringExecution: | |
| - labelSelector: | |
| matchLabels: | |
| app.kubernetes.io/name: external-secrets | |
| topologyKey: kubernetes.io/hostname |
bharath-b-rh
left a comment
bharath-b-rh
left a comment
Contributor
There was a problem hiding this comment.
Please update the following too
- Keep PR title and description aligned with the content.
- It's outside this PR scope, update the other APIs in the EP to reflect what was implemented in earlier releases.
|
|
||
| This enhancement further adds a first-class field for **core controller replica count** (`replicas`) so GitOps-managed clusters can tune high availability without unsupported workarounds. It also introduces an **`advancedOverrides`** escape hatch (`runtime.RawExtension`) on per-component configuration that applies a strategic merge patch to the component `Deployment`, covering knobs that are not yet first-class (for example pod anti-affinity, topology spread constraints, or core controller concurrency via `--concurrent`) while making clear that incorrect patches degrade the operand. | ||
|
|
||
| > **WARNING: DO NOT USE `advancedOverrides` UNLESS YOU KNOW EXACTLY WHAT YOU ARE DOING.** This field can overwrite your own first-class CRD settings. You must NOT use this field to add or modify containers, initContainers, or ports, as doing so breaks the structural integrity of the operand and will fail deployment reconciliation. |
Contributor
There was a problem hiding this comment.
I think below content is not required in a summary. If you feel required, then it should be rephrased to say, something alike will be added in the API and docs to make user aware of it.
Suggested change
| > **WARNING: DO NOT USE `advancedOverrides` UNLESS YOU KNOW EXACTLY WHAT YOU ARE DOING.** This field can overwrite your own first-class CRD settings. You must NOT use this field to add or modify containers, initContainers, or ports, as doing so breaks the structural integrity of the operand and will fail deployment reconciliation. |
| - As a platform engineer, I want to configure controller TLS trust without touching the Proxy CR when our cluster has no HTTP/HTTPS proxy, so that we do not misuse or hollow out a cluster-wide object just to ship a PEM bundle. | ||
| - As a security engineer, I want custom roots added without replacing the container system trust store, so that the controller still trusts public CAs (for example cloud secret managers) while also trusting internal enterprise CAs. | ||
| - As an OpenShift administrator, I want to set the core controller replica count via `ExternalSecretsConfig` so that the controller can run highly available with leader election and survive node or pod disruption. | ||
| - As a platform engineer, I want an explicit experimental escape hatch to strategic-merge-patch a component `Deployment` `spec` for knobs that are not yet first-class (for example pod anti-affinity), accepting that invalid patches mark the operand **Degraded**. |
Contributor
There was a problem hiding this comment.
Suggested change
| - As a platform engineer, I want an explicit experimental escape hatch to strategic-merge-patch a component `Deployment` `spec` for knobs that are not yet first-class (for example pod anti-affinity), accepting that invalid patches mark the operand **Degraded**. | |
| - As a platform engineer, I want an explicit experimental escape hatch to strategic-merge-patch a component `Deployment` for knobs that are not yet first-class (for example pod anti-affinity), accepting that invalid patches mark the operand **Degraded**. |
| - As an OpenShift Administrator, I want to reference a `ConfigMap` of custom CA bundle in `ExternalSecretsConfig`, so that the `external-secrets` controller can sync secrets from external secret management systems over **TLS** (HTTPS) using enterprise or private PKI. | ||
| - As a platform engineer, I want to configure controller TLS trust without touching the Proxy CR when our cluster has no HTTP/HTTPS proxy, so that we do not misuse or hollow out a cluster-wide object just to ship a PEM bundle. | ||
| - As a security engineer, I want custom roots added without replacing the container system trust store, so that the controller still trusts public CAs (for example cloud secret managers) while also trusting internal enterprise CAs. | ||
| - As an OpenShift administrator, I want to set the core controller replica count via `ExternalSecretsConfig` so that the controller can run highly available with leader election and survive node or pod disruption. |
Contributor
There was a problem hiding this comment.
Are we allowing replicas only for core controller? IMO, it should be allowed for all.
| - Allow optional, supported injection of a user-supplied CA bundle so the `external-secrets` **core controller** can verify **TLS** to external HTTPS backends (enterprise PKI, private CAs). | ||
| - Automatically mount the referenced ConfigMap into the ESO core controller pod at `/etc/pki/tls/user-certs`, without overriding the system trust store at `/etc/pki/tls/certs`. | ||
| - Existing proxy-based CA bundle injection behavior (CNO-managed) is preserved unchanged and can coexist with new user configured CA bundle. | ||
| - Provide a declarative API `replicas` field for the core controller `Deployment`, with leader election enabled so only one active reconciler runs when `replicas > 1`. |
Contributor
There was a problem hiding this comment.
I am unable to understand, what with leader election enabled so only one active reconciler runs when replicas > 1. Or is it really required?
| - Existing proxy-based CA bundle injection behavior (CNO-managed) is preserved unchanged and can coexist with new user configured CA bundle. | ||
| - Provide a declarative API `replicas` field for the core controller `Deployment`, with leader election enabled so only one active reconciler runs when `replicas > 1`. | ||
| - Provide `advancedOverrides` (`runtime.RawExtension`) on per-component configuration as a strategic merge patch of that component's `Deployment`, with **Degraded** (`UserConfigurationError`) status on invalid or broken patch application. | ||
| - Preserve backward-compatible defaults when `replicas` is unset (`replicas=1`). |
Contributor
There was a problem hiding this comment.
IMO, not required to explictly spell out, since deployment resource itself has the default, which will continue.
Suggested change
| - Preserve backward-compatible defaults when `replicas` is unset (`replicas=1`). |
| 10. Deploy with `trustedCABundle`; assert volume mount and `SSL_CERT_DIR` exist only on the core controller container; assert Degraded when reference invalid with `optional: false`; assert silent skip when `optional: true` and reference missing; assert Degraded for invalid PEM even when `optional: true`. | ||
| 11. With proxy configured and `trustedCABundle` referencing a ConfigMap labeled `config.openshift.io/inject-trusted-cabundle: "true"`, assert the operator does not add the trustedCABundle volume mount (since the bundle is already handled for the proxy path), does not set Degraded. | ||
| 12. Set `replicas`; assert replica count survives multiple reconcile loops (no revert to hardcoded defaults). | ||
| 13. Apply `advancedOverrides` affinity/topology patch; assert merged into live Deployment; assert **Degraded** (`UserConfigurationError`) for malformed RawExtension; assert **Degraded** and no Deployment change when the patch targets forbidden paths (`containers`, `initContainers`, `volumes`, `metadata`, `securityContext`, `serviceAccountName`, `automountServiceAccountToken`, `selector`). |
Contributor
There was a problem hiding this comment.
We should add a testcase to validate this alongside env var hatch solution.
| * **Downgrade:** If a user downgrades to a version that does not support these fields, the older operator **ignores** unknown `spec.controllerConfig` keys (or they are pruned from stored objects depending on CRD schema). Effects include: | ||
| * **`annotations` / `componentConfig`:** Deployments revert toward operator defaults; user annotations and `overrideEnv` entries from the newer schema are lost. | ||
| * **`annotations` / `componentConfigs`:** Deployments revert toward operator defaults; user annotations, `overrideEnv`, and `advancedOverrides` from the newer schema are lost. | ||
| * **`replicas`:** Core controller returns to single replica; HA tuning is lost. |
Contributor
There was a problem hiding this comment.
Shouldn't we also mention about advancedOverrides, and on downgrade, can use env var hatch solution on supported version, and overrides will be pruned.
|
|
||
| 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. | ||
|
|
||
| This enhancement further adds a first-class field for **core controller replica count** (`replicas`) so GitOps-managed clusters can tune high availability without unsupported workarounds. It also introduces an **`advancedOverrides`** escape hatch (`runtime.RawExtension`) on per-component configuration that applies a strategic merge patch to the component `Deployment`, covering knobs that are not yet first-class (for example pod anti-affinity, topology spread constraints, or core controller concurrency via `--concurrent`) while making clear that incorrect patches degrade the operand. |
Contributor
There was a problem hiding this comment.
Suggested change
| This enhancement further adds a first-class field for **core controller replica count** (`replicas`) so GitOps-managed clusters can tune high availability without unsupported workarounds. It also introduces an **`advancedOverrides`** escape hatch (`runtime.RawExtension`) on per-component configuration that applies a strategic merge patch to the component `Deployment`, covering knobs that are not yet first-class (for example pod anti-affinity, topology spread constraints, or core controller concurrency via `--concurrent`) while making clear that incorrect patches degrade the operand. | |
| This enhancement further adds a first-class field for **core controller replica count** (`replicas`) so GitOps-managed clusters can tune high availability without unsupported workarounds. It also introduces an `advancedOverrides` escape hatch on per-component configuration. This field applies a strategic merge patch to the component `Deployment`, covering scheduling knobs not yet available as first-class fields — for example pod anti-affinity, topology spread constraints, and core controller concurrency via `--concurrent`. Invalid patches cause the operator to set a `Degraded` condition.. |
|
|
||
| Administrators also frequently run external secret management systems (for example IBM Secret Server, Thycotic, HashiCorp Vault) that use certificates signed by external PKI; the CA certificates must be available to the `external-secrets` controller for **TLS server certificate verification** on outbound HTTPS. On OpenShift, the Cluster Network Operator (CNO) injects the merged trusted CA bundle into `ConfigMaps` that carry the label **`config.openshift.io/inject-trusted-cabundle: "true"`**. That mechanism is wired to the cluster **`Proxy`** object: administrators distribute user-configured CA certificates cluster-wide by setting `Proxy.spec.trustedCA` (and related proxy fields when they use an HTTP/HTTPS proxy). Asking administrators to edit the `Proxy` CR solely to attach a CA bundle when they do not use an HTTP/HTTPS proxy is a poor fit for clusters that otherwise do not operate `Proxy`. Some `external-secrets` providers expose per-store CA options, but not all do, and repeating configuration across many stores increases maintenance overhead. This enhancement extends `ExternalSecretsConfig` with an operator-local `trustedCABundle` for controller-wide trust when that model is appropriate. | ||
|
|
||
| A single controller replica also means downtime during node failures or disruptions, so a supported **`replicas`** field with proper **leader election** is needed for HA. Until first-class API fields cover every Deployment knob (affinity, topology spread, concurrency, etc.), an **`advancedOverrides`** escape hatch lets administrators apply a strategic merge patch to a component Deployment under explicit Degraded semantics. For example, large-scale deployments needing higher reconcile parallelism (`--concurrent`) can use `advancedOverrides` to tune the core controller container args. |
Contributor
There was a problem hiding this comment.
Suggested change
| A single controller replica also means downtime during node failures or disruptions, so a supported **`replicas`** field with proper **leader election** is needed for HA. Until first-class API fields cover every Deployment knob (affinity, topology spread, concurrency, etc.), an **`advancedOverrides`** escape hatch lets administrators apply a strategic merge patch to a component Deployment under explicit Degraded semantics. For example, large-scale deployments needing higher reconcile parallelism (`--concurrent`) can use `advancedOverrides` to tune the core controller container args. | |
| A single controller replica results in a reconciliation gap during node failures or pod evictions, until Kubernetes reschedules the pod. So a supported **`replicas`** field with proper **leader election** is needed for HA. Until first-class API fields cover every Deployment knob (affinity, topology spread, concurrency, etc.), an **`advancedOverrides`** escape hatch lets administrators apply a strategic merge patch to a component Deployment under explicit Degraded semantics. For example, large-scale deployments needing higher reconcile parallelism (`--concurrent`) can use `advancedOverrides` to tune the core controller container args. |
| - As a platform engineer, I want to configure controller TLS trust without touching the Proxy CR when our cluster has no HTTP/HTTPS proxy, so that we do not misuse or hollow out a cluster-wide object just to ship a PEM bundle. | ||
| - As a security engineer, I want custom roots added without replacing the container system trust store, so that the controller still trusts public CAs (for example cloud secret managers) while also trusting internal enterprise CAs. | ||
| - As an OpenShift administrator, I want to set the core controller replica count via `ExternalSecretsConfig` so that the controller can run highly available with leader election and survive node or pod disruption. | ||
| - As a platform engineer, I want an explicit experimental escape hatch to strategic-merge-patch a component `Deployment` `spec` for knobs that are not yet first-class (for example pod anti-affinity), accepting that invalid patches mark the operand **Degraded**. |
Contributor
There was a problem hiding this comment.
Suggested change
| - As a platform engineer, I want an explicit experimental escape hatch to strategic-merge-patch a component `Deployment` `spec` for knobs that are not yet first-class (for example pod anti-affinity), accepting that invalid patches mark the operand **Degraded**. | |
| - As a platform engineer, I want a supported mechanism to configure pod scheduling fields (affinity, tolerations, topology spread constraints) on individual external-secrets components, so that I can meet placement requirements without editing operator-managed Deployments directly. |
Contributor
|
@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. |
Contributor
Author
|
Hi @bharath-b-rh , I have resolved all the review comments, Can you PTAL |
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
Updates the external-secrets-component-config enhancement proposal to add three new capabilities to the
ExternalSecretsConfigAPI:concurrent(spec.controllerConfig.concurrent): First-class field that translates to the upstream--concurrent=<N>CLI argument on the core controller, allowing administrators to tune reconcile parallelism for large-scale deployments (thousands of ExternalSecrets). CRD-validated min1, max100; defaults to1when unset. Not configurable viaoverrideEnvsince it is a CLI arg, not an env var.replicas(spec.controllerConfig.replicas): First-class field for the core controller Deployment replica count, enabling high availability. CRD-validated min1, max10; defaults to1. Leader election (--enable-leader-election=true) is always enforced so only one replica actively reconciles.experimentalOverrides(spec.controllerConfig.componentConfigs[].experimentalOverrides): Per-componentruntime.RawExtensionescape hatch that applies a strategic merge patch to the componentDeployment.spec. Intended for scheduling fields (affinity, tolerations, nodeSelector, topologySpreadConstraints) until first-class fields exist. Patches targeting operator-owned nested lists (containers,initContainers,volumes, mounts) are rejected with a Degraded condition. Protected fields are re-asserted after any allowed patch.Summary by CodeRabbit