SSCSI-245: Add Secrets Store CSI driver configuration to ClusterCSIDriver API#2846
SSCSI-245: Add Secrets Store CSI driver configuration to ClusterCSIDriver API#2846chiragkyal wants to merge 3 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Hello @chiragkyal! Some important instructions when contributing to openshift/api: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request extends the CSI driver configuration API to support Secrets Store drivers. The Go API type CSIDriverType gains a new SecretsStore constant, CSIDriverConfigSpec gains a new secretsStore union field, and three new configuration struct types are introduced: SecretsStoreCSIDriverConfigSpec, SecretsStoreSecretRotation, and SecretsStoreTokenRequest. The corresponding CRD schemas in all five cluster-profile variants are updated with matching enum extensions, secretsStore schema definitions, and x-kubernetes-validations that require secretsStore presence only when driverType is SecretsStore. ChangesSecrets Store CSI driver support
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/XL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test all |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 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 `@operator/v1/types_csi_cluster_driver.go`:
- Around line 405-411: Update the field comments for TokenRequests (type
SecretsStoreTokenRequest) and the expirationSeconds field to document the
default behavior when omitted: state that if TokenRequests is nil/empty, no
service account tokens will be requested and the CSI driver will use the
kube-apiserver's default APIAudiences (or no tokens provided), and if
expirationSeconds is omitted the token lifetime defaults to the kubelet/cluster
provider default (or a specific default value used by the implementation).
Modify the comment blocks adjacent to the TokenRequests declaration and the
expirationSeconds declaration to include these omission semantics and expected
default values/behavior so the optional nature is clear.
- Around line 162-165: The new stable API field SecretsStore
(*SecretsStoreCSIDriverConfigSpec) must be gated with an OpenShift feature gate;
add the kubebuilder marker comment
+openshift:enable:FeatureGate=YourFeatureGateName immediately above the
`SecretsStore` field declaration (the SecretsStore
*SecretsStoreCSIDriverConfigSpec line) so the field is only enabled when the
feature gate is on, and update any related docs/tests to use that feature gate
name.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c45c7b74-a5df-45a7-8fef-3c1a81dc9f5c
⛔ Files ignored due to path filters (11)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/clustercsidrivers.operator.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/clustercsidrivers.operator.openshift.io/AWSEuropeanSovereignCloudInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/clustercsidrivers.operator.openshift.io/VSphereConfigurableMaxAllowedBlockVolumesPerNode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (6)
operator/v1/types_csi_cluster_driver.gopayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yaml
| // secretsStore is used to configure the Secrets Store CSI driver. | ||
| // +optional | ||
| SecretsStore *SecretsStoreCSIDriverConfigSpec `json:"secretsStore,omitempty"` | ||
| } |
There was a problem hiding this comment.
Gate the new stable v1 field behind a feature gate.
Line 162 introduces a new stable API field without a +openshift:enable:FeatureGate=... marker. That violates the stable API rollout requirement and can expose partially-supported config in GA surface.
Suggested direction
// secretsStore is used to configure the Secrets Store CSI driver.
+ // +openshift:enable:FeatureGate=SecretsStoreCSIDriver
// +optional
SecretsStore *SecretsStoreCSIDriverConfigSpec `json:"secretsStore,omitempty"`As per coding guidelines, **/types*.go: New fields on stable APIs should be introduced behind a feature gate using +openshift:enable:FeatureGate=MyFeatureGate.
📝 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.
| // secretsStore is used to configure the Secrets Store CSI driver. | |
| // +optional | |
| SecretsStore *SecretsStoreCSIDriverConfigSpec `json:"secretsStore,omitempty"` | |
| } | |
| // secretsStore is used to configure the Secrets Store CSI driver. | |
| // +openshift:enable:FeatureGate=SecretsStoreCSIDriver | |
| // +optional | |
| SecretsStore *SecretsStoreCSIDriverConfigSpec `json:"secretsStore,omitempty"` | |
| } |
🤖 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 `@operator/v1/types_csi_cluster_driver.go` around lines 162 - 165, The new
stable API field SecretsStore (*SecretsStoreCSIDriverConfigSpec) must be gated
with an OpenShift feature gate; add the kubebuilder marker comment
+openshift:enable:FeatureGate=YourFeatureGateName immediately above the
`SecretsStore` field declaration (the SecretsStore
*SecretsStoreCSIDriverConfigSpec line) so the field is only enabled when the
feature gate is on, and update any related docs/tests to use that feature gate
name.
There was a problem hiding this comment.
This is a day2 operator, not an OpenShift core operator. I think we can add this feature to GA directly.
@mytreya-rh what do you think?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@chiragkyal I've been assigned for this review, but before starting a proper review I wanted to try and better understand the architecture here.
From doing some digging, it looks like this API is shipped as a CRD in the core OpenShift payload but that there are individual OLM-managed operators that end up reading this API with a name associated with that CSI driver operator. Is this correct?
Is there a central CSI operator that runs as part of core OpenShift that uses this API as well?
There was a problem hiding this comment.
Yes, that's correct.
The ClusterCSIDriver is shipped with the core OpenShift payload and is not owned by any single operator.
To my understanding cluster-storage-operator (CSO) deploys CSI driver operators (EBS, Azure, GCP, vSphere) and creates their ClusterCSIDriver CR instances.
Via Search for WithKind("ClusterCSIDriver") - it shows the uses across openshift/csi-operator (monorepo for platform + some OLM drivers) and individual operator repos (like secrets-store and gcp-filestorage)
For secrets-store specifically, it's an OLM-managed optional operator (not deployed by CSO). After OLM installs the operator, the admin creates the ClusterCSIDriver named secrets-store.csi.k8s.io.
There was a problem hiding this comment.
For posterity, re-posting my comment in slack here:
I had a chat with @JoelSpeed about this yesterday as a sanity check.
We came to the conclusion that we can make this change without a gate and treat it the same way we would have treated a distinct layered-operator-owned API since that is historically how this has been treated.
One thing we would like to see added though is validation constraints that enforce that when the .metadata.name of a ClusterCSIDriver resource is set to a particular CSI driver, that in the spec, only the configuration options associated with that CSI driver can be configured.
As an example, for this particular case, when creating the ClusterCSIDriver for the secrets store csi driver using the name secrets-store.csi.k8s.io, spec.driverConfig.type can only be set to SecretsStore.
There was a problem hiding this comment.
Done. I've added a CEL validation rule at the ClusterCSIDriver type level:
+kubebuilder:validation:XValidation:rule="self.spec.driverConfig.driverType == 'SecretsStore' ? self.metadata.name == 'secrets-store.csi.k8s.io' : (self.metadata.name != 'secrets-store.csi.k8s.io' || self.spec.driverConfig.driverType == '')",message="metadata.name 'secrets-store.csi.k8s.io' requires driverType 'SecretsStore', and driverType 'SecretsStore' requires metadata.name 'secrets-store.csi.k8s.io'"
chiragkyal
force-pushed
the
configure-secret-rotation-and-wif
branch
from
dd82f59 to
87f2c52
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
chiragkyal
force-pushed
the
configure-secret-rotation-and-wif
branch
2 times, most recently
from
eb0e5bd to
e08f398
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yaml`:
- Around line 310-317: The schema for the CRD field expirationSeconds does not
enforce the documented lower bound of 600; update the OpenAPI schema for the
property named expirationSeconds in the CRD (within the properties block where
type: integer and format: int64 are defined) to include minimum: 600 so
admission validation will reject values below 600; ensure the minimum is set
alongside the existing type/format entry.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 02b1535f-ee7f-421e-b9f7-a4e43b5dd6c5
⛔ Files ignored due to path filters (11)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/clustercsidrivers.operator.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/clustercsidrivers.operator.openshift.io/AWSEuropeanSovereignCloudInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/clustercsidrivers.operator.openshift.io/VSphereConfigurableMaxAllowedBlockVolumesPerNode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (6)
operator/v1/types_csi_cluster_driver.gopayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yaml
- payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yaml
- operator/v1/types_csi_cluster_driver.go
- payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml
- payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yaml
|
/cc @mytreya-rh |
chiragkyal
force-pushed
the
configure-secret-rotation-and-wif
branch
from
e08f398 to
4836779
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
operator/v1/types_csi_cluster_driver.go (1)
116-125:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGate the new stable
SecretsStoreAPI surface.Lines 116-125 and 162-164 make
SecretsStorepart of the stable v1 discriminator/union, and the generated manifests in this PR already carry it into the ungated/default CRDs. If this is not intentionally GA-on-arrival, it needs to be feature-gated at the API layer so the stable surface does not expand for every cluster profile yet.As per coding guidelines,
**/types*.go: New fields on stable APIs should be introduced behind a feature gate using+openshift:enable:FeatureGate=MyFeatureGate.Also applies to: 162-164
🤖 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 `@operator/v1/types_csi_cluster_driver.go` around lines 116 - 125, The SecretsStore driver constant was added to the stable CSIDriverType union and must be feature-gated; add the openshift feature-gate annotation to the API so the new discriminator is not GA-on-arrival: locate the CSIDriverType declaration and the SecretsStoreDriverType constant (symbols: CSIDriverType and SecretsStoreDriverType) and add the kubebuilder/openshift annotation `+openshift:enable:FeatureGate=SecretsStoreCSIDriver` (or your chosen gate name) immediately above the relevant type/constant declarations (also apply the same annotation to the other occurrence referenced around lines 162-164) so CRD generation treats SecretsStore as gated.
🤖 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 `@operator/v1/types_csi_cluster_driver.go`:
- Around line 454-459: The ExpirationSeconds field on the CSIClusterDriver type
lacks the kubebuilder validation enforcing the documented minimum of 600
seconds; update the field tag for ExpirationSeconds (*int64
`json:"expirationSeconds,omitempty"`) to include
+kubebuilder:validation:Minimum=600 (and keep +optional) so the generated CRD
will reject values <600 and match the documented contract.
In
`@operator/v1/zz_generated.featuregated-crd-manifests/clustercsidrivers.operator.openshift.io/VSphereConfigurableMaxAllowedBlockVolumesPerNode.yaml`:
- Around line 289-296: The schema for the expirationSeconds field currently only
declares type/format but does not enforce the documented lower bound; update the
CRD manifest so the expirationSeconds property includes a numeric minimum of 600
(e.g. add "minimum: 600" alongside the existing "type: integer" and "format:
int64") to ensure values less than 600 are rejected by validation; locate the
expirationSeconds definition in the
VSphereConfigurableMaxAllowedBlockVolumesPerNode.yaml and add the minimum
constraint for that property.
---
Duplicate comments:
In `@operator/v1/types_csi_cluster_driver.go`:
- Around line 116-125: The SecretsStore driver constant was added to the stable
CSIDriverType union and must be feature-gated; add the openshift feature-gate
annotation to the API so the new discriminator is not GA-on-arrival: locate the
CSIDriverType declaration and the SecretsStoreDriverType constant (symbols:
CSIDriverType and SecretsStoreDriverType) and add the kubebuilder/openshift
annotation `+openshift:enable:FeatureGate=SecretsStoreCSIDriver` (or your chosen
gate name) immediately above the relevant type/constant declarations (also apply
the same annotation to the other occurrence referenced around lines 162-164) so
CRD generation treats SecretsStore as gated.
🪄 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: dc4e1222-9530-4acb-9ae8-509a500a44a1
⛔ Files ignored due to path filters (3)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!**/zz_generated*operator/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (14)
operator/v1/types_csi_cluster_driver.gooperator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamloperator/v1/zz_generated.featuregated-crd-manifests/clustercsidrivers.operator.openshift.io/AAA_ungated.yamloperator/v1/zz_generated.featuregated-crd-manifests/clustercsidrivers.operator.openshift.io/AWSEuropeanSovereignCloudInstall.yamloperator/v1/zz_generated.featuregated-crd-manifests/clustercsidrivers.operator.openshift.io/VSphereConfigurableMaxAllowedBlockVolumesPerNode.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml
- payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yaml
chiragkyal
force-pushed
the
configure-secret-rotation-and-wif
branch
from
4836779 to
d7c890b
Compare
openshift-ci
Bot
added
size/XXL
chiragkyal
force-pushed
the
configure-secret-rotation-and-wif
branch
from
d7c890b to
baa69d9
Compare
Extends CSIDriverConfigSpec with a new SecretsStore discriminated union variant containing secretRotation and tokenRequests fields. Signed-off-by: chiragkyal <ckyal@redhat.com>
chiragkyal
force-pushed
the
configure-secret-rotation-and-wif
branch
from
baa69d9 to
7a40d90
Compare
chiragkyal
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@chiragkyal: This pull request references SSCSI-245 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Signed-off-by: chiragkyal <ckyal@redhat.com>
chiragkyal
force-pushed
the
configure-secret-rotation-and-wif
branch
from
7a40d90 to
660698a
Compare
|
|
||
| // secretsStore is used to configure the Secrets Store CSI driver. | ||
| // +optional | ||
| SecretsStore *SecretsStoreCSIDriverConfigSpec `json:"secretsStore,omitempty"` |
There was a problem hiding this comment.
Just a note, because the configuration for this can never be secretsStore: {} based on the validations this does not need to be a pointer.
Instead this can be:
| SecretsStore *SecretsStoreCSIDriverConfigSpec `json:"secretsStore,omitempty"` | |
| SecretsStore SecretsStoreCSIDriverConfigSpec `json:"secretsStore,omitzero"` |
There was a problem hiding this comment.
With the latest changes, I think secretsStore can be empty, so having an easier differenciator between empty and unset, I think pointer makes more sense.
Moreover, for consistency with the other driver config fields in CSIDriverConfigSpec, which are all pointers.
There was a problem hiding this comment.
It still cannot be empty. There is a minimum properties of 1 constraint being enforced, making secretsStore: {} an invalid value.
I would strongly recommend removing the pointer. While I understand the consistency argument, we've been trying to adopt these newer approaches over the older ones as we've learned more about the pitfalls of some of our older approaches.
From an end-user perspective it will be consistent, which is the most important. Our controller implementations can handle the slight inconsistency.
| // secretsStore is used to configure the Secrets Store CSI driver. | ||
| // +optional | ||
| SecretsStore *SecretsStoreCSIDriverConfigSpec `json:"secretsStore,omitempty"` | ||
| } |
There was a problem hiding this comment.
For posterity, re-posting my comment in slack here:
I had a chat with @JoelSpeed about this yesterday as a sanity check.
We came to the conclusion that we can make this change without a gate and treat it the same way we would have treated a distinct layered-operator-owned API since that is historically how this has been treated.
One thing we would like to see added though is validation constraints that enforce that when the .metadata.name of a ClusterCSIDriver resource is set to a particular CSI driver, that in the spec, only the configuration options associated with that CSI driver can be configured.
As an example, for this particular case, when creating the ClusterCSIDriver for the secrets store csi driver using the name secrets-store.csi.k8s.io, spec.driverConfig.type can only be set to SecretsStore.
| // Default is 120 (2 minutes). | ||
| // +default=120 | ||
| // +optional | ||
| RotationPollIntervalSeconds *int32 `json:"rotationPollIntervalSeconds,omitempty"` |
There was a problem hiding this comment.
Is there reasonable lower and upper bounds we can enforce here?
For example, how realistic is it to set this to something like 1 second? What about 2147483647 seconds (~68 years)?
There was a problem hiding this comment.
The upstream Secrets Store CSI driver does not enforce any lower or upper bound on the rotation poll interval. It accepts any positive duration value. Can't we stay aligned with upstream rather than introduce arbitrary restrictions?
- It requires rigorous testing to validate the chosen bounds are safe for all scenarios
The +default=120 ensures a sane default when the field is omitted. If a need for bounds is identified later, can it not be added in a follow-up?
There was a problem hiding this comment.
A few things here:
The
+default=120ensures a sane default when the field is omitted
I would remove the defaulting behavior here. We don't recommend it for configuration APIs and because this is a discriminated union, the defaulting behavior may result in validation errors.
The upstream Secrets Store CSI driver does not enforce any lower or upper bound on the rotation poll interval. It accepts any positive duration value. Can't we stay aligned with upstream rather than introduce arbitrary restrictions?
We encourage having reasonable bounds on fields to prevent users from being able to create bad configurations where possible. If you feel strongly that there are no reasonable bounds to enforce here I won't push further, but setting the maximum int32 value for rotation seems unreasonable to me. We do recommend putting a buffer to your limits as well.
I think you should at least have a lower bound here. Specifying something less than 0 doesn't make sense here. Is setting 0 even a valid value? What does it mean to set this to 0?
For upper bounds, I did a quick "best practices" search and it looks like the standard recommendation is ~1-2 hours, which would be 3600-7200 seconds. I could imagine at least a daily or weekly rotation poll cadence being reasonable. Monthly and yearly sound unlikely but reasonable to account for.
Accounting for ~ yearly rotations you'd be at ~31560000 seconds. Maybe something like double that is a reasonable upper bounds to start with?
If a need for bounds is identified later, can it not be added in a follow-up?
Not easily. Adding bounds after the fact is a breaking change and requires collecting telemetry data to understand the potential impacts of adding the bounds after the fact. It is normally easier to loosen validations than restrict them. If you think you may need bounds here at all, I'd add them now.
| // When "Managed", the operator sets tokenRequests from the audiences | ||
| // list, replacing any previously configured values. | ||
| // Once set to "Managed", policy cannot be reverted back to "Unmanaged". | ||
| // +default="Unmanaged" |
There was a problem hiding this comment.
For configuration APIs, we try to avoid defaulting in this way because it contractually locks us into that default forever.
Instead, for configuration APIs we prefer that omission of the parent field represents that the end-user has "no opinion" and that the platform is left to choose the default value which may be subject to change over time.
There was a problem hiding this comment.
Addressed. The secretRotation and tokenRequests fields are now updated.
| // expirationSeconds is the requested duration of validity of the service account token. | ||
| // The token issuer may return a token with a different validity duration. | ||
| // When omitted, the token expiration is determined by the kube-apiserver. | ||
| // Must be at least 600 seconds (10 minutes). | ||
| // +optional | ||
| ExpirationSeconds *int64 `json:"expirationSeconds,omitempty"` |
There was a problem hiding this comment.
What does it mean to configure multiple audiences each with a different expiration seconds setting?
There was a problem hiding this comment.
Each audience in tokenRequests represents a separate service account token. Different cloud providers may require different token lifetime requirements.
Also this is same as the upstream storage.k8s.io/v1 CSIDriver.spec.tokenRequests field structure
A multi-cloud WIF scenario would configure multiple audiences:
tokenRequests:
type: Managed
managed:
audiences:
- audience: "sts.amazonaws.com"
expirationSeconds: 3600
- audience: "api://AzureADTokenExchange"
expirationSeconds: 600
chiragkyal
force-pushed
the
configure-secret-rotation-and-wif
branch
from
117191b to
c0734eb
Compare
Signed-off-by: chiragkyal <ckyal@redhat.com>
chiragkyal
force-pushed
the
configure-secret-rotation-and-wif
branch
from
c0734eb to
030c472
Compare
|
@chiragkyal: The following test failed, say
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. |
|
|
||
| // secretsStore is used to configure the Secrets Store CSI driver. | ||
| // +optional | ||
| SecretsStore *SecretsStoreCSIDriverConfigSpec `json:"secretsStore,omitempty"` |
There was a problem hiding this comment.
It still cannot be empty. There is a minimum properties of 1 constraint being enforced, making secretsStore: {} an invalid value.
I would strongly recommend removing the pointer. While I understand the consistency argument, we've been trying to adopt these newer approaches over the older ones as we've learned more about the pitfalls of some of our older approaches.
From an end-user perspective it will be consistent, which is the most important. Our controller implementations can handle the slight inconsistency.
| // secretRotation controls automatic secret rotation behavior. | ||
| // When omitted, secret rotation is enabled with a default poll interval of 2 minutes. | ||
| // +optional | ||
| SecretRotation *SecretsStoreSecretRotation `json:"secretRotation,omitempty"` |
There was a problem hiding this comment.
Does not need to be a pointer. This field cannot be explicitly set to secretRotation: {} because it has a required child field.
| SecretRotation *SecretsStoreSecretRotation `json:"secretRotation,omitempty"` | |
| SecretRotation SecretsStoreSecretRotation `json:"secretRotation,omitzero"` |
| // tokenRequests controls service account token configuration for | ||
| // workload identity federation (WIF) with cloud providers. | ||
| // +optional | ||
| TokenRequests *SecretsStoreTokenRequests `json:"tokenRequests,omitempty"` |
There was a problem hiding this comment.
Similar comment as above, this does not need to be a pointer:
| TokenRequests *SecretsStoreTokenRequests `json:"tokenRequests,omitempty"` | |
| TokenRequests SecretsStoreTokenRequests `json:"tokenRequests,omitzero"` |
|
|
||
| // tokenRequests controls service account token configuration for | ||
| // workload identity federation (WIF) with cloud providers. | ||
| // +optional |
There was a problem hiding this comment.
Explicitly document the behavior when this field is omitted.
| // managed holds configuration for operator-managed tokenRequests. | ||
| // Only valid when type is "Managed". | ||
| // +optional | ||
| Managed *ManagedTokenRequests `json:"managed,omitempty"` |
There was a problem hiding this comment.
What does it mean to set:
type: Managed
managed: {}?
Should that be allowed?
| // enable workload identity federation (WIF) with cloud providers such as | ||
| // AWS, Azure, and GCP. | ||
| // +optional | ||
| // +listType=atomic |
There was a problem hiding this comment.
Any particular reason why this needs to be atomic?
| // +optional | ||
| // +listType=atomic | ||
| // +kubebuilder:validation:MaxItems=10 | ||
| Audiences []SecretsStoreTokenRequest `json:"audiences,omitempty"` |
There was a problem hiding this comment.
It doesn't look like audiences: [] would be a supported value here, so please add a minimum length of 1 constraint here.
There was a problem hiding this comment.
Additionally, are there any other constraints that should be enforced here?
For example, can I have duplicate audience entries?
| // Default is 120 (2 minutes). | ||
| // +default=120 | ||
| // +optional | ||
| RotationPollIntervalSeconds *int32 `json:"rotationPollIntervalSeconds,omitempty"` |
There was a problem hiding this comment.
A few things here:
The
+default=120ensures a sane default when the field is omitted
I would remove the defaulting behavior here. We don't recommend it for configuration APIs and because this is a discriminated union, the defaulting behavior may result in validation errors.
The upstream Secrets Store CSI driver does not enforce any lower or upper bound on the rotation poll interval. It accepts any positive duration value. Can't we stay aligned with upstream rather than introduce arbitrary restrictions?
We encourage having reasonable bounds on fields to prevent users from being able to create bad configurations where possible. If you feel strongly that there are no reasonable bounds to enforce here I won't push further, but setting the maximum int32 value for rotation seems unreasonable to me. We do recommend putting a buffer to your limits as well.
I think you should at least have a lower bound here. Specifying something less than 0 doesn't make sense here. Is setting 0 even a valid value? What does it mean to set this to 0?
For upper bounds, I did a quick "best practices" search and it looks like the standard recommendation is ~1-2 hours, which would be 3600-7200 seconds. I could imagine at least a daily or weekly rotation poll cadence being reasonable. Monthly and yearly sound unlikely but reasonable to account for.
Accounting for ~ yearly rotations you'd be at ~31560000 seconds. Maybe something like double that is a reasonable upper bounds to start with?
If a need for bounds is identified later, can it not be added in a follow-up?
Not easily. Adding bounds after the fact is a breaking change and requires collecting telemetry data to understand the potential impacts of adding the bounds after the fact. It is normally easier to loosen validations than restrict them. If you think you may need bounds here at all, I'd add them now.
| // When omitted, the token expiration is determined by the kube-apiserver. | ||
| // Must be at least 600 seconds (10 minutes) and no more than 4294967296 (1 << 32) seconds. | ||
| // +kubebuilder:validation:Minimum=600 | ||
| // +kubebuilder:validation:Maximum=4294967296 |
There was a problem hiding this comment.
Is this a reasonable maximum? This is equivalent to ~138 years, which is essentially a "never expires" token.
The general best practice here is for these to be shorter lived, but I do realize that there may be end-users who want long-lived tokens. Something like 10 years seems like a sufficiently large (and arguably too large) range here.
4 participants
Summary
Adds
SecretsStoreas a newCSIDriverTypeenum value in theClusterCSIDriverAPI (operator.openshift.io/v1)Introduces
SecretsStoreCSIDriverConfigSpecwith configuration for:the poll interval (defaults to enabled with 2m interval)
workload identity federation (WIF)
Implements: SSCSI-254: Configurable secret rotation and WIF support for SSCSI enhancements#2012
Summary by CodeRabbit
SecretsStoreCSI driver type.SecretsStoredriver type.