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

Update aws-sdk-go to v1.23.13 #115

Closed
wants to merge 4 commits into from
Closed

Update aws-sdk-go to v1.23.13 #115

wants to merge 4 commits into from

Conversation

zdoherty
Copy link

@zdoherty zdoherty commented Apr 21, 2020 •
edited
Loading

Updates go.mod to use a more recent version of aws-sdk-go, specifically one which is recommended for supporting IAM Roles for Service Accounts (IRSA) when running in Kubernetes.

Fixes #114

Proposed Changes

  • Update go.mod and go.sum to reference the minimum recommended version of aws-sdk-go for IRSA
  • Include a note in the README about defaulting to instance profile and IRSA credentials

Description

Newer versions of the Go AWS SDK include additional methods for automatically obtaining IAM credentials. Specifically, credentials can be obtained using sts.AssumeRoleWithWebIdentity, which is a requirement for using IAM Roles for Service Accounts when running in Kubernetes. The AWS documentation recommends using v1.23.13 or above to use this feature.

Based on the existing S3 client code, using that version of the SDK should be all that's needed to support this feature.

Not sure how to best test this change. The built-in tests pass and I was able to use drone-cache with IRSA credentials using the updated SDK. Mocking out an STS service may prove to be tough. Any input on how to best accomplish this would be welcome.

Checklist

  • Read the CONTRIBUTING document.
  • Read the CODE OF CONDUCT document.
  • Add tests to cover changes.
  • Ensure your code follows the code style of this project.
  • Ensure CI and all other PR checks are green OR
    • Code compiles correctly.
    • Created tests which fail without the change (if possible).
    • All new and existing tests passed.
  • Add your changes to Unreleased section of CHANGELOG.
  • Improve and update the README (if necessary).
  • Ensure documentation is up-to-date. The same file will be updated in plugin index when your PR is accepted, so it will be available for end-users at http://plugins.drone.io.

kakkoyun
kakkoyun suggested changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution. This is great!
Overall it looks good to me. Trusting that our test cases cover existing functionality we could merge this.
However, if you think you can a basic test case to cover this case to prevent regression would be great.

Only thing I'd love to see, is to update the warning message to notify user

drone-cache/storage/backend/s3/s3.go

Line 42 in 3172b74

level.Warn(l).Log("msg", "aws key and/or Secret not provided (falling back to anonymous credentials)")

@kakkoyun
Copy link
Contributor

@zdoherty One other thing, why don't we upgrade to latest? https://github.com/aws/aws-sdk-go/releases
Are there any blockers? If they respect semver it should be good to upgrade.

@zdoherty
Copy link
Author

zdoherty commented Apr 26, 2020 •
edited
Loading

One other thing, why don't we upgrade to latest?

@kakkoyun Definitely agree that upgrading to the most recent version could be ideal, but I wasn't sure if that was something which would be welcome. I'll give it a shot!

Also, found this in the minio docs: https://github.com/minio/minio/blob/master/docs/sts/web-identity.md
That might be compatible enough with the AWS SDK STS client to provide a suitable service to test against.

@artemsablin
Copy link

Hi, any progress on this? This looks like a very useful feature.

@kakkoyun
Copy link
Contributor

@zdoherty any updates?

@kakkoyun
Copy link
Contributor

@zdoherty friendly ping

@kakkoyun
Copy link
Contributor

@zdoherty Any plans to push this to finish line? Or shall we close it?

@kakkoyun
Copy link
Contributor

kakkoyun commented Feb 2, 2021

Closing this one. Because of inactivity and we have another PR in the pipeline that updates this dependency.

@kakkoyun kakkoyun closed this Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kakkoyun kakkoyun kakkoyun requested changes

@limayekaiwalya limayekaiwalya Awaiting requested review from limayekaiwalya

@seandtaber seandtaber Awaiting requested review from seandtaber seandtaber is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support IAM Roles for Kubernetes Service Accounts (IRSA)
3 participants
@zdoherty @kakkoyun @artemsablin