From 9ce7a3c44f409d365d4156bc16db8445b3ba782d Mon Sep 17 00:00:00 2001 From: Bartosz Majsak Date: Mon, 16 Sep 2024 11:32:23 +0200 Subject: [PATCH] refactor(feature): decouples Feature from client (#1234) Including the client in the function signature makes its usage explicit and more idiomatic. This approach clarifies that the client is intended for use within the function. Decoupling the client from Feature struct enhances code readability. It's a follow-up to PR [#1228](https://github.com/opendatahub-io/opendatahub-operator/pull/1228#issuecomment-2343046440). (cherry picked from commit fde5d697de4304941347a4f24d287d60b3f2a52c) --- components/kserve/kserve_config_handler.go | 8 +-- components/kserve/servicemesh_setup.go | 8 +-- .../dscinitialization/servicemesh_setup.go | 15 ++--- pkg/feature/builder.go | 61 +------------------ pkg/feature/conditions.go | 12 ++-- pkg/feature/feature.go | 37 ++++++----- pkg/feature/feature_data.go | 6 +- pkg/feature/feature_tracker_handler.go | 16 ++--- pkg/feature/handler.go | 29 ++++----- pkg/feature/resources.go | 6 +- pkg/feature/serverless/conditions.go | 8 +-- pkg/feature/serverless/resources.go | 8 ++- pkg/feature/servicemesh/conditions.go | 18 +++--- pkg/feature/servicemesh/resources.go | 9 +-- .../integration/features/cleanup_int_test.go | 17 +++--- .../fixtures/cluster_test_fixtures.go | 6 +- .../features/manifests_int_test.go | 12 ++-- .../features/preconditions_int_test.go | 8 +-- .../features/resources_int_test.go | 24 ++++---- .../features/serverless_feature_test.go | 35 ++++------- .../features/servicemesh_feature_test.go | 22 +++---- .../integration/features/tracker_int_test.go | 31 +++++----- 22 files changed, 164 insertions(+), 232 deletions(-) diff --git a/components/kserve/kserve_config_handler.go b/components/kserve/kserve_config_handler.go index 8479e207053..e43e7313b2f 100644 --- a/components/kserve/kserve_config_handler.go +++ b/components/kserve/kserve_config_handler.go @@ -148,9 +148,9 @@ func (k *Kserve) configureServerless(ctx context.Context, cli client.Client, log return dependOpsErrors } - serverlessFeatures := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance)) + serverlessFeatures := feature.ComponentFeaturesHandler(owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance)) - if err := serverlessFeatures.Apply(ctx); err != nil { + if err := serverlessFeatures.Apply(ctx, cli); err != nil { return err } } @@ -158,9 +158,9 @@ func (k *Kserve) configureServerless(ctx context.Context, cli client.Client, log } func (k *Kserve) removeServerlessFeatures(ctx context.Context, cli client.Client, owner metav1.Object, instance *dsciv1.DSCInitializationSpec) error { - serverlessFeatures := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance)) + serverlessFeatures := feature.ComponentFeaturesHandler(owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance)) - return serverlessFeatures.Delete(ctx) + return serverlessFeatures.Delete(ctx, cli) } func checkDependentOperators(ctx context.Context, cli client.Client) *multierror.Error { diff --git a/components/kserve/servicemesh_setup.go b/components/kserve/servicemesh_setup.go index 23e3c5321bb..126e23d88ea 100644 --- a/components/kserve/servicemesh_setup.go +++ b/components/kserve/servicemesh_setup.go @@ -20,8 +20,8 @@ import ( func (k *Kserve) configureServiceMesh(ctx context.Context, cli client.Client, owner metav1.Object, dscispec *dsciv1.DSCInitializationSpec) error { if dscispec.ServiceMesh != nil { if dscispec.ServiceMesh.ManagementState == operatorv1.Managed && k.GetManagementState() == operatorv1.Managed { - serviceMeshInitializer := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec)) - return serviceMeshInitializer.Apply(ctx) + serviceMeshInitializer := feature.ComponentFeaturesHandler(owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec)) + return serviceMeshInitializer.Apply(ctx, cli) } if dscispec.ServiceMesh.ManagementState == operatorv1.Unmanaged && k.GetManagementState() == operatorv1.Managed { return nil @@ -32,8 +32,8 @@ func (k *Kserve) configureServiceMesh(ctx context.Context, cli client.Client, ow } func (k *Kserve) removeServiceMeshConfigurations(ctx context.Context, cli client.Client, owner metav1.Object, dscispec *dsciv1.DSCInitializationSpec) error { - serviceMeshInitializer := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec)) - return serviceMeshInitializer.Delete(ctx) + serviceMeshInitializer := feature.ComponentFeaturesHandler(owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec)) + return serviceMeshInitializer.Delete(ctx, cli) } func (k *Kserve) defineServiceMeshFeatures(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) feature.FeaturesProvider { diff --git a/controllers/dscinitialization/servicemesh_setup.go b/controllers/dscinitialization/servicemesh_setup.go index 7b5e6bc0977..4e45b96a0ca 100644 --- a/controllers/dscinitialization/servicemesh_setup.go +++ b/controllers/dscinitialization/servicemesh_setup.go @@ -8,6 +8,7 @@ import ( operatorv1 "github.com/openshift/api/operator/v1" conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1" corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" @@ -39,7 +40,7 @@ func (r *DSCInitializationReconciler) configureServiceMesh(ctx context.Context, capabilities = append(capabilities, authzCapability) for _, capability := range capabilities { - capabilityErr := capability.Apply(ctx) + capabilityErr := capability.Apply(ctx, r.Client) if capabilityErr != nil { r.Log.Error(capabilityErr, "failed applying service mesh resources") r.Recorder.Eventf(instance, corev1.EventTypeWarning, "DSCInitializationReconcileError", "failed applying service mesh resources") @@ -77,7 +78,7 @@ func (r *DSCInitializationReconciler) removeServiceMesh(ctx context.Context, ins capabilities = append(capabilities, authzCapability) for _, capability := range capabilities { - capabilityErr := capability.Delete(ctx) + capabilityErr := capability.Delete(ctx, r.Client) if capabilityErr != nil { r.Log.Error(capabilityErr, "failed deleting service mesh resources") r.Recorder.Eventf(instance, corev1.EventTypeWarning, "DSCInitializationReconcileError", "failed deleting service mesh resources") @@ -91,7 +92,7 @@ func (r *DSCInitializationReconciler) removeServiceMesh(ctx context.Context, ins func (r *DSCInitializationReconciler) serviceMeshCapability(instance *dsciv1.DSCInitialization, initialCondition *conditionsv1.Condition) *feature.HandlerWithReporter[*dsciv1.DSCInitialization] { //nolint:lll // Reason: generics are long return feature.NewHandlerWithReporter( - feature.ClusterFeaturesHandler(r.Client, instance, r.serviceMeshCapabilityFeatures(instance)), + feature.ClusterFeaturesHandler(instance, r.serviceMeshCapabilityFeatures(instance)), createCapabilityReporter(r.Client, instance, initialCondition), ) } @@ -119,7 +120,7 @@ func (r *DSCInitializationReconciler) authorizationCapability(ctx context.Contex } return feature.NewHandlerWithReporter( - feature.ClusterFeaturesHandler(r.Client, instance, r.authorizationFeatures(instance)), + feature.ClusterFeaturesHandler(instance, r.authorizationFeatures(instance)), createCapabilityReporter(r.Client, instance, condition), ), nil } @@ -128,7 +129,7 @@ func (r *DSCInitializationReconciler) serviceMeshCapabilityFeatures(instance *ds return func(registry feature.FeaturesRegistry) error { controlPlaneSpec := instance.Spec.ServiceMesh.ControlPlane - meshMetricsCollection := func(_ context.Context, _ *feature.Feature) (bool, error) { + meshMetricsCollection := func(_ context.Context, _ client.Client, _ *feature.Feature) (bool, error) { return controlPlaneSpec.MetricsCollection == "Istio", nil } @@ -221,13 +222,13 @@ func (r *DSCInitializationReconciler) authorizationFeatures(instance *dsciv1.DSC Include(path.Join(Templates.AuthorinoDir, "deployment.injection.patch.tmpl.yaml")), ). PreConditions( - func(ctx context.Context, f *feature.Feature) error { + func(ctx context.Context, cli client.Client, f *feature.Feature) error { namespace, err := servicemesh.FeatureData.Authorization.Namespace.Extract(f) if err != nil { return fmt.Errorf("failed trying to resolve authorization provider namespace for feature '%s': %w", f.Name, err) } - return feature.WaitForPodsToBeReady(namespace)(ctx, f) + return feature.WaitForPodsToBeReady(namespace)(ctx, cli, f) }, ). WithData(servicemesh.FeatureData.ControlPlane.Define(&instance.Spec).AsAction()). diff --git a/pkg/feature/builder.go b/pkg/feature/builder.go index 9cb53c58a70..54fadcabbec 100644 --- a/pkg/feature/builder.go +++ b/pkg/feature/builder.go @@ -4,14 +4,8 @@ import ( "context" "fmt" - "github.com/hashicorp/go-multierror" - "github.com/pkg/errors" - apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/rest" - "k8s.io/client-go/tools/clientcmd" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/config" "sigs.k8s.io/controller-runtime/pkg/log" featurev1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" @@ -27,8 +21,6 @@ type featureBuilder struct { owner metav1.Object targetNs string - client client.Client - builders []partialBuilder } @@ -193,16 +185,10 @@ func (fb *featureBuilder) OnDelete(cleanups ...CleanupFunc) *featureBuilder { return fb } -// UsingClient allows to provide a custom client to the feature. If not called, a default client will be created. -func (fb *featureBuilder) UsingClient(cli client.Client) *featureBuilder { - fb.client = cli - return fb -} - // Create creates a new Feature instance and add it to corresponding FeaturesHandler. // The actual feature creation in the cluster is not performed here. func (fb *featureBuilder) Create() (*Feature, error) { - alwaysEnabled := func(_ context.Context, _ *Feature) (bool, error) { + alwaysEnabled := func(_ context.Context, _ client.Client, _ *Feature) (bool, error) { return true, nil } @@ -215,15 +201,6 @@ func (fb *featureBuilder) Create() (*Feature, error) { owner: fb.owner, } - // UsingClient has not been called, so we need to create a new client - if fb.client == nil { - if err := createDefaultClient()(f); err != nil { - return nil, err - } - } else { - f.Client = fb.client - } - for i := range fb.builders { if err := fb.builders[i](f); err != nil { return nil, err @@ -232,39 +209,3 @@ func (fb *featureBuilder) Create() (*Feature, error) { return f, nil } - -func createDefaultClient() partialBuilder { - return func(f *Feature) error { - var err error - - restCfg, err := config.GetConfig() - if errors.Is(err, rest.ErrNotInCluster) { - // rollback to local kubeconfig - this can be helpful when running the process locally i.e. while debugging - kubeconfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig( - &clientcmd.ClientConfigLoadingRules{ExplicitPath: clientcmd.RecommendedHomeFile}, - &clientcmd.ConfigOverrides{}, - ) - - restCfg, err = kubeconfig.ClientConfig() - if err != nil { - return err - } - } else if err != nil { - return err - } - - f.Client, err = client.New(restCfg, client.Options{}) - if err != nil { - return errors.WithStack(err) - } - - var multiErr *multierror.Error - s := f.Client.Scheme() - multiErr = multierror.Append(multiErr, - featurev1.AddToScheme(s), - apiextv1.AddToScheme(s), - ) - - return multiErr.ErrorOrNil() - } -} diff --git a/pkg/feature/conditions.go b/pkg/feature/conditions.go index be86b4c0fb4..9a9373ba130 100644 --- a/pkg/feature/conditions.go +++ b/pkg/feature/conditions.go @@ -40,8 +40,8 @@ func (e *MissingOperatorError) Error() string { } func EnsureOperatorIsInstalled(operatorName string) Action { - return func(ctx context.Context, f *Feature) error { - if found, err := cluster.SubscriptionExists(ctx, f.Client, operatorName); !found || err != nil { + return func(ctx context.Context, cli client.Client, f *Feature) error { + if found, err := cluster.SubscriptionExists(ctx, cli, operatorName); !found || err != nil { return fmt.Errorf( "failed to find the pre-requisite operator subscription %q, please ensure operator is installed. %w", operatorName, @@ -53,13 +53,13 @@ func EnsureOperatorIsInstalled(operatorName string) Action { } func WaitForPodsToBeReady(namespace string) Action { - return func(ctx context.Context, f *Feature) error { + return func(ctx context.Context, cli client.Client, f *Feature) error { f.Log.Info("waiting for pods to become ready", "namespace", namespace, "duration (s)", duration.Seconds()) return wait.PollUntilContextTimeout(ctx, interval, duration, false, func(ctx context.Context) (bool, error) { var podList corev1.PodList - err := f.Client.List(ctx, &podList, client.InNamespace(namespace)) + err := cli.List(ctx, &podList, client.InNamespace(namespace)) if err != nil { return false, err } @@ -103,14 +103,14 @@ func WaitForPodsToBeReady(namespace string) Action { } func WaitForResourceToBeCreated(namespace string, gvk schema.GroupVersionKind) Action { - return func(ctx context.Context, f *Feature) error { + return func(ctx context.Context, cli client.Client, f *Feature) error { f.Log.Info("waiting for resource to be created", "namespace", namespace, "resource", gvk) return wait.PollUntilContextTimeout(ctx, interval, duration, false, func(ctx context.Context) (bool, error) { list := &unstructured.UnstructuredList{} list.SetGroupVersionKind(gvk) - err := f.Client.List(ctx, list, client.InNamespace(namespace), client.Limit(1)) + err := cli.List(ctx, list, client.InNamespace(namespace), client.Limit(1)) if err != nil { f.Log.Error(err, "failed waiting for resource", "namespace", namespace, "resource", gvk) diff --git a/pkg/feature/feature.go b/pkg/feature/feature.go index 3a16a2b9a40..33e569a0fd5 100644 --- a/pkg/feature/feature.go +++ b/pkg/feature/feature.go @@ -41,8 +41,7 @@ type Feature struct { Enabled EnabledFunc Managed bool - Client client.Client - Log logr.Logger + Log logr.Logger tracker *featurev1.FeatureTracker source *featurev1.Source @@ -61,7 +60,7 @@ type Feature struct { // Action is a func type which can be used for different purposes during Feature's lifecycle // while having access to Feature struct. -type Action func(ctx context.Context, f *Feature) error +type Action func(ctx context.Context, cli client.Client, f *Feature) error // CleanupFunc defines how to clean up resources associated with a feature. // By default, all resources created by the feature are deleted when the feature is, @@ -70,70 +69,70 @@ type Action func(ctx context.Context, f *Feature) error type CleanupFunc func(ctx context.Context, cli client.Client) error // EnabledFunc is a func type used to determine if a feature should be enabled. -type EnabledFunc func(ctx context.Context, feature *Feature) (bool, error) +type EnabledFunc func(ctx context.Context, cli client.Client, feature *Feature) (bool, error) // Apply applies the feature to the cluster. // It creates a FeatureTracker resource to establish ownership and reports the result of the operation as a condition. -func (f *Feature) Apply(ctx context.Context) error { +func (f *Feature) Apply(ctx context.Context, cli client.Client) error { // If the feature is disabled, but the FeatureTracker exists in the cluster, ensure clean-up is triggered. // This means that the feature was previously enabled, but now it is not anymore. - if enabled, err := f.Enabled(ctx, f); !enabled || err != nil { + if enabled, err := f.Enabled(ctx, cli, f); !enabled || err != nil { if err != nil { return err } - return f.Cleanup(ctx) + return f.Cleanup(ctx, cli) } - if trackerErr := createFeatureTracker(ctx, f); trackerErr != nil { + if trackerErr := createFeatureTracker(ctx, cli, f); trackerErr != nil { return trackerErr } - if _, updateErr := status.UpdateWithRetry(ctx, f.Client, f.tracker, func(saved *featurev1.FeatureTracker) { + if _, updateErr := status.UpdateWithRetry(ctx, cli, f.tracker, func(saved *featurev1.FeatureTracker) { status.SetProgressingCondition(&saved.Status.Conditions, string(featurev1.ConditionReason.FeatureCreated), fmt.Sprintf("Applying feature [%s]", f.Name)) saved.Status.Phase = status.PhaseProgressing }); updateErr != nil { return updateErr } - applyErr := f.applyFeature(ctx) - _, reportErr := createFeatureTrackerStatusReporter(f).ReportCondition(ctx, applyErr) + applyErr := f.applyFeature(ctx, cli) + _, reportErr := createFeatureTrackerStatusReporter(cli, f).ReportCondition(ctx, applyErr) return multierror.Append(applyErr, reportErr).ErrorOrNil() } -func (f *Feature) applyFeature(ctx context.Context) error { +func (f *Feature) applyFeature(ctx context.Context, cli client.Client) error { var multiErr *multierror.Error for _, dataProvider := range f.dataProviders { - multiErr = multierror.Append(multiErr, dataProvider(ctx, f)) + multiErr = multierror.Append(multiErr, dataProvider(ctx, cli, f)) } if errDataLoad := multiErr.ErrorOrNil(); errDataLoad != nil { return &withConditionReasonError{reason: featurev1.ConditionReason.LoadTemplateData, err: errDataLoad} } for _, precondition := range f.preconditions { - multiErr = multierror.Append(multiErr, precondition(ctx, f)) + multiErr = multierror.Append(multiErr, precondition(ctx, cli, f)) } if preconditionsErr := multiErr.ErrorOrNil(); preconditionsErr != nil { return &withConditionReasonError{reason: featurev1.ConditionReason.PreConditions, err: preconditionsErr} } for _, clusterOperation := range f.clusterOperations { - if errClusterOperation := clusterOperation(ctx, f); errClusterOperation != nil { + if errClusterOperation := clusterOperation(ctx, cli, f); errClusterOperation != nil { return &withConditionReasonError{reason: featurev1.ConditionReason.ResourceCreation, err: errClusterOperation} } } for i := range f.appliers { r := f.appliers[i] - if processErr := r.Apply(ctx, f.Client, f.data, DefaultMetaOptions(f)...); processErr != nil { + if processErr := r.Apply(ctx, cli, f.data, DefaultMetaOptions(f)...); processErr != nil { return &withConditionReasonError{reason: featurev1.ConditionReason.ApplyManifests, err: processErr} } } for _, postcondition := range f.postconditions { - multiErr = multierror.Append(multiErr, postcondition(ctx, f)) + multiErr = multierror.Append(multiErr, postcondition(ctx, cli, f)) } if postConditionErr := multiErr.ErrorOrNil(); postConditionErr != nil { return &withConditionReasonError{reason: featurev1.ConditionReason.PostConditions, err: postConditionErr} @@ -142,14 +141,14 @@ func (f *Feature) applyFeature(ctx context.Context) error { return nil } -func (f *Feature) Cleanup(ctx context.Context) error { +func (f *Feature) Cleanup(ctx context.Context, cli client.Client) error { // Ensure associated FeatureTracker instance has been removed as last one // in the chain of cleanups. f.addCleanup(removeFeatureTracker(f)) var cleanupErrors *multierror.Error for _, cleanupFunc := range f.cleanups { - cleanupErrors = multierror.Append(cleanupErrors, cleanupFunc(ctx, f.Client)) + cleanupErrors = multierror.Append(cleanupErrors, cleanupFunc(ctx, cli)) } return cleanupErrors.ErrorOrNil() diff --git a/pkg/feature/feature_data.go b/pkg/feature/feature_data.go index 0cfe5ee1f46..82c8076fe4d 100644 --- a/pkg/feature/feature_data.go +++ b/pkg/feature/feature_data.go @@ -4,6 +4,8 @@ import ( "context" "fmt" + "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/provider" ) @@ -13,8 +15,8 @@ import ( // // If the value is static, consider using provider.ValueOf(variable).Get as passed provider function. func Entry[T any](key string, providerFunc provider.DataProviderFunc[T]) Action { - return func(ctx context.Context, f *Feature) error { - data, err := providerFunc(ctx, f.Client) + return func(ctx context.Context, cli client.Client, f *Feature) error { + data, err := providerFunc(ctx, cli) if err != nil { return err } diff --git a/pkg/feature/feature_tracker_handler.go b/pkg/feature/feature_tracker_handler.go index 6ca1f682941..6ba728a11d6 100644 --- a/pkg/feature/feature_tracker_handler.go +++ b/pkg/feature/feature_tracker_handler.go @@ -30,8 +30,8 @@ func (e *withConditionReasonError) Error() string { // createFeatureTracker creates a FeatureTracker, persists it in the cluster, // and attaches it to the provided Feature instance. -func createFeatureTracker(ctx context.Context, f *Feature) error { - tracker, errGet := getFeatureTracker(ctx, f.Client, f.Name, f.TargetNamespace) +func createFeatureTracker(ctx context.Context, cli client.Client, f *Feature) error { + tracker, errGet := getFeatureTracker(ctx, cli, f.Name, f.TargetNamespace) if client.IgnoreNotFound(errGet) != nil { return errGet } @@ -43,18 +43,18 @@ func createFeatureTracker(ctx context.Context, f *Feature) error { AppNamespace: f.TargetNamespace, } if f.owner != nil { - ownerRef := cluster.OwnedBy(f.owner, f.Client.Scheme()) + ownerRef := cluster.OwnedBy(f.owner, cli.Scheme()) if errMetaOpts := cluster.ApplyMetaOptions(tracker, ownerRef); errMetaOpts != nil { return fmt.Errorf("failed adding owner to FeatureTracker %s: %w", tracker.Name, errMetaOpts) } } - if errCreate := f.Client.Create(ctx, tracker); errCreate != nil { + if errCreate := cli.Create(ctx, tracker); errCreate != nil { return fmt.Errorf("failed creating FeatureTracker %s: %w", tracker.Name, errCreate) } } - if errGVK := ensureGVKSet(tracker, f.Client.Scheme()); errGVK != nil { + if errGVK := ensureGVKSet(tracker, cli.Scheme()); errGVK != nil { return fmt.Errorf("failed ensuring GVK is set for %s: %w", tracker.Name, errGVK) } @@ -77,7 +77,7 @@ func removeFeatureTracker(f *Feature) CleanupFunc { } if associatedTracker != nil { - return client.IgnoreNotFound(f.Client.Delete(ctx, associatedTracker)) + return client.IgnoreNotFound(cli.Delete(ctx, associatedTracker)) } return nil @@ -109,8 +109,8 @@ func ensureGVKSet(obj runtime.Object, scheme *runtime.Scheme) error { return nil } -func createFeatureTrackerStatusReporter(f *Feature) *status.Reporter[*featurev1.FeatureTracker] { - return status.NewStatusReporter(f.Client, f.tracker, func(err error) status.SaveStatusFunc[*featurev1.FeatureTracker] { +func createFeatureTrackerStatusReporter(cli client.Client, f *Feature) *status.Reporter[*featurev1.FeatureTracker] { + return status.NewStatusReporter(cli, f.tracker, func(err error) status.SaveStatusFunc[*featurev1.FeatureTracker] { updatedCondition := func(saved *featurev1.FeatureTracker) { status.SetCompleteCondition(&saved.Status.Conditions, string(featurev1.ConditionReason.FeatureCreated), fmt.Sprintf("Applied feature [%s] successfully", f.Name)) saved.Status.Phase = status.PhaseReady diff --git a/pkg/feature/handler.go b/pkg/feature/handler.go index 834234adee0..890a51a1fcc 100644 --- a/pkg/feature/handler.go +++ b/pkg/feature/handler.go @@ -15,8 +15,8 @@ import ( ) type featuresHandler interface { - Apply(ctx context.Context) error - Delete(ctx context.Context) error + Apply(ctx context.Context, cli client.Client) error + Delete(ctx context.Context, cli client.Client) error } type FeaturesRegistry interface { @@ -28,7 +28,6 @@ var _ featuresHandler = (*FeaturesHandler)(nil) // FeaturesHandler provides a structured way to manage and coordinate the creation, application, // and deletion of features needed in particular Data Science Cluster configuration. type FeaturesHandler struct { - cli client.Client source featurev1.Source owner metav1.Object targetNamespace string @@ -45,7 +44,7 @@ func (fh *FeaturesHandler) Add(builders ...*featureBuilder) error { for i := range builders { fb := builders[i] - feature, err := fb.UsingClient(fh.cli). + feature, err := fb. TargetNamespace(fh.targetNamespace). OwnedBy(fh.owner). Source(fh.source). @@ -57,7 +56,7 @@ func (fh *FeaturesHandler) Add(builders ...*featureBuilder) error { return multiErr.ErrorOrNil() } -func (fh *FeaturesHandler) Apply(ctx context.Context) error { +func (fh *FeaturesHandler) Apply(ctx context.Context, cli client.Client) error { fh.features = make([]*Feature, 0) for _, featuresProvider := range fh.featuresProviders { @@ -68,7 +67,7 @@ func (fh *FeaturesHandler) Apply(ctx context.Context) error { var multiErr *multierror.Error for _, f := range fh.features { - if applyErr := f.Apply(ctx); applyErr != nil { + if applyErr := f.Apply(ctx, cli); applyErr != nil { multiErr = multierror.Append(multiErr, fmt.Errorf("failed applying FeatureHandler features. cause: %w", applyErr)) } } @@ -78,7 +77,7 @@ func (fh *FeaturesHandler) Apply(ctx context.Context) error { // Delete executes registered clean-up tasks for handled Features in the opposite order they were initiated. // This approach assumes that Features are either instantiated in the correct sequence or are self-contained. -func (fh *FeaturesHandler) Delete(ctx context.Context) error { +func (fh *FeaturesHandler) Delete(ctx context.Context, cli client.Client) error { fh.features = make([]*Feature, 0) for _, featuresProvider := range fh.featuresProviders { @@ -89,7 +88,7 @@ func (fh *FeaturesHandler) Delete(ctx context.Context) error { var multiErr *multierror.Error for i := len(fh.features) - 1; i >= 0; i-- { - if cleanupErr := fh.features[i].Cleanup(ctx); cleanupErr != nil { + if cleanupErr := fh.features[i].Cleanup(ctx, cli); cleanupErr != nil { multiErr = multierror.Append(multiErr, fmt.Errorf("failed executing cleanup in FeatureHandler. cause: %w", cleanupErr)) } } @@ -101,9 +100,8 @@ func (fh *FeaturesHandler) Delete(ctx context.Context) error { // and add them to the handler's registry. type FeaturesProvider func(registry FeaturesRegistry) error -func ClusterFeaturesHandler(cli client.Client, dsci *dsciv1.DSCInitialization, def ...FeaturesProvider) *FeaturesHandler { +func ClusterFeaturesHandler(dsci *dsciv1.DSCInitialization, def ...FeaturesProvider) *FeaturesHandler { return &FeaturesHandler{ - cli: cli, targetNamespace: dsci.Spec.ApplicationsNamespace, owner: dsci, source: featurev1.Source{Type: featurev1.DSCIType, Name: dsci.Name}, @@ -111,9 +109,8 @@ func ClusterFeaturesHandler(cli client.Client, dsci *dsciv1.DSCInitialization, d } } -func ComponentFeaturesHandler(cli client.Client, owner metav1.Object, componentName, targetNamespace string, def ...FeaturesProvider) *FeaturesHandler { +func ComponentFeaturesHandler(owner metav1.Object, componentName, targetNamespace string, def ...FeaturesProvider) *FeaturesHandler { return &FeaturesHandler{ - cli: cli, owner: owner, targetNamespace: targetNamespace, source: featurev1.Source{Type: featurev1.ComponentType, Name: componentName}, @@ -143,16 +140,16 @@ func NewHandlerWithReporter[T client.Object](handler *FeaturesHandler, reporter } } -func (h HandlerWithReporter[T]) Apply(ctx context.Context) error { - applyErr := h.handler.Apply(ctx) +func (h HandlerWithReporter[T]) Apply(ctx context.Context, cli client.Client) error { + applyErr := h.handler.Apply(ctx, cli) _, reportErr := h.reporter.ReportCondition(ctx, applyErr) // We could have failed during Apply phase as well as during reporting. // We should return both errors to the caller. return multierror.Append(applyErr, reportErr).ErrorOrNil() } -func (h HandlerWithReporter[T]) Delete(ctx context.Context) error { - deleteErr := h.handler.Delete(ctx) +func (h HandlerWithReporter[T]) Delete(ctx context.Context, cli client.Client) error { + deleteErr := h.handler.Delete(ctx, cli) _, reportErr := h.reporter.ReportCondition(ctx, deleteErr) // We could have failed during Delete phase as well as during reporting. // We should return both errors to the caller. diff --git a/pkg/feature/resources.go b/pkg/feature/resources.go index 7b89881c9a7..177660c595e 100644 --- a/pkg/feature/resources.go +++ b/pkg/feature/resources.go @@ -3,14 +3,16 @@ package feature import ( "context" + "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" ) // CreateNamespaceIfNotExists will create a namespace with the given name if it does not exist yet. // It does not set ownership nor apply extra metadata to the existing namespace. func CreateNamespaceIfNotExists(namespace string) Action { - return func(ctx context.Context, f *Feature) error { - _, err := cluster.CreateNamespace(ctx, f.Client, namespace) + return func(ctx context.Context, cli client.Client, _ *Feature) error { + _, err := cluster.CreateNamespace(ctx, cli, namespace) return err } diff --git a/pkg/feature/serverless/conditions.go b/pkg/feature/serverless/conditions.go index 12594c9624a..cbdb7f5306a 100644 --- a/pkg/feature/serverless/conditions.go +++ b/pkg/feature/serverless/conditions.go @@ -16,11 +16,11 @@ const ( KnativeServingNamespace = "knative-serving" ) -func EnsureServerlessAbsent(ctx context.Context, f *feature.Feature) error { +func EnsureServerlessAbsent(ctx context.Context, cli client.Client, f *feature.Feature) error { list := &unstructured.UnstructuredList{} list.SetGroupVersionKind(gvk.KnativeServing) - if err := f.Client.List(ctx, list, client.InNamespace("")); err != nil { + if err := cli.List(ctx, list, client.InNamespace("")); err != nil { return fmt.Errorf("failed to list KnativeServings: %w", err) } @@ -46,8 +46,8 @@ func EnsureServerlessAbsent(ctx context.Context, f *feature.Feature) error { return errors.New("existing KNativeServing resource was found; integrating to an existing installation is not supported") } -func EnsureServerlessOperatorInstalled(ctx context.Context, f *feature.Feature) error { - if err := feature.EnsureOperatorIsInstalled("serverless-operator")(ctx, f); err != nil { +func EnsureServerlessOperatorInstalled(ctx context.Context, cli client.Client, f *feature.Feature) error { + if err := feature.EnsureOperatorIsInstalled("serverless-operator")(ctx, cli, f); err != nil { return fmt.Errorf("failed to find the pre-requisite KNative Serving Operator subscription, please ensure Serverless Operator is installed. %w", err) } diff --git a/pkg/feature/serverless/resources.go b/pkg/feature/serverless/resources.go index cc28390895d..cfd7a3ef65b 100644 --- a/pkg/feature/serverless/resources.go +++ b/pkg/feature/serverless/resources.go @@ -3,13 +3,15 @@ package serverless import ( "context" + "sigs.k8s.io/controller-runtime/pkg/client" + infrav1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/infrastructure/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/servicemesh" ) -func ServingCertificateResource(ctx context.Context, f *feature.Feature) error { +func ServingCertificateResource(ctx context.Context, cli client.Client, f *feature.Feature) error { secretData, err := getSecretParams(f) if err != nil { return err @@ -17,7 +19,7 @@ func ServingCertificateResource(ctx context.Context, f *feature.Feature) error { switch secretData.Type { case infrav1.SelfSigned: - return cluster.CreateSelfSignedCertificate(ctx, f.Client, + return cluster.CreateSelfSignedCertificate(ctx, cli, secretData.Name, secretData.Domain, secretData.Namespace, @@ -25,7 +27,7 @@ func ServingCertificateResource(ctx context.Context, f *feature.Feature) error { case infrav1.Provided: return nil default: - return cluster.PropagateDefaultIngressCertificate(ctx, f.Client, secretData.Name, secretData.Namespace) + return cluster.PropagateDefaultIngressCertificate(ctx, cli, secretData.Name, secretData.Namespace) } } diff --git a/pkg/feature/servicemesh/conditions.go b/pkg/feature/servicemesh/conditions.go index ceedc718e69..64bd360b49e 100644 --- a/pkg/feature/servicemesh/conditions.go +++ b/pkg/feature/servicemesh/conditions.go @@ -23,30 +23,30 @@ const ( ) // EnsureAuthNamespaceExists creates a namespace for the Authorization provider and set ownership so it will be garbage collected when the operator is uninstalled. -func EnsureAuthNamespaceExists(ctx context.Context, f *feature.Feature) error { +func EnsureAuthNamespaceExists(ctx context.Context, cli client.Client, f *feature.Feature) error { authNs, err := FeatureData.Authorization.Namespace.Extract(f) if err != nil { return fmt.Errorf("could not get auth from feature: %w", err) } - _, err = cluster.CreateNamespace(ctx, f.Client, authNs, feature.OwnedBy(f), cluster.WithLabels(labels.ODH.OwnedNamespace, "true")) + _, err = cluster.CreateNamespace(ctx, cli, authNs, feature.OwnedBy(f), cluster.WithLabels(labels.ODH.OwnedNamespace, "true")) return err } -func EnsureServiceMeshOperatorInstalled(ctx context.Context, f *feature.Feature) error { - if err := feature.EnsureOperatorIsInstalled("servicemeshoperator")(ctx, f); err != nil { +func EnsureServiceMeshOperatorInstalled(ctx context.Context, cli client.Client, f *feature.Feature) error { + if err := feature.EnsureOperatorIsInstalled("servicemeshoperator")(ctx, cli, f); err != nil { return fmt.Errorf("failed to find the pre-requisite Service Mesh Operator subscription, please ensure Service Mesh Operator is installed. %w", err) } return nil } -func EnsureServiceMeshInstalled(ctx context.Context, f *feature.Feature) error { - if err := EnsureServiceMeshOperatorInstalled(ctx, f); err != nil { +func EnsureServiceMeshInstalled(ctx context.Context, cli client.Client, f *feature.Feature) error { + if err := EnsureServiceMeshOperatorInstalled(ctx, cli, f); err != nil { return err } - if err := WaitForControlPlaneToBeReady(ctx, f); err != nil { + if err := WaitForControlPlaneToBeReady(ctx, cli, f); err != nil { controlPlane, errGet := FeatureData.ControlPlane.Extract(f) if errGet != nil { return fmt.Errorf("failed to get control plane struct: %w", err) @@ -60,7 +60,7 @@ func EnsureServiceMeshInstalled(ctx context.Context, f *feature.Feature) error { return nil } -func WaitForControlPlaneToBeReady(ctx context.Context, f *feature.Feature) error { +func WaitForControlPlaneToBeReady(ctx context.Context, cli client.Client, f *feature.Feature) error { controlPlane, err := FeatureData.ControlPlane.Extract(f) if err != nil { return err @@ -72,7 +72,7 @@ func WaitForControlPlaneToBeReady(ctx context.Context, f *feature.Feature) error f.Log.Info("waiting for control plane components to be ready", "control-plane", smcp, "namespace", smcpNs, "duration (s)", duration.Seconds()) return wait.PollUntilContextTimeout(ctx, interval, duration, false, func(ctx context.Context) (bool, error) { - ready, err := CheckControlPlaneComponentReadiness(ctx, f.Client, smcp, smcpNs) + ready, err := CheckControlPlaneComponentReadiness(ctx, cli, smcp, smcpNs) if ready { f.Log.Info("done waiting for control plane components to be ready", "control-plane", smcp, "namespace", smcpNs) diff --git a/pkg/feature/servicemesh/resources.go b/pkg/feature/servicemesh/resources.go index f07fdd600d9..6eaa8e8103f 100644 --- a/pkg/feature/servicemesh/resources.go +++ b/pkg/feature/servicemesh/resources.go @@ -7,6 +7,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" @@ -14,7 +15,7 @@ import ( // MeshRefs stores service mesh configuration in the config map, so it can // be easily accessed by other components which rely on this information. -func MeshRefs(ctx context.Context, f *feature.Feature) error { +func MeshRefs(ctx context.Context, cli client.Client, f *feature.Feature) error { meshConfig, err := FeatureData.ControlPlane.Extract(f) if err != nil { return fmt.Errorf("failed to get control plane struct: %w", err) @@ -28,7 +29,7 @@ func MeshRefs(ctx context.Context, f *feature.Feature) error { return cluster.CreateOrUpdateConfigMap( ctx, - f.Client, + cli, &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: ConfigMapMeshRef, @@ -42,7 +43,7 @@ func MeshRefs(ctx context.Context, f *feature.Feature) error { // AuthRefs stores authorization configuration in the config map, so it can // be easily accessed by other components which rely on this information. -func AuthRefs(ctx context.Context, f *feature.Feature) error { +func AuthRefs(ctx context.Context, cli client.Client, f *feature.Feature) error { targetNamespace := f.TargetNamespace auth, err := FeatureData.Authorization.Spec.Extract(f) if err != nil { @@ -73,7 +74,7 @@ func AuthRefs(ctx context.Context, f *feature.Feature) error { return cluster.CreateOrUpdateConfigMap( ctx, - f.Client, + cli, &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: ConfigMapAuthRef, diff --git a/tests/integration/features/cleanup_int_test.go b/tests/integration/features/cleanup_int_test.go index 54f17d1fb3d..8d0634cfc0c 100644 --- a/tests/integration/features/cleanup_int_test.go +++ b/tests/integration/features/cleanup_int_test.go @@ -46,7 +46,6 @@ var _ = Describe("feature cleanup", func() { Type: featurev1.DSCIType, Name: dsci.Name, }). - UsingClient(envTestClient). PreConditions( feature.CreateNamespaceIfNotExists(namespace), ). @@ -63,7 +62,7 @@ var _ = Describe("feature cleanup", func() { It("should successfully create resource and associated feature tracker", func(ctx context.Context) { // when - Expect(testFeature.Apply(ctx)).Should(Succeed()) + Expect(testFeature.Apply(ctx, envTestClient)).Should(Succeed()) // then Eventually(createdSecretHasOwnerReferenceToOwningFeature(namespace, featureName)). @@ -75,7 +74,7 @@ var _ = Describe("feature cleanup", func() { It("should remove feature tracker on clean-up", func(ctx context.Context) { // when - Expect(testFeature.Cleanup(ctx)).To(Succeed()) + Expect(testFeature.Cleanup(ctx, envTestClient)).To(Succeed()) // then Consistently(createdSecretHasOwnerReferenceToOwningFeature(namespace, featureName)). @@ -111,7 +110,7 @@ var _ = Describe("feature cleanup", func() { err := fixtures.CreateOrUpdateNamespace(ctx, envTestClient, fixtures.NewNamespace("conditional-ns")) Expect(err).To(Not(HaveOccurred())) - featuresHandler = feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler = feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { return registry.Add(feature.Define(featureName). EnabledWhen(namespaceExists). PreConditions( @@ -122,7 +121,7 @@ var _ = Describe("feature cleanup", func() { }) // when - Expect(featuresHandler.Apply(ctx)).Should(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).Should(Succeed()) // then Eventually(createdSecretHasOwnerReferenceToOwningFeature(namespace, featureName)). @@ -138,7 +137,7 @@ var _ = Describe("feature cleanup", func() { Expect(err).To(Not(HaveOccurred())) // Mimic reconcile by re-loading the feature handler - featuresHandler = feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler = feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { return registry.Add(feature.Define(featureName). EnabledWhen(namespaceExists). PreConditions( @@ -148,7 +147,7 @@ var _ = Describe("feature cleanup", func() { ) }) - Expect(featuresHandler.Apply(ctx)).Should(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).Should(Succeed()) // then Consistently(createdSecretHasOwnerReferenceToOwningFeature(namespace, featureName)). @@ -212,8 +211,8 @@ func createdSecretHasOwnerReferenceToOwningFeature(namespace, featureName string } } -func namespaceExists(ctx context.Context, f *feature.Feature) (bool, error) { - namespace, err := fixtures.GetNamespace(ctx, f.Client, "conditional-ns") +func namespaceExists(ctx context.Context, cli client.Client, f *feature.Feature) (bool, error) { + namespace, err := fixtures.GetNamespace(ctx, cli, "conditional-ns") if errors.IsNotFound(err) { return false, nil } diff --git a/tests/integration/features/fixtures/cluster_test_fixtures.go b/tests/integration/features/fixtures/cluster_test_fixtures.go index 0dc13dc8255..0837050fd5a 100644 --- a/tests/integration/features/fixtures/cluster_test_fixtures.go +++ b/tests/integration/features/fixtures/cluster_test_fixtures.go @@ -79,8 +79,8 @@ func GetService(ctx context.Context, client client.Client, namespace, name strin return svc, err } -func CreateSecret(name, namespace string) func(ctx context.Context, f *feature.Feature) error { - return func(ctx context.Context, f *feature.Feature) error { +func CreateSecret(name, namespace string) feature.Action { + return func(ctx context.Context, cli client.Client, f *feature.Feature) error { secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -94,7 +94,7 @@ func CreateSecret(name, namespace string) func(ctx context.Context, f *feature.F }, } - return f.Client.Create(ctx, secret) + return cli.Create(ctx, secret) } } diff --git a/tests/integration/features/manifests_int_test.go b/tests/integration/features/manifests_int_test.go index 0575bcbfad7..a5b9171cba7 100644 --- a/tests/integration/features/manifests_int_test.go +++ b/tests/integration/features/manifests_int_test.go @@ -46,7 +46,7 @@ var _ = Describe("Applying resources", func() { It("should be able to process an embedded YAML file", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errNsCreate := registry.Add(feature.Define("create-namespaces"). Manifests( manifest.Location(fixtures.TestEmbeddedFiles). @@ -60,7 +60,7 @@ var _ = Describe("Applying resources", func() { }) // when - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) // then embeddedNs1, errNS1 := fixtures.GetNamespace(ctx, envTestClient, "embedded-test-ns-1") @@ -76,7 +76,7 @@ var _ = Describe("Applying resources", func() { It("should be able to process an embedded template file", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errSvcCreate := registry.Add(feature.Define("create-local-gw-svc"). Manifests( manifest.Location(fixtures.TestEmbeddedFiles). @@ -91,7 +91,7 @@ var _ = Describe("Applying resources", func() { }) // when - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) // then service, err := fixtures.GetService(ctx, envTestClient, namespace.Name, "knative-local-gateway") @@ -110,7 +110,7 @@ metadata: Expect(fixtures.CreateFile(tempDir, "namespace.yaml", nsYAML)).To(Succeed()) - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errSvcCreate := registry.Add(feature.Define("create-namespace"). Manifests( manifest.Location(os.DirFS(tempDir)). @@ -124,7 +124,7 @@ metadata: }) // when - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) // then realNs, err := fixtures.GetNamespace(ctx, envTestClient, "real-file-test-ns") diff --git a/tests/integration/features/preconditions_int_test.go b/tests/integration/features/preconditions_int_test.go index b5d620b18e0..4673a15abf4 100644 --- a/tests/integration/features/preconditions_int_test.go +++ b/tests/integration/features/preconditions_int_test.go @@ -46,7 +46,7 @@ var _ = Describe("feature preconditions", func() { defer objectCleaner.DeleteAll(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}}) // when - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("create-new-ns"). PreConditions(feature.CreateNamespaceIfNotExists(namespace)), ) @@ -57,7 +57,7 @@ var _ = Describe("feature preconditions", func() { }) // then - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) // and Eventually(func() error { @@ -81,7 +81,7 @@ var _ = Describe("feature preconditions", func() { defer objectCleaner.DeleteAll(ctx, ns) // when - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("create-new-ns"). PreConditions(feature.CreateNamespaceIfNotExists(namespace)), ) @@ -92,7 +92,7 @@ var _ = Describe("feature preconditions", func() { }) // then - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) }) diff --git a/tests/integration/features/resources_int_test.go b/tests/integration/features/resources_int_test.go index c8dc1219b1a..e8043f92ac1 100644 --- a/tests/integration/features/resources_int_test.go +++ b/tests/integration/features/resources_int_test.go @@ -55,7 +55,7 @@ var _ = Describe("Applying and updating resources", func() { It("should reconcile the resource to its managed state", func(ctx context.Context) { // given managed feature - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { return registry.Add( feature.Define("create-local-gw-svc"). Managed(). @@ -66,7 +66,7 @@ var _ = Describe("Applying and updating resources", func() { WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)), ) }) - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) // expect created svc to have managed annotation service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway") @@ -81,7 +81,7 @@ var _ = Describe("Applying and updating resources", func() { // then // expect that modification is reconciled away - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway") Expect(err).ToNot(HaveOccurred()) Expect(updatedService.Annotations).To( @@ -91,7 +91,7 @@ var _ = Describe("Applying and updating resources", func() { It("should not reconcile explicitly opt-ed out resource", func(ctx context.Context) { // given managed feature - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { return registry.Add( feature.Define("create-unmanaged-svc"). Managed(). @@ -102,7 +102,7 @@ var _ = Describe("Applying and updating resources", func() { WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)), ) }) - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) // when service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "unmanaged-svc") @@ -112,7 +112,7 @@ var _ = Describe("Applying and updating resources", func() { // then // expect that modification is reconciled away - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "unmanaged-svc") Expect(err).ToNot(HaveOccurred()) @@ -127,7 +127,7 @@ var _ = Describe("Applying and updating resources", func() { It("should not reconcile the resource", func(ctx context.Context) { // given unmanaged feature - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { return registry.Add( feature.Define("create-local-gw-svc"). Manifests( @@ -137,7 +137,7 @@ var _ = Describe("Applying and updating resources", func() { WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)), ) }) - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) // when service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway") @@ -147,7 +147,7 @@ var _ = Describe("Applying and updating resources", func() { // then // expect that modification is reconciled away - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway") Expect(err).ToNot(HaveOccurred()) Expect(updatedService.Annotations).To( @@ -160,7 +160,7 @@ var _ = Describe("Applying and updating resources", func() { When("a feature is unmanaged but the object is marked as managed", func() { It("should reconcile this resource", func(ctx context.Context) { // given unmanaged feature but object marked with managed annotation - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { return registry.Add( feature.Define("create-managed-svc"). Manifests( @@ -170,7 +170,7 @@ var _ = Describe("Applying and updating resources", func() { WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)), ) }) - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) // when service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "managed-svc") @@ -183,7 +183,7 @@ var _ = Describe("Applying and updating resources", func() { // then // expect that modification is reconciled away - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "managed-svc") Expect(err).ToNot(HaveOccurred()) Expect(updatedService.Annotations).To( diff --git a/tests/integration/features/serverless_feature_test.go b/tests/integration/features/serverless_feature_test.go index 9e6db931dd3..7f116479592 100644 --- a/tests/integration/features/serverless_feature_test.go +++ b/tests/integration/features/serverless_feature_test.go @@ -63,10 +63,10 @@ var _ = Describe("Serverless feature", func() { return nil } - featuresHandler := feature.ComponentFeaturesHandler(envTestClient, dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) + featuresHandler := feature.ComponentFeaturesHandler(dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) // when - applyErr := featuresHandler.Apply(ctx) + applyErr := featuresHandler.Apply(ctx, envTestClient) // then Expect(applyErr).To(MatchError(ContainSubstring("failed to find the pre-requisite operator subscription \"serverless-operator\""))) @@ -111,10 +111,10 @@ var _ = Describe("Serverless feature", func() { return nil } - featuresHandler := feature.ComponentFeaturesHandler(envTestClient, dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) + featuresHandler := feature.ComponentFeaturesHandler(dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) // then - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) }) It("should succeed if serving is not installed for KNative serving precondition", func(ctx context.Context) { @@ -130,10 +130,10 @@ var _ = Describe("Serverless feature", func() { return nil } - featuresHandler := feature.ComponentFeaturesHandler(envTestClient, dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) + featuresHandler := feature.ComponentFeaturesHandler(dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) // then - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) }) It("should fail if serving is already installed for KNative serving precondition", func(ctx context.Context) { @@ -160,10 +160,10 @@ var _ = Describe("Serverless feature", func() { return nil } - featuresHandler := feature.ComponentFeaturesHandler(envTestClient, dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) + featuresHandler := feature.ComponentFeaturesHandler(dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) // then - Expect(featuresHandler.Apply(ctx)).ToNot(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).ToNot(Succeed()) }) }) @@ -171,17 +171,6 @@ var _ = Describe("Serverless feature", func() { Context("default values", func() { - var testFeature *feature.Feature - - BeforeEach(func() { - // Stubbing feature as we want to test particular functions in isolation - testFeature = &feature.Feature{ - Name: "test-feature", - } - - testFeature.Client = envTestClient - }) - Context("ingress gateway TLS secret name", func() { It("should set default value when value is empty in the DSCI", func(ctx context.Context) { @@ -302,10 +291,10 @@ var _ = Describe("Serverless feature", func() { return nil } - featuresHandler := feature.ComponentFeaturesHandler(envTestClient, dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) + featuresHandler := feature.ComponentFeaturesHandler(dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) // when - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) // then Eventually(func() error { @@ -344,10 +333,10 @@ var _ = Describe("Serverless feature", func() { return nil } - featuresHandler := feature.ComponentFeaturesHandler(envTestClient, dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) + featuresHandler := feature.ComponentFeaturesHandler(dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) // when - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) // then Consistently(func() error { diff --git a/tests/integration/features/servicemesh_feature_test.go b/tests/integration/features/servicemesh_feature_test.go index 43c0a3b8d0b..16a7b419063 100644 --- a/tests/integration/features/servicemesh_feature_test.go +++ b/tests/integration/features/servicemesh_feature_test.go @@ -53,7 +53,7 @@ var _ = Describe("Service Mesh setup", func() { It("should fail using precondition check", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("no-service-mesh-operator-check"). PreConditions(servicemesh.EnsureServiceMeshOperatorInstalled), ) @@ -64,7 +64,7 @@ var _ = Describe("Service Mesh setup", func() { }) // when - applyErr := featuresHandler.Apply(ctx) + applyErr := featuresHandler.Apply(ctx, envTestClient) // then Expect(applyErr).To(MatchError(ContainSubstring("failed to find the pre-requisite operator subscription \"servicemeshoperator\""))) @@ -86,7 +86,7 @@ var _ = Describe("Service Mesh setup", func() { It("should succeed using precondition check", func(ctx context.Context) { // when - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add( feature.Define("service-mesh-operator-check"). WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)). @@ -99,7 +99,7 @@ var _ = Describe("Service Mesh setup", func() { }) // when - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) }) @@ -118,7 +118,7 @@ var _ = Describe("Service Mesh setup", func() { dsci.Spec.ServiceMesh.ControlPlane.Name = "test-name" // when - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("service-mesh-control-plane-check"). WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)). PreConditions(servicemesh.EnsureServiceMeshInstalled), @@ -130,7 +130,7 @@ var _ = Describe("Service Mesh setup", func() { }) // then - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) }) It("should fail to find Service Mesh Control Plane if not present", func(ctx context.Context) { @@ -139,7 +139,7 @@ var _ = Describe("Service Mesh setup", func() { dsci.Spec.ServiceMesh.ControlPlane.Namespace = "test-namespace" // when - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("no-service-mesh-control-plane-check"). WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)). PreConditions(servicemesh.EnsureServiceMeshInstalled), @@ -151,7 +151,7 @@ var _ = Describe("Service Mesh setup", func() { }) // then - Expect(featuresHandler.Apply(ctx)).To(MatchError(ContainSubstring("failed to find Service Mesh Control Plane"))) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(MatchError(ContainSubstring("failed to find Service Mesh Control Plane"))) }) }) @@ -195,7 +195,7 @@ var _ = Describe("Service Mesh setup", func() { createServiceMeshControlPlane(ctx, name, namespace) - handler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + handler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { return registry.Add(feature.Define("control-plane-with-external-authz-provider"). Manifests( manifest.Location(fixtures.TestEmbeddedFiles). @@ -217,7 +217,7 @@ var _ = Describe("Service Mesh setup", func() { // when By("verifying extension provider has been added after applying feature", func() { - Expect(handler.Apply(ctx)).To(Succeed()) + Expect(handler.Apply(ctx, envTestClient)).To(Succeed()) serviceMeshControlPlane, err := getServiceMeshControlPlane(ctx, namespace, name) Expect(err).ToNot(HaveOccurred()) @@ -240,7 +240,7 @@ var _ = Describe("Service Mesh setup", func() { // then By("verifying that extension provider has been removed and namespace is gone too", func() { - Expect(handler.Delete(ctx)).To(Succeed()) + Expect(handler.Delete(ctx, envTestClient)).To(Succeed()) Eventually(func(ctx context.Context) []any { serviceMeshControlPlane, err := getServiceMeshControlPlane(ctx, namespace, name) diff --git a/tests/integration/features/tracker_int_test.go b/tests/integration/features/tracker_int_test.go index 370bfe25117..b4f7bca7944 100644 --- a/tests/integration/features/tracker_int_test.go +++ b/tests/integration/features/tracker_int_test.go @@ -6,6 +6,7 @@ import ( conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1" corev1 "k8s.io/api/core/v1" + ctrlruntime "sigs.k8s.io/controller-runtime/pkg/client" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" featurev1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" @@ -35,7 +36,7 @@ var _ = Describe("Feature tracking capability", func() { Context("Reporting progress when applying Feature", func() { It("should indicate successful installation in FeatureTracker through Status conditions", func(ctx context.Context) { - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("always-working-feature")) Expect(errFeatureAdd).ToNot(HaveOccurred()) @@ -44,7 +45,7 @@ var _ = Describe("Feature tracking capability", func() { }) // when - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) // then featureTracker, err := fixtures.GetFeatureTracker(ctx, envTestClient, appNamespace, "always-working-feature") @@ -61,9 +62,9 @@ var _ = Describe("Feature tracking capability", func() { It("should indicate when failure occurs in preconditions through Status conditions", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("precondition-fail"). - PreConditions(func(_ context.Context, _ *feature.Feature) error { + PreConditions(func(_ context.Context, _ ctrlruntime.Client, _ *feature.Feature) error { return errors.New("during test always fail") }), ) @@ -74,7 +75,7 @@ var _ = Describe("Feature tracking capability", func() { }) // when - Expect(featuresHandler.Apply(ctx)).ToNot(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).ToNot(Succeed()) // then featureTracker, err := fixtures.GetFeatureTracker(ctx, envTestClient, appNamespace, "precondition-fail") @@ -91,9 +92,9 @@ var _ = Describe("Feature tracking capability", func() { It("should indicate when failure occurs in post-conditions through Status conditions", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("post-condition-failure"). - PostConditions(func(_ context.Context, _ *feature.Feature) error { + PostConditions(func(_ context.Context, _ ctrlruntime.Client, _ *feature.Feature) error { return errors.New("during test always fail") }), ) @@ -104,7 +105,7 @@ var _ = Describe("Feature tracking capability", func() { }) // when - Expect(featuresHandler.Apply(ctx)).ToNot(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).ToNot(Succeed()) // then featureTracker, err := fixtures.GetFeatureTracker(ctx, envTestClient, appNamespace, "post-condition-failure") @@ -124,7 +125,7 @@ var _ = Describe("Feature tracking capability", func() { It("should correctly indicate source in the feature tracker", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("always-working-feature")) Expect(errFeatureAdd).ToNot(HaveOccurred()) @@ -133,7 +134,7 @@ var _ = Describe("Feature tracking capability", func() { }) // when - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) // then featureTracker, err := fixtures.GetFeatureTracker(ctx, envTestClient, appNamespace, "always-working-feature") @@ -148,7 +149,7 @@ var _ = Describe("Feature tracking capability", func() { It("should correctly indicate app namespace in the feature tracker", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("empty-feature")) Expect(errFeatureAdd).ToNot(HaveOccurred()) @@ -157,7 +158,7 @@ var _ = Describe("Feature tracking capability", func() { }) // when - Expect(featuresHandler.Apply(ctx)).To(Succeed()) + Expect(featuresHandler.Apply(ctx, envTestClient)).To(Succeed()) // then featureTracker, err := fixtures.GetFeatureTracker(ctx, envTestClient, appNamespace, "empty-feature") @@ -175,7 +176,6 @@ var _ = Describe("Feature tracking capability", func() { Expect(fixtures.CreateOrUpdateDSCI(ctx, envTestClient, dsci)).ToNot(HaveOccurred()) feature, featErr := feature.Define("empty-feat-with-owner"). - UsingClient(envTestClient). Source(featurev1.Source{ Type: featurev1.DSCIType, Name: dsci.Name, @@ -186,7 +186,7 @@ var _ = Describe("Feature tracking capability", func() { // when Expect(featErr).ToNot(HaveOccurred()) - Expect(feature.Apply(ctx)).To(Succeed()) + Expect(feature.Apply(ctx, envTestClient)).To(Succeed()) // then tracker, err := fixtures.GetFeatureTracker(ctx, envTestClient, appNamespace, "empty-feat-with-owner") @@ -197,7 +197,6 @@ var _ = Describe("Feature tracking capability", func() { It("should not indicate owner in the feature tracker when owner not in feature", func(ctx context.Context) { // given feature, featErr := feature.Define("empty-feat-no-owner"). - UsingClient(envTestClient). Source(featurev1.Source{ Type: featurev1.DSCIType, Name: dsci.Name, @@ -207,7 +206,7 @@ var _ = Describe("Feature tracking capability", func() { // when Expect(featErr).ToNot(HaveOccurred()) - Expect(feature.Apply(ctx)).To(Succeed()) + Expect(feature.Apply(ctx, envTestClient)).To(Succeed()) // then tracker, err := fixtures.GetFeatureTracker(ctx, envTestClient, appNamespace, "empty-feat-no-owner")