-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Add support for using ECR as pull-through image cache #16593
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
base: master
Are you sure you want to change the base?
Conversation
Hi @rsafonseca. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/ok-to-test |
Any idea why these e2e tests might be failing @hakman?
could something be leaking somewhere? or do you think there's any change here that could be somehow related to this weird behaviour? EDIT: nvm, found the problem... hidden tabs in string literal.. damn vscode 😂 |
Please ignore the IPv6 tests |
All good then, though it looks like pull-kops-e2e-cni-cilium-eni is flaky, passed on retest. |
a77071a
to
a7dbaac
Compare
45e0800
to
f682340
Compare
615a93f
to
a7dbaac
Compare
Any update about this PR? Do you need any help or test, to be merged faster? |
Did you get a chance to review this yet @hakman @johngmyers ? :) |
/retest-required |
nodeup/pkg/model/kubelet.go
Outdated
configContent := `apiVersion: kubelet.config.k8s.io/v1 | ||
kind: CredentialProviderConfig | ||
providers: | ||
- name: ecr-credential-provider | ||
matchImages: | ||
- "*.dkr.ecr.*.amazonaws.com" | ||
` |
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.
In office hours, we talked about whether UseECRCredentialsForMirrors should be per mirror or whether we only apply this to urls that "look like" ECR. The (compelling) argument that @rsafonseca made is that people aren't likely to have multiple mirrors so it's not necessary.
👍
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
rebased again, the number of files that needed their hashes updated inside tests/integration increased dramatically since the last rebase so the bot just moved this from L to XXL 🫠 |
5e7e178
to
9a938ad
Compare
9a938ad
to
69e096e
Compare
Signed-off-by: Rafael da Fonseca <rafael.fonseca@wildlifestudios.com>
69e096e
to
575d0fa
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@rsafonseca: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
"ecr:ReplicateImage", | ||
"ecr:BatchImportUpstreamImage", | ||
"ecr:CreateRepository", | ||
"ecr:TagResource", |
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.
@rsafonseca Please move these to a separate addECRPullThroughPermissions(p *Policy)
function and call it only when the flag is set.
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.
Also, add the new option to this test, to see how the generated output looks like. Other than these, LGTM.
containerd: | |
skipInstall: true | |
registryMirrors: | |
docker.io: | |
- https://registry-1.docker.io | |
"*": | |
- http://HostIP2:Port2 | |
packages: | |
urlAmd64: https://github.com/containerd/containerd/releases/download/v1.3.9/cri-containerd-cni-1.3.9-linux-amd64.tar.gz | |
hashAmd64: "0000000000000000000000000000000000000000000000000000000000000000" |
This PR introduces a simple way to enable using ECR as a Pull-through image cache, without having to mutate images on the cluster using tools like Kyverno.
Containerd already has support for specifying registry mirrors in kops, but since ECR uses short lived tokens, it's not trivial (or even impossible without adding a few extra hacks on top of it) to use it as a pull-through cache.
This PR also bumps the ecr-credential-provider binary, which before version 1.29.0 specifically tried to parse an ECR repo URL from the image passed, leading to not being possible to enable this feature. This is now resolved in the latest versions.
This PR uses a flag to enable the feature when needed, and adds any server addresses configured in the mirrors to be allowed on the CredentialProviderConfig object that kops configures on the kubelet.
To configure this:
example: