Skip to content

Update Docker resolver to pass in Authorizer interface#2690

Merged
estesp merged 1 commit intocontainerd:masterfrom
dmcgowan:resolver-updates
Oct 3, 2018
Merged

Update Docker resolver to pass in Authorizer interface#2690
estesp merged 1 commit intocontainerd:masterfrom
dmcgowan:resolver-updates

Conversation

@dmcgowan
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan commented Sep 28, 2018

This creates an authorizer interface which can be shared across multiple requests and more configurable for setting the authorization. This fixes a bug today which doesn't case sharing of tokens between resolve and fetch due to creation of a different base object which caches. Additionally this change moves the implementation closer to being focused on the OCI distribution specification while keeping the Docker specific authorization as a separate component.

An additional change can be done later to remove the credentials function after the callers have updated. There is also an additional change to allow the host/scheme specification to be more robust and have better support for mirrored configuration without needing to create multiple instances for each host. I wanted to keep that separate from the authorization component.

Fixes #1005
Fixes #2683

@estesp
Copy link
Copy Markdown
Member

estesp commented Sep 28, 2018

Something not quite right:

failed to resolve reference "docker.io/library/alpine:latest": unexpected status code https://registry-1.docker.io/v2/library/alpine/manifests/latest: 401 Unauthorized

@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 28, 2018

Codecov Report

Merging #2690 into master will increase coverage by 0.08%.
The diff coverage is 68.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2690      +/-   ##
==========================================
+ Coverage   43.07%   43.16%   +0.08%     
==========================================
  Files         100      101       +1     
  Lines       10606    10639      +33     
==========================================
+ Hits         4569     4592      +23     
- Misses       5316     5323       +7     
- Partials      721      724       +3
Flag Coverage Δ
#linux 47.15% <71.42%> (+0.05%) ⬆️
#windows 40.34% <68.58%> (+0.1%) ⬆️
Impacted Files Coverage Δ
remotes/docker/authorizer.go 66.49% <66.49%> (ø)
remotes/docker/resolver.go 58.36% <81.25%> (-2.97%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac01f20...a6198b7. Read the comment docs.

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this just write the header into the request? There may be authorization schemes that add other headers.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I do prefer that, but I was very hesitant to make argument side effecting part of a public interface. I didn't want to have that argument, but if you also think this case is worth it, I don't mind making that change.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated, better to just keep the interface less opinionated

Signed-off-by: Derek McGowan <derek@mcgstyle.net>
@dmcgowan dmcgowan added this to the 1.2 milestone Oct 2, 2018
Copy link
Copy Markdown
Member

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 43acab8 into containerd:master Oct 3, 2018
@dmcgowan dmcgowan deleted the resolver-updates branch September 10, 2019 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Edge cases where containerd would fail to re-pull an image from a private GCR repository pulling image should not request auth tokens twice

4 participants