Plugin repository pinning by dmcgowan · Pull Request #28459 · moby/moby

dmcgowan · 2016-11-16T01:36:45Z

Add support for repository type pinning through authorization scope. This change sets the class in the authorization scope based on class of the repository. The repository info is updated to support setting a class which can be used by the registry client initialization.

Submitted early for design review, still blocked by upstream change distribution/distribution#2067. May be tested using registry built with that PR and the token server in distribution contrib.

aaronlehmann · 2016-11-16T02:25:02Z

Design LGTM

aaronlehmann · 2016-11-16T02:59:08Z

02:05:02 ----------------------------------------------------------------------
02:05:02 FAIL: docker_cli_events_test.go:278: DockerSuite.TestEventsPluginOps
02:05:02 
02:05:02 docker_cli_events_test.go:284:
02:05:02     dockerCmd(c, "plugin", "install", pluginName, "--grant-all-permissions")
02:05:02 docker_utils.go:505:
02:05:02     c.Assert(result, icmd.Matches, icmd.Success)
02:05:02 ... result *cmd.Result = &cmd.Result{Cmd:(*exec.Cmd)(0xc42024de40), ExitCode:1, Error:(*exec.ExitError)(0xc4234781c0), Timeout:false, outBuffer:(*cmd.lockedBuffer)(0xc4200915f0), errBuffer:(*cmd.lockedBuffer)(0xc420091680)} ("\nCommand: /go/src/github.com/docker/docker/bundles/1.14.0-dev/binary-client/docker plugin install tiborvass/no-remove:latest --grant-all-permissions\nExitCode: 1, Error: exit status 1\nStdout: \nStderr: Error response from daemon: repository tiborvass/no-remove not found: does not exist or no pull access\n\n")
02:05:02 ... expected cmd.Expected = cmd.Expected{ExitCode:0, Timeout:false, Error:"", Out:"", Err:""}
02:05:02 ... 
02:05:02 Command: /go/src/github.com/docker/docker/bundles/1.14.0-dev/binary-client/docker plugin install tiborvass/no-remove:latest --grant-all-permissions
02:05:02 ExitCode: 1, Error: exit status 1
02:05:02 Stdout: 
02:05:02 Stderr: Error response from daemon: repository tiborvass/no-remove not found: does not exist or no pull access
02:05:02 
02:05:02 
02:05:02 Failures:
02:05:02 ExitCode was 1 expected 0
02:05:02 Expected no error
02:05:02 
02:05:02 
02:05:03 
02:05:03 ----------------------------------------------------------------------

Distribution client change for class in resource Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)

Expose registry error translation for plugin distribution Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)

thaJeztah · 2016-11-23T00:37:04Z

ping @anusha-ragunathan @vieux is this needed for 1.13?

dmcgowan · 2016-11-23T01:06:25Z

@thaJeztah this is intended for 1.13. Still getting hub changes propagated to get tests passing.

thaJeztah · 2016-11-23T01:09:53Z

Thanks! Adding to the milestone

LK4D4 · 2016-11-30T19:41:44Z

@dmcgowan CI fails are definitely related.

dmcgowan · 2016-11-30T21:33:17Z

@LK4D4 I understand that, unfortunately these tests are still relying on the hub which required an update in order to pass. Looks like that update has gone out and I will have the tests re-run. Since testing this change requires a token server I added tests in the distribution integration tests to verify, although it is not yet run by CI since it is not in a Docker release yet. Feel free to have a look at them here distribution/distribution#2085 🐔 🥚

dmcgowan · 2016-11-30T23:34:07Z

CI is now passing, PTAL

dmcgowan · 2016-12-02T17:51:10Z

Ping @aaronlehmann @stevvooe

aaronlehmann · 2016-12-02T18:41:50Z

LGTM

stevvooe · 2016-12-02T19:05:04Z

distribution/registry.go

+			Actions:    actions,
+		}
+
+		// Keep image repositories blank for scope compatibility


This doesn't seem quite right. The field should be ignored by old registries. Shouldn't it just be set?

This is to keep the change isolated to plugins for now. By setting this value any token server would require being updated in order to handle plugins, while this change allows existing token servers to work without supporting the specification update. Older registries will handle additional fields in the JSON just fine but this gets sent to the token server first.

Shouldn't we just start sending the field anyways? If we keep this isolated to plugins, won't that just continue to propagate that these are special-cased?

This RepositoryScope is from the client auth package, this does not get serialized to JSON but to https://github.com/docker/distribution/blob/master/docs/spec/auth/scope.md#resource-scope-grammar. So it is not just a matter of ignoring the field, but parsing the scope according to the updated grammar. We cannot require authorization servers immediately support the expanded grammar. We probably need a method to inform the authorization servers and give them time to support it though for a future release.

I see. Shouldn't we hide those details behind the token manager? How do we intend to resolve support for this when we add it to repositories?

It is reasonable to hide those details, the reason I didn't in this case is because if we were to hide that then the scope type would end up having that check here https://github.com/docker/distribution/blob/master/registry/client/auth/session.go#L158. However I compared this change to update from GET to POST which we did hide behind the interface to enforce backwards compatibility.

stevvooe · 2016-12-02T19:06:05Z

Code looks good.

I am worried about not setting the Class field. Extra fields should generally be ignored on older registries.

stevvooe · 2016-12-05T23:03:16Z

LGTM

icecrime · 2016-12-07T21:21:18Z

Ping @vieux for merge.

vieux · 2016-12-08T19:28:41Z

LGTM

GordonTheTurtle added the status/0-triage label Nov 16, 2016

dmcgowan added status/1-design-review and removed status/0-triage labels Nov 16, 2016

aaronlehmann added the status/failing-ci Indicates that the PR in its current state fails the test suite label Nov 16, 2016

dmcgowan added 2 commits November 21, 2016 22:18

Update vendor distribution

d1f5e0f

Distribution client change for class in resource Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)

Add class to repository scope

a12b466

Expose registry error translation for plugin distribution Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)

dmcgowan force-pushed the plugin-repository-pinning branch from 5092196 to a12b466 Compare November 22, 2016 06:19

thaJeztah added this to the 1.13.0 milestone Nov 23, 2016

thaJeztah added status/2-code-review and removed status/1-design-review labels Nov 23, 2016

dmcgowan added rebuild/janky and removed rebuild/experimental status/failing-ci Indicates that the PR in its current state fails the test suite labels Nov 30, 2016

stevvooe reviewed Dec 2, 2016

View reviewed changes

icecrime added the process/cherry-pick label Dec 6, 2016

vieux merged commit c1a1b38 into moby:master Dec 8, 2016

vieux added process/cherry-picked and removed process/cherry-pick labels Dec 9, 2016

vieux mentioned this pull request Dec 9, 2016

1.13.0-rc4 cherry-picks: part1 #29229

Merged

thaJeztah mentioned this pull request Nov 30, 2024

registry: deprecate RepositoryInfo.Class #49006

Merged

vvoland mentioned this pull request Dec 2, 2024

[27.x backport] registry: deprecate RepositoryInfo.Class #49013

Merged

Conversation

dmcgowan commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronlehmann commented Nov 16, 2016

Uh oh!

aaronlehmann commented Nov 16, 2016

Uh oh!

thaJeztah commented Nov 23, 2016

Uh oh!

dmcgowan commented Nov 23, 2016

Uh oh!

thaJeztah commented Nov 23, 2016

Uh oh!

LK4D4 commented Nov 30, 2016

Uh oh!

dmcgowan commented Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmcgowan commented Nov 30, 2016

Uh oh!

dmcgowan commented Dec 2, 2016

Uh oh!

aaronlehmann commented Dec 2, 2016

Uh oh!

stevvooe Dec 2, 2016

Choose a reason for hiding this comment

Uh oh!

dmcgowan Dec 2, 2016

Choose a reason for hiding this comment

Uh oh!

stevvooe Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

dmcgowan Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

stevvooe Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

dmcgowan Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Dec 2, 2016

Uh oh!

stevvooe commented Dec 5, 2016

Uh oh!

icecrime commented Dec 7, 2016

Uh oh!

vieux commented Dec 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dmcgowan commented Nov 16, 2016 •

edited

Loading

dmcgowan commented Nov 30, 2016 •

edited

Loading