Replace some stalling event by normal event in HelmChart and

souleb · souleb · commit 00a888e35aae · 2022-05-24T10:39:15.000+02:00
HelmRepository_OCI reconcilers to make to retry on failure The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <soule@weave.works>
diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go
@@ -512,7 +512,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 	case sourcev1.HelmRepositoryTypeOCI:
 		if !registry.IsOCI(repo.Spec.URL) {
 			err := fmt.Errorf("invalid OCI registry URL: %s", repo.Spec.URL)
-			return chartRepoErrorReturn(err, obj)
+			return chartRepoConfigErrorReturn(err, obj)
 		}
 
 		// with this function call, we create a temporary file to store the credentials if needed.
@@ -521,7 +521,12 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		// or rework to enable reusing credentials to avoid the unneccessary handshake operations
 		registryClient, file, err := r.RegistryClientGenerator(logOpts != nil)
 		if err != nil {
-			return chartRepoErrorReturn(err, obj)
+			e := &serror.Event{
+				Err:    fmt.Errorf("failed to construct Helm client: %w", err),
+				Reason: meta.FailedReason,
+			}
+			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
+			return sreconcile.ResultEmpty, e
 		}
 
 		if file != "" {
@@ -534,7 +539,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient))
 		ociChartRepo, err := repository.NewOCIChartRepository(repo.Spec.URL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient))
 		if err != nil {
-			return chartRepoErrorReturn(err, obj)
+			return chartRepoConfigErrorReturn(err, obj)
 		}
 		chartRepo = ociChartRepo
 
@@ -543,7 +548,12 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		if logOpts != nil {
 			err = ociChartRepo.Login(logOpts...)
 			if err != nil {
-				return chartRepoErrorReturn(err, obj)
+				e := &serror.Event{
+					Err:    fmt.Errorf("failed to login to OCI registry: %w", err),
+					Reason: meta.FailedReason,
+				}
+				conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
+				return sreconcile.ResultEmpty, e
 			}
 		}
 	default:
@@ -553,7 +563,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 				r.IncCacheEvents(event, obj.Name, obj.Namespace)
 			}))
 		if err != nil {
-			return chartRepoErrorReturn(err, obj)
+			return chartRepoConfigErrorReturn(err, obj)
 		}
 		chartRepo = httpChartRepo
 		defer func() {
@@ -1142,7 +1152,7 @@ func reasonForBuild(build *chart.Build) string {
 	return sourcev1.ChartPullSucceededReason
 }
 
-func chartRepoErrorReturn(err error, obj *sourcev1.HelmChart) (sreconcile.Result, error) {
+func chartRepoConfigErrorReturn(err error, obj *sourcev1.HelmChart) (sreconcile.Result, error) {
 	switch err.(type) {
 	case *url.Error:
 		e := &serror.Stalling{
diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go
@@ -791,8 +791,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 	)
 
 	// Login to the registry
-	err := testRegistryserver.RegistryClient.Login(testRegistryserver.DockerRegistryHost,
-		registry.LoginOptBasicAuth(testUsername, testPassword),
+	err := testRegistryserver.registryClient.Login(testRegistryserver.dockerRegistryHost,
+		registry.LoginOptBasicAuth(testRegistryUsername, testRegistryPassword),
 		registry.LoginOptInsecure(true))
 	g.Expect(err).NotTo(HaveOccurred())
 
@@ -803,8 +803,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 	g.Expect(err).NotTo(HaveOccurred())
 
 	// Upload the test chart
-	ref := fmt.Sprintf("%s/testrepo/%s:%s", testRegistryserver.DockerRegistryHost, metadata.Name, metadata.Version)
-	_, err = testRegistryserver.RegistryClient.Push(chartData, ref)
+	ref := fmt.Sprintf("%s/testrepo/%s:%s", testRegistryserver.dockerRegistryHost, metadata.Name, metadata.Version)
+	_, err = testRegistryserver.registryClient.Push(chartData, ref)
 	g.Expect(err).NotTo(HaveOccurred())
 
 	storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
@@ -832,8 +832,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 					Name: "auth",
 				},
 				Data: map[string][]byte{
-					"username": []byte(testUsername),
-					"password": []byte(testPassword),
+					"username": []byte(testRegistryUsername),
+					"password": []byte(testRegistryPassword),
 				},
 			},
 			beforeFunc: func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository) {
@@ -953,7 +953,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 					GenerateName: "helmrepository-",
 				},
 				Spec: sourcev1.HelmRepositorySpec{
-					URL:     fmt.Sprintf("oci://%s/testrepo", testRegistryserver.DockerRegistryHost),
+					URL:     fmt.Sprintf("oci://%s/testrepo", testRegistryserver.dockerRegistryHost),
 					Timeout: &metav1.Duration{Duration: timeout},
 					Type:    sourcev1.HelmRepositoryTypeOCI,
 				},
diff --git a/controllers/helmrepository_controller_oci.go b/controllers/helmrepository_controller_oci.go
@@ -287,19 +287,15 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *
 		logOpts = append(logOpts, logOpt)
 	}
 
-	if result, err := r.validateSource(ctx, obj, logOpts...); err != nil || result == sreconcile.ResultEmpty {
-		return result, err
-	}
-
-	return sreconcile.ResultSuccess, nil
+	return r.validateSource(ctx, obj, logOpts...)
 }
 
 // validateSource the HelmRepository object by checking the url and connecting to the underlying registry
 // with he provided credentials.
 func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *sourcev1.HelmRepository, logOpts ...registry.LoginOption) (sreconcile.Result, error) {
 	registryClient, file, err := r.RegistryClientGenerator(logOpts != nil)
 	if err != nil {
-		e := &serror.Stalling{
+		e := &serror.Event{
 			Err:    fmt.Errorf("failed to create registry client:: %w", err),
 			Reason: meta.FailedReason,
 		}
diff --git a/controllers/helmrepository_controller_oci_test.go b/controllers/helmrepository_controller_oci_test.go
@@ -46,8 +46,8 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
 			Namespace:    ns.Name,
 		},
 		Data: map[string][]byte{
-			"username": []byte(testUsername),
-			"password": []byte(testPassword),
+			"username": []byte(testRegistryUsername),
+			"password": []byte(testRegistryPassword),
 		},
 	}
 
@@ -60,7 +60,7 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
 		},
 		Spec: sourcev1.HelmRepositorySpec{
 			Interval: metav1.Duration{Duration: interval},
-			URL:      fmt.Sprintf("oci://%s", testRegistryserver.DockerRegistryHost),
+			URL:      fmt.Sprintf("oci://%s", testRegistryserver.dockerRegistryHost),
 			SecretRef: &meta.LocalObjectReference{
 				Name: secret.Name,
 			},
diff --git a/controllers/helmrepository_controller_test.go b/controllers/helmrepository_controller_test.go
@@ -1109,7 +1109,7 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing.
 			URL:      testServer.URL(),
 		},
 	}
-	g.Expect(testEnv.Create(ctx, obj)).To(Succeed())
+	g.Expect(testEnv.CreateAndWait(ctx, obj)).To(Succeed())
 
 	key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace}
 
@@ -1154,14 +1154,14 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing.
 			Namespace:    "default",
 		},
 		Data: map[string][]byte{
-			"username": []byte(testUsername),
-			"password": []byte(testPassword),
+			"username": []byte(testRegistryUsername),
+			"password": []byte(testRegistryPassword),
 		},
 	}
 	g.Expect(testEnv.CreateAndWait(ctx, secret)).To(Succeed())
 
 	obj.Spec.Type = sourcev1.HelmRepositoryTypeOCI
-	obj.Spec.URL = fmt.Sprintf("oci://%s", testRegistryserver.DockerRegistryHost)
+	obj.Spec.URL = fmt.Sprintf("oci://%s", testRegistryserver.dockerRegistryHost)
 	obj.Spec.SecretRef = &meta.LocalObjectReference{
 		Name: secret.Name,
 	}
@@ -1221,7 +1221,7 @@ func TestHelmRepositoryReconciler_ReconcileSpecUpdatePredicateFilter(t *testing.
 			URL:      testServer.URL(),
 		},
 	}
-	g.Expect(testEnv.Create(ctx, obj)).To(Succeed())
+	g.Expect(testEnv.CreateAndWait(ctx, obj)).To(Succeed())
 
 	key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace}
 
@@ -1268,7 +1268,7 @@ func TestHelmRepositoryReconciler_ReconcileSpecUpdatePredicateFilter(t *testing.
 		if err := testEnv.Get(ctx, key, obj); err != nil {
 			return false
 		}
-		if !conditions.IsReady(obj) {
+		if !conditions.IsReady(obj) && obj.Status.Artifact == nil {
 			return false
 		}
 		readyCondition := conditions.Get(obj, meta.ReadyCondition)
diff --git a/controllers/suite_test.go b/controllers/suite_test.go
@@ -93,69 +93,66 @@ var (
 )
 
 var (
-	testRegistryClient *registry.Client
-	testRegistryserver *RegistryClientTestServer
+	testRegistryserver *registryClientTestServer
 )
 
 var (
-	testWorkspaceDir         = "registry-test"
-	testHtpasswdFileBasename = "authtest.htpasswd"
-	testUsername             = "myuser"
-	testPassword             = "mypass"
+	testRegistryWorkspaceDir         = "/tmp/registry-test"
+	testRegistryHtpasswdFileBasename = "authtest.htpasswd"
+	testRegistryUsername             = "myuser"
+	testRegistryPassword             = "mypass"
 )
 
 func init() {
 	rand.Seed(time.Now().UnixNano())
 }
 
-type RegistryClientTestServer struct {
-	Out                io.Writer
-	DockerRegistryHost string
-	WorkspaceDir       string
-	RegistryClient     *registry.Client
+type registryClientTestServer struct {
+	out                io.Writer
+	dockerRegistryHost string
+	workspaceDir       string
+	registryClient     *registry.Client
 }
 
-func SetupServer(server *RegistryClientTestServer) string {
+func setupRegistryServer(ctx context.Context) (*registryClientTestServer, error) {
+	server := &registryClientTestServer{}
 	// Create a temporary workspace directory for the registry
-	server.WorkspaceDir = testWorkspaceDir
-	os.RemoveAll(server.WorkspaceDir)
-	err := os.Mkdir(server.WorkspaceDir, 0700)
+	server.workspaceDir = testRegistryWorkspaceDir
+	os.RemoveAll(server.workspaceDir)
+	err := os.Mkdir(server.workspaceDir, 0o700)
 	if err != nil {
-		panic(fmt.Sprintf("failed to create workspace directory: %s", err))
+		return nil, fmt.Errorf("failed to create workspace directory: %s", err)
 	}
 
 	var out bytes.Buffer
-	server.Out = &out
+	server.out = &out
 
 	// init test client
-	server.RegistryClient, err = registry.NewClient(
+	server.registryClient, err = registry.NewClient(
 		registry.ClientOptDebug(true),
-		registry.ClientOptWriter(server.Out),
+		registry.ClientOptWriter(server.out),
 	)
 	if err != nil {
-		panic(fmt.Sprintf("failed to create registry client: %s", err))
+		return nil, fmt.Errorf("failed to create registry client: %s", err)
 	}
 
 	// create htpasswd file (w BCrypt, which is required)
-	pwBytes, err := bcrypt.GenerateFromPassword([]byte(testPassword), bcrypt.DefaultCost)
+	pwBytes, err := bcrypt.GenerateFromPassword([]byte(testRegistryPassword), bcrypt.DefaultCost)
 	if err != nil {
-		panic(fmt.Sprintf("failed to generate password: %s", err))
+		return nil, fmt.Errorf("failed to generate password: %s", err)
 	}
 
-	htpasswdPath := filepath.Join(testWorkspaceDir, testHtpasswdFileBasename)
-	err = ioutil.WriteFile(htpasswdPath, []byte(fmt.Sprintf("%s:%s\n", testUsername, string(pwBytes))), 0644)
+	htpasswdPath := filepath.Join(testRegistryWorkspaceDir, testRegistryHtpasswdFileBasename)
+	err = ioutil.WriteFile(htpasswdPath, []byte(fmt.Sprintf("%s:%s\n", testRegistryUsername, string(pwBytes))), 0644)
 	if err != nil {
-		panic(fmt.Sprintf("failed to create htpasswd file: %s", err))
+		return nil, fmt.Errorf("failed to create htpasswd file: %s", err)
 	}
 
 	// Registry config
 	config := &configuration.Configuration{}
 	port, err := freeport.GetFreePort()
-	if err != nil {
-		panic(fmt.Sprintf("failed to get free port: %s", err))
-	}
 
-	server.DockerRegistryHost = fmt.Sprintf("localhost:%d", port)
+	server.dockerRegistryHost = fmt.Sprintf("localhost:%d", port)
 	config.HTTP.Addr = fmt.Sprintf("127.0.0.1:%d", port)
 	config.HTTP.DrainTimeout = time.Duration(10) * time.Second
 	config.Storage = map[string]configuration.Parameters{"inmemory": map[string]interface{}{}}
@@ -165,15 +162,15 @@ func SetupServer(server *RegistryClientTestServer) string {
 			"path":  htpasswdPath,
 		},
 	}
-	dockerRegistry, err := dockerRegistry.NewRegistry(context.Background(), config)
+	dockerRegistry, err := dockerRegistry.NewRegistry(ctx, config)
 	if err != nil {
-		panic(fmt.Sprintf("failed to create docker registry: %s", err))
+		return nil, fmt.Errorf("failed to create docker registry: %s", err)
 	}
 
 	// Start Docker registry
 	go dockerRegistry.ListenAndServe()
 
-	return server.WorkspaceDir
+	return server, nil
 }
 
 func TestMain(m *testing.M) {
@@ -198,12 +195,9 @@ func TestMain(m *testing.M) {
 
 	testMetricsH = controller.MustMakeMetrics(testEnv)
 
-	testRegistryserver = &RegistryClientTestServer{}
-	registryWorkspaceDir := SetupServer(testRegistryserver)
-
-	testRegistryClient, err = registry.NewClient(registry.ClientOptWriter(os.Stdout))
+	testRegistryserver, err = setupRegistryServer(ctx)
 	if err != nil {
-		panic(fmt.Sprintf("Failed to create OCI registry client"))
+		panic(fmt.Sprintf("Failed to create a test registry server: %v", err))
 	}
 
 	if err := (&GitRepositoryReconciler{
@@ -280,7 +274,7 @@ func TestMain(m *testing.M) {
 		panic(fmt.Sprintf("Failed to remove storage server dir: %v", err))
 	}
 
-	if err := os.RemoveAll(registryWorkspaceDir); err != nil {
+	if err := os.RemoveAll(testRegistryserver.workspaceDir); err != nil {
 		panic(fmt.Sprintf("Failed to remove registry workspace dir: %v", err))
 	}
 
