Skip to content

SSCSI-245: Add Secrets Store CSI driver configuration to ClusterCSIDriver API #2846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chiragkyal wants to merge 7 commits into
base: master
Choose a base branch
from
Open

SSCSI-245: Add Secrets Store CSI driver configuration to ClusterCSIDriver API #2846

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6c7c195
Add Secrets Store CSI driver configuration to ClusterCSIDriver API
chiragkyal May 15, 2026
660698a
Add API integration tests
chiragkyal Jun 2, 2026
030c472
Address review comments
chiragkyal Jun 8, 2026
134c2aa
Address review comments
chiragkyal Jun 11, 2026
0c4a778
Address review comments
chiragkyal Jun 12, 2026
ed08ac9
Address review comments
chiragkyal Jun 15, 2026
ff412df
Address review comments
chiragkyal Jun 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
218 changes: 211 additions & 7 deletions openapi/generated_openapi/zz_generated.openapi.go
View file
Open in desktop

Large diffs are not rendered by default.

643 changes: 643 additions & 0 deletions operator/v1/tests/clustercsidrivers.operator.openshift.io/AAA_ungated.yaml
View file
Open in desktop

Large diffs are not rendered by default.

160 changes: 153 additions & 7 deletions operator/v1/types_csi_cluster_driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
// +kubebuilder:subresource:status
// +openshift:api-approved.openshift.io=https://github.com/openshift/api/pull/701
// +openshift:file-pattern=cvoRunLevel=0000_50,operatorName=csi-driver,operatorOrdering=01
// +kubebuilder:validation:XValidation:rule="!has(self.spec.driverConfig) || (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'"

@everettraven everettraven Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think you might be able to make this expression a bit easier to read by making this 2 expressions:

self.spec.?driverConfig.type.orValue("") == "SecretsStore" ? self.metadata.name == "secrets-store.csi.k8s.io" : true

and

self.metadata.name == "secrets-store.csi.k8s.io" ? (has(self.spec.driverConfig) ? self.spec.driverConfig.type == "SecretsStore" : true) : true

This should ensure that whenever driver config is specified and type SecretsStore is selected that the metadata.name must be "secrets-store.csi.k8s.io" and that whenever metadata.name is "secrets-store.csi.k8s.io" that driver config type must be "SecretsStore".

@chiragkyal chiragkyal Jun 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Thank you!


// ClusterCSIDriver object allows management and configuration of a CSI driver operator
// installed by default in OpenShift. Name of the object must be name of the CSI driver
Expand Down Expand Up @@ -113,25 +114,27 @@ type ClusterCSIDriverSpec struct {
}

// CSIDriverType indicates type of CSI driver being configured.
// +kubebuilder:validation:Enum="";AWS;Azure;GCP;IBMCloud;vSphere
// +kubebuilder:validation:Enum="";AWS;Azure;GCP;IBMCloud;vSphere;SecretsStore
type CSIDriverType string

const (
AWSDriverType CSIDriverType = "AWS"
AzureDriverType CSIDriverType = "Azure"
GCPDriverType CSIDriverType = "GCP"
IBMCloudDriverType CSIDriverType = "IBMCloud"
VSphereDriverType CSIDriverType = "vSphere"
AWSDriverType CSIDriverType = "AWS"
AzureDriverType CSIDriverType = "Azure"
GCPDriverType CSIDriverType = "GCP"
IBMCloudDriverType CSIDriverType = "IBMCloud"
VSphereDriverType CSIDriverType = "vSphere"
SecretsStoreDriverType CSIDriverType = "SecretsStore"
)

// CSIDriverConfigSpec defines configuration spec that can be
// used to optionally configure a specific CSI Driver.
// +kubebuilder:validation:XValidation:rule="has(self.driverType) && self.driverType == 'IBMCloud' ? has(self.ibmcloud) : !has(self.ibmcloud)",message="ibmcloud must be set if driverType is 'IBMCloud', but remain unset otherwise"
// +kubebuilder:validation:XValidation:rule="has(self.driverType) && self.driverType == 'SecretsStore' ? has(self.secretsStore) : !has(self.secretsStore)",message="secretsStore must be set if driverType is 'SecretsStore', but remain unset otherwise"
// +union
type CSIDriverConfigSpec struct {
// driverType indicates type of CSI driver for which the
// driverConfig is being applied to.
// Valid values are: AWS, Azure, GCP, IBMCloud, vSphere and omitted.
// Valid values are: AWS, Azure, GCP, IBMCloud, vSphere, SecretsStore and omitted.
// Consumers should treat unknown values as a NO-OP.
// +required
// +unionDiscriminator
Expand All @@ -156,6 +159,10 @@ type CSIDriverConfigSpec struct {
// vSphere is used to configure the vsphere CSI driver.
// +optional
VSphere *VSphereCSIDriverConfigSpec `json:"vSphere,omitempty"`

// secretsStore is used to configure the Secrets Store CSI driver.
// +optional
SecretsStore *SecretsStoreCSIDriverConfigSpec `json:"secretsStore,omitempty"`

@everettraven everettraven Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
SecretsStore *SecretsStoreCSIDriverConfigSpec `json:"secretsStore,omitempty"`
SecretsStore SecretsStoreCSIDriverConfigSpec `json:"secretsStore,omitzero"`

@chiragkyal chiragkyal Jun 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@everettraven everettraven Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@chiragkyal chiragkyal Jun 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to give some background context, so that we all remain on the same page.

The secrets-store-csi-driver-operator has been shipping with secret rotation enabled by default with 2 minute poll interval, with out any user configuration option. Existing clusters have a minimal ClusterCSIDriver in etcd with just managementState: Managed

apiVersion: operator.openshift.io/v1
kind: ClusterCSIDriver
metadata:
  name: secrets-store.csi.k8s.io
spec:
  managementState: Managed

— without `driverConfig, no secretsStore, no secretRotation, no tokenRequests fields exist in storage.

On upgrading to this new API version, we do not want to change the existing cluster's behaviour, so the controller should still rotate secrets every 2 minutes. We need to design the API in such a way that the upgrade works well with the default behaviour, and also should be configurable as a day 2 operation.

I think that upon upgrading, all these fields will remain absent (unset). So, the operator detects this absence and continues applying the current defaults (rotation enabled, 2m interval). This is why "field omitted = no opinion, use platform defaults" is the correct approach for us.

Now coming to this thread:

I agree on switching SecretRotation and TokenRequests to value types with omitzero.

The discriminated union requires a non-empty Type for any valid config, so:

  • Zero value (Type: "", Custom/Managed: nil) = means "not configured"
  • Any valid config has non-zero Type ("None", "Custom", "Managed", "Unmanaged")
ccd.Spec.DriverConfig.SecretsStore.SecretRotation.Type == ""             // zero value
ccd.Spec.DriverConfig.SecretsStore.TokenRequests.Type == ""              // zero value

The operator will check Type == "" to detect the "no opinion" / upgrade case.

However, for SecretsStore on CSIDriverConfigSpec, there is a complication I think.

The parent CSIDriverConfigSpec enforces a bidirectional CEL rule:

"has(self.driverType) && self.driverType == 'SecretsStore' ? has(self.secretsStore) : !has(self.secretsStore)"

This requires secretsStore to be present when driverType == SecretsStore.

With omitzero, if a user somehow submits secretsStore: {} (all children at zero value), it would pass admission (has(self.secretsStore) = true), but then omitzero strips it on serialization to etcd. On the next update, has(self.secretsStore) becomes false while driverType is still SecretsStore, breaking the CEL rule.

I think the existing +kubebuilder:validation:MinProperties=1 on SecretsStoreCSIDriverConfigSpec prevents this scenario - it rejects secretsStore: {} at admission, making it non-zero and safe from omitzero stripping.

With that, I wanted to confirm certain things

  • We should have a clear distinction between unset and zero values.
  • Can you confirm we should keep MinProperties=1 on SecretsStoreCSIDriverConfigSpec when switching to value type with omitzero?
  • Should we relax the CEL rule to only enforce the reverse direction: "if secretsStore is present, driverType must be SecretsStore" - allowing omission to mean "use platform defaults."

P.S: Some of the bits were taken from Cursor's suggestion.

@everettraven everettraven Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of your points here seem correct to me. Having the minimum properties constraint that requires at least one field to be set will mean that the OpenAPI schema evaluation will reject secretsStore: {} from ever being admitted.

We should have a clear distinction between unset and zero values.

I don't think you need this here. You do not allow the input of zero values for these fields, so if they are ever the zero value you can safely assume they have not been explicitly set by an end user - otherwise they wouldn't be allowed to have set it.

Can you confirm we should keep MinProperties=1 on SecretsStoreCSIDriverConfigSpec when switching to value type with omitzero?

Yes, you need to keep the min properties of 1 in this case.

Should we relax the CEL rule to only enforce the reverse direction: "if secretsStore is present, driverType must be SecretsStore" - allowing omission to mean "use platform defaults."

I don't think so. Absence of the driverConfig field should represent that scenario instead right? If they are specifying a driverConfig, they are likely opinionated on that configuration.

@chiragkyal chiragkyal Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the confirmation. Updated as per the suggestion.

}
Comment thread
everettraven marked this conversation as resolved.

// AWSCSIDriverConfigSpec defines properties that can be configured for the AWS CSI driver.
Expand Down Expand Up @@ -389,6 +396,145 @@ type VSphereCSIDriverConfigSpec struct {
MaxAllowedBlockVolumesPerNode int32 `json:"maxAllowedBlockVolumesPerNode,omitempty"`
}

// SecretsStoreCSIDriverConfigSpec defines properties that can be configured for the Secrets Store CSI driver.
// +kubebuilder:validation:MinProperties=1
// +kubebuilder:validation:XValidation:rule="!has(oldSelf.tokenRequests) || oldSelf.tokenRequests.type != 'Managed' || (has(self.tokenRequests) && self.tokenRequests.type == 'Managed')",message="tokenRequests cannot be removed when type is Managed"
type SecretsStoreCSIDriverConfigSpec struct {
// 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"`
Comment thread
chiragkyal marked this conversation as resolved.
Outdated

// tokenRequests controls service account token configuration for
// workload identity federation (WIF) with cloud providers.
// When omitted, the operator preserves any existing tokenRequests
// already configured on the CSIDriver object without modification.
// +optional
Comment thread
chiragkyal marked this conversation as resolved.
TokenRequests *SecretsStoreTokenRequests `json:"tokenRequests,omitempty"`
Comment thread
chiragkyal marked this conversation as resolved.
Outdated
}

// TokenRequestsType determines how the operator manages the tokenRequests
// field on the storage.k8s.io CSIDriver object.
// +kubebuilder:validation:Enum=Managed;Unmanaged
type TokenRequestsType string

const (
// TokenRequestsManaged means the operator uses the audiences list
// as the sole source of truth for the CSIDriver.spec.tokenRequests field.
TokenRequestsManaged TokenRequestsType = "Managed"

// TokenRequestsUnmanaged means the operator preserves any existing
// tokenRequests already configured on the CSIDriver object and does not
// overwrite them.
TokenRequestsUnmanaged TokenRequestsType = "Unmanaged"
)

// SecretsStoreTokenRequests configures how service account tokens are
// provided to the Secrets Store CSI driver for workload identity federation.
// +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'Managed' ? has(self.managed) : !has(self.managed)",message="managed must be set when type is 'Managed', and must not be set otherwise"
// +kubebuilder:validation:XValidation:rule="!has(oldSelf.type) || oldSelf.type != 'Managed' || self.type == 'Managed'",message="type cannot be changed from Managed back to Unmanaged"

@everettraven everettraven Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rules like this need to be enforced at the highest required parent field. Otherwise, someone could work around this by unsetting spec.driverConfig and then apply the modified spec.driverConfig.

@chiragkyal chiragkyal Jun 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. Addressed.

// +union
type SecretsStoreTokenRequests struct {
Comment thread
everettraven marked this conversation as resolved.
// type determines how the operator manages tokenRequests on the CSIDriver object.
// When "Unmanaged", existing tokenRequests on the CSIDriver are preserved
// and the managed field is not used.
// When "Managed", the operator sets tokenRequests from the audiences
// specified in the managed field, replacing any previously configured values.
// Once set to "Managed", type cannot be reverted back to "Unmanaged".
// +unionDiscriminator
// +required
Type TokenRequestsType `json:"type,omitempty"`

// managed holds configuration for operator-managed tokenRequests.
// Only valid when type is "Managed".
// +optional
Managed *ManagedTokenRequests `json:"managed,omitempty"`

@everettraven everettraven Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean to set:

type: Managed
managed: {}

?

Should that be allowed?

@chiragkyal chiragkyal Jun 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting type: Managed, managed: {} means the operator clears all audiences from the CSIDriver.

This is allowed intentionally because type is a one-way transition (cannot revert from Managed to Unmanaged). Without this, a user who opted into Managed would have no way to clear WIF configuration later. The semantic is:

type: Managed, managed: { audiences: [...] } → operator sets these audiences on CSIDriver
type: Managed, managed: {} (or empty audiences) → operator clears all tokenRequests from CSIDriver
type: Unmanaged → operator preserves whatever is already on the CSIDriver

@everettraven everettraven Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

managed: {} (or empty audiences)

Pick one, not both. We prefer having only a single way to represent a desired state. In this case, I think that audiences: [] is a more clear representation than managed: {}.

To prevent managed: {} you'll need to add a minimum properties of 1 to the ManagedTokenRequests type

@chiragkyal chiragkyal Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. Updated!

@chiragkyal chiragkyal Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@everettraven, following the same pattern as Custom, since we now have MinProperties=1 on ManagedTokenRequests, should we also convert this to a value type with omitzero?

Suggested change
Managed *ManagedTokenRequests `json:"managed,omitempty"`
Managed ManagedTokenRequests `json:"managed,omitzero"`

@everettraven everettraven Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

@chiragkyal chiragkyal Jun 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, thanks!

}

// ManagedTokenRequests holds the configuration for operator-managed
// service account token requests.
type ManagedTokenRequests struct {
// audiences specifies service account token audiences that kubelet will
// provide to the CSI driver during NodePublishVolume calls. These tokens
// enable workload identity federation (WIF) with cloud providers such as
// AWS, Azure, and GCP.
// When empty or omitted, the operator clears all tokenRequests from the
// CSIDriver object.
// +optional
// +listType=atomic

@everettraven everettraven Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason why this needs to be atomic?

@chiragkyal chiragkyal Jun 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

atomic because the operator applies the entire audiences list to CSIDriver.spec.tokenRequests. I don't see any use case for partial SSA merges.

Do you think we could use something else? Like +listType=map with +listMapKey=audience ? That would enforce uniqueness on audience automatically (no need for the CEL rule)

@everettraven everettraven Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Atomic means the list is always replaced and you cannot do server side apply operations that would just add one entry to the list as an example.

The reason I asked is because if you don't expect that use case to ever exist, atomic is probably fine, but in general we recommend allowing common SSA operations where possible.

I'd suggest map as the list type here with the uniqueness keyed on audience.

@chiragkyal chiragkyal Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, updated.

// +kubebuilder:validation:MaxItems=10
Audiences []SecretsStoreTokenRequest `json:"audiences,omitempty"`

@everettraven everettraven Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like audiences: [] would be a supported value here, so please add a minimum length of 1 constraint here.

@everettraven everettraven Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, are there any other constraints that should be enforced here?

For example, can I have duplicate audience entries?

@chiragkyal chiragkyal Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like audiences: [] would be a supported value here, so please add a minimum length of 1 constraint here.

An empty list will indicate the operator to clear it from CSIDriver. So I think it should be supported.

For example, can I have duplicate audience entries?

We can enforce uniqueness on the audience string. Does the following CEL works fine?

	// +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, y.audience == x.audience))",message="each audience must be unique"

@everettraven everettraven Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. I think that audiences: [] sounds like a reasonable use case here. In that case, you'll want this to be a pointer to a slice so that omitempty omits nil but not [].

@chiragkyal chiragkyal Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. Addressed.

@chiragkyal chiragkyal Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the linter is suggesting to revert back to a non-pointer type

-	Audiences *[]SecretsStoreTokenRequest `json:"audiences,omitempty"`
+	Audiences []SecretsStoreTokenRequest `json:"audiences,omitempty"`

@everettraven everettraven Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, if you add a minimum items of 0 here it should pick up that the zero-value is allowed and allow the pointer.

@chiragkyal chiragkyal Jun 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. Updated.

}

// SecretRotationType determines the secret rotation behavior for the
// Secrets Store CSI driver.
// +kubebuilder:validation:Enum=None;Custom
type SecretRotationType string

const (
// SecretRotationNone disables automatic secret rotation. Secrets are only
// fetched at initial pod mount time.
SecretRotationNone SecretRotationType = "None"

// SecretRotationCustom enables automatic secret rotation with the
// configuration specified in the custom field.
SecretRotationCustom SecretRotationType = "Custom"
)

// SecretsStoreSecretRotation configures the automatic secret rotation behavior
// for the Secrets Store CSI driver.
// +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'Custom' ? has(self.custom) : !has(self.custom)",message="custom must be set when type is 'Custom', and must not be set otherwise"
// +union
type SecretsStoreSecretRotation struct {
Comment thread
everettraven marked this conversation as resolved.
// type determines the secret rotation behavior.
// When "None", secret rotation is disabled and secrets are only fetched at
// initial pod mount time.
// When "Custom", secret rotation is enabled with the configuration specified
// in the custom field.
// +unionDiscriminator
// +required
Type SecretRotationType `json:"type,omitempty"`

// custom holds the custom rotation configuration.
// Only valid when type is "Custom".
// +optional
Custom *CustomSecretRotation `json:"custom,omitempty"`
Comment thread
chiragkyal marked this conversation as resolved.
Outdated
}

// CustomSecretRotation holds configuration for custom secret rotation behavior.
type CustomSecretRotation struct {
// rotationPollIntervalSeconds is the minimum time in seconds between secret
// rotation attempts. The driver skips provider calls if less than this interval
// has elapsed since the last successful rotation.
// Must be at least 1 second and no more than 31560000 seconds (~1 year).
// When omitted, this means no opinion and the platform is left to choose a
// reasonable default, which is subject to change over time.
// +kubebuilder:validation:Minimum=1
// +kubebuilder:validation:Maximum=31560000
// +optional
RotationPollIntervalSeconds *int32 `json:"rotationPollIntervalSeconds,omitempty"`

@everettraven everettraven Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

@chiragkyal chiragkyal Jun 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@everettraven everettraven Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things here:

The +default=120 ensures 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.

@chiragkyal chiragkyal Jun 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on all points:

  • Removed +default=120. Since we are prefering unset as "no opinion" semantic from user, the operator can inject the defaults internally.
  • Added Minimum=1 since setting 0 does not make sense to me.
  • Added Maximum=31560000 (~1 year) to start with.

@mytreya-rh FYI

@chiragkyal chiragkyal Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the linter is suggesting to use a non-pointer type which I think is fair given the min property 1 and the lower<>upper bounds.

-	RotationPollIntervalSeconds *int32 `json:"rotationPollIntervalSeconds,omitempty"`
+	RotationPollIntervalSeconds int32 `json:"rotationPollIntervalSeconds,omitempty"`

@chiragkyal chiragkyal Jun 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the confirmation. Updated.

}

// SecretsStoreTokenRequest specifies a service account token audience configuration
// for workload identity federation (WIF) with the Secrets Store CSI driver.
type SecretsStoreTokenRequest struct {
// audience is the intended audience of the service account token.
// An empty string means the issued token will use the kube-apiserver's default APIAudiences.
// +kubebuilder:validation:MinLength=0
// +kubebuilder:validation:MaxLength=253
// +required
Audience *string `json:"audience,omitempty"`

// 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) and no more than 315360000 seconds (~10 years).
// +kubebuilder:validation:Minimum=600
// +kubebuilder:validation:Maximum=315360000
// +optional
ExpirationSeconds int64 `json:"expirationSeconds,omitempty"`

@everettraven everettraven Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: With the current bounds, could you get away with this being an int32 instead of an int64?

@chiragkyal chiragkyal Jun 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. Updated

}

// ClusterCSIDriverStatus is the observed status of CSI driver operator
type ClusterCSIDriverStatus struct {
OperatorStatus `json:",inline"`
Expand Down
Loading