feat: support credential function #45

wangxiaoxuan273 · 2023-04-07T05:48:26Z

Resolves #33

codecov-commenter · 2023-04-07T05:49:36Z

Codecov Report

Merging #45 (086f9b2) into main (87a3ee4) will increase coverage by 0.21%.
The diff coverage is 85.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #45      +/-   ##
==========================================
+ Coverage   78.32%   78.53%   +0.21%     
==========================================
  Files           3        3              
  Lines         203      219      +16     
==========================================
+ Hits          159      172      +13     
- Misses         33       35       +2     
- Partials       11       12       +1

Impacted Files	Coverage Δ
registry.go	`74.07% <85.00%> (+3.02%)`	⬆️

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

Wwwsylvia · 2023-04-07T08:25:43Z

credential.go

+	"oras.land/oras-go/v2/registry/remote/auth"
+)
+
+func Credential(store Store) func(context.Context, string) (auth.Credential, error) {


nit: missing documentation.

Can we have some unit tests? It would be somewhat bit similar to https://github.com/oras-project/oras-go/blob/main/registry/remote/auth/client_test.go#L2220-L2239.

Added doc comment.

Added unit tests.

Wwwsylvia · 2023-04-07T13:33:29Z

credential.go

+		if registry == "" {
+			return auth.EmptyCredential, nil
+		}
+		return store.Get(ctx, registry)


Should we support something like https://github.com/oras-project/oras/blob/main/cmd/oras/internal/option/remote.go#L222-L234? /cc @qweeah

It MUST be included

#18 (reply in thread)

Added docker.io redirection.

registry.go

Wwwsylvia

LGTM

shizhMSFT · 2023-04-14T14:30:06Z

registry.go

+// Credential returns a Credential() function that can be used by auth.Client.
+func Credential(store Store) func(context.Context, string) (auth.Credential, error) {
+	return func(ctx context.Context, reg string) (auth.Credential, error) {
+		reg = mapHostname(reg)


There are 3 concepts in the docker: registry name, hostname, and credential key.

Let's do some experiments.

$ docker login docker.io -u <username> -p <password> Login Succeeded $ cat ~/.docker/config.json { "auths": { "https://index.docker.io/v1/": { "auth": "dummy/auth==" } } } $ rm ~/.docker/config.json $ docker login registry-1.docker.io -u <username> -p <password> Login Succeeded $ cat ~/.docker/config.json { "auths": { "registry-1.docker.io": { "auth": "dummy/auth==" } } } $ rm ~/.docker/config.json $ docker login index.docker.io -u <username> -p <password> Login Succeeded $ cat ~/.docker/config.json { "auths": { "index.docker.io": { "auth": "dummy/auth==" } } } $ rm ~/.docker/config.json $ docker login https://index.docker.io/v1/ -u <username> -p <password> Login Succeeded $ cat ~/.docker/config.json { "auths": { "https://index.docker.io/v1/": { "auth": "dummy/auth==" } } }

As you can observe,

The registry name docker.io maps to hostname registry-1.docker.io where the credential key is https://index.docker.io/v1/

The registry name registry-1.docker.io maps to hostname registry-1.docker.io where the credential key is registry-1.docker.io.

Therefore, mapHostname() does not do correct mapping and need to be fixed.

/cc @Wwwsylvia

This is very tricky. Thanks @shizhMSFT for the explanation.

Signed-off-by: wangxiaoxuan273 <[email protected]>

TerryHowe · 2023-04-18T14:06:44Z

registry.go

+
+// The Docker CLI expects that the 'docker.io' credential
+// will be added under the key "https://index.docker.io/v1/"
+func getLoginRegistry(hostname string) string {


Maybe getStoreRegistryName

How about mapCredentialKeyName? 🤔

I changed the function name to mapStoreRegistryName

TerryHowe · 2023-04-18T14:07:16Z

registry.go

 	if hostname == "docker.io" {
 		return "https://index.docker.io/v1/"
 	}
 	return hostname
 }
+
+// It is expected that the traffic targetting "registry-1.docker.io"
+// will be redirected to "https://index.docker.io/v1/"


Maybe something like getAuthenticationRegistryName

I think map might be better than get

Or we don't need to make it a function.

I think a function here makes the code clean. I changed the name to mapAuthenticationRegistryName.

Signed-off-by: wangxiaoxuan273 <[email protected]>

Wwwsylvia

LGTM

Wwwsylvia · 2023-04-19T09:26:10Z

registry.go

+	}
+}
+
+// The Docker CLI expects that the 'docker.io' credential


nit: Wondering if we should move this comment inside. 🤔

Moved them inside.

shizhMSFT · 2023-04-19T16:43:53Z

registry.go

+	}
+}
+
+// The Docker CLI expects that the 'docker.io' credential


Documentation starts with <function name> bla bla.

Do we have any reference so that other contributors can understand?

These are private functions and don't need documentation. I moved the comments inside the function.

Added reference code link.

shizhMSFT · 2023-04-19T16:44:06Z

registry.go

+	return registry
+}
+
+// It is expected that the traffic targetting "registry-1.docker.io"


Documentation starts with <function name> bla bla.

Do we have any reference so that other contributors can understand?

These are private functions and don't need documentation. I moved the comments inside the function.

Added reference code link.

shizhMSFT · 2023-04-19T16:44:46Z

registry.go

+
+// It is expected that the traffic targetting "registry-1.docker.io"
+// will be redirected to "https://index.docker.io/v1/"
+func mapAuthenticationRegistryName(target string) string {


Should rename target to hostname for better readability.

Signed-off-by: wangxiaoxuan273 <[email protected]>

Wwwsylvia

LGTM

shizhMSFT · 2023-04-20T09:01:37Z

registry.go

 func mapStoreRegistryName(registry string) string {
+	// The Docker CLI expects that the 'docker.io' credential
+	// will be added under the key "https://index.docker.io/v1/"
+	// See: https://github.com/moby/moby/blob/master/registry/config.go#L25-L48


Never use a branch name (e.g. master) in the link. Use tag or commit digest so that the link won't expire.

Updated the link to: https://github.com/moby/moby/blob/v24.0.0-beta.2/registry/config.go#L25-L48

Signed-off-by: wangxiaoxuan273 <[email protected]>

shizhMSFT

LGTM

wangxiaoxuan273 requested review from shizhMSFT, Wwwsylvia and TerryHowe as code owners April 7, 2023 05:48

Wwwsylvia suggested changes Apr 7, 2023

View reviewed changes

Wwwsylvia reviewed Apr 7, 2023

View reviewed changes

wangxiaoxuan273 requested a review from Wwwsylvia April 14, 2023 03:33

Wwwsylvia suggested changes Apr 14, 2023

View reviewed changes

registry.go Outdated Show resolved Hide resolved

wangxiaoxuan273 requested a review from Wwwsylvia April 14, 2023 08:43

Wwwsylvia approved these changes Apr 14, 2023

View reviewed changes

shizhMSFT suggested changes Apr 14, 2023

View reviewed changes

wangxiaoxuan273 added 7 commits April 17, 2023 18:40

feat: support credential function

3c54665

Signed-off-by: wangxiaoxuan273 <[email protected]>

rebased

a07546c

Signed-off-by: wangxiaoxuan273 <[email protected]>

rebased and added unit tests

e9bc229

Signed-off-by: wangxiaoxuan273 <[email protected]>

minor change

72b2cf1

Signed-off-by: wangxiaoxuan273 <[email protected]>

refined redirect

518f6ca

Signed-off-by: wangxiaoxuan273 <[email protected]>

added another map function, need refinement

9becb4c

Signed-off-by: wangxiaoxuan273 <[email protected]>

refined

0897940

Signed-off-by: wangxiaoxuan273 <[email protected]>

TerryHowe reviewed Apr 18, 2023

View reviewed changes

renamed the functions

c97288d

Signed-off-by: wangxiaoxuan273 <[email protected]>

Wwwsylvia approved these changes Apr 19, 2023

View reviewed changes

shizhMSFT suggested changes Apr 19, 2023

View reviewed changes

wangxiaoxuan273 added 2 commits April 20, 2023 02:40

resolved comments

2a558c2

Signed-off-by: wangxiaoxuan273 <[email protected]>

resolved comments

4a7bf46

Signed-off-by: wangxiaoxuan273 <[email protected]>

Wwwsylvia mentioned this pull request Apr 20, 2023

Credential library support for legacy auth config keys #1

Closed

Wwwsylvia approved these changes Apr 20, 2023

View reviewed changes

shizhMSFT reviewed Apr 20, 2023

View reviewed changes

updated reference link

086f9b2

Signed-off-by: wangxiaoxuan273 <[email protected]>

shizhMSFT approved these changes Apr 20, 2023

View reviewed changes

shizhMSFT merged commit 9d0706f into oras-project:main Apr 20, 2023

wangxiaoxuan273 deleted the credential branch April 20, 2023 09:10

feat: support credential function #45

feat: support credential function #45

Conversation

wangxiaoxuan273 commented Apr 7, 2023

codecov-commenter commented Apr 7, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wwwsylvia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangxiaoxuan273 Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangxiaoxuan273 Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Wwwsylvia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wwwsylvia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 7, 2023 •

edited

Loading

wangxiaoxuan273 Apr 19, 2023 •

edited

Loading

wangxiaoxuan273 Apr 19, 2023 •

edited

Loading