From 0e0d7df37c26338ef669f3565d980fcf37cd3e3b Mon Sep 17 00:00:00 2001 From: Soule BA Date: Tue, 24 May 2022 09:51:56 +0200 Subject: [PATCH] Replace stalling events in HelmChart and HelmRepository_OCI The setupRegistryServer has been refactored to take into account #690 reviews. Signed-off-by: Soule BA --- controllers/helmchart_controller.go | 22 ++++-- controllers/helmchart_controller_test.go | 18 ++--- controllers/helmrepository_controller_oci.go | 10 +-- .../helmrepository_controller_oci_test.go | 10 +-- controllers/helmrepository_controller_test.go | 12 +-- controllers/suite_test.go | 74 +++++++++---------- 6 files changed, 74 insertions(+), 72 deletions(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 1198adb3c..21c96102e 100644 --- a/controllers/helmchart_controller.go +++ b/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{ diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index 59ff1d0b1..d0fc9bc0b 100644 --- a/controllers/helmchart_controller_test.go +++ b/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, - helmreg.LoginOptBasicAuth(testUsername, testPassword), + err := testRegistryServer.registryClient.Login(testRegistryServer.dockerRegistryHost, + 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) - _, 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) @@ -835,8 +835,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { Type: corev1.SecretTypeDockerConfigJson, Data: map[string][]byte{ ".dockerconfigjson": []byte(`{"auths":{"` + - testRegistryserver.DockerRegistryHost + `":{"` + - `auth":"` + base64.StdEncoding.EncodeToString([]byte(testUsername+":"+testPassword)) + `"}}}`), + testRegistryServer.dockerRegistryHost + `":{"` + + `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), - "password": []byte(testPassword), + "username": []byte(testRegistryUsername), + "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.dockerRegistryHost), Timeout: &metav1.Duration{Duration: timeout}, Type: sourcev1.HelmRepositoryTypeOCI, }, diff --git a/controllers/helmrepository_controller_oci.go b/controllers/helmrepository_controller_oci.go index ec4330afb..787dbd614 100644 --- a/controllers/helmrepository_controller_oci.go +++ b/controllers/helmrepository_controller_oci.go @@ -292,11 +292,7 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj * } } - if result, err := r.validateSource(ctx, obj, loginOpts...); err != nil || result == sreconcile.ResultEmpty { - return result, err - } - - return sreconcile.ResultSuccess, nil + return r.validateSource(ctx, obj, loginOpts...) } // validateSource the HelmRepository object by checking the url and connecting to the underlying registry @@ -304,8 +300,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{ - Err: fmt.Errorf("failed to create registry client: %w", err), + e := &serror.Event{ + Err: fmt.Errorf("failed to create registry client:: %w", err), Reason: meta.FailedReason, } conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error()) diff --git a/controllers/helmrepository_controller_oci_test.go b/controllers/helmrepository_controller_oci_test.go index 21a221ef2..404cb41dc 100644 --- a/controllers/helmrepository_controller_oci_test.go +++ b/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), - "password": []byte(testPassword), + "username": []byte(testRegistryUsername), + "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 + `":{"` + - `auth":"` + base64.StdEncoding.EncodeToString([]byte(testUsername+":"+testPassword)) + `"}}}`), + testRegistryServer.dockerRegistryHost + `":{"` + + `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.dockerRegistryHost), SecretRef: &meta.LocalObjectReference{ Name: secret.Name, }, diff --git a/controllers/helmrepository_controller_test.go b/controllers/helmrepository_controller_test.go index 2230a72e3..9955cbc15 100644 --- 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 index 6531d633f..187892ae1 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -66,6 +66,12 @@ const ( retentionRecords = 2 ) +const ( + testRegistryHtpasswdFileBasename = "authtest.htpasswd" + testRegistryUsername = "myuser" + testRegistryPassword = "mypass" +) + var ( testEnv *testenv.Environment testStorage *Storage @@ -94,69 +100,62 @@ var ( ) var ( - testRegistryClient *helmreg.Client - testRegistryserver *RegistryClientTestServer -) - -var ( - testWorkspaceDir = "registry-test" - testHtpasswdFileBasename = "authtest.htpasswd" - testUsername = "myuser" - testPassword = "mypass" + testRegistryServer *registryClientTestServer ) func init() { rand.Seed(time.Now().UnixNano()) } -type RegistryClientTestServer struct { - Out io.Writer - DockerRegistryHost string - WorkspaceDir string - RegistryClient *helmreg.Client +type registryClientTestServer struct { + out io.Writer + dockerRegistryHost string + workspaceDir string + registryClient *helmreg.Client } -func SetupServer(server *RegistryClientTestServer) string { +func setupRegistryServer(ctx context.Context) (*registryClientTestServer, error) { + server := ®istryClientTestServer{} + // Create a temporary workspace directory for the registry - server.WorkspaceDir = testWorkspaceDir - os.RemoveAll(server.WorkspaceDir) - err := os.Mkdir(server.WorkspaceDir, 0700) + workspaceDir, err := os.MkdirTemp("", "registry-test-") 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) - err = ioutil.WriteFile(htpasswdPath, []byte(fmt.Sprintf("%s:%s\n", testUsername, string(pwBytes))), 0644) + htpasswdPath := filepath.Join(workspaceDir, 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)) + return nil, fmt.Errorf("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{}{}} @@ -166,15 +165,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) { @@ -199,12 +198,9 @@ func TestMain(m *testing.M) { testMetricsH = controller.MustMakeMetrics(testEnv) - testRegistryserver = &RegistryClientTestServer{} - registryWorkspaceDir := SetupServer(testRegistryserver) - - testRegistryClient, err = helmreg.NewClient(helmreg.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{ @@ -282,7 +278,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)) }