From 5bb6291e5dd9a57a7ded7bf11765e366b9cf425f Mon Sep 17 00:00:00 2001 From: bartoszmajsak Date: Fri, 16 Aug 2024 13:38:37 +0200 Subject: [PATCH] fix(feature): cleanup funcs should not rely on feature's data Initially, `Cleanup` functions did not rely on loading Feature's data for invocation, as certain data was held as fields of the Feature struct, making it implicitly available. With the refactoring of Feature to become a "glorified map" container, this behavior has changed, leading to errors under certain circumstances. With `DSCI.ServiceMesh` now being a pointer to a struct, there is a corner case where the (now needed) ServiceMesh spec is `nil`, leading to a panic. When the reconciliation of DSC is triggered, it is invoked for each component, even if it is never defined or changed. In a simple case where DSCI does not have a specified ServiceMesh (`nil` instead of the default), and DSC has a single `Managed` component that is not KServe/Serverless, the reconciliation will trigger the removal of KServe resources regardless. This, in turn, will call for the removal of Serverless features, which rely on SMCP config when they are applied. With data now being loaded in the cleanup step, this leads to an error, as there is no SMCP config to read from. By default, the cleanup logic removes the owner FeatureTracker for each Feature, and this does not require loading Feature's data, as FeatureTracker is used as `OwnerReference` for each resources created for a given Feature. The API allows defining custom functions that can be invoked in addition. There is one custom cleanup function requiring the Service Mesh control plane spec to remove part of the config introduced by its patch. The code in question only worked because it had default values to work on and was not failing if patched object was not found. The refactoring wrongly enforced loading of Feature data during cleanup for this single case, and this cascaded to other Feature sets where it was unnecessary exposing this faulty behaviour. With this change, no reference to Feature is enforced at the API level by introducing the `CleanupFunc(ctx, client)` type for defining custom cleanup logic. The custom patch function has been reworked to comply with this. --- .../dscinitialization/servicemesh_setup.go | 5 +- pkg/feature/builder.go | 6 +- pkg/feature/feature.go | 23 +++-- pkg/feature/feature_tracker_handler.go | 36 ++++---- pkg/feature/servicemesh/cleanup.go | 87 +++++++++---------- .../features/servicemesh_feature_test.go | 5 +- 6 files changed, 80 insertions(+), 82 deletions(-) diff --git a/controllers/dscinitialization/servicemesh_setup.go b/controllers/dscinitialization/servicemesh_setup.go index fb8168db122..283a0f33570 100644 --- a/controllers/dscinitialization/servicemesh_setup.go +++ b/controllers/dscinitialization/servicemesh_setup.go @@ -203,7 +203,10 @@ func (r *DSCInitializationReconciler) authorizationFeatures(instance *dsciv1.DSC feature.WaitForPodsToBeReady(serviceMeshSpec.ControlPlane.Namespace), ). OnDelete( - servicemesh.RemoveExtensionProvider, + servicemesh.RemoveExtensionProvider( + instance.Spec.ServiceMesh.ControlPlane, + instance.Spec.ApplicationsNamespace+"-auth-provider", + ), ), // We do not have the control over deployment resource creation. diff --git a/pkg/feature/builder.go b/pkg/feature/builder.go index 7ddfbbeb6bd..e182c716be3 100644 --- a/pkg/feature/builder.go +++ b/pkg/feature/builder.go @@ -174,11 +174,7 @@ func (fb *featureBuilder) PostConditions(postconditions ...Action) *featureBuild } // OnDelete allow to add cleanup hooks that are executed when the feature is going to be deleted. -// By default, all resources created by the feature are deleted when the feature is deleted, so there is no need to -// explicitly add cleanup hooks for them. -// -// This is useful when you need to perform some additional cleanup actions such as removing effects of a patch operation. -func (fb *featureBuilder) OnDelete(cleanups ...Action) *featureBuilder { +func (fb *featureBuilder) OnDelete(cleanups ...CleanupFunc) *featureBuilder { fb.builders = append(fb.builders, func(f *Feature) error { f.addCleanup(cleanups...) diff --git a/pkg/feature/feature.go b/pkg/feature/feature.go index 5ff39b08a48..967e284cacc 100644 --- a/pkg/feature/feature.go +++ b/pkg/feature/feature.go @@ -51,7 +51,7 @@ type Feature struct { appliers []resource.Applier - cleanups []Action + cleanups []CleanupFunc clusterOperations []Action preconditions []Action postconditions []Action @@ -62,6 +62,12 @@ type Feature struct { // while having access to Feature struct. type Action func(ctx context.Context, 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, +// so there is no need to explicitly add cleanup hooks for them. +// This is useful when you need to perform some additional cleanup actions such as removing effects of a patch operation. +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) @@ -138,26 +144,17 @@ func (f *Feature) applyFeature(ctx context.Context) error { func (f *Feature) Cleanup(ctx context.Context) error { // Ensure associated FeatureTracker instance has been removed as last one // in the chain of cleanups. - f.addCleanup(removeFeatureTracker) + f.addCleanup(removeFeatureTracker(f)) var cleanupErrors *multierror.Error - - for _, dataProvider := range f.dataProviders { - cleanupErrors = multierror.Append(cleanupErrors, dataProvider(ctx, f)) - } - - if dataLoadErr := cleanupErrors.ErrorOrNil(); dataLoadErr != nil { - return dataLoadErr - } - for _, cleanupFunc := range f.cleanups { - cleanupErrors = multierror.Append(cleanupErrors, cleanupFunc(ctx, f)) + cleanupErrors = multierror.Append(cleanupErrors, cleanupFunc(ctx, f.Client)) } return cleanupErrors.ErrorOrNil() } -func (f *Feature) addCleanup(cleanupFuncs ...Action) { +func (f *Feature) addCleanup(cleanupFuncs ...CleanupFunc) { f.cleanups = append(f.cleanups, cleanupFuncs...) } diff --git a/pkg/feature/feature_tracker_handler.go b/pkg/feature/feature_tracker_handler.go index f9976df05e2..403d87abbea 100644 --- a/pkg/feature/feature_tracker_handler.go +++ b/pkg/feature/feature_tracker_handler.go @@ -30,7 +30,7 @@ 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) + tracker, errGet := getFeatureTracker(ctx, f.Client, f.Name, f.TargetNamespace) if client.IgnoreNotFound(errGet) != nil { return errGet } @@ -56,28 +56,30 @@ func createFeatureTracker(ctx context.Context, f *Feature) error { } // removeFeatureTracker removes the FeatureTracker associated with the provided Feature instance if one exists in the cluster. -func removeFeatureTracker(ctx context.Context, f *Feature) error { - associatedTracker := f.tracker - if associatedTracker == nil { - // Check if it is persisted in the cluster, but Feature do not have it attached - if tracker, errGet := getFeatureTracker(ctx, f); client.IgnoreNotFound(errGet) != nil { - return errGet - } else { - associatedTracker = tracker +func removeFeatureTracker(f *Feature) CleanupFunc { + return func(ctx context.Context, cli client.Client) error { + associatedTracker := f.tracker + if associatedTracker == nil { + // Check if it is persisted in the cluster, but Feature do not have it attached + if tracker, errGet := getFeatureTracker(ctx, cli, f.Name, f.TargetNamespace); client.IgnoreNotFound(errGet) != nil { + return errGet + } else { + associatedTracker = tracker + } } - } - if associatedTracker != nil { - return client.IgnoreNotFound(f.Client.Delete(ctx, associatedTracker)) - } + if associatedTracker != nil { + return client.IgnoreNotFound(f.Client.Delete(ctx, associatedTracker)) + } - return nil + return nil + } } -func getFeatureTracker(ctx context.Context, f *Feature) (*featurev1.FeatureTracker, error) { - tracker := featurev1.NewFeatureTracker(f.Name, f.TargetNamespace) +func getFeatureTracker(ctx context.Context, cli client.Client, featureName, namespace string) (*featurev1.FeatureTracker, error) { + tracker := featurev1.NewFeatureTracker(featureName, namespace) - if errGet := f.Client.Get(ctx, client.ObjectKeyFromObject(tracker), tracker); errGet != nil { + if errGet := cli.Get(ctx, client.ObjectKeyFromObject(tracker), tracker); errGet != nil { return nil, errGet } diff --git a/pkg/feature/servicemesh/cleanup.go b/pkg/feature/servicemesh/cleanup.go index 87163442591..73ccc211c49 100644 --- a/pkg/feature/servicemesh/cleanup.go +++ b/pkg/feature/servicemesh/cleanup.go @@ -2,65 +2,62 @@ package servicemesh import ( "context" - "fmt" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "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/gvk" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" ) -func RemoveExtensionProvider(ctx context.Context, f *feature.Feature) error { - extensionName, errExtName := FeatureData.Authorization.ExtensionProviderName.Extract(f) - if errExtName != nil { - return fmt.Errorf("failed to get extension name struct: %w", errExtName) - } +func RemoveExtensionProvider(controlPlane infrav1.ControlPlaneSpec, extensionName string) feature.CleanupFunc { + return func(ctx context.Context, cli client.Client) error { + smcp := &unstructured.Unstructured{} + smcp.SetGroupVersionKind(gvk.ServiceMeshControlPlane) - controlPlane, err := FeatureData.ControlPlane.Extract(f) - if err != nil { - return fmt.Errorf("failed to get control plane struct: %w", err) - } + if err := cli.Get(ctx, client.ObjectKey{ + Namespace: controlPlane.Namespace, + Name: controlPlane.Name, + }, smcp); err != nil { + return client.IgnoreNotFound(err) + } - smcp := &unstructured.Unstructured{} - smcp.SetGroupVersionKind(gvk.ServiceMeshControlPlane) + extensionProviders, found, err := unstructured.NestedSlice(smcp.Object, "spec", "techPreview", "meshConfig", "extensionProviders") + if err != nil { + return err + } + if !found { + return nil + } - if err := f.Client.Get(ctx, client.ObjectKey{ - Namespace: controlPlane.Namespace, - Name: controlPlane.Name, - }, smcp); err != nil { - return client.IgnoreNotFound(err) - } + removed := false - extensionProviders, found, err := unstructured.NestedSlice(smcp.Object, "spec", "techPreview", "meshConfig", "extensionProviders") - if err != nil { - return err - } - if !found { - f.Log.Info("no extension providers found", "feature", f.Name, "control-plane", controlPlane.Name, "namespace", controlPlane.Namespace) - return nil - } + for i, v := range extensionProviders { + extensionProvider, ok := v.(map[string]interface{}) + if !ok { + continue + } - for i, v := range extensionProviders { - extensionProvider, ok := v.(map[string]interface{}) - if !ok { - f.Log.Info("WARN: Unexpected type for extensionProvider, it will not be removed") - continue - } - currentExtensionName, isString := extensionProvider["name"].(string) - if !isString { - f.Log.Info("WARN: Unexpected type for currentExtensionName, it will not be removed") - continue - } - if currentExtensionName == extensionName { - extensionProviders = append(extensionProviders[:i], extensionProviders[i+1:]...) - err = unstructured.SetNestedSlice(smcp.Object, extensionProviders, "spec", "techPreview", "meshConfig", "extensionProviders") - if err != nil { - return err + currentExtensionName, isString := extensionProvider["name"].(string) + if !isString { + continue + } + if currentExtensionName == extensionName { + extensionProviders = append(extensionProviders[:i], extensionProviders[i+1:]...) + err = unstructured.SetNestedSlice(smcp.Object, extensionProviders, "spec", "techPreview", "meshConfig", "extensionProviders") + if err != nil { + return err + } + removed = true + break } - break } - } - return f.Client.Update(ctx, smcp) + if removed { + return cli.Update(ctx, smcp) + } + + return nil + } } diff --git a/tests/integration/features/servicemesh_feature_test.go b/tests/integration/features/servicemesh_feature_test.go index abfd7285a3a..1c77c0eea7c 100644 --- a/tests/integration/features/servicemesh_feature_test.go +++ b/tests/integration/features/servicemesh_feature_test.go @@ -212,7 +212,10 @@ var _ = Describe("Service Mesh setup", func() { servicemesh.FeatureData.ControlPlane.Define(&dsci.Spec).AsAction(), ). OnDelete( - servicemesh.RemoveExtensionProvider, + servicemesh.RemoveExtensionProvider( + dsci.Spec.ServiceMesh.ControlPlane, + dsci.Spec.ApplicationsNamespace+"-auth-provider", + ), )) })