feat: Use RequiresRepublish for secret rotation #1622

dargudear-google · 2024-09-01T06:55:30Z

Remove the rotation controller and rely exclusively on RequiresRepublish for secret rotation.

k8s-ci-robot · 2024-09-01T06:55:39Z

Hi @dargudear-google. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 kubernetes-sigs/prow repository.

amitmodak

Approach broadly LGTM! IIUC, once this PR is submitted, auto-rotation won't work for k8s clusters < 1.21. What is the process to announce and manage this breaking change?

dargudear-google · 2024-09-11T06:28:36Z

Approach broadly LGTM! IIUC, once this PR is submitted, auto-rotation won't work for k8s clusters < 1.21. What is the process to announce and manage this breaking change?

Discussed in the last community call that we should publish that "For clusters < 1.21, please use v.1.4.5 or earlier."

codecov-commenter · 2024-09-19T16:13:34Z

Codecov Report

Attention: Patch coverage is 32.55814% with 58 lines in your changes missing coverage. Please review.

Project coverage is 32.07%. Comparing base (87f51ec) to head (76e6600).
Report is 195 commits behind head on main.

Files with missing lines	Patch %	Lines
...rollers/secretproviderclasspodstatus_controller.go	13.15%	31 Missing and 2 partials ⚠️
pkg/secrets-store/nodeserver.go	58.06%	10 Missing and 3 partials ⚠️
pkg/secrets-store/secrets-store.go	20.00%	8 Missing ⚠️
pkg/secrets-store/utils.go	50.00%	2 Missing and 1 partial ⚠️
cmd/secrets-store-csi-driver/main.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1622      +/-   ##
==========================================
- Coverage   35.83%   32.07%   -3.77%     
==========================================
  Files          63       57       -6     
  Lines        3759     3838      +79     
==========================================
- Hits         1347     1231     -116     
- Misses       2268     2501     +233     
+ Partials      144      106      -38

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

nilekhc · 2024-10-03T16:05:20Z

/ok-to-test

nilekhc

First pass.

In bats tests, where sleep duration/timeout is increased, could you re-evaluate if it's necessary? If yes then could you add a note to make sure we have context?

pkg/secrets-store/secrets-store.go

pkg/secrets-store/nodeserver.go

test/bats/e2e-provider.bats

test/bats/aws.bats

test/bats/e2e-provider.bats

dargudear-google · 2025-01-13T01:36:03Z

/test pull-secrets-store-csi-driver-e2e-gcp

dargudear-google · 2025-01-15T09:55:02Z

Tested the newly added e2e test for GCP provider. @aramase Can you please take a look?

micahhausler · 2025-04-08T17:57:45Z

@aramase are you still planning on reviewing this PR? I'd like to see this functionality get in

aramase · 2025-04-08T18:00:09Z

@aramase are you still planning on reviewing this PR? I'd like to see this functionality get in

yes, we're currently working on the next release for the driver. I'll review this week after the release is complete.

k8s-ci-robot · 2025-04-10T07:51:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dargudear-google, jainsuyogj
Once this PR has been reviewed and has the lgtm label, please ask for approval from aramase. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aramase

Another pass.

Makefile

controllers/secretproviderclasspodstatus_controller.go

pkg/secrets-store/nodeserver.go

aramase · 2025-04-16T22:12:10Z

pkg/secrets-store/nodeserver.go

+		if isRemountRequest {
+			// Mask error until fix available for https://github.com/kubernetes/kubernetes/issues/121271
+			return &csi.NodePublishVolumeResponse{}, nil
+		}


In the remount request scenario, if we reach this point, it means the rotation succeeded (the files were updated in the mount). However, if we failed to update the SecretProviderClassPodStatus, it's inconsistent—the SPCPS still reflects the old versions of the objects, while the files in the mount are newer.

This inconsistency is temporary and should be gone once kubernetes/kubernetes#121271 get fixed.

k8s-ci-robot · 2025-05-14T19:14:14Z

@dargudear-google: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
release-secrets-store-csi-driver-e2e-azure	`50ac633`	link	true	`/test release-secrets-store-csi-driver-e2e-azure`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

dargudear-google · 2025-05-14T19:29:13Z

/retest-required

ThirdEyeSqueegee · 2025-05-28T19:40:11Z

Hi @aramase, are y'all still expecting to include this and #1755 in the next release? We'd love to see both these features make it into the driver to support our use case.

aramase · 2025-06-11T22:54:37Z

Hi @aramase, are y'all still expecting to include this and #1755 in the next release? We'd love to see both these features make it into the driver to support our use case.

Yes, this will need to be merged before we can merge #1755. We want to include both changes in the next minor release. I still need to review the recent updates. I'm currently in the middle of the Kubernetes v1.34 enhancements freeze but will take a look soon.

micahhausler

I think my main suggestion would be to read the CSIDriver into a local cache and have NodePubilshVolume() read the requiresRepublish value from that local cache. That way users can avoid a CSI Driver restart if they change the value

micahhausler · 2025-06-16T19:00:33Z

manifest_staging/charts/secrets-store-csi-driver/templates/csidriver.yaml

  tokenRequests:
    {{- toYaml .Values.tokenRequests | nindent 2 }}
+  requiresRepublish: {{ .Values.requiresRepublish }}


Can you remove the hard-coded entry above this since its conditional here?

micahhausler · 2025-06-16T19:01:18Z

manifest_staging/charts/secrets-store-csi-driver/values.yaml

@@ -239,6 +239,10 @@ tokenRequests: []
 # - audience: aud1
 # - audience: aud2

+# To set the requiresRepublish which can be used to refresh the mounted secret periodically
+# refer to https://kubernetes-csi.github.io/docs/token-requests.html for more details.
+# Supported only for Kubernetes v1.20+


K8s 1.20 has long-since been deprecated. Can we scrub references to it now?

@aramase WDYT?

micahhausler · 2025-06-16T19:07:47Z

pkg/secrets-store/nodeserver.go

@@ -190,14 +201,7 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 	// and send it to the provider in the parameters.


Can you remove this comment block? its no longer needed

micahhausler · 2025-06-16T19:15:00Z

pkg/secrets-store/secrets-store.go

+// rotationConfig stores the information required to rotate the secrets.
+type rotationConfig struct {
+	enabled              bool
+	rotationPollInterval time.Duration


Is this really a poll interval? it seems more like a cache time for NodePublishVolume() to not act any sooner than this duration.

micahhausler · 2025-06-16T19:33:03Z

pkg/secrets-store/nodeserver.go

@@ -47,7 +44,7 @@ type nodeServer struct {
 	// This should be used sparingly and only when the client does not fit the use case.
 	reader          client.Reader
 	providerClients *PluginClientBuilder
-	tokenClient     *k8s.TokenClient
+	rotationConfig  *rotationConfig


What do you think of instead of having a rotationConfig that isn't thread-safe or configurable, using a (wrapped) K8s client to dynamically read this value off the CSIDriver?

Right now, if a user changes requiresRepublish on their driver configuration, they'll have to restart every CSI driver pod to update if this is enabled, because you're setting this value once in main.go.

In my PR I created a CSIDriver client that would just watch the one CSIDriver resource, and NodePublishVolume() would read from the local cache.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 1, 2024

k8s-ci-robot requested review from nilekhc and tam7t September 1, 2024 06:55

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 1, 2024

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 1, 2024

dargudear-google changed the title ~~Use in secret rotation~~ Use RequiresRepublish in secret rotation. Sep 7, 2024

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 7, 2024

dargudear-google marked this pull request as ready for review September 10, 2024 13:30

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 10, 2024

k8s-ci-robot requested a review from ritazh September 10, 2024 13:30

amitmodak reviewed Sep 11, 2024

View reviewed changes

dargudear-google requested a review from amitmodak September 16, 2024 12:48

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 3, 2024

dargudear-google force-pushed the rotation branch 7 times, most recently from aa6ffcb to 8d332b7 Compare October 7, 2024 09:20

nilekhc requested changes Oct 7, 2024

View reviewed changes

dargudear-google force-pushed the rotation branch from 9ca55a9 to a7f4796 Compare December 20, 2024 03:53

Update dependencies

23e8f92

dargudear-google force-pushed the rotation branch from acf1519 to 23e8f92 Compare December 20, 2024 04:46

Do not return error if it is remount request

5f93f09

enj mentioned this pull request Feb 20, 2025

Removed TokenClient and use CSI token propagation #1749

Open

3 tasks

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2025

Merge branch 'main' into rotation

935b2e8

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2025

jainsuyogj approved these changes Apr 10, 2025

View reviewed changes

aramase requested changes Apr 16, 2025

View reviewed changes

enj moved this to Subprojects - Needs Triage in SIG Auth Apr 29, 2025

enj added this to SIG Auth Apr 29, 2025

aramase moved this from Subprojects - Needs Triage to Changes Requested in SIG Auth Apr 29, 2025

aramase mentioned this pull request May 8, 2025

Auto rotation of mounted contents and synced Kubernetes Secrets - GA release #1805

Closed

dargudear-google and others added 2 commits May 14, 2025 23:18

Merge branch 'kubernetes-sigs:main' into rotation

a1f19b7

Fix the comments, logs and delete unused configs

4df9a6d

dargudear-google requested a review from aramase May 14, 2025 18:29

Fix rotation enable check

76e6600

mytreya-rh mentioned this pull request Jun 9, 2025

feat: Allow Fileownership change through FSGroup and VOLUME_MOUNT_GROUP #1841

Open

3 tasks

ThirdEyeSqueegee mentioned this pull request Jun 16, 2025

feat: CSI Token Requests aws/secrets-store-csi-driver-provider-aws#474

Draft

micahhausler suggested changes Jun 16, 2025

View reviewed changes

		@@ -190,14 +201,7 @@ func (ns nodeServer) NodePublishVolume(ctx context.Context, req csi.NodePublis
		// and send it to the provider in the parameters.

feat: Use RequiresRepublish for secret rotation #1622

Are you sure you want to change the base?

feat: Use RequiresRepublish for secret rotation #1622

Uh oh!

Conversation

dargudear-google commented Sep 1, 2024

Uh oh!

k8s-ci-robot commented Sep 1, 2024

Uh oh!

amitmodak left a comment

Choose a reason for hiding this comment

Uh oh!

dargudear-google commented Sep 11, 2024

Uh oh!

codecov-commenter commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nilekhc commented Oct 3, 2024

Uh oh!

nilekhc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dargudear-google commented Jan 13, 2025

Uh oh!

dargudear-google commented Jan 15, 2025

Uh oh!

micahhausler commented Apr 8, 2025

Uh oh!

aramase commented Apr 8, 2025

Uh oh!

k8s-ci-robot commented Apr 10, 2025

Uh oh!

aramase left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dargudear-google commented May 14, 2025

Uh oh!

ThirdEyeSqueegee commented May 28, 2025

Uh oh!

aramase commented Jun 11, 2025

Uh oh!

micahhausler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Sep 19, 2024 •

edited

Loading

k8s-ci-robot commented May 14, 2025 •

edited

Loading