🌱 Create a Generic client to download release assets

create a generic assetsclient interface

Current code is opinionated towards the github client for
downloading the assets, instead we need a more generic approach that can
incorporate other plugins (oci-registry) too.

Signed-off-by: Thomas Guettler <[email protected]>
Signed-off-by: janiskemper <[email protected]>

Author: janiskemper

api/v1alpha1/conditions_const.go

@@ -78,17 +78,17 @@ const (
 )
 
 const (
-	// GitAPIAvailableCondition is used when Git API is available.
-	GitAPIAvailableCondition clusterv1.ConditionType = "GitAPIAvailable"
+	// AssetsClientAPIAvailableCondition is used when AssetsClient API is available.
+	AssetsClientAPIAvailableCondition clusterv1.ConditionType = "AssetsClientAPIAvailable"
 
-	// GitTokenOrEnvVariableNotSetReason is used when user don't specify the token or environment variable.
-	GitTokenOrEnvVariableNotSetReason = "GitTokenOrEnvVariableNotSet" //#nosec
+	// FailedCreateAssetsClientReason is used when user don't specify the token or environment variable required for initializing the assets client.
+	FailedCreateAssetsClientReason = "FailedCreateAssetsClient" //#nosec
 )
 
 const (
-	// GitReleasesSyncedCondition is used when Git releases have been synced successfully.
-	GitReleasesSyncedCondition clusterv1.ConditionType = "GitReleasesSynced"
+	// ReleasesSyncedCondition is used when releases have been synced successfully.
+	ReleasesSyncedCondition clusterv1.ConditionType = "ReleasesSynced"
 
-	// FailedToSyncReason is used when Git releases could not be synced.
+	// FailedToSyncReason is used when releases could not be synced.
 	FailedToSyncReason = "FailedToSync"
 )

cmd/main.go

@@ -27,9 +27,10 @@ import (
 	//+kubebuilder:scaffold:imports
 	csov1alpha1 "github.com/SovereignCloudStack/cluster-stack-operator/api/v1alpha1"
 	"github.com/SovereignCloudStack/cluster-stack-operator/internal/controller"
+	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/assetsclient"
+	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/assetsclient/fake"
+	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/assetsclient/github"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/csoversion"
-	githubclient "github.com/SovereignCloudStack/cluster-stack-operator/pkg/github/client"
-	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/github/client/fake"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/kube"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/utillog"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/workloadcluster"
@@ -131,11 +132,11 @@ func main() {
 	// Setup the context that's going to be used in controllers and for the manager.
 	ctx := ctrl.SetupSignalHandler()
 
-	var gitFactory githubclient.Factory
+	var assetsClientFactory assetsclient.Factory
 	if localMode {
-		gitFactory = fake.NewFactory()
+		assetsClientFactory = fake.NewFactory()
 	} else {
-		gitFactory = githubclient.NewFactory()
+		assetsClientFactory = github.NewFactory()
 	}
 
 	restConfig := mgr.GetConfig()
@@ -153,7 +154,7 @@ func main() {
 	if err = (&controller.ClusterStackReconciler{
 		Client:              mgr.GetClient(),
 		ReleaseDirectory:    releaseDir,
-		GitHubClientFactory: gitFactory,
+		AssetsClientFactory: assetsClientFactory,
 		WatchFilterValue:    watchFilterValue,
 	}).SetupWithManager(ctx, mgr, controllerruntimecontroller.Options{MaxConcurrentReconciles: clusterStackConcurrency}); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "ClusterStack")
@@ -166,7 +167,7 @@ func main() {
 		ReleaseDirectory:    releaseDir,
 		WatchFilterValue:    watchFilterValue,
 		KubeClientFactory:   kube.NewFactory(),
-		GitHubClientFactory: gitFactory,
+		AssetsClientFactory: assetsClientFactory,
 	}).SetupWithManager(ctx, mgr, controllerruntimecontroller.Options{MaxConcurrentReconciles: clusterStackReleaseConcurrency}); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "ClusterStackRelease")
 		os.Exit(1)
@@ -179,6 +180,7 @@ func main() {
 		WatchFilterValue:       watchFilterValue,
 		KubeClientFactory:      kube.NewFactory(),
 		WorkloadClusterFactory: workloadcluster.NewFactory(),
+		AssetsClientFactory:    assetsClientFactory,
 	}).SetupWithManager(ctx, mgr, controllerruntimecontroller.Options{MaxConcurrentReconciles: clusterAddonConcurrency}); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "ClusterAddon")
 		os.Exit(1)

internal/controller/clusteraddon_controller.go

@@ -27,6 +27,7 @@ import (
 	"time"
 
 	csov1alpha1 "github.com/SovereignCloudStack/cluster-stack-operator/api/v1alpha1"
+	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/assetsclient"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/kube"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/release"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/workloadcluster"
@@ -67,6 +68,7 @@ type ClusterAddonReconciler struct {
 	RestConfigSettings
 	ReleaseDirectory       string
 	KubeClientFactory      kube.Factory
+	AssetsClientFactory    assetsclient.Factory
 	WatchFilterValue       string
 	WorkloadClusterFactory workloadcluster.Factory
 }

internal/controller/clusterstack_controller.go

@@ -23,11 +23,12 @@ import (
 	"reflect"
 	"sort"
 	"strings"
+	"time"
 
 	csov1alpha1 "github.com/SovereignCloudStack/cluster-stack-operator/api/v1alpha1"
 	"github.com/SovereignCloudStack/cluster-stack-operator/internal/clusterstackrelease"
+	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/assetsclient"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/clusterstack"
-	githubclient "github.com/SovereignCloudStack/cluster-stack-operator/pkg/github/client"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/version"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -55,7 +56,7 @@ import (
 // ClusterStackReconciler reconciles a ClusterStack object.
 type ClusterStackReconciler struct {
 	client.Client
-	GitHubClientFactory githubclient.Factory
+	AssetsClientFactory assetsclient.Factory
 	ReleaseDirectory    string
 	WatchFilterValue    string
 }
@@ -112,32 +113,39 @@ func (r *ClusterStackReconciler) Reconcile(ctx context.Context, req reconcile.Re
 	var latest *string
 
 	if clusterStack.Spec.AutoSubscribe {
-		gc, err := r.GitHubClientFactory.NewClient(ctx)
+		gc, err := r.AssetsClientFactory.NewClient(ctx)
 		if err != nil {
+			isSet := conditions.IsFalse(clusterStack, csov1alpha1.AssetsClientAPIAvailableCondition)
 			conditions.MarkFalse(clusterStack,
-				csov1alpha1.GitAPIAvailableCondition,
-				csov1alpha1.GitTokenOrEnvVariableNotSetReason,
+				csov1alpha1.AssetsClientAPIAvailableCondition,
+				csov1alpha1.FailedCreateAssetsClientReason,
 				clusterv1.ConditionSeverityError,
 				err.Error(),
 			)
-			record.Warnf(clusterStack, "GitTokenOrEnvVariableNotSet", err.Error())
-			return reconcile.Result{}, fmt.Errorf("failed to create Github client: %w", err)
+			record.Warnf(clusterStack, "FailedCreateAssetsClient", err.Error())
+
+			// give the assets client a second change
+			if isSet {
+				return reconcile.Result{RequeueAfter: 5 * time.Second}, nil
+			}
+			return reconcile.Result{}, nil
 		}
 
-		conditions.MarkTrue(clusterStack, csov1alpha1.GitAPIAvailableCondition)
+		conditions.MarkTrue(clusterStack, csov1alpha1.AssetsClientAPIAvailableCondition)
+
 		latest, err = getLatestReleaseFromRemoteRepository(ctx, clusterStack, gc)
 		if err != nil {
 			// only log error and mark condition as false, but continue
 			conditions.MarkFalse(clusterStack,
-				csov1alpha1.GitReleasesSyncedCondition,
+				csov1alpha1.ReleasesSyncedCondition,
 				csov1alpha1.FailedToSyncReason,
 				clusterv1.ConditionSeverityWarning,
 				err.Error(),
 			)
 			logger.Error(err, "failed to get latest release from remote repository")
 		}
 
-		conditions.MarkTrue(clusterStack, csov1alpha1.GitReleasesSyncedCondition)
+		conditions.MarkTrue(clusterStack, csov1alpha1.ReleasesSyncedCondition)
 	}
 
 	inUse, err := r.getClusterStackReleasesInUse(ctx, req.Namespace)
@@ -536,21 +544,18 @@ func getLatestReadyClusterStackRelease(clusterStackReleases []*csov1alpha1.Clust
 	return latest, k8sversion, nil
 }
 
-func getLatestReleaseFromRemoteRepository(ctx context.Context, clusterStack *csov1alpha1.ClusterStack, gc githubclient.Client) (*string, error) {
-	ghReleases, resp, err := gc.ListRelease(ctx)
+func getLatestReleaseFromRemoteRepository(ctx context.Context, clusterStack *csov1alpha1.ClusterStack, ac assetsclient.Client) (*string, error) {
+	releases, err := ac.ListRelease(ctx)
 	if err != nil {
-		return nil, fmt.Errorf("failed to list releases on remote Git repository: %w", err)
-	}
-	if resp != nil && resp.StatusCode != 200 {
-		return nil, fmt.Errorf("got unexpected status from call to remote Git repository: %s", resp.Status)
+		return nil, fmt.Errorf("failed to list releases on remote repository: %w", err)
 	}
 
 	var clusterStacks clusterstack.ClusterStacks
 
-	for _, ghRelease := range ghReleases {
-		clusterStackObject, matches, err := matchesSpec(ghRelease.GetTagName(), &clusterStack.Spec)
+	for _, release := range releases {
+		clusterStackObject, matches, err := matchesSpec(release, &clusterStack.Spec)
 		if err != nil {
-			return nil, fmt.Errorf("failed to get match release tag %q with spec of ClusterStack: %w", ghRelease.GetTagName(), err)
+			return nil, fmt.Errorf("failed to get match release tag %q with spec of ClusterStack: %w", release, err)
 		}
 
 		if matches {

internal/controller/clusterstackrelease_controller.go

@@ -20,7 +20,6 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	"net/http"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -29,8 +28,8 @@ import (
 	"time"
 
 	csov1alpha1 "github.com/SovereignCloudStack/cluster-stack-operator/api/v1alpha1"
+	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/assetsclient"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/clusterstack"
-	githubclient "github.com/SovereignCloudStack/cluster-stack-operator/pkg/github/client"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/kube"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/release"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -57,7 +56,7 @@ type ClusterStackReleaseReconciler struct {
 	RESTConfig                            *rest.Config
 	ReleaseDirectory                      string
 	KubeClientFactory                     kube.Factory
-	GitHubClientFactory                   githubclient.Factory
+	AssetsClientFactory                   assetsclient.Factory
 	externalTracker                       external.ObjectTracker
 	clusterStackRelDownloadDirectoryMutex sync.Mutex
 	WatchFilterValue                      string
@@ -116,27 +115,32 @@ func (r *ClusterStackReleaseReconciler) Reconcile(ctx context.Context, req recon
 	if download {
 		conditions.MarkFalse(clusterStackRelease, csov1alpha1.ClusterStackReleaseAssetsReadyCondition, csov1alpha1.ReleaseAssetsNotDownloadedYetReason, clusterv1.ConditionSeverityInfo, "assets not downloaded yet")
 
-		// this is the point where we download the release from github
+		// this is the point where we download the release
 		// acquire lock so that only one reconcile loop can download the release
 		r.clusterStackRelDownloadDirectoryMutex.Lock()
-
 		defer r.clusterStackRelDownloadDirectoryMutex.Unlock()
 
-		gc, err := r.GitHubClientFactory.NewClient(ctx)
+		ac, err := r.AssetsClientFactory.NewClient(ctx)
 		if err != nil {
+			isSet := conditions.IsFalse(clusterStackRelease, csov1alpha1.AssetsClientAPIAvailableCondition)
 			conditions.MarkFalse(clusterStackRelease,
-				csov1alpha1.GitAPIAvailableCondition,
-				csov1alpha1.GitTokenOrEnvVariableNotSetReason,
+				csov1alpha1.AssetsClientAPIAvailableCondition,
+				csov1alpha1.FailedCreateAssetsClientReason,
 				clusterv1.ConditionSeverityError,
 				err.Error(),
 			)
-			record.Warnf(clusterStackRelease, "GitTokenOrEnvVariableNotSet", err.Error())
-			return reconcile.Result{}, fmt.Errorf("failed to create Github client: %w", err)
+			record.Warnf(clusterStackRelease, "FailedCreateAssetsClient", err.Error())
+
+			// give the assets client a second change
+			if isSet {
+				return reconcile.Result{RequeueAfter: 5 * time.Second}, nil
+			}
+			return reconcile.Result{}, nil
 		}
 
-		conditions.MarkTrue(clusterStackRelease, csov1alpha1.GitAPIAvailableCondition)
+		conditions.MarkTrue(clusterStackRelease, csov1alpha1.AssetsClientAPIAvailableCondition)
 
-		if err := downloadReleaseAssets(ctx, releaseTag, releaseAssets.LocalDownloadPath, gc); err != nil {
+		if err := downloadReleaseAssets(ctx, releaseTag, releaseAssets.LocalDownloadPath, ac); err != nil {
 			return reconcile.Result{}, fmt.Errorf("failed to download release assets: %w", err)
 		}
 
@@ -215,18 +219,8 @@ func (r *ClusterStackReleaseReconciler) reconcileDelete(ctx context.Context, rel
 	return reconcile.Result{}, nil
 }
 
-func downloadReleaseAssets(ctx context.Context, releaseTag, downloadPath string, gc githubclient.Client) error {
-	repoRelease, resp, err := gc.GetReleaseByTag(ctx, releaseTag)
-	if err != nil {
-		return fmt.Errorf("failed to fetch release tag %q: %w", releaseTag, err)
-	}
-	if resp.StatusCode != http.StatusOK {
-		return fmt.Errorf("failed to fetch release tag %s with status code %d", releaseTag, resp.StatusCode)
-	}
-
-	assetlist := []string{"metadata.yaml", "cluster-addon-values.yaml", "cluster-addon", "cluster-class"}
-
-	if err := gc.DownloadReleaseAssets(ctx, repoRelease, downloadPath, assetlist); err != nil {
+func downloadReleaseAssets(ctx context.Context, releaseTag, downloadPath string, ac assetsclient.Client) error {
+	if err := ac.DownloadReleaseAssets(ctx, releaseTag, downloadPath); err != nil {
 		// if download failed for some reason, delete the release directory so that it can be retried in the next reconciliation
 		if err := os.RemoveAll(downloadPath); err != nil {
 			return fmt.Errorf("failed to remove release: %w", err)

internal/controller/controller_suite_test.go

@@ -58,14 +58,14 @@ var _ = BeforeSuite(func() {
 	Expect((&ClusterStackReconciler{
 		Client:              testEnv.Manager.GetClient(),
 		ReleaseDirectory:    "./../../test/releases",
-		GitHubClientFactory: testEnv.GitHubClientFactory,
+		AssetsClientFactory: testEnv.AssetsClientFactory,
 	}).SetupWithManager(ctx, testEnv.Manager, c.Options{})).To(Succeed())
 
 	Expect((&ClusterStackReleaseReconciler{
 		Client:              testEnv.Manager.GetClient(),
 		RESTConfig:          testEnv.Manager.GetConfig(),
 		KubeClientFactory:   kube.NewFactory(),
-		GitHubClientFactory: testEnv.GitHubClientFactory,
+		AssetsClientFactory: testEnv.AssetsClientFactory,
 		ReleaseDirectory:    "./../../test/releases",
 	}).SetupWithManager(ctx, testEnv.Manager, c.Options{})).To(Succeed())
 

internal/test/helpers/envtest.go

@@ -31,8 +31,8 @@ import (
 
 	csov1alpha1 "github.com/SovereignCloudStack/cluster-stack-operator/api/v1alpha1"
 	"github.com/SovereignCloudStack/cluster-stack-operator/internal/test/helpers/builder"
-	githubclient "github.com/SovereignCloudStack/cluster-stack-operator/pkg/github/client"
-	githubmocks "github.com/SovereignCloudStack/cluster-stack-operator/pkg/github/client/mocks"
+	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/assetsclient"
+	assetsclientmocks "github.com/SovereignCloudStack/cluster-stack-operator/pkg/assetsclient/mocks"
 	kubeclient "github.com/SovereignCloudStack/cluster-stack-operator/pkg/kube"
 	kubemocks "github.com/SovereignCloudStack/cluster-stack-operator/pkg/kube/mocks"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/test/utils"
@@ -147,9 +147,9 @@ type (
 		cancel                context.CancelFunc
 		kind                  *kind.Provider
 		WorkloadClusterClient *kubernetes.Clientset
-		GitHubClientFactory   githubclient.Factory
+		AssetsClientFactory   assetsclient.Factory
 		KubeClientFactory     kubeclient.Factory
-		GitHubClient          *githubmocks.Client
+		AssetsClient          *assetsclientmocks.Client
 		KubeClient            *kubemocks.Client
 	}
 )
@@ -203,16 +203,16 @@ func NewTestEnvironment() *TestEnvironment {
 		klog.Fatalf("unable to create manager pod namespace: %s", err)
 	}
 
-	githubClient := &githubmocks.Client{}
+	assetsClient := &assetsclientmocks.Client{}
 	kubeClient := &kubemocks.Client{}
 
 	testEnv := &TestEnvironment{
 		Manager:             mgr,
 		Client:              mgr.GetClient(),
 		Config:              mgr.GetConfig(),
-		GitHubClientFactory: githubmocks.NewGitHubFactory(githubClient),
+		AssetsClientFactory: assetsclientmocks.NewAssetsClientFactory(assetsClient),
 		KubeClientFactory:   kubemocks.NewKubeFactory(kubeClient),
-		GitHubClient:        githubClient,
+		AssetsClient:        assetsClient,
 		KubeClient:          kubeClient,
 	}
 

internal/test/integration/github/integration_suite_test.go

@@ -20,7 +20,7 @@ import (
 
 	"github.com/SovereignCloudStack/cluster-stack-operator/internal/controller"
 	"github.com/SovereignCloudStack/cluster-stack-operator/internal/test/helpers"
-	githubclient "github.com/SovereignCloudStack/cluster-stack-operator/pkg/github/client"
+	githubclient "github.com/SovereignCloudStack/cluster-stack-operator/pkg/assetsclient/github"
 	"github.com/SovereignCloudStack/cluster-stack-operator/pkg/kube"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -47,14 +47,14 @@ var _ = BeforeSuite(func() {
 	testEnv = helpers.NewTestEnvironment()
 	Expect((&controller.ClusterStackReconciler{
 		Client:              testEnv.Manager.GetClient(),
-		GitHubClientFactory: githubclient.NewFactory(),
+		AssetsClientFactory: githubclient.NewFactory(),
 		ReleaseDirectory:    "/tmp/downloads",
 	}).SetupWithManager(ctx, testEnv.Manager, controllerruntimecontroller.Options{})).To(Succeed())
 	Expect((&controller.ClusterStackReleaseReconciler{
 		Client:              testEnv.Manager.GetClient(),
 		RESTConfig:          testEnv.Manager.GetConfig(),
 		KubeClientFactory:   kube.NewFactory(),
-		GitHubClientFactory: githubclient.NewFactory(),
+		AssetsClientFactory: githubclient.NewFactory(),
 		ReleaseDirectory:    "/tmp/downloads",
 	}).SetupWithManager(ctx, testEnv.Manager, controllerruntimecontroller.Options{})).To(Succeed())
 

internal/test/integration/github/integration_test.go

@@ -69,7 +69,22 @@ var _ = Describe("ClusterStackReconciler", func() {
 		})
 
 		AfterEach(func() {
-			Expect(testEnv.Cleanup(ctx, testNs, clusterStack)).To(Succeed())
+			Eventually(func() error {
+				return testEnv.Cleanup(ctx, testNs, clusterStack)
+			}, timeout, interval).Should(BeNil())
+		})
+
+		It("checks if the AssetsClientAPIAvailableCondition condition is true", func() {
+			Eventually(func() bool {
+				var foundClusterStackRelease csov1alpha1.ClusterStackRelease
+				if err := testEnv.Get(ctx, clusterStackReleaseKey, &foundClusterStackRelease); err != nil {
+					testEnv.GetLogger().Error(err, "failed to get clusterStackRelease", "key", clusterStackReleaseKey)
+					return false
+				}
+
+				testEnv.GetLogger().Info("status condition of cluster stack release", "key", clusterStackReleaseKey, "status condition", foundClusterStackRelease.Status.Conditions)
+				return utils.IsPresentAndTrue(ctx, testEnv.Client, clusterStackReleaseKey, &foundClusterStackRelease, csov1alpha1.AssetsClientAPIAvailableCondition)
+			}, timeout, interval).Should(BeTrue())
 		})
 
 		It("creates the cluster stack release object", func() {