From 6d64f81ad4f8b8085522bd045b957cc0515d2092 Mon Sep 17 00:00:00 2001 From: Reinier van der Leer Date: Sun, 22 Jan 2023 17:51:45 +0100 Subject: [PATCH] pkg/registry: improve logging, tests & (test) readability --- pkg/registry/auth/auth.go | 6 +-- pkg/registry/auth/auth_test.go | 54 ++++++++++++------------ pkg/registry/helpers/helpers.go | 1 + pkg/registry/helpers/helpers_test.go | 8 +++- pkg/registry/manifest/manifest.go | 13 ++---- pkg/registry/manifest/manifest_test.go | 18 ++++---- pkg/registry/trust.go | 8 ++-- pkg/registry/trust_test.go | 58 +++++++++++++++----------- 8 files changed, 90 insertions(+), 76 deletions(-) diff --git a/pkg/registry/auth/auth.go b/pkg/registry/auth/auth.go index cb81f2aca..0971884d6 100644 --- a/pkg/registry/auth/auth.go +++ b/pkg/registry/auth/auth.go @@ -4,7 +4,7 @@ import ( "encoding/json" "errors" "fmt" - "io/ioutil" + "io" "net/http" "net/url" "strings" @@ -26,7 +26,7 @@ func GetToken(container types.Container, registryAuth string) (string, error) { } URL := GetChallengeURL(normalizedRef) - logrus.WithField("URL", URL.String()).Debug("Building challenge URL") + logrus.WithField("URL", URL.String()).Debug("Built challenge URL") var req *http.Request if req, err = GetChallengeRequest(URL); err != nil { @@ -99,7 +99,7 @@ func GetBearerHeader(challenge string, imageRef ref.Named, registryAuth string) return "", err } - body, _ := ioutil.ReadAll(authResponse.Body) + body, _ := io.ReadAll(authResponse.Body) tokenResponse := &types.TokenResponse{} err = json.Unmarshal(body, tokenResponse) diff --git a/pkg/registry/auth/auth_test.go b/pkg/registry/auth/auth_test.go index f85d27176..554a87f97 100644 --- a/pkg/registry/auth/auth_test.go +++ b/pkg/registry/auth/auth_test.go @@ -2,14 +2,14 @@ package auth_test import ( "fmt" - "github.com/containrrr/watchtower/internal/actions/mocks" - "github.com/containrrr/watchtower/pkg/registry/auth" "net/url" "os" "strings" "testing" "time" + "github.com/containrrr/watchtower/internal/actions/mocks" + "github.com/containrrr/watchtower/pkg/registry/auth" wtTypes "github.com/containrrr/watchtower/pkg/types" ref "github.com/docker/distribution/reference" . "github.com/onsi/ginkgo" @@ -53,7 +53,7 @@ var _ = Describe("the auth module", func() { mockCreated, mockDigest) - When("getting an auth url", func() { + Describe("GetToken", func() { It("should parse the token from the response", SkipIfCredentialsEmpty(GHCRCredentials, func() { creds := fmt.Sprintf("%s:%s", GHCRCredentials.Username, GHCRCredentials.Password) @@ -62,37 +62,39 @@ var _ = Describe("the auth module", func() { Expect(token).NotTo(Equal("")) }), ) + }) + Describe("GetAuthURL", func() { It("should create a valid auth url object based on the challenge header supplied", func() { - input := `bearer realm="https://ghcr.io/token",service="ghcr.io",scope="repository:user/image:pull"` + challenge := `bearer realm="https://ghcr.io/token",service="ghcr.io",scope="repository:user/image:pull"` + imageRef, _ := ref.ParseNormalizedNamed("containrrr/watchtower") expected := &url.URL{ Host: "ghcr.io", Scheme: "https", Path: "/token", RawQuery: "scope=repository%3Acontainrrr%2Fwatchtower%3Apull&service=ghcr.io", } - imageRef, _ := ref.ParseNormalizedNamed("containrrr/watchtower") - res, err := auth.GetAuthURL(input, imageRef) + + URL, err := auth.GetAuthURL(challenge, imageRef) Expect(err).NotTo(HaveOccurred()) - Expect(res).To(Equal(expected)) + Expect(URL).To(Equal(expected)) }) - It("should create a valid auth url object based on the challenge header supplied", func() { - input := `bearer realm="https://ghcr.io/token"` - imageRef, _ := ref.ParseNormalizedNamed("containrrr/watchtower") - res, err := auth.GetAuthURL(input, imageRef) - Expect(err).To(HaveOccurred()) - Expect(res).To(BeNil()) + + When("given an invalid challenge header", func() { + It("should return an error", func() { + challenge := `bearer realm="https://ghcr.io/token"` + imageRef, _ := ref.ParseNormalizedNamed("containrrr/watchtower") + URL, err := auth.GetAuthURL(challenge, imageRef) + Expect(err).To(HaveOccurred()) + Expect(URL).To(BeNil()) + }) }) When("deriving the auth scope from an image name", func() { It("should prepend official dockerhub images with \"library/\"", func() { + Expect(getScopeFromImageAuthURL("registry")).To(Equal("library/registry")) Expect(getScopeFromImageAuthURL("docker.io/registry")).To(Equal("library/registry")) - Expect(getScopeFromImageAuthURL("docker.io/registry")).To(Equal("library/registry")) - Expect(getScopeFromImageAuthURL("index.docker.io/registry")).To(Equal("library/registry")) Expect(getScopeFromImageAuthURL("index.docker.io/registry")).To(Equal("library/registry")) - - Expect(getScopeFromImageAuthURL("registry")).To(Equal("library/registry")) - Expect(getScopeFromImageAuthURL("registry")).To(Equal("library/registry")) }) It("should not include vanity hosts\"", func() { Expect(getScopeFromImageAuthURL("docker.io/containrrr/watchtower")).To(Equal("containrrr/watchtower")) @@ -100,26 +102,27 @@ var _ = Describe("the auth module", func() { }) It("should not destroy three segment image names\"", func() { Expect(getScopeFromImageAuthURL("piksel/containrrr/watchtower")).To(Equal("piksel/containrrr/watchtower")) - Expect(getScopeFromImageAuthURL("piksel/containrrr/watchtower")).To(Equal("piksel/containrrr/watchtower")) + Expect(getScopeFromImageAuthURL("ghcr.io/piksel/containrrr/watchtower")).To(Equal("piksel/containrrr/watchtower")) }) - It("should not add \"library/\" for one segment image names if they're not on dockerhub", func() { + It("should not prepend library/ to image names if they're not on dockerhub", func() { Expect(getScopeFromImageAuthURL("ghcr.io/watchtower")).To(Equal("watchtower")) - // Expect(getScopeFromImageName("watchtower")).To(Equal("watchtower")) + Expect(getScopeFromImageAuthURL("ghcr.io/containrrr/watchtower")).To(Equal("containrrr/watchtower")) }) }) }) - When("getting a challenge url", func() { + + Describe("GetChallengeURL", func() { It("should create a valid challenge url object based on the image ref supplied", func() { expected := url.URL{Host: "ghcr.io", Scheme: "https", Path: "/v2/"} imageRef, _ := ref.ParseNormalizedNamed("ghcr.io/containrrr/watchtower:latest") Expect(auth.GetChallengeURL(imageRef)).To(Equal(expected)) }) - It("should assume docker hub if the image ref is not fully qualified", func() { + It("should assume Docker Hub for image refs with no explicit registry", func() { expected := url.URL{Host: "index.docker.io", Scheme: "https", Path: "/v2/"} imageRef, _ := ref.ParseNormalizedNamed("containrrr/watchtower:latest") Expect(auth.GetChallengeURL(imageRef)).To(Equal(expected)) }) - It("should use docker hub if the image ref specifies docker.io", func() { + It("should use index.docker.io if the image ref specifies docker.io", func() { expected := url.URL{Host: "index.docker.io", Scheme: "https", Path: "/v2/"} imageRef, _ := ref.ParseNormalizedNamed("docker.io/containrrr/watchtower:latest") Expect(auth.GetChallengeURL(imageRef)).To(Equal(expected)) @@ -130,9 +133,8 @@ var _ = Describe("the auth module", func() { var scopeImageRegexp = MatchRegexp("^repository:[a-z0-9]+(/[a-z0-9]+)*:pull$") func getScopeFromImageAuthURL(imageName string) string { - challenge := `bearer realm="https://dummy.host/token",service="dummy.host",scope="repository:user/image:pull"` - normalizedRef, _ := ref.ParseNormalizedNamed(imageName) + challenge := `bearer realm="https://dummy.host/token",service="dummy.host",scope="repository:user/image:pull"` URL, _ := auth.GetAuthURL(challenge, normalizedRef) scope := URL.Query().Get("scope") diff --git a/pkg/registry/helpers/helpers.go b/pkg/registry/helpers/helpers.go index 8d38f1438..8d99f2df7 100644 --- a/pkg/registry/helpers/helpers.go +++ b/pkg/registry/helpers/helpers.go @@ -4,6 +4,7 @@ import ( "github.com/docker/distribution/reference" ) +// domains for Docker Hub, the default registry const ( DefaultRegistryDomain = "docker.io" DefaultRegistryHost = "index.docker.io" diff --git a/pkg/registry/helpers/helpers_test.go b/pkg/registry/helpers/helpers_test.go index e114e6a7b..a561c2cce 100644 --- a/pkg/registry/helpers/helpers_test.go +++ b/pkg/registry/helpers/helpers_test.go @@ -18,15 +18,19 @@ var _ = Describe("the helpers", func() { _, err := GetRegistryAddress("") Expect(err).To(HaveOccurred()) }) - It("should return index.docker.io if passed an image name with no explicit domain", func() { + It("should return index.docker.io for image refs with no explicit registry", func() { Expect(GetRegistryAddress("watchtower")).To(Equal("index.docker.io")) Expect(GetRegistryAddress("containrrr/watchtower")).To(Equal("index.docker.io")) }) + It("should return index.docker.io for image refs with docker.io domain", func() { + Expect(GetRegistryAddress("docker.io/watchtower")).To(Equal("index.docker.io")) + Expect(GetRegistryAddress("docker.io/containrrr/watchtower")).To(Equal("index.docker.io")) + }) It("should return the host if passed an image name containing a local host", func() { Expect(GetRegistryAddress("henk:80/watchtower")).To(Equal("henk:80")) Expect(GetRegistryAddress("localhost/watchtower")).To(Equal("localhost")) }) - It("should return the server name if passed a fully qualified image name", func() { + It("should return the server address if passed a fully qualified image name", func() { Expect(GetRegistryAddress("github.com/containrrr/config")).To(Equal("github.com")) }) }) diff --git a/pkg/registry/manifest/manifest.go b/pkg/registry/manifest/manifest.go index 41f5b30c7..d1b18a9db 100644 --- a/pkg/registry/manifest/manifest.go +++ b/pkg/registry/manifest/manifest.go @@ -18,12 +18,12 @@ func BuildManifestURL(container types.Container) (string, error) { return "", err } normalizedTaggedRef, isTagged := normalizedRef.(ref.NamedTagged) - if !isTagged { - return "", errors.New("Parsed container image ref has no tag: " + normalizedRef.String()) - } + if !isTagged { + return "", errors.New("Parsed container image ref has no tag: " + normalizedRef.String()) + } host, _ := helpers.GetRegistryAddress(normalizedTaggedRef.Name()) - img, tag := ExtractImageAndTag(normalizedTaggedRef) + img, tag := ref.Path(normalizedTaggedRef), normalizedTaggedRef.Tag() logrus.WithFields(logrus.Fields{ "image": img, @@ -43,8 +43,3 @@ func BuildManifestURL(container types.Container) (string, error) { } return url.String(), nil } - -// ExtractImageAndTag from image reference -func ExtractImageAndTag(namedTaggedRef ref.NamedTagged) (string, string) { - return ref.Path(namedTaggedRef), namedTaggedRef.Tag() -} diff --git a/pkg/registry/manifest/manifest_test.go b/pkg/registry/manifest/manifest_test.go index 2c81323ab..b24d9bca2 100644 --- a/pkg/registry/manifest/manifest_test.go +++ b/pkg/registry/manifest/manifest_test.go @@ -19,30 +19,34 @@ func TestManifest(t *testing.T) { var _ = Describe("the manifest module", func() { Describe("BuildManifestURL", func() { It("should return a valid url given a fully qualified image", func() { - expected := "https://ghcr.io/v2/containrrr/watchtower/manifests/latest" + imageRef := "ghcr.io/containrrr/watchtower:mytag" + expected := "https://ghcr.io/v2/containrrr/watchtower/manifests/mytag" - URL, err := buildMockContainerManifestURL("ghcr.io/containrrr/watchtower:latest") + URL, err := buildMockContainerManifestURL(imageRef) Expect(err).NotTo(HaveOccurred()) Expect(URL).To(Equal(expected)) }) - It("should assume Docker Hub for non-qualified images", func() { + It("should assume Docker Hub for image refs with no explicit registry", func() { + imageRef := "containrrr/watchtower:latest" expected := "https://index.docker.io/v2/containrrr/watchtower/manifests/latest" - URL, err := buildMockContainerManifestURL("containrrr/watchtower:latest") + URL, err := buildMockContainerManifestURL(imageRef) Expect(err).NotTo(HaveOccurred()) Expect(URL).To(Equal(expected)) }) - It("should assume latest for image refs without an explicit tag", func() { + It("should assume latest for image refs with no explicit tag", func() { + imageRef := "containrrr/watchtower" expected := "https://index.docker.io/v2/containrrr/watchtower/manifests/latest" - URL, err := buildMockContainerManifestURL("containrrr/watchtower") + URL, err := buildMockContainerManifestURL(imageRef) Expect(err).NotTo(HaveOccurred()) Expect(URL).To(Equal(expected)) }) It("should not prepend library/ for single-part container names in registries other than Docker Hub", func() { + imageRef := "docker-registry.domain/imagename:latest" expected := "https://docker-registry.domain/v2/imagename/manifests/latest" - URL, err := buildMockContainerManifestURL("docker-registry.domain/imagename:latest") + URL, err := buildMockContainerManifestURL(imageRef) Expect(err).NotTo(HaveOccurred()) Expect(URL).To(Equal(expected)) }) diff --git a/pkg/registry/trust.go b/pkg/registry/trust.go index e72ad7366..311e6db3a 100644 --- a/pkg/registry/trust.go +++ b/pkg/registry/trust.go @@ -18,7 +18,7 @@ import ( // loaded from environment variables or docker config // as available in that order func EncodedAuth(ref string) (string, error) { - auth, err := EncodedEnvAuth(ref) + auth, err := EncodedEnvAuth() if err != nil { auth, err = EncodedConfigAuth(ref) } @@ -28,7 +28,7 @@ func EncodedAuth(ref string) (string, error) { // EncodedEnvAuth returns an encoded auth config for the given registry // loaded from environment variables // Returns an error if authentication environment variables have not been set -func EncodedEnvAuth(ref string) (string, error) { +func EncodedEnvAuth() (string, error) { username := os.Getenv("REPO_USER") password := os.Getenv("REPO_PASS") if username != "" && password != "" { @@ -36,7 +36,7 @@ func EncodedEnvAuth(ref string) (string, error) { Username: username, Password: password, } - log.Debugf("Loaded auth credentials for user %s on registry %s", auth.Username, ref) + log.Debugf("Loaded auth credentials for registry user %s from environment", auth.Username) log.Tracef("Using auth password %s", auth.Password) return EncodeAuth(auth) } @@ -60,7 +60,7 @@ func EncodedConfigAuth(imageRef string) (string, error) { } configFile, err := cliconfig.Load(configDir) if err != nil { - log.Errorf("Unable to find default config file %s", err) + log.Errorf("Unable to find default config file: %s", err) return "", err } credStore := CredentialsStore(*configFile) diff --git a/pkg/registry/trust_test.go b/pkg/registry/trust_test.go index c02989f13..00fc8a70c 100644 --- a/pkg/registry/trust_test.go +++ b/pkg/registry/trust_test.go @@ -1,41 +1,49 @@ package registry import ( + "os" + . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - "os" ) -var _ = Describe("Testing with Ginkgo", func() { - It("encoded env auth_ should return an error if repo envs are unset", func() { - _ = os.Unsetenv("REPO_USER") - _ = os.Unsetenv("REPO_PASS") - - _, err := EncodedEnvAuth("") - Expect(err).To(HaveOccurred()) - }) - It("encoded env auth_ should return auth hash if repo envs are set", func() { - var err error - expectedHash := "eyJ1c2VybmFtZSI6ImNvbnRhaW5ycnItdXNlciIsInBhc3N3b3JkIjoiY29udGFpbnJyci1wYXNzIn0=" +var _ = Describe("Registry credential helpers", func() { + Describe("EncodedAuth", func() { + It("should return repo credentials from env when set", func() { + var err error + expected := "eyJ1c2VybmFtZSI6ImNvbnRhaW5ycnItdXNlciIsInBhc3N3b3JkIjoiY29udGFpbnJyci1wYXNzIn0=" - err = os.Setenv("REPO_USER", "containrrr-user") - Expect(err).NotTo(HaveOccurred()) + err = os.Setenv("REPO_USER", "containrrr-user") + Expect(err).NotTo(HaveOccurred()) - err = os.Setenv("REPO_PASS", "containrrr-pass") - Expect(err).NotTo(HaveOccurred()) + err = os.Setenv("REPO_PASS", "containrrr-pass") + Expect(err).NotTo(HaveOccurred()) - config, err := EncodedEnvAuth("") - Expect(config).To(Equal(expectedHash)) - Expect(err).NotTo(HaveOccurred()) + config, err := EncodedEnvAuth() + Expect(config).To(Equal(expected)) + Expect(err).NotTo(HaveOccurred()) + }) }) - It("encoded config auth_ should return an error if file is not present", func() { - var err error - err = os.Setenv("DOCKER_CONFIG", "/dev/null/should-fail") - Expect(err).NotTo(HaveOccurred()) + Describe("EncodedEnvAuth", func() { + It("should return an error if repo envs are unset", func() { + _ = os.Unsetenv("REPO_USER") + _ = os.Unsetenv("REPO_PASS") - _, err = EncodedConfigAuth("") - Expect(err).To(HaveOccurred()) + _, err := EncodedEnvAuth() + Expect(err).To(HaveOccurred()) + }) }) + + Describe("EncodedConfigAuth", func() { + It("should return an error if file is not present", func() { + var err error + + err = os.Setenv("DOCKER_CONFIG", "/dev/null/should-fail") + Expect(err).NotTo(HaveOccurred()) + + _, err = EncodedConfigAuth("") + Expect(err).To(HaveOccurred()) + }) }) })