Verify manifest with remote layers #13001

legionus · 2017-02-17T16:49:29Z

Problem
We pass all requests (including HEAD) to the remote service if the pullthrough is enabled. On the other hand when docker client pushes the manifest we check the presence of all these layers locally. The client checks a blob existance by HEAD request before sending it to the server.

If client image is based on the imported image (but not present in local registry) dockerregistry will say that it has all the layers from the base image. In this case docker client never send them to server, but manifest verification requires them locally. It means that the verification will always fail for remote layers.

Solution
Manifest verification must to take into account the possibility that the layers may not be local and check them on remote registry server before before give up.

We can't use pullthroughBlobStore because verification happens in ManifestService. So we need to move common code that gets the blobs from the remote server to BlobGetterService and use it for pullthrough and for verification.

@miminar PTAL

legionus · 2017-02-17T16:50:48Z

[test]

stevekuznetsov · 2017-02-17T19:49:32Z

test/end-to-end/core.sh

+os::cmd::expect_success "oc login -u user0 -p pass"
+os::cmd::expect_success "oc new-project project0"
+os::cmd::expect_success "oc new-build -D \$'FROM busybox:glibc\nRUN echo abc >/tmp/foo'"
+os::cmd::try_until_success "oc get build/busybox-1 -o go-template={{.metadata.name}} 2>&1 |grep busybox-1"


os::cmd::try_until_text "oc get build/busybox-1 -o 'jsonpath={.metadata.name}'" "busybox-1"

stevekuznetsov · 2017-02-17T19:51:14Z

test/end-to-end/core.sh

+os::cmd::expect_success "oc new-project project0"
+os::cmd::expect_success "oc new-build -D \$'FROM busybox:glibc\nRUN echo abc >/tmp/foo'"
+os::cmd::try_until_success "oc get build/busybox-1 -o go-template={{.metadata.name}} 2>&1 |grep busybox-1"
+os::cmd::try_until_not_text "oc get build/busybox-1 -o go-template='{{.status.phase}}' 2>&1" "^(Pending|Running)\$" "$(( 10*TIME_MIN ))"


os::cmd::try_until_not_text "oc get build/busybox-1 -o 'jsonpath={.status.phase}'" "(Pending|Running)" "$(( 10*TIME_MIN ))"

Although -- this seems a little weird, if we follow this up with expecting Complete we should just do one wait

stevekuznetsov · 2017-02-17T19:52:37Z

test/end-to-end/core.sh

+os::cmd::expect_success "oc new-build -D \$'FROM busybox:glibc\nRUN echo abc >/tmp/foo'"
+os::cmd::try_until_success "oc get build/busybox-1 -o go-template={{.metadata.name}} 2>&1 |grep busybox-1"
+os::cmd::try_until_not_text "oc get build/busybox-1 -o go-template='{{.status.phase}}' 2>&1" "^(Pending|Running)\$" "$(( 10*TIME_MIN ))"
+os::cmd::expect_success_and_text "oc get build/busybox-1 -o go-template='{{.status.phase}}'" '^Complete$'


os::cmd::try_until_text "oc get build/busybox-1 -o jsonpath='{.status.phase}'" 'Complete' "$(( 10*TIME_MIN ))"

@stevekuznetsov

Just after oc new-build ... output of oc get builds looks like this:

RUN: oc get builds No resources found. RUN: oc get builds NAME TYPE FROM STATUS STARTED DURATION busybox-1 Docker Dockerfile Pending RUN: oc get builds NAME TYPE FROM STATUS STARTED DURATION busybox-1 Docker Dockerfile Pending RUN: oc get builds NAME TYPE FROM STATUS STARTED DURATION busybox-1 Docker Dockerfile Running 1 second ago RUN: oc get builds NAME TYPE FROM STATUS STARTED DURATION busybox-1 Docker Dockerfile Complete 2 seconds ago 1s

If I leave only the os::cmd::try_until_text "oc get build/busybox-1 -o jsonpath='{.status.phase}'" 'Complete' "$(( 10*TIME_MIN ))", in case of an error, the test will wait a very long time.

In my test first os::cmd::try_until_success waits for the appearance of the build. Second os::cmd::try_until_not_text waits for any completion of build. And the last os::cmd::expect_success_and_text checks status of build.

/cc @mfojtik

@stevekuznetsov @legionus comment makes sense to me :-) we don't want to wait 10 minutes in case this test fails ;-)

I think we almost always just take the long way -- see also the conformance timeout. It's more readable and expresses your intent better. I don't feel too strongly.

soltysh · 2017-02-20T11:53:29Z

The changes lgtm, although the test error worries me, since it failed on pullthrough test, specifically and this PR modifies that part. Aside from that Steve's comments needs addressing.

miminar · 2017-02-20T11:41:02Z

pkg/dockerregistry/server/remoteblobgetter.go

+	"github.com/openshift/origin/pkg/image/importer"
+)
+
+type BlobGetterService interface {


miminar · 2017-02-20T11:41:08Z

pkg/dockerregistry/server/remoteblobgetter.go

+	distribution.BlobServer
+}
+
+type remoteBlobGetterService struct {


miminar · 2017-02-20T11:52:34Z

test/end-to-end/core.sh

+os::cmd::expect_success 'oc rollout status dc/docker-registry'
+os::log::info "Registry configured to disble mirroring"
+
+# verify local image based on remote image can be pushed to local registry


No need for the comment since there's an info line just below. Though I like the text in the comment more.

miminar · 2017-02-20T11:53:09Z

test/end-to-end/core.sh

+os::log::info "Configure registry to disable mirroring"
+os::cmd::expect_success "oc project '${CLUSTER_ADMIN_CONTEXT}'"
+os::cmd::expect_success 'oc env -n default dc/docker-registry REGISTRY_MIDDLEWARE_REPOSITORY_OPENSHIFT_MIRRORPULLTHROUGH=false'
+os::cmd::expect_success 'oc env -n default dc/docker-registry REGISTRY_MIDDLEWARE_REPOSITORY_OPENSHIFT_PULLTHROUGH=true'


This holds true by default.

miminar · 2017-02-20T12:00:23Z

test/end-to-end/core.sh

+os::cmd::try_until_success "oc get build/busybox-1 -o go-template={{.metadata.name}} 2>&1 |grep busybox-1"
+os::cmd::try_until_not_text "oc get build/busybox-1 -o go-template='{{.status.phase}}' 2>&1" "^(Pending|Running)\$" "$(( 10*TIME_MIN ))"
+os::cmd::expect_success_and_text "oc get build/busybox-1 -o go-template='{{.status.phase}}'" '^Complete$'
+


Can you also ensure the istag exists?

miminar · 2017-02-20T12:18:21Z

pkg/dockerregistry/server/pullthroughblobstore.go

@@ -123,130 +54,57 @@ func (r *pullthroughBlobStore) proxyStat(ctx context.Context, retriever importer
 // success response with no actual body content.
 // [1] https://docs.docker.com/registry/spec/api/#existing-layers
 func (pbs *pullthroughBlobStore) ServeBlob(ctx context.Context, w http.ResponseWriter, req *http.Request, dgst digest.Digest) error {
-	store, ok := pbs.digestToStore[dgst.String()]
-	if !ok {
+	if _, err := pbs.BlobStore.Stat(ctx, dgst); err == nil {


There's no need for the Stat. It's the first thing the pbs.BlobStore.ServeBlob does. Just handle the error case.

@miminar I know, but we need to figure out is this blob local or not to start mirroring. I need to change mirroring code to make the changes that you suggest. I don't want to do this before the release. The changes will be very large.

@miminar yep. You're right.

miminar · 2017-02-20T12:20:18Z

pkg/dockerregistry/server/pullthroughblobstore.go

+	if !found {
+		err := fmt.Errorf("pullthroughBlobStore.ServeBlob: failed to retrieve remote getter from context")
+		context.GetLogger(ctx).Error(err)
+		return err


Don't expose implementation details to clients (pullthroughBlobStore.ServeBlob: ).

@miminar Otherwise it is very difficult to understand where the error occurs.

I'm fine with logging it. I'm not fine with returning it as an error.

I've just accidentally commented from my private account. Nevertheless, I completely agree with my alter-ego.

miminar · 2017-02-20T12:25:43Z

pkg/dockerregistry/server/blobdescriptorservice.go

 		return desc, nil
 	}

+	// Second attemt: looking for the blob on a remote server


miminar · 2017-02-20T12:28:12Z

pkg/dockerregistry/server/repositorymiddleware.go

@@ -205,6 +205,13 @@ func newRepositoryWithClient(

 // Manifests returns r, which implements distribution.ManifestService.
 func (r *repository) Manifests(ctx context.Context, options ...distribution.ManifestServiceOption) (distribution.ManifestService, error) {
+	if r.pullthrough {
+		ctx = WithRemoteBlobGetter(ctx, &remoteBlobGetterService{


Could you explain in a comment, why this is needed here?

miminar · 2017-02-20T12:28:32Z

pkg/dockerregistry/server/repositorymiddleware.go

@@ -243,6 +250,13 @@ func (r *repository) Blobs(ctx context.Context) distribution.BlobStore {
 	repo := repository(*r)
 	repo.ctx = ctx

+	if r.pullthrough {
+		ctx = WithRemoteBlobGetter(ctx, &remoteBlobGetterService{


And again here.

legionus · 2017-02-20T17:05:13Z

@miminar all your comments addressed.

miminar · 2017-02-21T09:55:25Z

pkg/dockerregistry/server/pullthroughblobstore.go

+	if !found {
+		err := fmt.Errorf("pullthroughBlobStore.ServeBlob: failed to retrieve remote getter from context")
+		context.GetLogger(ctx).Error(err)
+		return err


Could this return ErrBlobUnknown as well?

miminar · 2017-02-21T10:04:49Z

pkg/dockerregistry/server/repositorymiddleware.go

+	if r.pullthrough {
+		// Add to the context the BlobGetterService that provide access to remote servers.
+		// It will be used to validate manifest blobs. It only makes sense
+		// if the pullthrough is enabled.


Mention that it needs to be instantiated here in order to share the cache among different stat calls made on manifest's dependencies.

miminar · 2017-02-21T10:07:02Z

pkg/dockerregistry/server/repositorymiddleware.go

+
+	if r.pullthrough {
+		// Add to the context the BlobGetterService that provide access to remote servers.
+		// It will be used in pullthroughBlobStore.


Mention that it needs to be instantiated here in order to share the cache for multiple stat calls made in descendant blob stores.

miminar · 2017-02-21T16:22:12Z

pkg/dockerregistry/server/blobdescriptorservice.go

-		return desc, err
+	// only non-empty layers is wise to check for existence in the image stream.
+	// schema v2 has no empty layers.
+	if dgst.String() != digestSHA256GzippedEmptyTar.String() {


What about digest.DigestSha256EmptyTar?

miminar · 2017-02-21T16:26:26Z

pkg/dockerregistry/server/blobdescriptorservice.go

+	if dgst.String() != digestSHA256GzippedEmptyTar.String() {
+		// ensure it's referenced inside of corresponding image stream
+		if !imageStreamHasBlob(repo, dgst) {
+			return distribution.Descriptor{}, distribution.ErrBlobUnknown


This needs to fall-back to remote stat.

Consider:

empty image stream ns/foo

remote manifest with layers {A} tagged as docker.io/foo:v1

remote manifest with layers {B} tagged as docker.io/foo:v2

and following scenario:

oc import-image --from=docker.io/foo:v1 foo:v1 --confirm docker pull docker.io/foo:v2 docker tag docker.io/foo:v2 "${integrated_registry_url}/ns/foo:v2" docker push "${integrated_registry_url}/ns/foo:v2"

This will fail because HEAD on B in ns/foo will succeed thanks to pullthrough. However, manifest verification will fail because B is not tagged in the image stream and it's not an empty layer.

@miminar It means that imageStreamHasBlob should be called only for local blobs ?
It seems you have me confused.

@legionus yes, that's what I meant.

miminar · 2017-02-21T16:39:52Z

test/end-to-end/core.sh

+os::cmd::expect_success "oc project '${CLUSTER_ADMIN_CONTEXT}'"
+os::cmd::expect_success 'oc env -n default dc/docker-registry REGISTRY_MIDDLEWARE_REPOSITORY_OPENSHIFT_MIRRORPULLTHROUGH=false'
+os::cmd::expect_success 'oc rollout status dc/docker-registry'
+os::log::info "Registry configured to disble mirroring"


miminar · 2017-02-21T16:40:09Z

test/end-to-end/core.sh

+os::cmd::expect_success 'oc rollout status dc/docker-registry'
+os::log::info "Registry configured to disble mirroring"
+
+os::log::info "Check the image based on a non-local base image."


s/non-local/remote/

Could this be more verbose? E.g.: Verify that an image based on a remote image can be pushed to the same image stream while pull-through enabled

miminar · 2017-02-21T16:43:59Z

test/end-to-end/core.sh

+os::log::info "Check the image based on a non-local base image."
+os::cmd::expect_success "oc login -u user0 -p pass"
+os::cmd::expect_success "oc new-project project0"
+os::cmd::expect_success "oc new-build -D \$'FROM busybox:glibc\nRUN echo abc >/tmp/foo'"


Please add a comment that this actually creates a source repository busybox for the source image and that the result will be pushed to the same repository.

bparees · 2017-02-22T15:16:01Z

either flake #13050 or this change is now causing that test to fail.

legionus · 2017-02-22T15:50:22Z

@bparees It is not #13047 ?

bparees · 2017-02-22T15:56:51Z

@legionus there were multiple failures. some of them were 13047, but the networking one didn't appear to be related to that, no.

miminar

LGTM

mfojtik · 2017-02-22T20:27:36Z

[test]

legionus · 2017-02-22T22:25:13Z

flake #12899

legionus · 2017-02-22T22:25:23Z

[test]

mfojtik · 2017-02-23T09:37:59Z

[test]

legionus · 2017-02-23T11:08:13Z

@mfojtik \o/
merge?

stevekuznetsov

You have not addressed any of my comments on your tests. Please do so.

legionus · 2017-02-23T13:09:36Z

@stevekuznetsov I believe that the proposed changes are wrong, but I made them. At least I know who to blame for the slow test.

please review again.

stevekuznetsov · 2017-02-23T13:15:58Z

I was OK with the way you had the wait written. I said as much in the comments. It was the other comments re: go-template, etc that I want to see addressed.

legionus · 2017-02-23T14:17:26Z

@stevekuznetsov Oh. I didn't understand you correctly. Now looks better ?

legionus · 2017-02-23T15:50:31Z

flake #12923 re[test]

mfojtik · 2017-02-23T21:35:05Z

@legionus can you rebase against master? @bparees fixed the issue that is causing fork_ami to fail, but for some reason I can see it happening for your branch... which is strange, because I pick "master" as the branch to rebase your PR again...

Signed-off-by: Gladkov Alexey <agladkov@redhat.com>

legionus · 2017-02-24T10:24:57Z

@mfojtik done

openshift-bot · 2017-02-24T10:29:28Z

Evaluated for origin test up to b4718ae

openshift-bot · 2017-02-24T11:46:50Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/536/) (Base Commit: f1dd3c7)

legionus · 2017-02-24T13:05:18Z

Made https://ci.openshift.redhat.com/jenkins/job/fork_ami/346/

mfojtik · 2017-02-27T09:05:01Z

QA verified the fork_ami

[merge]

openshift-bot · 2017-02-27T09:05:22Z

Evaluated for origin merge up to b4718ae

openshift-bot · 2017-02-27T09:05:34Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/571/) (Base Commit: 5357357) (Image: devenv-rhel7_5985)

legionus added component/imageregistry priority/P1 labels Feb 17, 2017

legionus self-assigned this Feb 17, 2017

legionus force-pushed the dockerregistry-remote-blob-verification branch from cf089ab to aea2c6c Compare February 17, 2017 16:50

stevekuznetsov suggested changes Feb 17, 2017

View reviewed changes

miminar suggested changes Feb 20, 2017

View reviewed changes

legionus force-pushed the dockerregistry-remote-blob-verification branch 2 times, most recently from 07c126e to 9874c50 Compare February 20, 2017 17:03

miminar suggested changes Feb 21, 2017

View reviewed changes

legionus force-pushed the dockerregistry-remote-blob-verification branch from 9874c50 to eed05d4 Compare February 21, 2017 13:55

miminar suggested changes Feb 21, 2017

View reviewed changes

legionus force-pushed the dockerregistry-remote-blob-verification branch 3 times, most recently from b204966 to ecc925b Compare February 22, 2017 13:54

bparees mentioned this pull request Feb 22, 2017

Extended.[networking][router] weighted openshift router The HAProxy router should appropriately serve a route that points to two services #13050

Closed

legionus force-pushed the dockerregistry-remote-blob-verification branch from ecc925b to e2f3983 Compare February 22, 2017 15:46

miminar approved these changes Feb 22, 2017

View reviewed changes

stevekuznetsov suggested changes Feb 23, 2017

View reviewed changes

legionus force-pushed the dockerregistry-remote-blob-verification branch from e2f3983 to 462bd16 Compare February 23, 2017 13:05

legionus force-pushed the dockerregistry-remote-blob-verification branch from 462bd16 to 82db14d Compare February 23, 2017 14:15

stevekuznetsov approved these changes Feb 23, 2017

View reviewed changes

Verify manifest with remote layers

b4718ae

Signed-off-by: Gladkov Alexey <agladkov@redhat.com>

legionus force-pushed the dockerregistry-remote-blob-verification branch from 82db14d to b4718ae Compare February 24, 2017 10:24

legionus mentioned this pull request Feb 24, 2017

[3.5] Verify manifest with remote layers #13099

Merged

openshift-bot merged commit 6789b13 into openshift:master Feb 27, 2017

legionus mentioned this pull request Feb 27, 2017

[3.4] Verify manifest with remote layers #13120

Merged

bparees mentioned this pull request Feb 27, 2017

Extended.[builds][Slow] build can have Dockerfile input being created from new-build should create a image via new-build and infer the origin tag #12329

Closed

legionus mentioned this pull request Mar 2, 2017

[3.3] Verify manifest with remote layers #13193

Closed

andig mentioned this pull request Feb 23, 2020

OSX: manifest blob unknown: blob unknown to registry pushing linux image distribution/distribution#3100

Open

Verify manifest with remote layers #13001

Verify manifest with remote layers #13001

Conversation

legionus commented Feb 17, 2017

legionus commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legionus commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Feb 22, 2017

legionus commented Feb 22, 2017

bparees commented Feb 22, 2017

miminar left a comment

Choose a reason for hiding this comment

mfojtik commented Feb 22, 2017

legionus commented Feb 22, 2017

legionus commented Feb 22, 2017

mfojtik commented Feb 23, 2017

legionus commented Feb 23, 2017

stevekuznetsov left a comment

Choose a reason for hiding this comment

legionus commented Feb 23, 2017

stevekuznetsov commented Feb 23, 2017

legionus commented Feb 23, 2017

legionus commented Feb 23, 2017

mfojtik commented Feb 23, 2017

legionus commented Feb 24, 2017

openshift-bot commented Feb 24, 2017

openshift-bot commented Feb 24, 2017

legionus commented Feb 24, 2017

mfojtik commented Feb 27, 2017

openshift-bot commented Feb 27, 2017

openshift-bot commented Feb 27, 2017 • edited Loading

openshift-bot commented Feb 27, 2017 •

edited

Loading