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

Prioritize ssm creds for linux #4155

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Prioritize ssm creds for linux #4155

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02962a0
Change credentials priority for linux: rotating credentials should be…
sidlovskyy Apr 24, 2024
884a8c2
Add checking if credentialChain is nil
sidlovskyy Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion ecs-agent/credentials/instancecreds/instancecreds_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,27 @@ import (
// GetCredentials returns the instance credentials chain. This is the default chain
// credentials plus the "rotating shared credentials provider", so credentials will
// be checked in this order:
//
// 1. Env vars (AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY).
//
// 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
//
// In the case of external instance `RotatingSharedCredentialsProvider` provider should be prioritized over
// `EC2RoleProvider` one as SSM credentials are going to be ignored in case agent is deployed to EC2
// with instance role which doesn't have permissions needed to run ECS Agent
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.

Looks like this change makes GetCredentials implementation for Linux identical to that of Windows. I think we should merge the two into a single instancecreds/instancecreds.go file.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the meanwhile I will reproduce this issue on my end. Once the PR is ready I can verify that the change works by testing it in the same environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to reproduce the issue on my end.

mu.Lock()
if credentialChain == nil {
credProviders := defaults.CredProviders(defaults.Config(), defaults.Handlers())
credProviders = append(credProviders, providers.NewRotatingSharedCredentialsProvider())
if isExternal {
credProviders = append(credProviders[:2], append([]credentials.Provider{providers.NewRotatingSharedCredentialsProvider()}, credProviders[2:]...)...)
} else {
credProviders = append(credProviders, providers.NewRotatingSharedCredentialsProvider())
}
credentialChain = credentials.NewCredentials(&credentials.ChainProvider{
VerboseErrors: false,
Providers: credProviders,
Expand Down
Loading