From cf3735e2a29dd16c42ad5022dbbdcedeb3b2d60a Mon Sep 17 00:00:00 2001 From: Sunny Date: Tue, 31 Oct 2023 19:40:08 +0000 Subject: [PATCH 1/4] Static helmrepository OCI Remove the HelmRepositoryOCI reconciler and make HelmRepository of type OCI static. The existing HelmRepository OCI objects are migrated to static object by removing their finalizers and status. New HelmRepository OCI objects go through one time migration to remove the status. These are not reconciled again, unless the type is changed to default. On type switching from HelmRepository default to OCI, the finalizer, status and artifact are removed to make the object static. On switching from OCI to default, a complete reconciliation of HelmRepository takes place to build artifact and add status and finalizer. The HelmRepository .spec.url has a new validation to check the URL scheme. This is to add some validation to HelmRepository OCI since it's not backed by a reconciler for full validation. Add HelmRepositoryOCIMigrationPredicate predicate to detect and allow reconciliation of HelmRepository OCI objects that need migration. The other predicates that filtered the HelmRepository events based on the type have been removed as all the HelmRepositories will now be reconciled by a single reconciler. HelmRepositoryOCIMigrationPredicate readily allows non-OCI objects and only checks if a migration is needed for OCI type object. Add controller tests for different migration scenarios. Signed-off-by: Sunny --- api/v1beta2/helmrepository_types.go | 1 + ...ce.toolkit.fluxcd.io_helmrepositories.yaml | 1 + hack/ci/e2e.sh | 2 - internal/controller/helmchart_controller.go | 7 + .../controller/helmchart_controller_test.go | 2 +- .../controller/helmrepository_controller.go | 58 ++- .../helmrepository_controller_oci.go | 418 --------------- .../helmrepository_controller_oci_test.go | 478 ------------------ .../helmrepository_controller_test.go | 123 +++-- internal/controller/suite_test.go | 12 - .../helmrepository_type_predicate.go | 80 +-- .../helmrepository_type_predicate_test.go | 270 +++++++--- main.go | 13 - 13 files changed, 395 insertions(+), 1070 deletions(-) delete mode 100644 internal/controller/helmrepository_controller_oci.go delete mode 100644 internal/controller/helmrepository_controller_oci_test.go diff --git a/api/v1beta2/helmrepository_types.go b/api/v1beta2/helmrepository_types.go index e1df71568..a73a445b8 100644 --- a/api/v1beta2/helmrepository_types.go +++ b/api/v1beta2/helmrepository_types.go @@ -44,6 +44,7 @@ const ( type HelmRepositorySpec struct { // URL of the Helm repository, a valid URL contains at least a protocol and // host. + // +kubebuilder:validation:Pattern="^(http|https|oci)://.*$" // +required URL string `json:"url"` diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml index 6de6911d8..5d1572873 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml @@ -373,6 +373,7 @@ spec: url: description: URL of the Helm repository, a valid URL contains at least a protocol and host. + pattern: ^(http|https|oci)://.*$ type: string required: - interval diff --git a/hack/ci/e2e.sh b/hack/ci/e2e.sh index c48393a60..d4bde9a20 100755 --- a/hack/ci/e2e.sh +++ b/hack/ci/e2e.sh @@ -75,7 +75,6 @@ kubectl -n source-system rollout status deploy/source-controller --timeout=1m kubectl -n source-system wait gitrepository/gitrepository-sample --for=condition=ready --timeout=1m kubectl -n source-system wait ocirepository/ocirepository-sample --for=condition=ready --timeout=1m kubectl -n source-system wait helmrepository/helmrepository-sample --for=condition=ready --timeout=1m -kubectl -n source-system wait helmrepository/helmrepository-sample-oci --for=condition=ready --timeout=1m kubectl -n source-system wait helmchart/helmchart-sample --for=condition=ready --timeout=1m kubectl -n source-system wait helmchart/helmchart-sample-oci --for=condition=ready --timeout=1m kubectl -n source-system delete -f "${ROOT_DIR}/config/samples" @@ -145,7 +144,6 @@ kubectl -n source-system wait gitrepository/large-repo --for=condition=ready --t echo "Run HelmChart from OCI registry tests" kubectl -n source-system apply -f "${ROOT_DIR}/config/testdata/helmchart-from-oci/source.yaml" -kubectl -n source-system wait helmrepository/podinfo --for=condition=ready --timeout=1m kubectl -n source-system wait helmchart/podinfo --for=condition=ready --timeout=1m kubectl -n source-system wait helmchart/podinfo-keyless --for=condition=ready --timeout=1m diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index f840a85bc..52b4b0edb 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -18,6 +18,7 @@ package controller import ( "context" + "crypto/tls" "errors" "fmt" "net/url" @@ -139,6 +140,12 @@ type HelmChartReconciler struct { patchOptions []patch.Option } +// RegistryClientGeneratorFunc is a function that returns a registry client +// 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(tlsConfig *tls.Config, isLogin bool) (*helmreg.Client, string, error) + func (r *HelmChartReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error { return r.SetupWithManagerAndOptions(ctx, mgr, HelmChartReconcilerOptions{}) } diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index 796bfdcd4..b9472419a 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/internal/controller/helmchart_controller_test.go @@ -197,7 +197,7 @@ func TestHelmChartReconciler_Reconcile(t *testing.T) { { name: "Stalling on invalid repository URL", beforeFunc: func(repository *helmv1.HelmRepository) { - repository.Spec.URL = "://unsupported" // Invalid URL + repository.Spec.URL = "https://unsupported/foo://" // Invalid URL }, assertFunc: func(g *WithT, obj *helmv1.HelmChart, _ *helmv1.HelmRepository) { key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace} diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go index 8e252979a..27663c625 100644 --- a/internal/controller/helmrepository_controller.go +++ b/internal/controller/helmrepository_controller.go @@ -22,11 +22,13 @@ import ( "errors" "fmt" "net/url" + "strings" "time" "github.com/docker/go-units" "github.com/opencontainers/go-digest" helmgetter "helm.sh/helm/v3/pkg/getter" + helmreg "helm.sh/helm/v3/pkg/registry" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" kuberecorder "k8s.io/client-go/tools/record" @@ -138,10 +140,7 @@ func (r *HelmRepositoryReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, For(&helmv1.HelmRepository{}). WithEventFilter( predicate.And( - predicate.Or( - intpredicates.HelmRepositoryTypePredicate{RepositoryType: helmv1.HelmRepositoryTypeDefault}, - intpredicates.HelmRepositoryTypePredicate{RepositoryType: ""}, - ), + intpredicates.HelmRepositoryOCIMigrationPredicate{}, predicate.Or(predicate.GenerationChangedPredicate{}, predicates.ReconcileRequestedPredicate{}), ), ). @@ -164,6 +163,11 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque // Initialize the patch helper with the current version of the object. serialPatcher := patch.NewSerialPatcher(obj, r.Client) + // If it's of type OCI, migrate the object to static. + if obj.Spec.Type == helmv1.HelmRepositoryTypeOCI { + return r.migrationToStatic(ctx, serialPatcher, obj) + } + // recResult stores the abstracted reconcile result. var recResult sreconcile.Result @@ -193,8 +197,8 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque r.Metrics.RecordDuration(ctx, obj, start) }() - // Examine if the object is under deletion or if a type change has happened. - if !obj.ObjectMeta.DeletionTimestamp.IsZero() || (obj.Spec.Type != "" && obj.Spec.Type != helmv1.HelmRepositoryTypeDefault) { + // Examine if the object is under deletion. + if !obj.ObjectMeta.DeletionTimestamp.IsZero() { recResult, retErr = r.reconcileDelete(ctx, obj) return } @@ -391,6 +395,18 @@ func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, sp *pat // pointer is set to the newly fetched index. func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, obj *helmv1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) { + // Ensure it's not an OCI URL. API validation ensures that only + // http/https/oci scheme are allowed. + if strings.HasPrefix(obj.Spec.URL, helmreg.OCIScheme) { + err := fmt.Errorf("'oci' URL scheme cannot be used with 'default' HelmRepository type") + e := serror.NewStalling( + fmt.Errorf("invalid Helm repository URL: %w", err), + sourcev1.URLInvalidReason, + ) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e + } + normalizedURL, err := repository.NormalizeURL(obj.Spec.URL) if err != nil { e := serror.NewStalling( @@ -685,3 +701,33 @@ func (r *HelmRepositoryReconciler) eventLogf(ctx context.Context, obj runtime.Ob } r.Eventf(obj, eventType, reason, msg) } + +// migrateToStatic is HelmRepository OCI migration to static object. +func (r *HelmRepositoryReconciler) migrationToStatic(ctx context.Context, sp *patch.SerialPatcher, obj *helmv1.HelmRepository) (result ctrl.Result, err error) { + // Skip migration if suspended and not being deleted. + if obj.Spec.Suspend && obj.DeletionTimestamp.IsZero() { + return ctrl.Result{}, nil + } + + if !intpredicates.HelmRepositoryOCIRequireMigration(obj) { + // Already migrated, nothing to do. + return ctrl.Result{}, nil + } + + // Delete any artifact. + _, err = r.reconcileDelete(ctx, obj) + if err != nil { + return ctrl.Result{}, err + } + // Delete finalizer and reset the status. + controllerutil.RemoveFinalizer(obj, sourcev1.SourceFinalizer) + obj.Status = helmv1.HelmRepositoryStatus{} + + if err := sp.Patch(ctx, obj); err != nil { + return ctrl.Result{}, err + } + + r.eventLogf(ctx, obj, eventv1.EventTypeTrace, "Migration", + "removed artifact and finalizer to migrate to static HelmRepository of type OCI") + return ctrl.Result{}, nil +} diff --git a/internal/controller/helmrepository_controller_oci.go b/internal/controller/helmrepository_controller_oci.go deleted file mode 100644 index e25eaf4fd..000000000 --- a/internal/controller/helmrepository_controller_oci.go +++ /dev/null @@ -1,418 +0,0 @@ -/* -Copyright 2022 The Flux authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controller - -import ( - "context" - "crypto/tls" - "errors" - "fmt" - "net/url" - "os" - "time" - - 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" - "k8s.io/apimachinery/pkg/runtime" - kerrors "k8s.io/apimachinery/pkg/util/errors" - kuberecorder "k8s.io/client-go/tools/record" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/predicate" - - eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" - "github.com/fluxcd/pkg/apis/meta" - "github.com/fluxcd/pkg/runtime/conditions" - helper "github.com/fluxcd/pkg/runtime/controller" - "github.com/fluxcd/pkg/runtime/jitter" - "github.com/fluxcd/pkg/runtime/patch" - "github.com/fluxcd/pkg/runtime/predicates" - rreconcile "github.com/fluxcd/pkg/runtime/reconcile" - - sourcev1 "github.com/fluxcd/source-controller/api/v1" - helmv1 "github.com/fluxcd/source-controller/api/v1beta2" - "github.com/fluxcd/source-controller/internal/helm/getter" - "github.com/fluxcd/source-controller/internal/helm/repository" - "github.com/fluxcd/source-controller/internal/object" - intpredicates "github.com/fluxcd/source-controller/internal/predicates" -) - -var helmRepositoryOCIOwnedConditions = []string{ - meta.ReadyCondition, - meta.ReconcilingCondition, - meta.StalledCondition, -} - -var helmRepositoryOCINegativeConditions = []string{ - meta.StalledCondition, - meta.ReconcilingCondition, -} - -// +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmrepositories,verbs=get;list;watch;create;update;patch;delete -// +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmrepositories/status,verbs=get;update;patch -// +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmrepositories/finalizers,verbs=get;create;update;patch;delete -// +kubebuilder:rbac:groups="",resources=events,verbs=create;patch - -// HelmRepositoryOCI Reconciler reconciles a v1beta2.HelmRepository object of type OCI. -type HelmRepositoryOCIReconciler struct { - client.Client - kuberecorder.EventRecorder - helper.Metrics - - ControllerName string - RegistryClientGenerator RegistryClientGeneratorFunc - - patchOptions []patch.Option - - // unmanagedConditions are the conditions that are not managed by this - // reconciler and need to be removed from the object before taking ownership - // of the object being reconciled. - unmanagedConditions []string -} - -// RegistryClientGeneratorFunc is a function that returns a registry client -// 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(tlsConfig *tls.Config, isLogin bool) (*helmreg.Client, string, error) - -func (r *HelmRepositoryOCIReconciler) SetupWithManager(mgr ctrl.Manager) error { - return r.SetupWithManagerAndOptions(mgr, HelmRepositoryReconcilerOptions{}) -} - -func (r *HelmRepositoryOCIReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts HelmRepositoryReconcilerOptions) error { - r.unmanagedConditions = conditionsDiff(helmRepositoryReadyCondition.Owned, helmRepositoryOCIOwnedConditions) - r.patchOptions = getPatchOptions(helmRepositoryOCIOwnedConditions, r.ControllerName) - - return ctrl.NewControllerManagedBy(mgr). - For(&helmv1.HelmRepository{}). - WithEventFilter( - predicate.And( - intpredicates.HelmRepositoryTypePredicate{RepositoryType: helmv1.HelmRepositoryTypeOCI}, - predicate.Or(predicate.GenerationChangedPredicate{}, predicates.ReconcileRequestedPredicate{}), - ), - ). - WithOptions(controller.Options{ - RateLimiter: opts.RateLimiter, - }). - Complete(r) -} - -func (r *HelmRepositoryOCIReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, retErr error) { - start := time.Now() - log := ctrl.LoggerFrom(ctx) - - // Fetch the HelmRepository - obj := &helmv1.HelmRepository{} - if err := r.Get(ctx, req.NamespacedName, obj); err != nil { - return ctrl.Result{}, client.IgnoreNotFound(err) - } - - // If the object contains any of the unmanaged conditions, requeue and wait - // for those conditions to be removed first before processing the object. - // NOTE: This will happen only when a HelmRepository's spec.type is switched - // from "default" to "oci". - if conditions.HasAny(obj, r.unmanagedConditions) { - r.eventLogf(ctx, obj, eventv1.EventTypeTrace, "IncompleteTransition", - "object contains conditions managed by other reconciler") - return ctrl.Result{RequeueAfter: time.Second}, nil - } - - // Initialize the patch helper with the current version of the object. - serialPatcher := patch.NewSerialPatcher(obj, r.Client) - - // Always attempt to patch the object after each reconciliation. - defer func() { - // If a reconcile annotation value is found, set it in the object status - // as status.lastHandledReconcileAt. - if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { - object.SetStatusLastHandledReconcileAt(obj, v) - } - - patchOpts := []patch.Option{} - patchOpts = append(patchOpts, r.patchOptions...) - - // Set status observed generation option if the object is stalled, or - // if the object is ready. - if conditions.IsStalled(obj) || conditions.IsReady(obj) { - patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{}) - } - - if err := serialPatcher.Patch(ctx, obj, patchOpts...); err != nil { - // Ignore patch error "not found" when the object is being deleted. - if !obj.GetDeletionTimestamp().IsZero() { - err = kerrors.FilterOut(err, func(e error) bool { return apierrors.IsNotFound(e) }) - } - retErr = kerrors.NewAggregate([]error{retErr, err}) - } - - // Always record suspend, readiness and duration metrics. - r.Metrics.RecordSuspend(ctx, obj, obj.Spec.Suspend) - r.Metrics.RecordReadiness(ctx, obj) - r.Metrics.RecordDuration(ctx, obj, start) - }() - - // Examine if the object is under deletion. - if !obj.ObjectMeta.DeletionTimestamp.IsZero() { - return r.reconcileDelete(ctx, obj) - } - - // Add finalizer first if it doesn't exist to avoid the race condition - // between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp - // is not set. - if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { - controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) - return ctrl.Result{Requeue: true}, nil - } - - // Return if the object is suspended. - if obj.Spec.Suspend { - log.Info("reconciliation is suspended for this object") - return ctrl.Result{}, nil - } - - // Examine if a type change has happened and act accordingly - if obj.Spec.Type != helmv1.HelmRepositoryTypeOCI { - // Remove any stale condition and ignore the object if the type has - // changed. - obj.Status.Conditions = nil - return ctrl.Result{}, nil - } - - result, retErr = r.reconcile(ctx, serialPatcher, obj) - return -} - -// reconcile reconciles the HelmRepository object. While reconciling, when an -// error is encountered, it sets the failure details in the appropriate status -// condition type and returns the error with appropriate ctrl.Result. The object -// status conditions and the returned results are evaluated in the deferred -// block at the very end to summarize the conditions to be in a consistent -// state. -func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, sp *patch.SerialPatcher, obj *helmv1.HelmRepository) (result ctrl.Result, retErr error) { - ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) - defer cancel() - - oldObj := obj.DeepCopy() - - defer func() { - // If it's stalled, ensure reconciling is removed. - if sc := conditions.Get(obj, meta.StalledCondition); sc != nil && sc.Status == metav1.ConditionTrue { - conditions.Delete(obj, meta.ReconcilingCondition) - } - - // Check if it's a successful reconciliation. - if result.RequeueAfter == obj.GetRequeueAfter() && !result.Requeue && - retErr == nil { - // Remove reconciling condition if the reconciliation was successful. - conditions.Delete(obj, meta.ReconcilingCondition) - // If it's not ready even though it's not reconciling or stalled, - // set the ready failure message as the error. - // Based on isNonStalledSuccess() from internal/reconcile/summarize. - if ready := conditions.Get(obj, meta.ReadyCondition); ready != nil && - ready.Status == metav1.ConditionFalse && !conditions.IsStalled(obj) { - retErr = errors.New(conditions.GetMessage(obj, meta.ReadyCondition)) - } - } - - // Presence of reconciling means that the reconciliation didn't succeed. - // Set the Reconciling reason to ProgressingWithRetry to indicate a - // failure retry. - if conditions.IsReconciling(obj) { - reconciling := conditions.Get(obj, meta.ReconcilingCondition) - reconciling.Reason = meta.ProgressingWithRetryReason - conditions.Set(obj, reconciling) - } - - // If it's still a successful reconciliation and it's not reconciling or - // stalled, mark Ready=True. - if !conditions.IsReconciling(obj) && !conditions.IsStalled(obj) && - retErr == nil && result.RequeueAfter == obj.GetRequeueAfter() { - conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "Helm repository is ready") - } - - // Emit events when object's state changes. - ready := conditions.Get(obj, meta.ReadyCondition) - // Became ready from not ready. - if !conditions.IsReady(oldObj) && conditions.IsReady(obj) { - r.eventLogf(ctx, obj, corev1.EventTypeNormal, ready.Reason, ready.Message) - } - // Became not ready from ready. - if conditions.IsReady(oldObj) && !conditions.IsReady(obj) { - r.eventLogf(ctx, obj, corev1.EventTypeWarning, ready.Reason, ready.Message) - } - - // Apply jitter. - if result.RequeueAfter == obj.GetRequeueAfter() { - result.RequeueAfter = jitter.JitteredIntervalDuration(result.RequeueAfter) - } - }() - - // Set reconciling condition. - rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress") - - var reconcileAtVal string - if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { - reconcileAtVal = v - } - - // Persist reconciling if generation differs or reconciliation is requested. - switch { - case obj.Generation != obj.Status.ObservedGeneration: - rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, - "processing object: new generation %d -> %d", obj.Status.ObservedGeneration, obj.Generation) - if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { - result, retErr = ctrl.Result{}, err - return - } - case reconcileAtVal != obj.Status.GetLastHandledReconcileRequest(): - if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { - result, retErr = ctrl.Result{}, err - return - } - } - - // Ensure that it's an OCI URL before continuing. - if !helmreg.IsOCI(obj.Spec.URL) { - u, err := url.Parse(obj.Spec.URL) - if err != nil { - err = fmt.Errorf("failed to parse URL: %w", err) - } else { - err = fmt.Errorf("URL scheme '%s' in '%s' is not supported", u.Scheme, obj.Spec.URL) - } - conditions.MarkStalled(obj, sourcev1.URLInvalidReason, err.Error()) - conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.URLInvalidReason, err.Error()) - ctrl.LoggerFrom(ctx).Error(err, "reconciliation stalled") - result, retErr = ctrl.Result{}, nil - return - } - - normalizedURL, err := repository.NormalizeURL(obj.Spec.URL) - if err != nil { - conditions.MarkStalled(obj, sourcev1.URLInvalidReason, err.Error()) - conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.URLInvalidReason, err.Error()) - result, retErr = ctrl.Result{}, nil - return - } - - conditions.Delete(obj, meta.StalledCondition) - - clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL) - if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error()) - result, retErr = ctrl.Result{}, err - return - } - if certsTmpDir != "" { - defer func() { - if err := os.RemoveAll(certsTmpDir); err != nil { - r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason, - "failed to delete temporary certs directory: %s", err) - } - }() - } - - // Create registry client and login if needed. - registryClient, file, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry()) - if err != nil { - e := fmt.Errorf("failed to create registry client: %w", err) - conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, e.Error()) - result, retErr = ctrl.Result{}, e - return - } - if file != "" { - defer func() { - if err := os.Remove(file); err != nil { - r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason, - "failed to delete temporary credentials file: %s", err) - } - }() - } - - chartRepo, err := repository.NewOCIChartRepository(obj.Spec.URL, repository.WithOCIRegistryClient(registryClient)) - if err != nil { - e := fmt.Errorf("failed to parse URL '%s': %w", obj.Spec.URL, err) - conditions.MarkStalled(obj, sourcev1.URLInvalidReason, e.Error()) - conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.URLInvalidReason, e.Error()) - result, retErr = ctrl.Result{}, nil - return - } - conditions.Delete(obj, meta.StalledCondition) - - // Attempt to login to the registry if credentials are provided. - if clientOpts.MustLoginToRegistry() { - err = chartRepo.Login(clientOpts.RegLoginOpts...) - if err != nil { - e := fmt.Errorf("failed to login to registry '%s': %w", obj.Spec.URL, err) - conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, e.Error()) - result, retErr = ctrl.Result{}, e - return - } - } - - // Remove any stale Ready condition, most likely False, set above. Its value - // is derived from the overall result of the reconciliation in the deferred - // block at the very end. - conditions.Delete(obj, meta.ReadyCondition) - - result, retErr = ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil - return -} - -func (r *HelmRepositoryOCIReconciler) reconcileDelete(ctx context.Context, obj *helmv1.HelmRepository) (ctrl.Result, error) { - // Remove our finalizer from the list - controllerutil.RemoveFinalizer(obj, sourcev1.SourceFinalizer) - - // Stop reconciliation as the object is being deleted - return ctrl.Result{}, nil -} - -// eventLogf records events, and logs at the same time. -// -// This log is different from the debug log in the EventRecorder, in the sense -// that this is a simple log. While the debug log contains complete details -// about the event. -func (r *HelmRepositoryOCIReconciler) eventLogf(ctx context.Context, obj runtime.Object, eventType string, reason string, messageFmt string, args ...interface{}) { - msg := fmt.Sprintf(messageFmt, args...) - // Log and emit event. - if eventType == corev1.EventTypeWarning { - ctrl.LoggerFrom(ctx).Error(errors.New(reason), msg) - } else { - ctrl.LoggerFrom(ctx).Info(msg) - } - r.Eventf(obj, eventType, reason, msg) -} - -func conditionsDiff(a, b []string) []string { - bMap := make(map[string]struct{}, len(b)) - for _, j := range b { - bMap[j] = struct{}{} - } - - r := []string{} - for _, i := range a { - if _, exists := bMap[i]; !exists { - r = append(r, i) - } - } - return r -} diff --git a/internal/controller/helmrepository_controller_oci_test.go b/internal/controller/helmrepository_controller_oci_test.go deleted file mode 100644 index 1d5361c91..000000000 --- a/internal/controller/helmrepository_controller_oci_test.go +++ /dev/null @@ -1,478 +0,0 @@ -/* -Copyright 2022 The Flux authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controller - -import ( - "encoding/base64" - "fmt" - "strconv" - "testing" - - . "github.com/onsi/gomega" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/tools/record" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" - - kstatus "github.com/fluxcd/cli-utils/pkg/kstatus/status" - "github.com/fluxcd/pkg/apis/meta" - "github.com/fluxcd/pkg/runtime/conditions" - conditionscheck "github.com/fluxcd/pkg/runtime/conditions/check" - "github.com/fluxcd/pkg/runtime/patch" - - sourcev1 "github.com/fluxcd/source-controller/api/v1" - helmv1 "github.com/fluxcd/source-controller/api/v1beta2" - "github.com/fluxcd/source-controller/internal/helm/registry" -) - -func TestHelmRepositoryOCIReconciler_deleteBeforeFinalizer(t *testing.T) { - g := NewWithT(t) - - namespaceName := "helmrepo-" + randStringRunes(5) - namespace := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{Name: namespaceName}, - } - g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred()) - t.Cleanup(func() { - g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred()) - }) - - helmrepo := &helmv1.HelmRepository{} - helmrepo.Name = "test-helmrepo" - helmrepo.Namespace = namespaceName - helmrepo.Spec = helmv1.HelmRepositorySpec{ - Interval: metav1.Duration{Duration: interval}, - URL: "https://example.com", - Type: "oci", - } - // Add a test finalizer to prevent the object from getting deleted. - helmrepo.SetFinalizers([]string{"test-finalizer"}) - g.Expect(k8sClient.Create(ctx, helmrepo)).NotTo(HaveOccurred()) - // Add deletion timestamp by deleting the object. - g.Expect(k8sClient.Delete(ctx, helmrepo)).NotTo(HaveOccurred()) - - r := &HelmRepositoryOCIReconciler{ - Client: k8sClient, - EventRecorder: record.NewFakeRecorder(32), - } - // NOTE: Only a real API server responds with an error in this scenario. - _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(helmrepo)}) - g.Expect(err).NotTo(HaveOccurred()) -} - -func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) { - tests := []struct { - name string - secretType corev1.SecretType - secretData map[string][]byte - }{ - { - name: "valid auth data", - secretData: map[string][]byte{ - "username": []byte(testRegistryUsername), - "password": []byte(testRegistryPassword), - }, - }, - { - name: "no auth data", - secretData: nil, - }, - { - name: "dockerconfigjson Secret", - secretType: corev1.SecretTypeDockerConfigJson, - secretData: map[string][]byte{ - ".dockerconfigjson": []byte(`{"auths":{"` + - testRegistryServer.registryHost + `":{"` + - `auth":"` + base64.StdEncoding.EncodeToString([]byte(testRegistryUsername+":"+testRegistryPassword)) + `"}}}`), - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(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: tt.secretData, - } - if tt.secretType != "" { - secret.Type = tt.secretType - } - - g.Expect(testEnv.CreateAndWait(ctx, secret)).To(Succeed()) - - origObj := &helmv1.HelmRepository{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "helmrepository-oci-reconcile-", - Namespace: ns.Name, - }, - Spec: helmv1.HelmRepositorySpec{ - Interval: metav1.Duration{Duration: interval}, - URL: fmt.Sprintf("oci://%s", testRegistryServer.registryHost), - SecretRef: &meta.LocalObjectReference{ - Name: secret.Name, - }, - Provider: helmv1.GenericOCIProvider, - Type: helmv1.HelmRepositoryTypeOCI, - }, - } - obj := origObj.DeepCopy() - g.Expect(testEnv.Create(ctx, obj)).To(Succeed()) - - 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 - waitForSourceReadyWithoutArtifact(ctx, g, obj) - - // Check if the object status is valid. - condns := &conditionscheck.Conditions{NegativePolarity: helmRepositoryReadyCondition.NegativePolarity} - checker := conditionscheck.NewChecker(testEnv.Client, condns) - checker.WithT(g).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 - waitForSourceDeletion(ctx, g, obj) - - // Check if a suspended object gets deleted. - obj = origObj.DeepCopy() - testSuspendedObjectDeleteWithoutArtifact(ctx, g, obj) - }) - } -} - -func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { - type secretOptions struct { - username string - password string - } - - tests := []struct { - name string - url string - registryOpts registryOptions - insecure bool - secretOpts secretOptions - secret *corev1.Secret - certsSecret *corev1.Secret - provider string - providerImg string - want ctrl.Result - wantErr bool - assertConditions []metav1.Condition - }{ - { - name: "HTTP without basic auth", - want: ctrl.Result{RequeueAfter: interval}, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Helm repository is ready"), - }, - }, - { - name: "HTTP with basic auth secret", - want: ctrl.Result{RequeueAfter: interval}, - insecure: true, - registryOpts: registryOptions{ - withBasicAuth: true, - }, - secretOpts: secretOptions{ - username: testRegistryUsername, - password: testRegistryPassword, - }, - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "auth-secretref", - }, - Type: corev1.SecretTypeDockerConfigJson, - Data: map[string][]byte{}, - }, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Helm repository is ready"), - }, - }, - { - name: "HTTP registry - basic auth with invalid secret", - want: ctrl.Result{}, - wantErr: true, - insecure: true, - registryOpts: registryOptions{ - withBasicAuth: true, - }, - secretOpts: secretOptions{ - username: "wrong-pass", - password: "wrong-pass", - }, - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "auth-secretref", - }, - Type: corev1.SecretTypeDockerConfigJson, - Data: map[string][]byte{}, - }, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "processing object: new generation"), - *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "failed to login to registry"), - }, - }, - { - name: "with contextual login provider", - wantErr: true, - insecure: true, - provider: "aws", - providerImg: "oci://123456789000.dkr.ecr.us-east-2.amazonaws.com/test", - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "processing object: new generation"), - *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "failed to get credential from"), - }, - }, - { - name: "with contextual login provider and secretRef", - want: ctrl.Result{RequeueAfter: interval}, - registryOpts: registryOptions{ - withBasicAuth: true, - }, - insecure: true, - secretOpts: secretOptions{ - username: testRegistryUsername, - password: testRegistryPassword, - }, - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "auth-secretref", - }, - Type: corev1.SecretTypeDockerConfigJson, - Data: map[string][]byte{}, - }, - provider: "azure", - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Helm repository is ready"), - }, - }, - { - name: "HTTPS With invalid CA cert", - wantErr: true, - registryOpts: registryOptions{ - withTLS: true, - withClientCertAuth: true, - }, - secretOpts: secretOptions{ - username: testRegistryUsername, - password: testRegistryPassword, - }, - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "auth-secretref", - }, - Type: corev1.SecretTypeDockerConfigJson, - Data: map[string][]byte{}, - }, - certsSecret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "certs-secretref", - }, - Data: map[string][]byte{ - "ca.crt": []byte("invalid caFile"), - }, - }, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "processing object: new generation 0 -> 1"), - *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "cannot append certificate into certificate pool: invalid CA certificate"), - }, - }, - { - name: "HTTPS With CA cert", - want: ctrl.Result{RequeueAfter: interval}, - registryOpts: registryOptions{ - withTLS: true, - withClientCertAuth: true, - }, - secretOpts: secretOptions{ - username: testRegistryUsername, - password: testRegistryPassword, - }, - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "auth-secretref", - }, - Type: corev1.SecretTypeDockerConfigJson, - Data: map[string][]byte{}, - }, - certsSecret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "certs-secretref", - }, - Data: map[string][]byte{ - "ca.crt": tlsCA, - "tls.crt": clientPublicKey, - "tls.key": clientPrivateKey, - }, - }, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Helm repository is ready"), - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - clientBuilder := fakeclient.NewClientBuilder(). - WithScheme(testEnv.GetScheme()). - WithStatusSubresource(&helmv1.HelmRepository{}) - - workspaceDir := t.TempDir() - if tt.insecure { - tt.registryOpts.disableDNSMocking = true - } - server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts) - g.Expect(err).NotTo(HaveOccurred()) - t.Cleanup(func() { - server.Close() - }) - - obj := &helmv1.HelmRepository{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "auth-strategy-", - Generation: 1, - }, - Spec: helmv1.HelmRepositorySpec{ - Interval: metav1.Duration{Duration: interval}, - Timeout: &metav1.Duration{Duration: timeout}, - Type: helmv1.HelmRepositoryTypeOCI, - Provider: helmv1.GenericOCIProvider, - URL: fmt.Sprintf("oci://%s", server.registryHost), - }, - } - - if tt.provider != "" { - obj.Spec.Provider = tt.provider - } - // If a provider specific image is provided, overwrite existing URL - // set earlier. It'll fail, but it's necessary to set them because - // the login check expects the URLs to be of certain pattern. - if tt.providerImg != "" { - obj.Spec.URL = tt.providerImg - } - - if tt.secretOpts.username != "" && tt.secretOpts.password != "" { - tt.secret.Data[".dockerconfigjson"] = []byte(fmt.Sprintf(`{"auths": {%q: {"username": %q, "password": %q}}}`, - server.registryHost, tt.secretOpts.username, tt.secretOpts.password)) - } - - if tt.secret != nil { - clientBuilder.WithObjects(tt.secret) - obj.Spec.SecretRef = &meta.LocalObjectReference{ - Name: tt.secret.Name, - } - } - - if tt.certsSecret != nil { - clientBuilder.WithObjects(tt.certsSecret) - obj.Spec.CertSecretRef = &meta.LocalObjectReference{ - Name: tt.certsSecret.Name, - } - } - - r := &HelmRepositoryOCIReconciler{ - Client: clientBuilder.Build(), - EventRecorder: record.NewFakeRecorder(32), - RegistryClientGenerator: registry.ClientGenerator, - patchOptions: getPatchOptions(helmRepositoryOCIOwnedConditions, "sc"), - } - - g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) - defer func() { - g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) - }() - - sp := patch.NewSerialPatcher(obj, r.Client) - got, err := r.reconcile(ctx, sp, obj) - g.Expect(err != nil).To(Equal(tt.wantErr)) - g.Expect(got).To(Equal(tt.want)) - g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) - - // In-progress status condition validity. - checker := conditionscheck.NewInProgressChecker(r.Client) - // NOTE: Check the object directly as reconcile() doesn't apply the - // final patch, the object has unapplied changes. - checker.DisableFetch = true - checker.WithT(g).CheckErr(ctx, obj) - }) - } -} - -func TestConditionsDiff(t *testing.T) { - tests := []struct { - a, b, want []string - }{ - {[]string{"a", "b", "c"}, []string{"b", "d"}, []string{"a", "c"}}, - {[]string{"a", "b", "c"}, []string{}, []string{"a", "b", "c"}}, - {[]string{}, []string{"b", "d"}, []string{}}, - {[]string{}, []string{}, []string{}}, - {[]string{"a", "b"}, nil, []string{"a", "b"}}, - {nil, []string{"a", "b"}, []string{}}, - {nil, nil, []string{}}, - } - - for i, tt := range tests { - t.Run(strconv.Itoa(i), func(t *testing.T) { - g := NewWithT(t) - g.Expect(conditionsDiff(tt.a, tt.b)).To(Equal(tt.want)) - }) - } -} diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index bae5b0ee3..1146085f4 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -40,6 +40,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" kstatus "github.com/fluxcd/cli-utils/pkg/kstatus/status" "github.com/fluxcd/pkg/apis/meta" @@ -54,6 +55,7 @@ import ( intdigest "github.com/fluxcd/source-controller/internal/digest" "github.com/fluxcd/source-controller/internal/helm/getter" "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" stls "github.com/fluxcd/source-controller/internal/tls" @@ -1522,50 +1524,22 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing. g.Expect(res.Status).To(Equal(kstatus.CurrentStatus)) // Switch to a OCI helm repository type - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "helmrepository-reconcile-", - Namespace: "default", - }, - Data: map[string][]byte{ - "username": []byte(testRegistryUsername), - "password": []byte(testRegistryPassword), - }, - } - g.Expect(testEnv.CreateAndWait(ctx, secret)).To(Succeed()) - obj.Spec.Type = helmv1.HelmRepositoryTypeOCI obj.Spec.URL = fmt.Sprintf("oci://%s", testRegistryServer.registryHost) - obj.Spec.SecretRef = &meta.LocalObjectReference{ - Name: secret.Name, - } oldGen := obj.GetGeneration() g.Expect(testEnv.Update(ctx, obj)).To(Succeed()) newGen := oldGen + 1 - // Wait for HelmRepository to be Ready with new generation. + // Wait for HelmRepository to become static for new generation. g.Eventually(func() bool { if err := testEnv.Get(ctx, key, obj); err != nil { return false } - if !conditions.IsReady(obj) && obj.Status.Artifact != nil { - return false - } - readyCondition := conditions.Get(obj, meta.ReadyCondition) - if readyCondition == nil { - return false - } - return readyCondition.Status == metav1.ConditionTrue && - newGen == readyCondition.ObservedGeneration && - newGen == obj.Status.ObservedGeneration + return newGen == obj.Generation && + !intpredicates.HelmRepositoryOCIRequireMigration(obj) }, timeout).Should(BeTrue()) - // Check if the object status is valid. - condns = &conditionscheck.Conditions{NegativePolarity: helmRepositoryOCINegativeConditions} - checker = conditionscheck.NewChecker(testEnv.Client, condns) - checker.WithT(g).CheckErr(ctx, obj) - g.Expect(testEnv.Delete(ctx, obj)).To(Succeed()) // Wait for HelmRepository to be deleted @@ -1730,3 +1704,90 @@ func TestHelmRepositoryReconciler_InMemoryCaching(t *testing.T) { _, cacheHit := testCache.Get(helmRepo.GetArtifact().Path) g.Expect(cacheHit).To(BeTrue()) } + +func TestHelmRepositoryReconciler_ociMigration(t *testing.T) { + g := NewWithT(t) + + testns, err := testEnv.CreateNamespace(ctx, "hr-oci-migration-test") + g.Expect(err).ToNot(HaveOccurred()) + + t.Cleanup(func() { + g.Expect(testEnv.Cleanup(ctx, testns)).ToNot(HaveOccurred()) + }) + + hr := &helmv1.HelmRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("hr-%s", randStringRunes(5)), + Namespace: testns.Name, + }, + } + hrKey := client.ObjectKeyFromObject(hr) + + // Migrates newly created object with finalizer. + + hr.ObjectMeta.Finalizers = append(hr.ObjectMeta.Finalizers, "foo.bar", sourcev1.SourceFinalizer) + hr.Spec = helmv1.HelmRepositorySpec{ + Type: helmv1.HelmRepositoryTypeOCI, + URL: "oci://foo/bar", + Interval: metav1.Duration{Duration: interval}, + } + g.Expect(testEnv.Create(ctx, hr)).ToNot(HaveOccurred()) + + g.Eventually(func() bool { + _ = testEnv.Get(ctx, hrKey, hr) + return !intpredicates.HelmRepositoryOCIRequireMigration(hr) + }, timeout, time.Second).Should(BeTrue()) + + // Migrates updated object with finalizer. + + patchHelper, err := patch.NewHelper(hr, testEnv.Client) + g.Expect(err).ToNot(HaveOccurred()) + hr.ObjectMeta.Finalizers = append(hr.ObjectMeta.Finalizers, sourcev1.SourceFinalizer) + hr.Spec.URL = "oci://foo/baz" + g.Expect(patchHelper.Patch(ctx, hr)).ToNot(HaveOccurred()) + + g.Eventually(func() bool { + _ = testEnv.Get(ctx, hrKey, hr) + return !intpredicates.HelmRepositoryOCIRequireMigration(hr) + }, timeout, time.Second).Should(BeTrue()) + + // Migrates deleted object with finalizer. + + patchHelper, err = patch.NewHelper(hr, testEnv.Client) + g.Expect(err).ToNot(HaveOccurred()) + + // Suspend the object to prevent finalizer from getting removed. + // Ensure only flux finalizer is set to allow the object to be garbage + // collected at the end. + // NOTE: Suspending and updating finalizers are done separately here as + // doing them in a single patch results in flaky test where the finalizer + // update doesn't gets registered with the kube-apiserver, resulting in + // timeout waiting for finalizer to appear on the object below. + hr.Spec.Suspend = true + g.Expect(patchHelper.Patch(ctx, hr)).ToNot(HaveOccurred()) + g.Eventually(func() bool { + _ = k8sClient.Get(ctx, hrKey, hr) + return hr.Spec.Suspend == true + }, timeout).Should(BeTrue()) + + patchHelper, err = patch.NewHelper(hr, testEnv.Client) + g.Expect(err).ToNot(HaveOccurred()) + + // Add finalizer and verify that finalizer exists on the object using a live + // client. + hr.ObjectMeta.Finalizers = []string{sourcev1.SourceFinalizer} + g.Expect(patchHelper.Patch(ctx, hr)).ToNot(HaveOccurred()) + g.Eventually(func() bool { + _ = k8sClient.Get(ctx, hrKey, hr) + return controllerutil.ContainsFinalizer(hr, sourcev1.SourceFinalizer) + }, timeout).Should(BeTrue()) + + // Delete the object and verify. + g.Expect(testEnv.Delete(ctx, hr)).ToNot(HaveOccurred()) + g.Eventually(func() bool { + if err := testEnv.Get(ctx, hrKey, hr); err != nil { + return apierrors.IsNotFound(err) + } + return false + }, timeout).Should(BeTrue()) +} diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index faa775d8a..c52c8dc30 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -58,7 +58,6 @@ import ( sourcev1 "github.com/fluxcd/source-controller/api/v1" sourcev1beta2 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/fluxcd/source-controller/internal/cache" - "github.com/fluxcd/source-controller/internal/helm/registry" // +kubebuilder:scaffold:imports ) @@ -372,17 +371,6 @@ func TestMain(m *testing.M) { panic(fmt.Sprintf("Failed to start HelmRepositoryReconciler: %v", err)) } - if err = (&HelmRepositoryOCIReconciler{ - Client: testEnv, - EventRecorder: record.NewFakeRecorder(32), - Metrics: testMetricsH, - RegistryClientGenerator: registry.ClientGenerator, - }).SetupWithManagerAndOptions(testEnv, HelmRepositoryReconcilerOptions{ - RateLimiter: controller.GetDefaultRateLimiter(), - }); err != nil { - panic(fmt.Sprintf("Failed to start HelmRepositoryOCIReconciler: %v", err)) - } - if err := (&HelmChartReconciler{ Client: testEnv, EventRecorder: record.NewFakeRecorder(32), diff --git a/internal/predicates/helmrepository_type_predicate.go b/internal/predicates/helmrepository_type_predicate.go index 76694b82f..cc7c8fc7e 100644 --- a/internal/predicates/helmrepository_type_predicate.go +++ b/internal/predicates/helmrepository_type_predicate.go @@ -18,69 +18,69 @@ package predicates import ( "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" ) -// helmRepositoryTypeFilter filters events for a given HelmRepository type. -// It returns true if the event is for a HelmRepository of the given type. -func helmRepositoryTypeFilter(repositoryType string, o client.Object) bool { - if o == nil { - return false - } - - // return true if the object is a HelmRepository - // and the type is the same as the one we are looking for. - hr, ok := o.(*sourcev1.HelmRepository) - if !ok { - return false - } +// HelmRepositoryOCIMigrationPredicate implements predicate functions to allow +// events for HelmRepository OCI that need migration to static object. Non-OCI +// HelmRepositories are always allowed. +type HelmRepositoryOCIMigrationPredicate struct { + predicate.Funcs +} - return hr.Spec.Type == repositoryType +// Create allows events for objects that need migration to static object. +func (HelmRepositoryOCIMigrationPredicate) Create(e event.CreateEvent) bool { + return HelmRepositoryOCIRequireMigration(e.Object) } -// HelmRepositoryTypePredicate is a predicate that filters events for a given HelmRepository type. -type HelmRepositoryTypePredicate struct { - RepositoryType string - predicate.Funcs +// Update allows events for objects that need migration to static object. +func (HelmRepositoryOCIMigrationPredicate) Update(e event.UpdateEvent) bool { + return HelmRepositoryOCIRequireMigration(e.ObjectNew) } -// Create returns true if the Create event is for a HelmRepository of the given type. -func (h HelmRepositoryTypePredicate) Create(e event.CreateEvent) bool { - return helmRepositoryTypeFilter(h.RepositoryType, e.Object) +// Delete allows events for objects that need migration to static object. +func (HelmRepositoryOCIMigrationPredicate) Delete(e event.DeleteEvent) bool { + return HelmRepositoryOCIRequireMigration(e.Object) } -// Update returns true if the Update event is for a HelmRepository of the given type. -func (h HelmRepositoryTypePredicate) Update(e event.UpdateEvent) bool { - if e.ObjectOld == nil || e.ObjectNew == nil { +// HelmRepositoryOCIRequireMigration returns if a given HelmRepository of type +// OCI requires migration to static object. For non-OCI HelmRepository, it +// returns true. +func HelmRepositoryOCIRequireMigration(o client.Object) bool { + if o == nil { return false } - // check if the old object is a HelmRepository - oldObj, ok := e.ObjectOld.(*sourcev1.HelmRepository) + hr, ok := o.(*sourcev1.HelmRepository) if !ok { return false } - // check if the new object is a HelmRepository - newObj, ok := e.ObjectNew.(*sourcev1.HelmRepository) - if !ok { - return false + if hr.Spec.Type != sourcev1.HelmRepositoryTypeOCI { + // Always allow non-OCI HelmRepository. + return true } - isOfRepositoryType := newObj.Spec.Type == h.RepositoryType - wasOfRepositoryType := oldObj.Spec.Type == h.RepositoryType && !isOfRepositoryType - return isOfRepositoryType || wasOfRepositoryType -} + if controllerutil.ContainsFinalizer(hr, sourcev1.SourceFinalizer) || !hasEmptyHelmRepositoryStatus(hr) { + return true + } -// Delete returns true if the Delete event is for a HelmRepository of the given type. -func (h HelmRepositoryTypePredicate) Delete(e event.DeleteEvent) bool { - return helmRepositoryTypeFilter(h.RepositoryType, e.Object) + return false } -// Generic returns true if the Generic event is for a HelmRepository of the given type. -func (h HelmRepositoryTypePredicate) Generic(e event.GenericEvent) bool { - return helmRepositoryTypeFilter(h.RepositoryType, e.Object) +// hasEmptyHelmRepositoryStatus checks if the status of a HelmRepository is +// empty. +func hasEmptyHelmRepositoryStatus(obj *sourcev1.HelmRepository) bool { + if obj.Status.ObservedGeneration == 0 && + obj.Status.Conditions == nil && + obj.Status.URL == "" && + obj.Status.Artifact == nil && + obj.Status.ReconcileRequestStatus.LastHandledReconcileAt == "" { + return true + } + return false } diff --git a/internal/predicates/helmrepository_type_predicate_test.go b/internal/predicates/helmrepository_type_predicate_test.go index e54726892..0d3489d1f 100644 --- a/internal/predicates/helmrepository_type_predicate_test.go +++ b/internal/predicates/helmrepository_type_predicate_test.go @@ -19,109 +19,241 @@ package predicates import ( "testing" - sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" - "github.com/onsi/gomega" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "sigs.k8s.io/controller-runtime/pkg/client" + . "github.com/onsi/gomega" "sigs.k8s.io/controller-runtime/pkg/event" -) -func TestHelmRepositoryTypePredicate_Create(t *testing.T) { - obj := &sourcev1.HelmRepository{Spec: sourcev1.HelmRepositorySpec{}} - http := &sourcev1.HelmRepository{Spec: sourcev1.HelmRepositorySpec{Type: "default"}} - oci := &sourcev1.HelmRepository{Spec: sourcev1.HelmRepositorySpec{Type: "oci"}} - not := &unstructured.Unstructured{} + "github.com/fluxcd/pkg/apis/meta" + "github.com/fluxcd/pkg/runtime/conditions" + v1 "github.com/fluxcd/source-controller/api/v1" + sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" +) + +func TestHelmRepositoryOCIMigrationPredicate_Create(t *testing.T) { tests := []struct { - name string - obj client.Object - want bool + name string + beforeFunc func(o *sourcev1.HelmRepository) + want bool }{ - {name: "new", obj: obj, want: false}, - {name: "http", obj: http, want: true}, - {name: "oci", obj: oci, want: false}, - {name: "not a HelmRepository", obj: not, want: false}, - {name: "nil", obj: nil, want: false}, + { + name: "new oci helm repo no status", + beforeFunc: func(o *sourcev1.HelmRepository) { + o.Spec.Type = sourcev1.HelmRepositoryTypeOCI + }, + want: false, + }, + { + name: "new oci helm repo with default observed gen status", + beforeFunc: func(o *sourcev1.HelmRepository) { + o.Spec.Type = sourcev1.HelmRepositoryTypeOCI + o.Status.ObservedGeneration = -1 + }, + want: true, + }, + { + name: "old oci helm repo with finalizer only", + beforeFunc: func(o *sourcev1.HelmRepository) { + o.Finalizers = []string{sourcev1.SourceFinalizer} + o.Spec.Type = sourcev1.HelmRepositoryTypeOCI + }, + want: true, + }, + { + name: "old oci helm repo with status only", + beforeFunc: func(o *sourcev1.HelmRepository) { + o.Spec.Type = sourcev1.HelmRepositoryTypeOCI + o.Status = sourcev1.HelmRepositoryStatus{ + ObservedGeneration: 3, + } + conditions.MarkTrue(o, meta.ReadyCondition, "foo", "bar") + }, + want: true, + }, + { + name: "old oci helm repo with finalizer and status", + beforeFunc: func(o *sourcev1.HelmRepository) { + o.Finalizers = []string{sourcev1.SourceFinalizer} + o.Spec.Type = sourcev1.HelmRepositoryTypeOCI + o.Status = sourcev1.HelmRepositoryStatus{ + ObservedGeneration: 3, + } + conditions.MarkTrue(o, meta.ReadyCondition, "foo", "bar") + }, + want: true, + }, + { + name: "new default helm repo", + beforeFunc: func(o *sourcev1.HelmRepository) { + o.Spec.Type = sourcev1.HelmRepositoryTypeDefault + }, + want: true, + }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - g := gomega.NewWithT(t) + g := NewWithT(t) - so := HelmRepositoryTypePredicate{RepositoryType: "default"} - e := event.CreateEvent{ - Object: tt.obj, + o := &sourcev1.HelmRepository{} + if tt.beforeFunc != nil { + tt.beforeFunc(o) } - g.Expect(so.Create(e)).To(gomega.Equal(tt.want)) + e := event.CreateEvent{Object: o} + p := HelmRepositoryOCIMigrationPredicate{} + g.Expect(p.Create(e)).To(Equal(tt.want)) }) } } -func TestHelmRepositoryTypePredicate_Update(t *testing.T) { - repoA := &sourcev1.HelmRepository{Spec: sourcev1.HelmRepositorySpec{ - Type: sourcev1.HelmRepositoryTypeDefault, - }} +func TestHelmRepositoryOCIMigrationPredicate_Update(t *testing.T) { + tests := []struct { + name string + beforeFunc func(oldObj, newObj *sourcev1.HelmRepository) + want bool + }{ + { + name: "update oci repo", + beforeFunc: func(oldObj, newObj *sourcev1.HelmRepository) { + oldObj.Spec = sourcev1.HelmRepositorySpec{ + Type: sourcev1.HelmRepositoryTypeOCI, + URL: "oci://foo/bar", + } + *newObj = *oldObj.DeepCopy() + newObj.Spec.URL = "oci://foo/baz" + }, + want: false, + }, + { + name: "migrate old oci repo with status only", + beforeFunc: func(oldObj, newObj *sourcev1.HelmRepository) { + oldObj.Generation = 2 + oldObj.Spec = sourcev1.HelmRepositorySpec{ + Type: sourcev1.HelmRepositoryTypeOCI, + } + oldObj.Status = sourcev1.HelmRepositoryStatus{ + ObservedGeneration: 2, + } + conditions.MarkTrue(oldObj, meta.ReadyCondition, "foo", "bar") - repoB := &sourcev1.HelmRepository{Spec: sourcev1.HelmRepositorySpec{ - Type: sourcev1.HelmRepositoryTypeOCI, - }} + *newObj = *oldObj.DeepCopy() + newObj.Generation = 3 + }, + want: true, + }, + { + name: "migrate old oci repo with finalizer only", + beforeFunc: func(oldObj, newObj *sourcev1.HelmRepository) { + oldObj.Generation = 2 + oldObj.Finalizers = []string{sourcev1.SourceFinalizer} + oldObj.Spec = sourcev1.HelmRepositorySpec{ + Type: sourcev1.HelmRepositoryTypeOCI, + } - empty := &sourcev1.HelmRepository{} - not := &unstructured.Unstructured{} + *newObj = *oldObj.DeepCopy() + newObj.Generation = 3 + }, + want: true, + }, + { + name: "type switch default to oci", + beforeFunc: func(oldObj, newObj *sourcev1.HelmRepository) { + oldObj.Spec = sourcev1.HelmRepositorySpec{ + Type: sourcev1.HelmRepositoryTypeDefault, + } + oldObj.Status = sourcev1.HelmRepositoryStatus{ + Artifact: &v1.Artifact{}, + URL: "http://some-address", + ObservedGeneration: 3, + } - tests := []struct { - name string - old client.Object - new client.Object - want bool - }{ - {name: "diff type", old: repoA, new: repoB, want: true}, - {name: "new with type", old: empty, new: repoA, want: true}, - {name: "old with type", old: repoA, new: empty, want: true}, - {name: "old not a HelmRepository", old: not, new: repoA, want: false}, - {name: "new not a HelmRepository", old: repoA, new: not, want: false}, - {name: "old nil", old: nil, new: repoA, want: false}, - {name: "new nil", old: repoA, new: nil, want: false}, + *newObj = *oldObj.DeepCopy() + newObj.Spec = sourcev1.HelmRepositorySpec{ + Type: sourcev1.HelmRepositoryTypeOCI, + } + }, + want: true, + }, + { + name: "type switch oci to default", + beforeFunc: func(oldObj, newObj *sourcev1.HelmRepository) { + oldObj.Spec = sourcev1.HelmRepositorySpec{ + Type: sourcev1.HelmRepositoryTypeOCI, + } + *newObj = *oldObj.DeepCopy() + newObj.Spec.Type = sourcev1.HelmRepositoryTypeDefault + }, + want: true, + }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - g := gomega.NewWithT(t) + g := NewWithT(t) - so := HelmRepositoryTypePredicate{RepositoryType: "default"} + oldObj := &sourcev1.HelmRepository{} + newObj := oldObj.DeepCopy() + if tt.beforeFunc != nil { + tt.beforeFunc(oldObj, newObj) + } e := event.UpdateEvent{ - ObjectOld: tt.old, - ObjectNew: tt.new, + ObjectOld: oldObj, + ObjectNew: newObj, } - g.Expect(so.Update(e)).To(gomega.Equal(tt.want)) + p := HelmRepositoryOCIMigrationPredicate{} + g.Expect(p.Update(e)).To(Equal(tt.want)) }) } } -func TestHelmRepositoryTypePredicate_Delete(t *testing.T) { - obj := &sourcev1.HelmRepository{Spec: sourcev1.HelmRepositorySpec{}} - http := &sourcev1.HelmRepository{Spec: sourcev1.HelmRepositorySpec{Type: "default"}} - oci := &sourcev1.HelmRepository{Spec: sourcev1.HelmRepositorySpec{Type: "oci"}} - not := &unstructured.Unstructured{} - +func TestHelmRepositoryOCIMigrationPredicate_Delete(t *testing.T) { tests := []struct { - name string - obj client.Object - want bool + name string + beforeFunc func(obj *sourcev1.HelmRepository) + want bool }{ - {name: "new", obj: obj, want: false}, - {name: "http", obj: http, want: true}, - {name: "oci", obj: oci, want: false}, - {name: "not a HelmRepository", obj: not, want: false}, - {name: "nil", obj: nil, want: false}, + { + name: "oci with finalizer", + beforeFunc: func(obj *sourcev1.HelmRepository) { + obj.Finalizers = []string{sourcev1.SourceFinalizer} + obj.Spec.Type = sourcev1.HelmRepositoryTypeOCI + }, + want: true, + }, + { + name: "oci with status", + beforeFunc: func(obj *sourcev1.HelmRepository) { + obj.Spec.Type = sourcev1.HelmRepositoryTypeOCI + obj.Status.ObservedGeneration = 4 + }, + want: true, + }, + { + name: "oci without finalizer or status", + beforeFunc: func(obj *sourcev1.HelmRepository) { + obj.Spec.Type = sourcev1.HelmRepositoryTypeOCI + }, + want: false, + }, + { + name: "default helm repo", + beforeFunc: func(obj *sourcev1.HelmRepository) { + obj.Spec.Type = sourcev1.HelmRepositoryTypeDefault + }, + want: true, + }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - g := gomega.NewWithT(t) + g := NewWithT(t) - so := HelmRepositoryTypePredicate{RepositoryType: "default"} - e := event.DeleteEvent{ - Object: tt.obj, + obj := &sourcev1.HelmRepository{} + if tt.beforeFunc != nil { + tt.beforeFunc(obj) } - g.Expect(so.Delete(e)).To(gomega.Equal(tt.want)) + e := event.DeleteEvent{Object: obj} + p := HelmRepositoryOCIMigrationPredicate{} + g.Expect(p.Delete(e)).To(Equal(tt.want)) }) } } diff --git a/main.go b/main.go index 315ded717..e2bd08efb 100644 --- a/main.go +++ b/main.go @@ -203,19 +203,6 @@ func main() { os.Exit(1) } - if err := (&controller.HelmRepositoryOCIReconciler{ - Client: mgr.GetClient(), - EventRecorder: eventRecorder, - Metrics: metrics, - ControllerName: controllerName, - RegistryClientGenerator: registry.ClientGenerator, - }).SetupWithManagerAndOptions(mgr, controller.HelmRepositoryReconcilerOptions{ - RateLimiter: helper.GetRateLimiter(rateLimiterOptions), - }); err != nil { - setupLog.Error(err, "unable to create controller", "controller", v1beta2.HelmRepositoryKind, "type", "OCI") - os.Exit(1) - } - if err := (&controller.HelmRepositoryReconciler{ Client: mgr.GetClient(), EventRecorder: eventRecorder, From b518499857e72c01bee4cd569fca357ab73c8519 Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 5 Oct 2023 19:46:48 +0000 Subject: [PATCH 2/4] Update HelmRepository OCI spec doc Signed-off-by: Sunny --- docs/spec/v1beta2/helmrepositories.md | 61 +++++++++++++++------------ 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/docs/spec/v1beta2/helmrepositories.md b/docs/spec/v1beta2/helmrepositories.md index ad9e736e0..f9d9542f1 100644 --- a/docs/spec/v1beta2/helmrepositories.md +++ b/docs/spec/v1beta2/helmrepositories.md @@ -5,9 +5,9 @@ There are 2 [Helm repository types](#type) defined by the `HelmRepository` API: - Helm HTTP/S repository, which defines a Source to produce an Artifact for a Helm repository index YAML (`index.yaml`). -- OCI Helm repository, which defines a source that does not produce an Artifact. -Instead a validation of the Helm repository is performed and the outcome is reported in the -`.status.conditions` field. +- OCI Helm repository, which defines a source that does not produce an Artifact. + It's a data container to store the information about the OCI repository that + can be used by [HelmChart](helmcharts.md) to access OCI Helm charts. ## Examples @@ -113,9 +113,11 @@ In the above example: - A HelmRepository named `podinfo` is created, indicated by the `.metadata.name` field. -- The source-controller performs the Helm repository url validation i.e. the url -is a valid OCI registry url, every five minutes with the information indicated by the -`.spec.interval` and `.spec.url` fields. +- A HelmChart that refers to this HelmRepository uses the URL in the `.spec.url` + field to access the OCI Helm chart. + +**NOTE:** The `.spec.interval` field is only used by the `default` Helm +repository and is ignored for any value in `oci` Helm repository. You can run this example by saving the manifest into `helmrepository.yaml`. @@ -129,25 +131,12 @@ You can run this example by saving the manifest into `helmrepository.yaml`. ```console NAME URL AGE READY STATUS - podinfo oci://ghcr.io/stefanprodan/charts 3m22s True Helm repository "podinfo" is ready + podinfo oci://ghcr.io/stefanprodan/charts 3m22s ``` -3. Run `kubectl describe helmrepository podinfo` to see the [Conditions](#conditions) -in the HelmRepository's Status: - - ```console - ... - Status: - Conditions: - Last Transition Time: 2022-05-12T14:02:12Z - Message: Helm repository "podinfo" is ready - Observed Generation: 1 - Reason: Succeeded - Status: True - Type: Ready - Observed Generation: 1 - Events: - ``` +Because the OCI Helm repository is a data container, there's nothing to report +for `READY` and `STATUS` columns above. The existence of the object can be +considered to be ready for use. ## Writing a HelmRepository spec @@ -360,6 +349,9 @@ for more information about setting up GKE Workload Identity. ### Interval +**Note:** This field is ineffectual for [OCI Helm +Repositories](#helm-oci-repository). + `.spec.interval` is a required field that specifies the interval which the Helm repository index must be consulted at. @@ -387,6 +379,9 @@ For Helm repositories which require authentication, see [Secret reference](#secr ### Timeout +**Note:** This field is not applicable to [OCI Helm +Repositories](#helm-oci-repository). + `.spec.timeout` is an optional field to specify a timeout for the fetch operation. The value must be in a [Go recognized duration string format](https://pkg.go.dev/time#ParseDuration), @@ -537,6 +532,9 @@ to HTTP/S Helm repositories. ### Suspend +**Note:** This field is not applicable to [OCI Helm +Repositories](#helm-oci-repository). + `.spec.suspend` is an optional field to suspend the reconciliation of a HelmRepository. When set to `true`, the controller will stop reconciling the HelmRepository, and changes to the resource or the Helm repository index will @@ -547,6 +545,10 @@ For practical information, see [suspending and resuming](#suspending-and-resuming). ## Working with HelmRepositories + +**Note:** This section does not apply to [OCI Helm +Repositories](#helm-oci-repository), being a data container, once created, they +are ready to used by [HelmCharts](helmcharts.md). ### Triggering a reconcile @@ -648,6 +650,10 @@ flux resume source helm ### Debugging a HelmRepository +**Note:** This section does not apply to [OCI Helm +Repositories](#helm-oci-repository), being a data container, they are static +objects that don't require debugging if valid. + There are several ways to gather information about a HelmRepository for debugging purposes. @@ -713,9 +719,11 @@ specific HelmRepository, e.g. `flux logs --level=error --kind=HelmRepository --n ## HelmRepository Status -### Artifact +**Note:** This section does not apply to [OCI Helm +Repositories](#helm-oci-repository), they do not contain any information in the +status. -**Note:** This section does not apply to [OCI Helm Repositories](#helm-oci-repository), they do not emit artifacts. +### Artifact The HelmRepository reports the last fetched repository index as an Artifact object in the `.status.artifact` of the resource. @@ -757,9 +765,6 @@ and reports `Reconciling` and `Stalled` conditions where applicable to provide better (timeout) support to solutions polling the HelmRepository to become `Ready`. - OCI Helm repositories use only `Reconciling`, `Ready`, `FetchFailed`, and `Stalled` - condition types. - #### Reconciling HelmRepository The source-controller marks a HelmRepository as _reconciling_ when one of the following From 3dacb3140edfac24388a7bd8b72f7e6ed3610f32 Mon Sep 17 00:00:00 2001 From: Sunny Date: Tue, 17 Oct 2023 21:04:07 +0000 Subject: [PATCH 3/4] optional interval and default timeout for helmrepo With static HelmRepository OCI, the interval become optional. Make interval optional in the API. Introduce getters for interval, in the form of GetRequeueAfter(), and timeout with internal default values. HelmRepository will not have interval and timeout fields unless it's explicitly set. Signed-off-by: Sunny --- api/v1beta2/helmrepository_types.go | 22 ++++++++++++++----- ...ce.toolkit.fluxcd.io_helmrepositories.yaml | 5 ++--- docs/api/v1beta2/source.md | 8 +++++-- docs/spec/v1beta2/helmrepositories.md | 9 ++++---- internal/controller/helmchart_controller.go | 4 ++-- internal/helm/getter/client_opts.go | 2 +- 6 files changed, 33 insertions(+), 17 deletions(-) diff --git a/api/v1beta2/helmrepository_types.go b/api/v1beta2/helmrepository_types.go index a73a445b8..1c25a9ebe 100644 --- a/api/v1beta2/helmrepository_types.go +++ b/api/v1beta2/helmrepository_types.go @@ -89,13 +89,13 @@ type HelmRepositorySpec struct { // efficient use of resources. // +kubebuilder:validation:Type=string // +kubebuilder:validation:Pattern="^([0-9]+(\\.[0-9]+)?(ms|s|m|h))+$" - // +required - Interval metav1.Duration `json:"interval"` + // +optional + Interval metav1.Duration `json:"interval,omitempty"` // Timeout is used for the index fetch operation for an HTTPS helm repository, - // and for remote OCI Repository operations like pulling for an OCI helm repository. + // and for remote OCI Repository operations like pulling for an OCI helm + // chart by the associated HelmChart. // Its default value is 60s. - // +kubebuilder:default:="60s" // +kubebuilder:validation:Type=string // +kubebuilder:validation:Pattern="^([0-9]+(\\.[0-9]+)?(ms|s|m))+$" // +optional @@ -170,7 +170,19 @@ func (in *HelmRepository) SetConditions(conditions []metav1.Condition) { // GetRequeueAfter returns the duration after which the source must be // reconciled again. func (in HelmRepository) GetRequeueAfter() time.Duration { - return in.Spec.Interval.Duration + if in.Spec.Interval.Duration != 0 { + return in.Spec.Interval.Duration + } + return time.Minute +} + +// GetTimeout returns the timeout duration used for various operations related +// to this HelmRepository. +func (in HelmRepository) GetTimeout() time.Duration { + if in.Spec.Timeout != nil { + return in.Spec.Timeout.Duration + } + return time.Minute } // GetArtifact returns the latest artifact from the source if present in the diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml index 5d1572873..a17ab56d9 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml @@ -357,10 +357,10 @@ spec: of this HelmRepository. type: boolean timeout: - default: 60s description: Timeout is used for the index fetch operation for an HTTPS helm repository, and for remote OCI Repository operations - like pulling for an OCI helm repository. Its default value is 60s. + like pulling for an OCI helm chart by the associated HelmChart. + Its default value is 60s. pattern: ^([0-9]+(\.[0-9]+)?(ms|s|m))+$ type: string type: @@ -376,7 +376,6 @@ spec: pattern: ^(http|https|oci)://.*$ type: string required: - - interval - url type: object status: diff --git a/docs/api/v1beta2/source.md b/docs/api/v1beta2/source.md index edfa29a5b..46649e233 100644 --- a/docs/api/v1beta2/source.md +++ b/docs/api/v1beta2/source.md @@ -866,6 +866,7 @@ Kubernetes meta/v1.Duration +(Optional)

Interval at which the HelmRepository URL is checked for updates. This interval is approximate and may be subject to jitter to ensure efficient use of resources.

@@ -883,7 +884,8 @@ Kubernetes meta/v1.Duration (Optional)

Timeout is used for the index fetch operation for an HTTPS helm repository, -and for remote OCI Repository operations like pulling for an OCI helm repository. +and for remote OCI Repository operations like pulling for an OCI helm +chart by the associated HelmChart. Its default value is 60s.

@@ -2583,6 +2585,7 @@ Kubernetes meta/v1.Duration +(Optional)

Interval at which the HelmRepository URL is checked for updates. This interval is approximate and may be subject to jitter to ensure efficient use of resources.

@@ -2600,7 +2603,8 @@ Kubernetes meta/v1.Duration (Optional)

Timeout is used for the index fetch operation for an HTTPS helm repository, -and for remote OCI Repository operations like pulling for an OCI helm repository. +and for remote OCI Repository operations like pulling for an OCI helm +chart by the associated HelmChart. Its default value is 60s.

diff --git a/docs/spec/v1beta2/helmrepositories.md b/docs/spec/v1beta2/helmrepositories.md index f9d9542f1..a05155ebb 100644 --- a/docs/spec/v1beta2/helmrepositories.md +++ b/docs/spec/v1beta2/helmrepositories.md @@ -352,8 +352,9 @@ for more information about setting up GKE Workload Identity. **Note:** This field is ineffectual for [OCI Helm Repositories](#helm-oci-repository). -`.spec.interval` is a required field that specifies the interval which the -Helm repository index must be consulted at. +`.spec.interval` is a an optional field that specifies the interval which the +Helm repository index must be consulted at. When not set, the default value is +`1m`. After successfully reconciling a HelmRepository object, the source-controller requeues the object for inspection after the specified interval. The value @@ -385,8 +386,8 @@ Repositories](#helm-oci-repository). `.spec.timeout` is an optional field to specify a timeout for the fetch operation. The value must be in a [Go recognized duration string format](https://pkg.go.dev/time#ParseDuration), -e.g. `1m30s` for a timeout of one minute and thirty seconds. The default value -is `60s`. +e.g. `1m30s` for a timeout of one minute and thirty seconds. When not set, the +default value is `1m`. ### Secret reference diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index 52b4b0edb..41b90194c 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -514,7 +514,7 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, sp *patch.Ser func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *helmv1.HelmChart, repo *helmv1.HelmRepository, b *chart.Build) (sreconcile.Result, error) { // Used to login with the repository declared provider - ctxTimeout, cancel := context.WithTimeout(ctx, repo.Spec.Timeout.Duration) + ctxTimeout, cancel := context.WithTimeout(ctx, repo.GetTimeout()) defer cancel() normalizedURL, err := repository.NormalizeURL(repo.Spec.URL) @@ -999,7 +999,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont } // Used to login with the repository declared provider - ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) + ctxTimeout, cancel := context.WithTimeout(ctx, obj.GetTimeout()) defer cancel() clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL) diff --git a/internal/helm/getter/client_opts.go b/internal/helm/getter/client_opts.go index ce7620ca1..4dfc97b40 100644 --- a/internal/helm/getter/client_opts.go +++ b/internal/helm/getter/client_opts.go @@ -72,7 +72,7 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit hrOpts := &ClientOpts{ GetterOpts: []helmgetter.Option{ helmgetter.WithURL(url), - helmgetter.WithTimeout(obj.Spec.Timeout.Duration), + helmgetter.WithTimeout(obj.GetTimeout()), helmgetter.WithPassCredentialsAll(obj.Spec.PassCredentials), }, } From 1a7adeb8011eec6e03099a37d96d169aae55a6e7 Mon Sep 17 00:00:00 2001 From: Sunny Date: Wed, 18 Oct 2023 03:32:45 +0000 Subject: [PATCH 4/4] Fix helmrepo reconciler unfinished worker bug Although all the APIs had interval as a required field, when tests objects were created, they had the zero value of interval, which the API server accepts. A zero interval value results in the test objects to reconcile only once when they are created and never reconcile again unless there's an update to the object. Most of the tests worked with this behavior. With HelmRepository removing the interval requirement and adding an internal default, all the HelmRepository objects created in the tests without any interval have a default interval value which results in objects to reconcile automatically if they are not cleaned up after running tests. TestHelmRepositoryReconciler_InMemoryCaching and TestHelmChartReconciler_Reconcile create HelmRepository but doesn't delete it at the end. This leads to a reconciliation of HelmRepository outside of the test in the envtest environment. It just happened to be that the reconciliation time matches with the end of test time. At the end of the test run, the reconcilers receive shutdown signal and any test server, like helmrepository server, are stopped. A HelmRepository reconciliation triggered just before the shutdown signal gets stuck in the reconciliation. HelmRepository can't download the index as the test index server has stopped and hangs for some time. The HelmRepository reconciler worker remains in active state, unlike other reconciler workers that shut down, resulting in the test to timeout at the end. The is fixed by deleting the HelmRepository object created in TestHelmRepositoryReconciler_InMemoryCaching and TestHelmChartReconciler_Reconcile at the end of the test similar to other tests. Signed-off-by: Sunny --- internal/controller/helmchart_controller_test.go | 1 + internal/controller/helmrepository_controller_test.go | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index b9472419a..e8d4f64ad 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/internal/controller/helmchart_controller_test.go @@ -293,6 +293,7 @@ func TestHelmChartReconciler_Reconcile(t *testing.T) { } g.Expect(testEnv.CreateAndWait(ctx, &repository)).To(Succeed()) + defer func() { g.Expect(testEnv.Delete(ctx, &repository)).To(Succeed()) }() obj := helmv1.HelmChart{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index 1146085f4..0da154a15 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -1703,6 +1703,16 @@ func TestHelmRepositoryReconciler_InMemoryCaching(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) _, cacheHit := testCache.Get(helmRepo.GetArtifact().Path) g.Expect(cacheHit).To(BeTrue()) + + g.Expect(testEnv.Delete(ctx, helmRepo)).To(Succeed()) + + // Wait for HelmRepository to be deleted + g.Eventually(func() bool { + if err := testEnv.Get(ctx, key, helmRepo); err != nil { + return apierrors.IsNotFound(err) + } + return false + }, timeout).Should(BeTrue()) } func TestHelmRepositoryReconciler_ociMigration(t *testing.T) {