Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/jetstack/version-checker/pkg/client/acr"
"github.com/jetstack/version-checker/pkg/client/docker"
"github.com/jetstack/version-checker/pkg/client/ecr"
"github.com/jetstack/version-checker/pkg/client/ecrpublic"
"github.com/jetstack/version-checker/pkg/client/fallback"
"github.com/jetstack/version-checker/pkg/client/gcr"
"github.com/jetstack/version-checker/pkg/client/ghcr"
Expand Down Expand Up @@ -39,6 +40,7 @@ type Options struct {
ACR acr.Options
Docker docker.Options
ECR ecr.Options
ECRPublic ecrpublic.Options // Add ECRPublic options
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

[nitpick] Remove the inline comment "Add ECRPublic options". Comments that simply describe what the code obviously does add no value and should be removed. The field name ECRPublic is self-documenting.

Suggested change
ECRPublic ecrpublic.Options // Add ECRPublic options
ECRPublic ecrpublic.Options

Copilot uses AI. Check for mistakes.
GCR gcr.Options
GHCR ghcr.Options
OCI oci.Options
Expand All @@ -54,6 +56,7 @@ func New(ctx context.Context, log *logrus.Entry, opts Options) (*Client, error)
if opts.Transport != nil {
opts.Quay.Transporter = opts.Transport
opts.ECR.Transporter = opts.Transport
opts.ECRPublic.Transporter = opts.Transport // Add ECR public transporter
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

[nitpick] Remove the inline comment "Add ECR public transporter". The code is self-explanatory and the comment adds no value.

Suggested change
opts.ECRPublic.Transporter = opts.Transport // Add ECR public transporter
opts.ECRPublic.Transporter = opts.Transport

Copilot uses AI. Check for mistakes.
opts.GHCR.Transporter = opts.Transport
opts.GCR.Transporter = opts.Transport
}
Expand All @@ -66,6 +69,10 @@ func New(ctx context.Context, log *logrus.Entry, opts Options) (*Client, error)
if err != nil {
return nil, fmt.Errorf("failed to create docker client: %w", err)
}
ecrPublicClient, err := ecrpublic.New(opts.ECRPublic, log)
if err != nil {
return nil, fmt.Errorf("failed to create ecr public client: %w", err)
}

var selfhostedClients []api.ImageClient
for _, sOpts := range opts.Selfhosted {
Expand Down Expand Up @@ -106,6 +113,7 @@ func New(ctx context.Context, log *logrus.Entry, opts Options) (*Client, error)
selfhostedClients,
acrClient,
ecr.New(opts.ECR),
ecrPublicClient,
dockerClient,
gcr.New(opts.GCR),
ghcr.New(opts.GHCR),
Expand Down
152 changes: 152 additions & 0 deletions pkg/client/ecrpublic/ecrpublic.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
package ecrpublic

import (
"context"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
"time"

"github.com/sirupsen/logrus"

retryablehttp "github.com/hashicorp/go-retryablehttp"
"github.com/jetstack/version-checker/pkg/api"
"github.com/jetstack/version-checker/pkg/client/util"
)

const (
ecrPublicLookupURL = "https://public.ecr.aws/v2/%s/%s/tags/list"
loginURL = "https://public.ecr.aws/token/?service=ecr-public"
)

type Options struct {
Username string
Password string
Transporter http.RoundTripper
}

type Client struct {
*http.Client
Options
}

Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

Missing compile-time interface assertion. All other client implementations in the codebase include var _ api.ImageClient = (*Client)(nil) to ensure the Client type implements the api.ImageClient interface at compile time. This should be added at the package level (typically after the Client struct definition).

Suggested change
var _ api.ImageClient = (*Client)(nil)

Copilot uses AI. Check for mistakes.
func New(opts Options, log *logrus.Entry) (*Client, error) {
retryclient := retryablehttp.NewClient()
if opts.Transporter != nil {
retryclient.HTTPClient.Transport = opts.Transporter
}
retryclient.HTTPClient.Timeout = 10 * time.Second
retryclient.RetryMax = 10
retryclient.RetryWaitMax = 10 * time.Minute
retryclient.RetryWaitMin = 1 * time.Second
// This custom backoff will fail requests that have a max wait of the RetryWaitMax
retryclient.Backoff = util.HTTPBackOff
retryclient.Logger = log.WithField("client", "ecrpublic")
client := retryclient.StandardClient()

return &Client{
Options: opts,
Client: client,
}, nil
}

func (c *Client) Name() string {
return "ecrpublic"
}

func (c *Client) Tags(ctx context.Context, _, repo, image string) ([]api.ImageTag, error) {
url := fmt.Sprintf(ecrPublicLookupURL, repo, image)
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

When image is empty (as in the test case "nginx" where repo="nginx" and image=""), the URL will be constructed as https://public.ecr.aws/v2/nginx//tags/list with a double slash. This may not be the correct format for the ECR Public API. Verify that this URL format works correctly, or handle the case where image is empty by constructing the URL differently, e.g., https://public.ecr.aws/v2/nginx/tags/list.

Suggested change
url := fmt.Sprintf(ecrPublicLookupURL, repo, image)
var url string
if image == "" {
url = fmt.Sprintf("https://public.ecr.aws/v2/%s/tags/list", repo)
} else {
url = fmt.Sprintf(ecrPublicLookupURL, repo, image)
}

Copilot uses AI. Check for mistakes.

var tags []api.ImageTag
for url != "" {
response, err := c.doRequest(ctx, url)
if err != nil {
return nil, err
}

for _, tag := range response.Tags {
// No images in this result, so continue early
if len(tag) == 0 {
continue
}

tags = append(tags, api.ImageTag{
Tag: tag,
})
}

url = response.Next
}

return tags, nil
}

func (c *Client) doRequest(ctx context.Context, url string) (*TagResponse, error) {
req, err := http.NewRequest(http.MethodGet, url, nil)
if err != nil {
return nil, err
}

// Always get a token for ECR Public
token, err := getAnonymousToken(ctx, c.Client)
if err != nil {
return nil, fmt.Errorf("failed to get anonymous token: %s", err)
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The error message formatting is inconsistent with the error returned from getAnonymousToken. The error should use %w instead of %s to properly wrap the error, allowing error unwrapping with errors.Is() and errors.As(). Change to: fmt.Errorf("failed to get anonymous token: %w", err)

Suggested change
return nil, fmt.Errorf("failed to get anonymous token: %s", err)
return nil, fmt.Errorf("failed to get anonymous token: %w", err)

Copilot uses AI. Check for mistakes.
}

req.URL.Scheme = "https"
req = req.WithContext(ctx)
if len(token) > 0 {
req.Header.Add("Authorization", "Bearer "+token)
}

resp, err := c.Do(req)
if err != nil {
return nil, fmt.Errorf("failed to get %q image: %s", c.Name(), err)
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

Similarly, this error should use %w instead of %s to properly wrap the error. Change to: fmt.Errorf("failed to get %q image: %w", c.Name(), err)

Suggested change
return nil, fmt.Errorf("failed to get %q image: %s", c.Name(), err)
return nil, fmt.Errorf("failed to get %q image: %w", c.Name(), err)

Copilot uses AI. Check for mistakes.
}
defer func() { _ = resp.Body.Close() }()

body, err := io.ReadAll(resp.Body)
if err != nil {
return nil, err
}

response := new(TagResponse)
if err := json.Unmarshal(body, response); err != nil {
return nil, fmt.Errorf("unexpected image tags response: %s", body)
}
Comment on lines +104 to +118
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The doRequest function doesn't check the HTTP status code before attempting to unmarshal the response body. If the API returns an error status (e.g., 404, 401, 500), the function will try to unmarshal the error response as a TagResponse, which will likely fail with a confusing error message. Consider adding a status code check similar to getAnonymousToken at line 142:

if resp.StatusCode != http.StatusOK {
    return nil, fmt.Errorf("failed to get tags, status code: %d, body: %s", resp.StatusCode, body)
}

Copilot uses AI. Check for mistakes.

return response, nil
}

func getAnonymousToken(ctx context.Context, client *http.Client) (string, error) {
req, err := http.NewRequest(http.MethodGet, loginURL, nil)
if err != nil {
return "", err
}

req = req.WithContext(ctx)

resp, err := client.Do(req)
if err != nil {
return "", err
}
defer func() { _ = resp.Body.Close() }()

body, err := io.ReadAll(resp.Body)
if err != nil {
return "", err
}

if resp.StatusCode != http.StatusOK {
return "", errors.New(string(body))
}

response := new(AuthResponse)
if err := json.Unmarshal(body, response); err != nil {
return "", err
}

return response.Token, nil
}
Comment on lines +59 to +152
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The main client functionality in ecrpublic.go lacks test coverage. While path_test.go tests IsHost() and RepoImageFromPath(), there are no tests for the core functions Tags(), doRequest(), and getAnonymousToken(). Other client implementations like docker and ghcr include comprehensive tests for these core functions. Consider adding tests similar to those in pkg/client/docker/docker_test.go that use httptest to mock API responses.

Copilot uses AI. Check for mistakes.
20 changes: 20 additions & 0 deletions pkg/client/ecrpublic/path.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package ecrpublic

import (
"regexp"
"strings"
)

var (
ecrPublicPattern = regexp.MustCompile(`^public\.ecr\.aws$`)
)

func (c *Client) IsHost(host string) bool {
return ecrPublicPattern.MatchString(host)
}

func (c *Client) RepoImageFromPath(path string) (string, string) {
parts := strings.Split(path, "/")

return parts[0], strings.Join(parts[1:], "/")
}
95 changes: 95 additions & 0 deletions pkg/client/ecrpublic/path_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package ecrpublic

import "testing"

func TestIsHost(t *testing.T) {
tests := map[string]struct {
host string
expIs bool
}{
"an empty host should be false": {
host: "",
expIs: false,
},
"random string should be false": {
host: "foobar",
expIs: false,
},
"path with two segments should be false": {
host: "joshvanl/version-checker",
expIs: false,
},
"path with three segments should be false": {
host: "jetstack/joshvanl/version-checker",
expIs: false,
},
"random string with dots should be false": {
host: "foobar.foo",
expIs: false,
},
"docker.io should be false": {
host: "docker.io",
expIs: false,
},
"docker.com should be false": {
host: "docker.com",
expIs: false,
},
"just public.ecr.aws should be true": {
host: "public.ecr.aws",
expIs: true,
},
"public.ecr.aws.foo should be false": {
host: "public.ecr.aws.foo",
expIs: false,
},
"foo.public.ecr.aws should be false": {
host: "foo.public.ecr.aws",
expIs: false,
},
}

handler := new(Client)
for name, test := range tests {
t.Run(name, func(t *testing.T) {
if isHost := handler.IsHost(test.host); isHost != test.expIs {
t.Errorf("%s: unexpected IsHost, exp=%t got=%t",
test.host, test.expIs, isHost)
}
})
}
}

func TestRepoImageFromPath(t *testing.T) {
tests := map[string]struct {
path string
expRepo, expImage string
}{
"single image should return registry and image": {
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The test name says "single image should return registry and image" but the expected expImage is empty. This is confusing and the test name doesn't match the expectation. Consider renaming to something like "single segment path should return repo with empty image" to better reflect what's being tested.

Suggested change
"single image should return registry and image": {
"single segment path should return repo with empty image": {

Copilot uses AI. Check for mistakes.
path: "nginx",
expRepo: "nginx",
expImage: "",
},
"two segments to path should return registry and repo": {
path: "eks-distro/kubernetes",
expRepo: "eks-distro",
expImage: "kubernetes",
},
"three segments to path should return registry and combined repo": {
path: "eks-distro/kubernetes/kube-proxy",
expRepo: "eks-distro",
expImage: "kubernetes/kube-proxy",
},
}

handler := new(Client)
for name, test := range tests {
t.Run(name, func(t *testing.T) {
repo, image := handler.RepoImageFromPath(test.path)
if repo != test.expRepo || image != test.expImage {
t.Errorf("%s: unexpected repo/image, exp=%s/%s got=%s/%s",
test.path, test.expRepo, test.expImage, repo, image)
}
})
}
}
11 changes: 11 additions & 0 deletions pkg/client/ecrpublic/types.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package ecrpublic

type AuthResponse struct {
Token string `json:"token"`
}

type TagResponse struct {
Next string `json:"next"`
Name string `json:"name"`
Tags []string `json:"tags"`
}
Loading