From 5f3c014966f68fc8e9b33da143fd5dd4d9e8edf8 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 9 Dec 2020 15:28:52 +0100 Subject: [PATCH] Validate provided name for charts from HelmRepos Following the rules described in https://helm.sh/docs/chart_best_practices/conventions/#chart-names. This guards against people following the wrong guidance of Artifactory, that supports and promotes repository indexes with e.g. '/' in the chart names. In a future version this should be moved to a validation webhook, but there are still discussions ongoing around the TLS certificates for this. Signed-off-by: Hidde Beydals --- controllers/helmchart_controller.go | 22 +++++++++++++++++++++- controllers/helmchart_controller_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index aca66319..431527ff 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -23,6 +23,7 @@ import ( "net/url" "os" "path" + "regexp" "strings" "time" @@ -187,7 +188,8 @@ func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { } // Emit an event if we did not have an artifact before, or the revision has changed - if chart.Status.Artifact == nil || reconciledChart.Status.Artifact.Revision != chart.Status.Artifact.Revision { + if (chart.Status.Artifact == nil && reconciledChart.Status.Artifact != nil) || + reconciledChart.Status.Artifact.Revision != chart.Status.Artifact.Revision { r.event(reconciledChart, events.EventSeverityInfo, sourcev1.HelmChartReadyMessage(reconciledChart)) } r.recordReadiness(reconciledChart) @@ -275,6 +277,12 @@ func (r *HelmChartReconciler) getSource(ctx context.Context, chart sourcev1.Helm func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context, repository sourcev1.HelmRepository, chart sourcev1.HelmChart, force bool) (sourcev1.HelmChart, error) { + // TODO: move this to a validation webhook once the discussion around + // certificates has settled: https://github.com/fluxcd/image-reflector-controller/issues/69 + if err := validHelmChartName(chart.Spec.Chart); err != nil { + return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), nil + } + // Configure ChartRepository getter options var clientOpts []getter.Option if secret, err := r.getHelmRepositorySecret(ctx, &repository); err != nil { @@ -877,3 +885,15 @@ func (r *HelmChartReconciler) requestsForBucketChange(obj handler.MapObject) []r } return reqs } + +// validHelmChartName returns an error if the given string is not a +// valid Helm chart name; a valid name must be lower case letters +// and numbers, words may be separated with dashes (-). +// Ref: https://helm.sh/docs/chart_best_practices/conventions/#chart-names +func validHelmChartName(s string) error { + chartFmt := regexp.MustCompile("^([-a-z0-9]*)$") + if !chartFmt.MatchString(s) { + return fmt.Errorf("invalid chart name %q, a valid name must be lower case letters and numbers and MAY be seperated with dashes (-)", s) + } + return nil +} diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index 8268a548..7b99063f 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -26,6 +26,7 @@ import ( "path" "path/filepath" "strings" + "testing" "time" "github.com/fluxcd/pkg/apis/meta" @@ -938,3 +939,26 @@ var _ = Describe("HelmChartReconciler", func() { }) }) }) + +func Test_validHelmChartName(t *testing.T) { + tests := []struct { + name string + chart string + expectErr bool + }{ + {"valid", "drupal", false}, + {"valid dash", "nginx-lego", false}, + {"valid dashes", "aws-cluster-autoscaler", false}, + {"valid alphanum", "ng1nx-leg0", false}, + {"invalid slash", "artifactory/invalid", true}, + {"invalid dot", "in.valid", true}, + {"invalid uppercase", "inValid", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := validHelmChartName(tt.chart); (err != nil) != tt.expectErr { + t.Errorf("validHelmChartName() error = %v, expectErr %v", err, tt.expectErr) + } + }) + } +}