-
Notifications
You must be signed in to change notification settings - Fork 545
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
EFS-CSI pod impersonation implementation #710
Conversation
Hi @lmouhib. 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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a really good solution in general! I was working on something similar but thought it'd be better if I got behind what you were doing instead, which looks great. There are a few things to look at but feel free to push back on any of them, they're just my thoughts and I'm not expert.
I'd say a couple of general things as well as the specific points I made in the review:
- It'd be worth looking into feature flagging this whole thing and setting it to false in the Helm Values. This means people have to explicitly opt-into it and will give them time to migrate, one of the most important things in this driver is backwards compatibility and if people have specific stuff setup to do with the serviceAccounts that already exist we want to give them a smooth upgrade path. I'd extend that to the new Helm Resources too.
- I think it would be work extending PodAuth out and split it up into functions a bit more. This will make it easier to Unit Test because you can test one small bit at a time, while still keeping a nice public interface in the
setPodCredentials
function. - This definitely needs unit tests adding to it, the kubernetes library has a really nice
fake
library that I've added in a few PRs already. It means you can set up a fake clientset and make unit tests that react to it without a lot of effort. I'd look into that. - Does any work need to be done when
NodeUnpublishVolume
is called?
Let me know if some of that doesn't make sense or you need any help, it's a really great feature to see added to the driver!
@@ -73,12 +93,41 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu | |||
// TODO when CreateVolume is implemented, it must use the same key names | |||
subpath := "/" | |||
encryptInTransit := true | |||
iamAuth := false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With regards to this, would it make more sense to feature flag this entire idea, then you can assume that if the user has turned the feature flag on this is what they want which gets rid of the need for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for the feature flag, because the CSIDriver object is immutable and we need to keep backward compatibility as you mentioned. We need iamAuth
there will be cases where iam auth
is not needed. The Driver is used across the cluster and you might have a mix of EFS that use IAM or not. If it is set to true by default, efs-utils
behavior when provided with iam
mount option but there is no IAM
credential available (for the pod) will try to use whatever credential available including instance profile which is not something desirable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - suggest to change the var names here and below as well
pkg/driver/node.go
Outdated
result := make(map[string]interface{}) | ||
json.Unmarshal([]byte(volContext["csi.storage.k8s.io/serviceAccount.tokens"]), &result) | ||
tokenInfo := fmt.Sprintf("%v", result["sts.amazonaws.com"].(map[string]interface{})["token"]) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could result be a map[string]map[string]string
here? Not the prettiest data type but it avoids the use of interface{}
. Further I wonder if there's a JWT library that would make this extraction a little bit easier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tokens are returned as an array of json string, not sure the JWT library would help here.
@jonathanrainer Thanks for the comments, I did address some of them. Feel free to implement the unit tests, if you would like. I did not implement them yet because I did not settle on how to pass the IAM auth parameter. I believe the current way to pass it is good and keep the same structure as the
|
/ok-to-test |
/retest |
/retest |
I have pulled the code performed test locally on my side, with service account associated role have permission, mount successes, otherwise fails |
/lgtm |
@@ -8,3 +8,10 @@ metadata: | |||
"helm.sh/resource-policy": keep | |||
spec: | |||
attachRequired: false | |||
{{- if .Values.iamAuth }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe something a bit more descriptive for the helm value / volumeAttribute? podIAMAuth
? `podIAMAuthorization? since this is b asically "mounting using the IAM authorization using the pod's IAM credentials" https://docs.aws.amazon.com/efs/latest/ug/mounting-IAM-option.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIxed, used podIAMAuthorization
as it carry more meaning.
|
||
This example shows how you can use the pod impersonation feature available | ||
in the CSI Driver to enforce access control when mounting EFS Access point protected with EFS resource policy. | ||
This feature leverages IAM roles for service accounts (IRSA) and your EKS cluster must be enabled for IRSA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be stated explicitly that this feature is exclusive to EKS. right now it's implied because you need IRSA and only EKS has IRSA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, specified that it only work with EKS.
lgtm I just have one nitpick about the naming of the attribute which will be impossible to change later a it's basically an API .... |
/assign @Ashley-wenyizha |
driver: efs.csi.aws.com | ||
volumeHandle: [FileSystemId]::[AccessPointId] | ||
volumeAttributes: | ||
iamAuth: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you revise here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
storage: 5Gi | ||
``` | ||
|
||
Here it is important to set the property `iamAuth` in `volumeAttribute` to `true`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
driver: efs.csi.aws.com | ||
volumeHandle: fs-e8a95a42::fsap-068c22f0246419f75 | ||
volumeAttributes: | ||
iamAuth: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
/approve Adding approve tag for now and let Matthew drop the |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ashley-wenyizha, lmouhib, 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:
Approvers can indicate their approval by writing |
/lgtm |
Is this a bug fix or adding new feature?
new feature
What is this PR about? / Why do we need it?
Add the ability for efs-csi to impersonate pods
What testing is done?
Tested with pods that have service account annotated with a role ARN. The efs-utils assume the role annotated to the service account and is able to mount an access point that is protected with an IAM resource policy.