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

[feat] pull-through caching/proxying for images #370

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DerekTBrown
Copy link
Contributor

@DerekTBrown DerekTBrown commented Nov 27, 2024

Background

  • Motivation: [feature request] Pulling images from remote registries #355

  • In my "real" Kubernetes clusters, I use ECR as an image registry. Nodes are automatically authorized/configured to pull from ECR through a combination of IAM and containerd settings.

  • I want my Tilt Kubernetes clusters to mirror "real" clusters as much as possible. I want to avoid making Tilt-specific modifications to Kubernetes manifests to make them work (see discussion in [feature request] Pulling images from remote registries #355).

  • There currently isn't a way to achieve both of these goals simulaneously:

    • I could use some combination of docker pull && docker push or kind load, though this makes the Tilt configuration clunky.
    • I could provide imagePullSecrets, but this causes the Tilt configuration to deviate from the "real" configuration (and would require modifying hundreds of helm charts).
  • Note: I think there are other, similar usecases for providing cluster authentication credentials. For example, a user might want to avoid Docker registry ratelimits by providing a user token.

What does this PR do?

  • This PR adds the ability for a user to configure pull-through proxy/cache registries, which are attached to the kind cluster (possible in other cluster provisioners, but I have chosen this as a starting point).
  • Authentication can be passed to the proxy via the username and password properties, or via a templated env var to password.

Test Plan

  • I created a cluster using a Docker Hub endpoint, passing in my username and password (token).
  • I was successfully able to start docker containers with an image contained in a private repo.

@DerekTBrown DerekTBrown force-pushed the debrown_feat_pull_through_2 branch from 658329d to 9385eb7 Compare November 27, 2024 20:15
@nicks nicks self-requested a review December 6, 2024 02:54
@nicks
Copy link
Member

nicks commented Dec 6, 2024

hey @DerekTBrown ! thanks for diving into the tangled mess that is containerd configs.

so...this is complicated. my understanding is that containerd-team is in the middle of removing this functionality. see the "mirror config deprecation PSA" here:

https://github.com/kubernetes-sigs/kind/releases/tag/v0.20.0

and the big DEPRECATED node at the top of this doc --

https://github.com/containerd/containerd/blob/main/docs/cri/registry.md#configure-image-registry

and that even today, the code you have here will break in a bunch of cases because ctlptl sets config_path. but i haven't chatted with the containerd folks for a while, lemme try to sus out what the right way to do this is.

@nicks
Copy link
Member

nicks commented Dec 6, 2024

lol, there's also a big note on why they're removing it 🙈

NOTE: registry.configs.*.auth is DEPRECATED and will NOT have an equivalent way to store unencrypted secrets in the host configuration files.

@nicks
Copy link
Member

nicks commented Dec 6, 2024

one crazy idea -- Kind does let you pass extra args to the kubelet

https://kind.sigs.k8s.io/docs/user/configuration/#kubeadm-config-patches

so you might be able to mount a custom Credential provider that does what you want

https://kubernetes.io/docs/tasks/administer-cluster/kubelet-credential-provider/#configure-a-kubelet-credential-provider

i think this might be closer to what the "real" EKS does to authenticate against ECR these days?

@DerekTBrown
Copy link
Contributor Author

NOTE: registry.configs.*.auth is DEPRECATED and will NOT have an equivalent way to store unencrypted secrets in the host configuration files.
lemme try to sus out what the right way to do this is.

I am not sure how I missed these deprecation warnings- I have seen this in practice a bunch.

I interpret the deprecation notice to mean that there will be some future interface, but that interface will require storing credentials in an encrypted format.

Assuming that is the case, I think this PR still makes sense: we would just need to migrate to that interface when it becomes available.

@DerekTBrown
Copy link
Contributor Author

one crazy idea -- Kind does let you pass extra args to the kubelet

Assuming we do need to implement an alternative authentication strategy, I have a slightly different approach in mind. We could extend the Registry API spec slightly to allow configuring the Registry as a pull-through cache with authentication:

https://distribution.github.io/distribution/recipes/mirror/#run-a-registry-as-a-pull-through-cache

We would then follow the exact same pattern currently used to redirect localhost:* images to the Cluster registry, but instead to redirect to the Pull-through registry.

I think this has a few advantages:

  • Because the authentication implementation lives in the registry layer, it is shared across all of the cluster providers; no need to implement authentication for each provider type (though some work would be required of course to do redirects).
  • I think this could be extra useful when coupled with Persistent Storage for Registry #346, since users could easily cache upstream images on the machine between tilt ci runs (for example).

Copy link
Member

@nicks nicks left a comment

Choose a reason for hiding this comment

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

ok, i guess i'm fine with trying this approach while we wait to see what happens with containerd

pkg/api/types.go Outdated Show resolved Hide resolved
pkg/api/types.go Outdated Show resolved Hide resolved
pkg/api/types.go Outdated Show resolved Hide resolved
pkg/cluster/admin_kind.go Show resolved Hide resolved
@nicks
Copy link
Member

nicks commented Dec 8, 2024

(though i'm also fine with trying the proxy approach you linked to)

Signed-off-by: Derek Brown <[email protected]>
@DerekTBrown DerekTBrown force-pushed the debrown_feat_pull_through_2 branch from 3fa854f to ee88e08 Compare December 23, 2024 03:53
@DerekTBrown DerekTBrown requested a review from nicks December 23, 2024 03:54
@DerekTBrown
Copy link
Contributor Author

@nicks should be good to go!

@DerekTBrown DerekTBrown force-pushed the debrown_feat_pull_through_2 branch from b1f7771 to eb07193 Compare December 23, 2024 03:59
nit
Signed-off-by: Derek Brown <[email protected]>
@DerekTBrown DerekTBrown force-pushed the debrown_feat_pull_through_2 branch from eb07193 to f6502ca Compare December 23, 2024 04:07
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