From 062c80199f42e8057151e21cc09450211acc8c27 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 29 Aug 2023 10:31:46 +0200 Subject: [PATCH 1/3] registry: combine TestEndpointParse and TestEndpointParseInvalid Combine the two tests into a TestV1EndpointParse function, and rewrite them to use gotest.tools for asserting. Also changing the test-cases to use "https://", as the scheme doesn't matter for this test, but using "http://" may trip-up some linters, so let's avoid that. Signed-off-by: Sebastiaan van Stijn --- registry/endpoint_test.go | 82 +++++++++++++++++++++++---------------- 1 file changed, 48 insertions(+), 34 deletions(-) diff --git a/registry/endpoint_test.go b/registry/endpoint_test.go index f5edc6ad08..9afbe54895 100644 --- a/registry/endpoint_test.go +++ b/registry/endpoint_test.go @@ -5,44 +5,58 @@ import ( "net/http/httptest" "net/url" "testing" + + "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" ) -func TestEndpointParse(t *testing.T) { - testData := []struct { - str string - expected string +func TestV1EndpointParse(t *testing.T) { + tests := []struct { + address string + expected string + expectedErr string }{ - {IndexServer, IndexServer}, - {"http://0.0.0.0:5000/v1/", "http://0.0.0.0:5000/v1/"}, - {"http://0.0.0.0:5000", "http://0.0.0.0:5000/v1/"}, - {"0.0.0.0:5000", "https://0.0.0.0:5000/v1/"}, - {"http://0.0.0.0:5000/nonversion/", "http://0.0.0.0:5000/nonversion/v1/"}, - {"http://0.0.0.0:5000/v0/", "http://0.0.0.0:5000/v0/v1/"}, + { + address: IndexServer, + expected: IndexServer, + }, + { + address: "https://0.0.0.0:5000/v1/", + expected: "https://0.0.0.0:5000/v1/", + }, + { + address: "https://0.0.0.0:5000", + expected: "https://0.0.0.0:5000/v1/", + }, + { + address: "0.0.0.0:5000", + expected: "https://0.0.0.0:5000/v1/", + }, + { + address: "https://0.0.0.0:5000/nonversion/", + expected: "https://0.0.0.0:5000/nonversion/v1/", + }, + { + address: "https://0.0.0.0:5000/v0/", + expected: "https://0.0.0.0:5000/v0/v1/", + }, + { + address: "https://0.0.0.0:5000/v2/", + expectedErr: "unsupported V1 version path v2", + }, } - for _, td := range testData { - e, err := newV1EndpointFromStr(td.str, nil, nil) - if err != nil { - t.Errorf("%q: %s", td.str, err) - } - if e == nil { - t.Logf("something's fishy, endpoint for %q is nil", td.str) - continue - } - if e.String() != td.expected { - t.Errorf("expected %q, got %q", td.expected, e.String()) - } - } -} - -func TestEndpointParseInvalid(t *testing.T) { - testData := []string{ - "http://0.0.0.0:5000/v2/", - } - for _, td := range testData { - e, err := newV1EndpointFromStr(td, nil, nil) - if err == nil { - t.Errorf("expected error parsing %q: parsed as %q", td, e) - } + for _, tc := range tests { + tc := tc + t.Run(tc.address, func(t *testing.T) { + ep, err := newV1EndpointFromStr(tc.address, nil, nil) + if tc.expectedErr != "" { + assert.Check(t, is.Error(err, tc.expectedErr)) + assert.Check(t, is.Nil(ep)) + } else { + assert.NilError(t, err) + assert.Check(t, is.Equal(ep.String(), tc.expected)) + } + }) } } From 14b53c63183881ebe41fad9fa74848aa550cb783 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 29 Aug 2023 10:43:13 +0200 Subject: [PATCH 2/3] registry: simplify `trimV1Address` First, remove the loop over `apiVersions`. The `apiVersions` map has two entries (`APIVersion1 => "v1"`, and `APIVersion2 => "v2"`), and `APIVersion1` is skipped, which means that the loop effectively translates to; if apiVersionStr == "v2" { return "", invalidParamf("unsupported V1 version path %s", apiVersionStr) } Which leaves us with "anything else" being returned as-is. This patch removes the loop, and replaces the remaining handling to check for the "v2" suffix to produce an error, or to strip the "v1" suffix. Signed-off-by: Sebastiaan van Stijn --- registry/endpoint_v1.go | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/registry/endpoint_v1.go b/registry/endpoint_v1.go index 4382cdbc0a..65f4824e17 100644 --- a/registry/endpoint_v1.go +++ b/registry/endpoint_v1.go @@ -82,23 +82,14 @@ func validateEndpoint(endpoint *v1Endpoint) error { return nil } -// trimV1Address trims the version off the address and returns the -// trimmed address or an error if there is a non-V1 version. +// trimV1Address trims the "v1" version suffix off the address and returns +// the trimmed address. It returns an error on "v2" endpoints. func trimV1Address(address string) (string, error) { - address = strings.TrimSuffix(address, "/") - chunks := strings.Split(address, "/") - apiVersionStr := chunks[len(chunks)-1] - if apiVersionStr == "v1" { - return strings.Join(chunks[:len(chunks)-1], "/"), nil + trimmed := strings.TrimSuffix(address, "/") + if strings.HasSuffix(trimmed, "/v2") { + return "", invalidParamf("unsupported V1 version path v2") } - - for k, v := range apiVersions { - if k != APIVersion1 && apiVersionStr == v { - return "", invalidParamf("unsupported V1 version path %s", apiVersionStr) - } - } - - return address, nil + return strings.TrimSuffix(trimmed, "/v1"), nil } func newV1EndpointFromStr(address string, tlsConfig *tls.Config, headers http.Header) (*v1Endpoint, error) { From aa59b0f5a28487288cc11038ce99f0edc06af69c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 29 Aug 2023 10:47:29 +0200 Subject: [PATCH 3/3] registry: improve error for invalid search endpoints Explain that search is not supported on v2 endpoints, and include the offending endpoint in the error-message. Signed-off-by: Sebastiaan van Stijn --- registry/endpoint_test.go | 2 +- registry/endpoint_v1.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/registry/endpoint_test.go b/registry/endpoint_test.go index 9afbe54895..654371a696 100644 --- a/registry/endpoint_test.go +++ b/registry/endpoint_test.go @@ -42,7 +42,7 @@ func TestV1EndpointParse(t *testing.T) { }, { address: "https://0.0.0.0:5000/v2/", - expectedErr: "unsupported V1 version path v2", + expectedErr: "search is not supported on v2 endpoints: https://0.0.0.0:5000/v2/", }, } for _, tc := range tests { diff --git a/registry/endpoint_v1.go b/registry/endpoint_v1.go index 65f4824e17..1fdf659ae0 100644 --- a/registry/endpoint_v1.go +++ b/registry/endpoint_v1.go @@ -87,7 +87,7 @@ func validateEndpoint(endpoint *v1Endpoint) error { func trimV1Address(address string) (string, error) { trimmed := strings.TrimSuffix(address, "/") if strings.HasSuffix(trimmed, "/v2") { - return "", invalidParamf("unsupported V1 version path v2") + return "", invalidParamf("search is not supported on v2 endpoints: %s", address) } return strings.TrimSuffix(trimmed, "/v1"), nil }