Add tags to EFS resources. Update EFS Utils #309

kbasv · 2021-01-19T06:09:15Z

What is this PR about? / Why do we need it?

Allow adding tags to EFS resources to configure IAM policy condition based on tags.
Add default tags and provide a command line option to provide more tags.
Update RBAC permissions and efs-utils version.
Fixes Dynamic PV tests flaky #303

What testing is done?

Tested against my EKS cluster.

Ran make test-e2e on a fresh ec2 instance

k8s-ci-robot · 2021-01-19T06:09:23Z

Hi @kbasv. 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/test-infra repository.

wongma7 · 2021-01-26T22:28:14Z

/ok-to-test

kbasv · 2021-01-27T17:39:00Z

/retest

kbasv · 2021-01-27T20:38:20Z

/retest

kbasv · 2021-01-28T02:24:10Z

/retest

wongma7 · 2021-01-28T22:47:36Z

/retest

cmd/main.go

pkg/driver/efs_watch_dog.go

test/e2e/e2e.go

kbasv · 2021-02-18T23:29:05Z

@wongma7 dynamic provisioning e2e tests passes now

wongma7 · 2021-03-02T00:28:31Z

/lgtm
/approve

k8s-ci-robot · 2021-03-02T00:28:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kbasv, wongma7

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~~ [wongma7]

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

daisuke-yoshimoto · 2021-03-07T08:18:56Z

@wongma7 @nckturner @kbasv
Is this PR not yet merged?

daisuke-yoshimoto · 2021-03-10T00:58:04Z

@wongma7 @nckturner @kbasv
Is there any update on this?

wongma7 · 2021-03-10T01:13:38Z

coveralls is blocking. I guess it has a point this time, it wouldn't hurt for parsing functions to have some tests

wongma7 · 2021-03-10T01:16:02Z

pkg/driver/driver.go

+func parseTagsFromStr(tagStr string) map[string]string {
+	defer func() {
+		if r := recover(); r != nil {
+			klog.Errorf("Failed to parse input tag string: %v", tagStr)


could you add a unit test for this, using at least the same example in the command help text.

Instead of panicking and recovering given invalid input, let's check the length of p, this can be a test case as well.

I can add a test here.

The panicking and recovering is when there is a typo in the input or the key value pair is not of the form key:value. For example: key-value key;value

Checking for len would do the same thing but I guess it is a matter of style so it's fine. If len(p) is not 2, then the user must have made a typo like key-value or key:value:key etc.

I was thinking of just unit tests of parseTagsFromStr rather than "integration" test of all of CreateVolume but what you have added is fine

Also WDYT about parsing at startup and fast failing instead of at createvolume time?

With current implementation, driver will startup and mount/unmount will still work but then all createvolumes will fail and the user will have to do debugging. Whereas if we fast fail they will presumably see the error message printed by the plugin

edit: never mind, I think it could be better to fail at createvolume time. Chasing logs of a crashlooping pod is not fun.

…n based on tags. * Add default tags and provide a command line option to provide more tags * Update permissions and efs-utils version

jqmichael · 2021-03-10T17:56:36Z

test/e2e/e2e.go

+		// mount access point
+		command := exec.Command("/bin/sh", "-c", "mount", "-t", "efs", "-o", "tls,accesspoint="+accessPointId, FileSystemId, target)
+		if err = command.Run(); err != nil {
+			framework.ExpectNoError(err, "Failed to mount using access point")
+		}


Is this a test or setup step? If it's a test, shouldn't it be moved to a dedicated test method, rather than running in a BeforeSuite? If it's a setup, why did we delete the access point afterwards?

This is a setup step to bootstrap access points

do we expect users to do this? Do we need to document the need to bootstrap access points anywhere?

wongma7 · 2021-03-10T18:40:29Z

One last question: what do yo uthink about making tags a storageclass parameter instead of a command line flag?

It is more flexible storageclass a could have its volumes tagged a certain way, b another way, and as a user I can more easijly audit both.

e: we can always add this later. I think for now, making it a flag satisfies most usecases

kbasv · 2021-03-10T19:18:32Z

I like the idea of tags in the storage class parameters. We can eventually move it as a storage class parameter or we can do it now. I'm okay with either way

wongma7 · 2021-03-11T23:26:07Z

/lgtm

I am really wary of the e2e test changes, I think we need to document, but in interest of gathering data let's merge it and see if it helps with the test flakiness

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 Jan 19, 2021

k8s-ci-robot requested review from nckturner and wongma7 January 19, 2021 06:09

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 19, 2021

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 Jan 26, 2021

kbasv force-pushed the dpUpdate branch 2 times, most recently from 26e60d0 to 08368aa Compare January 27, 2021 17:32

k8s-ci-robot added 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 Jan 27, 2021

wongma7 reviewed Jan 28, 2021

View reviewed changes

cmd/main.go Show resolved Hide resolved

wongma7 reviewed Jan 28, 2021

View reviewed changes

pkg/driver/efs_watch_dog.go Show resolved Hide resolved

wongma7 reviewed Jan 28, 2021

View reviewed changes

test/e2e/e2e.go Show resolved Hide resolved

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

kbasv force-pushed the dpUpdate branch 2 times, most recently from 60ab06a to e8d8e1d Compare February 18, 2021 22:29

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

k8s-ci-robot assigned wongma7 Mar 2, 2021

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

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

wongma7 reviewed Mar 10, 2021

View reviewed changes

edanc mentioned this pull request Mar 10, 2021

Add PVC and PV metadata as tags to EFS access points #349

Closed

kbasv force-pushed the dpUpdate branch from e8d8e1d to bb83c3d Compare March 10, 2021 03:41

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

* Allow adding tags to EFS resources to configure IAM policy conditio…

75a902e

…n based on tags. * Add default tags and provide a command line option to provide more tags * Update permissions and efs-utils version

kbasv force-pushed the dpUpdate branch from bb83c3d to 75a902e Compare March 10, 2021 03:53

jqmichael reviewed Mar 10, 2021

View reviewed changes

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

k8s-ci-robot merged commit 3f7f71c into kubernetes-sigs:master Mar 12, 2021

kbasv mentioned this pull request Mar 17, 2021

Access Denied when using Access Points #347

Closed

kbasv mentioned this pull request Apr 23, 2021

REQUEST: New membership for kbasv kubernetes/org#2656

Closed

kbasv deleted the dpUpdate branch May 12, 2021 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tags to EFS resources. Update EFS Utils #309

Add tags to EFS resources. Update EFS Utils #309

kbasv commented Jan 19, 2021 •

edited

Loading

k8s-ci-robot commented Jan 19, 2021

wongma7 commented Jan 26, 2021

kbasv commented Jan 27, 2021

kbasv commented Jan 27, 2021

kbasv commented Jan 28, 2021

wongma7 commented Jan 28, 2021

kbasv commented Feb 18, 2021

wongma7 commented Mar 2, 2021

k8s-ci-robot commented Mar 2, 2021

daisuke-yoshimoto commented Mar 7, 2021

daisuke-yoshimoto commented Mar 10, 2021

wongma7 commented Mar 10, 2021

wongma7 Mar 10, 2021

kbasv Mar 10, 2021

wongma7 Mar 10, 2021

wongma7 Mar 10, 2021 •

edited

Loading

jqmichael Mar 10, 2021

kbasv Mar 10, 2021

wongma7 Mar 10, 2021

wongma7 commented Mar 10, 2021 •

edited

Loading

kbasv commented Mar 10, 2021

wongma7 commented Mar 11, 2021

Add tags to EFS resources. Update EFS Utils #309

Add tags to EFS resources. Update EFS Utils #309

Conversation

kbasv commented Jan 19, 2021 • edited Loading

k8s-ci-robot commented Jan 19, 2021

wongma7 commented Jan 26, 2021

kbasv commented Jan 27, 2021

kbasv commented Jan 27, 2021

kbasv commented Jan 28, 2021

wongma7 commented Jan 28, 2021

kbasv commented Feb 18, 2021

wongma7 commented Mar 2, 2021

k8s-ci-robot commented Mar 2, 2021

daisuke-yoshimoto commented Mar 7, 2021

daisuke-yoshimoto commented Mar 10, 2021

wongma7 commented Mar 10, 2021

wongma7 Mar 10, 2021

Choose a reason for hiding this comment

kbasv Mar 10, 2021

Choose a reason for hiding this comment

wongma7 Mar 10, 2021

Choose a reason for hiding this comment

wongma7 Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

jqmichael Mar 10, 2021

Choose a reason for hiding this comment

kbasv Mar 10, 2021

Choose a reason for hiding this comment

wongma7 Mar 10, 2021

Choose a reason for hiding this comment

wongma7 commented Mar 10, 2021 • edited Loading

kbasv commented Mar 10, 2021

wongma7 commented Mar 11, 2021

kbasv commented Jan 19, 2021 •

edited

Loading

wongma7 Mar 10, 2021 •

edited

Loading

wongma7 commented Mar 10, 2021 •

edited

Loading