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

Serve container image layers from AWS by default (make exception when clients are from Google) #143

Closed
hh opened this issue Jan 29, 2023 · 22 comments
Assignees

Comments

@hh
Copy link
Member

hh commented Jan 29, 2023

Our current logic is to default to Google for traffic not from AWS.
We should update the logic to default to AWS if not Google.

This will directly address our top two priorities from our meeting last week.

  • -1 : GCP spend HAS to go down (So we stay within budget and make room for other things we need the $$'s for)
  • 0 : AWS spend HAS to go up (If we don't use it, we will end up not getting more)

Our main logic for handling redirects is here:
https://github.com/kubernetes/registry.k8s.io/blob/main/cmd/archeio/app/handlers.go#L123-L131

		// check if client is known to be coming from an AWS region
		awsRegion, ipIsKnown := regionMapper.GetIP(clientIP)
		if !ipIsKnown {
			// no region match, redirect to main upstream registry
			redirectURL := upstreamRedirectURL(rc, rPath)
			klog.V(2).InfoS("redirecting blob request to upstream registry", "path", rPath, "redirect", redirectURL)
			http.Redirect(w, r, redirectURL, http.StatusTemporaryRedirect)
			return
		}

I'm suggesting the following or similar:

	// initialize map of clientIP to AWS region
	regionMapper := gcp.NewGCPRegionMapper()
       //... snip ...//
		// check if client is known to be coming from an GCP region
		gcpRegion, ipIsKnown := regionMapper.GetIP(clientIP) // 
		if !ipIsKnown {
			// no region match at GCP, redirect to main upstream registry
			redirectURL := upstreamRedirectURL(rc, rPath)
			klog.V(2).InfoS("redirecting blob request to upstream registry", "path", rPath, "redirect", redirectURL)
			http.Redirect(w, r, redirectURL, http.StatusTemporaryRedirect)
			return
		}

We will need to create a net/cidrs/gcp similar to main/pkg/net/cidrs/aws

It should be nearly the same code, with minor changes to main/pkg/net/cidrs/aws/internal/ranges2go/genrawdata.sh

Swapping out the AWS ranges with GCP ranges:

@dims
Copy link
Member

dims commented Jan 29, 2023

cc @BenTheElder @thockin @ameukam

@thockin
Copy link
Member

thockin commented Jan 30, 2023

SGTM on principle.

@BenTheElder
Copy link
Member

Yeah:

  1. Doing this is an obvious next step now that we have a real budget from amazon. We've been discussing that since last year following the announcement in the SIG meeting and other forums.
  2. Adding the GCP ranges is relatively easy, however matching them separately is inefficient, so we should refactor a bit.

The main detail that needs settling is how we handle routing non-AWS users to AWS.

@ameukam suggested perhaps we should just go ahead and switch to cloudfront.
Otherwise I think we should probably need to switch from IP => region to cloud run instance => region, and figure out and configure a reasonable mapping.

@dims
Copy link
Member

dims commented Jan 30, 2023

@BenTheElder switching to cloudfront sounds like a good quick win, lets do that and leave the other suggestion for a longer time frame. (switching to cloudfront sounds like a reversible choice)

@sftim
Copy link

sftim commented Jan 30, 2023

Are we switching layer serving to CloudFront, or https://registry.k8s.io/ itself? The first option is straightforward but doesn't cut the GCP bill.

To tell the truth, I'm not sure how the second option helps the GCP bill either.

@sftim
Copy link

sftim commented Jan 30, 2023

Perhaps we're thinking of using AWS (and CloudFront) to serve the lot, and not use GCP at all?

@thockin
Copy link
Member

thockin commented Jan 30, 2023 via email

@BenTheElder
Copy link
Member

BenTheElder commented Jan 30, 2023

@ameukam was suggesting migrating to cloudfront for layer serving instead of regionalizing to s3 ourselves (which we currently do by mapping AWS client IP known region to nearest-serving S3 region).

We either have to do that or otherwise update how we regionalize to work with non-AWS users, as a prerequesite to "default layer serving to Amazon".

As Tim said, serving content blobs is the only expensive part.

The option to regionalize by assigning a default S3 bucket per cloud run region is potentially less work than spinning up cloudfront, depending on who's working on it. It doesn't require new infra but the mapping would take some thought.

@sftim
Copy link

sftim commented Jan 30, 2023

Ah, right. Then for layers we either:

  • serve direct from AWS for clients already inside AWS, and use a CloudFront distribution for the rest of the world (low-cost price class / backed by a cheap region)
  • serve everything through CloudFront (low-cost price class / backed by a cheap region)
  • serve everything through CloudFront (low-cost price class / backed by a cheap region), except for clients inside GCP
  • one of the above three options with an extra fallback to GCP in case S3 is offline

Serving directly from S3 for clients inside AWS has benefits (that mainly accrue to the client) - for example, they can use a gateway-type VPC endpoint for image pulls and avoid using the public internet. Switching away might merit a notification that people who relied on this property now cannot.

@ameukam
Copy link
Member

ameukam commented Jan 30, 2023

serve everything through CloudFront

This option is the one we need to go with. we don't want to deal with specific use cases that will increase our operational burden. For users with specific requirements we will suggest to have a local mirror.

@sftim
Copy link

sftim commented Jan 30, 2023

OK; I do think we should announce the change though. We don't need to add a wait period, because we already told people not to rely on implementation details.

@BenTheElder
Copy link
Member

one of the above two options with a fallback to GCP in case S3 is offline

We have to do this not just because S3 offline (seems unlikely anyhow) but the more common problem that async layer population hasn't happened yet. Synchronous promotion to AWS has not landed in the image promoter / release process.

This part is already implemented. https://github.com/kubernetes/registry.k8s.io/blob/main/cmd/archeio/docs/request-handling.md

serve everything through CloudFront

I think people here are conflating sticking cloudfront in front of the entire service, which I do not agree with and had not been suggested previously, as opposed to sticking cloudfront in front of the layer store, it doesn't make technical sense when registry.k8s.io itself is serving nothing* but redirects.

We should look at cloudfront for the layer hosting.

@sftim
Copy link

sftim commented Jan 30, 2023

I amended #143 (comment) to clarify

@BenTheElder
Copy link
Member

Also, in the future we'll want to do different cost routing (say we start to also use fastly, or azure), which is easier to do if it's just updating the redirect logic.

@sftim
Copy link

sftim commented Jan 30, 2023

/retitle Serve container image layers from AWS by default (make exception when clients are from Google)

@k8s-ci-robot k8s-ci-robot changed the title Redirect to AWS by Default (unless customers are from Google) Serve container image layers from AWS by default (make exception when clients are from Google) Jan 30, 2023
@BenTheElder
Copy link
Member

Serving directly from S3 for clients inside AWS has benefits (that mainly accrue to the client) - for example, they can use a gateway-type VPC endpoint for image pulls and avoid using the public internet. Switching away might merit a notification that people who relied on this property now cannot.

That's an interesting point, though our stance so far has very much been that:

  1. We don't have a mechanism to notify all users.
  2. Users may not depend on any implementation details, only OCI compliance https://github.com/kubernetes/registry.k8s.io#stability is at the top of the README that https://registry.k8s.io redirects to and outlines this in more detail.
  3. Due to 2) we don't need a mechanism to notify all users / we are understaffed and funded to manage this.

This sort of detail is what prevented us from redirecting k8s.gcr.io and bringing our costs down immediately, we cannot dig ourselves back into that hole.

If anything we should make "breaking" changes to those depending on implementation details more often (e.g. perhaps renaming the buckets) to underline the point that they're just implementation details and we will use whatever we can fund.

@sftim
Copy link

sftim commented Jan 30, 2023

BTW https://kubernetes.io/blog/2022/11/28/registry-k8s-io-faster-cheaper-ga/#why-isn-t-there-a-stable-list-of-domains-ips-why-can-t-i-restrict-image-pulls is a better link for “don't depend on any implementation details”.

@hh
Copy link
Member Author

hh commented Jan 31, 2023

Similar changes in the wider community and their communication: https://support.hashicorp.com/hc/en-us/articles/11239867821203?_ga=2.46340071.1359745362.1675131001-690834462.1675131001

@BenTheElder
Copy link
Member

PRs are ready, #147 and kubernetes/k8s.io#4739

@BenTheElder
Copy link
Member

kubernetes/k8s.io#4741 promoted the image. last step updating prod.

this change is safe, because even if we misconfigured a default url we will detect the content as not available on AWS and fallback to upstream registry on AR. the runtime logic diff is pretty small, mostly of the diff is in refactoring the cloud IP management and updating the runtime deployment configs to map cloud run region to default s3 region (for clients where we cannot detect a known region based on IP).

will follow-up with a prod deployment PR shortly. sandbox is running smoothly.

@BenTheElder
Copy link
Member

kubernetes/k8s.io#4742 this is deployed

@github-project-automation github-project-automation bot moved this from 🔖 Ready to ✅ Done in AWS Infrastructure (SIG K8s Infra) Feb 9, 2023
@BenTheElder
Copy link
Member

BenTheElder commented Feb 9, 2023

AFAICT the simple per-cloud-run-region regionalizing approach is working well, based on logs etc.

For example pulling from the California Bay area, I am redirected to GCP us-west2 Artifact Registry (Los Angeles) and AWS us-west1 S3 bucket (N. California).

We can revisit cloudfront later, but I don't think we need to rush.

We might want to consider adding more S3 regions, notably South America where we have cloud run / artifact registry but no AWS presence kubernetes/k8s.io#4739 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

6 participants