Skip to content
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.

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

Remove Bucket access check from node csi drvier #255

Merged
merged 1 commit into from
May 14, 2024

Conversation

saikat-royc
Copy link
Collaborator

@saikat-royc saikat-royc commented May 13, 2024

Issues:

  1. CSI node driver was initializing the storage client on every node publish. This is an expensive call which does a 3-way handshake to k8s API server, STS server, and GCP IAM.
  2. GCSFUse already validates the bucket access during mount init. This makes the CSI node driver checks redundant.

Fix:
remove the bucket access check and the storage client init on every node publish call.

Tests:

  1. manually tested on 1.29 cluster, 1.29(master), 1.28(nodepool), success and failure scenarios..

Verified that gcsfuse denies mount request of the SA does not have the necessary permissions on the bucket

SetUpBucket: Error in iterating through objects: Get \"https://storage.googleapis.com/storage/v1/b/test-csi-fix/o?alt=json&delimiter=&endOffset=&includeFoldersAsPrefixes=false&includeTrailingDelimiter=false&matchGlob=&maxResults=1&pageToken=&prefix=&prettyPrint=false&projection=full&startOffset=&versions=false\": metadata: GCE metadata \"instance/service-accounts/default/token?scopes=https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fdevstorage.full_control\" not defined\n"}
Error while mounting gcsfuse: mountWithArgs: mountWithStorageHandle: fs.NewServer: create file system: SetUpBucket: Error in iterating through objects: Get "https://storage.googleapis.com/storage/v1/b/test-csi-fix/o?alt=json&delimiter=&endOffset=&includeFoldersAsPrefixes=false&includeTrailingDelimiter=false&matchGlob=&maxResults=1&pageToken=&prefix=&prettyPrint=false&projection=full&startOffset=&versions=false": metadata: GCE metadata "instance/service-accounts/default/token?scopes=https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fdevstorage.full_control" not defined
E0510 22:44:39.602449       1 logger.go:60] gcsfuse exited with error: exit status 1

@msau42
Copy link
Collaborator

msau42 commented May 13, 2024

/lgtm

@tyuchn tyuchn merged commit 94df091 into GoogleCloudPlatform:main May 14, 2024
8 checks passed
saikat-royc added a commit to saikat-royc/gcs-fuse-csi-driver that referenced this pull request May 24, 2024
1. Add support for PV volume attribute  'skipCSIBucketAccessCheck' to
   skip CSI bucket access checks
2. Restore the bucket access check logic that was removed as part of GoogleCloudPlatform#255
saikat-royc added a commit to saikat-royc/gcs-fuse-csi-driver that referenced this pull request May 24, 2024
1. Add support for PV volume attribute  'skipCSIBucketAccessCheck' to
   skip CSI bucket access checks
2. Restore the bucket access check logic that was removed as part of GoogleCloudPlatform#255
3. Restore original logic for failed mount e2e test
saikat-royc added a commit to saikat-royc/gcs-fuse-csi-driver that referenced this pull request May 29, 2024
1. Add support for PV volume attribute  'skipCSIBucketAccessCheck' to
   skip CSI bucket access checks
2. Restore the bucket access check logic that was removed as part of GoogleCloudPlatform#255
3. Restore original logic for failed mount e2e test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants