Merge pull request #728 from souleb/revisit-oci-events

Helm reconcilers conditions and test improvements

controllers/helmchart_controller.go

@@ -513,7 +513,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 	case sourcev1.HelmRepositoryTypeOCI:
 		if !helmreg.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.
@@ -522,7 +522,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(loginOpts != 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 != "" {
@@ -538,7 +543,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
 
@@ -547,7 +552,12 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		if loginOpts != nil {
 			err = ociChartRepo.Login(loginOpts...)
 			if err != nil {
-				return chartRepoErrorReturn(err, obj)
+				e := &serror.Event{
+					Err:    fmt.Errorf("failed to login to OCI registry: %w", err),
+					Reason: sourcev1.AuthenticationFailedReason,
+				}
+				conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
+				return sreconcile.ResultEmpty, e
 			}
 		}
 	default:
@@ -556,7 +566,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() {
@@ -1145,7 +1155,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{

controllers/helmchart_controller_test.go

@@ -792,8 +792,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 	)
 
 	// Login to the registry
-	err := testRegistryserver.RegistryClient.Login(testRegistryserver.DockerRegistryHost,
+	err := testRegistryServer.registryClient.Login(testRegistryServer.registryHost,
-		helmreg.LoginOptBasicAuth(testUsername, testPassword),
+		helmreg.LoginOptBasicAuth(testRegistryUsername, testRegistryPassword),
 		helmreg.LoginOptInsecure(true))
 	g.Expect(err).NotTo(HaveOccurred())
 
@@ -804,8 +804,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)
+	ref := fmt.Sprintf("%s/testrepo/%s:%s", testRegistryServer.registryHost, metadata.Name, metadata.Version)
-	_, err = testRegistryserver.RegistryClient.Push(chartData, ref)
+	_, err = testRegistryServer.registryClient.Push(chartData, ref)
 	g.Expect(err).NotTo(HaveOccurred())
 
 	storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
@@ -835,8 +835,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 				Type: corev1.SecretTypeDockerConfigJson,
 				Data: map[string][]byte{
 					".dockerconfigjson": []byte(`{"auths":{"` +
-						testRegistryserver.DockerRegistryHost + `":{"` +
+						testRegistryServer.registryHost + `":{"` +
-						`auth":"` + base64.StdEncoding.EncodeToString([]byte(testUsername+":"+testPassword)) + `"}}}`),
+						`auth":"` + base64.StdEncoding.EncodeToString([]byte(testRegistryUsername+":"+testRegistryPassword)) + `"}}}`),
 				},
 			},
 			beforeFunc: func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository) {
@@ -862,8 +862,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 					Name: "auth",
 				},
 				Data: map[string][]byte{
-					"username": []byte(testUsername),
+					"username": []byte(testRegistryUsername),
-					"password": []byte(testPassword),
+					"password": []byte(testRegistryPassword),
 				},
 			},
 			beforeFunc: func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository) {
@@ -983,7 +983,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.registryHost),
 					Timeout: &metav1.Duration{Duration: timeout},
 					Type:    sourcev1.HelmRepositoryTypeOCI,
 				},

controllers/helmrepository_controller_oci.go

@@ -21,7 +21,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"strings"
 	"time"
 
 	"github.com/fluxcd/pkg/apis/meta"
@@ -257,6 +256,15 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *source
 }
 
 func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmRepository) (sreconcile.Result, error) {
+	if !helmreg.IsOCI(obj.Spec.URL) {
+		e := &serror.Stalling{
+			Err:    fmt.Errorf("the url scheme is not supported: %s", obj.Spec.URL),
+			Reason: sourcev1.URLInvalidReason,
+		}
+		conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
+		return sreconcile.ResultEmpty, e
+	}
+
 	var loginOpts []helmreg.LoginOption
 	// Configure any authentication related options
 	if obj.Spec.SecretRef != nil {
@@ -292,11 +300,7 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *
 		}
 	}
 
-	if result, err := r.validateSource(ctx, obj, loginOpts...); err != nil || result == sreconcile.ResultEmpty {
+	return r.validateSource(ctx, obj, loginOpts...)
-		return result, err
-	}
-
-	return sreconcile.ResultSuccess, nil
 }
 
 // validateSource the HelmRepository object by checking the url and connecting to the underlying registry
@@ -304,8 +308,8 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *
 func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *sourcev1.HelmRepository, logOpts ...helmreg.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),
+			Err:    fmt.Errorf("failed to create registry client:: %w", err),
 			Reason: meta.FailedReason,
 		}
 		conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
@@ -323,21 +327,12 @@ func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *s
 
 	chartRepo, err := repository.NewOCIChartRepository(obj.Spec.URL, repository.WithOCIRegistryClient(registryClient))
 	if err != nil {
-		if strings.Contains(err.Error(), "parse") {
+		e := &serror.Stalling{
-			e := &serror.Stalling{
+			Err:    fmt.Errorf("failed to parse URL '%s': %w", obj.Spec.URL, err),
-				Err:    fmt.Errorf("failed to parse URL '%s': %w", obj.Spec.URL, err),
+			Reason: sourcev1.URLInvalidReason,
-				Reason: sourcev1.URLInvalidReason,
-			}
-			conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
-			return sreconcile.ResultEmpty, e
-		} else if strings.Contains(err.Error(), "the url scheme is not supported") {
-			e := &serror.Event{
-				Err:    err,
-				Reason: sourcev1.URLInvalidReason,
-			}
-			conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
-			return sreconcile.ResultEmpty, e
 		}
+		conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
+		return sreconcile.ResultEmpty, e
 	}
 
 	// Attempt to login to the registry if credentials are provided.

controllers/helmrepository_controller_oci_test.go

@@ -43,8 +43,8 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
 		{
 			name: "valid auth data",
 			secretData: map[string][]byte{
-				"username": []byte(testUsername),
+				"username": []byte(testRegistryUsername),
-				"password": []byte(testPassword),
+				"password": []byte(testRegistryPassword),
 			},
 		},
 		{
@@ -56,8 +56,8 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
 			secretType: corev1.SecretTypeDockerConfigJson,
 			secretData: map[string][]byte{
 				".dockerconfigjson": []byte(`{"auths":{"` +
-					testRegistryserver.DockerRegistryHost + `":{"` +
+					testRegistryServer.registryHost + `":{"` +
-					`auth":"` + base64.StdEncoding.EncodeToString([]byte(testUsername+":"+testPassword)) + `"}}}`),
+					`auth":"` + base64.StdEncoding.EncodeToString([]byte(testRegistryUsername+":"+testRegistryPassword)) + `"}}}`),
 			},
 		},
 	}
@@ -90,7 +90,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.registryHost),
 					SecretRef: &meta.LocalObjectReference{
 						Name: secret.Name,
 					},

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),
+			"username": []byte(testRegistryUsername),
-			"password": []byte(testPassword),
+			"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.registryHost)
 	obj.Spec.SecretRef = &meta.LocalObjectReference{
 		Name: secret.Name,
 	}
@@ -1223,7 +1223,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}
 
@@ -1263,20 +1263,22 @@ func TestHelmRepositoryReconciler_ReconcileSpecUpdatePredicateFilter(t *testing.
 
 	// Change spec Interval to validate spec update
 	obj.Spec.Interval = metav1.Duration{Duration: interval + time.Second}
+	oldGen := obj.GetGeneration()
 	g.Expect(testEnv.Update(ctx, obj)).To(Succeed())
+	newGen := oldGen + 1
 
 	// Wait for HelmRepository to be Ready
 	g.Eventually(func() bool {
 		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)
 		return readyCondition.Status == metav1.ConditionTrue &&
-			obj.Generation == readyCondition.ObservedGeneration &&
+			newGen == readyCondition.ObservedGeneration &&
-			obj.Generation == obj.Status.ObservedGeneration
+			newGen == obj.Status.ObservedGeneration
 	}, timeout).Should(BeTrue())
 
 	// Check if the object status is valid.

controllers/suite_test.go

@@ -68,6 +68,12 @@ const (
 	retentionRecords = 2
 )
 
+const (
+	testRegistryHtpasswdFileBasename = "authtest.htpasswd"
+	testRegistryUsername             = "myuser"
+	testRegistryPassword             = "mypass"
+)
+
 var (
 	testEnv      *testenv.Environment
 	testStorage  *Storage
@@ -96,69 +102,62 @@ var (
 )
 
 var (
-	testRegistryClient *helmreg.Client
+	testRegistryServer *registryClientTestServer
-	testRegistryserver *RegistryClientTestServer
-)
-
-var (
-	testWorkspaceDir         = "registry-test"
-	testHtpasswdFileBasename = "authtest.htpasswd"
-	testUsername             = "myuser"
-	testPassword             = "mypass"
 )
 
 func init() {
 	rand.Seed(time.Now().UnixNano())
 }
 
-type RegistryClientTestServer struct {
+type registryClientTestServer struct {
-	Out                io.Writer
+	out            io.Writer
-	DockerRegistryHost string
+	registryHost   string
-	WorkspaceDir       string
+	workspaceDir   string
-	RegistryClient     *helmreg.Client
+	registryClient *helmreg.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
+	workspaceDir, err := os.MkdirTemp("", "registry-test-")
-	os.RemoveAll(server.WorkspaceDir)
-	err := os.Mkdir(server.WorkspaceDir, 0700)
 	if err != nil {
-		panic(fmt.Sprintf("failed to create workspace directory: %s", err))
+		return nil, fmt.Errorf("failed to create workspace directory: %w", err)
 	}
+	server.workspaceDir = workspaceDir
 
 	var out bytes.Buffer
-	server.Out = &out
+	server.out = &out
 
 	// init test client
-	server.RegistryClient, err = helmreg.NewClient(
+	server.registryClient, err = helmreg.NewClient(
 		helmreg.ClientOptDebug(true),
-		helmreg.ClientOptWriter(server.Out),
+		helmreg.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)
+	htpasswdPath := filepath.Join(workspaceDir, testRegistryHtpasswdFileBasename)
-	err = ioutil.WriteFile(htpasswdPath, []byte(fmt.Sprintf("%s:%s\n", testUsername, string(pwBytes))), 0644)
+	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))
+		return nil, fmt.Errorf("failed to get free port: %s", err)
 	}
 
-	server.DockerRegistryHost = fmt.Sprintf("localhost:%d", port)
+	server.registryHost = 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{}{}}
@@ -168,15 +167,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: %w", err)
 	}
 
 	// Start Docker registry
 	go dockerRegistry.ListenAndServe()
 
-	return server.WorkspaceDir
+	return server, nil
 }
 
 func TestMain(m *testing.M) {
@@ -201,12 +200,9 @@ func TestMain(m *testing.M) {
 
 	testMetricsH = controller.MustMakeMetrics(testEnv)
 
-	testRegistryserver = &RegistryClientTestServer{}
+	testRegistryServer, err = setupRegistryServer(ctx)
-	registryWorkspaceDir := SetupServer(testRegistryserver)
-
-	testRegistryClient, err = helmreg.NewClient(helmreg.ClientOptWriter(os.Stdout))
 	if err != nil {
-		panic(fmt.Sprintf("Failed to create OCI registry client"))
+		panic(fmt.Sprintf("Failed to create a test registry server: %v", err))
 	}
 
 	managed.InitManagedTransport(logr.Discard())
@@ -286,7 +282,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))
 	}