From 973ff743084030e3e73aab9aba9ac33d4f29ccb9 Mon Sep 17 00:00:00 2001 From: Bartosz Majsak Date: Wed, 10 Jul 2024 14:47:40 +0200 Subject: [PATCH] feat: reworks Feature to be generic data container (#1052) Feature Struct Improvements - Eliminates the `.Spec` field, eliminating a single point of coupling with various domain structs used across the project. - Introduces a generic map to store keys relevant to a given feature, which are utilized in templates and other functions. Feature Builder Changes - Renamed few functions to better reflect their intent - entry builder function `feature.CreateFeature` becomes `feature.Define` - Load() becomes Create() - `.WithData` builder function serves as an entry point to define the contents of a given feature. This chain method allows adding key-values to the feature's context. These values are later used in templates and can also be accessed in pre/post conditions and for programmatic resource creation. Additional Changes - Simplifies and decouples the use of `FeatureHandler`. It is no longer necessary to patss it to the Feature builder to group features together. - Introduces a new `FeatureRegistry` interface, allowing convenient addition of feature sets to the handler via the `FeatureProvider` function. - Introduces a convention for defining `FeatureData` for specific parts of the platform (e.g., Serverless or Service Mesh setup). This approach enhances readability and promotes the reuse of commonly used data entries. - Adds a `README.md` file providing a high-level overview of the `Feature DSL`. --- apis/features/v1/features_types.go | 1 + components/kserve/kserve_config_handler.go | 4 +- ...dictor-authorizationpolicy.patch.tmpl.yaml | 2 +- components/kserve/serverless_setup.go | 67 ++--- components/kserve/servicemesh_setup.go | 38 ++- .../resources/authorino/auth-smm.tmpl.yaml | 2 +- .../operator-cluster-wide-no-tls.tmpl.yaml | 2 +- .../deployment.injection.patch.tmpl.yaml | 2 +- .../mesh-authz-ext-provider.patch.tmpl.yaml | 4 +- .../dscinitialization/servicemesh_setup.go | 138 +++++----- pkg/feature/README.md | 237 ++++++++++++++++++ pkg/feature/builder.go | 130 +++++----- pkg/feature/feature.go | 96 ++++--- pkg/feature/feature_data.go | 81 ++++++ pkg/feature/feature_suite_test.go | 13 + pkg/feature/feature_tracker_handler.go | 18 +- pkg/feature/handler.go | 150 ++++++----- pkg/feature/manifest_test.go | 16 +- pkg/feature/provider/data_provider_test.go | 71 ++++++ pkg/feature/provider/provider_suite_test.go | 13 + pkg/feature/provider/types.go | 69 +++++ pkg/feature/serverless/data.go | 67 +++++ pkg/feature/serverless/loaders.go | 38 --- pkg/feature/serverless/resources.go | 53 +++- pkg/feature/servicemesh/cleanup.go | 27 +- pkg/feature/servicemesh/conditions.go | 26 +- pkg/feature/servicemesh/data.go | 116 +++++++++ pkg/feature/servicemesh/loaders.go | 33 --- pkg/feature/servicemesh/resources.go | 33 ++- pkg/feature/types.go | 28 --- .../integration/features/cleanup_int_test.go | 69 +++-- .../mesh-authz-ext-provider.patch.tmpl.yaml | 4 +- .../features/manifests_int_test.go | 33 ++- .../features/preconditions_int_test.go | 22 +- .../features/resources_int_test.go | 67 ++--- .../features/serverless_feature_test.go | 187 ++++++++------ .../features/servicemesh_feature_test.go | 78 +++--- .../integration/features/tracker_int_test.go | 56 ++--- 38 files changed, 1400 insertions(+), 691 deletions(-) create mode 100644 pkg/feature/README.md create mode 100644 pkg/feature/feature_data.go create mode 100644 pkg/feature/feature_suite_test.go create mode 100644 pkg/feature/provider/data_provider_test.go create mode 100644 pkg/feature/provider/provider_suite_test.go create mode 100644 pkg/feature/provider/types.go create mode 100644 pkg/feature/serverless/data.go delete mode 100644 pkg/feature/serverless/loaders.go create mode 100644 pkg/feature/servicemesh/data.go delete mode 100644 pkg/feature/servicemesh/loaders.go delete mode 100644 pkg/feature/types.go diff --git a/apis/features/v1/features_types.go b/apis/features/v1/features_types.go index 41eae967c71..5b6289d2f7e 100644 --- a/apis/features/v1/features_types.go +++ b/apis/features/v1/features_types.go @@ -60,6 +60,7 @@ var ConditionReason = struct { const ( ComponentType OwnerType = "Component" DSCIType OwnerType = "DSCI" + UnknownType OwnerType = "Unknown" ) func (s *FeatureTracker) ToOwnerReference() metav1.OwnerReference { diff --git a/components/kserve/kserve_config_handler.go b/components/kserve/kserve_config_handler.go index c92bf11c30d..5f4ca66629e 100644 --- a/components/kserve/kserve_config_handler.go +++ b/components/kserve/kserve_config_handler.go @@ -140,7 +140,7 @@ func (k *Kserve) configureServerless(ctx context.Context, cli client.Client, ins return dependOpsErrors } - serverlessFeatures := feature.ComponentFeaturesHandler(k.GetComponentName(), instance, k.configureServerlessFeatures()) + serverlessFeatures := feature.ComponentFeaturesHandler(k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance)) if err := serverlessFeatures.Apply(ctx); err != nil { return err @@ -150,7 +150,7 @@ func (k *Kserve) configureServerless(ctx context.Context, cli client.Client, ins } func (k *Kserve) removeServerlessFeatures(ctx context.Context, instance *dsciv1.DSCInitializationSpec) error { - serverlessFeatures := feature.ComponentFeaturesHandler(k.GetComponentName(), instance, k.configureServerlessFeatures()) + serverlessFeatures := feature.ComponentFeaturesHandler(k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance)) return serverlessFeatures.Delete(ctx) } diff --git a/components/kserve/resources/servicemesh/z-migrations/kserve-predictor-authorizationpolicy.patch.tmpl.yaml b/components/kserve/resources/servicemesh/z-migrations/kserve-predictor-authorizationpolicy.patch.tmpl.yaml index aff5984cd53..0d141dd8d17 100644 --- a/components/kserve/resources/servicemesh/z-migrations/kserve-predictor-authorizationpolicy.patch.tmpl.yaml +++ b/components/kserve/resources/servicemesh/z-migrations/kserve-predictor-authorizationpolicy.patch.tmpl.yaml @@ -5,4 +5,4 @@ metadata: namespace: {{ .ControlPlane.Namespace }} spec: provider: - name: {{ .AppNamespace }}-auth-provider + name: {{ .AuthExtensionName }} diff --git a/components/kserve/serverless_setup.go b/components/kserve/serverless_setup.go index e71f7d5a95d..b54697ed38c 100644 --- a/components/kserve/serverless_setup.go +++ b/components/kserve/serverless_setup.go @@ -1,23 +1,26 @@ package kserve import ( - "context" "path" + dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/serverless" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/servicemesh" ) -func (k *Kserve) configureServerlessFeatures() feature.FeaturesProvider { - return func(handler *feature.FeaturesHandler) error { - servingDeploymentErr := feature.CreateFeature("serverless-serving-deployment"). - For(handler). +func (k *Kserve) configureServerlessFeatures(dsciSpec *dsciv1.DSCInitializationSpec) feature.FeaturesProvider { + return func(registry feature.FeaturesRegistry) error { + servingDeployment := feature.Define("serverless-serving-deployment"). ManifestsLocation(Resources.Location). Manifests( path.Join(Resources.InstallDir), ). - WithData(PopulateComponentSettings(k)). + WithData( + serverless.FeatureData.IngressDomain.Define(&k.Serving).AsAction(), + serverless.FeatureData.Serving.Define(&k.Serving).AsAction(), + servicemesh.FeatureData.ControlPlane.Define(dsciSpec).AsAction(), + ). PreConditions( serverless.EnsureServerlessOperatorInstalled, serverless.EnsureServerlessAbsent, @@ -26,53 +29,37 @@ func (k *Kserve) configureServerlessFeatures() feature.FeaturesProvider { ). PostConditions( feature.WaitForPodsToBeReady(serverless.KnativeServingNamespace), - ). - Load() - if servingDeploymentErr != nil { - return servingDeploymentErr - } + ) - servingNetIstioSecretFilteringErr := feature.CreateFeature("serverless-net-istio-secret-filtering"). - For(handler). + istioSecretFiltering := feature.Define("serverless-net-istio-secret-filtering"). ManifestsLocation(Resources.Location). Manifests( path.Join(Resources.BaseDir, "serving-net-istio-secret-filtering.patch.tmpl.yaml"), ). - WithData(PopulateComponentSettings(k)). + WithData(serverless.FeatureData.Serving.Define(&k.Serving).AsAction()). PreConditions(serverless.EnsureServerlessServingDeployed). PostConditions( feature.WaitForPodsToBeReady(serverless.KnativeServingNamespace), - ). - Load() - if servingNetIstioSecretFilteringErr != nil { - return servingNetIstioSecretFilteringErr - } + ) - serverlessGwErr := feature.CreateFeature("serverless-serving-gateways"). - For(handler). - PreConditions(serverless.EnsureServerlessServingDeployed). - WithData( - PopulateComponentSettings(k), - serverless.ServingDefaultValues, - serverless.ServingIngressDomain, - ). - WithResources(serverless.ServingCertificateResource). + servingGateway := feature.Define("serverless-serving-gateways"). ManifestsLocation(Resources.Location). Manifests( path.Join(Resources.GatewaysDir), ). - Load() - if serverlessGwErr != nil { - return serverlessGwErr - } - - return nil - } -} + WithData( + serverless.FeatureData.IngressDomain.Define(&k.Serving).AsAction(), + serverless.FeatureData.CertificateName.Define(&k.Serving).AsAction(), + serverless.FeatureData.Serving.Define(&k.Serving).AsAction(), + servicemesh.FeatureData.ControlPlane.Define(dsciSpec).AsAction(), + ). + WithResources(serverless.ServingCertificateResource). + PreConditions(serverless.EnsureServerlessServingDeployed) -func PopulateComponentSettings(k *Kserve) feature.Action { - return func(_ context.Context, f *feature.Feature) error { - f.Spec.Serving = &k.Serving - return nil + return registry.Add( + servingDeployment, + istioSecretFiltering, + servingGateway, + ) } } diff --git a/components/kserve/servicemesh_setup.go b/components/kserve/servicemesh_setup.go index 7a98c7a9009..6655abddde9 100644 --- a/components/kserve/servicemesh_setup.go +++ b/components/kserve/servicemesh_setup.go @@ -17,7 +17,7 @@ import ( func (k *Kserve) configureServiceMesh(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) error { if dscispec.ServiceMesh != nil { if dscispec.ServiceMesh.ManagementState == operatorv1.Managed && k.GetManagementState() == operatorv1.Managed { - serviceMeshInitializer := feature.ComponentFeaturesHandler(k.GetComponentName(), dscispec, k.defineServiceMeshFeatures(ctx, cli)) + serviceMeshInitializer := feature.ComponentFeaturesHandler(k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec)) return serviceMeshInitializer.Apply(ctx) } if dscispec.ServiceMesh.ManagementState == operatorv1.Unmanaged && k.GetManagementState() == operatorv1.Managed { @@ -29,21 +29,19 @@ func (k *Kserve) configureServiceMesh(ctx context.Context, cli client.Client, ds } func (k *Kserve) removeServiceMeshConfigurations(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) error { - serviceMeshInitializer := feature.ComponentFeaturesHandler(k.GetComponentName(), dscispec, k.defineServiceMeshFeatures(ctx, cli)) + serviceMeshInitializer := feature.ComponentFeaturesHandler(k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec)) return serviceMeshInitializer.Delete(ctx) } -func (k *Kserve) defineServiceMeshFeatures(ctx context.Context, cli client.Client) feature.FeaturesProvider { - return func(handler *feature.FeaturesHandler) error { +func (k *Kserve) defineServiceMeshFeatures(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) feature.FeaturesProvider { + return func(registry feature.FeaturesRegistry) error { authorinoInstalled, err := cluster.SubscriptionExists(ctx, cli, "authorino-operator") if err != nil { return fmt.Errorf("failed to list subscriptions %w", err) } if authorinoInstalled { - kserveExtAuthzErr := feature.CreateFeature("kserve-external-authz"). - For(handler). - Managed(). + kserveExtAuthzErr := registry.Add(feature.Define("kserve-external-authz"). ManifestsLocation(Resources.Location). Manifests( path.Join(Resources.ServiceMeshDir, "activator-envoyfilter.tmpl.yaml"), @@ -51,8 +49,15 @@ func (k *Kserve) defineServiceMeshFeatures(ctx context.Context, cli client.Clien path.Join(Resources.ServiceMeshDir, "kserve-predictor-authorizationpolicy.tmpl.yaml"), path.Join(Resources.ServiceMeshDir, "z-migrations"), ). - WithData(servicemesh.ClusterDetails). - Load() + Managed(). + WithData( + feature.Entry("Domain", cluster.GetDomain), + servicemesh.FeatureData.ControlPlane.Define(dscispec).AsAction(), + ). + WithData( + servicemesh.FeatureData.Authorization.All(dscispec)..., + ), + ) if kserveExtAuthzErr != nil { return kserveExtAuthzErr @@ -61,20 +66,13 @@ func (k *Kserve) defineServiceMeshFeatures(ctx context.Context, cli client.Clien fmt.Println("WARN: Authorino operator is not installed on the cluster, skipping authorization capability") } - temporaryFixesErr := feature.CreateFeature("kserve-temporary-fixes"). - For(handler). - Managed(). + return registry.Add(feature.Define("kserve-temporary-fixes"). ManifestsLocation(Resources.Location). Manifests( path.Join(Resources.ServiceMeshDir, "grpc-envoyfilter-temp-fix.tmpl.yaml"), ). - WithData(servicemesh.ClusterDetails). - Load() - - if temporaryFixesErr != nil { - return temporaryFixesErr - } - - return nil + Managed(). + WithData(servicemesh.FeatureData.ControlPlane.Define(dscispec).AsAction()), + ) } } diff --git a/controllers/dscinitialization/resources/authorino/auth-smm.tmpl.yaml b/controllers/dscinitialization/resources/authorino/auth-smm.tmpl.yaml index 6b0aa06aa82..f746b2fdaa9 100644 --- a/controllers/dscinitialization/resources/authorino/auth-smm.tmpl.yaml +++ b/controllers/dscinitialization/resources/authorino/auth-smm.tmpl.yaml @@ -2,7 +2,7 @@ apiVersion: maistra.io/v1 kind: ServiceMeshMember metadata: name: default - namespace: {{ .Auth.Namespace }} + namespace: {{ .AuthNamespace }} spec: controlPlaneRef: namespace: {{ .ControlPlane.Namespace }} diff --git a/controllers/dscinitialization/resources/authorino/base/operator-cluster-wide-no-tls.tmpl.yaml b/controllers/dscinitialization/resources/authorino/base/operator-cluster-wide-no-tls.tmpl.yaml index 5624128f904..8bd62ab94aa 100644 --- a/controllers/dscinitialization/resources/authorino/base/operator-cluster-wide-no-tls.tmpl.yaml +++ b/controllers/dscinitialization/resources/authorino/base/operator-cluster-wide-no-tls.tmpl.yaml @@ -2,7 +2,7 @@ apiVersion: operator.authorino.kuadrant.io/v1beta1 kind: Authorino metadata: name: {{ .AuthProviderName }} - namespace: {{ .Auth.Namespace }} + namespace: {{ .AuthNamespace }} spec: authConfigLabelSelectors: security.opendatahub.io/authorization-group=default clusterWide: true diff --git a/controllers/dscinitialization/resources/authorino/deployment.injection.patch.tmpl.yaml b/controllers/dscinitialization/resources/authorino/deployment.injection.patch.tmpl.yaml index 7040f76da79..3f1f6aa0daa 100644 --- a/controllers/dscinitialization/resources/authorino/deployment.injection.patch.tmpl.yaml +++ b/controllers/dscinitialization/resources/authorino/deployment.injection.patch.tmpl.yaml @@ -2,7 +2,7 @@ apiVersion: apps/v1 kind: Deployment metadata: name: {{ .AuthProviderName }} - namespace: {{ .Auth.Namespace }} + namespace: {{ .AuthNamespace }} spec: template: metadata: diff --git a/controllers/dscinitialization/resources/authorino/mesh-authz-ext-provider.patch.tmpl.yaml b/controllers/dscinitialization/resources/authorino/mesh-authz-ext-provider.patch.tmpl.yaml index 1a91092c7c2..fd2e906af19 100644 --- a/controllers/dscinitialization/resources/authorino/mesh-authz-ext-provider.patch.tmpl.yaml +++ b/controllers/dscinitialization/resources/authorino/mesh-authz-ext-provider.patch.tmpl.yaml @@ -7,7 +7,7 @@ spec: techPreview: meshConfig: extensionProviders: - - name: {{ .AppNamespace }}-auth-provider + - name: {{ .AuthExtensionName }} envoyExtAuthzGrpc: - service: {{ .AuthProviderName }}-authorino-authorization.{{ .Auth.Namespace }}.svc.cluster.local + service: {{ .AuthProviderName }}-authorino-authorization.{{ .AuthNamespace }}.svc.cluster.local port: 50051 diff --git a/controllers/dscinitialization/servicemesh_setup.go b/controllers/dscinitialization/servicemesh_setup.go index 21a8df55ab5..6b6aeb025d6 100644 --- a/controllers/dscinitialization/servicemesh_setup.go +++ b/controllers/dscinitialization/servicemesh_setup.go @@ -124,108 +124,100 @@ func (r *DSCInitializationReconciler) authorizationCapability(ctx context.Contex } func (r *DSCInitializationReconciler) serviceMeshCapabilityFeatures(instance *dsciv1.DSCInitialization) feature.FeaturesProvider { - return func(handler *feature.FeaturesHandler) (err error) { //nolint:lll,nonamedreturns // Reason: we use the named return to handle errors in a unified fashion through deferred function. + return func(registry feature.FeaturesRegistry) error { serviceMeshSpec := instance.Spec.ServiceMesh - smcpCreationErr := feature.CreateFeature("mesh-control-plane-creation"). - For(handler). + + smcp := feature.Define("mesh-control-plane-creation"). ManifestsLocation(Templates.Location). Manifests( path.Join(Templates.ServiceMeshDir), ). + WithData(servicemesh.FeatureData.ControlPlane.Define(&instance.Spec).AsAction()). PreConditions( servicemesh.EnsureServiceMeshOperatorInstalled, feature.CreateNamespaceIfNotExists(serviceMeshSpec.ControlPlane.Namespace), ). PostConditions( feature.WaitForPodsToBeReady(serviceMeshSpec.ControlPlane.Namespace), - ). - Load() - - if smcpCreationErr != nil { - return smcpCreationErr - } + ) if serviceMeshSpec.ControlPlane.MetricsCollection == "Istio" { - metricsCollectionErr := feature.CreateFeature("mesh-metrics-collection"). - For(handler). - PreConditions( - servicemesh.EnsureServiceMeshInstalled, - ). + metricsCollectionErr := registry.Add(feature.Define("mesh-metrics-collection"). ManifestsLocation(Templates.Location). Manifests( path.Join(Templates.MetricsDir), ). - Load() + WithData( + servicemesh.FeatureData.ControlPlane.Define(&instance.Spec).AsAction(), + ). + PreConditions( + servicemesh.EnsureServiceMeshInstalled, + )) + if metricsCollectionErr != nil { return metricsCollectionErr } } - cfgMapErr := feature.CreateFeature("mesh-shared-configmap"). - For(handler). + cfgMap := feature.Define("mesh-shared-configmap"). WithResources(servicemesh.MeshRefs, servicemesh.AuthRefs). - Load() - if cfgMapErr != nil { - return cfgMapErr - } + WithData( + servicemesh.FeatureData.ControlPlane.Define(&instance.Spec).AsAction(), + ). + WithData( + servicemesh.FeatureData.Authorization.All(&instance.Spec)..., + ) - return nil + return registry.Add(smcp, cfgMap) } } func (r *DSCInitializationReconciler) authorizationFeatures(instance *dsciv1.DSCInitialization) feature.FeaturesProvider { - return func(handler *feature.FeaturesHandler) error { + return func(registry feature.FeaturesRegistry) error { serviceMeshSpec := instance.Spec.ServiceMesh - errExtAuthz := feature.CreateFeature("mesh-control-plane-external-authz"). - For(handler). - ManifestsLocation(Templates.Location). - Manifests( - path.Join(Templates.AuthorinoDir, "auth-smm.tmpl.yaml"), - path.Join(Templates.AuthorinoDir, "base"), - path.Join(Templates.AuthorinoDir, "mesh-authz-ext-provider.patch.tmpl.yaml"), - ). - WithData(servicemesh.ClusterDetails). - PreConditions( - feature.EnsureOperatorIsInstalled("authorino-operator"), - servicemesh.EnsureServiceMeshInstalled, - servicemesh.EnsureAuthNamespaceExists, - ). - PostConditions( - feature.WaitForPodsToBeReady(serviceMeshSpec.ControlPlane.Namespace), - ). - OnDelete( - servicemesh.RemoveExtensionProvider, - ). - Load() - if errExtAuthz != nil { - return errExtAuthz - } - - // We do not have the control over deployment resource creation. - // It is created by Authorino operator using Authorino CR and labels are not propagated from Authorino CR to spec.template - // See https://issues.redhat.com/browse/RHOAIENG-5494 - // - // To make it part of Service Mesh we have to patch it with injection - // enabled instead, otherwise it will not have proxy pod injected. - errAuthorinoInjectionPatch := feature.CreateFeature("enable-proxy-injection-in-authorino-deployment"). - For(handler). - ManifestsLocation(Templates.Location). - Manifests( - path.Join(Templates.AuthorinoDir, "deployment.injection.patch.tmpl.yaml"), - ). - PreConditions( - servicemesh.EnsureAuthNamespaceExists, - func(ctx context.Context, f *feature.Feature) error { - return feature.WaitForPodsToBeReady(handler.DSCInitializationSpec.ServiceMesh.Auth.Namespace)(ctx, f) - }, - ). - Load() - - if errAuthorinoInjectionPatch != nil { - return errAuthorinoInjectionPatch - } - - return nil + return registry.Add( + feature.Define("mesh-control-plane-external-authz"). + ManifestsLocation(Templates.Location). + Manifests( + path.Join(Templates.AuthorinoDir, "auth-smm.tmpl.yaml"), + path.Join(Templates.AuthorinoDir, "base"), + path.Join(Templates.AuthorinoDir, "mesh-authz-ext-provider.patch.tmpl.yaml"), + ). + WithData( + servicemesh.FeatureData.ControlPlane.Define(&instance.Spec).AsAction(), + ). + WithData( + servicemesh.FeatureData.Authorization.All(&instance.Spec)..., + ). + PreConditions( + feature.EnsureOperatorIsInstalled("authorino-operator"), + servicemesh.EnsureServiceMeshInstalled, + servicemesh.EnsureAuthNamespaceExists, + ). + OnDelete( + servicemesh.RemoveExtensionProvider, + ), + + // We do not have the control over deployment resource creation. + // It is created by Authorino operator using Authorino CR and labels are not propagated from Authorino CR to spec.template + // See https://issues.redhat.com/browse/RHOAIENG-5494 + // + // To make it part of Service Mesh we have to patch it with injection + // enabled instead, otherwise it will not have proxy pod injected. + feature.Define("enable-proxy-injection-in-authorino-deployment"). + ManifestsLocation(Templates.Location). + Manifests( + path.Join(Templates.AuthorinoDir, "deployment.injection.patch.tmpl.yaml"), + ). + WithData(servicemesh.FeatureData.ControlPlane.Define(&instance.Spec).AsAction()). + WithData(servicemesh.FeatureData.Authorization.All(&instance.Spec)...). + PreConditions( + servicemesh.EnsureAuthNamespaceExists, + func(ctx context.Context, f *feature.Feature) error { + return feature.WaitForPodsToBeReady(serviceMeshSpec.Auth.Namespace)(ctx, f) + }, + ), + ) } } diff --git a/pkg/feature/README.md b/pkg/feature/README.md new file mode 100644 index 00000000000..c70c77baa4c --- /dev/null +++ b/pkg/feature/README.md @@ -0,0 +1,237 @@ +# Feature DSL + +`Feature` struct encapsulates a set of resources to be created and actions to be performed in the cluster in order to enable desired functionality. For example, this could involve deploying a service or configuring the cluster infrastructure necessary for a component to function properly. + +## Goals + +- Abstraction for self-contained units of desired changes in the cluster. +- Granular control of resource management. +- Intuitive way of defining desired cluster changes in the controller code. +- Support both programmatic and declarative means of changing cluster state. +- Detailed status reporting enables quick diagnosis of issues. + +## Overview + +```mermaid +%%{init: {'noteBkgColor': '#ffcc00', 'noteTextColor': '#000000', 'noteBorderColor': '#000000'}}%% +--- +title: Feature Domain Model +--- +classDiagram + direction LR + + namespace Controller { + class Feature { + <> + Precondtions <1> + Manifests <2> + Resources <3> + PostConditions <4> + CleanupHooks <5> + Data <6> + } + + + class FeatureTracker { + <> + } + } + + namespace Cluster { + class Resource { + <> + } + } + + Feature -- FeatureTracker: is associated with + FeatureTracker --> "1..*" Resource: owns + + note for FeatureTracker "Internal Custom Resource (not part of user-facing API)" +``` + +- `<1>` Preconditions which are needed for the feature to be successfully applied (e.g. existence of particular CRDs) +> [!NOTE] +> Although not strictly required due to the declarative nature of Kubernetes, it can improve error reporting and prevent the application of resources that would lead to subsequent errors. +- `<2>` Manifests (i.e. YAML files) to be applied on the cluster. These can be arbitrary files or Go templates. +- `<3>` Programmatic resource creation required by the feature. +- `<4>` Post-creation checks, for example waiting for pods to be ready. +- `<5>` Cleanup hooks (i.e. undoing patch changes) +- `<6>` Data store needed by templates and actions (functions) performed during feature's lifecycle. + +## Creating a Feature + +### Getting started + +The easiest way to define a feature is by using the Feature Builder (builder.go), which provides a guided flow through a fluent API, making the construction process straightforward and intuitive. + +```go +smcp, errSMCPCreate := feature.Define("mesh-control-plane-creation"). + TargetNamespace("opendatahub"). + ManifestsLocation(Templates.Location). + Manifests( + path.Join(Templates.ServiceMeshDir), + ). + WithData( + servicemesh.FeatureData.ControlPlane.Create(&dsci.Spec).AsAction(), + ). + PreConditions( + servicemesh.EnsureServiceMeshOperatorInstalled, + feature.CreateNamespaceIfNotExists(serviceMeshSpec.ControlPlane.Namespace), + ). + PostConditions( + feature.WaitForPodsToBeReady(serviceMeshSpec.ControlPlane.Namespace), + ). + Create() +``` + +For more examples have a look at `integration/feature` tests. + +### Using dynamic values + +It may be necessary to supply data that is only available at runtime, such as cluster configuration details. For this purpose, a `DataProviderFunc` can be utilized. + +```go +WithData( + feature.Entry("Domain", func (ctx context.Context, c client.Client) (string, error) { + //... fetch the data somehow + return domain, nil + }), +) +``` + +Default values can be applied when an expected field in the struct is "empty" by using utilities from the provider package, for example: + +```go +WithData( + feature.Entry("Secret", provider.ValueOf(spec.SecretName).OrElse(DefaultCertificateSecretName), +) +``` + +For more on how to further simplify re-use of Feature's context data see a [dedicated section about conventions](#feature-context-re-use). + +## Execution flow + +The diagram below depicts the flow when Feature is applied. + +```mermaid +flowchart TD + Data[Resolve Feature Data] + PC[Pre-condition check] + R[Create Resources] + M[Apply manifests] + P[Post-condition check] + E(((Error))) + S(((Feature applied))) + + Data --> OK1{OK} + OK1 -->|No| E + OK1 -->|Yes|PC + PC --> OK2{OK} + OK2 -->|No| E + OK2 -->|Yes| R + R --> M + M --> OK3{OK} + OK3 -->|No| E + OK3 -->|Yes| P + P --> OK4{OK} + OK4 -->|Yes| S + OK4 -->|No| E +``` + +## Feature Tracker + +`FeatureTracker` is an internal CRD, not intended to be used in user-facing API. Its primary goal is to establish ownership of all resources that are part of the given feature. This way we can transparently +garbage collect them when feature is no longer needed. + +Additionally, it updates the `.status` field with detailed information about the Feature's lifecycle operations. This can be useful for troubleshooting, as it indicates which part of the feature application process is failing. + +## Managing Features with `FeaturesHandler` + +The `FeaturesHandler` (`handler.go`) provides a structured way to manage and coordinate the creation, application, and deletion of features needed in particular Data Science Cluster configuration such as cluster setup or component configuration. + +When creating a `FeaturesHandler`, developers can provide a FeaturesProvider implementations. This allows for the straightforward registration of a list of features that the handler will manage. + +## Conventions + +### Templates + +Golang templating can be used to create (or patch) resources in the cluster which are part of the Feature. Following rules are applied: + +* Any file which has `.tmpl.` in its name will be treated as a template for the target resource. +* Any file which has `.patch.` in its name will be treated a patch operation for the target resource. + +By convention, these files can be stored in the resources folder next to the Feature setup code, so they can be embedded as an embedded filesystem when defining a feature, for example, by using the Builder. + +Anonymous struct can be used on per feature set basis to organize resource access easier: + +```go +//go:embed resources +var resourcesFS embed.FS + +const baseDir = "resources" + +var Resources = struct { + // InstallDir is the path to the Serving install templates. + InstallDir string + // Location specifies the file system that contains the templates to be used. + Location fs.FS + // BaseDir is the path to the base of the embedded FS + BaseDir string +}{ + InstallDir: path.Join(baseDir, "installation"), + Location: resourcesFS, + BaseDir: baseDir, +} +``` + +### Feature context re-use + +The `FeatureData` anonymous struct convention provides a clear and consistent way to manage data for features. + +By defining data and extraction functions, it simplifies handling of feature-related data in both templates and functions where this data is required. + +```go +const ( + servingKey = "Serving" // <1> +) + +var FeatureData = struct { + Serving feature.DataDefinition[infrav1.ServingSpec, infrav1.ServingSpec] // <2> +}{ + Serving: feature.DataDefinition[infrav1.ServingSpec, infrav1.ServingSpec]{ + Define /*<3>*/: func(source *infrav1.ServingSpec) feature.DataEntry[infrav1.ServingSpec] { + return feature.DataEntry[infrav1.ServingSpec]{ + Key: servingKey, + Value: provider.ValueOf(*source).Get, + } + }, + Extract /*<4>*/: feature.ExtractEntry[infrav1.ServingSpec](servingKey), + }, +} +``` +- `<1>` Key used to store defined data entry in the Feature's context. This can be later used in templates as well as golang functions. +- `<2>` Generic struct used to define how context entry is created. Parametrized types hold information about the source and target types. The source type represents the origin from which data is taken, defining the type of data being input or extracted from the source. The target type determines the type of the value that will be stored in the key-value store (feature context data). +- `<3>` Defines how an entry (key-valaue) is created and stored in Feature's context. +- `<4>` Defines how to extract the value from the context (typed access instead of relying on string keys). + +Example below illustrates both storing data in the feature and accessing it from Feature's action function: + +```go +// Define the feature using builder +// ... +WithData(serverless.FeatureData.Serving.Define(servingSpec).AsAction()) +// ... + +// To extract in the action function +func DoSomethingWithServingData(f *feature.Feature) (string, error) { + serving, err := FeatureData.Serving.Extract(f); + if err != nil { + return "", err + } + // do something with serving data - create a resource, etc. + // .... +} +``` +> [!NOTE] +> Calling `.AsAction()` is a workaround due to limitation of generic on receiver functions + diff --git a/pkg/feature/builder.go b/pkg/feature/builder.go index eb628796c4d..6e567e6bd5c 100644 --- a/pkg/feature/builder.go +++ b/pkg/feature/builder.go @@ -1,6 +1,8 @@ package feature import ( + "context" + "fmt" "io/fs" "github.com/hashicorp/go-multierror" @@ -11,73 +13,75 @@ import ( "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" - infrav1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/infrastructure/v1" ) type partialBuilder func(f *Feature) error type featureBuilder struct { - name string - config *rest.Config - builders []partialBuilder - featuresHandler *FeaturesHandler - fsys fs.FS - targetNS string - managed bool + featureName string + source featurev1.Source + targetNs string + config *rest.Config + managed bool + fsys fs.FS + builders []partialBuilder } -// CreateFeature creates a new feature builder with the given name. -func CreateFeature(name string) *usingFeaturesHandler { - return &usingFeaturesHandler{ - name: name, +// Define creates a new feature builder with the given name. +func Define(featureName string) *featureBuilder { + fb := &featureBuilder{ + featureName: featureName, + source: featurev1.Source{ + Type: featurev1.UnknownType, + Name: featureName, + }, } -} - -type usingFeaturesHandler struct { - name string -} -// For sets the associated FeaturesHandler for the feature which will serve as entry point managing all the related features. -func (u *usingFeaturesHandler) For(featuresHandler *FeaturesHandler) *featureBuilder { - createSpec := func(f *Feature) error { - f.Spec = &Spec{ - ServiceMeshSpec: featuresHandler.DSCInitializationSpec.ServiceMesh, - Serving: &infrav1.ServingSpec{}, - Source: &featuresHandler.source, - AppNamespace: featuresHandler.DSCInitializationSpec.ApplicationsNamespace, - AuthProviderName: "authorino", + initializeContext := func(f *Feature) error { + if len(fb.targetNs) == 0 { + return fmt.Errorf("target namespace for '%s' feature is not defined", fb.featureName) } - return nil - } + f.TargetNamespace = fb.targetNs - fb := &featureBuilder{ - name: u.name, - featuresHandler: featuresHandler, - targetNS: featuresHandler.DSCInitializationSpec.ApplicationsNamespace, + return f.Set("TargetNamespace", fb.targetNs) } - // Ensures creation of .Spec object is always invoked first - fb.builders = append([]partialBuilder{createSpec}, fb.builders...) + // Ensures creation of shared data is always invoked first + fb.builders = append([]partialBuilder{initializeContext}, fb.builders...) + + return fb +} + +// TargetNamespace sets the namespace in which the feature should be applied. +func (fb *featureBuilder) TargetNamespace(targetNs string) *featureBuilder { + fb.targetNs = targetNs + + return fb +} + +func (fb *featureBuilder) Source(source featurev1.Source) *featureBuilder { + fb.source = source return fb } // Used to enforce that Manifests() is called after ManifestsLocation() in the chain. -type featureBuilderWithManifestSource struct { +type requiresManifestSourceBuilder struct { *featureBuilder } // ManifestsLocation sets the root file system (fsys) from which manifest paths are loaded. -func (fb *featureBuilder) ManifestsLocation(fsys fs.FS) *featureBuilderWithManifestSource { +func (fb *featureBuilder) ManifestsLocation(fsys fs.FS) *requiresManifestSourceBuilder { fb.fsys = fsys - return &featureBuilderWithManifestSource{featureBuilder: fb} + return &requiresManifestSourceBuilder{featureBuilder: fb} } // Manifests loads manifests from the provided paths. -func (fb *featureBuilderWithManifestSource) Manifests(paths ...string) *featureBuilderWithManifestSource { +func (fb *requiresManifestSourceBuilder) Manifests(paths ...string) *featureBuilder { fb.builders = append(fb.builders, func(f *Feature) error { var err error var manifests []Manifest @@ -94,15 +98,7 @@ func (fb *featureBuilderWithManifestSource) Manifests(paths ...string) *featureB return nil }) - return fb -} - -// TargetNamespace sets the namespace in which the feature should be applied. -// If not set, the feature will be applied in the application namespace (where this operator lives). -func (fb *featureBuilder) TargetNamespace(targetNs string) *featureBuilder { - fb.targetNS = targetNs - - return fb + return fb.featureBuilder } // Managed marks the feature as managed by the operator. @@ -120,11 +116,12 @@ func (fb *featureBuilder) Managed() *featureBuilder { return fb } -// WithData adds data loaders to the feature. This way you can define what data should be loaded before the feature is applied. +// WithData adds data providers to the feature (implemented as Actions). +// This way you can define what data should be loaded before the feature is applied. // This can be later used in templates and when creating resources programmatically. -func (fb *featureBuilder) WithData(loader ...Action) *featureBuilder { +func (fb *featureBuilder) WithData(dataProviders ...Action) *featureBuilder { fb.builders = append(fb.builders, func(f *Feature) error { - f.loaders = append(f.loaders, loader...) + f.dataProviders = append(f.dataProviders, dataProviders...) return nil }) @@ -196,36 +193,39 @@ func (fb *featureBuilder) OnDelete(cleanups ...Action) *featureBuilder { return fb } -// Load creates a new Feature instance and add it to corresponding FeaturesHandler. +// 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) Load() error { - feature := newFeature(fb.name) +func (fb *featureBuilder) Create() (*Feature, error) { + f := &Feature{ + Name: fb.featureName, + Managed: fb.managed, + Enabled: func(_ context.Context, feature *Feature) (bool, error) { + return true, nil + }, + Log: log.Log.WithName("features").WithValues("feature", fb.featureName), + fsys: fb.fsys, + source: &fb.source, + } // UsingConfig builder wasn't called while constructing this feature. // Get default settings and create needed clients. if fb.config == nil { if err := fb.withDefaultClient(); err != nil { - return err + return nil, err } } - if err := createClient(fb.config)(feature); err != nil { - return err + if err := createClient(fb.config)(f); err != nil { + return nil, err } for i := range fb.builders { - if err := fb.builders[i](feature); err != nil { - return err + if err := fb.builders[i](f); err != nil { + return nil, err } } - feature.Spec.TargetNamespace = fb.targetNS - feature.fsys = fb.fsys - feature.Managed = fb.managed - - fb.featuresHandler.features = append(fb.featuresHandler.features, feature) - - return nil + return f, nil } // UsingConfig allows to pass a custom rest.Config to the feature. Useful for testing. diff --git a/pkg/feature/feature.go b/pkg/feature/feature.go index 353a7599191..ad02d07cda9 100644 --- a/pkg/feature/feature.go +++ b/pkg/feature/feature.go @@ -10,50 +10,64 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" - ctrlLog "sigs.k8s.io/controller-runtime/pkg/log" featurev1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" ) +// Feature is a high-level abstraction that represents a collection of resources and actions +// that are applied to the cluster to enable a specific feature. +// +// Features can be either managed or unmanaged. Managed features are reconciled to their +// desired state based on defined manifests. +// +// In addition to creating resources using manifest files or through Golang functions, a Feature +// allows defining preconditions and postconditions. These conditions are checked to ensure +// the cluster is in the desired state for the feature to be applied successfully. +// +// When a Feature is applied, an associated resource called FeatureTracker is created. This +// resource establishes ownership for related resources, allowing for easy cleanup of all resources +// associated with the feature when it is about to be removed during reconciliation. +// +// Each Feature can have a list of cleanup functions. These functions can be particularly useful +// when the cleanup involves actions other than the removal of resources, such as reverting a patch operation. +// +// To create a Feature, use the provided FeatureBuilder. This builder guides through the process +// using a fluent API. type Feature struct { - Name string - Spec *Spec - Enabled EnabledFunc - Managed bool - Tracker *featurev1.FeatureTracker + Name string + TargetNamespace string + Enabled EnabledFunc + Managed bool Client client.Client + Log logr.Logger + tracker *featurev1.FeatureTracker + source *featurev1.Source + + data map[string]any manifests []Manifest cleanups []Action resources []Action preconditions []Action postconditions []Action - loaders []Action - fsys fs.FS - - Log logr.Logger -} + dataProviders []Action -func newFeature(name string) *Feature { - return &Feature{ - Name: name, - Enabled: func(_ context.Context, feature *Feature) (bool, error) { - return true, nil - }, - Log: ctrlLog.Log.WithName("features").WithValues("feature", name), - } + fsys fs.FS } -// Action is a func type which can be used for different purposes while having access to Feature struct. -type Action func(ctx context.Context, feature *Feature) error +// 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 // EnabledFunc is a func type used to determine if a feature should be enabled. type EnabledFunc func(ctx context.Context, 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 { // 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. @@ -69,7 +83,7 @@ func (f *Feature) Apply(ctx context.Context) error { return trackerErr } - if _, updateErr := status.UpdateWithRetry(ctx, f.Client, f.Tracker, func(saved *featurev1.FeatureTracker) { + if _, updateErr := status.UpdateWithRetry(ctx, f.Client, 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 { @@ -85,21 +99,20 @@ func (f *Feature) Apply(ctx context.Context) error { func (f *Feature) applyFeature(ctx context.Context) error { var multiErr *multierror.Error + for _, dataProvider := range f.dataProviders { + multiErr = multierror.Append(multiErr, dataProvider(ctx, 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)) } - if preconditionsErr := multiErr.ErrorOrNil(); preconditionsErr != nil { return &withConditionReasonError{reason: featurev1.ConditionReason.PreConditions, err: preconditionsErr} } - for _, loader := range f.loaders { - multiErr = multierror.Append(multiErr, loader(ctx, f)) - } - if dataLoadErr := multiErr.ErrorOrNil(); dataLoadErr != nil { - return &withConditionReasonError{reason: featurev1.ConditionReason.LoadTemplateData, err: dataLoadErr} - } - for _, resource := range f.resources { if resourceCreateErr := resource(ctx, f); resourceCreateErr != nil { return &withConditionReasonError{reason: featurev1.ConditionReason.ResourceCreation, err: resourceCreateErr} @@ -110,7 +123,7 @@ func (f *Feature) applyFeature(ctx context.Context) error { manifest := f.manifests[i] apply := f.createApplier(manifest) - objs, processErr := manifest.Process(f.Spec) + objs, processErr := manifest.Process(f.data) if processErr != nil { return &withConditionReasonError{reason: featurev1.ConditionReason.ApplyManifests, err: processErr} } @@ -140,6 +153,15 @@ func (f *Feature) Cleanup(ctx context.Context) error { f.addCleanup(removeFeatureTracker) 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)) } @@ -147,6 +169,10 @@ func (f *Feature) Cleanup(ctx context.Context) error { return cleanupErrors.ErrorOrNil() } +func (f *Feature) addCleanup(cleanupFuncs ...Action) { + f.cleanups = append(f.cleanups, cleanupFuncs...) +} + type applier func(ctx context.Context, objects []*unstructured.Unstructured) error func (f *Feature) createApplier(m Manifest) applier { @@ -170,14 +196,12 @@ func (f *Feature) createApplier(m Manifest) applier { } } -func (f *Feature) addCleanup(cleanupFuncs ...Action) { - f.cleanups = append(f.cleanups, cleanupFuncs...) -} - +// AsOwnerReference returns an OwnerReference for the FeatureTracker resource. func (f *Feature) AsOwnerReference() metav1.OwnerReference { - return f.Tracker.ToOwnerReference() + return f.tracker.ToOwnerReference() } +// OwnedBy returns a cluster.MetaOptions that sets the owner reference to the FeatureTracker resource. func OwnedBy(f *Feature) cluster.MetaOptions { return cluster.WithOwnerReference(f.AsOwnerReference()) } diff --git a/pkg/feature/feature_data.go b/pkg/feature/feature_data.go new file mode 100644 index 00000000000..0cfe5ee1f46 --- /dev/null +++ b/pkg/feature/feature_data.go @@ -0,0 +1,81 @@ +package feature + +import ( + "context" + "fmt" + + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/provider" +) + +// Entry allows to define association between a name under which the data is stored in the Feature and a data provider +// defining the logic for fetching. Provider is a function allowing to fetch a value for a given key dynamically by +// interacting with Kubernetes client. +// +// 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) + if err != nil { + return err + } + + return f.Set(key, data) + } +} + +// DataEntry associates data provider with a key under which the data is stored in the Feature. +type DataEntry[T any] struct { + Key string + Value provider.DataProviderFunc[T] +} + +// DataDefinition defines how the data is created and fetched from the Feature's data context. +// S is a source type from which the data is created. +// T is a type of the data stored in the Feature. +type DataDefinition[S, T any] struct { + // Define is a factory function to create a Feature's DataEntry from the given source. + Define func(source *S) DataEntry[T] + // Extract allows to fetch data from the Feature. + Extract func(f *Feature) (T, error) +} + +// ExtractEntry is a convenient way to define how to extract a value from the given Feature's data using defined key. +func ExtractEntry[T any](key string) func(f *Feature) (T, error) { + return func(f *Feature) (T, error) { + return Get[T](f, key) + } +} + +// AsAction converts DataEntry to an Action which is the used to populate defined key-value in the feature itself. +func (d DataEntry[T]) AsAction() Action { + return Entry[T](d.Key, d.Value) +} + +// Get allows to retrieve arbitrary value from the Feature's data container. +func Get[T any](f *Feature, key string) (T, error) { + var data T + var ok bool + + input, found := f.data[key] + if !found { + return data, fmt.Errorf("key %s not found in feature %s", key, f.Name) + } + + data, ok = input.(T) + if !ok { + return data, fmt.Errorf("invalid type %T for key %s in feature %s", f.data[key], key, f.Name) + } + + return data, nil +} + +// Set allows to store a value under given key in the Feature's data container. +func (f *Feature) Set(key string, data any) error { + if f.data == nil { + f.data = map[string]any{} + } + + f.data[key] = data + + return nil +} diff --git a/pkg/feature/feature_suite_test.go b/pkg/feature/feature_suite_test.go new file mode 100644 index 00000000000..27c8afc4e9b --- /dev/null +++ b/pkg/feature/feature_suite_test.go @@ -0,0 +1,13 @@ +package feature_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestFeatures(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Features SDK Suite") +} diff --git a/pkg/feature/feature_tracker_handler.go b/pkg/feature/feature_tracker_handler.go index 1215cd662ff..f9976df05e2 100644 --- a/pkg/feature/feature_tracker_handler.go +++ b/pkg/feature/feature_tracker_handler.go @@ -36,28 +36,28 @@ func createFeatureTracker(ctx context.Context, f *Feature) error { } if k8serr.IsNotFound(errGet) { - tracker = featurev1.NewFeatureTracker(f.Name, f.Spec.AppNamespace) + tracker = featurev1.NewFeatureTracker(f.Name, f.TargetNamespace) tracker.Spec = featurev1.FeatureTrackerSpec{ - Source: *f.Spec.Source, - AppNamespace: f.Spec.AppNamespace, + Source: *f.source, + AppNamespace: f.TargetNamespace, } if errCreate := f.Client.Create(ctx, tracker); errCreate != nil { return errCreate } } - if gvkErr := ensureGVKSet(tracker, f.Client.Scheme()); gvkErr != nil { - return gvkErr + if errGVK := ensureGVKSet(tracker, f.Client.Scheme()); errGVK != nil { + return errGVK } - f.Tracker = tracker + f.tracker = tracker return nil } // 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 + 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 { @@ -75,7 +75,7 @@ func removeFeatureTracker(ctx context.Context, f *Feature) error { } func getFeatureTracker(ctx context.Context, f *Feature) (*featurev1.FeatureTracker, error) { - tracker := featurev1.NewFeatureTracker(f.Name, f.Spec.AppNamespace) + tracker := featurev1.NewFeatureTracker(f.Name, f.TargetNamespace) if errGet := f.Client.Get(ctx, client.ObjectKeyFromObject(tracker), tracker); errGet != nil { return nil, errGet @@ -100,7 +100,7 @@ func ensureGVKSet(obj runtime.Object, scheme *runtime.Scheme) error { } func createFeatureTrackerStatusReporter(f *Feature) *status.Reporter[*featurev1.FeatureTracker] { - return status.NewStatusReporter(f.Client, f.Tracker, func(err error) status.SaveStatusFunc[*featurev1.FeatureTracker] { + return status.NewStatusReporter(f.Client, 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 515e88dad2e..c6434b7d687 100644 --- a/pkg/feature/handler.go +++ b/pkg/feature/handler.go @@ -13,29 +13,111 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" ) -type featureHandler interface { +type featuresHandler interface { Apply(ctx context.Context) error Delete(ctx context.Context) error } -var _ featureHandler = (*FeaturesHandler)(nil) +type FeaturesRegistry interface { + Add(builders ...*featureBuilder) error +} + +var _ featuresHandler = (*FeaturesHandler)(nil) -// FeaturesHandler coordinates feature creations and removal from within controllers. +// 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 { - *dsciv1.DSCInitializationSpec + targetNamespace string source featurev1.Source features []*Feature featuresProviders []FeaturesProvider } +var _ FeaturesRegistry = (*FeaturesHandler)(nil) + +// Add loads features defined by passed builders and adds to internal list which is then used to Apply on the cluster. +// It also makes sure that both TargetNamespace and Source are added to the feature before it's `Create()`ed. +func (fh *FeaturesHandler) Add(builders ...*featureBuilder) error { + var multiErr *multierror.Error + + for i := range builders { + fb := builders[i] + feature, err := fb.TargetNamespace(fh.targetNamespace). + Source(fh.source). + Create() + multiErr = multierror.Append(multiErr, err) + fh.features = append(fh.features, feature) + } + + return multiErr.ErrorOrNil() +} + +func (fh *FeaturesHandler) Apply(ctx context.Context) error { + fh.features = make([]*Feature, 0) + + for _, featuresProvider := range fh.featuresProviders { + if err := featuresProvider(fh); err != nil { + return fmt.Errorf("failed adding features to the handler. cause: %w", err) + } + } + + var multiErr *multierror.Error + for _, f := range fh.features { + if applyErr := f.Apply(ctx); applyErr != nil { + multiErr = multierror.Append(multiErr, fmt.Errorf("failed applying FeatureHandler features. cause: %w", applyErr)) + } + } + + return multiErr.ErrorOrNil() +} + +// 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 { + fh.features = make([]*Feature, 0) + + for _, featuresProvider := range fh.featuresProviders { + if err := featuresProvider(fh); err != nil { + return fmt.Errorf("delete phase failed when wiring Feature instances in FeatureHandler.Delete. cause: %w", err) + } + } + + var multiErr *multierror.Error + for i := len(fh.features) - 1; i >= 0; i-- { + if cleanupErr := fh.features[i].Cleanup(ctx); cleanupErr != nil { + multiErr = multierror.Append(multiErr, fmt.Errorf("failed executing cleanup in FeatureHandler. cause: %w", cleanupErr)) + } + } + + return multiErr.ErrorOrNil() +} + +// FeaturesProvider is a function which allow to define list of features +// and add them to the handler's registry. +type FeaturesProvider func(registry FeaturesRegistry) error + +func ClusterFeaturesHandler(dsci *dsciv1.DSCInitialization, def ...FeaturesProvider) *FeaturesHandler { + return &FeaturesHandler{ + targetNamespace: dsci.Spec.ApplicationsNamespace, + source: featurev1.Source{Type: featurev1.DSCIType, Name: dsci.Name}, + featuresProviders: def, + } +} + +func ComponentFeaturesHandler(componentName, targetNamespace string, def ...FeaturesProvider) *FeaturesHandler { + return &FeaturesHandler{ + targetNamespace: targetNamespace, + source: featurev1.Source{Type: featurev1.ComponentType, Name: componentName}, + featuresProviders: def, + } +} + // EmptyFeaturesHandler is noop handler so that we can avoid nil checks in the code and safely call Apply/Delete methods. var EmptyFeaturesHandler = &FeaturesHandler{ features: []*Feature{}, featuresProviders: []FeaturesProvider{}, } -var _ featureHandler = (*HandlerWithReporter[client.Object])(nil) - // HandlerWithReporter is a wrapper around FeaturesHandler and status.Reporter // It is intended apply features related to a given resource capabilities and report its status using custom reporter. type HandlerWithReporter[T client.Object] struct { @@ -43,6 +125,8 @@ type HandlerWithReporter[T client.Object] struct { reporter *status.Reporter[T] } +var _ featuresHandler = (*HandlerWithReporter[client.Object])(nil) + func NewHandlerWithReporter[T client.Object](handler *FeaturesHandler, reporter *status.Reporter[T]) *HandlerWithReporter[T] { return &HandlerWithReporter[T]{ handler: handler, @@ -65,57 +149,3 @@ func (h HandlerWithReporter[T]) Delete(ctx context.Context) error { // We should return both errors to the caller. return multierror.Append(deleteErr, reportErr).ErrorOrNil() } - -// FeaturesProvider is a function which allow to define list of features -// and couple them with the given initializer. -type FeaturesProvider func(handler *FeaturesHandler) error - -func ClusterFeaturesHandler(dsci *dsciv1.DSCInitialization, def ...FeaturesProvider) *FeaturesHandler { - return &FeaturesHandler{ - DSCInitializationSpec: &dsci.Spec, - source: featurev1.Source{Type: featurev1.DSCIType, Name: dsci.Name}, - featuresProviders: def, - } -} - -func ComponentFeaturesHandler(componentName string, spec *dsciv1.DSCInitializationSpec, def ...FeaturesProvider) *FeaturesHandler { - return &FeaturesHandler{ - DSCInitializationSpec: spec, - source: featurev1.Source{Type: featurev1.ComponentType, Name: componentName}, - featuresProviders: def, - } -} - -func (fh *FeaturesHandler) Apply(ctx context.Context) error { - for _, featuresProvider := range fh.featuresProviders { - if err := featuresProvider(fh); err != nil { - return fmt.Errorf("apply phase failed when applying features: %w", err) - } - } - - var applyErrors *multierror.Error - for _, f := range fh.features { - applyErrors = multierror.Append(applyErrors, f.Apply(ctx)) - } - - return applyErrors.ErrorOrNil() -} - -// Delete executes registered clean-up tasks in the opposite order they were initiated (following a stack structure). -// For instance, this allows for the undoing patches before its deletion. -// This approach assumes that Features are either instantiated in the correct sequence -// or are self-contained. -func (fh *FeaturesHandler) Delete(ctx context.Context) error { - for _, featuresProvider := range fh.featuresProviders { - if err := featuresProvider(fh); err != nil { - return fmt.Errorf("delete phase failed when wiring Feature instances: %w", err) - } - } - - var cleanupErrors *multierror.Error - for i := len(fh.features) - 1; i >= 0; i-- { - cleanupErrors = multierror.Append(cleanupErrors, fh.features[i].Cleanup(ctx)) - } - - return cleanupErrors.ErrorOrNil() -} diff --git a/pkg/feature/manifest_test.go b/pkg/feature/manifest_test.go index 8d85dfc0f3d..f900cff973f 100644 --- a/pkg/feature/manifest_test.go +++ b/pkg/feature/manifest_test.go @@ -3,7 +3,6 @@ package feature_test import ( "io/fs" "path/filepath" - "testing" "github.com/spf13/afero" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -57,7 +56,7 @@ data: // given manifest := feature.CreateRawManifestFrom(inMemFS, path) - data := feature.Spec{ + data := struct{ TargetNamespace string }{ TargetNamespace: "not-used", } @@ -80,7 +79,7 @@ metadata: name: my-configmap namespace: {{.TargetNamespace}} data: - key: Data + key: FeatureContext ` BeforeEach(func() { @@ -107,7 +106,7 @@ data: It("should substitute target namespace in the templated manifest", func() { // given - data := feature.Spec{ + data := struct{ TargetNamespace string }{ TargetNamespace: "template-ns", } manifest := feature.CreateTemplateManifestFrom(inMemFS, path) @@ -128,11 +127,11 @@ data: }) -func processManifests(data feature.Spec, m []feature.Manifest) []*unstructured.Unstructured { +func processManifests(data any, m []feature.Manifest) []*unstructured.Unstructured { var objs []*unstructured.Unstructured var err error for i := range m { - objs, err = m[i].Process(&data) + objs, err = m[i].Process(data) if err != nil { break } @@ -140,8 +139,3 @@ func processManifests(data feature.Spec, m []feature.Manifest) []*unstructured.U Expect(err).NotTo(HaveOccurred()) return objs } - -func TestManifests(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "Manifest Process Suite") -} diff --git a/pkg/feature/provider/data_provider_test.go b/pkg/feature/provider/data_provider_test.go new file mode 100644 index 00000000000..4e59f978bd9 --- /dev/null +++ b/pkg/feature/provider/data_provider_test.go @@ -0,0 +1,71 @@ +package provider_test + +import ( + "context" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/provider" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Using DataProvider with default value", func() { + + var c client.Client + + BeforeEach(func() { + c = fake.NewClientBuilder(). + WithObjects(&corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-service", + Namespace: "test-namespace", + }, + }). + Build() + }) + + It("should return the default value if the original value is zero", func(ctx context.Context) { + var originalValue int + defaultValue := 10 + dataProviderWithDefault := provider.ValueOf(originalValue).OrElse(defaultValue) + + data, err := dataProviderWithDefault(ctx, c) + + Expect(err).NotTo(HaveOccurred()) + Expect(data).To(Equal(defaultValue)) + }) + + It("should return the default slice value if original zero", func(ctx context.Context) { + var originalValue []int + defaultValue := []int{1, 2, 3, 4} + dataProviderWithDefault := provider.ValueOf(originalValue).OrElse(defaultValue) + + data, err := dataProviderWithDefault(ctx, c) + + Expect(err).NotTo(HaveOccurred()) + Expect(data).To(Equal(defaultValue)) + }) + + It("should fall back to Get function when passed value is not defined", func(ctx context.Context) { + var nonExistingService *corev1.Service + serviceProviderWithDefault := provider.ValueOf(nonExistingService).OrGet(func(ctx context.Context, c client.Client) (*corev1.Service, error) { + service := &corev1.Service{} + if errGet := c.Get(ctx, client.ObjectKey{Name: "test-service", Namespace: "test-namespace"}, service); errGet != nil { + return nil, errGet + } + + return service, nil + }) + + actualService, err := serviceProviderWithDefault(ctx, c) + + Expect(err).NotTo(HaveOccurred()) + Expect(actualService.Name).To(Equal("test-service")) + }) + +}) diff --git a/pkg/feature/provider/provider_suite_test.go b/pkg/feature/provider/provider_suite_test.go new file mode 100644 index 00000000000..211a2ec1f00 --- /dev/null +++ b/pkg/feature/provider/provider_suite_test.go @@ -0,0 +1,13 @@ +package provider_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestProviders(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Data Provider Suite") +} diff --git a/pkg/feature/provider/types.go b/pkg/feature/provider/types.go new file mode 100644 index 00000000000..370f99ee05a --- /dev/null +++ b/pkg/feature/provider/types.go @@ -0,0 +1,69 @@ +package provider + +import ( + "context" + "reflect" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// DataProvider is a contract on how the data for the Feature container can be fetched. +// It is expected that either found instance is returned or error occurred during invocation. +type DataProvider[T any] interface { + Get(ctx context.Context, c client.Client) (T, error) +} + +// DataProviderFunc defines function signature which is used for fetching data. +// This allows to pass simple closures while construction data providers. +type DataProviderFunc[T any] func(ctx context.Context, c client.Client) (T, error) + +// ValueOf is a constructor which allows to define a value with optional provider. +func ValueOf[T any](value T) DataProviderWithDefault[T] { + return DataProviderWithDefault[T]{value: value} +} + +// Defaulter defines how a default value can be supplied when original one is zero-value. +type Defaulter[T any] interface { + Value() T + OrElse(other T) DataProviderFunc[T] + OrGet(getFunc DataProviderFunc[T]) DataProviderFunc[T] +} + +// DataProviderWithDefault allows to define a value and optional means of supplying it if original value is empty. +// When the original value is zero the alternative can be provided using: +// - `OrElse` to define a static value +// - `OrGet` to perform dynamic lookup by providing DataProviderFunc. +type DataProviderWithDefault[T any] struct { + value T //nolint:structcheck //reason used in e.g. Get +} + +var _ DataProvider[any] = (*DataProviderWithDefault[any])(nil) +var _ Defaulter[any] = (*DataProviderWithDefault[any])(nil) + +// Get returns Value() of Defaulter and ensures DataProviderWithDefault can be used as DataProviderFunc. +func (d DataProviderWithDefault[T]) Get(_ context.Context, _ client.Client) (T, error) { + return d.Value(), nil +} + +// Value returns actual value stored by the provider. +func (d DataProviderWithDefault[T]) Value() T { + return d.value +} + +// OrElse allows to define static default value when the stored one is a zero-value. +func (d DataProviderWithDefault[T]) OrElse(other T) DataProviderFunc[T] { + if reflect.ValueOf(d.Value()).IsZero() { + d.value = other + } + + return d.Get +} + +// OrGet allows to define dynamic value provider when the stored one is a zero-value. +func (d DataProviderWithDefault[T]) OrGet(getFunc DataProviderFunc[T]) DataProviderFunc[T] { + if reflect.ValueOf(d.Value()).IsZero() { + return getFunc + } + + return d.Get +} diff --git a/pkg/feature/serverless/data.go b/pkg/feature/serverless/data.go new file mode 100644 index 00000000000..3a4c98ca3cb --- /dev/null +++ b/pkg/feature/serverless/data.go @@ -0,0 +1,67 @@ +package serverless + +import ( + "context" + "fmt" + + "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/provider" +) + +const DefaultCertificateSecretName = "knative-serving-cert" + +const ( + servingKey = "Serving" + certificateKey = "KnativeCertificateSecret" + knativeIngressDomainKey = "KnativeIngressDomain" +) + +// FeatureData is a convention to simplify how the data for the Serverless features is Defined and accessed. +var FeatureData = struct { + Serving feature.DataDefinition[infrav1.ServingSpec, infrav1.ServingSpec] + CertificateName feature.DataDefinition[infrav1.ServingSpec, string] + IngressDomain feature.DataDefinition[infrav1.ServingSpec, string] +}{ + Serving: feature.DataDefinition[infrav1.ServingSpec, infrav1.ServingSpec]{ + Define: func(source *infrav1.ServingSpec) feature.DataEntry[infrav1.ServingSpec] { + return feature.DataEntry[infrav1.ServingSpec]{ + Key: servingKey, + Value: provider.ValueOf(*source).Get, + } + }, + Extract: feature.ExtractEntry[infrav1.ServingSpec](servingKey), + }, + CertificateName: feature.DataDefinition[infrav1.ServingSpec, string]{ + Define: func(source *infrav1.ServingSpec) feature.DataEntry[string] { + return feature.DataEntry[string]{ + Key: certificateKey, + Value: provider.ValueOf(source.IngressGateway.Certificate.SecretName).OrElse(DefaultCertificateSecretName), + } + }, + Extract: feature.ExtractEntry[string](certificateKey), + }, + IngressDomain: feature.DataDefinition[infrav1.ServingSpec, string]{ + Define: func(source *infrav1.ServingSpec) feature.DataEntry[string] { + return feature.DataEntry[string]{ + Key: knativeIngressDomainKey, + Value: provider.ValueOf(source.IngressGateway.Domain).OrGet(knativeDomain), + } + }, + Extract: feature.ExtractEntry[string](knativeIngressDomainKey), + }, +} + +func knativeDomain(ctx context.Context, c client.Client) (string, error) { + var errDomain error + domain, errDomain := cluster.GetDomain(ctx, c) + if errDomain != nil { + return "", fmt.Errorf("failed to fetch OpenShift domain to generate certificate for Serverless: %w", errDomain) + } + + domain = "*." + domain + return domain, nil +} diff --git a/pkg/feature/serverless/loaders.go b/pkg/feature/serverless/loaders.go deleted file mode 100644 index c0845b226b5..00000000000 --- a/pkg/feature/serverless/loaders.go +++ /dev/null @@ -1,38 +0,0 @@ -package serverless - -import ( - "context" - "fmt" - "strings" - - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" -) - -const DefaultCertificateSecretName = "knative-serving-cert" - -func ServingDefaultValues(_ context.Context, f *feature.Feature) error { - certificateSecretName := strings.TrimSpace(f.Spec.Serving.IngressGateway.Certificate.SecretName) - if len(certificateSecretName) == 0 { - certificateSecretName = DefaultCertificateSecretName - } - - f.Spec.KnativeCertificateSecret = certificateSecretName - return nil -} - -func ServingIngressDomain(ctx context.Context, f *feature.Feature) error { - domain := strings.TrimSpace(f.Spec.Serving.IngressGateway.Domain) - if len(domain) == 0 { - var errDomain error - domain, errDomain = cluster.GetDomain(ctx, f.Client) - if errDomain != nil { - return fmt.Errorf("failed to fetch OpenShift domain to generate certificate for Serverless: %w", errDomain) - } - - domain = "*." + domain - } - - f.Spec.KnativeIngressDomain = domain - return nil -} diff --git a/pkg/feature/serverless/resources.go b/pkg/feature/serverless/resources.go index 5b9f3d3d0f9..cc28390895d 100644 --- a/pkg/feature/serverless/resources.go +++ b/pkg/feature/serverless/resources.go @@ -6,19 +6,62 @@ import ( 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 { - switch certType := f.Spec.Serving.IngressGateway.Certificate.Type; certType { + secretData, err := getSecretParams(f) + if err != nil { + return err + } + + switch secretData.Type { case infrav1.SelfSigned: return cluster.CreateSelfSignedCertificate(ctx, f.Client, - f.Spec.KnativeCertificateSecret, - f.Spec.KnativeIngressDomain, - f.Spec.ControlPlane.Namespace, + secretData.Name, + secretData.Domain, + secretData.Namespace, feature.OwnedBy(f)) case infrav1.Provided: return nil default: - return cluster.PropagateDefaultIngressCertificate(ctx, f.Client, f.Spec.KnativeCertificateSecret, f.Spec.ControlPlane.Namespace) + return cluster.PropagateDefaultIngressCertificate(ctx, f.Client, secretData.Name, secretData.Namespace) + } +} + +type secretParams struct { + Name string + Namespace string + Domain string + Type infrav1.CertType +} + +func getSecretParams(f *feature.Feature) (*secretParams, error) { + result := &secretParams{} + + if secret, err := FeatureData.CertificateName.Extract(f); err == nil { + result.Name = secret + } else { + return nil, err } + + if domain, err := FeatureData.IngressDomain.Extract(f); err == nil { + result.Domain = domain + } else { + return nil, err + } + + if serving, err := FeatureData.Serving.Extract(f); err == nil { + result.Type = serving.IngressGateway.Certificate.Type + } else { + return nil, err + } + + if controlPlane, err := servicemesh.FeatureData.ControlPlane.Extract(f); err == nil { + result.Namespace = controlPlane.Namespace + } else { + return nil, err + } + + return result, nil } diff --git a/pkg/feature/servicemesh/cleanup.go b/pkg/feature/servicemesh/cleanup.go index da5de8b1951..87163442591 100644 --- a/pkg/feature/servicemesh/cleanup.go +++ b/pkg/feature/servicemesh/cleanup.go @@ -12,15 +12,22 @@ import ( ) func RemoveExtensionProvider(ctx context.Context, f *feature.Feature) error { - ossmAuthzProvider := fmt.Sprintf("%s-auth-provider", f.Spec.AppNamespace) + extensionName, errExtName := FeatureData.Authorization.ExtensionProviderName.Extract(f) + if errExtName != nil { + return fmt.Errorf("failed to get extension name struct: %w", errExtName) + } + + controlPlane, err := FeatureData.ControlPlane.Extract(f) + if err != nil { + return fmt.Errorf("failed to get control plane struct: %w", err) + } - mesh := f.Spec.ControlPlane smcp := &unstructured.Unstructured{} smcp.SetGroupVersionKind(gvk.ServiceMeshControlPlane) if err := f.Client.Get(ctx, client.ObjectKey{ - Namespace: mesh.Namespace, - Name: mesh.Name, + Namespace: controlPlane.Namespace, + Name: controlPlane.Name, }, smcp); err != nil { return client.IgnoreNotFound(err) } @@ -30,18 +37,22 @@ func RemoveExtensionProvider(ctx context.Context, f *feature.Feature) error { return err } if !found { - f.Log.Info("no extension providers found", "feature", f.Name, "control-plane", mesh.Name, "namespace", mesh.Namespace) + 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 { - f.Log.Info("WARN: Unexpected type for extensionProvider will not be removed") + f.Log.Info("WARN: Unexpected type for extensionProvider, it will not be removed") continue } - - if extensionProvider["name"] == ossmAuthzProvider { + 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 { diff --git a/pkg/feature/servicemesh/conditions.go b/pkg/feature/servicemesh/conditions.go index 942b4070b80..ceedc718e69 100644 --- a/pkg/feature/servicemesh/conditions.go +++ b/pkg/feature/servicemesh/conditions.go @@ -24,11 +24,12 @@ 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 { - if resolveNsErr := ResolveAuthNamespace(f); resolveNsErr != nil { - return resolveNsErr + 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, f.Spec.Auth.Namespace, feature.OwnedBy(f), cluster.WithLabels(labels.ODH.OwnedNamespace, "true")) + _, err = cluster.CreateNamespace(ctx, f.Client, authNs, feature.OwnedBy(f), cluster.WithLabels(labels.ODH.OwnedNamespace, "true")) return err } @@ -45,11 +46,13 @@ func EnsureServiceMeshInstalled(ctx context.Context, f *feature.Feature) error { return err } - smcp := f.Spec.ControlPlane.Name - smcpNs := f.Spec.ControlPlane.Namespace - if err := WaitForControlPlaneToBeReady(ctx, f); err != nil { - f.Log.Error(err, "failed waiting for control plane being ready", "control-plane", smcp, "namespace", smcpNs) + controlPlane, errGet := FeatureData.ControlPlane.Extract(f) + if errGet != nil { + return fmt.Errorf("failed to get control plane struct: %w", err) + } + + f.Log.Error(err, "failed waiting for control plane being ready", "control-plane", controlPlane.Name, "namespace", controlPlane.Namespace) return multierror.Append(err, errors.New("service mesh control plane is not ready")).ErrorOrNil() } @@ -58,8 +61,13 @@ func EnsureServiceMeshInstalled(ctx context.Context, f *feature.Feature) error { } func WaitForControlPlaneToBeReady(ctx context.Context, f *feature.Feature) error { - smcp := f.Spec.ControlPlane.Name - smcpNs := f.Spec.ControlPlane.Namespace + controlPlane, err := FeatureData.ControlPlane.Extract(f) + if err != nil { + return err + } + + smcp := controlPlane.Name + smcpNs := controlPlane.Namespace f.Log.Info("waiting for control plane components to be ready", "control-plane", smcp, "namespace", smcpNs, "duration (s)", duration.Seconds()) diff --git a/pkg/feature/servicemesh/data.go b/pkg/feature/servicemesh/data.go new file mode 100644 index 00000000000..85a2ae3b850 --- /dev/null +++ b/pkg/feature/servicemesh/data.go @@ -0,0 +1,116 @@ +package servicemesh + +import ( + "context" + "strings" + + "sigs.k8s.io/controller-runtime/pkg/client" + + dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" + infrav1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/infrastructure/v1" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" +) + +// These keys are used in FeatureData struct, as fields of a struct are not accessible in closures which we define for +// creating and fetching the data. +const ( + controlPlaneKey string = "ControlPlane" + authKey string = "Auth" + authProviderNsKey string = "AuthNamespace" + authProviderNameKey string = "AuthProviderName" + authExtensionNameKey string = "AuthExtensionName" +) + +// FeatureData is a convention to simplify how the data for the Service Mesh features is Defined and accessed. +// Being a "singleton" it is based on anonymous struct concept. +var FeatureData = struct { + ControlPlane feature.DataDefinition[dsciv1.DSCInitializationSpec, infrav1.ControlPlaneSpec] + Authorization AuthorizationData +}{ + ControlPlane: feature.DataDefinition[dsciv1.DSCInitializationSpec, infrav1.ControlPlaneSpec]{ + Define: func(source *dsciv1.DSCInitializationSpec) feature.DataEntry[infrav1.ControlPlaneSpec] { + return feature.DataEntry[infrav1.ControlPlaneSpec]{ + Key: controlPlaneKey, + Value: func(_ context.Context, _ client.Client) (infrav1.ControlPlaneSpec, error) { + return source.ServiceMesh.ControlPlane, nil + }, + } + }, + Extract: feature.ExtractEntry[infrav1.ControlPlaneSpec](controlPlaneKey), + }, + Authorization: AuthorizationData{ + Spec: authSpec, + Namespace: authNs, + Provider: authProvider, + ExtensionProviderName: authExtensionName, + All: func(source *dsciv1.DSCInitializationSpec) []feature.Action { + return []feature.Action{ + authSpec.Define(source).AsAction(), + authNs.Define(source).AsAction(), + authProvider.Define(source).AsAction(), + authExtensionName.Define(source).AsAction(), + } + }, + }, +} + +type AuthorizationData struct { + Spec feature.DataDefinition[dsciv1.DSCInitializationSpec, infrav1.AuthSpec] + Namespace feature.DataDefinition[dsciv1.DSCInitializationSpec, string] + Provider feature.DataDefinition[dsciv1.DSCInitializationSpec, string] + ExtensionProviderName feature.DataDefinition[dsciv1.DSCInitializationSpec, string] + All func(source *dsciv1.DSCInitializationSpec) []feature.Action +} + +var authSpec = feature.DataDefinition[dsciv1.DSCInitializationSpec, infrav1.AuthSpec]{ + Define: func(source *dsciv1.DSCInitializationSpec) feature.DataEntry[infrav1.AuthSpec] { + return feature.DataEntry[infrav1.AuthSpec]{ + Key: authKey, + Value: func(_ context.Context, _ client.Client) (infrav1.AuthSpec, error) { + return source.ServiceMesh.Auth, nil + }, + } + }, + Extract: feature.ExtractEntry[infrav1.AuthSpec](authKey), +} + +var authNs = feature.DataDefinition[dsciv1.DSCInitializationSpec, string]{ + Define: func(source *dsciv1.DSCInitializationSpec) feature.DataEntry[string] { + return feature.DataEntry[string]{ + Key: authProviderNsKey, + Value: func(_ context.Context, _ client.Client) (string, error) { + ns := strings.TrimSpace(source.ServiceMesh.Auth.Namespace) + if len(ns) == 0 { + ns = source.ApplicationsNamespace + "-auth-provider" + } + + return ns, nil + }, + } + }, + Extract: feature.ExtractEntry[string](authProviderNsKey), +} + +var authProvider = feature.DataDefinition[dsciv1.DSCInitializationSpec, string]{ + Define: func(source *dsciv1.DSCInitializationSpec) feature.DataEntry[string] { + return feature.DataEntry[string]{ + Key: authProviderNameKey, + Value: func(_ context.Context, _ client.Client) (string, error) { + return "authorino", nil + }, + } + }, + Extract: feature.ExtractEntry[string](authProviderNameKey), +} + +var authExtensionName = feature.DataDefinition[dsciv1.DSCInitializationSpec, string]{ + Define: func(source *dsciv1.DSCInitializationSpec) feature.DataEntry[string] { + return feature.DataEntry[string]{ + Key: authExtensionNameKey, + Value: func(_ context.Context, _ client.Client) (string, error) { + return source.ApplicationsNamespace + "-auth-provider", nil + }, + } + }, + Extract: feature.ExtractEntry[string](authExtensionNameKey), +} diff --git a/pkg/feature/servicemesh/loaders.go b/pkg/feature/servicemesh/loaders.go deleted file mode 100644 index 0d966e24d79..00000000000 --- a/pkg/feature/servicemesh/loaders.go +++ /dev/null @@ -1,33 +0,0 @@ -package servicemesh - -import ( - "context" - "strings" - - "github.com/pkg/errors" - - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" -) - -func ClusterDetails(ctx context.Context, f *feature.Feature) error { - data := f.Spec - - if domain, err := cluster.GetDomain(ctx, f.Client); err == nil { - data.Domain = domain - } else { - return errors.WithStack(err) - } - - return nil -} - -func ResolveAuthNamespace(f *feature.Feature) error { - dsciAuthNamespace := strings.TrimSpace(f.Spec.Auth.Namespace) - - if len(dsciAuthNamespace) == 0 { - f.Spec.Auth.Namespace = f.Spec.AppNamespace + "-auth-provider" - } - - return nil -} diff --git a/pkg/feature/servicemesh/resources.go b/pkg/feature/servicemesh/resources.go index 5b6f6579d28..1aa0ae9466c 100644 --- a/pkg/feature/servicemesh/resources.go +++ b/pkg/feature/servicemesh/resources.go @@ -2,6 +2,7 @@ package servicemesh import ( "context" + "fmt" "strings" corev1 "k8s.io/api/core/v1" @@ -14,8 +15,11 @@ 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 { - meshConfig := f.Spec.ControlPlane - namespace := f.Spec.AppNamespace + meshConfig, err := FeatureData.ControlPlane.Extract(f) + if err != nil { + return fmt.Errorf("failed to get control plane struct: %w", err) + } + namespace := f.TargetNamespace data := map[string]string{ "CONTROL_PLANE_NAME": meshConfig.Name, @@ -39,19 +43,30 @@ 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 { - audiences := f.Spec.Auth.Audiences - appNamespace := f.Spec.AppNamespace - authNamespace := f.Spec.Auth.Namespace - if len(authNamespace) == 0 { - authNamespace = appNamespace + "-auth-provider" + targetNamespace := f.TargetNamespace + auth, err := FeatureData.Authorization.Spec.Extract(f) + if err != nil { + return fmt.Errorf("could not get auth from feature: %w", err) + } + + authNamespace, errAuthNs := FeatureData.Authorization.Namespace.Extract(f) + if errAuthNs != nil { + return fmt.Errorf("could not get auth provider namespace from feature: %w", err) } + + authProviderName, errAuthProvider := FeatureData.Authorization.Provider.Extract(f) + if errAuthProvider != nil { + return fmt.Errorf("could not get auth provider name from feature: %w", err) + } + + audiences := auth.Audiences audiencesList := "" if audiences != nil && len(*audiences) > 0 { audiencesList = strings.Join(*audiences, ",") } data := map[string]string{ "AUTH_AUDIENCE": audiencesList, - "AUTH_PROVIDER": appNamespace + "-auth-provider", + "AUTH_PROVIDER": authProviderName, "AUTH_NAMESPACE": authNamespace, "AUTHORINO_LABEL": "security.opendatahub.io/authorization-group=default", } @@ -62,7 +77,7 @@ func AuthRefs(ctx context.Context, f *feature.Feature) error { &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "auth-refs", - Namespace: appNamespace, + Namespace: targetNamespace, }, Data: data, }, diff --git a/pkg/feature/types.go b/pkg/feature/types.go deleted file mode 100644 index c84c71c8c2c..00000000000 --- a/pkg/feature/types.go +++ /dev/null @@ -1,28 +0,0 @@ -package feature - -import ( - featurev1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" - infrav1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/infrastructure/v1" -) - -type Spec struct { - *infrav1.ServiceMeshSpec - Serving *infrav1.ServingSpec - AuthProviderName string - OAuth OAuth - AppNamespace string - TargetNamespace string - Domain string - KnativeCertificateSecret string - KnativeIngressDomain string - Source *featurev1.Source -} - -type OAuth struct { - AuthzEndpoint, - TokenEndpoint, - Route, - Port, - ClientSecret, - Hmac string -} diff --git a/tests/integration/features/cleanup_int_test.go b/tests/integration/features/cleanup_int_test.go index f8205a61048..ea5f7308b10 100644 --- a/tests/integration/features/cleanup_int_test.go +++ b/tests/integration/features/cleanup_int_test.go @@ -29,34 +29,35 @@ var _ = Describe("feature cleanup", func() { ) var ( - dsci *dsciv1.DSCInitialization - namespace string - featuresHandler *feature.FeaturesHandler + dsci *dsciv1.DSCInitialization + namespace string + testFeature *feature.Feature ) BeforeAll(func() { namespace = envtestutil.AppendRandomNameTo("test-secret-ownership") dsci = fixtures.NewDSCInitialization(namespace) - featuresHandler = feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - secretCreationErr := feature.CreateFeature(featureName). - For(handler). - UsingConfig(envTest.Config). - PreConditions( - feature.CreateNamespaceIfNotExists(namespace), - ). - WithResources(fixtures.CreateSecret(secretName, namespace)). - Load() - - Expect(secretCreationErr).ToNot(HaveOccurred()) - - return nil - }) + var errSecretCreation error + testFeature, errSecretCreation = feature.Define(featureName). + TargetNamespace(dsci.Spec.ApplicationsNamespace). + Source(featurev1.Source{ + Type: featurev1.DSCIType, + Name: dsci.Name, + }). + UsingConfig(envTest.Config). + PreConditions( + feature.CreateNamespaceIfNotExists(namespace), + ). + WithResources(fixtures.CreateSecret(secretName, namespace)). + Create() + + Expect(errSecretCreation).ToNot(HaveOccurred()) }) It("should successfully create resource and associated feature tracker", func(ctx context.Context) { // when - Expect(featuresHandler.Apply(ctx)).Should(Succeed()) + Expect(testFeature.Apply(ctx)).Should(Succeed()) // then Eventually(createdSecretHasOwnerReferenceToOwningFeature(namespace, featureName)). @@ -68,7 +69,7 @@ var _ = Describe("feature cleanup", func() { It("should remove feature tracker on clean-up", func(ctx context.Context) { // when - Expect(featuresHandler.Delete(ctx)).To(Succeed()) + Expect(testFeature.Cleanup(ctx)).To(Succeed()) // then Consistently(createdSecretHasOwnerReferenceToOwningFeature(namespace, featureName)). @@ -103,20 +104,15 @@ var _ = Describe("feature cleanup", func() { err := fixtures.CreateOrUpdateNamespace(ctx, envTestClient, fixtures.NewNamespace("conditional-ns")) Expect(err).To(Not(HaveOccurred())) - featuresHandler = feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - conditionalCreationErr := feature.CreateFeature(featureName). - For(handler). + featuresHandler = feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + return registry.Add(feature.Define(featureName). UsingConfig(envTest.Config). + EnabledWhen(namespaceExists). PreConditions( feature.CreateNamespaceIfNotExists(namespace), ). - EnabledWhen(namespaceExists). - WithResources(fixtures.CreateSecret(secretName, namespace)). - Load() - - Expect(conditionalCreationErr).ToNot(HaveOccurred()) - - return nil + WithResources(fixtures.CreateSecret(secretName, namespace)), + ) }) // when @@ -136,20 +132,15 @@ var _ = Describe("feature cleanup", func() { Expect(err).To(Not(HaveOccurred())) // Mimic reconcile by re-loading the feature handler - featuresHandler = feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - conditionalCreationErr := feature.CreateFeature(featureName). - For(handler). + featuresHandler = feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + return registry.Add(feature.Define(featureName). UsingConfig(envTest.Config). + EnabledWhen(namespaceExists). PreConditions( feature.CreateNamespaceIfNotExists(namespace), ). - EnabledWhen(namespaceExists). - WithResources(fixtures.CreateSecret(secretName, namespace)). - Load() - - Expect(conditionalCreationErr).ToNot(HaveOccurred()) - - return nil + WithResources(fixtures.CreateSecret(secretName, namespace)), + ) }) Expect(featuresHandler.Apply(ctx)).Should(Succeed()) diff --git a/tests/integration/features/fixtures/templates/mesh-authz-ext-provider.patch.tmpl.yaml b/tests/integration/features/fixtures/templates/mesh-authz-ext-provider.patch.tmpl.yaml index 1a91092c7c2..fd2e906af19 100644 --- a/tests/integration/features/fixtures/templates/mesh-authz-ext-provider.patch.tmpl.yaml +++ b/tests/integration/features/fixtures/templates/mesh-authz-ext-provider.patch.tmpl.yaml @@ -7,7 +7,7 @@ spec: techPreview: meshConfig: extensionProviders: - - name: {{ .AppNamespace }}-auth-provider + - name: {{ .AuthExtensionName }} envoyExtAuthzGrpc: - service: {{ .AuthProviderName }}-authorino-authorization.{{ .Auth.Namespace }}.svc.cluster.local + service: {{ .AuthProviderName }}-authorino-authorization.{{ .AuthNamespace }}.svc.cluster.local port: 50051 diff --git a/tests/integration/features/manifests_int_test.go b/tests/integration/features/manifests_int_test.go index 139610b6032..e8a0cfee344 100644 --- a/tests/integration/features/manifests_int_test.go +++ b/tests/integration/features/manifests_int_test.go @@ -10,6 +10,7 @@ import ( dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/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/provider" "github.com/opendatahub-io/opendatahub-operator/v2/tests/envtestutil" "github.com/opendatahub-io/opendatahub-operator/v2/tests/integration/features/fixtures" @@ -43,15 +44,14 @@ var _ = Describe("Manifest sources", func() { It("should be able to process an embedded YAML file", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - createNamespaceErr := feature.CreateFeature("create-namespace"). - For(handler). + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + errNsCreate := registry.Add(feature.Define("create-namespace"). UsingConfig(envTest.Config). ManifestsLocation(fixtures.TestEmbeddedFiles). - Manifests(path.Join(fixtures.BaseDir, "namespace.yaml")). - Load() + Manifests(path.Join(fixtures.BaseDir, "namespace.yaml")), + ) - Expect(createNamespaceErr).ToNot(HaveOccurred()) + Expect(errNsCreate).ToNot(HaveOccurred()) return nil }) @@ -68,15 +68,15 @@ var _ = Describe("Manifest sources", func() { It("should be able to process an embedded template file", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - createServiceErr := feature.CreateFeature("create-local-gw-svc"). - For(handler). + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + errSvcCreate := registry.Add(feature.Define("create-local-gw-svc"). UsingConfig(envTest.Config). ManifestsLocation(fixtures.TestEmbeddedFiles). Manifests(path.Join(fixtures.BaseDir, "local-gateway-svc.tmpl.yaml")). - Load() + WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)), + ) - Expect(createServiceErr).ToNot(HaveOccurred()) + Expect(errSvcCreate).ToNot(HaveOccurred()) return nil }) @@ -101,15 +101,14 @@ metadata: Expect(fixtures.CreateFile(tempDir, "namespace.yaml", nsYAML)).To(Succeed()) - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - createServiceErr := feature.CreateFeature("create-namespace"). - For(handler). + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + errSvcCreate := registry.Add(feature.Define("create-namespace"). UsingConfig(envTest.Config). ManifestsLocation(os.DirFS(tempDir)). - Manifests(path.Join("namespace.yaml")). // must be relative to root DirFS defined above - Load() + Manifests(path.Join("namespace.yaml")), // must be relative to root DirFS defined above + ) - Expect(createServiceErr).ToNot(HaveOccurred()) + Expect(errSvcCreate).ToNot(HaveOccurred()) return nil }) diff --git a/tests/integration/features/preconditions_int_test.go b/tests/integration/features/preconditions_int_test.go index aec231eb6f3..f1310b13f89 100644 --- a/tests/integration/features/preconditions_int_test.go +++ b/tests/integration/features/preconditions_int_test.go @@ -41,14 +41,13 @@ var _ = Describe("feature preconditions", func() { defer objectCleaner.DeleteAll(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}}) // when - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - testFeatureErr := feature.CreateFeature("create-new-ns"). - For(handler). - PreConditions(feature.CreateNamespaceIfNotExists(namespace)). + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add(feature.Define("create-new-ns"). UsingConfig(envTest.Config). - Load() + PreConditions(feature.CreateNamespaceIfNotExists(namespace)), + ) - Expect(testFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -78,14 +77,13 @@ var _ = Describe("feature preconditions", func() { defer objectCleaner.DeleteAll(ctx, ns) // when - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - testFeatureErr := feature.CreateFeature("create-new-ns"). - For(handler). - PreConditions(feature.CreateNamespaceIfNotExists(namespace)). + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add(feature.Define("create-new-ns"). UsingConfig(envTest.Config). - Load() + PreConditions(feature.CreateNamespaceIfNotExists(namespace)), + ) - Expect(testFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) diff --git a/tests/integration/features/resources_int_test.go b/tests/integration/features/resources_int_test.go index b26a0c3148a..7fc3d29d52c 100644 --- a/tests/integration/features/resources_int_test.go +++ b/tests/integration/features/resources_int_test.go @@ -10,6 +10,7 @@ import ( dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/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/provider" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" "github.com/opendatahub-io/opendatahub-operator/v2/tests/envtestutil" "github.com/opendatahub-io/opendatahub-operator/v2/tests/integration/features/fixtures" @@ -52,15 +53,15 @@ var _ = Describe("Applying and updating resources", func() { When("a feature is managed", func() { It("should reconcile the resource to its managed state", func(ctx context.Context) { // given managed feature - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - return feature.CreateFeature("create-local-gw-svc"). - For(handler). - UsingConfig(envTest.Config). - Managed(). - ManifestsLocation(fixtures.TestEmbeddedFiles). - Manifests(path.Join(fixtures.BaseDir, "local-gateway-svc.tmpl.yaml")). - Load() - + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + return registry.Add( + feature.Define("create-local-gw-svc"). + UsingConfig(envTest.Config). + ManifestsLocation(fixtures.TestEmbeddedFiles). + Manifests(path.Join(fixtures.BaseDir, "local-gateway-svc.tmpl.yaml")). + Managed(). + WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)), + ) }) Expect(featuresHandler.Apply(ctx)).To(Succeed()) @@ -87,15 +88,15 @@ 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(dsci, func(handler *feature.FeaturesHandler) error { - return feature.CreateFeature("create-unmanaged-svc"). - For(handler). - UsingConfig(envTest.Config). - Managed(). - ManifestsLocation(fixtures.TestEmbeddedFiles). - Manifests(path.Join(fixtures.BaseDir, "unmanaged-svc.tmpl.yaml")). - Load() - + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + return registry.Add( + feature.Define("create-unmanaged-svc"). + UsingConfig(envTest.Config). + ManifestsLocation(fixtures.TestEmbeddedFiles). + Manifests(path.Join(fixtures.BaseDir, "unmanaged-svc.tmpl.yaml")). + Managed(). + WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)), + ) }) Expect(featuresHandler.Apply(ctx)).To(Succeed()) @@ -121,13 +122,14 @@ var _ = Describe("Applying and updating resources", func() { It("should not reconcile the resource", func(ctx context.Context) { // given unmanaged feature - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - return feature.CreateFeature("create-local-gw-svc"). - For(handler). - UsingConfig(envTest.Config). - ManifestsLocation(fixtures.TestEmbeddedFiles). - Manifests(path.Join(fixtures.BaseDir, "local-gateway-svc.tmpl.yaml")). - Load() + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + return registry.Add( + feature.Define("create-local-gw-svc"). + UsingConfig(envTest.Config). + ManifestsLocation(fixtures.TestEmbeddedFiles). + Manifests(path.Join(fixtures.BaseDir, "local-gateway-svc.tmpl.yaml")). + WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)), + ) }) Expect(featuresHandler.Apply(ctx)).To(Succeed()) @@ -152,13 +154,14 @@ 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(dsci, func(handler *feature.FeaturesHandler) error { - return feature.CreateFeature("create-managed-svc"). - For(handler). - UsingConfig(envTest.Config). - ManifestsLocation(fixtures.TestEmbeddedFiles). - Manifests(path.Join(fixtures.BaseDir, "managed-svc.tmpl.yaml")). - Load() + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + return registry.Add( + feature.Define("create-managed-svc"). + UsingConfig(envTest.Config). + ManifestsLocation(fixtures.TestEmbeddedFiles). + Manifests(path.Join(fixtures.BaseDir, "managed-svc.tmpl.yaml")). + WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)), + ) }) Expect(featuresHandler.Apply(ctx)).To(Succeed()) diff --git a/tests/integration/features/serverless_feature_test.go b/tests/integration/features/serverless_feature_test.go index a937e4ea55c..57190d7a45c 100644 --- a/tests/integration/features/serverless_feature_test.go +++ b/tests/integration/features/serverless_feature_test.go @@ -18,6 +18,7 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/components/kserve" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/serverless" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/servicemesh" "github.com/opendatahub-io/opendatahub-operator/v2/tests/envtestutil" "github.com/opendatahub-io/opendatahub-operator/v2/tests/integration/features/fixtures" @@ -49,14 +50,14 @@ var _ = Describe("Serverless feature", func() { It("should fail on precondition check", func(ctx context.Context) { // given - featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), &dsci.Spec, func(handler *feature.FeaturesHandler) error { - verificationFeatureErr := feature.CreateFeature("no-serverless-operator-check"). - For(handler). - UsingConfig(envTest.Config). - PreConditions(serverless.EnsureServerlessOperatorInstalled). - Load() + featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add( + feature.Define("no-serverless-operator-check"). + UsingConfig(envTest.Config). + PreConditions(serverless.EnsureServerlessOperatorInstalled), + ) - Expect(verificationFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -96,14 +97,14 @@ var _ = Describe("Serverless feature", func() { It("should succeed checking operator installation using precondition", func(ctx context.Context) { // when - featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), &dsci.Spec, func(handler *feature.FeaturesHandler) error { - verificationFeatureErr := feature.CreateFeature("serverless-operator-check"). - For(handler). - UsingConfig(envTest.Config). - PreConditions(serverless.EnsureServerlessOperatorInstalled). - Load() + featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add( + feature.Define("serverless-operator-check"). + UsingConfig(envTest.Config). + PreConditions(serverless.EnsureServerlessOperatorInstalled), + ) - Expect(verificationFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -114,14 +115,14 @@ var _ = Describe("Serverless feature", func() { It("should succeed if serving is not installed for KNative serving precondition", func(ctx context.Context) { // when - featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), &dsci.Spec, func(handler *feature.FeaturesHandler) error { - verificationFeatureErr := feature.CreateFeature("no-serving-installed-yet"). - For(handler). - UsingConfig(envTest.Config). - PreConditions(serverless.EnsureServerlessAbsent). - Load() + featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add( + feature.Define("no-serving-installed-yet"). + UsingConfig(envTest.Config). + PreConditions(serverless.EnsureServerlessAbsent), + ) - Expect(verificationFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -143,14 +144,14 @@ var _ = Describe("Serverless feature", func() { Expect(envTestClient.Create(ctx, knativeServing)).To(Succeed()) // when - featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), &dsci.Spec, func(handler *feature.FeaturesHandler) error { - verificationFeatureErr := feature.CreateFeature("serving-already-installed"). - For(handler). - UsingConfig(envTest.Config). - PreConditions(serverless.EnsureServerlessAbsent). - Load() + featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add( + feature.Define("serving-already-installed"). + UsingConfig(envTest.Config). + PreConditions(serverless.EnsureServerlessAbsent), + ) - Expect(verificationFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -170,10 +171,6 @@ var _ = Describe("Serverless feature", func() { // Stubbing feature as we want to test particular functions in isolation testFeature = &feature.Feature{ Name: "test-feature", - Spec: &feature.Spec{ - ServiceMeshSpec: &infrav1.ServiceMeshSpec{}, - Serving: &infrav1.ServingSpec{}, - }, } testFeature.Client = envTestClient @@ -182,39 +179,81 @@ var _ = Describe("Serverless feature", func() { Context("ingress gateway TLS secret name", func() { It("should set default value when value is empty in the DSCI", func(ctx context.Context) { - // Default value is blank -> testFeature.Spec.Serving.IngressGateway.Certificate.SecretName = "" - Expect(serverless.ServingDefaultValues(ctx, testFeature)).To(Succeed()) - Expect(testFeature.Spec.KnativeCertificateSecret).To(Equal(serverless.DefaultCertificateSecretName)) + // given + serving := infrav1.ServingSpec{ + IngressGateway: infrav1.IngressGatewaySpec{ + Certificate: infrav1.CertificateSpec{ + SecretName: "", + }, + }, + } + + // when + actualSecretName, err := serverless.FeatureData.CertificateName.Define(&serving).Value(ctx, envTestClient) + + // then + Expect(err).ToNot(HaveOccurred()) + Expect(actualSecretName).To(Equal(serverless.DefaultCertificateSecretName)) }) It("should use user value when set in the DSCI", func(ctx context.Context) { - testFeature.Spec.Serving.IngressGateway.Certificate.SecretName = "fooBar" - Expect(serverless.ServingDefaultValues(ctx, testFeature)).To(Succeed()) - Expect(testFeature.Spec.KnativeCertificateSecret).To(Equal("fooBar")) + // given + serving := infrav1.ServingSpec{ + IngressGateway: infrav1.IngressGatewaySpec{ + Certificate: infrav1.CertificateSpec{ + SecretName: "top-secret-service", + }, + }, + } + + // when + actualSecretName, err := serverless.FeatureData.CertificateName.Define(&serving).Value(ctx, envTestClient) + + // then + Expect(err).ToNot(HaveOccurred()) + Expect(actualSecretName).To(Equal("top-secret-service")) }) }) Context("ingress domain name suffix", func() { It("should use OpenShift ingress domain when value is empty in the DSCI", func(ctx context.Context) { - // Create KNativeServing the CRD + // given osIngressResource := &unstructured.Unstructured{} Expect(yaml.Unmarshal([]byte(fixtures.OpenshiftClusterIngress), osIngressResource)).ToNot(HaveOccurred()) - c, err := client.New(envTest.Config, client.Options{}) - Expect(err).ToNot(HaveOccurred()) - Expect(c.Create(ctx, osIngressResource)).To(Succeed()) + Expect(envTestClient.Create(ctx, osIngressResource)).To(Succeed()) + + serving := infrav1.ServingSpec{ + IngressGateway: infrav1.IngressGatewaySpec{ + Domain: "", + }, + } - // Default value is blank -> testFeature.Spec.Serving.IngressGateway.Domain = "" - Expect(serverless.ServingIngressDomain(ctx, testFeature)).To(Succeed()) - Expect(testFeature.Spec.KnativeIngressDomain).To(Equal("*.foo.io")) + // when + domain, err := serverless.FeatureData.IngressDomain.Define(&serving).Value(ctx, envTestClient) + + // then + Expect(err).ToNot(HaveOccurred()) + Expect(domain).To(Equal("*.foo.io")) }) It("should use user value when set in the DSCI", func(ctx context.Context) { - testFeature.Spec.Serving.IngressGateway.Domain = fixtures.TestDomainFooCom - Expect(serverless.ServingIngressDomain(ctx, testFeature)).To(Succeed()) - Expect(testFeature.Spec.KnativeIngressDomain).To(Equal(fixtures.TestDomainFooCom)) + // given + serving := infrav1.ServingSpec{ + IngressGateway: infrav1.IngressGatewaySpec{ + Domain: fixtures.TestDomainFooCom, + }, + } + + // when + domain, err := serverless.FeatureData.IngressDomain.Define(&serving).Value(ctx, envTestClient) + + // then + Expect(err).ToNot(HaveOccurred()) + Expect(domain).To(Equal(fixtures.TestDomainFooCom)) }) }) + }) Context("resources creation", func() { @@ -240,19 +279,20 @@ var _ = Describe("Serverless feature", func() { kserveComponent.Serving.IngressGateway.Certificate.Type = infrav1.SelfSigned kserveComponent.Serving.IngressGateway.Domain = fixtures.TestDomainFooCom - featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), &dsci.Spec, func(handler *feature.FeaturesHandler) error { - verificationFeatureErr := feature.CreateFeature("tls-secret-creation"). - For(handler). - UsingConfig(envTest.Config). - WithData( - kserve.PopulateComponentSettings(kserveComponent), - serverless.ServingDefaultValues, - serverless.ServingIngressDomain, - ). - WithResources(serverless.ServingCertificateResource). - Load() + featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add( + feature.Define("tls-secret-creation"). + UsingConfig(envTest.Config). + WithData( + servicemesh.FeatureData.ControlPlane.Define(&dsci.Spec).AsAction(), + serverless.FeatureData.Serving.Define(&kserveComponent.Serving).AsAction(), + serverless.FeatureData.IngressDomain.Define(&kserveComponent.Serving).AsAction(), + serverless.FeatureData.CertificateName.Define(&kserveComponent.Serving).AsAction(), + ). + WithResources(serverless.ServingCertificateResource), + ) - Expect(verificationFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -279,19 +319,20 @@ var _ = Describe("Serverless feature", func() { // given kserveComponent.Serving.IngressGateway.Certificate.Type = infrav1.Provided kserveComponent.Serving.IngressGateway.Domain = fixtures.TestDomainFooCom - featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), &dsci.Spec, func(handler *feature.FeaturesHandler) error { - verificationFeatureErr := feature.CreateFeature("tls-secret-creation"). - For(handler). - UsingConfig(envTest.Config). - WithData( - kserve.PopulateComponentSettings(kserveComponent), - serverless.ServingDefaultValues, - serverless.ServingIngressDomain, - ). - WithResources(serverless.ServingCertificateResource). - Load() - - Expect(verificationFeatureErr).ToNot(HaveOccurred()) + featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add( + feature.Define("tls-secret-creation"). + UsingConfig(envTest.Config). + WithData( + servicemesh.FeatureData.ControlPlane.Define(&dsci.Spec).AsAction(), + serverless.FeatureData.Serving.Define(&kserveComponent.Serving).AsAction(), + serverless.FeatureData.IngressDomain.Define(&kserveComponent.Serving).AsAction(), + serverless.FeatureData.CertificateName.Define(&kserveComponent.Serving).AsAction(), + ). + WithResources(serverless.ServingCertificateResource), + ) + + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) diff --git a/tests/integration/features/servicemesh_feature_test.go b/tests/integration/features/servicemesh_feature_test.go index 25ff95abccb..bd1cc661ecf 100644 --- a/tests/integration/features/servicemesh_feature_test.go +++ b/tests/integration/features/servicemesh_feature_test.go @@ -15,6 +15,7 @@ import ( 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" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/provider" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/servicemesh" "github.com/opendatahub-io/opendatahub-operator/v2/tests/envtestutil" "github.com/opendatahub-io/opendatahub-operator/v2/tests/integration/features/fixtures" @@ -50,14 +51,13 @@ var _ = Describe("Service Mesh setup", func() { It("should fail using precondition check", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - verificationFeatureErr := feature.CreateFeature("no-service-mesh-operator-check"). - For(handler). + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add(feature.Define("no-service-mesh-operator-check"). UsingConfig(envTest.Config). - PreConditions(servicemesh.EnsureServiceMeshOperatorInstalled). - Load() + PreConditions(servicemesh.EnsureServiceMeshOperatorInstalled), + ) - Expect(verificationFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -85,14 +85,15 @@ var _ = Describe("Service Mesh setup", func() { It("should succeed using precondition check", func(ctx context.Context) { // when - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - verificationFeatureErr := feature.CreateFeature("service-mesh-operator-check"). - For(handler). - UsingConfig(envTest.Config). - PreConditions(servicemesh.EnsureServiceMeshOperatorInstalled). - Load() + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add( + feature.Define("service-mesh-operator-check"). + UsingConfig(envTest.Config). + WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)). + PreConditions(servicemesh.EnsureServiceMeshOperatorInstalled), + ) - Expect(verificationFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -117,14 +118,14 @@ var _ = Describe("Service Mesh setup", func() { dsci.Spec.ServiceMesh.ControlPlane.Name = "test-name" // when - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - verificationFeatureErr := feature.CreateFeature("service-mesh-control-plane-check"). - For(handler). + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add(feature.Define("service-mesh-control-plane-check"). UsingConfig(envTest.Config). - PreConditions(servicemesh.EnsureServiceMeshInstalled). - Load() + WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)). + PreConditions(servicemesh.EnsureServiceMeshInstalled), + ) - Expect(verificationFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -136,16 +137,17 @@ var _ = Describe("Service Mesh setup", func() { It("should fail to find Service Mesh Control Plane if not present", func(ctx context.Context) { // given dsci.Spec.ServiceMesh.ControlPlane.Name = "test-name" + dsci.Spec.ServiceMesh.ControlPlane.Namespace = "test-namespace" // when - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - verificationFeatureErr := feature.CreateFeature("no-service-mesh-control-plane-check"). - For(handler). + 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)). UsingConfig(envTest.Config). - PreConditions(servicemesh.EnsureServiceMeshInstalled). - Load() + PreConditions(servicemesh.EnsureServiceMeshInstalled), + ) - Expect(verificationFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -195,16 +197,20 @@ var _ = Describe("Service Mesh setup", func() { createServiceMeshControlPlane(ctx, name, namespace) - handler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - return feature.CreateFeature("control-plane-with-external-authz-provider"). - For(handler). + handler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + return registry.Add(feature.Define("control-plane-with-external-authz-provider"). + UsingConfig(envTest.Config). ManifestsLocation(fixtures.TestEmbeddedFiles). Manifests(path.Join("templates", "mesh-authz-ext-provider.patch.tmpl.yaml")). + WithData( + servicemesh.FeatureData.Authorization.All(&dsci.Spec)..., + ). + WithData( + servicemesh.FeatureData.ControlPlane.Define(&dsci.Spec).AsAction(), + ). OnDelete( servicemesh.RemoveExtensionProvider, - ). - UsingConfig(envTest.Config). - Load() + )) }) // when @@ -233,7 +239,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()) - Eventually(func() []interface{} { + Eventually(func(ctx context.Context) []any { serviceMeshControlPlane, err := getServiceMeshControlPlane(ctx, namespace, name) Expect(err).ToNot(HaveOccurred()) @@ -247,7 +253,11 @@ var _ = Describe("Service Mesh setup", func() { return extensionProviders - }).WithTimeout(fixtures.Timeout).WithPolling(fixtures.Interval).Should(BeEmpty()) + }). + WithTimeout(fixtures.Timeout). + WithPolling(fixtures.Interval). + WithContext(ctx). + Should(BeEmpty()) }) }) @@ -273,7 +283,7 @@ func installServiceMeshCRD(ctx context.Context) *apiextensionsv1.CustomResourceD func createServiceMeshControlPlane(ctx context.Context, name string, namespace string) { serviceMeshControlPlane := &unstructured.Unstructured{ Object: map[string]interface{}{ - "apiVersion": "maistra.io/v2", + "apiVersion": "maistra.io/featurev1", "kind": "ServiceMeshControlPlane", "metadata": map[string]interface{}{ "name": name, diff --git a/tests/integration/features/tracker_int_test.go b/tests/integration/features/tracker_int_test.go index fe7480d8d78..3f7af1528d9 100644 --- a/tests/integration/features/tracker_int_test.go +++ b/tests/integration/features/tracker_int_test.go @@ -33,13 +33,13 @@ 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(dsci, func(handler *feature.FeaturesHandler) error { - verificationFeatureErr := feature.CreateFeature("always-working-feature"). - For(handler). - UsingConfig(envTest.Config). - Load() + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add( + feature.Define("always-working-feature"). + UsingConfig(envTest.Config), + ) - Expect(verificationFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -62,16 +62,15 @@ 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(dsci, func(handler *feature.FeaturesHandler) error { - verificationFeatureErr := feature.CreateFeature("precondition-fail"). - For(handler). + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add(feature.Define("precondition-fail"). UsingConfig(envTest.Config). PreConditions(func(_ context.Context, _ *feature.Feature) error { return errors.New("during test always fail") - }). - Load() + }), + ) - Expect(verificationFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -94,16 +93,15 @@ 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(dsci, func(handler *feature.FeaturesHandler) error { - verificationFeatureErr := feature.CreateFeature("post-condition-failure"). - For(handler). + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add(feature.Define("post-condition-failure"). UsingConfig(envTest.Config). PostConditions(func(_ context.Context, _ *feature.Feature) error { return errors.New("during test always fail") - }). - Load() + }), + ) - Expect(verificationFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -129,13 +127,12 @@ var _ = Describe("Feature tracking capability", func() { It("should correctly indicate source in the feature tracker", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - emptyFeatureErr := feature.CreateFeature("always-working-feature"). - For(handler). - UsingConfig(envTest.Config). - Load() + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add(feature.Define("always-working-feature"). + UsingConfig(envTest.Config), + ) - Expect(emptyFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil }) @@ -156,13 +153,12 @@ var _ = Describe("Feature tracking capability", func() { It("should correctly indicate app namespace in the feature tracker", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error { - emptyFeatureErr := feature.CreateFeature("empty-feature"). - For(handler). - UsingConfig(envTest.Config). - Load() + featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add(feature.Define("empty-feature"). + UsingConfig(envTest.Config), + ) - Expect(emptyFeatureErr).ToNot(HaveOccurred()) + Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil })