8000 Plugin repository pinning by dmcgowan · Pull Request #28459 · moby/moby · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Plugin repository pinning #28459

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 8, 2016
Merged

Plugin repository pinning #28459

merged 2 commits into from
Dec 8, 2016

Conversation

dmcgowan
Copy link
Member
@dmcgowan dmcgowan commented Nov 16, 2016 •
edited
Loading

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
Copy link
Contributor

Design LGTM

@aaronlehmann
Copy link
Contributor 
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 ----------------------------------------------------------------------

@aaronlehmann aaronlehmann added the status/failing-ci Indicates that the PR in its current state fails the test suite label Nov 16, 2016
@dmcgowan
Update vendor distribution
d1f5e0f 
Distribution client change for class in resource

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
@dmcgowan
Add class to repository scope
a12b466 
Expose registry error translation for plugin distribution

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
@dmcgowan dmcgowan force-pushed the plugin-repository-pinning branch from 5092196 to a12b466 Compare November 22, 2016 06:19
@thaJeztah
Copy link
Member

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

@dmcgowan
Copy link
Member Author

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

@thaJeztah thaJeztah added this to the 1.13.0 milestone Nov 23, 2016
@thaJeztah
Copy link
Member

Thanks! Adding to the milestone

@LK4D4
Copy link
Contributor
LK4D4 commented Nov 30, 2016

@dmcgowan CI fails are definitely related.

@dmcgowan
Copy link
Member Author
dmcgowan commented Nov 30, 2016 •
edited
Loading

@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 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
@dmcgowan
Copy link
Member Author

CI is now passing, PTAL

@dmcgowan
Copy link
Member Author
dmcgowan commented Dec 2, 2016

Ping @aaronlehmann @stevvooe

@aaronlehmann
Copy link
Contributor

LGTM

stevvooe
stevvooe reviewed Dec 2, 2016
View reviewed changes
distribution/registry.go
Actions: actions,
}

// Keep image repositories blank for scope compatibility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 8000 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor
stevvooe commented Dec 2, 2016

Code looks good.

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

@stevvooe
Copy link
Contributor
stevvooe commented Dec 5, 2016

LGTM

@icecrime
Copy link
Contributor
icecrime commented Dec 7, 2016

Ping @vieux for merge.

@vieux
Copy link
Contributor
vieux commented Dec 8, 2016

LGTM

@vieux vieux merged commit c1a1b38 into moby:master Dec 8, 2016
@vieux vieux 67E6 mentioned this pull request Dec 9, 2016
Merged
@thaJeztah thaJeztah mentioned this pull request Nov 30, 2024
Merged
@vvoland vvoland mentioned this pull request Dec 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevvooe stevvooe stevvooe left review comments

Assignees
No one assigned
Labels
process/cherry-picked status/2-code-review
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.
8 participants
@dmcgowan @aaronlehmann @thaJeztah @LK4D4 @stevvooe @icecrime @vieux @GordonTheTurtle
0