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

Fix credential rotation issue for ECS-A Windows #3184

Merged
merged 1 commit into from
May 2, 2022

Conversation

vsiddharth
Copy link
Contributor

Summary

Fix credential rotation issue for ECS-A Windows

Implementation details

Credentials were not being rotated properly on ECS-A Windows instances.
This patch addresses the issue by using the correct file-paths for
credentials on supported platforms. The credential chain hierarchy is
also updated on ECS-A windows to ensure that credential chain is not
broken for other launch types.

Testing

Manually updated the ECS-Agent on both ECS/EC2 and ECS/ECS-A Windows instances to validate credentials being refreshed.

New tests cover the changes: No

Description for the changelog

Bug - Fixed credential rotation issue with ECS-A Windows

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@vsiddharth vsiddharth force-pushed the ecs-a-creds-win branch 2 times, most recently from 7b319e4 to 5212e8f Compare April 20, 2022 22:32
Credentials were not being rotated properly on ECS-A Windows instances.
This patch addresses the issue by using the correct file-paths for
credentials on supported platforms. The credential chain hierarchy is
also updated on ECS-A windows to ensure that credential chain is not
broken for other launch types.

Signed-off-by: Siddharth Vinothkumar <sidvin@amazon.com>
// 2. Shared credentials file (https://docs.aws.amazon.com/ses/latest/DeveloperGuide/create-shared-credentials-file.html) (file at ~/.aws/credentials containing access key id and secret access key).
// 3. EC2 role credentials. This is an IAM role that the user specifies when they launch their EC2 container instance (ie ecsInstanceRole (https://docs.aws.amazon.com/AmazonECS/latest/developerguide/instance_IAM_role.html)).
// 4. Rotating shared credentials file located at /rotatingcreds/credentials
func GetCredentials(isExternal bool) *credentials.Credentials {
Copy link
Contributor

Choose a reason for hiding this comment

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

For Linux mark this as _ since its unused right?

Copy link
Contributor

@jterry75 jterry75 left a comment

Choose a reason for hiding this comment

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

LGTM. Just one nit

@jdstuart
Copy link

jdstuart commented Apr 30, 2022

@vsiddharth before merging should you also consider bumping the go version to 1.17 or 1.18 to add support for windows/arm64 architectures?
cc: @mythri-garaga @sparrc
PS. Thanks for the quick turnaround on this.

@vsiddharth
Copy link
Contributor Author

@vsiddharth before merging should you also consider bumping the go version to 1.17 or 1.18 to add support for windows/arm64 architectures? cc: @mythri-garaga @sparrc PS. Thanks for the quick turnaround on this.

The agent is already using go 1.17.5

Can you please provide additional context on this?

@sparrc sparrc merged commit 5ca8089 into aws:dev May 2, 2022
@sparrc
Copy link
Contributor

sparrc commented May 2, 2022

before merging should you also consider bumping the go version to 1.17 or 1.18 to add support for windows/arm64 architectures?

I think windows/arm64 would be a larger feature to support, are you currently custom-building the ecs agent to run on that platform?

@jdstuart
Copy link

jdstuart commented May 2, 2022

No, we do not currently run on windows/arm64.

I mistakenly looked at https://github.com/aws/amazon-ecs-agent/blob/master/.travis.yml and thought go 1.15 is still used. I didn't see the GO_VERION files.

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.

6 participants