registry client: Add alternative sources methods to gather sources from ImageContentSourcePolicies #939

sallyom · 2020-11-06T16:30:29Z

Allow for multiple image source options when retrieving repository (from icsp, from mirrored)

There may be multiple image source options (from icsp, mirrored) rather than a single image source.
This PR adds registry client Context interface and methods to gather ICSP sources for a given Context.

/cc @smarterclayton
/cc @soltysh

fake bump here: openshift/oc#439

soltysh · 2020-11-09T13:31:44Z

/assign @dmage
who is also looking at the other PR

dmage

No, RepositoryList cannot exist.

To create an instance of distribution.Repository, the client sends an HTTP request to the /v2/ endpoint. If the first mirror is available and has all necessary images, the client shouldn't try to access other mirrors nor the original repository. It means distribution.Repository for the second mirror shouldn't be created until the client has checked the first mirror.

On disconnected clusters firewall can drop requests to external registries, i.e. attempt to connect to an external registry can take few minutes. So this extra action may be very time consuming.

pkg/image/registryclient/client.go

sallyom · 2020-11-13T00:14:02Z

/retest

smarterclayton · 2020-11-16T20:30:36Z

Some high level comments:

If we're making a client-specific tradeoff, I think it's best to have that functionality only in oc, since library-go is so widely used.

This is not desirable - it needs to be part of library-go and the general client implementation because the behavior of ICSP is general to all OpenShift clusters, and ICSP is useful to both OpenShift specific commands (oc adm release) and generic registry commands (oc image), and in general no oc command really cares about the details or should have to care.

Oleg raised the point about explicit request to use cluster ICSP vs implicit - and that's a good distinction to make. oc image does not talk to a cluster by default, and should not, because it's generic to registries. A user may want to tell the command to use a cluster ICSP, but that has to be opt in, so it's reasonable to support --use-cluster-icsp on the oc image * commands. Conversely, oc adm release * should be able to honor the cluster ICSP implicitly (because a user can run oc adm release extract --tools and expects the that to succeed regardless). However, for a lot of frequent interactions with the server the contract of oc adm release * in general needs to be fast and looking up the cluster ICSP is not appropriate by default. In a node context ICSP is opinionated and tries to access the exact list order to allow admins to optimize - but when implicitly retrieving a cluster ICSP we can't say for certain that's the best outcome for the user. In specific commands we may very well do so - such as oc adm release mirror or oc adm release new where the ICSP likely has the most performant entry first - because that benefits the admin on an expensive call. However, another goal of ICSP is to improve resiliency, so we have to keep consulting the list of repositories even on subsequent calls to the same distribution.Repository object (i.e. the shim we provide has to have some dynamism).

soltysh

Also seem my comments in openshift/oc#439

pkg/image/registryclient/alternative-sources.go

smarterclayton · 2021-04-06T14:13:36Z

pkg/image/strategy/simplelookup.go

+
+	alternates []reference.DockerImageReference
+	icspFile   string
+	icspClient operatorv1alpha1client.ImageContentSourcePolicyInterface


This should be in a separate package as discussed before

cleaned up commits & cherry-picks

smarterclayton · 2021-04-06T14:14:57Z

pkg/image/registryclient/client.go

 	"github.com/docker/distribution/registry/client/auth/challenge"
 	"github.com/docker/distribution/registry/client/transport"
 	"github.com/opencontainers/go-digest"
-	imagereference "github.com/openshift/library-go/pkg/image/reference"


This commit merged already - surprised it still shows up here

i cleaned up the commits & cherry-picks, it's no longer there

sallyom · 2021-04-06T17:26:49Z

@smarterclayton the small modifications from your commits are in 0e622f2 , otherwise I picked yours and updated the commit msgs

sallyom · 2021-04-06T21:17:22Z

@ricardomaraschini can you please run with this PR to detect issues? Thank you, reach out on slack if needed.

openshift-ci-robot · 2021-04-08T11:54:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sallyom, soltysh
To complete the pull request process, please assign sttts after the PR has been reviewed.
You can assign the PR to them by writing /assign @sttts in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sallyom · 2021-04-08T13:06:11Z

I'm fine with this approach because I can make it work to solve the oc issue, but I prefer the simpler solution https://github.com/openshift/library-go/compare/master...sallyom:icsp_old?expand=1 integrated with oc. @ricardomaraschini afaict verified that solution works for image-registry, however, I don't know if this mirrored_client has been tested with image-registry. oc is not using the majority of the changes introduced here, so I cannot verify them.

ricardomaraschini · 2021-04-12T09:10:18Z

pkg/image/registryclient/client.go

+// is true, HTTP connections are allowed and HTTPS certificate verification errors will be ignored. The returned
+// Repository instance is threadsafe but the ManifestService, TagService, or BlobService are not. Note - the caller
+// is responsible for providing a valid registry url for docker.io - use RepositoryForRef() to avoid that.
+func (c *Context) Repository(ctx context.Context, registry *url.URL, repoName string, insecure bool) (RepositoryWithLocation, error) {


This change made *Context to stop implementing RepositoryRetriever interface. Was that expected?

Are you currently using that interface? I'm wondering if anybody uses it. I can update the interface though.

Yes, image registry repository leverages this interface in a few places.

sallyom · 2021-04-14T13:43:42Z

@ricardomaraschini I was able to add back to oc the ICSP logic, so I am now using more of the mirrored_client. Have you tried to run w/ it yet? I don't have it working quite yet, so I think perhaps something in the mirrored_client might need to be tweaked. Or, maybe I have to update my implementation of it in oc - if you have it working it will help! If not, I'll keep working on it.

sallyom · 2021-04-14T17:19:51Z

@smarterclayton @ricardomaraschini I have the ICSP stuff working w/ the mirrored_client now- using the *WithLocation fns, I'll push to the oc PR so you can check it out I have cleanup w/ that PR but it's working

ricardomaraschini · 2021-04-15T07:58:05Z

@ricardomaraschini I was able to add back to oc the ICSP logic, so I am now using more of the mirrored_client. Have you tried to run w/ it yet? I don't have it working quite yet, so I think perhaps something in the mirrored_client might need to be tweaked. Or, maybe I have to update my implementation of it in oc - if you have it working it will help! If not, I'll keep working on it.

I have managed to make it work this morning. The problem now (for the image registry) is the one related to the interface I mentioned here.

Other problem I can solve later on is the fact that simpleLookupICSP prefers to try the original repository first before moving to the mirrors and we would like to go for the mirrors first. I can solve that by providing a new implementation of AlternateBlobSourceStrategy.

Implements the core of the discussion around supporting multiple sources of input. Each wrapped method in the mirroredRepository structure that needs to check alternates (methods that invoke digest lookups) will use that structure. Methods that cannot be retried will use the first alternative that has a working client (such as ServeBlob). Methods that must always use the source (mutable calls or those that don't deal with a digest) will only look at the original source, although the logic is structured so we can use data about these requests in the future. The strategy is clarified to call either FirstRequest and OnFailure, or just FirstRequest. Each repository will make a single call to the strategy, and it's up to the strategy to include the original source when invoking FirstRequest.

sallyom · 2021-04-24T13:11:41Z

@ricardomaraschini I was able to add back to oc the ICSP logic, so I am now using more of the mirrored_client. Have you tried to run w/ it yet? I don't have it working quite yet, so I think perhaps something in the mirrored_client might need to be tweaked. Or, maybe I have to update my implementation of it in oc - if you have it working it will help! If not, I'll keep working on it.

I have managed to make it work this morning. The problem now (for the image registry) is the one related to the interface I mentioned here.

Other problem I can solve later on is the fact that simpleLookupICSP prefers to try the original repository first before moving to the mirrors and we would like to go for the mirrors first. I can solve that by providing a new implementation of AlternateBlobSourceStrategy.

Right, preferring the original allows oc to implicitly lookup alternate sources, but only if the original cannot be fetched. This is what is required of oc, but i see this is the opposite of image-registry. We can add another strategy in a follow-up PR, or, can keep the new strategy local to image-registry/not in lib-go, or, can add to this PR.

I updated the Repository method in the RepositoryRetriever interface, ptal.

ricardomaraschini · 2021-04-26T08:21:35Z

@ricardomaraschini I was able to add back to oc the ICSP logic, so I am now using more of the mirrored_client. Have you tried to run w/ it yet? I don't have it working quite yet, so I think perhaps something in the mirrored_client might need to be tweaked. Or, maybe I have to update my implementation of it in oc - if you have it working it will help! If not, I'll keep working on it.

I have managed to make it work this morning. The problem now (for the image registry) is the one related to the interface I mentioned here.
Other problem I can solve later on is the fact that simpleLookupICSP prefers to try the original repository first before moving to the mirrors and we would like to go for the mirrors first. I can solve that by providing a new implementation of AlternateBlobSourceStrategy.

Right, preferring the original allows oc to implicitly lookup alternate sources, but only if the original cannot be fetched. This is what is required of oc, but i see this is the opposite of image-registry. We can add another strategy in a follow-up PR, or, can keep the new strategy local to image-registry/not in lib-go, or, can add to this PR.

I updated the Repository method in the RepositoryRetriever interface, ptal.

We can do that in another PR for sure. As long as the interface has been updated I am OK with it :-)

ricardomaraschini · 2021-05-06T09:25:54Z

Could we get this in? I have dependent work waiting to be done in openshift/image-registry#267

smarterclayton · 2021-05-10T17:14:08Z

pkg/image/strategy/simplelookup.go

@@ -0,0 +1,159 @@
+package strategy


discussed with maciej:

move to oc (logic isn't generic to all callers and is more structured towards the "run once" behavior of oc, whereas controllers + registry are more informer driven)

needs to be "lookup once and cache" to match the general behavior of oc (at the time you invoke oc any assumptions are in place)

would like a unit test that verifies only one load is done of either file or client

smarterclayton · 2021-05-10T17:17:50Z

pkg/image/registryclient/client_test.go

@@ -1,8 +1,9 @@
 package registryclient

 import (


More extensive integration / unit style testing should happen in oc, which also helps us test the standard shared code. Mock testing and some basic integration testing can happen here, but oc will need to test the strategy coupled to the caller code.

Longer term there should probably a shim / stub / helper http serve function that makes it easier to stub out faking returning images (the hard bits) and inject errors and http status code. Can be longer term.

soltysh · 2021-05-20T19:22:09Z

Replaced with #1084
/close

openshift-ci · 2021-05-20T19:22:24Z

@soltysh: Closed this PR.

Details

In response to this:

Replaced with #1084
/close

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/test-infra repository.

openshift-ci-robot requested review from smarterclayton and soltysh November 6, 2020 16:30

sallyom mentioned this pull request Nov 6, 2020

Bug 1823143: Implement enhancement for "Add ImageContentSource awareness to oc" openshift/oc#439

Closed

openshift-ci-robot assigned dmage Nov 9, 2020

dmage suggested changes Nov 9, 2020

View reviewed changes

sallyom force-pushed the alternate-lookup-registry branch 2 times, most recently from 1bc35ad to 4c38c9d Compare November 9, 2020 16:25

smarterclayton reviewed Nov 9, 2020

View reviewed changes

pkg/image/registryclient/client.go Outdated Show resolved Hide resolved

sallyom force-pushed the alternate-lookup-registry branch 5 times, most recently from b8e5e9e to cae89d2 Compare November 12, 2020 07:07

dmage reviewed Nov 12, 2020

View reviewed changes

pkg/image/registryclient/client.go Outdated Show resolved Hide resolved

sallyom force-pushed the alternate-lookup-registry branch from 1c57118 to c343a9c Compare November 12, 2020 23:56

sallyom force-pushed the alternate-lookup-registry branch from c343a9c to 9d13369 Compare November 16, 2020 16:35

sallyom force-pushed the alternate-lookup-registry branch 4 times, most recently from a3bac30 to 43a99f6 Compare November 20, 2020 00:39

sallyom force-pushed the alternate-lookup-registry branch from 43a99f6 to f3ae645 Compare November 20, 2020 14:31

sallyom force-pushed the alternate-lookup-registry branch from f3ae645 to 043d5bf Compare December 3, 2020 04:59

sallyom changed the title ~~registry client: add interface for choosing repo from multiple image source options (from icsp, mirrored) rather than a single~~ registry client: Add AlternativeSources interface and methods to gather sources from ImageContentSourcePolicies Dec 3, 2020

soltysh suggested changes Dec 3, 2020

View reviewed changes

pkg/image/registryclient/alternative-sources.go Outdated Show resolved Hide resolved

pkg/image/registryclient/alternative-sources.go Outdated Show resolved Hide resolved

sallyom force-pushed the alternate-lookup-registry branch from 043d5bf to 84548ee Compare December 4, 2020 00:17

smarterclayton reviewed Apr 6, 2021

View reviewed changes

sallyom force-pushed the alternate-lookup-registry branch 2 times, most recently from b436f08 to 0e622f2 Compare April 6, 2021 17:23

sallyom force-pushed the alternate-lookup-registry branch from 0e622f2 to 4acafb6 Compare April 6, 2021 17:28

soltysh approved these changes Apr 8, 2021

View reviewed changes

ricardomaraschini reviewed Apr 12, 2021

View reviewed changes

smarterclayton and others added 6 commits April 22, 2021 12:25

Allow manifest location to be detected

5e02985

add simple lookup strategy for alternative image sources

7b995ef

bump(*): go mod tidy,vendor

b95085c

remove lock & errors for no alternates

5c7e884

update RepositoryRetriever

3defb0b

sallyom force-pushed the alternate-lookup-registry branch from 4acafb6 to 3defb0b Compare April 24, 2021 13:11

smarterclayton reviewed May 10, 2021

View reviewed changes

soltysh mentioned this pull request May 20, 2021

Alternate lookups #1084

Merged

openshift-ci bot closed this May 20, 2021

registry client: Add alternative sources methods to gather sources from ImageContentSourcePolicies #939

registry client: Add alternative sources methods to gather sources from ImageContentSourcePolicies #939

Uh oh!

Conversation

sallyom commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soltysh commented Nov 9, 2020

Uh oh!

dmage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sallyom commented Nov 13, 2020

Uh oh!

smarterclayton commented Nov 16, 2020

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sallyom commented Apr 6, 2021

Uh oh!

sallyom commented Apr 6, 2021

Uh oh!

openshift-ci-robot commented Apr 8, 2021

Uh oh!

sallyom commented Apr 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sallyom commented Apr 14, 2021

Uh oh!

sallyom commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricardomaraschini commented Apr 15, 2021

Uh oh!

sallyom commented Apr 24, 2021

Uh oh!

ricardomaraschini commented Apr 26, 2021

Uh oh!

ricardomaraschini commented May 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soltysh commented May 20, 2021

Uh oh!

openshift-ci bot commented May 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sallyom commented Nov 6, 2020 •

edited

Loading

sallyom commented Apr 14, 2021 •

edited

Loading