Skip to content

fix: provide function to support hostname mapping#549

Merged
shizhMSFT merged 7 commits into
oras-project:mainfrom
junczhu:junczhu/auth-docker.io
Sep 13, 2022
Merged

fix: provide function to support hostname mapping#549
shizhMSFT merged 7 commits into
oras-project:mainfrom
junczhu:junczhu/auth-docker.io

Conversation

@junczhu

@junczhu junczhu commented Sep 9, 2022

Copy link
Copy Markdown
Contributor

Resolve: #542
Signed-off-by: Juncheng Zhu junczhu@microsoft.com

@qweeah

qweeah commented Sep 9, 2022

Copy link
Copy Markdown
Contributor

Update: Just realize that this issue only happens when using cred store, please resolve What if username/password are passed through flags? Replacing host name in cred store only works when credential is empty, see
https://github.com/oras-project/oras/blob/main/cmd/oras/internal/option/remote.go#L152

Comment thread internal/credential/store.go Outdated
}

// convertHostname maps docker.io to registry-1.docker.io
func convertHostname(registry string) string {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use registry.Host directly

@shizhMSFT

Copy link
Copy Markdown
Contributor

@qweeah I've discussed offline with @junczhu. The behavior of docker.io in docker is special that it connects to the host of registry-1.docker.io using the credential with the reigstry name https://index.docker.io/v1/, which is really not a valid name (but it uses).

@shizhMSFT shizhMSFT modified the milestones: future, v0.15.0 Sep 10, 2022

@shizhMSFT shizhMSFT left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please update this PR as discussed to use the credentials of https://index.docker.io/v1/ for docker.io when connecting registry-1.docker.io as docker.io. Meanwhile, please well document the docker behavior in the corresponding comment block.

@codecov-commenter

codecov-commenter commented Sep 12, 2022

Copy link
Copy Markdown

Codecov Report

Merging #549 (3d819ff) into main (9a014d6) will decrease coverage by 1.40%.
The diff coverage is 31.81%.

@@            Coverage Diff             @@
##             main     #549      +/-   ##
==========================================
- Coverage   70.72%   69.31%   -1.41%     
==========================================
  Files          11       11              
  Lines         427      440      +13     
==========================================
+ Hits          302      305       +3     
- Misses        101      110       +9     
- Partials       24       25       +1     
Impacted Files Coverage Δ
internal/credential/store.go 65.27% <ø> (-0.48%) ⬇️
cmd/oras/internal/option/remote.go 61.53% <31.81%> (-3.98%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@junczhu junczhu requested a review from qweeah September 12, 2022 17:29
Comment thread cmd/oras/internal/option/remote.go Outdated

// authClient assembles a oras auth client.
func (opts *Remote) authClient(debug bool) (client *auth.Client, err error) {
func (opts *Remote) authClient(host string, debug bool) (client *auth.Client, err error) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why we need this new argument? It's not used anywhere.

@qweeah qweeah left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add a unit test case for this host name replacement?

Comment thread cmd/oras/internal/option/remote.go
Comment thread cmd/oras/login.go Outdated
// According the the behavior of Docker CLI,
// credential should be added under key "https://index.docker.io/v1/"
hostname := opts.Hostname
if opts.Hostname == "docker.io" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if opts.Hostname == "docker.io" {
if hostname == "docker.io" {

@shizhMSFT shizhMSFT left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>
Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>
Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>
Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>
Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>
Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>
Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>
@junczhu junczhu force-pushed the junczhu/auth-docker.io branch from 16d8c5f to c77a73a Compare September 13, 2022 08:34

@shizhMSFT shizhMSFT left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@shizhMSFT shizhMSFT merged commit 22d2e43 into oras-project:main Sep 13, 2022
TerryHowe pushed a commit to TerryHowe/oras that referenced this pull request Feb 2, 2023
Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

ORAS CLI should support interacting with docker.io

4 participants