diff --git a/apis/ossm.plugins.kubeflow.org/v1alpha1/ossm_types.go b/apis/ossm.plugins.kubeflow.org/v1alpha1/ossm_types.go index 32cb7abef20..b02ef767a5c 100644 --- a/apis/ossm.plugins.kubeflow.org/v1alpha1/ossm_types.go +++ b/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 { diff --git a/config/crd/bases/ossm.plugins.kubeflow.org_ossmplugins.yaml b/config/crd/bases/ossm.plugins.kubeflow.org_ossmplugins.yaml index 59b16a11552..9c316ab4ea3 100644 --- a/config/crd/bases/ossm.plugins.kubeflow.org_ossmplugins.yaml +++ b/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: diff --git a/config/crd/bases/ossmplugin.internal.kubeflow.org_kfossmplugins.yaml b/config/crd/bases/ossmplugin.internal.kubeflow.org_kfossmplugins.yaml index 832c0ce65a3..837f0fdaf37 100644 --- a/config/crd/bases/ossmplugin.internal.kubeflow.org_kfossmplugins.yaml +++ b/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: diff --git a/controllers/kfdef.apps.kubeflow.org/kfdef_controller.go b/controllers/kfdef.apps.kubeflow.org/kfdef_controller.go index ab92f23b11a..e6399c2e719 100644 --- a/controllers/kfdef.apps.kubeflow.org/kfdef_controller.go +++ b/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. diff --git a/controllers/kfdef.apps.kubeflow.org/status.go b/controllers/kfdef.apps.kubeflow.org/status.go index 3aca3f9d2ca..23b07408df6 100644 --- a/controllers/kfdef.apps.kubeflow.org/status.go +++ b/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, }) diff --git a/pkg/kfapp/coordinator/coordinator.go b/pkg/kfapp/coordinator/coordinator.go index cb617866ae6..01d68611d50 100644 --- a/pkg/kfapp/coordinator/coordinator.go +++ b/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 } diff --git a/pkg/kfapp/ossm/feature/builder.go b/pkg/kfapp/ossm/feature/builder.go index 2218b02a76a..adcb1b99b58 100644 --- a/pkg/kfapp/ossm/feature/builder.go +++ b/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 diff --git a/pkg/kfapp/ossm/feature/cleanup.go b/pkg/kfapp/ossm/feature/cleanup.go index 1928c13afc7..38d5315aaa7 100644 --- a/pkg/kfapp/ossm/feature/cleanup.go +++ b/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{}) diff --git a/pkg/kfapp/ossm/feature/feature.go b/pkg/kfapp/ossm/feature/feature.go index b7419221244..6099c28d6cf 100644 --- a/pkg/kfapp/ossm/feature/feature.go +++ b/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)) diff --git a/pkg/kfapp/ossm/feature/resources.go b/pkg/kfapp/ossm/feature/resources.go index 439b8a78b7e..8d3c97d1d91 100644 --- a/pkg/kfapp/ossm/feature/resources.go +++ b/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 } diff --git a/pkg/kfapp/ossm/feature/verification.go b/pkg/kfapp/ossm/feature/verification.go index 752de668569..1dfff28b236 100644 --- a/pkg/kfapp/ossm/feature/verification.go +++ b/pkg/kfapp/ossm/feature/verification.go @@ -2,69 +2,77 @@ package feature import ( "context" + "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/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/dynamic" + "sigs.k8s.io/controller-runtime/pkg/client" + "time" ) -func EnsureCRDIsInstalled(group string, version string, resource string) action { +func EnsureCRDIsInstalled(name string) action { return func(f *Feature) error { - crdGVR := schema.GroupVersionResource{ - Group: group, - Version: version, - Resource: resource, - } - - _, err := f.dynamicClient.Resource(crdGVR).List(context.Background(), metav1.ListOptions{}) - - return err + return f.client.Get(context.Background(), client.ObjectKey{Name: name}, &apiextv1.CustomResourceDefinition{}) } } func EnsureServiceMeshInstalled(feature *Feature) error { - if err := EnsureCRDIsInstalled("maistra.io", "v2", "servicemeshcontrolplanes")(feature); err != nil { - log.Info("Failed to find the pre-requisite SMCP CRD, please ensure OSSM operator is installed.") + if err := EnsureCRDIsInstalled("servicemeshcontrolplanes.maistra.io")(feature); err != nil { + log.Info("Failed to find the pre-requisite Service Mesh Control Plane CRD, please ensure Service Mesh Operator is installed.") + return err } smcp := feature.Spec.Mesh.Name smcpNs := feature.Spec.Mesh.Namespace - status, err := checkSMCPStatus(feature.dynamicClient, smcp, smcpNs) - if err != nil { - log.Info("An error occurred while checking SMCP status - ensure the SMCP referenced exists.") - return err - } - if status != "Ready" { - log.Info("The referenced SMCP is not ready.", "name", smcp, "namespace", smcpNs) - return errors.New("SMCP status is not ready") + ready, err := CheckControlPlaneComponentReadiness(feature.dynamicClient, smcp, smcpNs) + if err != nil || !ready { + log.Error(err, "failed waiting for control plane being ready", "name", smcp, "namespace", smcpNs) + + return multierror.Append(err, errors.New("service mesh control plane is not ready")).ErrorOrNil() } - return nil + return nil } -func checkSMCPStatus(dynamicClient dynamic.Interface, name, namespace string) (string, error) { - gvr := schema.GroupVersionResource{ - Group: "maistra.io", - Version: "v1", - Resource: "servicemeshcontrolplanes", - } +const ( + interval = 5 * time.Second + duration = 5 * time.Minute +) + +func WaitForControlPlaneToBeReady(feature *Feature) error { + return wait.PollImmediate(interval, duration, func() (done bool, err error) { + smcp := feature.Spec.Mesh.Name + smcpNs := feature.Spec.Mesh.Namespace - unstructObj, err := dynamicClient.Resource(gvr).Namespace(namespace).Get(context.TODO(), name, metav1.GetOptions{}) + log.Info("waiting for control plane components to be ready", "name", smcp, "namespace", smcpNs, "duration (s)", duration.Seconds()) + + return CheckControlPlaneComponentReadiness(feature.dynamicClient, smcp, smcpNs) + }) +} + +func CheckControlPlaneComponentReadiness(dynamicClient dynamic.Interface, smcp, smcpNs string) (bool, error) { + unstructObj, err := dynamicClient.Resource(smcpGVR).Namespace(smcpNs).Get(context.Background(), smcp, metav1.GetOptions{}) if err != nil { - log.Info("Failed to find SMCP") - return "", err + log.Info("failed to find Service Mesh Control Plane", "name", smcp, "namespace", smcpNs) + + return false, err } - conditions, found, err := unstructured.NestedSlice(unstructObj.Object, "status", "conditions") + components, found, err := unstructured.NestedMap(unstructObj.Object, "status", "readiness", "components") if err != nil || !found { - log.Info("status conditions not found or error in parsing of SMCP") - return "", err + log.Info("status conditions not found or error in parsing of Service Mesh Control Plane") + + return false, err } - lastCondition := conditions[len(conditions)-1].(map[string]interface{}) - status := lastCondition["type"].(string) - return status, nil + readyComponents := len(components["ready"].([]interface{})) + pendingComponents := len(components["pending"].([]interface{})) + unreadyComponents := len(components["unready"].([]interface{})) + + return pendingComponents == 0 && unreadyComponents == 0 && readyComponents > 0, nil } diff --git a/pkg/kfapp/ossm/ossm_installer.go b/pkg/kfapp/ossm/ossm_installer.go index 4acfe52e2f5..e191f74e5f8 100644 --- a/pkg/kfapp/ossm/ossm_installer.go +++ b/pkg/kfapp/ossm/ossm_installer.go @@ -58,6 +58,8 @@ func (o *OssmInstaller) GetPluginSpec() (*ossmplugin.OssmPluginSpec, error) { return o.PluginSpec, nil } +// Init performs validation of the plugin spec and ensures all resources, +// such as features and their templates, are processed and initialized. func (o *OssmInstaller) Init(_ kftypesv3.ResourceEnum) error { if o.KfConfig.Spec.SkipInitProject { log.Info("Skipping init phase", "plugin", PluginName) @@ -69,7 +71,9 @@ func (o *OssmInstaller) Init(_ kftypesv3.ResourceEnum) error { return internalError(errors.WithStack(err)) } - pluginSpec.SetDefaults() + if err := pluginSpec.SetDefaults(); err != nil { + return internalError(errors.WithStack(err)) + } if valid, reason := pluginSpec.IsValid(); !valid { return internalError(errors.New(reason)) @@ -89,6 +93,27 @@ func (o *OssmInstaller) enableFeatures() error { return internalError(errors.WithStack(err)) } + if smcpInstallation, err := feature.CreateFeature("control-plane-install-managed"). + For(o.PluginSpec). + UsingConfig(o.config). + Manifests( + path.Join(rootDir, feature.ControlPlaneDir, "control-plane-minimal.tmpl"), + ). + EnabledIf(func(f *feature.Feature) bool { + return f.Spec.Mesh.InstallationMode != ossmplugin.PreInstalled + }). + Preconditions( + feature.EnsureCRDIsInstalled("servicemeshcontrolplanes.maistra.io"), + ). + Postconditions( + feature.WaitForControlPlaneToBeReady, + ). + Load(); err != nil { + return err + } else { + o.features = append(o.features, smcpInstallation) + } + if oauth, err := feature.CreateFeature("control-plane-configure-oauth"). For(o.PluginSpec). UsingConfig(o.config). @@ -103,14 +128,13 @@ func (o *OssmInstaller) enableFeatures() error { ). WithData(feature.ClusterDetails, feature.OAuthConfig). Preconditions( - feature.EnsureCRDIsInstalled("operator.authorino.kuadrant.io", "v1beta1", "authorinos"), feature.EnsureServiceMeshInstalled, ). OnDelete( feature.RemoveOAuthClient, feature.RemoveTokenVolumes, ).Load(); err != nil { - return nil + return err } else { o.features = append(o.features, oauth) } @@ -170,6 +194,10 @@ func (o *OssmInstaller) enableFeatures() error { path.Join(rootDir, feature.AuthDir, "mesh-authz-ext-provider.patch.tmpl"), ). WithData(feature.ClusterDetails). + Preconditions( + feature.EnsureCRDIsInstalled("authconfigs.authorino.kuadrant.io"), + feature.EnsureServiceMeshInstalled, + ). OnDelete(feature.RemoveExtensionProvider). Load(); err != nil { return err @@ -180,7 +208,12 @@ func (o *OssmInstaller) enableFeatures() error { return nil } -func (o *OssmInstaller) Generate(_ kftypesv3.ResourceEnum) error { +// Apply is implemented as a patched logic in coordinator.go similar to already existing GCP one. +// Plugins called from within the Kubernetes are not invoked in this particular phase by default. +// In order to prevent the accumulation of unmanaged resources in the event of a failure, +// the use of Generate() alone is not viable. This is due to its consequence of prematurely halting +// the Delete() chain without invoking our associated Delete hook. +func (o *OssmInstaller) Apply(_ kftypesv3.ResourceEnum) error { var applyErrors *multierror.Error for _, f := range o.features { @@ -191,15 +224,35 @@ func (o *OssmInstaller) Generate(_ kftypesv3.ResourceEnum) error { return applyErrors.ErrorOrNil() } -func (o *OssmInstaller) CleanupResources() error { +// Delete is implemented as a patched logic in coordinator.go similar to the Apply(). +// Plugins called from within the Kubernetes are not invoked in this particular phase by default. +// Because we create resources we need to clean them up on deletion of KfDef though. +func (o *OssmInstaller) Delete(_ kftypesv3.ResourceEnum) error { var cleanupErrors *multierror.Error - for _, f := range o.features { - cleanupErrors = multierror.Append(cleanupErrors, f.Cleanup()) + // Performs cleanups in reverse order (stack), this way e.g. we can unpatch SMCP before deleting it (if managed) + // Though it sounds unnecessary it keeps features isolated and there is no need to rely on the InstallationMode + // between the features when it comes to clean-up. This is based on the assumption, that features + // are created in the correct order or are self-contained. + for i := len(o.features) - 1; i >= 0; i-- { + log.Info("cleanup", "name", o.features[i].Name) + cleanupErrors = multierror.Append(cleanupErrors, o.features[i].Cleanup()) } return cleanupErrors.ErrorOrNil() } +// Generate function is not utilized by the plugin. This is because, in the event of an error, using Generate() +// would lead to the complete omission of the Delete() call. +// This, in turn, would leave resources created by it dangling in the cluster. +func (o *OssmInstaller) Generate(_ kftypesv3.ResourceEnum) error { + return nil +} + +// Dump is unused. Plugins called from within the Kubernetes are not invoked in this particular phase by default. +func (o *OssmInstaller) Dump(_ kftypesv3.ResourceEnum) error { + return nil +} + func internalError(err error) error { return &kfapisv3.KfError{ Code: int(kfapisv3.INTERNAL_ERROR), diff --git a/pkg/kfapp/ossm/ossm_installer_noop.go b/pkg/kfapp/ossm/ossm_installer_noop.go deleted file mode 100644 index b4a515e601c..00000000000 --- a/pkg/kfapp/ossm/ossm_installer_noop.go +++ /dev/null @@ -1,24 +0,0 @@ -package ossm - -import kftypesv3 "github.com/opendatahub-io/opendatahub-operator/apis/apps" - -// Below are the functions which are not used/executed at this point. -// They're here to satisfy the Plugin interface. - -func (o *OssmInstaller) Apply(resources kftypesv3.ResourceEnum) error { - // Plugins invoked within k8s (as a platform) won't be participating in Apply - // This is responsibility of PackageManagers - in this case kustomize - return nil -} - -func (o *OssmInstaller) Delete(resources kftypesv3.ResourceEnum) error { - // Plugins invoked within k8s (as a platform) won't be participating in Delete - // This is responsibility of PackageManagers - in this case kustomize - return nil -} - -func (o *OssmInstaller) Dump(resources kftypesv3.ResourceEnum) error { - // Plugins invoked within k8s (as a platform) won't be participating in Dump - // This is responsibility of PackageManagers - in this case kustomize - return nil -} diff --git a/pkg/kfapp/ossm/templates/control-plane/control-plane-minimal.tmpl b/pkg/kfapp/ossm/templates/control-plane/control-plane-minimal.tmpl new file mode 100644 index 00000000000..d0719fef5f0 --- /dev/null +++ b/pkg/kfapp/ossm/templates/control-plane/control-plane-minimal.tmpl @@ -0,0 +1,19 @@ +apiVersion: maistra.io/v2 +kind: ServiceMeshControlPlane +metadata: + name: {{ .Mesh.Name }} + namespace: {{ .Mesh.Namespace }} +spec: + version: v2.4 + tracing: + type: None + addons: + prometheus: + enabled: false + grafana: + enabled: false + jaeger: + name: jaeger + kiali: + name: kiali + enabled: false diff --git a/pkg/kfconfig/ossmplugin/types.go b/pkg/kfconfig/ossmplugin/types.go index d4d4f9ceed3..d501bcb3a41 100644 --- a/pkg/kfconfig/ossmplugin/types.go +++ b/pkg/kfconfig/ossmplugin/types.go @@ -1,6 +1,7 @@ package ossmplugin import ( + "github.com/pkg/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "reflect" ) @@ -25,10 +26,25 @@ type OssmPluginSpec struct { AppNamespace string `json:"appNamespace,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" +) + type MeshSpec struct { - Name string `json:"name,omitempty" default:"basic"` - Namespace string `json:"namespace,omitempty" default:"istio-system"` - Certificate CertSpec `json:"certificate,omitempty"` + Name string `json:"name,omitempty" default:"basic"` + Namespace string `json:"namespace,omitempty" default:"istio-system"` + InstallationMode InstallationMode `json:"installationMode,omitempty" default:"pre-installed"` + Certificate CertSpec `json:"certificate,omitempty"` } type CertSpec struct { @@ -59,11 +75,11 @@ func (plugin *OssmPluginSpec) IsValid() (bool, string) { return true, "" } -func (plugin *OssmPluginSpec) SetDefaults() { - setDefaults(plugin) +func (plugin *OssmPluginSpec) SetDefaults() error { + return setDefaults(plugin) } -func setDefaults(obj interface{}) { +func setDefaults(obj interface{}) error { value := reflect.ValueOf(obj).Elem() typ := value.Type() @@ -71,15 +87,26 @@ func setDefaults(obj interface{}) { field := value.Field(i) tag := typ.Field(i).Tag.Get("default") - if tag != "" && field.CanSet() && isEmptyValue(field) { - defaultValue := reflect.ValueOf(tag) - field.Set(defaultValue) + if field.Kind() == reflect.Struct { + if err := setDefaults(field.Addr().Interface()); err != nil { + return err + } } - if field.Kind() == reflect.Struct { - setDefaults(field.Addr().Interface()) + if tag != "" && field.IsValid() && field.CanSet() && isEmptyValue(field) { + defaultValue := reflect.ValueOf(tag) + targetType := field.Type() + if defaultValue.Type().ConvertibleTo(targetType) { + convertedValue := defaultValue.Convert(targetType) + field.Set(convertedValue) + } else { + return errors.Errorf("unable to convert \"%s\" to %s\n", defaultValue, targetType.Name()) + } } + } + + return nil } func isEmptyValue(value reflect.Value) bool { diff --git a/tests/data/test-data.tar.gz b/tests/data/test-data.tar.gz index 5e3383f05d5..1727c8fe39e 100644 Binary files a/tests/data/test-data.tar.gz and b/tests/data/test-data.tar.gz differ diff --git a/tests/integration/ossm_installer_int_test.go b/tests/integration/ossm_installer_int_test.go index 0c19099d51e..b1db5c3990b 100644 --- a/tests/integration/ossm_installer_int_test.go +++ b/tests/integration/ossm_installer_int_test.go @@ -46,15 +46,13 @@ var _ = Describe("CRD presence verification", func() { It("should successfully check existing CRD", func() { // given example CRD installed into env from /ossm/test/crd/ - crdGroup := "ossm.plugins.kubeflow.org" - crdVersion := "test-version" - crdResource := "test-resources" + name := "test-resources.ossm.plugins.kubeflow.org" var err error verificationFeature, err = feature.CreateFeature("CRD verification"). For(ossmPluginSpec). UsingConfig(envTest.Config). - Preconditions(feature.EnsureCRDIsInstalled(crdGroup, crdVersion, crdResource)). + Preconditions(feature.EnsureCRDIsInstalled(name)). Load() Expect(err).ToNot(HaveOccurred()) @@ -67,15 +65,13 @@ var _ = Describe("CRD presence verification", func() { It("should fail to check non-existing CRD", func() { // given - crdGroup := "non-existing-group" - crdVersion := "non-existing-version" - crdResource := "non-existing-resource" + name := "non-existing-resource.non-existing-group.io" var err error verificationFeature, err = feature.CreateFeature("CRD verification"). For(ossmPluginSpec). UsingConfig(envTest.Config). - Preconditions(feature.EnsureCRDIsInstalled(crdGroup, crdVersion, crdResource)). + Preconditions(feature.EnsureCRDIsInstalled(name)). Load() Expect(err).ToNot(HaveOccurred()) @@ -84,7 +80,7 @@ var _ = Describe("CRD presence verification", func() { // then Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("server could not find the requested resource")) + Expect(err.Error()).To(ContainSubstring("\"non-existing-resource.non-existing-group.io\" not found")) }) }) @@ -224,6 +220,113 @@ var _ = Describe("Data Science Project Migration", func() { }) +var _ = Describe("Feature enablement", func() { + + var ( + objectCleaner *testenv.Cleaner + ossmInstaller *ossm.OssmInstaller + ossmPluginSpec *ossmplugin.OssmPluginSpec + name = "test-name" + namespace = "test-namespace" + ) + + Context("installing Service Mesh Control Plane", func() { + + BeforeEach(func() { + ossmInstaller = newOssmInstaller(namespace) + var err error + ossmPluginSpec, err = ossmInstaller.GetPluginSpec() + Expect(err).ToNot(HaveOccurred()) + + ossmPluginSpec.Mesh.Name = name + ossmPluginSpec.Mesh.Namespace = namespace + + objectCleaner = testenv.CreateCleaner(envTestClient, envTest.Config, timeout, interval) + }) + + It("should install control planed when enabled", func() { + // given + ns := createNamespace(namespace) + Expect(envTestClient.Create(context.Background(), ns)).To(Succeed()) + defer objectCleaner.DeleteAll(ns) + + ossmPluginSpec.Mesh.InstallationMode = ossmplugin.Minimal + + serviceMeshInstallation, err := feature.CreateFeature("control-plane-installation"). + For(ossmPluginSpec). + UsingConfig(envTest.Config). + Manifests(fromTestTmpDir(path.Join(feature.ControlPlaneDir, "control-plane-minimal.tmpl"))). + EnabledIf(func(f *feature.Feature) bool { + return f.Spec.Mesh.InstallationMode == ossmplugin.Minimal + }). + Load() + + Expect(err).ToNot(HaveOccurred()) + + // when + Expect(serviceMeshInstallation.Apply()).ToNot(HaveOccurred()) + + // then + controlPlane, err := getServiceMeshControlPlane(envTest.Config, namespace, name) + Expect(err).ToNot(HaveOccurred()) + Expect(controlPlane.GetName()).To(Equal(name)) + }) + + It("should not install control plane when disabled", func() { + // given + ns := createNamespace(namespace) + Expect(envTestClient.Create(context.Background(), ns)).To(Succeed()) + defer objectCleaner.DeleteAll(ns) + + serviceMeshInstallation, err := feature.CreateFeature("control-plane-installation"). + For(ossmPluginSpec). + UsingConfig(envTest.Config). + Manifests(fromTestTmpDir(path.Join(feature.ControlPlaneDir, "control-plane-minimal.tmpl"))). + EnabledIf(func(f *feature.Feature) bool { + return false + }). + Load() + + Expect(err).ToNot(HaveOccurred()) + + // when + Expect(serviceMeshInstallation.Apply()).ToNot(HaveOccurred()) + + // then + _, err = getServiceMeshControlPlane(envTest.Config, namespace, name) + Expect(errors.IsNotFound(err)).To(BeTrue()) + }) + + It("should not install control plane by default", func() { + // given + ns := createNamespace(namespace) + Expect(envTestClient.Create(context.Background(), ns)).To(Succeed()) + defer objectCleaner.DeleteAll(ns) + + Expect(ossmPluginSpec.SetDefaults()).ToNot(HaveOccurred()) + + serviceMeshInstallation, err := feature.CreateFeature("control-plane-installation"). + For(ossmPluginSpec). + UsingConfig(envTest.Config). + Manifests(fromTestTmpDir(path.Join(feature.ControlPlaneDir, "control-plane-minimal.tmpl"))). + EnabledIf(func(f *feature.Feature) bool { + return f.Spec.Mesh.InstallationMode != ossmplugin.PreInstalled + }). + Load() + + Expect(err).ToNot(HaveOccurred()) + + // when + Expect(serviceMeshInstallation.Apply()).ToNot(HaveOccurred()) + + // then + _, err = getServiceMeshControlPlane(envTest.Config, namespace, name) + Expect(errors.IsNotFound(err)).To(BeTrue()) + }) + }) + +}) + var _ = Describe("Cleanup operations", func() { Context("configuring control plane for auth(z)", func() { @@ -259,7 +362,7 @@ var _ = Describe("Cleanup operations", func() { controlPlaneWithSecretVolumes, err := feature.CreateFeature("control-plane-with-secret-volumes"). For(ossmPluginSpec). - Manifests(inTestTmpDir(path.Join(feature.ControlPlaneDir, "base/control-plane-ingress.patch.tmpl"))). + Manifests(fromTestTmpDir(path.Join(feature.ControlPlaneDir, "base/control-plane-ingress.patch.tmpl"))). UsingConfig(envTest.Config). Load() @@ -293,7 +396,7 @@ var _ = Describe("Cleanup operations", func() { controlPlaneWithExtAuthzProvider, err := feature.CreateFeature("control-plane-with-external-authz-provider"). For(ossmPluginSpec). - Manifests(inTestTmpDir(path.Join(feature.AuthDir, "mesh-authz-ext-provider.patch.tmpl"))). + Manifests(fromTestTmpDir(path.Join(feature.AuthDir, "mesh-authz-ext-provider.patch.tmpl"))). UsingConfig(envTest.Config). Load() @@ -419,7 +522,10 @@ func createSMCPInCluster(cfg *rest.Config, smcpObj *unstructured.Unstructured, n "readiness": map[string]interface{}{ "components": map[string]interface{}{ "pending": []interface{}{}, - "ready": []interface{}{}, + "ready": []interface{}{ + "istiod", + "ingress-gateway", + }, "unready": []interface{}{}, }, }, @@ -457,7 +563,7 @@ func getServiceMeshControlPlane(cfg *rest.Config, namespace, name string) (*unst return smcp, nil } -func inTestTmpDir(fileName string) string { +func fromTestTmpDir(fileName string) string { root, err := findProjectRoot() Expect(err).ToNot(HaveOccurred())