ESO-308: Api changes to support customizations at install time #91
Conversation
WalkthroughControllerConfig gained Annotations and ComponentConfigs. New public types KVPair, Annotation, ComponentConfig, DeploymentConfig and corev1.EnvVar usage were added. Generated deepcopy helpers were updated (metav1/v1 import adjustments). CRD, CSV, bundle manifests, API docs and tests were extended to include schema, validations, and tests for annotations and per-component env/deployment overrides. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": bin/kube-api-linter.so, plugin: not implemented Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: siddhibhor-56 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 |
siddhibhor-56
force-pushed
the
customization
branch
3 times, most recently
from
d338612 to
6fce02f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/v1alpha1/external_secrets_config_types.go (1)
149-158: Consider JSON tag consistency.The field name
ComponentConfigs(plural) differs from the JSON tagcomponentConfig(singular). While this is a stylistic choice, ensure this is intentional for API clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
api/v1alpha1/external_secrets_config_types.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
api/v1alpha1/external_secrets_config_types.go
🔇 Additional comments (3)
api/v1alpha1/external_secrets_config_types.go (3)
4-4: LGTM!The
corev1import is correctly added to support the[]corev1.EnvVartype used in the newOverrideEnvfield.
117-130: Good validation rules for reserved annotation prefixes.The validation rule correctly prevents users from overriding reserved annotation prefixes. The
listType=mapwithlistMapKey=keyensures uniqueness.
183-191: LGTM!Using a pointer for
RevisionHistoryLimitcorrectly allows distinguishing between unset and explicitly set values. The minimum value of 1 ensures rollback capability as documented.
| // KVPair represents a generic key-value pair for configuration. | ||
| type KVPair struct { | ||
| Key string `json:"key,omitempty"` | ||
| Value string `json:"value,omitempty"` | ||
| } | ||
|
|
||
| // Annotation represents a custom annotation key-value pair. | ||
| // Embeds KVPair inline for reusability. | ||
| type Annotation struct { | ||
| // Embedded KVPair provides key and value fields | ||
| KVPair `json:",inline"` | ||
| } |
There was a problem hiding this comment.
Add validation for Key field to prevent empty keys.
The KVPair.Key field lacks a MinLength validation. Since Annotations uses listMapKey=key for uniqueness, empty keys could cause undefined behavior or API validation issues.
// KVPair represents a generic key-value pair for configuration.
type KVPair struct {
+ // +kubebuilder:validation:MinLength:=1
+ // +kubebuilder:validation:Required
Key string `json:"key,omitempty"`
Value string `json:"value,omitempty"`
}📝 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.
| // KVPair represents a generic key-value pair for configuration. | |
| type KVPair struct { | |
| Key string `json:"key,omitempty"` | |
| Value string `json:"value,omitempty"` | |
| } | |
| // Annotation represents a custom annotation key-value pair. | |
| // Embeds KVPair inline for reusability. | |
| type Annotation struct { | |
| // Embedded KVPair provides key and value fields | |
| KVPair `json:",inline"` | |
| } | |
| // KVPair represents a generic key-value pair for configuration. | |
| type KVPair struct { | |
| // +kubebuilder:validation:MinLength=1 | |
| Key string `json:"key,omitempty"` | |
| Value string `json:"value,omitempty"` | |
| } | |
| // Annotation represents a custom annotation key-value pair. | |
| // Embeds KVPair inline for reusability. | |
| type Annotation struct { | |
| // Embedded KVPair provides key and value fields | |
| KVPair `json:",inline"` | |
| } |
🤖 Prompt for AI Agents
In api/v1alpha1/external_secrets_config_types.go around lines 193 to 204,
KVPair.Key lacks a minimum-length validation which allows empty keys and can
break listMapKey=key uniqueness; add a kubebuilder validation marker above the
Key field: `// +kubebuilder:validation:MinLength=1` so generated CRD enforces
non-empty keys (keep existing json tag and struct layout unchanged). Ensure you
run controller-gen to regenerate CRD manifests after adding the marker.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
api/v1alpha1/zz_generated.deepcopy.go(9 hunks)bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(1 hunks)bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml(2 hunks)config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml(2 hunks)docs/api_reference.md(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
bundle/manifests/external-secrets-operator.clusterserviceversion.yamldocs/api_reference.mdbundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamlapi/v1alpha1/zz_generated.deepcopy.go
🧬 Code graph analysis (1)
api/v1alpha1/zz_generated.deepcopy.go (1)
api/v1alpha1/external_secrets_config_types.go (4)
Annotation(201-204)KVPair(194-197)ComponentConfig(160-182)DeploymentConfig(183-191)
🔇 Additional comments (12)
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (1)
223-223: CSVcreatedAttimestamp update is fineMetadata-only change, no effect on operator behavior or APIs.
docs/api_reference.md (1)
19-35: API docs for annotations/componentConfig match the CRD schemaNew types (Annotation, KVPair, DeploymentConfig, ComponentConfig) and the
ControllerConfig.annotations/ControllerConfig.componentConfigfields line up with the CRD schema and intended validation semantics (list‑map usage, per‑component overrides, and env var override rules). Looks good.Also applies to: 135-151, 162-163, 220-222, 243-257, 433-447
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
1176-1205: CRD schema and CEL validations for annotations/componentConfig look sound
annotationsuses list‑map semantics keyed bykeyplus a CEL guard against reserved prefixes; this matches the stated constraints.componentConfigcorrectly constrainscomponentName(enum, unique, max 4), enforcesrevisionHistoryLimit >= 1, and blocks overriding env vars with reserved prefixes via CEL.- List‑map usage and validations should behave well with strategic merge and keep the API surface maintainable.
Also applies to: 1295-1466
config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml (1)
1176-1205: Base CRD changes are consistent with bundle CRD and implement the intended constraintsThe added
controllerConfig.annotationsandcontrollerConfig.componentConfigschemas (including list‑map configuration and CEL validations) mirror the bundle manifest and correctly enforce reserved annotation/env prefixes and per‑component override constraints.Also applies to: 1295-1466
api/v1alpha1/zz_generated.deepcopy.go (8)
23-28: LGTM!Import changes correctly support the new field types (v1.EnvVar, metav1.Duration, metav1.Condition) used in the deepcopy methods.
30-44: LGTM!Deepcopy implementation for Annotation is correct. Since KVPair contains only string fields (immutable), shallow copy semantics are appropriate.
181-202: LGTM!ComponentConfig deepcopy correctly handles nested DeploymentConfigs and the OverrideEnv slice. Element-wise DeepCopyInto calls are necessary since EnvVar can contain nested pointer fields.
307-325: LGTM!DeploymentConfig deepcopy correctly handles the optional RevisionHistoryLimit pointer field with proper nil checking and allocation.
545-558: LGTM!KVPair deepcopy is correct. Struct assignment is appropriate since both Key and Value are immutable strings.
256-260: LGTM!Annotations slice handling is correct. Using copy() is appropriate since Annotation contains only immutable string fields (via embedded KVPair).
268-274: LGTM!ComponentConfigs slice handling is correct. Element-wise DeepCopyInto calls are necessary since ComponentConfig contains nested structures (DeploymentConfigs) and slices with pointer fields (OverrideEnv).
97-102: LGTM!Type changes to metav1.Duration, v1.ResourceRequirements, v1.Affinity, v1.Toleration, and []metav1.Condition align with standard Kubernetes API types and are consistent with the updated imports.
Also applies to: 142-152, 224-224, 652-652
docs/api_reference.md
Outdated
| | `annotations` _[Annotation](#annotation) array_ | annotations allows adding custom annotations to all external-secrets component<br />Deployments and Pod templates. These annotations are applied globally to all<br />operand components (Controller, Webhook, CertController, BitwardenSDKServer).<br />These annotations are merged with any default annotations set by the operator.<br />User-specified annotations take precedence over defaults in case of conflicts.<br />Annotations with keys starting with kubernetes.io, app.kubernetes, openshift.io, or k8s.io<br />are reserved and cannot be overridden. | | Optional: \{\} <br /> | | ||
| | `networkPolicies` _[NetworkPolicy](#networkpolicy) array_ | networkPolicies specifies the list of network policy configurations<br />to be applied to external-secrets pods.<br />Each entry allows specifying a name for the generated NetworkPolicy object,<br />along with its full Kubernetes NetworkPolicy definition.<br />If this field is not provided, external-secrets components will be isolated<br />with deny-all network policies, which will prevent proper operation. | | MaxItems: 50 <br />MinItems: 0 <br />Optional: \{\} <br /> | |
There was a problem hiding this comment.
Clarify reserved annotation key prefixes in docs
The text says keys “starting with kubernetes.io, app.kubernetes, openshift.io, or k8s.io”, but the CRD validation checks for kubernetes.io/, app.kubernetes.io/, openshift.io/, and k8s.io/. Recommend updating the wording here to include the full prefixes with / and .io to match actual enforcement.
🤖 Prompt for AI Agents
In docs/api_reference.md around lines 220-221, the reserved annotation key
prefixes are described incompletely; change the wording to match CRD validation
by listing the exact prefixes with slashes: kubernetes.io/, app.kubernetes.io/,
openshift.io/, and k8s.io/ so the docs reflect the actual enforcement; update
the sentence and any examples to use these full prefixes.
openshift-ci-robot
added
the
jira/valid-reference
|
@siddhibhor-56: This pull request references ESO-308 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
siddhibhor-56
force-pushed
the
customization
branch
from
207bbe2 to
617e0b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
docs/api_reference.md (1)
148-150: Update docs to match enum values and reserved annotation prefixesTwo doc discrepancies with the actual schema:
- In
ComponentConfig, thecomponentNamedescription lists “Allowed values: Controller, Webhook, CertController, Bitwarden” while the CRD enum is[ExternalSecretsCoreController Webhook CertController BitwardenSDKServer]. Please update the text to match the real enum values.- In
ControllerConfig.annotations, the text says keys starting withkubernetes.io,app.kubernetes,openshift.io, ork8s.ioare reserved, but validation enforceskubernetes.io/,app.kubernetes.io/,openshift.io/, andk8s.io/. The sentence should list these exact prefixes (with/and.io) so it reflects what is actually rejected.Also applies to: 220-222
api/v1alpha1/external_secrets_config_types.go (1)
160-199: Fix ComponentName “Allowed values” comment and tighten KVPair key validationThe implementation/validation for
ComponentConfigandOverrideEnvlooks good. Two small API‑surface issues remain:
ComponentConfig.ComponentName’s comment says “Allowed values: Controller, Webhook, CertController, Bitwarden” but the enum isExternalSecretsCoreController,Webhook,CertController,BitwardenSDKServer. The doc should match the real enum identifiers.KVPair.Keyis required but can still be empty; since it’s used as alistMapKey=key(viaAnnotation+ listType=map/listMapKey=key), allowing""is awkward and can lead to surprising behavior. Adding// +kubebuilder:validation:MinLength=1(and re‑running controller‑gen) would enforce non‑empty keys and make the schema clearer.type ComponentConfig struct { // componentName specifies which deployment component this configuration applies to. - // Allowed values: Controller, Webhook, CertController, Bitwarden + // Allowed values: ExternalSecretsCoreController, Webhook, CertController, BitwardenSDKServer @@ // KVPair represents a generic key-value pair for configuration. type KVPair struct { - // +kubebuilder:validation:Required - Key string `json:"key"` + // +kubebuilder:validation:Required + // +kubebuilder:validation:MinLength=1 + Key string `json:"key"` Value string `json:"value,omitempty"` }
🧹 Nitpick comments (1)
api/v1alpha1/external_secrets_config_types.go (1)
117-130: Align annotations comment with actual reserved prefixesThe validation/message correctly forbids keys starting with
kubernetes.io/,app.kubernetes.io/,openshift.io/, ork8s.io/, but the preceding comment omits the/and the.ioonapp.kubernetes.io. Suggest updating the comment to list the exact prefixes enforced by the CEL rule so consumers aren’t misled about what is actually rejected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
api/v1alpha1/external_secrets_config_types.go(3 hunks)bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(1 hunks)bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml(2 hunks)config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml(2 hunks)docs/api_reference.md(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bundle/manifests/external-secrets-operator.clusterserviceversion.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
api/v1alpha1/external_secrets_config_types.goconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.mdbundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml
🔇 Additional comments (2)
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
1176-1206: CRD schema for annotations and componentConfig looks consistentThe new
controllerConfig.annotationsandcontrollerConfig.componentConfigschema (including listMap semantics, enums forcomponentName,revisionHistoryLimitminimum, and CEL validations for annotation keys and env var names) is consistent with the Go types and intended behavior. No structural or validation issues from this diff.Also applies to: 1298-1470
config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml (1)
1176-1206: Base CRD updates mirror bundle schema correctlyThe additions for
controllerConfig.annotationsandcontrollerConfig.componentConfigin the base CRD match the bundle manifest (listMap behavior, enums, min values, and CEL validations). This keeps generated and bundled CRDs aligned; no further changes needed from this diff.Also applies to: 1298-1470
|
Please add test cases for the new changes in test file |
Done |
openshift-merge-robot
added
the
needs-rebase
| // are reserved and cannot be overridden. | ||
| // | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:validation:XValidation:rule="self.all(a, !a.key.startsWith('kubernetes.io/') && !a.key.startsWith('app.kubernetes.io/') && !a.key.startsWith('openshift.io/') && !a.key.startsWith('k8s.io/'))",message="Annotation keys cannot start with 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', or 'k8s.io/' as these are reserved" |
There was a problem hiding this comment.
nit: can we simply the rule like below?
| // +kubebuilder:validation:XValidation:rule="self.all(a, !a.key.startsWith('kubernetes.io/') && !a.key.startsWith('app.kubernetes.io/') && !a.key.startsWith('openshift.io/') && !a.key.startsWith('k8s.io/'))",message="Annotation keys cannot start with 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', or 'k8s.io/' as these are reserved" | |
| // +kubebuilder:validation:XValidation:rule="self.all(key, !['kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', 'k8s.io/'].exists(p, key.startsWith(p)))",message="annotations with reserved prefixes 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', 'k8s.io/' are not allowed" |
There was a problem hiding this comment.
yes true , updated the code
| // annotations allows adding custom annotations to all external-secrets component | ||
| // Deployments and Pod templates. These annotations are applied globally to all | ||
| // operand components (Controller, Webhook, CertController, BitwardenSDKServer). | ||
| // These annotations are merged with any default annotations set by the operator. | ||
| // User-specified annotations take precedence over defaults in case of conflicts. | ||
| // Annotations with keys starting with kubernetes.io, app.kubernetes, openshift.io, or k8s.io | ||
| // are reserved and cannot be overridden. |
There was a problem hiding this comment.
| // annotations allows adding custom annotations to all external-secrets component | |
| // Deployments and Pod templates. These annotations are applied globally to all | |
| // operand components (Controller, Webhook, CertController, BitwardenSDKServer). | |
| // These annotations are merged with any default annotations set by the operator. | |
| // User-specified annotations take precedence over defaults in case of conflicts. | |
| // Annotations with keys starting with kubernetes.io, app.kubernetes, openshift.io, or k8s.io | |
| // are reserved and cannot be overridden. | |
| // annotations is for adding custom annotations to all the resources created for external-secrets deployment. The annotations are merged with any default annotations set by the operator. User-specified annotations takes precedence over defaults in case of conflicts. Annotation keys with prefixes `kubernetes.io`, `app.kubernetes`, `openshift.io`, or `k8s.io` are not allowed. |
| NetworkPolicies []NetworkPolicy `json:"networkPolicies,omitempty"` | ||
|
|
||
| // componentConfigs allows specifying component-specific (Controller, Webhook, CertController, Bitwarden) configuration overrides. | ||
| // +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x.componentName == y.componentName))",message="componentName must be unique across all componentConfig entries" |
There was a problem hiding this comment.
I don't this is required, listMapKey should take care of uniqueness.
|
|
||
| type ComponentConfig struct { | ||
| // componentName specifies which deployment component this configuration applies to. | ||
| // Allowed values: Controller, Webhook, CertController, Bitwarden |
There was a problem hiding this comment.
| // Allowed values: Controller, Webhook, CertController, Bitwarden | |
| // Allowed values: ExternalSecretsCoreController, Webhook, CertController, Bitwarden |
| // | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:validation:MaxItems:=50 | ||
| // +kubebuilder:validation:XValidation:rule="self.all(e, !e.name.startsWith('HOSTNAME') && !e.name.startsWith('KUBERNETES_') && !e.name.startsWith('EXTERNAL_SECRETS_'))",message="Environment variable names cannot start with 'HOSTNAME', 'KUBERNETES_', or 'EXTERNAL_SECRETS_' as these are reserved" |
There was a problem hiding this comment.
nit: we can simplify the rule here as well.
There was a problem hiding this comment.
true, updated
# Conflicts: # bundle/manifests/external-secrets-operator.clusterserviceversion.yaml
- Make Annotation key field required (used as list map key) - Add maxItems=50 to OverrideEnv to reduce CEL validation cost - Regenerated CRDs and bundle
siddhibhor-56
force-pushed
the
customization
branch
from
a1daf9c to
5f98df7
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
1172-1203: CRD schema matches new Go types; regenerate after fixing annotations XValidationThe added
controllerConfig.annotationsandcontrollerConfig.componentConfigschema look consistent with the Go structs (Annotation/KVPair, ComponentConfig, DeploymentConfig, OverrideEnv) and their constraints (map-list semantics, enum, min/max items, env name reserved prefixes).Given the annotations XValidation issue noted in
external_secrets_config_types.go(using the element object instead of itskeyfield), once you update the kubebuilder tag there, please regenerate this bundle so the CEL rule and any comment text are brought in line automatically.Also applies to: 1295-1450
config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml (1)
1172-1203: Base CRD updates align with API types; rely on regenerated output after XValidation fixThe new
controllerConfig.annotationsandcontrollerConfig.componentConfigsections in the base CRD correctly mirror the Go API (list-map semantics, enum values, deploymentConfigs and overrideEnv structure, reserved env var prefixes).As with the bundle manifest, once you correct the annotations XValidation in
external_secrets_config_types.goto reference the element’skeyfield, please regenerate this base CRD so the CEL rule here is updated automatically.Also applies to: 1295-1450
api/v1alpha1/external_secrets_config_types.go (1)
151-156: AlignComponentConfig.ComponentName“Allowed values” comment with the enumThe comment lists
Bitwardenas an allowed value, but the enum and schema useBitwardenSDKServer. This will surface in generated docs/CRDs and can mislead users.Proposed diff
type ComponentConfig struct { // componentName specifies which deployment component this configuration applies to. - // Allowed values: ExternalSecretsCoreController, Webhook, CertController, Bitwarden + // Allowed values: ExternalSecretsCoreController, Webhook, CertController, BitwardenSDKServer // +kubebuilder:validation:Enum:=ExternalSecretsCoreController;Webhook;CertController;BitwardenSDKServer // +kubebuilder:validation:Required ComponentName ComponentName `json:"componentName"`Once updated, please regenerate CRDs/docs so the user-facing API reference matches.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (7)
api/v1alpha1/external_secrets_config_types.goapi/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yamlapi/v1alpha1/zz_generated.deepcopy.gobundle/manifests/external-secrets-operator.clusterserviceversion.yamlbundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.md
🚧 Files skipped from review as they are similar to previous changes (2)
- bundle/manifests/external-secrets-operator.clusterserviceversion.yaml
- api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
docs/api_reference.mdapi/v1alpha1/external_secrets_config_types.gobundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamlapi/v1alpha1/zz_generated.deepcopy.go
🧬 Code graph analysis (1)
api/v1alpha1/zz_generated.deepcopy.go (2)
api/v1alpha1/external_secrets_config_types.go (5)
Annotation(194-197)KVPair(186-190)ComponentConfig(151-174)NetworkPolicy(292-315)DeploymentConfig(175-183)api/v1alpha1/external_secrets_manager_types.go (1)
Condition(108-118)
🔇 Additional comments (7)
api/v1alpha1/zz_generated.deepcopy.go (7)
24-26: LGTM! Import changes align with type usage.The import paths correctly reference k8s.io/api/core/v1 and k8s.io/apimachinery/pkg/apis/meta/v1 (as metav1). These imports align with the updated type references throughout the deepcopy methods.
30-44: LGTM! Annotation deepcopy correctly handles embedded KVPair.The generated deepcopy methods for Annotation are correct. Since KVPair contains only string fields (Key and Value), shallow copying is appropriate.
181-202: LGTM! ComponentConfig deepcopy correctly handles nested structures.The generated deepcopy methods properly handle:
- DeploymentConfigs by calling DeepCopyInto (required for pointer field RevisionHistoryLimit)
- OverrideEnv slice by allocating a new slice and deep-copying each EnvVar element (required for nested pointer fields)
307-325: LGTM! DeploymentConfig deepcopy correctly handles pointer field.The generated deepcopy methods properly handle the RevisionHistoryLimit pointer field by allocating a new int32 and copying the value.
545-558: LGTM! KVPair deepcopy is correctly implemented.The generated deepcopy methods for KVPair are correct. Since it contains only string fields, shallow copying is appropriate.
256-274: LGTM! ControllerConfig deepcopy correctly handles new slice fields.The generated deepcopy methods properly handle:
- Annotations slice using copy() (appropriate since Annotation contains only strings)
- ComponentConfigs slice with element-wise DeepCopyInto (required for nested pointer and slice fields in ComponentConfig)
97-97: LGTM! Duration type references correctly updated to metav1.Duration.The generated deepcopy methods correctly use metav1.Duration for CertificateDuration, CertificateRenewBefore, and CertificateCheckInterval, aligning with the updated import path.
Also applies to: 102-102, 652-652
siddhibhor-56
force-pushed
the
customization
branch
2 times, most recently
from
74bdbad to
a450c70
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
api/v1alpha1/external_secrets_config_types.go (1)
186-190: AddMinLengthvalidation to prevent empty keys.The
KVPair.Keyfield hasRequiredbut noMinLengthvalidation. SinceAnnotationsuseslistMapKey=key, empty keys could cause issues with map-type list semantics.docs/api_reference.md (1)
220-222: Annotation prefix documentation is inconsistent with validation rules.Same as the Go source issue - prefixes shown without trailing slashes don't match the actual validation that checks for
kubernetes.io/,app.kubernetes.io/, etc.
🧹 Nitpick comments (2)
api/v1alpha1/external_secrets_config_types.go (1)
117-123: Documentation inconsistency: annotation prefixes missing trailing slashes.The comment on line 117 mentions prefixes
kubernetes.io,app.kubernetes,openshift.io,k8s.iobut the XValidation rule on line 119 correctly checks forkubernetes.io/,app.kubernetes.io/,openshift.io/,k8s.io/(with trailing slashes). Update the comment to match the actual validation for clarity.Also note:
app.kubernetesshould beapp.kubernetes.ioin the comment.🔎 Suggested fix
- // annotations are for adding custom annotations to all the resources created for external-secrets deployment. The annotations are merged with any default annotations set by the operator. User-specified annotations takes precedence over defaults in case of conflicts. Annotation keys with prefixes `kubernetes.io`, `app.kubernetes`, `openshift.io`, or `k8s.io` are not allowed + // annotations are for adding custom annotations to all the resources created for external-secrets deployment. The annotations are merged with any default annotations set by the operator. User-specified annotations takes precedence over defaults in case of conflicts. Annotation keys with prefixes `kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not alloweddocs/api_reference.md (1)
222-222: Missing description forcomponentConfigfield.The
componentConfigfield lacks a description in the documentation table. This should describe its purpose for per-component configuration overrides.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
api/v1alpha1/external_secrets_config_types.goapi/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yamlbundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlapi/v1alpha1/external_secrets_config_types.godocs/api_reference.mdconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamlapi/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml
🔇 Additional comments (7)
api/v1alpha1/external_secrets_config_types.go (2)
151-174: ComponentConfig structure looks good.The new
ComponentConfigtype is well-designed with proper enum validation forcomponentName, optionaldeploymentConfigs, andoverrideEnvwith reserved prefix validation. The max items limits (4 for componentConfig, 50 for overrideEnv) are reasonable.
175-183: DeploymentConfig with minimum rollback capability is appropriate.Enforcing
Minimum=1forrevisionHistoryLimitensures rollback capability is always available.docs/api_reference.md (1)
19-35: New API type documentation looks comprehensive.The documentation for
Annotation,ComponentConfig,DeploymentConfig, andKVPairtypes is complete with proper field descriptions, validation constraints, and cross-references.Also applies to: 135-151, 243-257
config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml (2)
1176-1203: Annotations schema and validation look correct.The annotations field is properly defined as a map-type list with
keyas the list map key. The XValidation rule correctly checks for reserved prefixes using CEL.
1294-1459: ComponentConfig schema is comprehensive.The componentConfig schema correctly includes:
- Enum validation for componentName with all 4 values
- Nested deploymentConfigs with revisionHistoryLimit (minimum: 1)
- overrideEnv using standard Kubernetes EnvVar schema with valueFrom support
- XValidation for reserved environment variable prefixes
- Map-type list semantics with componentName as the key
api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml (1)
496-517: New test coverage for annotations and componentConfig is good.The tests comprehensively cover:
- Valid annotation configurations
- Valid componentConfig with overrideEnv and deploymentConfigs
- Multiple componentConfigs for different components
- revisionHistoryLimit validation (minimum 1)
The test structure is well-organized with clear positive and negative test cases.
Also applies to: 562-587, 627-694
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
1294-1459: Well-structured component configuration schema.The componentConfig field implementation is solid:
- Enforces uniqueness per component via map-type list semantics
- Appropriate constraints (maxItems: 4 matches the 4 component types)
- Follows standard Kubernetes EnvVar schema for overrideEnv
- Reserved prefix validations prevent conflicts with system-managed environment variables
- revisionHistoryLimit minimum of 1 ensures rollback capability as documented
...lpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml
Outdated
Show resolved
Hide resolved
...lpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml
Outdated
Show resolved
Hide resolved
| annotations: | ||
| description: annotations are for adding custom annotations to | ||
| all the resources created for external-secrets deployment. The | ||
| annotations are merged with any default annotations set by the | ||
| operator. User-specified annotations takes precedence over defaults | ||
| in case of conflicts. Annotation keys with prefixes `kubernetes.io`, | ||
| `app.kubernetes`, `openshift.io`, or `k8s.io` are not allowed | ||
| items: | ||
| description: |- | ||
| Annotation represents a custom annotation key-value pair. | ||
| Embeds KVPair inline for reusability. | ||
| properties: | ||
| key: | ||
| type: string | ||
| value: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| type: array | ||
| x-kubernetes-list-map-keys: | ||
| - key | ||
| x-kubernetes-list-type: map | ||
| x-kubernetes-validations: | ||
| - message: annotations with reserved prefixes 'kubernetes.io/', | ||
| 'app.kubernetes.io/', 'openshift.io/', 'k8s.io/' are not allowed | ||
| rule: self.all(a, !['kubernetes.io/', 'app.kubernetes.io/', | ||
| 'openshift.io/', 'k8s.io/'].exists(p, a.key.startsWith(p))) |
There was a problem hiding this comment.
Fix documentation inconsistency in annotation prefix restrictions.
The description at line 1182 lists restricted prefixes as `kubernetes.io`, `app.kubernetes`, `openshift.io`, or `k8s.io` but the validation rule checks for 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', 'k8s.io/' (with trailing slashes and complete domain names).
Issues:
app.kubernetesis missing the.iosuffix- All prefixes are missing the trailing
/that the validation enforces
The validation is correct, but the description should match it for clarity.
🔎 Suggested fix
annotations:
description: annotations are for adding custom annotations to
all the resources created for external-secrets deployment. The
annotations are merged with any default annotations set by the
operator. User-specified annotations takes precedence over defaults
in case of conflicts. Annotation keys with prefixes `kubernetes.io`,
- `app.kubernetes`, `openshift.io`, or `k8s.io` are not allowed
+ `kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowedCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml around
lines 1176 to 1203, the human-readable description of restricted annotation
prefixes is inconsistent with the validation rule; update the description to
exactly match the validation (include the full domains and trailing slashes) so
it lists "kubernetes.io/", "app.kubernetes.io/", "openshift.io/", and "k8s.io/"
(with the .io where missing and trailing slashes present) to reflect the
enforced rule.
a450c70 to
2e0ac85
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml (2)
518-561: Error messages in annotation tests don't match CRD validation.The expected error messages in these four test cases don't match the actual XValidation message in the CRD (line 1200-1202 of the CRD manifest):
Tests expect:
Annotation keys cannot start with 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', or 'k8s.io/' as these are reservedCRD actually produces:
annotations with reserved prefixes 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', 'k8s.io/' are not allowedUpdate all four test cases (lines 528, 539, 550, 561) to use the actual CRD validation message.
588-626: Error messages in overrideEnv tests don't match CRD validation.The expected error messages in these three test cases don't match the actual XValidation message in the CRD (line 1446-1448 of the CRD manifest):
Tests expect:
Environment variable names cannot start with 'HOSTNAME', 'KUBERNETES_', or 'EXTERNAL_SECRETS_' as these are reservedCRD actually produces:
Environment variable names with reserved prefixes 'HOSTNAME', 'KUBERNETES_', 'EXTERNAL_SECRETS_' are not allowedUpdate all three test cases (lines 600, 613, 626) to use the actual CRD validation message.
docs/api_reference.md (2)
146-150: Incorrect enum value in documentation.Line 148 lists
Bitwardenin the allowed values, but the actual enum value (and CRD schema) usesBitwardenSDKServer.Update the documentation to reflect the correct enum value.
220-220: Incomplete annotation prefix documentation.The description lists restricted prefixes as
`kubernetes.io`, `app.kubernetes`, `openshift.io`, or `k8s.io`but the actual validation (in the CRD and Go code) enforces:
kubernetes.io/app.kubernetes.io/openshift.io/k8s.io/Update the documentation to include the complete domain names with trailing slashes to match the actual validation.
api/v1alpha1/external_secrets_config_types.go (2)
151-156: Comment doesn't match enum values.Line 153 lists
Bitwardenin the allowed values comment, but the actual kubebuilder enum (line 154) usesBitwardenSDKServer.Update the comment to match the enum:
- // Allowed values: ExternalSecretsCoreController, Webhook, CertController, Bitwarden + // Allowed values: ExternalSecretsCoreController, Webhook, CertController, BitwardenSDKServer
185-190: Missing minimum length validation on Key field.The
KVPair.Keyfield lacks aMinLengthvalidation. SinceAnnotationsuseslistMapKey=keyfor uniqueness (line 121), empty keys could cause validation issues or undefined behavior.Add a minimum length constraint:
// KVPair represents a generic key-value pair for configuration. type KVPair struct { + // +kubebuilder:validation:MinLength:=1 // +kubebuilder:validation:Required Key string `json:"key"` Value string `json:"value,omitempty"` }After adding the marker, regenerate the CRDs using controller-gen.
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
1176-1203: Documentation inconsistency in annotation prefix restrictions.Line 1182 describes restricted prefixes as
`kubernetes.io`, `app.kubernetes`, `openshift.io`, or `k8s.io`but the validation rule (lines 1200-1203) enforces:
kubernetes.io/app.kubernetes.io/openshift.io/k8s.io/The description is missing:
- The
.iosuffix onapp.kubernetes- The trailing
/on all prefixesUpdate the description to match the validation exactly:
- `app.kubernetes`, `openshift.io`, or `k8s.io` are not allowed + `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowedAnd ensure
kubernetes.ioalso includes the trailing slash throughout the text.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
api/v1alpha1/external_secrets_config_types.goapi/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yamlbundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yamlapi/v1alpha1/external_secrets_config_types.goconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamlbundle/manifests/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.md
🔇 Additional comments (5)
api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml (1)
496-517: LGTM! Good test coverage for new features.The new test cases properly cover:
- Custom annotations with valid (non-reserved) keys
- Valid environment variable overrides
- Component-level deployment configuration (revisionHistoryLimit)
- Validation boundaries (minimum value enforcement)
- Multiple componentConfigs for different components
The test structure and expected outcomes are appropriate.
Also applies to: 562-587, 627-694
docs/api_reference.md (1)
19-34: LGTM! Clear documentation for new types.The documentation for Annotation, DeploymentConfig, and KVPair types is well-structured with:
- Clear type descriptions
- Complete field listings with types
- Proper cross-references ("Appears in")
- Accurate validation constraints
Also applies to: 243-257, 433-448
api/v1alpha1/external_secrets_config_types.go (1)
4-4: LGTM! Well-structured API types with proper validations.The implementation includes:
- Appropriate import of
corev1for using Kubernetes EnvVar type- Correct CEL validation for annotations that properly accesses
a.key.startsWith(p)- Proper list type configuration for ComponentConfigs with
listMapKey=componentName- Correct CEL validation for overrideEnv that accesses
e.name.startsWith(p)- Appropriate minimum value constraint (1) for RevisionHistoryLimit
- Clean type embedding with Annotation using KVPair inline
The validation rules correctly prevent reserved prefixes and enforce sensible constraints.
Also applies to: 117-123, 143-149, 163-174, 175-183, 192-197
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
1294-1459: LGTM! Comprehensive CRD schema for component configuration.The componentConfig schema is well-structured:
- Correct enum values for componentName including
BitwardenSDKServer- Proper validation for deploymentConfigs.revisionHistoryLimit (minimum: 1)
- Complete environment variable schema with all nested reference types (ConfigMapKeyRef, FieldRef, ResourceFieldRef, SecretKeyRef)
- Appropriate XValidation rule enforcing reserved prefix restrictions for environment variables
- Correct list type configuration with componentName as the map key
- Reasonable item limits (4 components max, 50 env vars max)
The schema accurately reflects the Go type definitions.
config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml (1)
1294-1459: LGTM!The componentConfig schema is well-structured:
- The maxItems of 4 correctly matches the number of componentName enum values, ensuring each component can have at most one configuration entry.
- The revisionHistoryLimit minimum of 1 ensures rollback capability is preserved.
- The overrideEnv uses the standard Kubernetes EnvVar schema with appropriate validations to prevent overriding reserved environment variables.
- CEL validations correctly enforce reserved prefix restrictions for environment variable names.
| annotations: | ||
| description: annotations are for adding custom annotations to | ||
| all the resources created for external-secrets deployment. The | ||
| annotations are merged with any default annotations set by the | ||
| operator. User-specified annotations takes precedence over defaults | ||
| in case of conflicts. Annotation keys with prefixes `kubernetes.io`, | ||
| `app.kubernetes`, `openshift.io`, or `k8s.io` are not allowed | ||
| items: | ||
| description: |- | ||
| Annotation represents a custom annotation key-value pair. | ||
| Embeds KVPair inline for reusability. | ||
| properties: | ||
| key: | ||
| type: string | ||
| value: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| type: array | ||
| x-kubernetes-list-map-keys: | ||
| - key | ||
| x-kubernetes-list-type: map | ||
| x-kubernetes-validations: | ||
| - message: annotations with reserved prefixes 'kubernetes.io/', | ||
| 'app.kubernetes.io/', 'openshift.io/', 'k8s.io/' are not allowed | ||
| rule: self.all(a, !['kubernetes.io/', 'app.kubernetes.io/', | ||
| 'openshift.io/', 'k8s.io/'].exists(p, a.key.startsWith(p))) |
There was a problem hiding this comment.
Fix description to match validation rule.
Line 1182 mentions app.kubernetes in the description, but the validation rule at line 1202 uses app.kubernetes.io/. The description should include the full prefix to match the validation rule.
🔎 Proposed fix
- in case of conflicts. Annotation keys with prefixes `kubernetes.io`,
- `app.kubernetes`, `openshift.io`, or `k8s.io` are not allowed
+ in case of conflicts. Annotation keys with prefixes `kubernetes.io/`,
+ `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowed🤖 Prompt for AI Agents
In config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml around
lines 1176 to 1203, the human-readable description lists the reserved prefix
"app.kubernetes" but the validation rule uses "app.kubernetes.io/"; update the
description to exactly match the validation prefixes (use "kubernetes.io/",
"app.kubernetes.io/", "openshift.io/", "k8s.io/") including the trailing slashes
so the text and the x-kubernetes-validations rule are consistent.
2e0ac85 to
a37361d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml (1)
1176-1204: Align annotation description with actual reserved prefixes (include.io/).The description still says prefixes
kubernetes.io,app.kubernetes,openshift.io,k8s.iowhile the validation and message correctly enforce'kubernetes.io/','app.kubernetes.io/','openshift.io/','k8s.io/'. Update the description to match the enforced prefixes (including.ioand trailing/) to avoid confusion.Proposed diff
- description: annotations are for adding custom annotations to - all the resources created for external-secrets deployment. The - annotations are merged with any default annotations set by the - operator. User-specified annotations takes precedence over defaults - in case of conflicts. Annotation keys with prefixes `kubernetes.io`, - `app.kubernetes`, `openshift.io`, or `k8s.io` are not allowed + description: annotations are for adding custom annotations to + all the resources created for external-secrets deployment. The + annotations are merged with any default annotations set by the + operator. User-specified annotations takes precedence over defaults + in case of conflicts. Annotation keys with prefixes `kubernetes.io/`, + `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowedapi/v1alpha1/external_secrets_config_types.go (1)
152-175: KeepComponentConfigAllowed values comment in sync with the enum.The comment says
Bitwarden, but the actual enum value and CRD schema useBitwardenSDKServer. Update the human-readable list to match the real identifiers.Proposed change
type ComponentConfig struct { // componentName specifies which deployment component this configuration applies to. - // Allowed values: ExternalSecretsCoreController, Webhook, CertController, Bitwarden + // Allowed values: ExternalSecretsCoreController, Webhook, CertController, BitwardenSDKServer // +kubebuilder:validation:Enum:=ExternalSecretsCoreController;Webhook;CertController;BitwardenSDKServer // +kubebuilder:validation:Required ComponentName ComponentName `json:"componentName"`config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml (1)
1176-1204: Sync annotations description with CEL reserved-prefix rule.As in the bundle manifest, the description lists
kubernetes.io,app.kubernetes,openshift.io,k8s.iobut the validation and error message correctly use'kubernetes.io/','app.kubernetes.io/','openshift.io/','k8s.io/'. Please update the description here to match the actual prefixes being enforced.Proposed diff
- description: annotations are for adding custom annotations to - all the resources created for external-secrets deployment. The - annotations are merged with any default annotations set by the - operator. User-specified annotations takes precedence over defaults - in case of conflicts. Annotation keys with prefixes `kubernetes.io`, - `app.kubernetes`, `openshift.io`, or `k8s.io` are not allowed + description: annotations are for adding custom annotations to + all the resources created for external-secrets deployment. The + annotations are merged with any default annotations set by the + operator. User-specified annotations takes precedence over defaults + in case of conflicts. Annotation keys with prefixes `kubernetes.io/`, + `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowed
🧹 Nitpick comments (1)
api/v1alpha1/external_secrets_config_types.go (1)
186-191: HardenKVPair.Keywith a minimum-length validation.Since
AnnotationsuseslistMapKey=keyand these keys will ultimately be used as annotation keys, allowing an empty string here can surface as a later failure when rendering resources. Consider enforcing non-empty keys via MinLength.Proposed change
type KVPair struct { - // +kubebuilder:validation:Required - Key string `json:"key"` + // +kubebuilder:validation:Required + // +kubebuilder:validation:MinLength:=1 + Key string `json:"key"` Value string `json:"value,omitempty"` }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
api/v1alpha1/external_secrets_config_types.goapi/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yamlbundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/api_reference.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yamlbundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamlapi/v1alpha1/external_secrets_config_types.go
🔇 Additional comments (1)
api/v1alpha1/tests/externalsecretsconfig.operator.openshift.io/externalsecretsconfig.testsuite.yaml (1)
496-694: New tests correctly exercise annotation/env/deployment validations.The added cases for controllerConfig.annotations, componentConfig.overrideEnv, and deploymentConfigs.revisionHistoryLimit mirror the CRD/XValidation rules (including exact error messages and paths) and give good coverage of the new behavior.
| // annotations are for adding custom annotations to all the resources created for external-secrets deployment. The annotations are merged with any default annotations set by the operator. User-specified annotations takes precedence over defaults in case of conflicts. Annotation keys with prefixes `kubernetes.io`, `app.kubernetes`, `openshift.io`, or `k8s.io` are not allowed | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:validation:MaxItems:=20 | ||
| // +kubebuilder:validation:XValidation:rule="self.all(a, !['kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', 'k8s.io/'].exists(p, a.key.startsWith(p)))",message="annotations with reserved prefixes 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', 'k8s.io/' are not allowed" | ||
| // +listType=map | ||
| // +listMapKey=key | ||
| // +optional | ||
| Annotations []Annotation `json:"annotations,omitempty"` |
There was a problem hiding this comment.
Make annotations doc comment match the enforced reserved prefixes.
The comment lists prefixes kubernetes.io, app.kubernetes, openshift.io, k8s.io while the XValidation and CRDs enforce 'kubernetes.io/', 'app.kubernetes.io/', 'openshift.io/', 'k8s.io/'. Adjust the comment so it reflects the exact prefixes you actually block.
Proposed change
- // annotations are for adding custom annotations to all the resources created for external-secrets deployment. The annotations are merged with any default annotations set by the operator. User-specified annotations takes precedence over defaults in case of conflicts. Annotation keys with prefixes `kubernetes.io`, `app.kubernetes`, `openshift.io`, or `k8s.io` are not allowed
+ // annotations are for adding custom annotations to all the resources created for external-secrets deployment. The annotations are merged with any default annotations set by the operator. User-specified annotations takes precedence over defaults in case of conflicts. Annotation keys with prefixes `kubernetes.io/`, `app.kubernetes.io/`, `openshift.io/`, or `k8s.io/` are not allowed.🤖 Prompt for AI Agents
In api/v1alpha1/external_secrets_config_types.go around lines 117 to 124, the
doc comment lists reserved prefixes without trailing slashes while the
XValidation rule enforces prefixes with trailing slashes; update the comment
text to exactly match the enforced prefixes (kubernetes.io/, app.kubernetes.io/,
openshift.io/, k8s.io/) so the documentation and validation are consistent and
clearly indicate those exact blocked prefixes.
| componentConfig: | ||
| items: | ||
| properties: | ||
| componentName: | ||
| description: |- | ||
| componentName specifies which deployment component this configuration applies to. | ||
| Allowed values: ExternalSecretsCoreController, Webhook, CertController, Bitwarden | ||
| enum: | ||
| - ExternalSecretsCoreController | ||
| - Webhook | ||
| - CertController | ||
| - BitwardenSDKServer | ||
| type: string | ||
| deploymentConfigs: | ||
| description: deploymentConfigs allows specifying deployment-level | ||
| configuration overrides. | ||
| properties: | ||
| revisionHistoryLimit: | ||
| description: |- | ||
| revisionHistoryLimit specifies the number of old ReplicaSets to retain for rollback. | ||
| Minimum value of 1 is enforced to ensure rollback capability. | ||
| format: int32 | ||
| minimum: 1 | ||
| type: integer | ||
| type: object | ||
| overrideEnv: |
There was a problem hiding this comment.
Fix componentName Allowed values text to match enum (BitwardenSDKServer).
The componentName description says Allowed values include Bitwarden, but the enum (and Go type) use BitwardenSDKServer. This mismatch can confuse users authoring CRs; update the description to list the exact enum values.
Proposed diff
- description: |-
- componentName specifies which deployment component this configuration applies to.
- Allowed values: ExternalSecretsCoreController, Webhook, CertController, Bitwarden
+ description: |-
+ componentName specifies which deployment component this configuration applies to.
+ Allowed values: ExternalSecretsCoreController, Webhook, CertController, BitwardenSDKServer📝 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.
| componentConfig: | |
| items: | |
| properties: | |
| componentName: | |
| description: |- | |
| componentName specifies which deployment component this configuration applies to. | |
| Allowed values: ExternalSecretsCoreController, Webhook, CertController, Bitwarden | |
| enum: | |
| - ExternalSecretsCoreController | |
| - Webhook | |
| - CertController | |
| - BitwardenSDKServer | |
| type: string | |
| deploymentConfigs: | |
| description: deploymentConfigs allows specifying deployment-level | |
| configuration overrides. | |
| properties: | |
| revisionHistoryLimit: | |
| description: |- | |
| revisionHistoryLimit specifies the number of old ReplicaSets to retain for rollback. | |
| Minimum value of 1 is enforced to ensure rollback capability. | |
| format: int32 | |
| minimum: 1 | |
| type: integer | |
| type: object | |
| overrideEnv: | |
| componentName: | |
| description: |- | |
| componentName specifies which deployment component this configuration applies to. | |
| Allowed values: ExternalSecretsCoreController, Webhook, CertController, BitwardenSDKServer | |
| enum: | |
| - ExternalSecretsCoreController | |
| - Webhook | |
| - CertController | |
| - BitwardenSDKServer | |
| type: string | |
| deploymentConfigs: | |
| description: deploymentConfigs allows specifying deployment-level | |
| configuration overrides. | |
| properties: | |
| revisionHistoryLimit: | |
| description: |- | |
| revisionHistoryLimit specifies the number of old ReplicaSets to retain for rollback. | |
| Minimum value of 1 is enforced to ensure rollback capability. | |
| format: int32 | |
| minimum: 1 | |
| type: integer | |
| type: object | |
| overrideEnv: |
🤖 Prompt for AI Agents
In config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml around
lines 1295-1320, the componentName description lists "Bitwarden" as an allowed
value but the enum uses "BitwardenSDKServer"; update the description so the
Allowed values text exactly matches the enum entries
(ExternalSecretsCoreController, Webhook, CertController, BitwardenSDKServer) to
avoid mismatch with the CRD and Go types.
|
@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. |
4 participants
Add support for custom annotations, deployment configuration, and environment variable overrides for external-secrets operand components.