Verify layer sizes in the integrated registry #16885

dmage · 2017-10-16T16:20:22Z

cc @miminar @legionus

fixes bug https://bugzilla.redhat.com/show_bug.cgi?id=1491589

bparees · 2017-10-17T16:44:26Z

@dmage is this prepared for a review?

dmage · 2017-10-17T19:17:46Z

@bparees I’m going to add some code to test this fix. Now it has a bug.

dmage · 2017-10-23T21:48:00Z

/retest

miminar · 2017-10-24T08:13:36Z

/test extended_image_registry

miminar

LGTM otherwise.

miminar · 2017-10-24T08:28:29Z

pkg/dockerregistry/server/manifestschema2handler.go

-				_, err = repo.Blobs(ctx).Stat(ctx, fsLayer.Digest)
+				desc, err = repo.Blobs(ctx).Stat(ctx, fsLayer.Digest)
+				if err == nil && fsLayer.Size != desc.Size {
+					errs = append(errs, ErrManifestBlobBadSize{


The err is appended twice.

🤦‍♂️ sorry, I guess I wasn't reading.

miminar · 2017-10-24T08:48:17Z

pkg/dockerregistry/server/manifestschema2handler.go


 	target := h.manifest.Target()
-	_, err := repo.Blobs(ctx).Stat(ctx, target.Digest)
+	desc, err := repo.Blobs(ctx).Stat(ctx, target.Digest)


The Target() is already verified within .References() loop.

sounds like we can delete this whole block since .References() will include the digest.

bparees · 2017-10-24T10:00:23Z

pkg/cmd/dockerregistry/dockerregistry.go

+	}
+}
+
+// Start runs the Docker registry. Start always returns a non-nil error.


this comment makes it sounds like Start will never return nil, but i don't think that's what you mean, is it?

It will never return nil. It acts like http.ListenAndServe.

bparees · 2017-10-24T13:36:05Z

pkg/image/util/helpers.go


 	switch manifest.SchemaVersion {
-	case 0:
-		// legacy config object


on what basis are we confident/safe to remove this?

This is for Docker before v1.3.0. Our code relies on DockerImageMetadata and this function for will leave it unfilled for schema 0 without any errors. We need to fix our code if we still want to have support for schema 0.

bparees · 2017-10-24T13:44:46Z

@dmage sounds like a couple minor nits and we can get this on the merge queue today.

openshift-ci-robot · 2017-10-24T15:27:57Z

@michojel: changing LGTM is restricted to assignees, and only openshift org members may be assigned issues.

Details

In response to this:

/lgtm

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.

miminar · 2017-10-24T15:28:30Z

/lgtm

miminar · 2017-10-24T15:29:02Z

/test extended_image_registry
/assign @bparees
for the final approval

legionus · 2017-10-24T15:31:16Z

/lgtm

bparees · 2017-10-24T15:31:52Z

/lgtm

openshift-bot · 2017-10-24T19:23:58Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-10-25T01:26:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-10-25T07:29:58Z

/retest

Please review the full test history for this PR and help us cut down flakes.

legionus · 2017-10-25T12:31:12Z

/lgtm

openshift-merge-robot · 2017-10-25T12:31:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, dmage, legionus, miminar

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

Details

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cmd/dockerregistry/OWNERS~~ [bparees,dmage,legionus,miminar]
~~pkg/dockerregistry/OWNERS~~ [bparees,dmage,legionus,miminar]
~~pkg/image/OWNERS~~ [bparees]
~~test/integration/OWNERS~~ [bparees]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-10-25T14:12:02Z

Automatic merge from submit-queue.

openshift-merge-robot assigned smarterclayton and bparees Oct 16, 2017

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 16, 2017

dmage changed the title ~~Verify layer sizes in the integrated registry~~ [WIP] Verify layer sizes in the integrated registry Oct 16, 2017

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2017

bparees mentioned this pull request Oct 18, 2017

registry: Calculate schema 1 layer sizes in the registry #16776

Closed

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 20, 2017

dmage force-pushed the layers-metadata branch 2 times, most recently from cd8e7dd to 4f98282 Compare October 23, 2017 16:41

dmage changed the title ~~[WIP] Verify layer sizes in the integrated registry~~ Verify layer sizes in the integrated registry Oct 23, 2017

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 23, 2017

dmage force-pushed the layers-metadata branch from 4f98282 to 166ca8c Compare October 23, 2017 16:47

miminar reviewed Oct 24, 2017

View reviewed changes

bparees reviewed Oct 24, 2017

View reviewed changes

dmage force-pushed the layers-metadata branch from 166ca8c to 0a77353 Compare October 24, 2017 15:21

openshift-ci-robot assigned miminar Oct 24, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2017

legionus approved these changes Oct 24, 2017

View reviewed changes

openshift-ci-robot assigned legionus Oct 24, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2017

bparees added the kind/bug Categorizes issue or PR as related to a bug. label Oct 24, 2017

Verify layer sizes in the integrated registry

fd23261

dmage force-pushed the layers-metadata branch from 0a77353 to fd23261 Compare October 25, 2017 10:05

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2017

openshift-merge-robot merged commit f1f5221 into openshift:master Oct 25, 2017

Verify layer sizes in the integrated registry #16885

Verify layer sizes in the integrated registry #16885

Uh oh!

Conversation

dmage commented Oct 16, 2017 • edited by bparees Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bparees commented Oct 17, 2017

Uh oh!

dmage commented Oct 17, 2017

Uh oh!

dmage commented Oct 23, 2017

Uh oh!

miminar commented Oct 24, 2017

Uh oh!

miminar left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmage Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparees commented Oct 24, 2017

Uh oh!

openshift-ci-robot commented Oct 24, 2017

Uh oh!

miminar commented Oct 24, 2017

Uh oh!

miminar commented Oct 24, 2017

Uh oh!

legionus commented Oct 24, 2017

Uh oh!

bparees commented Oct 24, 2017

Uh oh!

openshift-bot commented Oct 24, 2017

Uh oh!

openshift-bot commented Oct 25, 2017

Uh oh!

openshift-bot commented Oct 25, 2017

Uh oh!

legionus commented Oct 25, 2017

Uh oh!

openshift-merge-robot commented Oct 25, 2017

Uh oh!

openshift-merge-robot commented Oct 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dmage commented Oct 16, 2017 •

edited by bparees

Loading

dmage Oct 24, 2017 •

edited

Loading