[release-1.7] 🌱 test: check for metadata.yaml when resolving releases to not try to use unreleased versions + avoid retry on 404 (also in clusterctl)

cmd/clusterctl/client/repository/repository_github.go

@@ -414,6 +414,12 @@ func (g *gitHubRepository) httpGetFilesFromRelease(ctx context.Context, version,
 		}
 		defer resp.Body.Close()
 
+		// if we get 404 there is no reason to retry
+		if resp.StatusCode == http.StatusNotFound {
+			retryError = errNotFound
+			return true, nil
+		}
+
 		if resp.StatusCode != http.StatusOK {
 			retryError = errors.Errorf("error getting file, status code: %d", resp.StatusCode)
 			return false, nil

test/framework/clusterctl/e2e_config.go

@@ -19,6 +19,8 @@ package clusterctl
 import (
 	"context"
 	"fmt"
+	"io"
+	"net/http"
 	"net/url"
 	"os"
 	"path/filepath"
@@ -33,6 +35,7 @@ import (
 	. "github.com/onsi/gomega"
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/util/version"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/yaml"
 
@@ -304,13 +307,16 @@ func ResolveRelease(ctx context.Context, releaseMarker string) (string, error) {
 		return "", errors.Errorf("releasemarker does not support disabling the go proxy: GOPROXY=%q", os.Getenv("GOPROXY"))
 	}
 	goproxyClient := goproxy.NewClient(scheme, host)
-	return resolveReleaseMarker(ctx, releaseMarker, goproxyClient)
+	return resolveReleaseMarker(ctx, releaseMarker, goproxyClient, githubReleaseMetadataURL)
 }
 
 // resolveReleaseMarker resolves releaseMarker string to verion string e.g.
 // - Resolves "go://sigs.k8s.io/cluster-api@v1.0" to the latest stable patch release of v1.0.
 // - Resolves "go://sigs.k8s.io/cluster-api@latest-v1.0" to the latest patch release of v1.0 including rc and pre releases.
-func resolveReleaseMarker(ctx context.Context, releaseMarker string, goproxyClient *goproxy.Client) (string, error) {
+// It also checks if a release actually exists by trying to query for the `metadata.yaml` file.
+// To do that the url gets calculated by the toMetadataURL function. The toMetadataURL func
+// is passed as variable to be able to write a proper unit test.
+func resolveReleaseMarker(ctx context.Context, releaseMarker string, goproxyClient *goproxy.Client, toMetadataURL func(gomodule, version string) string) (string, error) {
 	if !strings.HasPrefix(releaseMarker, "go://") {
 		return "", errors.Errorf("unknown release marker scheme")
 	}
@@ -330,31 +336,131 @@ func resolveReleaseMarker(ctx context.Context, releaseMarker string, goproxyClie
 	if strings.HasPrefix(gomoduleParts[1], "latest-") {
 		includePrereleases = true
 	}
-	version := strings.TrimPrefix(gomoduleParts[1], "latest-") + ".0"
-	version = strings.TrimPrefix(version, "v")
-	semVersion, err := semver.Parse(version)
+	rawVersion := strings.TrimPrefix(gomoduleParts[1], "latest-") + ".0"
+	rawVersion = strings.TrimPrefix(rawVersion, "v")
+	semVersion, err := semver.Parse(rawVersion)
 	if err != nil {
-		return "", errors.Wrapf(err, "parsing semver for %s", version)
+		return "", errors.Wrapf(err, "parsing semver for %s", rawVersion)
 	}
 
-	parsedTags, err := goproxyClient.GetVersions(ctx, gomodule)
+	versions, err := goproxyClient.GetVersions(ctx, gomodule)
 	if err != nil {
 		return "", err
 	}
 
-	var picked semver.Version
-	for i, tag := range parsedTags {
-		if !includePrereleases && len(tag.Pre) > 0 {
+	// Search for the latest release according to semantic version ordering.
+	// Releases with tag name that are not in semver format are ignored.
+	versionCandidates := []semver.Version{}
+	for _, version := range versions {
+		if !includePrereleases && len(version.Pre) > 0 {
 			continue
 		}
-		if tag.Major == semVersion.Major && tag.Minor == semVersion.Minor {
-			picked = parsedTags[i]
+		if version.Major != semVersion.Major || version.Minor != semVersion.Minor {
+			continue
 		}
+
+		versionCandidates = append(versionCandidates, version)
 	}
-	if picked.Major == 0 && picked.Minor == 0 && picked.Patch == 0 {
+
+	if len(versionCandidates) == 0 {
 		return "", errors.Errorf("no suitable release available for release marker %s", releaseMarker)
 	}
-	return picked.String(), nil
+
+	// Sort parsed versions by semantic version order.
+	sort.SliceStable(versionCandidates, func(i, j int) bool {
+		// Prioritize pre-release versions over releases. For example v2.0.0-alpha > v1.0.0
+		// If both are pre-releases, sort by semantic version order as usual.
+		if len(versionCandidates[i].Pre) == 0 && len(versionCandidates[j].Pre) > 0 {
+			return false
+		}
+		if len(versionCandidates[j].Pre) == 0 && len(versionCandidates[i].Pre) > 0 {
+			return true
+		}
+
+		return versionCandidates[j].LT(versionCandidates[i])
+	})
+
+	// Limit the number of searchable versions by 5.
+	versionCandidates = versionCandidates[:min(5, len(versionCandidates))]
+
+	for _, v := range versionCandidates {
+		// Iterate through sorted versions and try to fetch a file from that release.
+		// If it's completed successfully, we get the latest release.
+		// Note: the fetched file will be cached and next time we will get it from the cache.
+		versionString := "v" + v.String()
+		_, err := httpGetURL(ctx, toMetadataURL(gomodule, versionString))
+		if err != nil {
+			if errors.Is(err, errNotFound) {
+				// Ignore this version
+				continue
+			}
+
+			return "", err
+		}
+
+		return v.String(), nil
+	}
+
+	// If we reached this point, it means we didn't find any release.
+	return "", errors.New("failed to find releases tagged with a valid semantic version number")
+}
+
+var (
+	retryableOperationInterval = 10 * time.Second
+	retryableOperationTimeout  = 1 * time.Minute
+	errNotFound                = errors.New("404 Not Found")
+)
+
+func githubReleaseMetadataURL(gomodule, version string) string {
+	// Rewrite gomodule to the github repository
+	if strings.HasPrefix(gomodule, "k8s.io") {
+		gomodule = strings.Replace(gomodule, "k8s.io", "github.com/kubernetes", 1)
+	}
+	if strings.HasPrefix(gomodule, "sigs.k8s.io") {
+		gomodule = strings.Replace(gomodule, "sigs.k8s.io", "github.com/kubernetes-sigs", 1)
+	}
+
+	return fmt.Sprintf("https://%s/releases/download/%s/metadata.yaml", gomodule, version)
+}
+
+// httpGetURL does a GET request to the given url and returns its content.
+// If the responses StatusCode is 404 (StatusNotFound) it does not do a retry because
+// the result is not expected to change.
+func httpGetURL(ctx context.Context, url string) ([]byte, error) {
+	var retryError error
+	var content []byte
+	_ = wait.PollUntilContextTimeout(ctx, retryableOperationInterval, retryableOperationTimeout, true, func(context.Context) (bool, error) {
+		resp, err := http.Get(url) //nolint:gosec,noctx
+		if err != nil {
+			retryError = errors.Wrap(err, "error sending request")
+			return false, nil
+		}
+		defer resp.Body.Close()
+
+		// if we get 404 there is no reason to retry
+		if resp.StatusCode == http.StatusNotFound {
+			retryError = errNotFound
+			return true, nil
+		}
+
+		if resp.StatusCode != http.StatusOK {
+			retryError = errors.Errorf("error getting file, status code: %d", resp.StatusCode)
+			return false, nil
+		}
+
+		content, err = io.ReadAll(resp.Body)
+		if err != nil {
+			retryError = errors.Wrap(err, "error reading response body")
+			return false, nil
+		}
+
+		retryError = nil
+		return true, nil
+	})
+	if retryError != nil {
+		return nil, retryError
+	}
+	return content, nil
 }
 
 // Defaults assigns default values to the object. More specifically:

test/framework/clusterctl/e2e_config_test.go

@@ -20,6 +20,8 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"net/http/httptest"
+	"strings"
 	"testing"
 
 	. "github.com/onsi/gomega"
@@ -33,14 +35,44 @@ func Test_resolveReleaseMarker(t *testing.T) {
 	clientGoproxy := goproxy.NewClient(scheme, host)
 	defer teardownGoproxy()
 
-	// setup an handler with fake releases
+	toMetadataURL, mux, teardown := newFakeGithubReleases()
+	defer teardown()
+
+	validMetadataURLs := []string{
+		"github.com/o/r1/releases/download/v1.2.0/metadata.yaml",
+		"github.com/o/r1/releases/download/v1.2.1-rc.0/metadata.yaml",
+		"github.com/o/r2/releases/download/v1.2.0/metadata.yaml",
+		"github.com/kubernetes-sigs/foo/releases/download/v1.0.0/metadata.yaml",
+	}
+	// Setup an handler for returning metadata.yaml.
+	for _, url := range validMetadataURLs {
+		mux.HandleFunc("/"+url, func(w http.ResponseWriter, r *http.Request) {
+			goproxytest.HTTPTestMethod(t, r, "GET")
+			fmt.Fprint(w, `somedata`)
+		})
+	}
+
+	// setup an handlers with fake releases
 	muxGoproxy.HandleFunc("/github.com/o/r1/@v/list", func(w http.ResponseWriter, r *http.Request) {
 		goproxytest.HTTPTestMethod(t, r, "GET")
 		fmt.Fprint(w, "v1.2.0\n")
 		fmt.Fprint(w, "v1.2.1-rc.0\n")
 		fmt.Fprint(w, "v1.3.0-rc.0\n")
 		fmt.Fprint(w, "v1.3.0-rc.1\n")
 	})
+	muxGoproxy.HandleFunc("/github.com/o/r2/@v/list", func(w http.ResponseWriter, r *http.Request) {
+		goproxytest.HTTPTestMethod(t, r, "GET")
+		fmt.Fprint(w, "v1.2.0\n")
+		fmt.Fprint(w, "v1.2.1\n")
+		fmt.Fprint(w, "v1.2.2\n")
+	})
+	muxGoproxy.HandleFunc("/sigs.k8s.io/foo/@v/list", func(w http.ResponseWriter, r *http.Request) {
+		goproxytest.HTTPTestMethod(t, r, "GET")
+		fmt.Fprint(w, "v1.0.0\n")
+		fmt.Fprint(w, "v1.0.1\n")
+		fmt.Fprint(w, "v1.0.2\n")
+	})
+
 	tests := []struct {
 		name          string
 		releaseMarker string
@@ -71,13 +103,25 @@ func Test_resolveReleaseMarker(t *testing.T) {
 			want:          "",
 			wantErr:       true,
 		},
+		{
+			name:          "Get latest release with metadata",
+			releaseMarker: "go://github.com/o/r2@latest-v1.2",
+			want:          "1.2.0",
+			wantErr:       false,
+		},
+		{
+			name:          "Get latest release for kubernetes-sigs project",
+			releaseMarker: "go://sigs.k8s.io/foo@latest-v1.0",
+			want:          "1.0.0",
+			wantErr:       false,
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			got, err := resolveReleaseMarker(context.Background(), tt.releaseMarker, clientGoproxy)
+			got, err := resolveReleaseMarker(context.Background(), tt.releaseMarker, clientGoproxy, toMetadataURL)
 			if tt.wantErr {
 				g.Expect(err).To(HaveOccurred())
 				return
@@ -159,3 +203,24 @@ func Test_E2EConfig_DeepCopy(t *testing.T) {
 		"config1-interval": {"2m", "10s"},
 	}))
 }
+
+// newFakeGithubReleases sets up a test HTTP server along with a github.Client that is
+// configured to talk to that test server. Tests should register handlers on
+// mux which provide mock responses for the API method being tested.
+func newFakeGithubReleases() (toMetadataURL func(gomodule, version string) string, mux *http.ServeMux, teardown func()) {
+	// mux is the HTTP request multiplexer used with the test server.
+	mux = http.NewServeMux()
+
+	apiHandler := http.NewServeMux()
+	apiHandler.Handle("/", mux)
+
+	// server is a test HTTP server used to provide mock API responses.
+	server := httptest.NewServer(apiHandler)
+
+	toMetadataURL = func(gomodule, version string) string {
+		url := githubReleaseMetadataURL(gomodule, version)
+		return fmt.Sprintf("%s/%s", server.URL, strings.TrimPrefix(url, "https://"))
+	}
+
+	return toMetadataURL, mux, server.Close
+}