Skip to content

Commit 01c459d

Browse files
hiddecohiddeco
committed
controllers: more tidying of wiring
1 parent cfd087a commit 01c459d

File tree

3 files changed

+46
-99
lines changed

3 files changed

+46
-99
lines changed

‎controllers/helmchart_controller.go

+30-54
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,12 @@ import (
2222
"net/url"
2323
"os"
2424
"path/filepath"
25-
"regexp"
2625
"strings"
2726
"time"
2827

2928
securejoin "github.com/cyphar/filepath-securejoin"
3029
"github.com/go-logr/logr"
31-
extgetter "helm.sh/helm/v3/pkg/getter"
30+
helmgetter "helm.sh/helm/v3/pkg/getter"
3231
corev1 "k8s.io/api/core/v1"
3332
"k8s.io/apimachinery/pkg/api/errors"
3433
apimeta "k8s.io/apimachinery/pkg/api/meta"
@@ -69,7 +68,7 @@ type HelmChartReconciler struct {
6968
client.Client
7069
Scheme *runtime.Scheme
7170
Storage *Storage
72-
Getters extgetter.Providers
71+
Getters helmgetter.Providers
7372
EventRecorder kuberecorder.EventRecorder
7473
ExternalEventRecorder *events.Recorder
7574
MetricsRecorder *metrics.Recorder
@@ -199,7 +198,7 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
199198
}
200199

201200
// Create working directory
202-
workDir, err := os.MkdirTemp("", chart.Kind + "-" + chart.Namespace + "-" + chart.Name + "-")
201+
workDir, err := os.MkdirTemp("", chart.Kind+"-"+chart.Namespace+"-"+chart.Name+"-")
203202
if err != nil {
204203
err = fmt.Errorf("failed to create temporary working directory: %w", err)
205204
chart = sourcev1.HelmChartNotReady(*chart.DeepCopy(), sourcev1.ChartPullFailedReason, err.Error())
@@ -216,21 +215,6 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
216215
var reconcileErr error
217216
switch typedSource := source.(type) {
218217
case *sourcev1.HelmRepository:
219-
// TODO: move this to a validation webhook once the discussion around
220-
// certificates has settled: https://github.com/fluxcd/image-reflector-controller/issues/69
221-
if err := validHelmChartName(chart.Spec.Chart); err != nil {
222-
reconciledChart = sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error())
223-
log.Error(err, "validation failed")
224-
if err := r.updateStatus(ctx, req, reconciledChart.Status); err != nil {
225-
log.Info(fmt.Sprintf("%v", reconciledChart.Status))
226-
log.Error(err, "unable to update status")
227-
return ctrl.Result{Requeue: true}, err
228-
}
229-
r.event(ctx, reconciledChart, events.EventSeverityError, err.Error())
230-
r.recordReadiness(ctx, reconciledChart)
231-
// Do not requeue as there is no chance on recovery.
232-
return ctrl.Result{Requeue: false}, nil
233-
}
234218
reconciledChart, reconcileErr = r.fromHelmRepository(ctx, *typedSource, *chart.DeepCopy(), workDir, changed)
235219
case *sourcev1.GitRepository, *sourcev1.Bucket:
236220
reconciledChart, reconcileErr = r.fromTarballArtifact(ctx, *typedSource.GetArtifact(), *chart.DeepCopy(),
@@ -309,10 +293,10 @@ func (r *HelmChartReconciler) getSource(ctx context.Context, chart sourcev1.Helm
309293
func (r *HelmChartReconciler) fromHelmRepository(ctx context.Context, repo sourcev1.HelmRepository, c sourcev1.HelmChart,
310294
workDir string, force bool) (sourcev1.HelmChart, error) {
311295
// Configure Index getter options
312-
clientOpts := []extgetter.Option{
313-
extgetter.WithURL(repo.Spec.URL),
314-
extgetter.WithTimeout(repo.Spec.Timeout.Duration),
315-
extgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
296+
clientOpts := []helmgetter.Option{
297+
helmgetter.WithURL(repo.Spec.URL),
298+
helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
299+
helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
316300
}
317301
if secret, err := r.getHelmRepositorySecret(ctx, &repo); err != nil {
318302
return sourcev1.HelmChartNotReady(c, sourcev1.AuthenticationFailedReason, err.Error()), err
@@ -423,7 +407,7 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so
423407
err = fmt.Errorf("artifact untar error: %w", err)
424408
return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err
425409
}
426-
if err =f.Close(); err != nil {
410+
if err = f.Close(); err != nil {
427411
err = fmt.Errorf("artifact close error: %w", err)
428412
return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err
429413
}
@@ -440,20 +424,17 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so
440424
return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err
441425
}
442426
dm := chart.NewDependencyManager(
443-
chart.WithRepositoryCallback(r.getNamespacedChartRepositoryCallback(ctx, authDir, c.GetNamespace())),
427+
chart.WithRepositoryCallback(r.namespacedChartRepositoryCallback(ctx, authDir, c.GetNamespace())),
444428
)
445429
defer dm.Clear()
446430

447-
// Get any cached chart
448-
var cachedChart string
449-
if artifact := c.Status.Artifact; artifact != nil {
450-
cachedChart = artifact.Path
451-
}
452-
431+
// Configure builder options, including any previously cached chart
453432
buildsOpts := chart.BuildOptions{
454-
ValueFiles: c.GetValuesFiles(),
455-
CachedChart: cachedChart,
456-
Force: force,
433+
ValueFiles: c.GetValuesFiles(),
434+
Force: force,
435+
}
436+
if artifact := c.Status.Artifact; artifact != nil {
437+
buildsOpts.CachedChart = artifact.Path
457438
}
458439

459440
// Add revision metadata to chart build
@@ -465,7 +446,7 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so
465446

466447
// Build chart
467448
chartB := chart.NewLocalBuilder(dm)
468-
build, err := chartB.Build(ctx, chart.LocalReference{BaseDir: sourceDir, Path: chartPath}, filepath.Join(workDir, "chart.tgz"), buildsOpts)
449+
build, err := chartB.Build(ctx, chart.LocalReference{WorkDir: sourceDir, Path: chartPath}, filepath.Join(workDir, "chart.tgz"), buildsOpts)
469450
if err != nil {
470451
return sourcev1.HelmChartNotReady(c, sourcev1.ChartPackageFailedReason, err.Error()), err
471452
}
@@ -475,7 +456,8 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so
475456

476457
// If the path of the returned build equals the cache path,
477458
// there are no changes to the chart
478-
if build.Path == cachedChart {
459+
if apimeta.IsStatusConditionTrue(c.Status.Conditions, meta.ReadyCondition) &&
460+
build.Path == buildsOpts.CachedChart {
479461
// Ensure hostname is updated
480462
if c.GetArtifact().URL != newArtifact.URL {
481463
r.Storage.SetArtifactURL(c.GetArtifact())
@@ -515,11 +497,17 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so
515497
return sourcev1.HelmChartReady(c, newArtifact, cUrl, sourcev1.ChartPackageSucceededReason, build.Summary()), nil
516498
}
517499

518-
// TODO(hidde): factor out to helper?
519-
func (r *HelmChartReconciler) getNamespacedChartRepositoryCallback(ctx context.Context, dir, namespace string) chart.GetChartRepositoryCallback {
500+
// namespacedChartRepositoryCallback returns a chart.GetChartRepositoryCallback
501+
// scoped to the given namespace. Credentials for retrieved v1beta1.HelmRepository
502+
// objects are stored in the given directory.
503+
// The returned callback returns a repository.ChartRepository configured with the
504+
// retrieved v1beta1.HelmRepository, or a shim with defaults if no object could
505+
// be found.
506+
func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, dir, namespace string) chart.GetChartRepositoryCallback {
520507
return func(url string) (*repository.ChartRepository, error) {
521508
repo, err := r.resolveDependencyRepository(ctx, url, namespace)
522509
if err != nil {
510+
// Return Kubernetes client errors, but ignore others
523511
if errors.ReasonForError(err) != metav1.StatusReasonUnknown {
524512
return nil, err
525513
}
@@ -530,10 +518,10 @@ func (r *HelmChartReconciler) getNamespacedChartRepositoryCallback(ctx context.C
530518
},
531519
}
532520
}
533-
clientOpts := []extgetter.Option{
534-
extgetter.WithURL(repo.Spec.URL),
535-
extgetter.WithTimeout(repo.Spec.Timeout.Duration),
536-
extgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
521+
clientOpts := []helmgetter.Option{
522+
helmgetter.WithURL(repo.Spec.URL),
523+
helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
524+
helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
537525
}
538526
if secret, err := r.getHelmRepositorySecret(ctx, repo); err != nil {
539527
return nil, err
@@ -801,18 +789,6 @@ func (r *HelmChartReconciler) requestsForBucketChange(o client.Object) []reconci
801789
return reqs
802790
}
803791

804-
// validHelmChartName returns an error if the given string is not a
805-
// valid Helm chart name; a valid name must be lower case letters
806-
// and numbers, words may be separated with dashes (-).
807-
// Ref: https://helm.sh/docs/chart_best_practices/conventions/#chart-names
808-
func validHelmChartName(s string) error {
809-
chartFmt := regexp.MustCompile("^([-a-z0-9]*)$")
810-
if !chartFmt.MatchString(s) {
811-
return fmt.Errorf("invalid chart name %q, a valid name must be lower case letters and numbers and MAY be separated with dashes (-)", s)
812-
}
813-
return nil
814-
}
815-
816792
func (r *HelmChartReconciler) recordSuspension(ctx context.Context, chart sourcev1.HelmChart) {
817793
if r.MetricsRecorder == nil {
818794
return

‎controllers/helmchart_controller_test.go

-24
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import (
2525
"path"
2626
"path/filepath"
2727
"strings"
28-
"testing"
2928
"time"
3029

3130
"github.com/fluxcd/pkg/apis/meta"
@@ -1327,26 +1326,3 @@ var _ = Describe("HelmChartReconciler", func() {
13271326
})
13281327
})
13291328
})
1330-
1331-
func Test_validHelmChartName(t *testing.T) {
1332-
tests := []struct {
1333-
name string
1334-
chart string
1335-
expectErr bool
1336-
}{
1337-
{"valid", "drupal", false},
1338-
{"valid dash", "nginx-lego", false},
1339-
{"valid dashes", "aws-cluster-autoscaler", false},
1340-
{"valid alphanum", "ng1nx-leg0", false},
1341-
{"invalid slash", "artifactory/invalid", true},
1342-
{"invalid dot", "in.valid", true},
1343-
{"invalid uppercase", "inValid", true},
1344-
}
1345-
for _, tt := range tests {
1346-
t.Run(tt.name, func(t *testing.T) {
1347-
if err := validHelmChartName(tt.chart); (err != nil) != tt.expectErr {
1348-
t.Errorf("validHelmChartName() error = %v, expectErr %v", err, tt.expectErr)
1349-
}
1350-
})
1351-
}
1352-
}

‎controllers/helmrepository_controller.go

+16-21
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import (
2424
"time"
2525

2626
"github.com/go-logr/logr"
27-
extgetter "helm.sh/helm/v3/pkg/getter"
27+
helmgetter "helm.sh/helm/v3/pkg/getter"
2828
corev1 "k8s.io/api/core/v1"
2929
apimeta "k8s.io/apimachinery/pkg/api/meta"
3030
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -43,9 +43,9 @@ import (
4343
"github.com/fluxcd/pkg/runtime/metrics"
4444
"github.com/fluxcd/pkg/runtime/predicates"
4545

46+
sourcev1 "github.com/fluxcd/source-controller/api/v1beta1"
4647
"github.com/fluxcd/source-controller/internal/helm/getter"
4748
"github.com/fluxcd/source-controller/internal/helm/repository"
48-
sourcev1 "github.com/fluxcd/source-controller/api/v1beta1"
4949
)
5050

5151
// +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmrepositories,verbs=get;list;watch;create;update;patch;delete
@@ -58,7 +58,7 @@ type HelmRepositoryReconciler struct {
5858
client.Client
5959
Scheme *runtime.Scheme
6060
Storage *Storage
61-
Getters extgetter.Providers
61+
Getters helmgetter.Providers
6262
EventRecorder kuberecorder.EventRecorder
6363
ExternalEventRecorder *events.Recorder
6464
MetricsRecorder *metrics.Recorder
@@ -171,10 +171,10 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque
171171
}
172172

173173
func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.HelmRepository) (sourcev1.HelmRepository, error) {
174-
clientOpts := []extgetter.Option{
175-
extgetter.WithURL(repo.Spec.URL),
176-
extgetter.WithTimeout(repo.Spec.Timeout.Duration),
177-
extgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
174+
clientOpts := []helmgetter.Option{
175+
helmgetter.WithURL(repo.Spec.URL),
176+
helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
177+
helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
178178
}
179179
if repo.Spec.SecretRef != nil {
180180
name := types.NamespacedName{
@@ -189,7 +189,7 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.
189189
return sourcev1.HelmRepositoryNotReady(repo, sourcev1.AuthenticationFailedReason, err.Error()), err
190190
}
191191

192-
authDir, err := os.MkdirTemp("", "helm-repository-")
192+
authDir, err := os.MkdirTemp("", repo.Kind+"-"+repo.Namespace+"-"+repo.Name+"-")
193193
if err != nil {
194194
err = fmt.Errorf("failed to create temporary working directory for credentials: %w", err)
195195
return sourcev1.HelmRepositoryNotReady(repo, sourcev1.AuthenticationFailedReason, err.Error()), err
@@ -213,7 +213,7 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.
213213
return sourcev1.HelmRepositoryNotReady(repo, sourcev1.IndexationFailedReason, err.Error()), err
214214
}
215215
}
216-
revision, err := chartRepo.CacheIndex()
216+
checksum, err := chartRepo.CacheIndex()
217217
if err != nil {
218218
err = fmt.Errorf("failed to download repository index: %w", err)
219219
return sourcev1.HelmRepositoryNotReady(repo, sourcev1.IndexationFailedReason, err.Error()), err
@@ -222,12 +222,12 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.
222222

223223
artifact := r.Storage.NewArtifactFor(repo.Kind,
224224
repo.ObjectMeta.GetObjectMeta(),
225-
revision,
226-
fmt.Sprintf("index-%s.yaml", revision))
225+
"",
226+
fmt.Sprintf("index-%s.yaml", checksum))
227227

228228
// Return early on unchanged index
229229
if apimeta.IsStatusConditionTrue(repo.Status.Conditions, meta.ReadyCondition) &&
230-
repo.GetArtifact().HasRevision(artifact.Revision) {
230+
(repo.GetArtifact() != nil && repo.GetArtifact().Checksum == checksum) {
231231
if artifact.URL != repo.GetArtifact().URL {
232232
r.Storage.SetArtifactURL(repo.GetArtifact())
233233
repo.Status.URL = r.Storage.SetHostname(repo.Status.URL)
@@ -239,7 +239,9 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.
239239
if err := chartRepo.LoadFromCache(); err != nil {
240240
return sourcev1.HelmRepositoryNotReady(repo, sourcev1.IndexationFailedReason, err.Error()), err
241241
}
242-
defer chartRepo.Unload()
242+
// The repository checksum is the SHA256 of the loaded bytes, after sorting
243+
artifact.Revision = chartRepo.Checksum
244+
chartRepo.Unload()
243245

244246
// Create artifact dir
245247
err = r.Storage.MkdirAll(artifact)
@@ -257,16 +259,9 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.
257259
defer unlock()
258260

259261
// Save artifact to storage
260-
storageTarget := r.Storage.LocalPath(artifact)
261-
if storageTarget == "" {
262-
err := fmt.Errorf("failed to calcalute local storage path to store artifact to")
263-
return sourcev1.HelmRepositoryNotReady(repo, sourcev1.StorageOperationFailedReason, err.Error()), err
264-
}
265-
if err = chartRepo.Index.WriteFile(storageTarget, 0644); err != nil {
262+
if err = r.Storage.CopyFromPath(&artifact, chartRepo.CachePath); err != nil {
266263
return sourcev1.HelmRepositoryNotReady(repo, sourcev1.StorageOperationFailedReason, err.Error()), err
267264
}
268-
// TODO(hidde): it would be better to make the Storage deal with this
269-
artifact.Checksum = chartRepo.Checksum
270265
artifact.LastUpdateTime = metav1.Now()
271266

272267
// Update index symlink

0 commit comments

Comments
 (0)