Skip to content
This repository has been archived by the owner on Apr 7, 2024. It is now read-only.

feat: support credential function #45

Merged
merged 11 commits into from
Apr 20, 2023
34 changes: 27 additions & 7 deletions registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func Login(ctx context.Context, store Store, reg *remote.Registry, cred auth.Cre
if err := regClone.Ping(ctx); err != nil {
return fmt.Errorf("unable to ping the registry %s: %w", regClone.Reference.Registry, err)
}
hostname := mapHostname(regClone.Reference.Registry)
hostname := mapStoreRegistryName(regClone.Reference.Registry)
if err := store.Put(ctx, hostname, cred); err != nil {
return fmt.Errorf("unable to store the credential for %s: %w", hostname, err)
}
Expand All @@ -57,18 +57,38 @@ func Login(ctx context.Context, store Store, reg *remote.Registry, cred auth.Cre

// Logout provides the logout functionality given the registry name.
func Logout(ctx context.Context, store Store, registryName string) error {
registryName = mapHostname(registryName)
registryName = mapStoreRegistryName(registryName)
if err := store.Delete(ctx, registryName); err != nil {
return fmt.Errorf("unable to delete the credential for %s: %w", registryName, err)
}
return nil
}

func mapHostname(hostname string) string {
// The Docker CLI expects that the 'docker.io' credential
// will be added under the key "https://index.docker.io/v1/"
if hostname == "docker.io" {
// 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 = mapAuthenticationRegistryName(reg)
if reg == "" {
return auth.EmptyCredential, nil
}
return store.Get(ctx, reg)
}
}

// The Docker CLI expects that the 'docker.io' credential
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved them inside.

Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation starts with <function name> bla bla.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added reference code link.

// will be added under the key "https://index.docker.io/v1/"
func mapStoreRegistryName(registry string) string {
if registry == "docker.io" {
return "https://index.docker.io/v1/"
}
return registry
}

// It is expected that the traffic targetting "registry-1.docker.io"
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation starts with <function name> bla bla.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added reference code link.

// will be redirected to "https://index.docker.io/v1/"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like getAuthenticationRegistryName

Copy link
Member

Choose a reason for hiding this comment

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

I think map might be better than get

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@wangxiaoxuan273 wangxiaoxuan273 Apr 19, 2023

Choose a reason for hiding this comment

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

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

func mapAuthenticationRegistryName(target string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should rename target to hostname for better readability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed.

if target == "registry-1.docker.io" {
return "https://index.docker.io/v1/"
}
return hostname
return target
}
58 changes: 51 additions & 7 deletions registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,21 +159,65 @@ func Test_mapHostname(t *testing.T) {
want string
}{
{
"map docker.io to https://index.docker.io/v1/",
"docker.io",
"https://index.docker.io/v1/",
name: "map docker.io to https://index.docker.io/v1/",
host: "docker.io",
want: "https://index.docker.io/v1/",
},
{
"do not map other host names",
"localhost:2333",
"localhost:2333",
name: "do not map other host names",
host: "localhost:2333",
want: "localhost:2333",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := mapHostname(tt.host); got != tt.want {
if got := mapStoreRegistryName(tt.host); got != tt.want {
t.Errorf("mapHostname() = %v, want %v", got, tt.want)
}
})
}
}

func TestCredential(t *testing.T) {
// create a test store
s := &testStore{}
s.storage = map[string]auth.Credential{
"localhost:2333": {Username: "test_user", Password: "test_word"},
"https://index.docker.io/v1/": {Username: "user", Password: "word"},
}
// create a test client using Credential
testClient := &auth.Client{}
testClient.Credential = Credential(s)
tests := []struct {
name string
registry string
wantCredential auth.Credential
}{
{
name: "get credentials for localhost:2333",
registry: "localhost:2333",
wantCredential: auth.Credential{Username: "test_user", Password: "test_word"},
},
{
name: "get credentials for registry-1.docker.io",
registry: "registry-1.docker.io",
wantCredential: auth.Credential{Username: "user", Password: "word"},
},
{
name: "get credentials for a registry not stored",
registry: "localhost:6666",
wantCredential: auth.EmptyCredential,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := testClient.Credential(context.Background(), tt.registry)
if err != nil {
t.Errorf("could not get credential: %v", err)
}
if !reflect.DeepEqual(got, tt.wantCredential) {
t.Errorf("Credential() = %v, want %v", got, tt.wantCredential)
}
})
}
}