feat: allow providers to have the driver write files #481

tam7t · 2021-03-17T21:40:01Z

What this PR does / why we need it:

Expands the provider<->driver interface to allow the providers to have the driver write all files to the mount.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Fixes #460

Special notes for your reviewer:

k8s-ci-robot · 2021-03-17T21:40:03Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tam7t · 2021-03-18T14:44:04Z

/retest

tam7t · 2021-03-18T17:19:13Z

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

aramase · 2021-03-18T17:57:44Z

vault e2e tests should be fixed with #483

aramase · 2021-03-19T16:41:11Z

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

pkg/util/fileutil/writer_test.go

pkg/util/tmpdir/tmpdir.go

pkg/secrets-store/nodeserver_test.go

pkg/util/fileutil/writer.go

aramase · 2021-03-19T20:31:57Z

pkg/util/fileutil/writer.go

+)
+
+const (
+	maxFileNameLength = 255


For Azure Key vault, the max object name length is 128 and I assume this is a reasonable limit for the Google Secrets Manager as well. Is there a restriction for Hashicorp vault? Since this limit is being imposed now we should make sure we aren't breaking any exist cases.

It does not seem like the vault provider includes an restrictions on path lengths: https://github.com/hashicorp/vault-csi-provider/blob/20173a0f069739c2e9236c3b23bbe98a4af2c3ba/internal/provider/provider.go#L254

The GCP plugin has a length of 253 since IsConfigMapKey is currently used:
https://github.com/kubernetes/apimachinery/blob/2456ebdaba229616fab2161a615148884b46644b/pkg/util/validation/validation.go#L424

@tomhjp Do you see anything breaking with imposing this limit on the secret name length for the vault provider?

There's no explicit limit currently but I'm ok with introducing this limit 👍

aramase · 2021-03-19T20:36:35Z

pkg/util/fileutil/writer.go

+	if targetPath == "" {
+		return fmt.Errorf("invalid path: must not be empty: %q", targetPath)
+	}
+	if path.IsAbs(targetPath) {


We should use filepath so this is valid for windows too

this was from https://github.com/kubernetes/kubernetes/blob/9c4ffa7e11bb2d0beb25ef1cac6e0699acfc350a/pkg/volume/util/atomic_writer.go#L256

Looking at path.IsAbs and filepath.IsAbs I'm fairly certain that their implementations are equivalent, so I'll move to filepath as that seems more semantically correct. Think we should propose a change to automic_writer.go too?

Yeah, we should propose a change to atomic_writer.go. We can open a PR and get feedback from the community.

pkg/util/fileutil/writer_test.go

aramase · 2021-03-19T20:46:17Z

provider/v1alpha1/service.proto

+    string path = 1;
+    // The mode bits used to set permissions on this file.
+    // Must be a decimal value between 0 and 511.
+    int32 mode = 2;


Today, the mode is sent to the provider from the driver. Having this will make it extensible in the future if we decide to support user configuration for mode.

Ack. My initial thoughts here are that we should deprecate permission in MountRequest and that permissions should be specified in the provider specific portion of the SecretProviderClass yaml at the same spot that secrets->files are mapped.

aramase · 2021-03-19T20:46:32Z

provider/v1alpha1/service.proto

+
+// File holds secret file contents and location in the mount path to write the
+// file.
+message File {


Could we also add a comment with the limitations on the size of the payload?

Ah, i completely forgot about that. Do you think I should also add a configurable message size flag to this PR?

I'm assuming bigger size than default supported would mean we would need to use grpc stream? I would be open to keeping the default with the limitation documented. If there are users with additional size, then we can add an option to configure. WDYT?

I updated the repeated File files = 3 comment but we can just add a flag to control the MaxCallRecvMsgSize option when creating the plugin grpc client. This would require config/deploy from the user but would not require switching to grpc streaming. I believe this was what @tomhjp preferred in the design doc

tam7t · 2021-03-23T17:34:11Z

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

aramase · 2021-03-23T17:44:46Z

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

k8s-ci-robot · 2021-03-24T02:28:43Z

@tam7t: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
release-secrets-store-csi-driver-e2e-gcp	`171ef9a`	link	`/test release-secrets-store-csi-driver-e2e-gcp`
release-secrets-store-csi-driver-e2e-vault	`171ef9a`	link	`/test release-secrets-store-csi-driver-e2e-vault`
release-secrets-store-csi-driver-e2e-azure	`171ef9a`	link	`/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/test-infra repository. I understand the commands that are listed here.

pkg/secrets-store/provider_client.go

ritazh · 2021-03-25T01:35:43Z

pkg/secrets-store/provider_client_test.go

+			},
+		},
+		{
+			name:       "provider response with multiple files",


maybe add a test for 0 file returned

The "provider successful response" case covers this, but I've updated it to be explicit and this gave me the idea to use filepath.walk + go-cmp for better file comparisons

ritazh · 2021-03-25T01:54:27Z

provider/v1alpha1/service.proto

@@ -59,6 +61,27 @@ message MountRequest {
 message MountResponse {
    repeated ObjectVersion object_version = 1;
    Error error = 2;
+    // files contains the entire mount volume filesystem.
+    //
+    // The total size of all files should not exceed 1MiB or syncing to


Do we want to validate the file content size if this is a known limit?

We could add it for the 1 MiB case but over 4MiB the RPC will fail. I've added a klog to MountContent for when the proto is > 1MiB.

aramase · 2021-03-26T18:54:12Z

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

ritazh · 2021-03-26T20:09:20Z

/lgtm

aramase

/approve

k8s-ci-robot · 2021-03-26T21:14:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase, tam7t

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:

~~OWNERS~~ [aramase]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 17, 2021

k8s-ci-robot requested review from aramase and ritazh March 17, 2021 21:40

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 17, 2021

tam7t marked this pull request as ready for review March 17, 2021 21:43

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 Mar 17, 2021

tam7t commented Mar 19, 2021

View reviewed changes

pkg/util/fileutil/writer_test.go Outdated Show resolved Hide resolved

aramase reviewed Mar 19, 2021

View reviewed changes

ritazh reviewed Mar 25, 2021

View reviewed changes

pkg/secrets-store/provider_client.go Show resolved Hide resolved

ritazh reviewed Mar 25, 2021

View reviewed changes

aramase added this to the v0.0.21 milestone Mar 25, 2021

tam7t force-pushed the tam7t/obj-write-spike branch from 171ef9a to b7a9369 Compare March 26, 2021 00:11

feat: allow providers to have the driver write files

bc3ec81

tam7t force-pushed the tam7t/obj-write-spike branch from b7a9369 to bc3ec81 Compare March 26, 2021 01:25

k8s-ci-robot assigned ritazh Mar 26, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 26, 2021

aramase reviewed Mar 26, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2021

k8s-ci-robot merged commit ab0b325 into kubernetes-sigs:master Mar 26, 2021

tam7t deleted the tam7t/obj-write-spike branch March 29, 2021 13:53

This was referenced Apr 7, 2021

Atomic file updates #500

Closed

feat: allow maxCallRecvMsgSize to be increased for large secret mounts #512

Merged

tam7t mentioned this pull request Apr 28, 2021

feat: use the atomic file writer #520

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow providers to have the driver write files #481

feat: allow providers to have the driver write files #481

tam7t commented Mar 17, 2021

k8s-ci-robot commented Mar 17, 2021

tam7t commented Mar 18, 2021

tam7t commented Mar 18, 2021

aramase commented Mar 18, 2021 •

edited

Loading

aramase commented Mar 19, 2021

aramase Mar 19, 2021

tam7t Mar 23, 2021

aramase Mar 23, 2021 •

edited

Loading

tomhjp Mar 26, 2021

aramase Mar 19, 2021

tam7t Mar 22, 2021

aramase Mar 24, 2021

aramase Mar 19, 2021

tam7t Mar 23, 2021

aramase Mar 19, 2021

tam7t Mar 23, 2021

aramase Mar 24, 2021 •

edited

Loading

tam7t Mar 24, 2021

tam7t commented Mar 23, 2021

aramase commented Mar 23, 2021

k8s-ci-robot commented Mar 24, 2021

ritazh Mar 25, 2021

tam7t Mar 25, 2021

ritazh Mar 25, 2021

tam7t Mar 26, 2021

aramase commented Mar 26, 2021

ritazh commented Mar 26, 2021

aramase left a comment

k8s-ci-robot commented Mar 26, 2021

feat: allow providers to have the driver write files #481

feat: allow providers to have the driver write files #481

Conversation

tam7t commented Mar 17, 2021

k8s-ci-robot commented Mar 17, 2021

tam7t commented Mar 18, 2021

tam7t commented Mar 18, 2021

aramase commented Mar 18, 2021 • edited Loading

aramase commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aramase Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aramase Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tam7t commented Mar 23, 2021

aramase commented Mar 23, 2021

k8s-ci-robot commented Mar 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aramase commented Mar 26, 2021

ritazh commented Mar 26, 2021

aramase left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 26, 2021

aramase commented Mar 18, 2021 •

edited

Loading

aramase Mar 23, 2021 •

edited

Loading

aramase Mar 24, 2021 •

edited

Loading