From 1070d1287aa053d5a6354358e9908ad98c54b3f1 Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Fri, 20 May 2022 21:14:34 +0200 Subject: [PATCH 01/10] fix nil pointer dereference When the Secret referenced in an OCI HelmRepository doesn't contain a username and password, the controller doesn't panic, anymore. Signed-off-by: Max Jonas Werner --- controllers/helmrepository_controller_oci.go | 4 +- .../helmrepository_controller_oci_test.go | 187 ++++++++++-------- 2 files changed, 105 insertions(+), 86 deletions(-) diff --git a/controllers/helmrepository_controller_oci.go b/controllers/helmrepository_controller_oci.go index 05da9af0..676cee43 100644 --- a/controllers/helmrepository_controller_oci.go +++ b/controllers/helmrepository_controller_oci.go @@ -284,7 +284,9 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj * return sreconcile.ResultEmpty, e } - logOpts = append(logOpts, logOpt) + if logOpt != nil { + logOpts = append(logOpts, logOpt) + } } if result, err := r.validateSource(ctx, obj, logOpts...); err != nil || result == sreconcile.ResultEmpty { diff --git a/controllers/helmrepository_controller_oci_test.go b/controllers/helmrepository_controller_oci_test.go index 6069fe8c..068dce55 100644 --- a/controllers/helmrepository_controller_oci_test.go +++ b/controllers/helmrepository_controller_oci_test.go @@ -34,99 +34,116 @@ import ( ) func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) { - g := NewWithT(t) - - ns, err := testEnv.CreateNamespace(ctx, "helmrepository-oci-reconcile-test") - g.Expect(err).ToNot(HaveOccurred()) - defer func() { g.Expect(testEnv.Delete(ctx, ns)).To(Succeed()) }() - - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "helmrepository-", - Namespace: ns.Name, - }, - Data: map[string][]byte{ - "username": []byte(testUsername), - "password": []byte(testPassword), - }, - } - - g.Expect(testEnv.CreateAndWait(ctx, secret)).To(Succeed()) - - obj := &sourcev1.HelmRepository{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "helmrepository-oci-reconcile-", - Namespace: ns.Name, - }, - Spec: sourcev1.HelmRepositorySpec{ - Interval: metav1.Duration{Duration: interval}, - URL: fmt.Sprintf("oci://%s", testRegistryserver.DockerRegistryHost), - SecretRef: &meta.LocalObjectReference{ - Name: secret.Name, + tests := []struct { + name string + secretData map[string][]byte + }{ + { + name: "valid auth data", + secretData: map[string][]byte{ + "username": []byte(testUsername), + "password": []byte(testPassword), }, - Type: sourcev1.HelmRepositoryTypeOCI, + }, + { + name: "no auth data", + secretData: nil, }, } - g.Expect(testEnv.Create(ctx, obj)).To(Succeed()) - key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) - // Wait for finalizer to be set - g.Eventually(func() bool { - if err := testEnv.Get(ctx, key, obj); err != nil { - return false - } - return len(obj.Finalizers) > 0 - }, timeout).Should(BeTrue()) + ns, err := testEnv.CreateNamespace(ctx, "helmrepository-oci-reconcile-test") + g.Expect(err).ToNot(HaveOccurred()) + defer func() { g.Expect(testEnv.Delete(ctx, ns)).To(Succeed()) }() - // 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) { - return false - } - readyCondition := conditions.Get(obj, meta.ReadyCondition) - return obj.Generation == readyCondition.ObservedGeneration && - obj.Generation == obj.Status.ObservedGeneration - }, timeout).Should(BeTrue()) + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "helmrepository-", + Namespace: ns.Name, + }, + Data: tt.secretData, + } - // Check if the object status is valid. - condns := &status.Conditions{NegativePolarity: helmRepositoryReadyCondition.NegativePolarity} - checker := status.NewChecker(testEnv.Client, condns) - checker.CheckErr(ctx, obj) + g.Expect(testEnv.CreateAndWait(ctx, secret)).To(Succeed()) - // kstatus client conformance check. - u, err := patch.ToUnstructured(obj) - g.Expect(err).ToNot(HaveOccurred()) - res, err := kstatus.Compute(u) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(res.Status).To(Equal(kstatus.CurrentStatus)) + obj := &sourcev1.HelmRepository{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "helmrepository-oci-reconcile-", + Namespace: ns.Name, + }, + Spec: sourcev1.HelmRepositorySpec{ + Interval: metav1.Duration{Duration: interval}, + URL: fmt.Sprintf("oci://%s", testRegistryserver.DockerRegistryHost), + SecretRef: &meta.LocalObjectReference{ + Name: secret.Name, + }, + Type: sourcev1.HelmRepositoryTypeOCI, + }, + } + g.Expect(testEnv.Create(ctx, obj)).To(Succeed()) - // Patch the object with reconcile request annotation. - patchHelper, err := patch.NewHelper(obj, testEnv.Client) - g.Expect(err).ToNot(HaveOccurred()) - annotations := map[string]string{ - meta.ReconcileRequestAnnotation: "now", + key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace} + + // Wait for finalizer to be set + g.Eventually(func() bool { + if err := testEnv.Get(ctx, key, obj); err != nil { + return false + } + return len(obj.Finalizers) > 0 + }, timeout).Should(BeTrue()) + + // 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) { + return false + } + readyCondition := conditions.Get(obj, meta.ReadyCondition) + return obj.Generation == readyCondition.ObservedGeneration && + obj.Generation == obj.Status.ObservedGeneration + }, timeout).Should(BeTrue()) + + // Check if the object status is valid. + condns := &status.Conditions{NegativePolarity: helmRepositoryReadyCondition.NegativePolarity} + checker := status.NewChecker(testEnv.Client, condns) + checker.CheckErr(ctx, obj) + + // kstatus client conformance check. + u, err := patch.ToUnstructured(obj) + g.Expect(err).ToNot(HaveOccurred()) + res, err := kstatus.Compute(u) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(res.Status).To(Equal(kstatus.CurrentStatus)) + + // Patch the object with reconcile request annotation. + patchHelper, err := patch.NewHelper(obj, testEnv.Client) + g.Expect(err).ToNot(HaveOccurred()) + annotations := map[string]string{ + meta.ReconcileRequestAnnotation: "now", + } + obj.SetAnnotations(annotations) + g.Expect(patchHelper.Patch(ctx, obj)).ToNot(HaveOccurred()) + g.Eventually(func() bool { + if err := testEnv.Get(ctx, key, obj); err != nil { + return false + } + return obj.Status.LastHandledReconcileAt == "now" + }, timeout).Should(BeTrue()) + + g.Expect(testEnv.Delete(ctx, obj)).To(Succeed()) + + // Wait for HelmRepository to be deleted + g.Eventually(func() bool { + if err := testEnv.Get(ctx, key, obj); err != nil { + return apierrors.IsNotFound(err) + } + return false + }, timeout).Should(BeTrue()) + }) } - obj.SetAnnotations(annotations) - g.Expect(patchHelper.Patch(ctx, obj)).ToNot(HaveOccurred()) - g.Eventually(func() bool { - if err := testEnv.Get(ctx, key, obj); err != nil { - return false - } - return obj.Status.LastHandledReconcileAt == "now" - }, timeout).Should(BeTrue()) - - g.Expect(testEnv.Delete(ctx, obj)).To(Succeed()) - - // Wait for HelmRepository to be deleted - g.Eventually(func() bool { - if err := testEnv.Get(ctx, key, obj); err != nil { - return apierrors.IsNotFound(err) - } - return false - }, timeout).Should(BeTrue()) - } From bb4d886ba26ec9bba77fed0e33ddd1e8776ca04e Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Fri, 20 May 2022 20:59:08 +0200 Subject: [PATCH 02/10] dockerconfigjson for OCI registry authentication `loginOptionFromSecret` now derives username/password from a docker config stored in Secrets of type "kubernetes.io/dockerconfigjson". Signed-off-by: Max Jonas Werner --- controllers/helmchart_controller.go | 2 +- controllers/helmchart_controller_test.go | 30 ++++++++++++++++++ controllers/helmrepository_controller_oci.go | 31 +++++++++++++++++-- .../helmrepository_controller_oci_test.go | 14 +++++++++ 4 files changed, 73 insertions(+), 4 deletions(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 8afb96c7..1a1092bb 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -492,7 +492,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } // Build registryClient options from secret - logOpt, err := loginOptionFromSecret(*secret) + logOpt, err := loginOptionFromSecret(repo.Spec.URL, *secret) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index 9bc5a39e..9796ea6e 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -19,6 +19,7 @@ package controllers import ( "bytes" "context" + "encoding/base64" "errors" "fmt" "io" @@ -825,6 +826,35 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { assertFunc func(g *WithT, obj *sourcev1.HelmChart, build chart.Build) cleanFunc func(g *WithT, build *chart.Build) }{ + { + name: "Reconciles chart build with docker repository credentials", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth", + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{ + ".dockerconfigjson": []byte(`{"auths":{"` + + testRegistryserver.DockerRegistryHost + `":{"` + + `auth":"` + base64.StdEncoding.EncodeToString([]byte(testUsername+":"+testPassword)) + `"}}}`), + }, + }, + beforeFunc: func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository) { + obj.Spec.Chart = metadata.Name + obj.Spec.Version = metadata.Version + repository.Spec.SecretRef = &meta.LocalObjectReference{Name: "auth"} + }, + want: sreconcile.ResultSuccess, + assertFunc: func(g *WithT, _ *sourcev1.HelmChart, build chart.Build) { + g.Expect(build.Name).To(Equal(metadata.Name)) + g.Expect(build.Version).To(Equal(metadata.Version)) + g.Expect(build.Path).ToNot(BeEmpty()) + g.Expect(build.Path).To(BeARegularFile()) + }, + cleanFunc: func(g *WithT, build *chart.Build) { + g.Expect(os.Remove(build.Path)).To(Succeed()) + }, + }, { name: "Reconciles chart build with repository credentials", secret: &corev1.Secret{ diff --git a/controllers/helmrepository_controller_oci.go b/controllers/helmrepository_controller_oci.go index 676cee43..6cd51613 100644 --- a/controllers/helmrepository_controller_oci.go +++ b/controllers/helmrepository_controller_oci.go @@ -17,12 +17,15 @@ limitations under the License. package controllers import ( + "bytes" "context" "fmt" + "net/url" "os" "strings" "time" + "github.com/docker/cli/cli/config" "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/runtime/conditions" helper "github.com/fluxcd/pkg/runtime/controller" @@ -273,7 +276,7 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj * } // Construct actual options - logOpt, err := loginOptionFromSecret(secret) + logOpt, err := loginOptionFromSecret(obj.Spec.URL, secret) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), @@ -352,8 +355,30 @@ func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *s return sreconcile.ResultSuccess, nil } -func loginOptionFromSecret(secret corev1.Secret) (registry.LoginOption, error) { - username, password := string(secret.Data["username"]), string(secret.Data["password"]) +// loginOptionFromSecret derives authentication data from a Secret to login to an OCI registry. This Secret +// may either hold "username" and "password" fields or be of the corev1.SecretTypeDockerConfigJson type and hold +// a corev1.DockerConfigJsonKey field with a complete Docker configuration. If both, "username" and "password" are +// empty, a nil LoginOption and a nil error will be returned. +func loginOptionFromSecret(registryURL string, secret corev1.Secret) (registry.LoginOption, error) { + var username, password string + if secret.Type == corev1.SecretTypeDockerConfigJson { + dockerCfg, err := config.LoadFromReader(bytes.NewReader(secret.Data[corev1.DockerConfigJsonKey])) + if err != nil { + return nil, fmt.Errorf("unable to load Docker config: %w", err) + } + parsedURL, err := url.Parse(registryURL) + if err != nil { + return nil, fmt.Errorf("unable to parse registry URL: %w", err) + } + authConfig, err := dockerCfg.GetAuthConfig(parsedURL.Host) + if err != nil { + return nil, fmt.Errorf("unable to get authentication data from Secret: %w", err) + } + username = authConfig.Username + password = authConfig.Password + } else { + username, password = string(secret.Data["username"]), string(secret.Data["password"]) + } switch { case username == "" && password == "": return nil, nil diff --git a/controllers/helmrepository_controller_oci_test.go b/controllers/helmrepository_controller_oci_test.go index 068dce55..21a221ef 100644 --- a/controllers/helmrepository_controller_oci_test.go +++ b/controllers/helmrepository_controller_oci_test.go @@ -17,6 +17,7 @@ limitations under the License. package controllers import ( + "encoding/base64" "fmt" "testing" @@ -36,6 +37,7 @@ import ( func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) { tests := []struct { name string + secretType corev1.SecretType secretData map[string][]byte }{ { @@ -49,6 +51,15 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) { name: "no auth data", secretData: nil, }, + { + name: "dockerconfigjson Secret", + secretType: corev1.SecretTypeDockerConfigJson, + secretData: map[string][]byte{ + ".dockerconfigjson": []byte(`{"auths":{"` + + testRegistryserver.DockerRegistryHost + `":{"` + + `auth":"` + base64.StdEncoding.EncodeToString([]byte(testUsername+":"+testPassword)) + `"}}}`), + }, + }, } for _, tt := range tests { @@ -66,6 +77,9 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) { }, Data: tt.secretData, } + if tt.secretType != "" { + secret.Type = tt.secretType + } g.Expect(testEnv.CreateAndWait(ctx, secret)).To(Succeed()) From ce072c7eda080ba0616c448c866c94e43cc187f1 Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Sun, 22 May 2022 20:10:03 +0200 Subject: [PATCH 03/10] better variable names; improved logging When setup of one of the two controller reconciling HelmRepositories fails, it's now possible to judge from the log which setup call failed by regarding the "type" log field. Signed-off-by: Max Jonas Werner --- controllers/helmchart_controller.go | 13 ++++++------- controllers/helmrepository_controller_oci.go | 10 +++++----- main.go | 4 ++-- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 1a1092bb..2fb9e70a 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -447,7 +447,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * repo *sourcev1.HelmRepository, b *chart.Build) (sreconcile.Result, error) { var ( tlsConfig *tls.Config - logOpts []registry.LoginOption + loginOpts []registry.LoginOption ) // Construct the Getter options from the HelmRepository data @@ -492,7 +492,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } // Build registryClient options from secret - logOpt, err := loginOptionFromSecret(repo.Spec.URL, *secret) + loginOpt, err := loginOptionFromSecret(repo.Spec.URL, *secret) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), @@ -503,7 +503,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * return sreconcile.ResultEmpty, e } - logOpts = append([]registry.LoginOption{}, logOpt) + loginOpts = append([]registry.LoginOption{}, loginOpt) } // Initialize the chart repository @@ -519,7 +519,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // this is needed because otherwise the credentials are stored in ~/.docker/config.json. // TODO@souleb: remove this once the registry move to Oras v2 // or rework to enable reusing credentials to avoid the unneccessary handshake operations - registryClient, file, err := r.RegistryClientGenerator(logOpts != nil) + registryClient, file, err := r.RegistryClientGenerator(loginOpts != nil) if err != nil { return chartRepoErrorReturn(err, obj) } @@ -540,14 +540,13 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // If login options are configured, use them to login to the registry // The OCIGetter will later retrieve the stored credentials to pull the chart - if logOpts != nil { - err = ociChartRepo.Login(logOpts...) + if loginOpts != nil { + err = ociChartRepo.Login(loginOpts...) if err != nil { return chartRepoErrorReturn(err, obj) } } default: - var httpChartRepo *repository.ChartRepository httpChartRepo, err := repository.NewChartRepository(repo.Spec.URL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts, repository.WithMemoryCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) { r.IncCacheEvents(event, obj.Name, obj.Namespace) diff --git a/controllers/helmrepository_controller_oci.go b/controllers/helmrepository_controller_oci.go index 6cd51613..7702e446 100644 --- a/controllers/helmrepository_controller_oci.go +++ b/controllers/helmrepository_controller_oci.go @@ -257,7 +257,7 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *source } func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmRepository) (sreconcile.Result, error) { - var logOpts []registry.LoginOption + var loginOpts []registry.LoginOption // Configure any authentication related options if obj.Spec.SecretRef != nil { // Attempt to retrieve secret @@ -276,7 +276,7 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj * } // Construct actual options - logOpt, err := loginOptionFromSecret(obj.Spec.URL, secret) + loginOpt, err := loginOptionFromSecret(obj.Spec.URL, secret) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), @@ -287,12 +287,12 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj * return sreconcile.ResultEmpty, e } - if logOpt != nil { - logOpts = append(logOpts, logOpt) + if loginOpt != nil { + loginOpts = append(loginOpts, loginOpt) } } - if result, err := r.validateSource(ctx, obj, logOpts...); err != nil || result == sreconcile.ResultEmpty { + if result, err := r.validateSource(ctx, obj, loginOpts...); err != nil || result == sreconcile.ResultEmpty { return result, err } diff --git a/main.go b/main.go index 88f4ad2d..5088d599 100644 --- a/main.go +++ b/main.go @@ -229,7 +229,7 @@ func main() { MaxConcurrentReconciles: concurrent, RateLimiter: helper.GetRateLimiter(rateLimiterOptions), }); err != nil { - setupLog.Error(err, "unable to create controller", "controller", sourcev1.HelmRepositoryKind) + setupLog.Error(err, "unable to create controller", "controller", sourcev1.HelmRepositoryKind, "type", "default") os.Exit(1) } @@ -244,7 +244,7 @@ func main() { MaxConcurrentReconciles: concurrent, RateLimiter: helper.GetRateLimiter(rateLimiterOptions), }); err != nil { - setupLog.Error(err, "unable to create controller", "controller", sourcev1.HelmRepositoryKind) + setupLog.Error(err, "unable to create controller", "controller", sourcev1.HelmRepositoryKind, "type", "OCI") os.Exit(1) } From d5e3c37833d40e650ecfc5aeea2765edac821697 Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Mon, 23 May 2022 08:50:27 +0200 Subject: [PATCH 04/10] fix code formatting Signed-off-by: Max Jonas Werner --- controllers/helmchart_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 2fb9e70a..80bb773f 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -447,7 +447,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * repo *sourcev1.HelmRepository, b *chart.Build) (sreconcile.Result, error) { var ( tlsConfig *tls.Config - loginOpts []registry.LoginOption + loginOpts []registry.LoginOption ) // Construct the Getter options from the HelmRepository data From ace21c56660cb44e51343f486f419a5723894ee0 Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Mon, 23 May 2022 11:11:27 +0200 Subject: [PATCH 05/10] make tidy Signed-off-by: Max Jonas Werner --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 0cc5df23..a3529282 100644 --- a/go.mod +++ b/go.mod @@ -18,6 +18,7 @@ require ( github.com/cyphar/filepath-securejoin v0.2.3 github.com/darkowlzz/controller-check v0.0.0-20220325122359-11f5827b7981 github.com/distribution/distribution/v3 v3.0.0-20211118083504-a29a3c99a684 + github.com/docker/cli v20.10.11+incompatible github.com/docker/go-units v0.4.0 github.com/elazarl/goproxy v0.0.0-20220417044921-416226498f94 github.com/fluxcd/gitkit v0.5.0 @@ -101,7 +102,6 @@ require ( github.com/chai2010/gettext-go v0.0.0-20160711120539-c6fed771bfd5 // indirect github.com/containerd/containerd v1.6.1 // indirect github.com/davecgh/go-spew v1.1.1 // indirect - github.com/docker/cli v20.10.11+incompatible // indirect github.com/docker/distribution v2.8.0+incompatible // indirect github.com/docker/docker v20.10.12+incompatible // indirect github.com/docker/docker-credential-helpers v0.6.4 // indirect From c795da2280468f8a4324c58e624bfeab177e9006 Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Mon, 23 May 2022 11:11:53 +0200 Subject: [PATCH 06/10] introduce `internal/helm/registry` package This new package holds all Helm OCI registry-specific code now so we have a single location to look for such code which makes it easier to find yourself around. Signed-off-by: Max Jonas Werner --- controllers/helmchart_controller.go | 15 ++++--- controllers/helmchart_controller_test.go | 10 ++--- controllers/helmrepository_controller_oci.go | 47 +++----------------- controllers/suite_test.go | 18 ++++---- internal/helm/registry/auth.go | 44 ++++++++++++++++++ internal/helm/{util => registry}/client.go | 4 +- main.go | 6 +-- 7 files changed, 77 insertions(+), 67 deletions(-) create mode 100644 internal/helm/registry/auth.go rename internal/helm/{util => registry}/client.go (94%) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 80bb773f..07efa41e 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -29,7 +29,7 @@ import ( "time" helmgetter "helm.sh/helm/v3/pkg/getter" - "helm.sh/helm/v3/pkg/registry" + helmreg "helm.sh/helm/v3/pkg/registry" corev1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -64,6 +64,7 @@ import ( sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" "github.com/fluxcd/source-controller/internal/util" + "github.com/fluxcd/source-controller/internal/helm/registry" ) // helmChartReadyCondition contains all the conditions information @@ -380,7 +381,7 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1 // Assert source has an artifact if s.GetArtifact() == nil || !r.Storage.ArtifactExist(*s.GetArtifact()) { - if helmRepo, ok := s.(*sourcev1.HelmRepository); !ok || !registry.IsOCI(helmRepo.Spec.URL) { + if helmRepo, ok := s.(*sourcev1.HelmRepository); !ok || !helmreg.IsOCI(helmRepo.Spec.URL) { conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "NoSourceArtifact", "no artifact available for %s source '%s'", obj.Spec.SourceRef.Kind, obj.Spec.SourceRef.Name) r.eventLogf(ctx, obj, events.EventTypeTrace, "NoSourceArtifact", @@ -447,7 +448,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * repo *sourcev1.HelmRepository, b *chart.Build) (sreconcile.Result, error) { var ( tlsConfig *tls.Config - loginOpts []registry.LoginOption + loginOpts []helmreg.LoginOption ) // Construct the Getter options from the HelmRepository data @@ -492,7 +493,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } // Build registryClient options from secret - loginOpt, err := loginOptionFromSecret(repo.Spec.URL, *secret) + loginOpt, err := registry.LoginOptionFromSecret(repo.Spec.URL, *secret) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), @@ -503,14 +504,14 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * return sreconcile.ResultEmpty, e } - loginOpts = append([]registry.LoginOption{}, loginOpt) + loginOpts = append([]helmreg.LoginOption{}, loginOpt) } // Initialize the chart repository var chartRepo chart.Remote switch repo.Spec.Type { case sourcev1.HelmRepositoryTypeOCI: - if !registry.IsOCI(repo.Spec.URL) { + if !helmreg.IsOCI(repo.Spec.URL) { err := fmt.Errorf("invalid OCI registry URL: %s", repo.Spec.URL) return chartRepoErrorReturn(err, obj) } @@ -551,7 +552,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * repository.WithMemoryCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) { r.IncCacheEvents(event, obj.Name, obj.Namespace) })) - if err != nil { + if err != nil { return chartRepoErrorReturn(err, obj) } chartRepo = httpChartRepo diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index 9796ea6e..59ff1d0b 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -36,7 +36,7 @@ import ( . "github.com/onsi/gomega" hchart "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" - "helm.sh/helm/v3/pkg/registry" + helmreg "helm.sh/helm/v3/pkg/registry" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -54,7 +54,7 @@ import ( sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" serror "github.com/fluxcd/source-controller/internal/error" "github.com/fluxcd/source-controller/internal/helm/chart" - "github.com/fluxcd/source-controller/internal/helm/util" + "github.com/fluxcd/source-controller/internal/helm/registry" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" ) @@ -793,8 +793,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { // Login to the registry err := testRegistryserver.RegistryClient.Login(testRegistryserver.DockerRegistryHost, - registry.LoginOptBasicAuth(testUsername, testPassword), - registry.LoginOptInsecure(true)) + helmreg.LoginOptBasicAuth(testUsername, testPassword), + helmreg.LoginOptInsecure(true)) g.Expect(err).NotTo(HaveOccurred()) // Load a test chart @@ -975,7 +975,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Getters: testGetters, Storage: storage, - RegistryClientGenerator: util.RegistryClientGenerator, + RegistryClientGenerator: registry.ClientGenerator, } repository := &sourcev1.HelmRepository{ diff --git a/controllers/helmrepository_controller_oci.go b/controllers/helmrepository_controller_oci.go index 7702e446..ba2d356d 100644 --- a/controllers/helmrepository_controller_oci.go +++ b/controllers/helmrepository_controller_oci.go @@ -17,15 +17,12 @@ limitations under the License. package controllers import ( - "bytes" "context" "fmt" - "net/url" "os" "strings" "time" - "github.com/docker/cli/cli/config" "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/runtime/conditions" helper "github.com/fluxcd/pkg/runtime/controller" @@ -33,12 +30,13 @@ import ( "github.com/fluxcd/pkg/runtime/predicates" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" serror "github.com/fluxcd/source-controller/internal/error" + "github.com/fluxcd/source-controller/internal/helm/registry" "github.com/fluxcd/source-controller/internal/helm/repository" intpredicates "github.com/fluxcd/source-controller/internal/predicates" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" helmgetter "helm.sh/helm/v3/pkg/getter" - "helm.sh/helm/v3/pkg/registry" + helmreg "helm.sh/helm/v3/pkg/registry" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" kuberecorder "k8s.io/client-go/tools/record" @@ -94,7 +92,7 @@ type HelmRepositoryOCIReconciler struct { // and an optional file name. // The file is used to store the registry client credentials. // The caller is responsible for deleting the file. -type RegistryClientGeneratorFunc func(isLogin bool) (*registry.Client, string, error) +type RegistryClientGeneratorFunc func(isLogin bool) (*helmreg.Client, string, error) // helmRepositoryOCIReconcileFunc is the function type for all the // v1beta2.HelmRepository (sub)reconcile functions for OCI type. The type implementations @@ -257,7 +255,7 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *source } func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmRepository) (sreconcile.Result, error) { - var loginOpts []registry.LoginOption + var loginOpts []helmreg.LoginOption // Configure any authentication related options if obj.Spec.SecretRef != nil { // Attempt to retrieve secret @@ -276,7 +274,7 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj * } // Construct actual options - loginOpt, err := loginOptionFromSecret(obj.Spec.URL, secret) + loginOpt, err := registry.LoginOptionFromSecret(obj.Spec.URL, secret) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), @@ -301,7 +299,7 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj * // 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) { +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{ @@ -354,36 +352,3 @@ func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *s return sreconcile.ResultSuccess, nil } - -// loginOptionFromSecret derives authentication data from a Secret to login to an OCI registry. This Secret -// may either hold "username" and "password" fields or be of the corev1.SecretTypeDockerConfigJson type and hold -// a corev1.DockerConfigJsonKey field with a complete Docker configuration. If both, "username" and "password" are -// empty, a nil LoginOption and a nil error will be returned. -func loginOptionFromSecret(registryURL string, secret corev1.Secret) (registry.LoginOption, error) { - var username, password string - if secret.Type == corev1.SecretTypeDockerConfigJson { - dockerCfg, err := config.LoadFromReader(bytes.NewReader(secret.Data[corev1.DockerConfigJsonKey])) - if err != nil { - return nil, fmt.Errorf("unable to load Docker config: %w", err) - } - parsedURL, err := url.Parse(registryURL) - if err != nil { - return nil, fmt.Errorf("unable to parse registry URL: %w", err) - } - authConfig, err := dockerCfg.GetAuthConfig(parsedURL.Host) - if err != nil { - return nil, fmt.Errorf("unable to get authentication data from Secret: %w", err) - } - username = authConfig.Username - password = authConfig.Password - } else { - username, password = string(secret.Data["username"]), string(secret.Data["password"]) - } - switch { - case username == "" && password == "": - return nil, nil - case username == "" || password == "": - return nil, fmt.Errorf("invalid '%s' secret data: required fields 'username' and 'password'", secret.Name) - } - return registry.LoginOptBasicAuth(username, password), nil -} diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 7cef15e3..6531d633 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -30,7 +30,7 @@ import ( "golang.org/x/crypto/bcrypt" "helm.sh/helm/v3/pkg/getter" - "helm.sh/helm/v3/pkg/registry" + helmreg "helm.sh/helm/v3/pkg/registry" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" @@ -49,7 +49,7 @@ import ( sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/fluxcd/source-controller/internal/cache" "github.com/fluxcd/source-controller/internal/features" - "github.com/fluxcd/source-controller/internal/helm/util" + "github.com/fluxcd/source-controller/internal/helm/registry" // +kubebuilder:scaffold:imports ) @@ -94,7 +94,7 @@ var ( ) var ( - testRegistryClient *registry.Client + testRegistryClient *helmreg.Client testRegistryserver *RegistryClientTestServer ) @@ -113,7 +113,7 @@ type RegistryClientTestServer struct { Out io.Writer DockerRegistryHost string WorkspaceDir string - RegistryClient *registry.Client + RegistryClient *helmreg.Client } func SetupServer(server *RegistryClientTestServer) string { @@ -129,9 +129,9 @@ func SetupServer(server *RegistryClientTestServer) string { server.Out = &out // init test client - server.RegistryClient, err = registry.NewClient( - registry.ClientOptDebug(true), - registry.ClientOptWriter(server.Out), + server.RegistryClient, err = helmreg.NewClient( + helmreg.ClientOptDebug(true), + helmreg.ClientOptWriter(server.Out), ) if err != nil { panic(fmt.Sprintf("failed to create registry client: %s", err)) @@ -202,7 +202,7 @@ func TestMain(m *testing.M) { testRegistryserver = &RegistryClientTestServer{} registryWorkspaceDir := SetupServer(testRegistryserver) - testRegistryClient, err = registry.NewClient(registry.ClientOptWriter(os.Stdout)) + testRegistryClient, err = helmreg.NewClient(helmreg.ClientOptWriter(os.Stdout)) if err != nil { panic(fmt.Sprintf("Failed to create OCI registry client")) } @@ -241,7 +241,7 @@ func TestMain(m *testing.M) { EventRecorder: record.NewFakeRecorder(32), Metrics: testMetricsH, Getters: testGetters, - RegistryClientGenerator: util.RegistryClientGenerator, + RegistryClientGenerator: registry.ClientGenerator, }).SetupWithManager(testEnv); err != nil { panic(fmt.Sprintf("Failed to start HelmRepositoryOCIReconciler: %v", err)) } diff --git a/internal/helm/registry/auth.go b/internal/helm/registry/auth.go new file mode 100644 index 00000000..64922cdd --- /dev/null +++ b/internal/helm/registry/auth.go @@ -0,0 +1,44 @@ +package registry + +import ( + "bytes" + "fmt" + "net/url" + + "github.com/docker/cli/cli/config" + "helm.sh/helm/v3/pkg/registry" + corev1 "k8s.io/api/core/v1" +) + +// LoginOptionFromSecret derives authentication data from a Secret to login to an OCI registry. This Secret +// may either hold "username" and "password" fields or be of the corev1.SecretTypeDockerConfigJson type and hold +// a corev1.DockerConfigJsonKey field with a complete Docker configuration. If both, "username" and "password" are +// empty, a nil LoginOption and a nil error will be returned. +func LoginOptionFromSecret(registryURL string, secret corev1.Secret) (registry.LoginOption, error) { + var username, password string + if secret.Type == corev1.SecretTypeDockerConfigJson { + dockerCfg, err := config.LoadFromReader(bytes.NewReader(secret.Data[corev1.DockerConfigJsonKey])) + if err != nil { + return nil, fmt.Errorf("unable to load Docker config: %w", err) + } + parsedURL, err := url.Parse(registryURL) + if err != nil { + return nil, fmt.Errorf("unable to parse registry URL: %w", err) + } + authConfig, err := dockerCfg.GetAuthConfig(parsedURL.Host) + if err != nil { + return nil, fmt.Errorf("unable to get authentication data from Secret: %w", err) + } + username = authConfig.Username + password = authConfig.Password + } else { + username, password = string(secret.Data["username"]), string(secret.Data["password"]) + } + switch { + case username == "" && password == "": + return nil, nil + case username == "" || password == "": + return nil, fmt.Errorf("invalid '%s' secret data: required fields 'username' and 'password'", secret.Name) + } + return registry.LoginOptBasicAuth(username, password), nil +} diff --git a/internal/helm/util/client.go b/internal/helm/registry/client.go similarity index 94% rename from internal/helm/util/client.go rename to internal/helm/registry/client.go index 1bd8944f..0e835e8f 100644 --- a/internal/helm/util/client.go +++ b/internal/helm/registry/client.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package util +package registry import ( "io" @@ -26,7 +26,7 @@ import ( // RegistryClientGenerator generates a registry client and a temporary credential file. // The client is meant to be used for a single reconciliation. // The file is meant to be used for a single reconciliation and deleted after. -func RegistryClientGenerator(isLogin bool) (*registry.Client, string, error) { +func ClientGenerator(isLogin bool) (*registry.Client, string, error) { if isLogin { // create a temporary file to store the credentials // this is needed because otherwise the credentials are stored in ~/.docker/config.json. diff --git a/main.go b/main.go index 5088d599..a4b878a2 100644 --- a/main.go +++ b/main.go @@ -42,7 +42,7 @@ import ( "github.com/fluxcd/pkg/runtime/pprof" "github.com/fluxcd/pkg/runtime/probes" "github.com/fluxcd/source-controller/internal/features" - "github.com/fluxcd/source-controller/internal/helm/util" + "github.com/fluxcd/source-controller/internal/helm/registry" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/fluxcd/source-controller/controllers" @@ -239,7 +239,7 @@ func main() { Metrics: metricsH, Getters: getters, ControllerName: controllerName, - RegistryClientGenerator: util.RegistryClientGenerator, + RegistryClientGenerator: registry.ClientGenerator, }).SetupWithManagerAndOptions(mgr, controllers.HelmRepositoryReconcilerOptions{ MaxConcurrentReconciles: concurrent, RateLimiter: helper.GetRateLimiter(rateLimiterOptions), @@ -270,7 +270,7 @@ func main() { if err = (&controllers.HelmChartReconciler{ Client: mgr.GetClient(), - RegistryClientGenerator: util.RegistryClientGenerator, + RegistryClientGenerator: registry.ClientGenerator, Storage: storage, Getters: getters, EventRecorder: eventRecorder, From a3be7e5d3d7bb924a066ea7253eb29fd2fb8a25a Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Mon, 23 May 2022 11:28:01 +0200 Subject: [PATCH 07/10] document generateBuildResult Signed-off-by: Max Jonas Werner --- internal/helm/chart/builder_remote.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/helm/chart/builder_remote.go b/internal/helm/chart/builder_remote.go index 97de6813..d170ec29 100644 --- a/internal/helm/chart/builder_remote.go +++ b/internal/helm/chart/builder_remote.go @@ -199,9 +199,9 @@ func (b *remoteChartBuilder) downloadFromRepository(remote *repository.ChartRepo if err != nil { return nil, err } + *buildResult = *result if shouldReturn { - *buildResult = *result return nil, nil } @@ -212,11 +212,11 @@ func (b *remoteChartBuilder) downloadFromRepository(remote *repository.ChartRepo return nil, &BuildError{Reason: ErrChartPull, Err: err} } - *buildResult = *result - return res, nil } +// generateBuildResult returns a Build object generated from the given chart version and build options. It also returns +// true if the given chart can be retrieved from cache and doesn't need to be downloaded again. func generateBuildResult(cv *repo.ChartVersion, opts BuildOptions) (*Build, bool, error) { result := &Build{} result.Version = cv.Version From 09a2458cfd27139e0e93909db1c928c54c064506 Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Mon, 23 May 2022 15:10:21 +0200 Subject: [PATCH 08/10] fix import order Signed-off-by: Max Jonas Werner --- controllers/helmchart_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 07efa41e..a294c8cb 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -60,11 +60,11 @@ import ( serror "github.com/fluxcd/source-controller/internal/error" "github.com/fluxcd/source-controller/internal/helm/chart" "github.com/fluxcd/source-controller/internal/helm/getter" + "github.com/fluxcd/source-controller/internal/helm/registry" "github.com/fluxcd/source-controller/internal/helm/repository" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" "github.com/fluxcd/source-controller/internal/util" - "github.com/fluxcd/source-controller/internal/helm/registry" ) // helmChartReadyCondition contains all the conditions information @@ -552,7 +552,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * repository.WithMemoryCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) { r.IncCacheEvents(event, obj.Name, obj.Namespace) })) - if err != nil { + if err != nil { return chartRepoErrorReturn(err, obj) } chartRepo = httpChartRepo From 7cfd94effba6b5984100dd43f52e3c038e4e7546 Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Mon, 23 May 2022 15:54:28 +0200 Subject: [PATCH 09/10] fix func doc Signed-off-by: Max Jonas Werner --- internal/helm/registry/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/helm/registry/client.go b/internal/helm/registry/client.go index 0e835e8f..9cb68a45 100644 --- a/internal/helm/registry/client.go +++ b/internal/helm/registry/client.go @@ -23,7 +23,7 @@ import ( "helm.sh/helm/v3/pkg/registry" ) -// RegistryClientGenerator generates a registry client and a temporary credential file. +// ClientGenerator generates a registry client and a temporary credential file. // The client is meant to be used for a single reconciliation. // The file is meant to be used for a single reconciliation and deleted after. func ClientGenerator(isLogin bool) (*registry.Client, string, error) { From bb569bec1fd46de57bd8b1a496a86e3bd5be5240 Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Tue, 24 May 2022 10:30:32 +0200 Subject: [PATCH 10/10] include Secret name in returned errors Signed-off-by: Max Jonas Werner --- internal/helm/registry/auth.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/helm/registry/auth.go b/internal/helm/registry/auth.go index 64922cdd..a37e4c65 100644 --- a/internal/helm/registry/auth.go +++ b/internal/helm/registry/auth.go @@ -19,15 +19,16 @@ func LoginOptionFromSecret(registryURL string, secret corev1.Secret) (registry.L if secret.Type == corev1.SecretTypeDockerConfigJson { dockerCfg, err := config.LoadFromReader(bytes.NewReader(secret.Data[corev1.DockerConfigJsonKey])) if err != nil { - return nil, fmt.Errorf("unable to load Docker config: %w", err) + return nil, fmt.Errorf("unable to load Docker config from Secret '%s': %w", secret.Name, err) } parsedURL, err := url.Parse(registryURL) if err != nil { - return nil, fmt.Errorf("unable to parse registry URL: %w", err) + return nil, fmt.Errorf("unable to parse registry URL '%s' while reconciling Secret '%s': %w", + registryURL, secret.Name, err) } authConfig, err := dockerCfg.GetAuthConfig(parsedURL.Host) if err != nil { - return nil, fmt.Errorf("unable to get authentication data from Secret: %w", err) + return nil, fmt.Errorf("unable to get authentication data from Secret '%s': %w", secret.Name, err) } username = authConfig.Username password = authConfig.Password