From 4e8bb43b2d4d1a77ebfee9858e482e1382a04626 Mon Sep 17 00:00:00 2001 From: Gerard Ryan Date: Tue, 25 Feb 2025 17:58:11 +0000 Subject: [PATCH 1/3] Add tests around FTs in Kserve before removal JIRA: https://issues.redhat.com/browse/RHOAIENG-18045 Some of these tests will change when the code changes, and the hope is that those test changes will give more clarity and confidence in the code changes. --- tests/e2e/kserve_test.go | 61 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/tests/e2e/kserve_test.go b/tests/e2e/kserve_test.go index 20575d47e8c..08d2177f557 100644 --- a/tests/e2e/kserve_test.go +++ b/tests/e2e/kserve_test.go @@ -11,6 +11,7 @@ import ( k8serr "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -44,6 +45,9 @@ func kserveTestSuite(t *testing.T) { t.Run("Validate FeatureTrackers", componentCtx.validateFeatureTrackers) t.Run("Validate model controller", componentCtx.validateModelControllerInstance) t.Run("Validate operands have OwnerReferences", componentCtx.ValidateOperandsOwnerReferences) + t.Run("Validate Kserve owns FeatureTrackers", componentCtx.ValidateKserveOwnsFeatureTrackers) + t.Run("Validate FeatureTrackers own children", componentCtx.ValidateFeatureTrackersOwnChildren) + t.Run("Validate KnativeServing Structure", componentCtx.ValidateKnativeServingStructure) t.Run("Validate default certs", componentCtx.validateDefaultCertsAvailable) t.Run("Validate update operand resources", componentCtx.ValidateUpdateDeploymentsResources) t.Run("Validate component disabled", componentCtx.ValidateComponentDisabled) @@ -206,3 +210,60 @@ func (c *KserveTestCtx) validateDefaultCertsAvailable(t *testing.T) { g.Expect(ctrlPlaneSecret.Type).Should(Equal(defaultIngressSecret.Type)) g.Expect(defaultIngressSecret.Data).Should(Equal(ctrlPlaneSecret.Data)) } + +func (c *KserveTestCtx) ValidateKserveOwnsFeatureTrackers(t *testing.T) { + g := c.NewWithT(t) + + fts := []string{ + c.ApplicationNamespace + "-kserve-external-authz", + c.ApplicationNamespace + "-serverless-serving-gateways", + c.ApplicationNamespace + "-serverless-serving-deployment", + c.ApplicationNamespace + "-serverless-net-istio-secret-filtering", + } + + for _, ft := range fts { + g.Get( + gvk.FeatureTracker, types.NamespacedName{Name: ft}, + ).Eventually().Should( + jq.Match(`.metadata.ownerReferences | any(.kind == "%s")`, gvk.Kserve.Kind), + `Ensuring Kserve ownership of FeatureTracker %s`, ft, + ) + } +} + +func (c *KserveTestCtx) ValidateFeatureTrackersOwnChildren(t *testing.T) { + g := c.NewWithT(t) + + children := []struct { + gvk schema.GroupVersionKind + nn types.NamespacedName + }{ + {gvk.KnativeServing, types.NamespacedName{Namespace: "knative-serving", Name: "knative-serving"}}, + {gvk.ServiceMeshMember, types.NamespacedName{Namespace: "knative-serving", Name: "default"}}, + {gvk.EnvoyFilter, types.NamespacedName{Namespace: "istio-system", Name: "activator-host-header"}}, + {gvk.EnvoyFilter, types.NamespacedName{Namespace: "istio-system", Name: "envoy-oauth-temp-fix-after"}}, + {gvk.EnvoyFilter, types.NamespacedName{Namespace: "istio-system", Name: "envoy-oauth-temp-fix-before"}}, + {gvk.EnvoyFilter, types.NamespacedName{Namespace: "istio-system", Name: "kserve-inferencegraph-host-header"}}, + {gvk.AuthorizationPolicy, types.NamespacedName{Namespace: "istio-system", Name: "kserve-inferencegraph"}}, + {gvk.AuthorizationPolicy, types.NamespacedName{Namespace: "istio-system", Name: "kserve-predictor"}}, + {gvk.Gateway, types.NamespacedName{Namespace: "istio-system", Name: "kserve-local-gateway"}}, + {gvk.Gateway, types.NamespacedName{Namespace: "knative-serving", Name: "knative-ingress-gateway"}}, + {gvk.Gateway, types.NamespacedName{Namespace: "knative-serving", Name: "knative-local-gateway"}}, + } + + for _, child := range children { + g.Get(child.gvk, child.nn).Eventually().Should( + jq.Match(`.metadata.ownerReferences | any(.kind == "%s")`, gvk.FeatureTracker.Kind), + `Checking if %s/%s in %s has expected owner refs`, child.gvk, child.nn.Name, child.nn.Namespace, + ) + } +} + +func (c *KserveTestCtx) ValidateKnativeServingStructure(t *testing.T) { + g := c.NewWithT(t) + + g.Get(gvk.KnativeServing, types.NamespacedName{Namespace: "knative-serving", Name: "knative-serving"}).Eventually().Should(And( + jq.Match(`.spec.workloads | length == 3`), + jq.Match(`.metadata.annotations."serverless.openshift.io/default-enable-http2" == "true"`), + ), `Ensuring KnativeServing has content from both templates`) +} From 17b7a7df0bb29e831b3c0240f532a4c7be0bb8de Mon Sep 17 00:00:00 2001 From: Gerard Ryan Date: Mon, 20 Jan 2025 02:39:43 +0000 Subject: [PATCH 2/3] Remove FeatureTracker code from Kserve reconciler Jira: https://issues.redhat.com/browse/RHOAIENG-18045 --- .../components/kserve/kserve_controller.go | 58 ++---- .../kserve/kserve_controller_actions.go | 132 ++++++++++--- .../components/kserve/kserve_support.go | 187 +++++------------- pkg/cluster/gvk/gvk.go | 6 + tests/e2e/kserve_test.go | 103 +++------- 5 files changed, 200 insertions(+), 286 deletions(-) diff --git a/controllers/components/kserve/kserve_controller.go b/controllers/components/kserve/kserve_controller.go index 397bc0fda47..7b882286784 100644 --- a/controllers/components/kserve/kserve_controller.go +++ b/controllers/components/kserve/kserve_controller.go @@ -33,15 +33,14 @@ import ( componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" - featuresv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/gc" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render/kustomize" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render/template" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/status/releases" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/updatestatus" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/handlers" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/predicates" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/predicates/clusterrole" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/predicates/component" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/predicates/generation" @@ -53,8 +52,6 @@ import ( // NewComponentReconciler creates a ComponentReconciler for the Dashboard API. func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error { - ownedViaFTMapFunc := ownedViaFT(mgr.GetClient()) - _, err := reconciler.ReconcilerFor(mgr, &componentApi.Kserve{}). // operands - owned Owns(&corev1.Secret{}). @@ -69,12 +66,20 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl. // changes. The compareHashPredicate ensures that we don't needlessly enqueue // requests if there are no changes that we don't care about. Owns(&templatev1.Template{}, reconciler.WithPredicates(hash.Updated())). - Owns(&featuresv1.FeatureTracker{}). Owns(&networkingv1.NetworkPolicy{}). Owns(&monitoringv1.ServiceMonitor{}). Owns(&admissionregistrationv1.MutatingWebhookConfiguration{}). Owns(&admissionregistrationv1.ValidatingWebhookConfiguration{}). Owns(&appsv1.Deployment{}, reconciler.WithPredicates(resources.NewDeploymentPredicate())). + + // operands - dynamically owned + OwnsGVK(gvk.Gateway, reconciler.Dynamic(ifGVKInstalled(gvk.Gateway))). + OwnsGVK(gvk.EnvoyFilter, reconciler.Dynamic(ifGVKInstalled(gvk.EnvoyFilter))). + OwnsGVK(gvk.KnativeServing, reconciler.Dynamic(ifGVKInstalled(gvk.KnativeServing))). + OwnsGVK(gvk.ServiceMeshMember, reconciler.Dynamic(ifGVKInstalled(gvk.ServiceMeshMember))). + OwnsGVK(gvk.AuthorizationPolicy, reconciler.Dynamic(ifGVKInstalled(gvk.AuthorizationPolicy))). + OwnsGVK(gvk.AuthorizationPolicyv1beta1, reconciler.Dynamic(ifGVKInstalled(gvk.AuthorizationPolicyv1beta1))). + // operands - watched // // By default the Watches functions adds: @@ -111,51 +116,20 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl. reconciler.WithEventHandler(handlers.ToNamed(componentApi.KserveInstanceName)), reconciler.WithPredicates(predicate.Or(generation.New(), resources.DSCIReadiness)), ). - // operands - dynamically watched - // - // A watch will be created dynamically for these kinds, if they exist on the cluster - // (they come from ServiceMesh and Serverless operators). - // - // They're owned by FeatureTrackers, which are owned by a Kserve; so there's a - // custom event mapper to enqueue a reconcile request for a Kserve object, if - // applicable. - // - // They also don't have the "partOf" label that Watches expects in the - // implicit predicate, so the simpler "DefaultPredicate" is also added. - WatchesGVK( - gvk.KnativeServing, - reconciler.Dynamic(ifGVKInstalled(gvk.KnativeServing)), - reconciler.WithEventMapper(ownedViaFTMapFunc), - reconciler.WithPredicates(predicates.DefaultPredicate)). - WatchesGVK( - gvk.ServiceMeshMember, - reconciler.Dynamic(ifGVKInstalled(gvk.ServiceMeshMember)), - reconciler.WithEventMapper(ownedViaFTMapFunc), - reconciler.WithPredicates(predicates.DefaultPredicate)). - WatchesGVK( - gvk.EnvoyFilter, - reconciler.Dynamic(ifGVKInstalled(gvk.EnvoyFilter)), - reconciler.WithEventMapper(ownedViaFTMapFunc), - reconciler.WithPredicates(predicates.DefaultPredicate)). - WatchesGVK( - gvk.AuthorizationPolicy, - reconciler.Dynamic(ifGVKInstalled(gvk.AuthorizationPolicy)), - reconciler.WithEventMapper(ownedViaFTMapFunc), - reconciler.WithPredicates(predicates.DefaultPredicate)). - WatchesGVK( - gvk.Gateway, - reconciler.Dynamic(ifGVKInstalled(gvk.Gateway)), - reconciler.WithEventMapper(ownedViaFTMapFunc), - reconciler.WithPredicates(predicates.DefaultPredicate)). // actions WithAction(checkPreConditions). WithAction(initialize). WithAction(devFlags). WithAction(releases.NewAction()). - WithAction(removeLegacyFeatureTrackerOwnerRef). WithAction(configureServerless). WithAction(configureServiceMesh). + WithAction(template.NewAction( + template.WithCache(), + template.WithDataFn(getTemplateData), + )). + WithAction(rmFTOwnerRefs). + WithAction(deleteFeatureTrackers). WithAction(kustomize.NewAction( kustomize.WithCache(), // These are the default labels added by the legacy deploy method diff --git a/controllers/components/kserve/kserve_controller_actions.go b/controllers/components/kserve/kserve_controller_actions.go index 55c755baaa8..3764e6e572f 100644 --- a/controllers/components/kserve/kserve_controller_actions.go +++ b/controllers/components/kserve/kserve_controller_actions.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" "strings" operatorv1 "github.com/openshift/api/operator/v1" @@ -24,7 +25,6 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/conditions" odhtypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" ) @@ -142,7 +142,7 @@ func devFlags(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { return nil } -func removeLegacyFeatureTrackerOwnerRef(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { +func deleteFeatureTrackers(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { ftNames := []string{ rr.DSCI.Spec.ApplicationsNamespace + "-serverless-serving-deployment", rr.DSCI.Spec.ApplicationsNamespace + "-serverless-net-istio-secret-filtering", @@ -150,24 +150,51 @@ func removeLegacyFeatureTrackerOwnerRef(ctx context.Context, rr *odhtypes.Reconc rr.DSCI.Spec.ApplicationsNamespace + "-kserve-external-authz", } - for _, ftName := range ftNames { - obj := &featuresv1.FeatureTracker{} - err := rr.Client.Get(ctx, client.ObjectKey{Name: ftName}, obj) - switch { - case k8serr.IsNotFound(err): + for _, n := range ftNames { + ft := featuresv1.FeatureTracker{} + err := rr.Client.Get(ctx, client.ObjectKey{Name: n}, &ft) + if k8serr.IsNotFound(err) { continue - case err != nil: - return fmt.Errorf("error while retrieving FeatureTracker %s: %w", ftName, err) + } else if err != nil { + return fmt.Errorf("failed to lookup FeatureTracker %s: %w", ft.GetName(), err) } - if err := resources.RemoveOwnerReferences(ctx, rr.Client, obj, isLegacyOwnerRef); err != nil { - return err + err = rr.Client.Delete(ctx, &ft, client.PropagationPolicy(metav1.DeletePropagationForeground)) + if k8serr.IsNotFound(err) { + continue + } else if err != nil { + return fmt.Errorf("failed to delete FeatureTracker %s: %w", ft.GetName(), err) } } return nil } +func rmFTOwnerRefs(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { + hasFTOwner := func(or metav1.OwnerReference) bool { return or.Kind == gvk.FeatureTracker.Kind } + + for _, res := range rr.Resources { + current := resources.GvkToUnstructured(res.GroupVersionKind()) + + lookupErr := rr.Client.Get(ctx, client.ObjectKeyFromObject(&res), current) + switch { + case k8serr.IsNotFound(lookupErr): + continue + case lookupErr != nil: + return fmt.Errorf("failed to lookup object %s/%s: %w", res.GetNamespace(), res.GetName(), lookupErr) + default: + ors := slices.DeleteFunc(current.GetOwnerReferences(), hasFTOwner) + + if len(ors) < len(current.GetOwnerReferences()) { + if err := resources.RemoveOwnerReferences(ctx, rr.Client, current, hasFTOwner); err != nil { + return fmt.Errorf("failed to remove FeatureTracker owner: %w", err) + } + } + } + } + return nil +} + func configureServerless(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { k, ok := rr.Instance.(*componentApi.Kserve) if !ok { @@ -183,9 +210,6 @@ func configureServerless(ctx context.Context, rr *odhtypes.ReconciliationRequest case operatorv1.Removed: // we remove serving CR logger.Info("existing Serverless CR (owned by operator) will be removed") - if err := removeServerlessFeatures(ctx, rr.Client, k, &rr.DSCI.Spec); err != nil { - return err - } case operatorv1.Managed: // standard workflow to create CR if rr.DSCI.Spec.ServiceMesh == nil { @@ -199,34 +223,88 @@ func configureServerless(ctx context.Context, rr *odhtypes.ReconciliationRequest "as it is required by the KServe serving field", rr.DSCI.Spec.ServiceMesh.ManagementState) } - serverlessFeatures := feature.ComponentFeaturesHandler(rr.Instance, componentName, rr.DSCI.Spec.ApplicationsNamespace, configureServerlessFeatures(&rr.DSCI.Spec, k)) + err := createServingCertResource(ctx, cli, &rr.DSCI.Spec, k) + if err != nil { + return fmt.Errorf("unable to create serverless serving certificate secret: %w", err) + } - if err := serverlessFeatures.Apply(ctx, cli); err != nil { - return err + templates := []odhtypes.TemplateInfo{ + { + FS: resourcesFS, + Path: "resources/serving-install/service-mesh-subscription.tmpl.yaml", + }, + { + FS: resourcesFS, + Path: "resources/serving-install/knative-serving.tmpl.yaml", + }, + { + FS: resourcesFS, + Path: "resources/serving-net-istio-secret-filtering.patch.tmpl.yaml", + }, + + { + FS: resourcesFS, + Path: "resources/servicemesh/routing/istio-ingress-gateway.tmpl.yaml", + }, + { + FS: resourcesFS, + Path: "resources/servicemesh/routing/istio-kserve-local-gateway.tmpl.yaml", + }, + { + FS: resourcesFS, + Path: "resources/servicemesh/routing/istio-local-gateway.yaml", + }, + { + FS: resourcesFS, + Path: "resources/servicemesh/routing/kserve-local-gateway-svc.tmpl.yaml", + }, + { + FS: resourcesFS, + Path: "resources/servicemesh/routing/local-gateway-svc.tmpl.yaml", + }, } + + rr.Templates = append(rr.Templates, templates...) } return nil } func configureServiceMesh(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { - k, ok := rr.Instance.(*componentApi.Kserve) - if !ok { - return fmt.Errorf("resource instance %v is not a componentApi.Kserve)", rr.Instance) - } - - cli := rr.Client - if rr.DSCI.Spec.ServiceMesh != nil { if rr.DSCI.Spec.ServiceMesh.ManagementState == operatorv1.Managed { - serviceMeshInitializer := feature.ComponentFeaturesHandler(k, componentName, rr.DSCI.Spec.ApplicationsNamespace, defineServiceMeshFeatures(ctx, cli, &rr.DSCI.Spec)) - return serviceMeshInitializer.Apply(ctx, cli) + templates := []odhtypes.TemplateInfo{ + { + FS: resourcesFS, + Path: "resources/servicemesh/activator-envoyfilter.tmpl.yaml", + }, + { + FS: resourcesFS, + Path: "resources/servicemesh/envoy-oauth-temp-fix.tmpl.yaml", + }, + { + FS: resourcesFS, + Path: "resources/servicemesh/kserve-predictor-authorizationpolicy.tmpl.yaml", + }, + { + FS: resourcesFS, + Path: "resources/servicemesh/kserve-inferencegraph-envoyfilter.tmpl.yaml", + }, + { + FS: resourcesFS, + Path: "resources/servicemesh/kserve-inferencegraph-authorizationpolicy.tmpl.yaml", + }, + } + + rr.Templates = append(rr.Templates, templates...) + + return nil } if rr.DSCI.Spec.ServiceMesh.ManagementState == operatorv1.Unmanaged { return nil } } - return removeServiceMeshConfigurations(ctx, cli, k, &rr.DSCI.Spec) + return nil } func customizeKserveConfigMap(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { diff --git a/controllers/components/kserve/kserve_support.go b/controllers/components/kserve/kserve_support.go index 1e18661eeaa..a24b29b72c9 100644 --- a/controllers/components/kserve/kserve_support.go +++ b/controllers/components/kserve/kserve_support.go @@ -2,36 +2,30 @@ package kserve import ( "context" + "embed" "encoding/base64" "encoding/json" "fmt" - "path" corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/reconcile" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" - featuresv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" + 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/cluster/gvk" odhtypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/manifest" - "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/pkg/resources" ) +//go:embed resources +var resourcesFS embed.FS + func kserveManifestInfo(sourcePath string) odhtypes.ManifestInfo { return odhtypes.ManifestInfo{ Path: deploy.DefaultManifestPath, @@ -40,115 +34,61 @@ func kserveManifestInfo(sourcePath string) odhtypes.ManifestInfo { } } -func configureServerlessFeatures(dsciSpec *dsciv1.DSCInitializationSpec, kserve *componentApi.Kserve) feature.FeaturesProvider { - return func(registry feature.FeaturesRegistry) error { - servingDeployment := feature.Define("serverless-serving-deployment"). - Manifests( - manifest.Location(Resources.Location). - Include( - path.Join(Resources.InstallDir), - ), - ). - WithData( - serverless.FeatureData.IngressDomain.Define(&kserve.Spec.Serving).AsAction(), - serverless.FeatureData.Serving.Define(&kserve.Spec.Serving).AsAction(), - servicemesh.FeatureData.ControlPlane.Define(dsciSpec).AsAction(), - ). - PreConditions( - serverless.EnsureServerlessOperatorInstalled, - serverless.EnsureServerlessAbsent, - servicemesh.EnsureServiceMeshInstalled, - feature.CreateNamespaceIfNotExists(serverless.KnativeServingNamespace), - ). - PostConditions( - feature.WaitForPodsToBeReady(serverless.KnativeServingNamespace), - ) - - istioSecretFiltering := feature.Define("serverless-net-istio-secret-filtering"). - Manifests( - manifest.Location(Resources.Location). - Include( - path.Join(Resources.BaseDir, "serving-net-istio-secret-filtering.patch.tmpl.yaml"), - ), - ). - WithData(serverless.FeatureData.Serving.Define(&kserve.Spec.Serving).AsAction()). - PreConditions(serverless.EnsureServerlessServingDeployed). - PostConditions( - feature.WaitForPodsToBeReady(serverless.KnativeServingNamespace), - ) - - servingGateway := feature.Define("serverless-serving-gateways"). - Manifests( - manifest.Location(Resources.Location). - Include( - path.Join(Resources.GatewaysDir), - ), - ). - WithData( - serverless.FeatureData.IngressDomain.Define(&kserve.Spec.Serving).AsAction(), - serverless.FeatureData.CertificateName.Define(&kserve.Spec.Serving).AsAction(), - serverless.FeatureData.Serving.Define(&kserve.Spec.Serving).AsAction(), - servicemesh.FeatureData.ControlPlane.Define(dsciSpec).AsAction(), - ). - WithResources(serverless.ServingCertificateResource). - PreConditions(serverless.EnsureServerlessServingDeployed) +func createServingCertResource(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec, kserve *componentApi.Kserve) error { + domain := getKnativeDomain(ctx, cli, kserve) + secretName := getKnativeCertSecretName(kserve) - return registry.Add( - servingDeployment, - istioSecretFiltering, - servingGateway, - ) + switch kserve.Spec.Serving.IngressGateway.Certificate.Type { + case infrav1.SelfSigned: + return cluster.CreateSelfSignedCertificate(ctx, cli, secretName, + domain, dscispec.ServiceMesh.ControlPlane.Namespace, + cluster.OwnedBy(kserve, cli.Scheme())) + case infrav1.Provided: + return nil + default: + return cluster.PropagateDefaultIngressCertificate(ctx, cli, + secretName, dscispec.ServiceMesh.ControlPlane.Namespace) } } -func 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) - } +func getTemplateData(ctx context.Context, rr *odhtypes.ReconciliationRequest) (map[string]any, error) { + k, ok := rr.Instance.(*componentApi.Kserve) + if !ok { + return nil, fmt.Errorf("resource instance %v is not a componentApi.Kserve)", rr.Instance) + } - if authorinoInstalled { - kserveExtAuthzErr := registry.Add(feature.Define("kserve-external-authz"). - Manifests( - manifest.Location(Resources.Location). - Include( - path.Join(Resources.ServiceMeshDir, "activator-envoyfilter.tmpl.yaml"), - path.Join(Resources.ServiceMeshDir, "envoy-oauth-temp-fix.tmpl.yaml"), - path.Join(Resources.ServiceMeshDir, "kserve-predictor-authorizationpolicy.tmpl.yaml"), - path.Join(Resources.ServiceMeshDir, "kserve-inferencegraph-envoyfilter.tmpl.yaml"), - path.Join(Resources.ServiceMeshDir, "kserve-inferencegraph-authorizationpolicy.tmpl.yaml"), - ), - ). - Managed(). - WithData( - feature.Entry("Domain", cluster.GetDomain), - servicemesh.FeatureData.ControlPlane.Define(dscispec).AsAction(), - ). - WithData( - servicemesh.FeatureData.Authorization.All(dscispec)..., - ), - ) + knativeIngressDomain := getKnativeDomain(ctx, rr.Client, k) + knativeCertificateSecret := getKnativeCertSecretName(k) - if kserveExtAuthzErr != nil { - return kserveExtAuthzErr - } - } else { - ctrl.Log.Info("WARN: Authorino operator is not installed on the cluster, skipping authorization capability") - } + return map[string]any{ + "AuthExtensionName": rr.DSCI.Spec.ApplicationsNamespace + "-auth-provider", + "ControlPlane": rr.DSCI.Spec.ServiceMesh.ControlPlane, + "KnativeCertificateSecret": knativeCertificateSecret, + "KnativeIngressDomain": knativeIngressDomain, + "Serving": k.Spec.Serving, + }, nil +} - return nil +func getKnativeDomain(ctx context.Context, cli client.Client, k *componentApi.Kserve) string { + domain := k.Spec.Serving.IngressGateway.Domain + if domain != "" { + return domain } -} -func removeServiceMeshConfigurations(ctx context.Context, cli client.Client, owner metav1.Object, dscispec *dsciv1.DSCInitializationSpec) error { - serviceMeshInitializer := feature.ComponentFeaturesHandler(owner, componentName, dscispec.ApplicationsNamespace, defineServiceMeshFeatures(ctx, cli, dscispec)) - return serviceMeshInitializer.Delete(ctx, cli) + domain, err := cluster.GetDomain(ctx, cli) + if err != nil { + return "" + } + return domain } -func removeServerlessFeatures(ctx context.Context, cli client.Client, k *componentApi.Kserve, dscispec *dsciv1.DSCInitializationSpec) error { - serverlessFeatures := feature.ComponentFeaturesHandler(k, componentName, dscispec.ApplicationsNamespace, configureServerlessFeatures(dscispec, k)) - return serverlessFeatures.Delete(ctx, cli) +func getKnativeCertSecretName(k *componentApi.Kserve) string { + name := k.Spec.Serving.IngressGateway.Certificate.SecretName + if name == "" { + name = "knative-serving-cert" + } + + return name } func getDefaultDeploymentMode(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) (string, error) { @@ -243,35 +183,6 @@ func hashConfigMap(cm *corev1.ConfigMap) (string, error) { return base64.RawURLEncoding.EncodeToString(h), nil } -func ownedViaFT(cli client.Client) handler.MapFunc { - return func(ctx context.Context, a client.Object) []reconcile.Request { - for _, or := range a.GetOwnerReferences() { - if or.Kind == "FeatureTracker" { - ft := featuresv1.FeatureTracker{} - if err := cli.Get(ctx, client.ObjectKey{Name: or.Name}, &ft); err != nil { - return []reconcile.Request{} - } - - for _, ftor := range ft.GetOwnerReferences() { - if ftor.Kind == componentApi.KserveKind && ftor.Name != "" { - return []reconcile.Request{{ - NamespacedName: types.NamespacedName{ - Name: ftor.Name, - }, - }} - } - } - } - } - - return []reconcile.Request{} - } -} - -func isLegacyOwnerRef(or metav1.OwnerReference) bool { - return or.APIVersion == gvk.DataScienceCluster.GroupVersion().String() && or.Kind == gvk.DataScienceCluster.Kind -} - func ifGVKInstalled(kvg schema.GroupVersionKind) func(context.Context, *odhtypes.ReconciliationRequest) bool { return func(ctx context.Context, rr *odhtypes.ReconciliationRequest) bool { hasCRD, err := cluster.HasCRD(ctx, rr.Client, kvg) diff --git a/pkg/cluster/gvk/gvk.go b/pkg/cluster/gvk/gvk.go index 6430d92c515..6f6fe49016a 100644 --- a/pkg/cluster/gvk/gvk.go +++ b/pkg/cluster/gvk/gvk.go @@ -233,6 +233,12 @@ var ( Kind: "AuthorizationPolicy", } + AuthorizationPolicyv1beta1 = schema.GroupVersionKind{ + Group: "security.istio.io", + Version: "v1beta1", + Kind: "AuthorizationPolicy", + } + Gateway = schema.GroupVersionKind{ Group: "networking.istio.io", Version: "v1beta1", diff --git a/tests/e2e/kserve_test.go b/tests/e2e/kserve_test.go index 08d2177f557..96332cb4600 100644 --- a/tests/e2e/kserve_test.go +++ b/tests/e2e/kserve_test.go @@ -42,12 +42,10 @@ func kserveTestSuite(t *testing.T) { t.Run("Validate component enabled", componentCtx.ValidateComponentEnabled) t.Run("Validate component spec", componentCtx.validateSpec) - t.Run("Validate FeatureTrackers", componentCtx.validateFeatureTrackers) t.Run("Validate model controller", componentCtx.validateModelControllerInstance) t.Run("Validate operands have OwnerReferences", componentCtx.ValidateOperandsOwnerReferences) - t.Run("Validate Kserve owns FeatureTrackers", componentCtx.ValidateKserveOwnsFeatureTrackers) - t.Run("Validate FeatureTrackers own children", componentCtx.ValidateFeatureTrackersOwnChildren) - t.Run("Validate KnativeServing Structure", componentCtx.ValidateKnativeServingStructure) + t.Run("Validate no FeatureTracker OwnerReferences", componentCtx.ValidateNoFeatureTrackerOwnerReferences) + t.Run("Validate no Kserve FeatureTrackers", componentCtx.ValidateNoKserveFeatureTrackers) t.Run("Validate default certs", componentCtx.validateDefaultCertsAvailable) t.Run("Validate update operand resources", componentCtx.ValidateUpdateDeploymentsResources) t.Run("Validate component disabled", componentCtx.ValidateComponentDisabled) @@ -125,49 +123,6 @@ func (c *KserveTestCtx) validateSpec(t *testing.T) { )) } -func (c *KserveTestCtx) validateFeatureTrackers(t *testing.T) { - g := c.NewWithT(t) - ftName := types.NamespacedName{Name: c.ApplicationNamespace + "-serverless-serving-deployment"} - - g.Get(gvk.FeatureTracker, ftName).Eventually().Should(And( - jq.Match(`(.metadata.ownerReferences | length) == 1`), - jq.Match(`.metadata.ownerReferences[0].apiVersion == "%s"`, gvk.Kserve.GroupVersion().String()), - jq.Match(`.metadata.ownerReferences[0].kind == "%s"`, gvk.Kserve.Kind), - jq.Match(`.metadata.ownerReferences[0].blockOwnerDeletion == true`), - jq.Match(`.metadata.ownerReferences[0].controller == true`), - )) - - dsc, err := c.GetDSC() - g.Expect(err).NotTo(HaveOccurred()) - - g.Update( - gvk.FeatureTracker, - ftName, - func(obj *unstructured.Unstructured) error { - if err := controllerutil.SetOwnerReference(dsc, obj, c.Client().Scheme()); err != nil { - return err - } - - // trigger reconciliation as spec changes - if err = unstructured.SetNestedField(obj.Object, xid.New().String(), "spec", "source", "name"); err != nil { - return err - } - - return nil - }, - ).Eventually().Should(And( - jq.Match(`(.metadata.ownerReferences | length) == 2`), - )) - - g.Get(gvk.FeatureTracker, ftName).Eventually().Should(And( - jq.Match(`(.metadata.ownerReferences | length) == 1`), - jq.Match(`.metadata.ownerReferences[0].apiVersion == "%s"`, gvk.Kserve.GroupVersion().String()), - jq.Match(`.metadata.ownerReferences[0].kind == "%s"`, gvk.Kserve.Kind), - jq.Match(`.metadata.ownerReferences[0].blockOwnerDeletion == true`), - jq.Match(`.metadata.ownerReferences[0].controller == true`), - )) -} - func (c *KserveTestCtx) validateModelControllerInstance(t *testing.T) { g := c.NewWithT(t) @@ -211,30 +166,10 @@ func (c *KserveTestCtx) validateDefaultCertsAvailable(t *testing.T) { g.Expect(defaultIngressSecret.Data).Should(Equal(ctrlPlaneSecret.Data)) } -func (c *KserveTestCtx) ValidateKserveOwnsFeatureTrackers(t *testing.T) { +func (c *KserveTestCtx) ValidateNoFeatureTrackerOwnerReferences(t *testing.T) { g := c.NewWithT(t) - fts := []string{ - c.ApplicationNamespace + "-kserve-external-authz", - c.ApplicationNamespace + "-serverless-serving-gateways", - c.ApplicationNamespace + "-serverless-serving-deployment", - c.ApplicationNamespace + "-serverless-net-istio-secret-filtering", - } - - for _, ft := range fts { - g.Get( - gvk.FeatureTracker, types.NamespacedName{Name: ft}, - ).Eventually().Should( - jq.Match(`.metadata.ownerReferences | any(.kind == "%s")`, gvk.Kserve.Kind), - `Ensuring Kserve ownership of FeatureTracker %s`, ft, - ) - } -} - -func (c *KserveTestCtx) ValidateFeatureTrackersOwnChildren(t *testing.T) { - g := c.NewWithT(t) - - children := []struct { + operands := []struct { gvk schema.GroupVersionKind nn types.NamespacedName }{ @@ -251,19 +186,29 @@ func (c *KserveTestCtx) ValidateFeatureTrackersOwnChildren(t *testing.T) { {gvk.Gateway, types.NamespacedName{Namespace: "knative-serving", Name: "knative-local-gateway"}}, } - for _, child := range children { - g.Get(child.gvk, child.nn).Eventually().Should( - jq.Match(`.metadata.ownerReferences | any(.kind == "%s")`, gvk.FeatureTracker.Kind), - `Checking if %s/%s in %s has expected owner refs`, child.gvk, child.nn.Name, child.nn.Namespace, - ) + for _, op := range operands { + g.Get(op.gvk, op.nn).Eventually().Should(And( + jq.Match(`.metadata.ownerReferences | any(.kind == "%s")`, gvk.Kserve.Kind), + jq.Match(`.metadata.ownerReferences | all(.kind != "%s")`, gvk.FeatureTracker.Kind), + ), + `Checking if %s/%s in %s has expected owner refs`, op.gvk, op.nn.Name, op.nn.Namespace) } } -func (c *KserveTestCtx) ValidateKnativeServingStructure(t *testing.T) { +func (c *KserveTestCtx) ValidateNoKserveFeatureTrackers(t *testing.T) { g := c.NewWithT(t) - g.Get(gvk.KnativeServing, types.NamespacedName{Namespace: "knative-serving", Name: "knative-serving"}).Eventually().Should(And( - jq.Match(`.spec.workloads | length == 3`), - jq.Match(`.metadata.annotations."serverless.openshift.io/default-enable-http2" == "true"`), - ), `Ensuring KnativeServing has content from both templates`) + g.List( + gvk.FeatureTracker, + ).Eventually().Should( + HaveEach(And( + jq.Match(`.metadata.name != "%s"`, c.ApplicationNamespace+"-kserve-external-authz"), + jq.Match(`.metadata.name != "%s"`, c.ApplicationNamespace+"-serverless-serving-gateways"), + jq.Match(`.metadata.name != "%s"`, c.ApplicationNamespace+"-serverless-serving-deployment"), + jq.Match(`.metadata.name != "%s"`, c.ApplicationNamespace+"-serverless-net-istio-secret-filtering"), + + // there should be no FeatureTrackers owned by a Kserve + jq.Match(`.metadata.ownerReferences | all(.kind != "%s")`, gvk.Kserve.Kind), + )), + `Ensuring there are no Kserve FeatureTrackers`) } From d35d61952e304af303569d379cbcc7e1050aafd7 Mon Sep 17 00:00:00 2001 From: Gerard Ryan Date: Wed, 19 Feb 2025 01:39:01 +0000 Subject: [PATCH 3/3] Deduplicate KnativeServing template in Kserve --- .../kserve/kserve_controller_actions.go | 5 ----- .../serving-install/knative-serving.tmpl.yaml | 6 ++++++ ...net-istio-secret-filtering.patch.tmpl.yaml | 21 ------------------- 3 files changed, 6 insertions(+), 26 deletions(-) delete mode 100644 controllers/components/kserve/resources/serving-net-istio-secret-filtering.patch.tmpl.yaml diff --git a/controllers/components/kserve/kserve_controller_actions.go b/controllers/components/kserve/kserve_controller_actions.go index 3764e6e572f..9f3fda4b234 100644 --- a/controllers/components/kserve/kserve_controller_actions.go +++ b/controllers/components/kserve/kserve_controller_actions.go @@ -237,11 +237,6 @@ func configureServerless(ctx context.Context, rr *odhtypes.ReconciliationRequest FS: resourcesFS, Path: "resources/serving-install/knative-serving.tmpl.yaml", }, - { - FS: resourcesFS, - Path: "resources/serving-net-istio-secret-filtering.patch.tmpl.yaml", - }, - { FS: resourcesFS, Path: "resources/servicemesh/routing/istio-ingress-gateway.tmpl.yaml", diff --git a/controllers/components/kserve/resources/serving-install/knative-serving.tmpl.yaml b/controllers/components/kserve/resources/serving-install/knative-serving.tmpl.yaml index 98abfe3f3e2..166b51c2588 100644 --- a/controllers/components/kserve/resources/serving-install/knative-serving.tmpl.yaml +++ b/controllers/components/kserve/resources/serving-install/knative-serving.tmpl.yaml @@ -15,6 +15,12 @@ spec: sidecar.istio.io/inject: "true" sidecar.istio.io/rewriteAppHTTPProbers: "true" name: autoscaler + - name: net-istio-controller + env: + - container: controller + envVars: + - name: ENABLE_SECRET_INFORMER_FILTERING_BY_CERT_UID + value: 'true' ingress: istio: enabled: true diff --git a/controllers/components/kserve/resources/serving-net-istio-secret-filtering.patch.tmpl.yaml b/controllers/components/kserve/resources/serving-net-istio-secret-filtering.patch.tmpl.yaml deleted file mode 100644 index 5153cbc0fa9..00000000000 --- a/controllers/components/kserve/resources/serving-net-istio-secret-filtering.patch.tmpl.yaml +++ /dev/null @@ -1,21 +0,0 @@ -apiVersion: operator.knative.dev/v1beta1 -kind: KnativeServing -metadata: - name: {{ .Serving.Name }} - namespace: knative-serving -spec: - workloads: - - name: activator - annotations: - sidecar.istio.io/inject: "true" - sidecar.istio.io/rewriteAppHTTPProbers: "true" - - name: autoscaler - annotations: - sidecar.istio.io/inject: "true" - sidecar.istio.io/rewriteAppHTTPProbers: "true" - - name: net-istio-controller - env: - - container: controller - envVars: - - name: ENABLE_SECRET_INFORMER_FILTERING_BY_CERT_UID - value: 'true'