Skip to content

Comments

feat: skip explicit docker login and use default config if no registry credentials are provided#1

Merged
tcsullens merged 3 commits intomainfrom
feature/sc-135780/replace-ecr-login-credentials-with-amazon
Feb 12, 2025
Merged

feat: skip explicit docker login and use default config if no registry credentials are provided#1
tcsullens merged 3 commits intomainfrom
feature/sc-135780/replace-ecr-login-credentials-with-amazon

Conversation

@tcsullens
Copy link

@tcsullens tcsullens commented Jan 22, 2025

To support replacing ECR Login Credentials with amazon-ecr-credential-helper in our CI workflows, this customizes the Docker prepare_job Hook to skip the docker login step and "fallback" to a default docker config location if no registry credentials are provided to the Hook, i.e. the container.credentials are not provided.

Additional changes of note:

  • Passing additional environment variables to the docker command to support the Credential Helper (e.g. the AWS_ vars)
  • Setting the GITHUB_ACTIONS and CI environment variables to the container on creation (necessary for some of our internal logic/handling)

PR #2a of Using Docker Credential Helpers for ECR Registries in GitHub Actions

sc-135780

@tcsullens tcsullens force-pushed the feature/sc-135780/replace-ecr-login-credentials-with-amazon branch 7 times, most recently from 0ba19e2 to 91c402f Compare January 28, 2025 15:57
@tcsullens tcsullens force-pushed the feature/sc-135780/replace-ecr-login-credentials-with-amazon branch 7 times, most recently from 2b35bab to a17ab22 Compare January 31, 2025 20:09
@tcsullens tcsullens force-pushed the feature/sc-135780/replace-ecr-login-credentials-with-amazon branch 3 times, most recently from 454fbd2 to 450aacf Compare February 3, 2025 19:22
@tcsullens tcsullens force-pushed the feature/sc-135780/replace-ecr-login-credentials-with-amazon branch from 450aacf to bc406e0 Compare February 3, 2025 19:25
Comment on lines +105 to +118
// If using the ECR docker credential helper, need to ensure that these environment variables
// are present in the shell of the docker pull command.
const containerPullCliEnvs = new Set([
'AWS_PROFILE',
'AWS_ACCESS_KEY_ID',
'AWS_SECRET_ACCESS_KEY',
'AWS_STS_REGIONAL_ENDPOINTS',
'AWS_DEFAULT_REGION',
'AWS_REGION',
'AWS_ROLE_ARN',
'AWS_WEB_IDENTITY_TOKEN_FILE',
'HOME',
'PATH'
])
Copy link
Author

Choose a reason for hiding this comment

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

As mentioned in the code comment, we need these environment variables present in the shell the runner exec uses to run our hook, in order for our hook/docker to properly use the Credential Helper.

Choose a reason for hiding this comment

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

Will these be automatically filtered out from the output?

Copy link
Author

Choose a reason for hiding this comment

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

Per our discussion, added functionality in eddfa91 to ensure the values of these env vars get obfuscated if they're ever present in log output!

}
}
// Explicitly set GITHUB_ACTIONS and CI env vars on the container
dockerArgs.push('-e', 'GITHUB_ACTIONS', '-e', 'CI')
Copy link
Author

@tcsullens tcsullens Feb 4, 2025

Choose a reason for hiding this comment

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

GitHub's Runner does this, and we depend on these being set inside the Job container, so incorporating that here on container creation (the env variables are passed to the docker command in https://github.com/wrapbook/runner-container-hooks/pull/1/files#diff-a01582fa233588888b956b55c88cb9118fb5a841a129372a3e84961890ee5b33R48-R50)

@tcsullens tcsullens requested review from a team and Nulluminati February 7, 2025 17:17
@tcsullens tcsullens marked this pull request as ready for review February 7, 2025 17:18
@tcsullens tcsullens force-pushed the feature/sc-135780/replace-ecr-login-credentials-with-amazon branch from 2477dbf to eddfa91 Compare February 12, 2025 17:46
@tcsullens tcsullens merged commit 3cd3b03 into main Feb 12, 2025
1 check passed
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.

2 participants