Pr 150 enhanced context#163
Draft
anandkuma77 wants to merge 3 commits into
Draft
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anandkuma77 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 |
WalkthroughThis PR adds a comprehensive set of new documentation files with no code changes: entry-point guides (AGENTS.md, ARCHITECTURE.md, CLAUDE.md, CONTRIBUTING.md), a GLOSSARY.md, five domain guideline docs (API contracts, error handling, integration, performance, security, testing), four ADRs plus an ADR index, and a docs/CONTEXT_MAP.md organizing all documentation. ChangesDocumentation additions
Estimated code review effort: 2 (Simple) | ~15 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings, 1 inconclusive)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
docs/performance-guidelines.md (1)
38-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the predicate rule to managed-resource watches.
The same document already calls out exceptions (
ExternalSecretsManager, metadata-only watches) that intentionally do not usemanagedResources, so this "always" reads too broadly.♻️ Suggested wording tweak
- When adding new watched resources, always apply at minimum the `managedResources` predicate to avoid reconciling unrelated objects. + When adding new label-filtered managed resources, apply at minimum the `managedResources` predicate to avoid reconciling unrelated objects.🤖 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 `@docs/performance-guidelines.md` around lines 38 - 39, The guidance in the performance guidelines is too broad because it says to always apply managedResources, even though some watches intentionally use different predicates. Update the wording in the relevant docs section near WatchesMetadata to scope the rule to managed-resource watches only, and keep the exceptions for ExternalSecretsManager and metadata-only watches clear and explicit.DEVELOPMENT.md (1)
255-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
go install ...@latesthere.The guide already says the repo uses vendored tooling; pulling Delve from
@latestmakes the setup non-reproducible and bypasses the repo-managed toolchain.Based on learnings, use the repository’s Makefile targets and vendored toolchain instead of installing tools globally.♻️ Suggested fix
-# Install delve -go install github.com/go-delve/delve/cmd/dlv@latest +# Use the repository-managed Delve version (or pin the exact version used by CI).🤖 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 `@DEVELOPMENT.md` around lines 255 - 265, Replace the Delve install step with the repository-managed tooling instead of using go install ...@latest, since the guide should stay reproducible and use vendored tools. Update the debugger setup in DEVELOPMENT.md to point readers to the existing Makefile targets/toolchain for launching and using dlv, and keep the rest of the debugging flow aligned with those repo-supported commands.Source: Learnings
docs/decisions/001-bindata-over-helm.md (1)
34-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the Tiller-era Helm reference.
This rationale mixes Helm 2-only behavior with current Helm trade-offs. If the intent is to document modern Helm behavior, keep the release-tracking / three-way-merge concerns and remove Tiller.
♻️ Suggested fix
-- Helm adds operational complexity (Tiller/3-way merge, release tracking) +- Helm adds operational complexity (release tracking, three-way 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 `@docs/decisions/001-bindata-over-helm.md` around lines 34 - 39, The “Why not Helm?” rationale in the decision doc still references Tiller, which is Helm 2-era behavior and no longer relevant. Update the Helm trade-offs text to focus on current concerns only by removing the Tiller mention while keeping the release-tracking and three-way-merge points in the same section.
🤖 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 `@DEVELOPMENT.md`:
- Around line 333-339: The shell example in DEVELOPMENT.md has broken line
continuations because the backslashes are followed by inline comments. Update
the example so the command uses valid continuations in the same block,
referencing the go run invocation and the --zap-log-level,
--metrics-bind-address, --health-probe-bind-address, and --leader-elect flags,
with comments moved to separate lines or removed from the continued lines.
In `@docs/decisions/004-singleton-cr-pattern.md`:
- Around line 51-57: The singleton rationale in the “Why Singleton?” section
overstates the design by mentioning leader election, which is unrelated to the
CR ownership argument. Update the wording in the decision doc so the explanation
stays focused on config/reconciliation ownership, especially the rationale
around “Simplifies reconciliation” and “Clear ownership,” and remove any
implication that singleton CRs eliminate or depend on leader election.
In `@docs/error-handling-guidelines.md`:
- Around line 50-58: The error-handling guidance currently lists Exists
alongside client operations that should be wrapped with common.FromClientError,
but that should be removed. Update the wording in the error-handling guidelines
so only Get, Create, Update, Delete, and Patch are covered, and note that
CtrlClient.Exists already normalizes NotFound to (false, nil) in
pkg/controller/client/client.go. Keep the standard reconciliation references
(such as deployments.go and services.go) aligned with this corrected contract.
In `@docs/examples/advanced-configuration.yaml`:
- Around line 114-148: The advanced configuration example enables the Bitwarden
plugin but only adds egress rules for the controller, so the bitwardenSDKServer
path is not covered. Update the networkPolicies section to include an explicit
egress rule for the bitwardenSDKServer to reach Bitwarden or your proxy, or add
clear documentation near plugins.bitwardenSecretManagerProvider explaining the
required network dependency.
- Around line 54-73: The advanced configuration example hard-codes infra-node
placement and combines both affinity and nodeSelector, which makes the base
example unsuitable for compact topologies and contradicts the “alternative”
note. Update the YAML example to remove the fixed infra nodeSelector from the
base example and keep only one scheduling mechanism in this snippet, using the
existing affinity/tolerations or moving the selector into a separate overlay
example.
In `@docs/examples/bitwarden-plugin.yaml`:
- Around line 39-95: The Bitwarden example currently includes realistic-looking
secret material in the Secret manifests and auth token, which should not ship in
docs. Update the examples in bitwarden-plugin.yaml to use obvious placeholders
or commented generation commands for the TLS data, caBundle, and token values,
and keep the ClusterSecretStore/Secret structure intact so readers can still
follow the setup using non-sensitive sample content.
In `@docs/examples/cert-manager-integration.yaml`:
- Around line 15-60: Move the cert-manager issuer example out of the same
manifest as the ExternalSecretsConfig, because the current
certProvider.certManager issuerRef points to a ClusterIssuer that is defined
later in the file and makes the example order-dependent. Update the docs/example
so the ClusterIssuer is created before the ExternalSecretsConfig, or place it in
a separate manifest, while keeping the issuerRef and certManager configuration
as-is.
In `@docs/examples/corporate-proxy.yaml`:
- Around line 90-121: This proxy example is being added as a second
ExternalSecretsConfig/cluster singleton, which would overwrite the earlier
example if applied together. Update the docs/example so it is a patch/overlay or
otherwise clearly separate from the existing cluster singleton, and keep the
configuration consistent with the first ExternalSecretsConfig block. Also revise
controllerConfig.networkPolicies in the example to allow egress to the proxy
host/endpoints instead of the AWS Secrets Manager CIDR, so the policy matches
the documented proxy-based flow.
- Around line 23-70: The proxy example is using the wrong CA bundle contract by
nesting trusted CA under controllerConfig and pointing it at an openshift-config
ConfigMap. Update the example to use the operator’s top-level trustedCABundle
field from the external secrets config contract, and show the ConfigMap in the
operand namespace as required. Keep the proxy settings under
controllerConfig.proxy, but remove the mixed trustedCA/OpenShift CNO wiring so
the example matches the trustedCABundle behavior.
In `@docs/examples/README.md`:
- Around line 12-21: The quick-start instructions reference a non-existent
standalone issuer manifest, causing the setup steps to break. Update the example
in README’s production-grade setup section to point the ClusterIssuer command at
the actual manifest used in this PR, or add a separate issuer file and reference
it consistently. Use the cert-manager setup steps in the README and the
cert-manager-integration example as the source of truth.
In `@TROUBLESHOOTING.md`:
- Around line 284-297: The troubleshooting example under the
ExternalSecretsConfig section still hard-codes the worker-only nodeSelector,
which breaks SNO and Two-Node topologies. Update that managed-resources example
to remove the node-role.kubernetes.io/worker selector from the
controllerConfig/controller snippet, or explicitly note it is only for
dedicated-worker clusters; keep the guidance aligned with the
ExternalSecretsConfig and controllerConfig examples.
---
Nitpick comments:
In `@DEVELOPMENT.md`:
- Around line 255-265: Replace the Delve install step with the
repository-managed tooling instead of using go install ...@latest, since the
guide should stay reproducible and use vendored tools. Update the debugger setup
in DEVELOPMENT.md to point readers to the existing Makefile targets/toolchain
for launching and using dlv, and keep the rest of the debugging flow aligned
with those repo-supported commands.
In `@docs/decisions/001-bindata-over-helm.md`:
- Around line 34-39: The “Why not Helm?” rationale in the decision doc still
references Tiller, which is Helm 2-era behavior and no longer relevant. Update
the Helm trade-offs text to focus on current concerns only by removing the
Tiller mention while keeping the release-tracking and three-way-merge points in
the same section.
In `@docs/performance-guidelines.md`:
- Around line 38-39: The guidance in the performance guidelines is too broad
because it says to always apply managedResources, even though some watches
intentionally use different predicates. Update the wording in the relevant docs
section near WatchesMetadata to scope the rule to managed-resource watches only,
and keep the exceptions for ExternalSecretsManager and metadata-only watches
clear and explicit.
🪄 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: f3ff14ab-15fc-4df9-8785-be03c51192d9
📒 Files selected for processing (26)
AGENTS.mdARCHITECTURE.mdCLAUDE.mdCONTRIBUTING.mdDEVELOPMENT.mdGLOSSARY.mdSECURITY.mdTROUBLESHOOTING.mddocs/CONTEXT_MAP.mddocs/api-contracts-guidelines.mddocs/decisions/001-bindata-over-helm.mddocs/decisions/002-three-controller-design.mddocs/decisions/003-cel-validation-only.mddocs/decisions/004-singleton-cr-pattern.mddocs/decisions/README.mddocs/error-handling-guidelines.mddocs/examples/README.mddocs/examples/advanced-configuration.yamldocs/examples/basic-installation.yamldocs/examples/bitwarden-plugin.yamldocs/examples/cert-manager-integration.yamldocs/examples/corporate-proxy.yamldocs/integration-guidelines.mddocs/performance-guidelines.mddocs/security-guidelines.mddocs/testing-guidelines.md
Comment on lines
+333
to
+339
| ```bash | ||
| go run ./cmd/external-secrets-operator/main.go \ | ||
| --zap-log-level=4 \ # Verbose logs | ||
| --metrics-bind-address=:8080 \ # Enable metrics/pprof | ||
| --health-probe-bind-address=:8081 \ # Health checks | ||
| --leader-elect=false # Disable leader election (safe for local dev) | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the shell example's line continuations.
The inline comments after \ break the command if someone copies it verbatim.
♻️ Suggested fix
go run ./cmd/external-secrets-operator/main.go \
- --zap-log-level=4 \ # Verbose logs
- --metrics-bind-address=:8080 \ # Enable metrics/pprof
- --health-probe-bind-address=:8081 \ # Health checks
- --leader-elect=false # Disable leader election (safe for local dev)
+ --zap-log-level=4 \
+ --metrics-bind-address=:8080 \
+ --health-probe-bind-address=:8081 \
+ --leader-elect=false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ```bash | |
| go run ./cmd/external-secrets-operator/main.go \ | |
| --zap-log-level=4 \ # Verbose logs | |
| --metrics-bind-address=:8080 \ # Enable metrics/pprof | |
| --health-probe-bind-address=:8081 \ # Health checks | |
| --leader-elect=false # Disable leader election (safe for local dev) | |
| ``` |
🤖 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 `@DEVELOPMENT.md` around lines 333 - 339, The shell example in DEVELOPMENT.md
has broken line continuations because the backslashes are followed by inline
comments. Update the example so the command uses valid continuations in the same
block, referencing the go run invocation and the --zap-log-level,
--metrics-bind-address, --health-probe-bind-address, and --leader-elect flags,
with comments moved to separate lines or removed from the continued lines.
Comment on lines
+51
to
+57
| ### Why Singleton? | ||
|
|
||
| 1. **Prevents conflicts**: Multiple configs would create ambiguity about which should apply | ||
|
|
||
| 2. **Simplifies reconciliation**: Controller always knows which CR to watch (no need for leader election or config merging) | ||
|
|
||
| 3. **Clear ownership**: One CR, one operator instance managing it |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== docs/decisions/004-singleton-cr-pattern.md (around lines 51-57) ==\n'
sed -n '40,70p' docs/decisions/004-singleton-cr-pattern.md
printf '\n== Search for leader election references ==\n'
rg -n --hidden --no-ignore-vcs -S "leader election|leader-election|leaderElection|LeaderElection" .Repository: openshift/external-secrets-operator
Length of output: 50391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Non-vendor leader election references ==\n'
rg -n -S --glob '!vendor/**' --glob '!**/vendor/**' "LeaderElection|leader election|LeaderElectionID|NeedLeaderElection" .
printf '\n== Decision doc full context ==\n'
sed -n '1,120p' docs/decisions/004-singleton-cr-pattern.mdRepository: openshift/external-secrets-operator
Length of output: 6917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '110,220p' cmd/external-secrets-operator/main.goRepository: openshift/external-secrets-operator
Length of output: 4780
Drop the leader-election claim from the singleton rationale Singleton CRs remove config-merging ambiguity; leader election is a separate manager setting, so keep this sentence scoped to reconciliation/config ownership.
🤖 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 `@docs/decisions/004-singleton-cr-pattern.md` around lines 51 - 57, The
singleton rationale in the “Why Singleton?” section overstates the design by
mentioning leader election, which is unrelated to the CR ownership argument.
Update the wording in the decision doc so the explanation stays focused on
config/reconciliation ownership, especially the rationale around “Simplifies
reconciliation” and “Clear ownership,” and remove any implication that singleton
CRs eliminate or depend on leader election.
Comment on lines
+50
to
+58
| For any Kubernetes client operation (Get, Create, Update, Delete, Patch, Exists), wrap the error with `common.FromClientError`: | ||
|
|
||
| ```go | ||
| if err := r.Create(r.ctx, obj); err != nil { | ||
| return common.FromClientError(err, "failed to create %s deployment resource", name) | ||
| } | ||
| ``` | ||
|
|
||
| This is the standard pattern across all resource reconciliation files (rbacs.go, deployments.go, services.go, secret.go, certificate.go, configmap.go, networkpolicy.go, validatingwebhook.go, serviceaccounts.go). `FromClientError` auto-classifies the API error so callers do not manually pick irrecoverable vs retryable. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude Exists from the FromClientError guidance.
CtrlClient.Exists already normalizes NotFound to (false, nil) in pkg/controller/client/client.go; wrapping it here would turn a plain presence check into a retry/error path and conflict with the helper contract.
🛠️ Suggested wording fix
- For any Kubernetes client operation (Get, Create, Update, Delete, Patch, Exists), wrap the error with `common.FromClientError`:
+ For Kubernetes client operations that can return API errors (Get, Create, Update, Delete, Patch), wrap the error with `common.FromClientError`:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| For any Kubernetes client operation (Get, Create, Update, Delete, Patch, Exists), wrap the error with `common.FromClientError`: | |
| ```go | |
| if err := r.Create(r.ctx, obj); err != nil { | |
| return common.FromClientError(err, "failed to create %s deployment resource", name) | |
| } | |
| ``` | |
| This is the standard pattern across all resource reconciliation files (rbacs.go, deployments.go, services.go, secret.go, certificate.go, configmap.go, networkpolicy.go, validatingwebhook.go, serviceaccounts.go). `FromClientError` auto-classifies the API error so callers do not manually pick irrecoverable vs retryable. | |
| For Kubernetes client operations that can return API errors (Get, Create, Update, Delete, Patch), wrap the error with `common.FromClientError`: | |
🤖 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 `@docs/error-handling-guidelines.md` around lines 50 - 58, The error-handling
guidance currently lists Exists alongside client operations that should be
wrapped with common.FromClientError, but that should be removed. Update the
wording in the error-handling guidelines so only Get, Create, Update, Delete,
and Patch are covered, and note that CtrlClient.Exists already normalizes
NotFound to (false, nil) in pkg/controller/client/client.go. Keep the standard
reconciliation references (such as deployments.go and services.go) aligned with
this corrected contract.
Comment on lines
+54
to
+73
| # Node affinity - run on infra nodes | ||
| affinity: | ||
| nodeAffinity: | ||
| preferredDuringSchedulingIgnoredDuringExecution: | ||
| - weight: 100 | ||
| preference: | ||
| matchExpressions: | ||
| - key: node-role.kubernetes.io/infra | ||
| operator: Exists | ||
|
|
||
| # Tolerations for tainted nodes | ||
| tolerations: | ||
| - key: node-role.kubernetes.io/infra | ||
| operator: Exists | ||
| effect: NoSchedule | ||
|
|
||
| # Node selector (alternative to affinity) | ||
| nodeSelector: | ||
| node-role.kubernetes.io/infra: "" | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't hard-code infra placement in the base example.
As per path instructions, YAML examples must not assume dedicated worker/infra nodes without considering SNO and Two-Node topologies. This nodeSelector makes the example unschedulable on compact clusters, and it also contradicts the "alternative" wording because the affinity and selector are both active. Keep only one mechanism here or move the selector to a separate overlay.
🤖 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 `@docs/examples/advanced-configuration.yaml` around lines 54 - 73, The advanced
configuration example hard-codes infra-node placement and combines both affinity
and nodeSelector, which makes the base example unsuitable for compact topologies
and contradicts the “alternative” note. Update the YAML example to remove the
fixed infra nodeSelector from the base example and keep only one scheduling
mechanism in this snippet, using the existing affinity/tolerations or moving the
selector into a separate overlay example.
Source: Path instructions
Comment on lines
+114
to
+148
| # Custom network policies | ||
| networkPolicies: | ||
| # Allow egress to AWS Secrets Manager (us-east-1) | ||
| - name: allow-aws-secrets-manager | ||
| componentName: controller | ||
| policyTypes: | ||
| - Egress | ||
| egress: | ||
| - to: | ||
| - ipBlock: | ||
| cidr: 52.94.0.0/16 # AWS IP range | ||
| ports: | ||
| - protocol: TCP | ||
| port: 443 | ||
|
|
||
| # Allow egress to GCP Secret Manager | ||
| - name: allow-gcp-secret-manager | ||
| componentName: controller | ||
| policyTypes: | ||
| - Egress | ||
| egress: | ||
| - to: | ||
| - ipBlock: | ||
| cidr: 35.190.247.0/24 # GCP Secret Manager | ||
| ports: | ||
| - protocol: TCP | ||
| port: 443 | ||
|
|
||
| # Bitwarden plugin enabled | ||
| plugins: | ||
| bitwardenSecretManagerProvider: | ||
| mode: Enabled | ||
| secretRef: | ||
| name: bitwarden-tls | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add Bitwarden egress coverage or document the dependency.
This "production" example enables the Bitwarden plugin but only defines controller egress policies. If network policies are managed, the bitwardenSDKServer still needs an explicit rule to reach Bitwarden (or your proxy), otherwise the plugin cannot fetch secrets.
🤖 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 `@docs/examples/advanced-configuration.yaml` around lines 114 - 148, The
advanced configuration example enables the Bitwarden plugin but only adds egress
rules for the controller, so the bitwardenSDKServer path is not covered. Update
the networkPolicies section to include an explicit egress rule for the
bitwardenSDKServer to reach Bitwarden or your proxy, or add clear documentation
near plugins.bitwardenSecretManagerProvider explaining the required network
dependency.
Comment on lines
+15
to
+60
| apiVersion: operator.openshift.io/v1alpha1 | ||
| kind: ExternalSecretsConfig | ||
| metadata: | ||
| name: cluster | ||
| spec: | ||
| managementState: Managed | ||
|
|
||
| controllerConfig: | ||
| certProvider: | ||
| certManager: | ||
| # Enable cert-manager mode (IMMUTABLE - cannot change once set) | ||
| mode: Enabled | ||
|
|
||
| # Reference to cert-manager Issuer (IMMUTABLE) | ||
| issuerRef: | ||
| # ClusterIssuer (cluster-scoped) or Issuer (namespace-scoped) | ||
| kind: ClusterIssuer | ||
| name: letsencrypt-prod # Must exist before creating this CR | ||
|
|
||
| # Inject CA bundle into operand CRDs via annotations (IMMUTABLE) | ||
| # When "true", crd-annotator adds cert-manager.io/inject-ca-from | ||
| # annotation to ExternalSecret/SecretStore/etc. CRDs | ||
| injectAnnotations: "true" | ||
|
|
||
| # Certificate validity and renewal settings | ||
| certificateCheckInterval: "5m" # How often to check cert validity | ||
| certificateDuration: "2160h" # 90 days (default: 8760h = 1 year) | ||
| certificateRenewBefore: "720h" # Renew 30 days before expiry | ||
|
|
||
| # Example ClusterIssuer (create separately): | ||
| --- | ||
| apiVersion: cert-manager.io/v1 | ||
| kind: ClusterIssuer | ||
| metadata: | ||
| name: letsencrypt-prod | ||
| spec: | ||
| acme: | ||
| server: https://acme-v02.api.letsencrypt.org/directory | ||
| email: admin@example.com | ||
| privateKeySecretRef: | ||
| name: letsencrypt-prod-account-key | ||
| solvers: | ||
| - http01: | ||
| ingress: | ||
| class: openshift-default | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Split the issuer example from the CR.
The file says issuerRef must already exist, but the ClusterIssuer is defined later in the same manifest. That makes the example order-dependent and can race the controller. Put the issuer first or move it to a separate file.
🤖 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 `@docs/examples/cert-manager-integration.yaml` around lines 15 - 60, Move the
cert-manager issuer example out of the same manifest as the
ExternalSecretsConfig, because the current certProvider.certManager issuerRef
points to a ClusterIssuer that is defined later in the file and makes the
example order-dependent. Update the docs/example so the ClusterIssuer is created
before the ExternalSecretsConfig, or place it in a separate manifest, while
keeping the issuerRef and certManager configuration as-is.
Comment on lines
+23
to
+70
| controllerConfig: | ||
| # Proxy configuration | ||
| proxy: | ||
| httpProxy: "http://proxy.example.com:8080" | ||
| httpsProxy: "http://proxy.example.com:8080" | ||
|
|
||
| # NO_PROXY: comma-separated list (no spaces) | ||
| # Include: cluster.local, .svc, pod/service CIDRs, internal APIs | ||
| noProxy: ".cluster.local,.svc,10.0.0.0/8,172.30.0.0/16,localhost,127.0.0.1" | ||
|
|
||
| # Trusted CA bundle (for proxy TLS inspection) | ||
| trustedCA: | ||
| # ConfigMap name in "openshift-config" namespace | ||
| name: corporate-ca-bundle | ||
|
|
||
| # Key within ConfigMap (default: "ca-bundle.crt") | ||
| key: "ca-bundle.crt" | ||
|
|
||
| # Network policy provisioning must be Managed for automatic proxy egress | ||
| networkPolicyProvisioning: Managed | ||
|
|
||
| # Create ConfigMap with proxy CA certificate: | ||
| --- | ||
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| name: corporate-ca-bundle | ||
| namespace: openshift-config # Standard OpenShift location | ||
| data: | ||
| ca-bundle.crt: | | ||
| -----BEGIN CERTIFICATE----- | ||
| MIIDXTCCAkWgAwIBAgIJAKZ... (Corporate Root CA) | ||
| -----END CERTIFICATE----- | ||
| -----BEGIN CERTIFICATE----- | ||
| MIIDYDCCAkigAwIBAgIJALa... (Intermediate CA, if any) | ||
| -----END CERTIFICATE----- | ||
|
|
||
| # OR inject cluster-wide CA via OpenShift CNO (auto-populates ConfigMap): | ||
| --- | ||
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| name: corporate-ca-bundle | ||
| namespace: openshift-config | ||
| labels: | ||
| # OpenShift CNO injects cluster CA bundle into ConfigMaps with this label | ||
| config.openshift.io/inject-trusted-cabundle: "true" | ||
| data: {} # CNO populates this |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the operator's trusted CABundle contract here.
Based on the upstream contract in api/v1alpha1/external_secrets_config_types.go:148-181, the trusted CA bundle is a top-level trustedCABundle and its ConfigMap must live in the operand namespace. This example instead wires controllerConfig.trustedCA to an openshift-config ConfigMap, which mixes two different CA paths and won't work as written.
🤖 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 `@docs/examples/corporate-proxy.yaml` around lines 23 - 70, The proxy example
is using the wrong CA bundle contract by nesting trusted CA under
controllerConfig and pointing it at an openshift-config ConfigMap. Update the
example to use the operator’s top-level trustedCABundle field from the external
secrets config contract, and show the ConfigMap in the operand namespace as
required. Keep the proxy settings under controllerConfig.proxy, but remove the
mixed trustedCA/OpenShift CNO wiring so the example matches the trustedCABundle
behavior.
Comment on lines
+90
to
+121
| # Additional network policy for external provider through proxy: | ||
| --- | ||
| apiVersion: operator.openshift.io/v1alpha1 | ||
| kind: ExternalSecretsConfig | ||
| metadata: | ||
| name: cluster | ||
| spec: | ||
| managementState: Managed | ||
|
|
||
| controllerConfig: | ||
| proxy: | ||
| httpProxy: "http://proxy.example.com:8080" | ||
| httpsProxy: "http://proxy.example.com:8080" | ||
| noProxy: ".cluster.local,.svc,10.0.0.0/8" | ||
|
|
||
| trustedCA: | ||
| name: corporate-ca-bundle | ||
|
|
||
| # Custom network policy for AWS Secrets Manager through proxy | ||
| networkPolicies: | ||
| - name: allow-aws-secrets-manager | ||
| componentName: controller | ||
| policyTypes: | ||
| - Egress | ||
| egress: | ||
| - to: | ||
| # AWS Secrets Manager endpoint CIDR (example: us-east-1) | ||
| - ipBlock: | ||
| cidr: 52.94.0.0/16 # AWS IP range | ||
| ports: | ||
| - protocol: TCP | ||
| port: 443 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't turn the proxy example into a second singleton CR.
This second ExternalSecretsConfig/cluster will overwrite the first example if the file is applied as-is, so it's not a standalone configuration. The custom egress rule also targets the AWS CIDR instead of the proxy endpoint, which defeats the stated proxy flow. Split this into a patch/overlay or rewrite the policy around the proxy host.
🤖 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 `@docs/examples/corporate-proxy.yaml` around lines 90 - 121, This proxy example
is being added as a second ExternalSecretsConfig/cluster singleton, which would
overwrite the earlier example if applied together. Update the docs/example so it
is a patch/overlay or otherwise clearly separate from the existing cluster
singleton, and keep the configuration consistent with the first
ExternalSecretsConfig block. Also revise controllerConfig.networkPolicies in the
example to allow egress to the proxy host/endpoints instead of the AWS Secrets
Manager CIDR, so the policy matches the documented proxy-based flow.
Comment on lines
+12
to
+21
| **Production-grade setup** (recommended): | ||
| ```bash | ||
| # 1. Install cert-manager first | ||
| kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v1.13.0/cert-manager.yaml | ||
|
|
||
| # 2. Create ClusterIssuer (see cert-manager-integration.yaml for example) | ||
| kubectl apply -f clusterissuer.yaml | ||
|
|
||
| # 3. Apply configuration | ||
| kubectl apply -f cert-manager-integration.yaml |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the quick-start target.
clusterissuer.yaml is not one of the example files in this PR; the issuer manifest only appears inside cert-manager-integration.yaml. As written, this copy-paste path will fail. Point the command at the actual file or add a standalone issuer manifest.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 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 `@docs/examples/README.md` around lines 12 - 21, The quick-start instructions
reference a non-existent standalone issuer manifest, causing the setup steps to
break. Update the example in README’s production-grade setup section to point
the ClusterIssuer command at the actual manifest used in this PR, or add a
separate issuer file and reference it consistently. Use the cert-manager setup
steps in the README and the cert-manager-integration example as the source of
truth.
Comment on lines
+284
to
+297
| **Fix**: | ||
| - **Don't manually edit managed resources** - they will be reverted | ||
| - Instead, configure via `ExternalSecretsConfig`: | ||
| ```yaml | ||
| spec: | ||
| controllerConfig: | ||
| controller: # for main controller deployment | ||
| resources: | ||
| limits: | ||
| cpu: 500m | ||
| memory: 256Mi | ||
| nodeSelector: | ||
| node-role.kubernetes.io/worker: "" | ||
| ``` |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the worker-only selector from the example.
node-role.kubernetes.io/worker makes this snippet unschedulable on SNO/Two-Node clusters. Either omit the selector or call out that it only applies to dedicated-worker topologies.
🐛 Proposed docs fix
- nodeSelector:
- node-role.kubernetes.io/worker: ""
+ # Omit this selector on SNO / Two-Node clusters.
+ # If you need pinning, document a topology-aware selector instead.As per coding guidelines, **/*.{yaml,yml}: "Flag deployment manifests or operator code that assume dedicated worker nodes exist without considering SNO and Two-Node topologies where all workloads run on control-plane nodes."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **Fix**: | |
| - **Don't manually edit managed resources** - they will be reverted | |
| - Instead, configure via `ExternalSecretsConfig`: | |
| ```yaml | |
| spec: | |
| controllerConfig: | |
| controller: # for main controller deployment | |
| resources: | |
| limits: | |
| cpu: 500m | |
| memory: 256Mi | |
| nodeSelector: | |
| node-role.kubernetes.io/worker: "" | |
| ``` | |
| **Fix**: | |
| - **Don't manually edit managed resources** - they will be reverted | |
| - Instead, configure via `ExternalSecretsConfig`: |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 287-287: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 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 `@TROUBLESHOOTING.md` around lines 284 - 297, The troubleshooting example under
the ExternalSecretsConfig section still hard-codes the worker-only nodeSelector,
which breaks SNO and Two-Node topologies. Update that managed-resources example
to remove the node-role.kubernetes.io/worker selector from the
controllerConfig/controller snippet, or explicitly note it is only for
dedicated-worker clusters; keep the guidance aligned with the
ExternalSecretsConfig and controllerConfig examples.
Source: Coding guidelines
Extends PR openshift#150 with focused Track 1 Contextualization additions: Added ADRs (Architecture Decision Records): - docs/decisions/001-bindata-over-helm.md - Why bindata pattern - docs/decisions/002-three-controller-design.md - Why three controllers - docs/decisions/003-cel-validation-only.md - Why CEL validation - docs/decisions/004-singleton-cr-pattern.md - Why singleton CRs - docs/decisions/README.md - ADR index Added Core Documentation: - GLOSSARY.md - Domain-specific terminology (bindata, ESC/ESM, etc.) - docs/CONTEXT_MAP.md - Documentation navigation map These additions provide: - Prescriptive context explaining architectural "why" (ADRs) - Essential terminology for AI agents and developers (GLOSSARY) - Clear documentation organization (CONTEXT_MAP) All documentation follows Track 1 principles: - Grounded in source code (file paths, line numbers) - Separates prescriptive from descriptive context - Designed for both human and AI consumption Ready for Track 1 Phase 2 context registration in Cyborg. Total additions: 7 files focusing on core architectural context.
anandkuma77
force-pushed
the
pr-150-enhanced-context
branch
from
254d785 to
2b034cc
Compare
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 `@docs/CONTEXT_MAP.md`:
- Around line 193-210: Add a language identifier to the dependency tree fenced
block in CONTEXT_MAP.md so it no longer uses an untyped code fence; update the
diagram block under the dependency tree section to use a suitable tag such as
text or plaintext, keeping the existing content and structure unchanged.
- Around line 237-251: The Context registry example markdown fence is missing
surrounding whitespace, so add a blank line before the YAML code block in the
Context Map section. Update the nearby markdown around the “Context registry
example” block so the fenced example is separated from the preceding text and
remains lint-clean, without changing the content of the example itself.
- Line 187: Fix the filename casing in CONTEXT_MAP.md so the referenced
ARCHITECTURE entry matches the actual lowercase filename. Update the
ARCHITECTURE.MD reference under the relevant list item to ARCHITECTURE.md,
keeping the rest of the context map text unchanged. Use the existing
ARCHITECTURE entry in CONTEXT_MAP.md to locate and correct this typo.
🪄 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: 2e0f9ae8-9656-458e-a5e9-4ca128c1890e
📒 Files selected for processing (7)
GLOSSARY.mddocs/CONTEXT_MAP.mddocs/decisions/001-bindata-over-helm.mddocs/decisions/002-three-controller-design.mddocs/decisions/003-cel-validation-only.mddocs/decisions/004-singleton-cr-pattern.mddocs/decisions/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/decisions/001-bindata-over-helm.md
| ### Most Referenced Docs | ||
|
|
||
| 1. **AGENTS.md** - Referenced by README, CONTRIBUTING, CLAUDE.md, all guidelines | ||
| 2. **ARCHITECTURE.MD** - Referenced by AGENTS, ADRs, guidelines |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the filename casing.
ARCHITECTURE.MD should be ARCHITECTURE.md to match the actual file name and avoid copy/paste failures on case-sensitive systems.
🤖 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 `@docs/CONTEXT_MAP.md` at line 187, Fix the filename casing in CONTEXT_MAP.md
so the referenced ARCHITECTURE entry matches the actual lowercase filename.
Update the ARCHITECTURE.MD reference under the relevant list item to
ARCHITECTURE.md, keeping the rest of the context map text unchanged. Use the
existing ARCHITECTURE entry in CONTEXT_MAP.md to locate and correct this typo.
Comment on lines
+193
to
+210
| ``` | ||
| README.md | ||
| ├── AGENTS.md (AI entry point) | ||
| ├── CONTRIBUTING.md (process) | ||
| └── ARCHITECTURE.md (design) | ||
|
|
||
| AGENTS.md | ||
| ├── docs/*-guidelines.md (domain docs) | ||
| ├── GLOSSARY.md (terminology) | ||
| └── ARCHITECTURE.md (system design) | ||
|
|
||
| ARCHITECTURE.md | ||
| └── docs/decisions/ (why decisions) | ||
|
|
||
| CONTRIBUTING.md | ||
| ├── AGENTS.md (architecture context) | ||
| └── docs/testing-guidelines.md (testing) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the dependency tree fence.
The untyped code block triggers markdownlint MD040 and makes the block less searchable; text or plaintext fits this diagram.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/CONTEXT_MAP.md` around lines 193 - 210, Add a language identifier to the
dependency tree fenced block in CONTEXT_MAP.md so it no longer uses an untyped
code fence; update the diagram block under the dependency tree section to use a
suitable tag such as text or plaintext, keeping the existing content and
structure unchanged.
Source: Linters/SAST tools
Comment on lines
+237
to
+251
| **Context registry example**: | ||
| ```yaml | ||
| # Future Cyborg YAML | ||
| team: external-secrets-operator | ||
| context: | ||
| entry_point: github.com/.../AGENTS.md | ||
| architecture: github.com/.../ARCHITECTURE.md | ||
| decisions: github.com/.../docs/decisions/ | ||
| domain_guidelines: | ||
| - github.com/.../docs/api-contracts-guidelines.md | ||
| - github.com/.../docs/error-handling-guidelines.md | ||
| # ... etc | ||
| terminology: github.com/.../GLOSSARY.md | ||
| process: github.com/.../CONTRIBUTING.md | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Insert a blank line before the YAML example.
markdownlint flags this fence for missing surrounding whitespace; adding the blank line improves readability and keeps the doc lint-clean.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 238-238: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 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 `@docs/CONTEXT_MAP.md` around lines 237 - 251, The Context registry example
markdown fence is missing surrounding whitespace, so add a blank line before the
YAML code block in the Context Map section. Update the nearby markdown around
the “Context registry example” block so the fenced example is separated from the
preceding text and remains lint-clean, without changing the content of the
example itself.
Source: Linters/SAST tools
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
2 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 by CodeRabbit