plumb service account token down to csi driver #93130

zshihang · 2020-07-16T00:13:26Z

What type of PR is this?

/kind api-change

What this PR does / why we need it:
KEP: kubernetes/enhancements#1855
Issue: #86448
release: kubernetes/enhancements#2047

Does this PR introduce a user-facing change?:

this PR will introduce a feature gate CSIServiceAccountToken with two additional fields in `CSIDriverSpec`.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/pull/1855

k8s-ci-robot · 2020-07-16T00:13:34Z

Hi @zshihang. Thanks for your PR.

I'm waiting for a kubernetes 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/test-infra repository.

zshihang · 2020-07-16T00:15:38Z

/assign @msau42 @liggitt @mikedanese

fejta-bot · 2020-07-16T01:08:54Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

wongma7 · 2020-07-17T21:52:26Z

/ok-to-test

msau42 · 2020-11-10T20:15:19Z

/lgtm

zshihang · 2020-11-10T21:16:15Z

/retest

mikedanese · 2020-11-11T18:36:41Z

/lgtm

pkg/apis/storage/validation/validation.go

liggitt · 2020-11-06T02:19:13Z

pkg/kubelet/volumemanager/volume_manager.go

@@ -260,6 +260,11 @@ type volumeManager struct {
 func (vm *volumeManager) Run(sourcesReady config.SourcesReady, stopCh <-chan struct{}) {
 	defer runtime.HandleCrash()

+	if vm.kubeClient != nil {


curious why this had to move

not necessary here but the sooner to populate the csi driver lister cache before desire workload state populator starts the better. racing is still there tho. Michelle opened an issue to figure out the place to start WaitForCacheSync.

racing is still there though

what is the impact of losing the race? functional failure or longer startup time?

@zshihang and I discussed. The impact is NodePublish (mount) may be called without the token. The CSI driver is expected to fail if it expects one. The volume reconciler will retry NodePublish until successful.

pkg/printers/internalversion/printers.go

pkg/registry/storage/csidriver/strategy.go

liggitt · 2020-11-11T18:53:42Z

pkg/volume/plugins.go

@@ -1027,6 +1027,7 @@ func (pm *VolumePluginMgr) Run(stopCh <-chan struct{}) {
 		// start informer for CSIDriver
 		informerFactory := kletHost.GetInformerFactory()
 		informerFactory.Start(stopCh)
+		informerFactory.WaitForCacheSync(stopCh)


For this feature, if there isn't any negative effect from the informer being not in sync, then I'm ok with not waiting on it, and letting a retry resolve itself.

is this still outstanding? should WaitForCacheSync be removed?

pkg/volume/csi/csi_mounter.go

pkg/apis/storage/types.go

liggitt · 2020-11-11T18:59:48Z

pkg/volume/csi/csi_mounter.go

+	}
+
+	outputs := map[string]string{}
+	for _, tokenRequest := range csiDriver.Spec.TokenRequests {


how frequently is republish called? do we recreate all tokens on every republish call or do we reuse the previously requested one if it is still valid?

100ms between republish calls. the tokens are cached in kubelet. it is using the same handler as projected tokens.

does that mean we're calling the CSI driver every 100ms?

no because every reconciler loop takes some time and the reconciler loop calls republish.

kubernetes/pkg/kubelet/volumemanager/reconciler/reconciler.go

Lines 80 to 81 in 8be0a29

// loopSleepDuration - the amount of time the reconciler loop sleeps between

// successive executions

from what i observed from e2e test, it is about 2~3/second.

pkg/volume/csi/csi_mounter.go

pkg/volume/csi/csi_mounter_test.go

zshihang · 2020-11-11T23:51:01Z

For this feature, if there isn't any negative effect from the informer being not in sync, then I'm ok with not waiting on it, and letting a retry resolve itself.

is this still outstanding? should WaitForCacheSync be removed?

no, this will be called in a go routine. it is better to have it happen sooner. @msau42

liggitt · 2020-11-12T14:18:29Z

/approve

API and validation changes look good. I did not review the volume plugin changes.

Needs rebase, and storage reviewers have lgtm on remaining items.

k8s-ci-robot · 2020-11-12T17:27:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, msau42, zshihang

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~api/OWNERS~~ [liggitt]
~~pkg/apis/storage/OWNERS~~ [liggitt]
~~pkg/controller/volume/attachdetach/OWNERS~~ [liggitt,msau42]
~~pkg/controller/volume/persistentvolume/OWNERS~~ [liggitt,msau42]
~~pkg/features/OWNERS~~ [liggitt]
~~pkg/kubelet/volumemanager/OWNERS~~ [liggitt,msau42]
~~pkg/printers/OWNERS~~ [liggitt]
~~pkg/registry/OWNERS~~ [liggitt]
~~pkg/volume/OWNERS~~ [liggitt,msau42]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~test/e2e/framework/OWNERS~~ [liggitt,msau42]
~~test/e2e/storage/OWNERS~~ [liggitt,msau42]

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

zshihang · 2020-11-12T17:55:31Z

/retest

msau42 · 2020-11-12T17:59:28Z

/lgtm

zshihang · 2020-11-12T18:03:52Z

/retest

zshihang · 2020-11-12T19:37:35Z

/retest

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

k8s-ci-robot requested review from jingxu97 and saad-ali July 16, 2020 00:13

k8s-ci-robot assigned liggitt, mikedanese and msau42 Jul 16, 2020

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

zshihang force-pushed the master branch from c9f168a to 9250f82 Compare July 16, 2020 19:02

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

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 Jul 17, 2020

zshihang force-pushed the master branch from 9250f82 to 91f208c Compare July 22, 2020 06:41

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2020

liggitt added this to the v1.20 milestone Nov 11, 2020

liggitt reviewed Nov 11, 2020

View reviewed changes

zshihang force-pushed the master branch from 37f574e to d27c774 Compare November 11, 2020 22:33

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2020

liggitt reviewed Nov 11, 2020

View reviewed changes

pkg/volume/csi/csi_mounter_test.go Outdated Show resolved Hide resolved

liggitt reviewed Nov 11, 2020

View reviewed changes

pkg/volume/csi/csi_mounter_test.go Outdated Show resolved Hide resolved

zshihang force-pushed the master branch from d27c774 to 17d5449 Compare November 11, 2020 23:01

zshihang force-pushed the master branch from 17d5449 to e66c3f9 Compare November 12, 2020 01:13

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 12, 2020

plumb service account token down to csi driver

d2859cd

zshihang force-pushed the master branch from e66c3f9 to d2859cd Compare November 12, 2020 17:26

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

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2020

kikisdeliveryservice mentioned this pull request Nov 12, 2020

release issue for feature CSIServiceAccountToken kubernetes/enhancements#2047

Closed

20 tasks

k8s-ci-robot merged commit 55856ed into kubernetes:master Nov 12, 2020

github-actions bot mentioned this pull request Nov 18, 2020

Week Ending November 15, 2020 dev-obs/actus#273

Open

tam7t mentioned this pull request Nov 20, 2020

Develop a sustainable solution for using workload identity GoogleCloudPlatform/secrets-store-csi-driver-provider-gcp#13

Closed

liggitt moved this from Assigned to API review completed, 1.20 in API Reviews Nov 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plumb service account token down to csi driver #93130

plumb service account token down to csi driver #93130

zshihang commented Jul 16, 2020 •

edited

Loading

k8s-ci-robot commented Jul 16, 2020

zshihang commented Jul 16, 2020

fejta-bot commented Jul 16, 2020

wongma7 commented Jul 17, 2020

msau42 commented Nov 10, 2020

zshihang commented Nov 10, 2020

mikedanese commented Nov 11, 2020

liggitt Nov 6, 2020

zshihang Nov 11, 2020

liggitt Nov 12, 2020

msau42 Nov 12, 2020

liggitt Nov 11, 2020

liggitt Nov 11, 2020

zshihang Nov 11, 2020 •

edited

Loading

liggitt Nov 11, 2020

zshihang Nov 11, 2020 •

edited

Loading

zshihang commented Nov 11, 2020

liggitt commented Nov 12, 2020

k8s-ci-robot commented Nov 12, 2020

zshihang commented Nov 12, 2020

msau42 commented Nov 12, 2020

zshihang commented Nov 12, 2020

zshihang commented Nov 12, 2020

	// loopSleepDuration - the amount of time the reconciler loop sleeps between
	// successive executions

plumb service account token down to csi driver #93130

plumb service account token down to csi driver #93130

Conversation

zshihang commented Jul 16, 2020 • edited Loading

k8s-ci-robot commented Jul 16, 2020

zshihang commented Jul 16, 2020

fejta-bot commented Jul 16, 2020

wongma7 commented Jul 17, 2020

msau42 commented Nov 10, 2020

zshihang commented Nov 10, 2020

mikedanese commented Nov 11, 2020

liggitt Nov 6, 2020

Choose a reason for hiding this comment

zshihang Nov 11, 2020

Choose a reason for hiding this comment

liggitt Nov 12, 2020

Choose a reason for hiding this comment

msau42 Nov 12, 2020

Choose a reason for hiding this comment

liggitt Nov 11, 2020

Choose a reason for hiding this comment

liggitt Nov 11, 2020

Choose a reason for hiding this comment

zshihang Nov 11, 2020 • edited Loading

Choose a reason for hiding this comment

liggitt Nov 11, 2020

Choose a reason for hiding this comment

zshihang Nov 11, 2020 • edited Loading

Choose a reason for hiding this comment

zshihang commented Nov 11, 2020

liggitt commented Nov 12, 2020

k8s-ci-robot commented Nov 12, 2020

zshihang commented Nov 12, 2020

msau42 commented Nov 12, 2020

zshihang commented Nov 12, 2020

zshihang commented Nov 12, 2020

zshihang commented Jul 16, 2020 •

edited

Loading

zshihang Nov 11, 2020 •

edited

Loading

zshihang Nov 11, 2020 •

edited

Loading