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

AWS IAM: Support for AWS EKS ServiceAccount roles for CloudWatch and S3 image upload #21594

Merged
merged 6 commits into from
Apr 8, 2020

Conversation

patstrom
Copy link
Contributor

@patstrom patstrom commented Jan 18, 2020

What this PR does / why we need it:

This PR replicates (most of) the behavior of aws-sdk-go when fetching a WebIdentityRole such as they are used in IAM roles for service account in EKS.

The reason for the "(most of)" qualifier is that this will only read the environment variables, so if e.g a role_session_name is configured in a configuration file somewhere it won't be picked up. I suspect this won't be much of a problem since all documentation and whatnot for EKS IAM roles for Service Accounts exclusively uses those environment variables.

As discussed in the issue the "proper" way to fix this would probably be to always use the default credentials chain unless a specific way is specified in which case only that way should be used.

I've tried this out in an EKS cluster with a standard setup. In other words I've created a deployment and a service account, linked them together, annotated the service account with eks.amazonaws.com/role-arn and added the necessary trust policies on the role and I can confirm it manages to find the credentials.

Which issue(s) this PR fixes:

Fix #20473

@patstrom
Copy link
Contributor Author

I realized this change is probably needed in the s3uploader for images as well. Perhaps the AWS configuration should be centralized into a single pkg that both the cloudwatch executor, the image uploader and any potential future interactions can re-use?

@marefr marefr added area/backend pr/external This PR is from external contributor labels Jan 20, 2020
@patstrom patstrom requested a review from aknuds1 January 27, 2020 08:46
@aknuds1 aknuds1 self-assigned this Jan 27, 2020
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

I made a couple of suggestions for minor improvements. I'll see if I can test this in order to reproduce the issue.

pkg/components/imguploader/s3uploader.go Outdated Show resolved Hide resolved
pkg/tsdb/cloudwatch/credentials.go Outdated Show resolved Hide resolved
@aknuds1
Copy link
Contributor

aknuds1 commented Jan 30, 2020

Thanks for applying my suggestions, I'm just waiting to get access to an EKS cluster so I can test.

use WebIdentityRole to assume another role
@aknuds1 aknuds1 assigned ryantxu and unassigned aknuds1 Feb 3, 2020
@aknuds1
Copy link
Contributor

aknuds1 commented Feb 3, 2020

Handing this over to @ryantxu since he needs to look it over too.

@patstrom
Copy link
Contributor Author

patstrom commented Feb 10, 2020

@evq you need to sign the cla.

@ryantxu what is the status on this?

@evq
Copy link
Contributor

evq commented Feb 12, 2020

@patstrom signed

@iptizer
Copy link

iptizer commented Feb 18, 2020

Is there anything I can do to speed this up a little bit? Hit me!

How I see it @aknuds1 currently has some changes requested. Can I help you testing or something?

@patstrom
Copy link
Contributor Author

I've already addressed the requested changes. Perhaps @aknuds1 needs to mark them as resolved as well?

@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM, but waiting for @ryantxu's review.

@patstrom
Copy link
Contributor Author

@ryantxu What is the status on this?

@dnascimento
Copy link
Contributor

@ryantxu When may you review this?

@chancez
Copy link

chancez commented Apr 6, 2020

How does this work in combination with configuring the assume role ARN for the cloudwatch datasource? Can we use the EKS service account as the source profile to assume role with the AWS IAM role specified in the datasource? I'm trying to see if once this is fixed if I can use the SA IAM role to allow assuming into a different IAM role per cloudwatch datasource so I can use a single grafana with multiple AWS accounts.

@@ -60,6 +61,7 @@ func GetCredentials(dsInfo *DatasourceInfo) (*credentials.Credentials, error) {
&credentials.EnvProvider{},
&credentials.SharedCredentialsProvider{Filename: "", Profile: dsInfo.Profile},
remoteCredProvider(stsSess),
Copy link
Contributor

Choose a reason for hiding this comment

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

@patstromm, could you set webIdentityProvider before remoteCredProvider? It's likely that the EC2 instances have a role attached to it, so this order will prioritise the EC2 instance role over the webIdentity. It's better to try first to use the WebIdentity

@dnascimento
Copy link
Contributor

How does this work in combination with configuring the assume role ARN for the cloudwatch datasource? Can we use the EKS service account as the source profile to assume role with the AWS IAM role specified in the datasource? I'm trying to see if once this is fixed if I can use the SA IAM role to allow assuming into a different IAM role per cloudwatch datasource so I can use a single grafana with multiple AWS accounts.

The EKS service account role can assume a role as well if you define the arn, the code flow is:

if dsInfo.AuthType == "arn" {

webIdentityProvider(stsSess),

resp, err := svc.AssumeRole(params)

@patstrom I tested the code in our systems, could you address the 2 issues above so that this can be reviewed and merged?

@chancez
Copy link

chancez commented Apr 7, 2020

@dnascimento yep, I see now, looks like it should work. I'll build this locally and see if I can get it to work with multiple roles. Thanks.

@patstrom
Copy link
Contributor Author

patstrom commented Apr 7, 2020

I currently have a WIP of the "proper" way here, where I add a AWS SDK Default authentication provider that does exactly what you'd expect.

I plan to open a separate MR when it's ready (hopefully later this week).

@ryantxu
Copy link
Member

ryantxu commented Apr 8, 2020

@patstrom -- the new approach looks great. thanks for the effort

@aknuds1
Copy link
Contributor

aknuds1 commented Apr 8, 2020

Synced your branch with Grafana master, to fix CI issues. Hope you don't mind!

@aknuds1 aknuds1 closed this Apr 8, 2020
@aknuds1 aknuds1 reopened this Apr 8, 2020
@patstrom patstrom requested a review from a team as a code owner April 8, 2020 12:30
@aknuds1 aknuds1 merged commit dbda5ae into grafana:master Apr 8, 2020
@aknuds1
Copy link
Contributor

aknuds1 commented Apr 8, 2020

This took a while, but we finally got it merged - thank you for contributing to Grafana!

@patstrom
Copy link
Contributor Author

patstrom commented Apr 8, 2020

Cheers! 🎉

You're not getting rid of me that easily though. I just opened a new PR with the "proper" way to solve this that also includes some other fixes to the Cloudwatch data source executor.

@marefr marefr changed the title cloudwatch: Replicate SDK behaviour for WebIdentityRole CloudWatch: Replicate SDK behaviour for WebIdentityRole Apr 9, 2020
@marefr marefr changed the title CloudWatch: Replicate SDK behaviour for WebIdentityRole AWS IAM: Support for AWS EKS ServiceAccount roles Apr 9, 2020
peterholmberg pushed a commit that referenced this pull request Apr 9, 2020
* Replicate SDK behaviour for WebIdentityRole

Fix #20473

* Use WebIdentityRole in s3 uploader as well

* Use consistent casing

* use WebIdentityRole to assume another role

Co-authored-by: eV <ev@7pr.xyz>
@aknuds1 aknuds1 added this to the 7.0 milestone Apr 17, 2020
@marefr marefr changed the title AWS IAM: Support for AWS EKS ServiceAccount roles AWS IAM: Support for AWS EKS ServiceAccount roles for CloudWatch and S3 image upload Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS IAM: Support for AWS EKS ServiceAccount roles
9 participants