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.

Sign up for GitHub

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

Jump to bottom

Should not pass in mount option of awscredsuri #755

Merged
merged 1 commit into from
Aug 17, 2022
Merged

Should not pass in mount option of awscredsuri #755

merged 1 commit into from
Aug 17, 2022

Conversation

Ashley-wenyizha
Copy link
Contributor

Is this a bug fix or adding new feature?

Should not pass in mount option of awscredsuri as there is no EKS customer should be using it, the uri is used together with ECS metadata endpoint. We should ignore that option if customer passed it in efs-csi-driver.

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

What testing is done?

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 11, 2022
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2022
@wangnyue
Copy link

/lgtm

@k8s-ci-robot
Copy link
Contributor

@wangnyue: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

yinsihaws
yinsihaws reviewed Aug 11, 2022
View reviewed changes
pkg/driver/node.go Outdated
if f == "awscredsuri" {
klog.Warning("awscredsuri is not accepted as a efs-csi-driver mount option")
return nil, status.Errorf(codes.InvalidArgument,
"Should not pass in awscredsuri as a mount option")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: align

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ashley-wenyizha, wangnyue, yinsihaws

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:

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

@Ashley-wenyizha
Copy link
Contributor Author

/re-test

@Ashley-wenyizha
Copy link
Contributor Author

After running hack/update-gofmt, pull-aws-efs-csi-driver-verify still fails, similar in #752

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 11, 2022
@Ashley-wenyizha
Copy link
Contributor Author

failed test was able to be fixed updating Go to latest version and run /hack/update-gofmt

Cappuccinuo
Cappuccinuo reviewed Aug 12, 2022
View reviewed changes
pkg/driver/node.go
@@ -165,6 +165,12 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu
}
}

if f == "awscredsuri" {
Copy link

@Cappuccinuo Cappuccinuo Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I would suggest to have a non supported options list, and check whether that option is in that list
  2. If that option is not supported, I think we should fail gracefully by just ignore that option + warn log, instead of returning an error. If we do want an exception here, I would recommend we update document somewhere and tell customer the option is not supported by csi-driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, logged this behavior, return nil but do not fail

Updated this behavior in ReadMe

@Cappuccinuo
Copy link

Added on above comment:

  1. Can we add some unit test for this change? Basically validating when the option is passed, the csi-driver won't fail the mount but post a warn log, or that option is not passed to efs-utils at all.
  2. I think we should ignore the netns option as well, but that could be in a separate PR.

@Ashley-wenyizha Ashley-wenyizha force-pushed the CVEMitigation branch 3 times, most recently from d2487b4 to 0953a84 Compare August 12, 2022 15:02
@nckturner
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 12, 2022
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 12, 2022
@Ashley-wenyizha
Copy link
Contributor Author

/re-test

@nckturner
Copy link
Contributor

/retest

Cappuccinuo
Cappuccinuo reviewed Aug 13, 2022
View reviewed changes
examples/kubernetes/access_points/README.md Outdated
the volume from a pod.

- `awscredsuri` mount option is not supported through efs-csi-driver due to security concerns.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not due to security concerns, it is due to this option is designed and used by ECS tasks.

Cappuccinuo
Cappuccinuo reviewed Aug 13, 2022
View reviewed changes
pkg/driver/node.go Outdated
@@ -165,6 +165,11 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu
}
}

if f == "awscredsuri" {
klog.Warning("awscredsuri is not accepted as a efs-csi-driver mount option")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awscredsuri mount option is not supported by efs-csi-driver.

@Ashley-wenyizha
Should not pass in mount option of awscredsuri
019e772 
Log this behavior, return nil but do not fail
@nckturner
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2022
@k8s-ci-robot k8s-ci-robot merged commit 88e6a0e into kubernetes-sigs:master Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cappuccinuo Cappuccinuo Cappuccinuo left review comments

@yinsihaws yinsihaws yinsihaws approved these changes

@wangnyue wangnyue wangnyue approved these changes

@nckturner nckturner Awaiting requested review from nckturner

@wongma7 wongma7 Awaiting requested review from wongma7

Assignees

@nckturner nckturner

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Ashley-wenyizha @wangnyue @k8s-ci-robot @Cappuccinuo @nckturner @yinsihaws