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

Update credential provider API to beta for 1.24+ #1089

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

mmerkes
Copy link
Member

@mmerkes mmerkes commented Nov 7, 2022

Description of changes:
As of 1.22, we added ECR credential provider support, which was an alpha feature. The feature was enabled here, but in my testing, those branches didn't actually catch correctly, so the changes were never applied. As of 1.24, the feature have graduated to beta, so this PR updates the API version for 1.24+.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Testing Done

Created 1.24 and 1.23 AMIs, verified that they joined a cluster and confirmed that --image-credential-provider-config /etc/eks/ecr-credential-provider/ecr-credential-provider-config --image-credential-provider-bin-dir /etc/eks/ecr-credential-provider flags were set correctly and that 1.23 set alpha APIs and 1.24 set beta APIs.

See this guide for recommended testing for PRs. Some tests may not apply. Completing tests and providing additional validation steps are not required, but it is recommended and may reduce review time and time to merge.

@mmerkes mmerkes force-pushed the creds branch 4 times, most recently from 774e608 to 4fe8729 Compare November 7, 2022 06:41
files/bootstrap.sh Outdated Show resolved Hide resolved
@cartermckinnon
Copy link
Member

We need some test cases for this 😬

.gitignore Outdated Show resolved Hide resolved

The issue is discussed in [this StackExchange post](https://unix.stackexchange.com/questions/101080/realpath-command-not-found).

On OSX, running `brew install coreutils` resolves the issue.
Copy link
Member

Choose a reason for hiding this comment

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

huh, must have forgotten I did this in the past.

elif vercmp "$KUBELET_VERSION" gteq "1.22.0"; then
# Ensure that these exist for testing purposes
mkdir -p /etc/eks/ecr-credential-provider
touch /etc/eks/ecr-credential-provider/ecr-credential-provider-config
Copy link
Member

Choose a reason for hiding this comment

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

is this necessary given the vercmp above? Shouldn't these always be present for k8s >= 1.22?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because we copy them in install-worker.sh, so when that doesn't run, they don't exist.

Copy link
Member

Choose a reason for hiding this comment

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

Can you handle this in your test cases? Just seems odd to be in the bootstrap script

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya, it is a little odd and not ideal. It's not my test cases. The issue is that I didn't want to go edit every other test case that invokes bootstrap.sh and have it be a thing that perpetually needs to be handled.

@mmerkes mmerkes merged commit a521047 into awslabs:master Nov 10, 2022
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.

3 participants