-
Notifications
You must be signed in to change notification settings - Fork 521
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
Add k8s credential provider support #2377
Conversation
1cb6a2b
to
e4fd621
Compare
e4fd621
to
6ea8f27
Compare
Note: right now this contains some of John's work from #2378 since that will need to merge before the addition of |
3aa9fb4
to
bebd295
Compare
bebd295
to
66e0854
Compare
66e0854
to
5f77dd3
Compare
3671e96
to
159bbb7
Compare
Marking as ready to review, but still performing testing/validation. Everything should be in place though, so pending any testing issues found this should be good to go. |
packages/ecr-credential-provider/ecr-credential-provider-tmpfiles.conf
Outdated
Show resolved
Hide resolved
cd7eda1
to
007cf75
Compare
I believe I have addressed all comments so far. Please let me know if I have missed anything. I've resolved all of the ones that I think were fully addressed. There are a few that either had questions or were open to further discussion which I have not resolved yet pending some sort of resolution. |
a57cd47
to
7293603
Compare
@arnaldo2792 pinging for re-review |
7293603
to
619b0fd
Compare
Updated to resolve merge conflict in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm a little confused about what the "service" named aws
is, but assuming that is done correctly... looks good.
sources/models/src/lib.rs
Outdated
struct AwsSettings { | ||
region: SingleLineString, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon further reflection, I now understand why we arrived at aws.settings.*
for this. It's because there's essentially one ~/.aws/
profile on the machine and it wouldn't make sense to bury that under, e.g. settings.kubernetes
. Makes sense now: 👍
sources/api/migration/migrations/v1.11.0/aws-creds-metadata/src/main.rs
Outdated
Show resolved
Hide resolved
619b0fd
to
54a0bb2
Compare
sources/api/migration/migrations/v1.11.0/kubelet-aws-config/src/main.rs
Outdated
Show resolved
Hide resolved
sources/api/migration/migrations/v1.11.0/kubelet-credential-providers/src/main.rs
Outdated
Show resolved
Hide resolved
6cb448e
to
1d876a8
Compare
This adds the capability to use Kubernetes image credential providers to retrieve credentials to use when pulling images for container creation. Initially we will only support the ecr-credential-provider, but things are set up so we may add more providers in future updates. Signed-off-by: Sean McGinnis <stmcg@amazon.com>
1d876a8
to
c361847
Compare
Rebased and resolved merge conflicts. |
Thanks everyone! |
Issue number:
Closes #1702
Description of changes:
This adds the capability to use Kubernetes image credential providers to
retrieve credentials to use when pulling images for container creation.
Initially we will only support the ecr-credential-provider, but things
are set up so we may add more providers in future updates.
Yet to do:
ecr-credential-provider
binaryTesting done:
ecr-credential-provider
binary: Added RPM files, built and deployed instance, verified the binary was in the expected location on the filesystem.apiclient set
. Verified settings saved and available usingapiclient get
and viewing config settings written to filesystem.any-enabled
template helper: Addition of unit tests to validate various input. Created small test app to execute templating code and verify expected output.aws-config
andaws-credentials
as well as combined using onlyaws-config
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.