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

Add AssumeRole support for EFS provider #215

Merged
merged 8 commits into from
Aug 21, 2019
Merged

Conversation

hakanmemisoglu
Copy link
Contributor

Change Overview

Adds AssumeRole support for EFS provider.

Pull request type

Please check the type of change your PR introduces:

  • Feature

@@ -61,13 +62,23 @@ func NewEFSProvider(config map[string]string) (blockstorage.Provider, error) {
return nil, errors.New("Account ID is empty")
}
accountID := *user.Account
efsCli := awsefs.New(s, aws.NewConfig().WithRegion(region))
backupCli := backup.New(s, aws.NewConfig().WithRegion(region))
creds := awsConfig.Credentials
Copy link

Choose a reason for hiding this comment

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

@hakanmemisoglu - this makes sense to me i.e. adding the ability to accept an AWS role as an input and using that if specified - this is the direction we want to head in for all AWS operations.

@julio-lopez - second pair of eyes please? Some context on why we're adding this. Instead of creating a "test user" that has the credentials required - we've created a "customer permissions" role - that we will allow all users to assume. The EFS tests are the first one to use that functionality

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that we should encapsulate the support for AWS roles as part of creating either the config or the session for AWS ops. This could be used for block and potentially object store operations as well.
For now, it may be enough to factor out the role-related functionality into a separate function, and then we can figure out how to generalize it. Thoughts?

pkg/blockstorage/awsefs/awsefs.go Outdated Show resolved Hide resolved
@tdmanv
Copy link
Contributor

tdmanv commented Aug 20, 2019

This change looks good and makes sense to me.
I have a high level question though - @hakanmemisoglu do you have a sense for how this parameter will be specified from the callers? What is the end mechanism to configure this?

@hakanmemisoglu
Copy link
Contributor Author

hakanmemisoglu commented Aug 20, 2019

Blockstorage structs are mainly used by the structs in function package. These function structs are responsible for setting up the config.

I think, as a next step, we should add EFS support in these functions. Config should be set up exactly how EBS is set up. As an additional step, we can also check Role information. If we find one, role argument should be passed to the provider.

Right now, configuration building is done by checking credentials in profile and the info from PVC (in EBS case). We can add additional key-pair for AssumeRole ARN in credentials. Or we can add operation parameters to the whole process; so PVC info, secret credentials and operation parameters together will be sufficient to run Kanister ops (functions).

pkg/blockstorage/awsebs/awsebs.go Outdated Show resolved Hide resolved
@@ -61,13 +62,23 @@ func NewEFSProvider(config map[string]string) (blockstorage.Provider, error) {
return nil, errors.New("Account ID is empty")
}
accountID := *user.Account
efsCli := awsefs.New(s, aws.NewConfig().WithRegion(region))
backupCli := backup.New(s, aws.NewConfig().WithRegion(region))
creds := awsConfig.Credentials
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that we should encapsulate the support for AWS roles as part of creating either the config or the session for AWS ops. This could be used for block and potentially object store operations as well.
For now, it may be enough to factor out the role-related functionality into a separate function, and then we can figure out how to generalize it. Thoughts?

Copy link

@vkamra vkamra left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for addressing the comments.

@hakanmemisoglu hakanmemisoglu merged commit 2210b2c into master Aug 21, 2019
@depohmel depohmel deleted the efs-assume-role branch August 22, 2019 21:58
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.

None yet

4 participants