Merge pull request kubernetes-sigs#7951 from chrischdi/pr-repository-client-remove-redirect-handling

🌱 Remove redirection handling in repository_github client

Author: k8s-ci-robot

cmd/clusterctl/client/repository/repository_github.go

@@ -378,7 +378,7 @@ func (g *gitHubRepository) downloadFilesFromRelease(release *github.RepositoryRe
 	_ = wait.PollImmediate(retryableOperationInterval, retryableOperationTimeout, func() (bool, error) {
 		var redirect string
 		var downloadReleaseError error
-		reader, redirect, downloadReleaseError = client.Repositories.DownloadReleaseAsset(context.TODO(), g.owner, g.repository, *assetID, http.DefaultClient)
+		reader, redirect, downloadReleaseError = client.Repositories.DownloadReleaseAsset(ctx, g.owner, g.repository, *assetID, http.DefaultClient)
 		if downloadReleaseError != nil {
 			retryError = g.handleGithubErr(downloadReleaseError, "failed to download file %q from %q release", *release.TagName, fileName)
 			// return immediately if we are rate limited
@@ -388,19 +388,11 @@ func (g *gitHubRepository) downloadFilesFromRelease(release *github.RepositoryRe
 			return false, nil
 		}
 		if redirect != "" {
-			req, err := http.NewRequestWithContext(ctx, http.MethodGet, redirect, http.NoBody)
-			if err != nil {
-				retryError = errors.Wrapf(err, "failed to download file %q from %q release via redirect location %q: failed to create request", *release.TagName, fileName, redirect)
-				return false, nil
-			}
-
-			response, err := http.DefaultClient.Do(req) //nolint:bodyclose // (NB: The reader is actually closed in a defer)
-			if err != nil {
-				retryError = errors.Wrapf(err, "failed to download file %q from %q release via redirect location %q", *release.TagName, fileName, redirect)
-				return false, nil
-			}
-			reader = response.Body
+			// NOTE: DownloadReleaseAsset should not return a redirect address when used with the DefaultClient
+			retryError = errors.New("unexpected redirect while downloading the release asset")
+			return true, retryError
 		}
+
 		retryError = nil
 		return true, nil
 	})

cmd/clusterctl/client/repository/repository_github_test.go

@@ -719,7 +719,8 @@ func Test_gitHubRepository_downloadFilesFromRelease(t *testing.T) {
 	client, mux, teardown := test.NewFakeGitHub()
 	defer teardown()
 
-	providerConfig := config.NewProvider("test", "https://github.com/o/r/releases/v0.4.1/file.yaml", clusterctlv1.CoreProviderType) // tree/main/path not relevant for the test
+	providerConfig := config.NewProvider("test", "https://github.com/o/r/releases/v0.4.1/file.yaml", clusterctlv1.CoreProviderType)                           // tree/main/path not relevant for the test
+	providerConfigWithRedirect := config.NewProvider("test", "https://github.com/o/r-with-redirect/releases/v0.4.1/file.yaml", clusterctlv1.CoreProviderType) // tree/main/path not relevant for the test
 
 	// test.NewFakeGitHub an handler for returning a fake release asset
 	mux.HandleFunc("/repos/o/r/releases/assets/1", func(w http.ResponseWriter, r *http.Request) {
@@ -728,6 +729,11 @@ func Test_gitHubRepository_downloadFilesFromRelease(t *testing.T) {
 		w.Header().Set("Content-Disposition", "attachment; filename=file.yaml")
 		fmt.Fprint(w, "content")
 	})
+	// handler which redirects to a different location
+	mux.HandleFunc("/repos/o/r-with-redirect/releases/assets/1", func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "GET")
+		http.Redirect(w, r, "/api-v3/repos/o/r/releases/assets/1", http.StatusFound)
+	})
 
 	configVariablesClient := test.NewFakeVariableClient()
 
@@ -741,10 +747,11 @@ func Test_gitHubRepository_downloadFilesFromRelease(t *testing.T) {
 		fileName string
 	}
 	tests := []struct {
-		name    string
-		args    args
-		want    []byte
-		wantErr bool
+		name           string
+		args           args
+		providerConfig config.Provider
+		want           []byte
+		wantErr        bool
 	}{
 		{
 			name: "Pass if file exists",
@@ -760,8 +767,27 @@ func Test_gitHubRepository_downloadFilesFromRelease(t *testing.T) {
 				},
 				fileName: file,
 			},
-			want:    []byte("content"),
-			wantErr: false,
+			providerConfig: providerConfig,
+			want:           []byte("content"),
+			wantErr:        false,
+		},
+		{
+			name: "Pass if file exists with redirect",
+			args: args{
+				release: &github.RepositoryRelease{
+					TagName: &tagName,
+					Assets: []*github.ReleaseAsset{
+						{
+							ID:   &id1,
+							Name: &file,
+						},
+					},
+				},
+				fileName: file,
+			},
+			providerConfig: providerConfigWithRedirect,
+			want:           []byte("content"),
+			wantErr:        false,
 		},
 		{
 			name: "Fails if file does not exists",
@@ -777,7 +803,8 @@ func Test_gitHubRepository_downloadFilesFromRelease(t *testing.T) {
 				},
 				fileName: "another file",
 			},
-			wantErr: true,
+			providerConfig: providerConfig,
+			wantErr:        true,
 		},
 		{
 			name: "Fails if file does not exists",
@@ -793,15 +820,16 @@ func Test_gitHubRepository_downloadFilesFromRelease(t *testing.T) {
 				},
 				fileName: "another file",
 			},
-			wantErr: true,
+			providerConfig: providerConfig,
+			wantErr:        true,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 			resetCaches()
 
-			gRepo, err := NewGitHubRepository(providerConfig, configVariablesClient, injectGithubClient(client))
+			gRepo, err := NewGitHubRepository(tt.providerConfig, configVariablesClient, injectGithubClient(client))
 			g.Expect(err).NotTo(HaveOccurred())
 
 			got, err := gRepo.(*gitHubRepository).downloadFilesFromRelease(tt.args.release, tt.args.fileName)