feat: introduces conditional control plane installation (#10)

* chore: extracts smcp GVR to a var

and reuse it in smcp-related funcs

* fix: returns err on oauth creation failure

instead of nil by mistake

* feat: adds ability to enable feature based on predicates

in certain cases this information can only be determined at runtime, for example based on user-defined values of the plugin spec.

* feat: introduces control plane installation

coupled with wait condition

* fix: makes feature enabled by default

* chore: reworks smcp creation

It relies on owner reference so can be cleaned up as other resources

* fix: applies cleanups only if feature is enabled

Additionally moves OssmResourceTracker creation to .Apply func

* feat: reworks plugin integration to use Apply and Delete funcs of KfApp

This way we can cleanup the resources in case deploying service mesh manifests because creating those in Generate phase leaves them hanging due to failing Delete hook

* fix(reconcile): corrects KfDef status in case of any error

Additionally propagates the error through reconcile to keep trying, as it was ignored.

* fix: reworks SMCP component readiness check

* chore: improves KfApp func docs

* chore: reworks logging in verifiers

* chore: simplifies CRD existence check

* chore: adds tests for feature enablement

* fix: sets interval for SMCP polling to 5s

* fix: handles types which are derived for built-in ones

for example `type InstallationMode string` was previously failing when
converting the default value defined in a custom tag. Now there's a
check performed and if value can be convereted to a given type it will.

In all other cases it will return an error.

* fix: sets default control plane installation mode rely on existing
installation

Proposed installation modes are:

- minimal
- pre-installed

apis/ossm.plugins.kubeflow.org/v1alpha1/ossm_types.go

@@ -24,11 +24,27 @@ type OssmPluginSpec struct {
 	Auth AuthSpec `json:"auth,omitempty"`
 }
 
+// InstallationMode defines how the plugin should handle OpenShift Service Mesh installation.
+// If not specified `use-existing` is assumed.
+type InstallationMode string
+
+var (
+	// PreInstalled indicates that KfDef plugin for Openshift Service Mesh will use existing
+	// installation and patch Service Mesh Control Plane.
+	PreInstalled InstallationMode = "pre-installed"
+
+	// Minimal results in installing Openshift Service Mesh Control Plane
+	// in defined namespace with minimal required configuration.
+	Minimal InstallationMode = "minimal"
+)
+
 // MeshSpec holds information on how Service Mesh should be configured.
 type MeshSpec struct {
-	Name        string   `json:"name,omitempty"`
-	Namespace   string   `json:"namespace,omitempty"`
-	Certificate CertSpec `json:"certificate,omitempty"`
+	Name      string `json:"name,omitempty"`
+	Namespace string `json:"namespace,omitempty"`
+	// +kubebuilder:validation:Enum=minimal;pre-installed
+	InstallationMode InstallationMode `json:"installationMode,omitempty"`
+	Certificate      CertSpec         `json:"certificate,omitempty"`
 }
 
 type CertSpec struct {

config/crd/bases/ossm.plugins.kubeflow.org_ossmplugins.yaml

@@ -62,6 +62,14 @@ spec:
                       name:
                         type: string
                     type: object
+                  installationMode:
+                    description: InstallationMode defines how the plugin should handle
+                      OpenShift Service Mesh installation. If not specified `use-existing`
+                      is assumed.
+                    enum:
+                    - minimal
+                    - pre-installed
+                    type: string
                   name:
                     type: string
                   namespace:

config/crd/bases/ossmplugin.internal.kubeflow.org_kfossmplugins.yaml

@@ -65,6 +65,11 @@ spec:
                       name:
                         type: string
                     type: object
+                  installationMode:
+                    description: InstallationMode defines how the plugin should handle
+                      OpenShift Service Mesh installation. If not specified `use-existing`
+                      is assumed.
+                    type: string
                   name:
                     type: string
                   namespace:

controllers/kfdef.apps.kubeflow.org/kfdef_controller.go

@@ -247,12 +247,11 @@ func (r *KfDefReconciler) Reconcile(ctx context.Context, request ctrl.Request) (
 	}
 
 	// set status of the KfDef resource
-	if err := r.reconcileStatus(instance); err != nil {
-		return ctrl.Result{}, err
+	if reconcileError := r.reconcileStatus(instance); reconcileError != nil {
+		return ctrl.Result{}, reconcileError
 	}
 
-	// If deployment created successfully - don't requeue
-	return ctrl.Result{}, nil
+	return ctrl.Result{}, err
 }
 
 // SetupWithManager sets up the controller with the Manager.

controllers/kfdef.apps.kubeflow.org/status.go

@@ -40,7 +40,11 @@ func (r *KfDefReconciler) reconcileStatus(cr *kfdefv1.KfDef) error {
 func getReconcileStatus(cr *kfdefv1.KfDef, err error) error {
 	conditions := []kfdefv1.KfDefCondition{}
 
+	availabilityStatus := corev1.ConditionTrue
+
 	if err != nil {
+		availabilityStatus = corev1.ConditionFalse
+
 		conditions = append(conditions, kfdefv1.KfDefCondition{
 			LastUpdateTime: cr.CreationTimestamp,
 			Status:         corev1.ConditionTrue,
@@ -51,7 +55,7 @@ func getReconcileStatus(cr *kfdefv1.KfDef, err error) error {
 
 	conditions = append(conditions, kfdefv1.KfDefCondition{
 		LastUpdateTime: cr.CreationTimestamp,
-		Status:         corev1.ConditionTrue,
+		Status:         availabilityStatus,
 		Reason:         DeploymentCompleted,
 		Type:           kfdefv1.KfAvailable,
 	})

pkg/kfapp/coordinator/coordinator.go

@@ -404,6 +404,22 @@ func (kfapp *coordinator) Apply(resources kftypesv3.ResourceEnum) error {
 		}
 	}
 
+	serviceMeshConfig := func() error {
+		if kfapp.KfDef.Spec.Platform != kftypesv3.OSSM {
+			return nil
+		}
+
+		if p, ok := kfapp.Platforms[kfapp.KfDef.Spec.Platform]; !ok {
+			return &kfapis.KfError{
+				Code:    int(kfapis.INTERNAL_ERROR),
+				Message: "Platform OSSM specified but not loaded.",
+			}
+		} else {
+			ossmInstaller := p.(*ossm.OssmInstaller)
+			return ossmInstaller.Apply(kftypesv3.K8S)
+		}
+	}
+
 	if err := kfapp.KfDef.SyncCache(); err != nil {
 		return &kfapis.KfError{
 			Code:    int(kfapis.INTERNAL_ERROR),
@@ -423,6 +439,9 @@ func (kfapp *coordinator) Apply(resources kftypesv3.ResourceEnum) error {
 	case kftypesv3.PLATFORM:
 		return platform()
 	case kftypesv3.K8S:
+		if err := serviceMeshConfig(); err != nil {
+			return err
+		}
 		if err := k8s(); err != nil {
 			return err
 		}
@@ -471,7 +490,7 @@ func (kfapp *coordinator) Delete(resources kftypesv3.ResourceEnum) error {
 		return nil
 	}
 
-	ossmCleanup := func() error {
+	serviceMeshCleanup := func() error {
 		if kfapp.KfDef.Spec.Platform != kftypesv3.OSSM {
 			return nil
 		}
@@ -483,7 +502,7 @@ func (kfapp *coordinator) Delete(resources kftypesv3.ResourceEnum) error {
 			}
 		} else {
 			ossmInstaller := p.(*ossm.OssmInstaller)
-			return ossmInstaller.CleanupResources()
+			return ossmInstaller.Delete(kftypesv3.K8S)
 		}
 	}
 
@@ -515,7 +534,7 @@ func (kfapp *coordinator) Delete(resources kftypesv3.ResourceEnum) error {
 		if err := k8s(); err != nil {
 			return err
 		}
-		return ossmCleanup()
+		return serviceMeshCleanup()
 	}
 	return nil
 }

pkg/kfapp/ossm/feature/builder.go

@@ -3,6 +3,7 @@ package feature
 import (
 	"github.com/opendatahub-io/opendatahub-operator/pkg/kfconfig/ossmplugin"
 	"github.com/pkg/errors"
+	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
@@ -53,6 +54,10 @@ func (fb *featureBuilder) UsingConfig(config *rest.Config) *featureBuilder {
 			return errors.WithStack(err)
 		}
 
+		if err := apiextv1.AddToScheme(f.client.Scheme()); err != nil {
+			return err
+		}
+
 		return nil
 	})
 
@@ -99,6 +104,16 @@ func (fb *featureBuilder) Preconditions(preconditions ...action) *featureBuilder
 	return fb
 }
 
+func (fb *featureBuilder) Postconditions(postconditions ...action) *featureBuilder {
+	fb.builders = append(fb.builders, func(f *Feature) error {
+		f.postconditions = append(f.postconditions, postconditions...)
+
+		return nil
+	})
+
+	return fb
+}
+
 func (fb *featureBuilder) OnDelete(cleanups ...action) *featureBuilder {
 	fb.builders = append(fb.builders, func(f *Feature) error {
 		f.addCleanup(cleanups...)
@@ -119,9 +134,20 @@ func (fb *featureBuilder) WithResources(resources ...action) *featureBuilder {
 	return fb
 }
 
+func (fb *featureBuilder) EnabledIf(enabled func(f *Feature) bool) *featureBuilder {
+	fb.builders = append(fb.builders, func(f *Feature) error {
+		f.Enabled = enabled(f)
+
+		return nil
+
+	})
+	return fb
+}
+
 func (fb *featureBuilder) Load() (*Feature, error) {
 	feature := &Feature{
-		Name: fb.name,
+		Name:    fb.name,
+		Enabled: true,
 	}
 
 	for i := range fb.builders {
@@ -130,8 +156,10 @@ func (fb *featureBuilder) Load() (*Feature, error) {
 		}
 	}
 
-	if err := feature.createResourceTracker(); err != nil {
-		return nil, err
+	if feature.Enabled {
+		if err := feature.createResourceTracker(); err != nil {
+			return feature, err
+		}
 	}
 
 	return feature, nil

pkg/kfapp/ossm/feature/cleanup.go

@@ -9,19 +9,19 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
+var smcpGVR = schema.GroupVersionResource{
+	Group:    "maistra.io",
+	Version:  "v2",
+	Resource: "servicemeshcontrolplanes",
+}
+
 func RemoveTokenVolumes(feature *Feature) error {
 	tokenVolume := fmt.Sprintf("%s-oauth2-tokens", feature.Spec.AppNamespace)
 
-	gvr := schema.GroupVersionResource{
-		Group:    "maistra.io",
-		Version:  "v2",
-		Resource: "servicemeshcontrolplanes",
-	}
-
 	meshNs := feature.Spec.Mesh.Namespace
 	meshName := feature.Spec.Mesh.Name
 
-	smcp, err := feature.dynamicClient.Resource(gvr).Namespace(meshNs).Get(context.Background(), meshName, metav1.GetOptions{})
+	smcp, err := feature.dynamicClient.Resource(smcpGVR).Namespace(meshNs).Get(context.Background(), meshName, metav1.GetOptions{})
 	if err != nil {
 		return err
 	}
@@ -37,7 +37,7 @@ func RemoveTokenVolumes(feature *Feature) error {
 	for i, v := range volumes {
 		volume, ok := v.(map[string]interface{})
 		if !ok {
-			fmt.Println("Unexpected type for volume")
+			log.Info("unexpected type for volume", "type", fmt.Sprintf("%T", volume))
 			continue
 		}
 
@@ -46,7 +46,7 @@ func RemoveTokenVolumes(feature *Feature) error {
 			return err
 		}
 		if !found {
-			fmt.Println("No volumeMount found in the volume")
+			log.Info("no volumeMount found in the volume")
 			continue
 		}
 
@@ -60,12 +60,9 @@ func RemoveTokenVolumes(feature *Feature) error {
 		}
 	}
 
-	_, err = feature.dynamicClient.Resource(gvr).Namespace(meshNs).Update(context.Background(), smcp, metav1.UpdateOptions{})
-	if err != nil {
-		return err
-	}
+	_, err = feature.dynamicClient.Resource(smcpGVR).Namespace(meshNs).Update(context.Background(), smcp, metav1.UpdateOptions{})
 
-	return nil
+	return err
 }
 
 func RemoveOAuthClient(feature *Feature) error {
@@ -86,6 +83,7 @@ func RemoveOAuthClient(feature *Feature) error {
 
 	if err := feature.dynamicClient.Resource(gvr).Delete(context.Background(), oauthClientName, metav1.DeleteOptions{}); err != nil {
 		log.Error(err, "failed deleting OAuthClient", "name", oauthClientName)
+
 		return err
 	}
 
@@ -95,15 +93,9 @@ func RemoveOAuthClient(feature *Feature) error {
 func RemoveExtensionProvider(feature *Feature) error {
 	ossmAuthzProvider := fmt.Sprintf("%s-odh-auth-provider", feature.Spec.AppNamespace)
 
-	gvr := schema.GroupVersionResource{
-		Group:    "maistra.io",
-		Version:  "v2",
-		Resource: "servicemeshcontrolplanes",
-	}
-
 	mesh := feature.Spec.Mesh
 
-	smcp, err := feature.dynamicClient.Resource(gvr).
+	smcp, err := feature.dynamicClient.Resource(smcpGVR).
 		Namespace(mesh.Namespace).
 		Get(context.Background(), mesh.Name, metav1.GetOptions{})
 	if err != nil {
@@ -136,7 +128,7 @@ func RemoveExtensionProvider(feature *Feature) error {
 		}
 	}
 
-	_, err = feature.dynamicClient.Resource(gvr).
+	_, err = feature.dynamicClient.Resource(smcpGVR).
 		Namespace(mesh.Namespace).
 		Update(context.Background(), smcp, metav1.UpdateOptions{})
 

pkg/kfapp/ossm/feature/feature.go

@@ -23,8 +23,9 @@ import (
 var log = ctrlLog.Log.WithName("ossm-features")
 
 type Feature struct {
-	Name string
-	Spec *Spec
+	Name    string
+	Spec    *Spec
+	Enabled bool
 
 	clientset     *kubernetes.Clientset
 	dynamicClient dynamic.Interface
@@ -42,6 +43,13 @@ type Feature struct {
 type action func(feature *Feature) error
 
 func (f *Feature) Apply() error {
+
+	if !f.Enabled {
+		log.Info("feature is disabled, skipping.", "name", f.Name)
+
+		return nil
+	}
+
 	// Verify all precondition and collect errors
 	var multiErr *multierror.Error
 	for _, precondition := range f.preconditions {
@@ -80,12 +88,24 @@ func (f *Feature) Apply() error {
 		return err
 	}
 
-	// TODO postconditions
+	for _, postcondition := range f.postconditions {
+		multiErr = multierror.Append(multiErr, postcondition(f))
+	}
+
+	if multiErr.ErrorOrNil() != nil {
+		return multiErr.ErrorOrNil()
+	}
 
 	return nil
 }
 
 func (f *Feature) Cleanup() error {
+	if !f.Enabled {
+		log.Info("feature is disabled, skipping.", "name", f.Name)
+
+		return nil
+	}
+
 	var cleanupErrors *multierror.Error
 	for _, cleanupFunc := range f.cleanups {
 		cleanupErrors = multierror.Append(cleanupErrors, cleanupFunc(f))

pkg/kfapp/ossm/feature/resources.go

@@ -113,15 +113,16 @@ func ServiceMeshEnabledInDashboard(feature *Feature) error {
 	dashboardConfig := spec["dashboardConfig"].(map[string]interface{})
 	dashboardConfig["disableServiceMesh"] = false
 
-	_, err = feature.dynamicClient.Resource(gvr).
+	if _, err := feature.dynamicClient.Resource(gvr).
 		Namespace(feature.Spec.AppNamespace).
-		Update(context.Background(), &config, metav1.UpdateOptions{})
-	if err != nil {
+		Update(context.Background(), &config, metav1.UpdateOptions{}); err != nil {
 		log.Error(err, "Failed to update odhdashboardconfig")
+
 		return err
 	}
 
 	log.Info("Successfully patched odhdashboardconfig")
+
 	return nil
 }