From 4c411a63bd67d0aec5a66a736a70db0b474230c3 Mon Sep 17 00:00:00 2001 From: Dhiraj Bokde Date: Mon, 26 Aug 2024 06:11:41 -0700 Subject: [PATCH] feat: add default cert for model registry, fixes RHOAIENG-9909 (#1165) * feat: add default cert for model registry, fixes RHOAIENG-9909 * fix: fixed lint errors * fix: add servicemesh feature check for MR, add MR enable check in e2e default cert test * fix: changed MR servicemesh status check to look for Managed state * fix: ignore missing model-registry default cert if already removed --- components/modelregistry/modelregistry.go | 47 ++++++++++++++++++++++- tests/e2e/dsc_creation_test.go | 46 ++++++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) diff --git a/components/modelregistry/modelregistry.go b/components/modelregistry/modelregistry.go index 6a4588c32b8..b1ae6d58b71 100644 --- a/components/modelregistry/modelregistry.go +++ b/components/modelregistry/modelregistry.go @@ -4,11 +4,13 @@ package modelregistry import ( "context" + "errors" "fmt" "path/filepath" "github.com/go-logr/logr" operatorv1 "github.com/openshift/api/operator/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -18,6 +20,8 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" ) +const DefaultModelRegistryCert = "default-modelregistry-cert" + var ( ComponentName = "model-registry-operator" Path = deploy.DefaultManifestPath + "/" + ComponentName + "/overlays/odh" @@ -70,6 +74,15 @@ func (m *ModelRegistry) ReconcileComponent(ctx context.Context, cli client.Clien monitoringEnabled := dscispec.Monitoring.ManagementState == operatorv1.Managed if enabled { + // return error if ServiceMesh is not enabled, as it's a required feature + if dscispec.ServiceMesh == nil || dscispec.ServiceMesh.ManagementState != operatorv1.Managed { + return errors.New("ServiceMesh needs to be set to 'Managed' in DSCI CR, it is required by Model Registry") + } + + if err := m.createDependencies(ctx, cli, dscispec); err != nil { + return err + } + if m.DevFlags != nil { // Download manifests and update paths if err := m.OverrideManifests(ctx, platform); err != nil { @@ -79,7 +92,10 @@ func (m *ModelRegistry) ReconcileComponent(ctx context.Context, cli client.Clien // Update image parameters only when we do not have customized manifests set if (dscispec.DevFlags == nil || dscispec.DevFlags.ManifestsUri == "") && (m.DevFlags == nil || len(m.DevFlags.Manifests) == 0) { - if err := deploy.ApplyParams(Path, imageParamMap); err != nil { + extraParamsMap := map[string]string{ + "DEFAULT_CERT": DefaultModelRegistryCert, + } + if err := deploy.ApplyParams(Path, imageParamMap, extraParamsMap); err != nil { return fmt.Errorf("failed to update image from %s : %w", Path, err) } } @@ -90,7 +106,13 @@ func (m *ModelRegistry) ReconcileComponent(ctx context.Context, cli client.Clien if err != nil { return err } + } else { + err := m.removeDependencies(ctx, cli, dscispec) + if err != nil { + return err + } } + // Deploy ModelRegistry Operator if err := deploy.DeployManifestsFromPath(ctx, cli, owner, Path, dscispec.ApplicationsNamespace, m.GetComponentName(), enabled); err != nil { return err @@ -124,3 +146,26 @@ func (m *ModelRegistry) ReconcileComponent(ctx context.Context, cli client.Clien } return nil } + +func (m *ModelRegistry) createDependencies(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) error { + // create DefaultModelRegistryCert + if err := cluster.PropagateDefaultIngressCertificate(ctx, cli, DefaultModelRegistryCert, dscispec.ServiceMesh.ControlPlane.Namespace); err != nil { + return err + } + return nil +} + +func (m *ModelRegistry) removeDependencies(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) error { + // delete DefaultModelRegistryCert + certSecret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultModelRegistryCert, + Namespace: dscispec.ServiceMesh.ControlPlane.Namespace, + }, + } + // ignore error if the secret has already been removed + if err := cli.Delete(ctx, &certSecret); client.IgnoreNotFound(err) != nil { + return err + } + return nil +} diff --git a/tests/e2e/dsc_creation_test.go b/tests/e2e/dsc_creation_test.go index b2fbe7966dc..dc17a503498 100644 --- a/tests/e2e/dsc_creation_test.go +++ b/tests/e2e/dsc_creation_test.go @@ -26,6 +26,7 @@ import ( 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/components" + "github.com/opendatahub-io/opendatahub-operator/v2/components/modelregistry" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/serverless" ) @@ -76,6 +77,10 @@ func creationTestSuite(t *testing.T) { err = testCtx.testDefaultCertsAvailable() require.NoError(t, err, "error getting default cert secrets for Kserve") }) + t.Run("Validate default model registry cert available", func(t *testing.T) { + err = testCtx.testDefaultModelRegistryCertAvailable() + require.NoError(t, err, "error getting default cert secret for ModelRegistry") + }) t.Run("Validate Controller reconcile", func(t *testing.T) { // only test Dashboard component for now err = testCtx.testUpdateComponentReconcile() @@ -424,6 +429,47 @@ func (tc *testContext) testDefaultCertsAvailable() error { return nil } +func (tc *testContext) testDefaultModelRegistryCertAvailable() error { + // return if MR is not set to Managed + if tc.testDsc.Spec.Components.ModelRegistry.ManagementState != operatorv1.Managed { + return nil + } + + // Get expected cert secrets + defaultIngressCtrl, err := cluster.FindAvailableIngressController(tc.ctx, tc.customClient) + if err != nil { + return fmt.Errorf("failed to get ingress controller: %w", err) + } + + defaultIngressCertName := cluster.GetDefaultIngressCertSecretName(defaultIngressCtrl) + + defaultIngressSecret, err := cluster.GetSecret(tc.ctx, tc.customClient, "openshift-ingress", defaultIngressCertName) + if err != nil { + return err + } + + // Verify secret from Control Plane namespace matches the default MR cert secret + defaultMRSecretName := modelregistry.DefaultModelRegistryCert + defaultMRSecret, err := cluster.GetSecret(tc.ctx, tc.customClient, tc.testDSCI.Spec.ServiceMesh.ControlPlane.Namespace, + defaultMRSecretName) + if err != nil { + return err + } + + if defaultMRSecret.Type != defaultIngressSecret.Type { + return fmt.Errorf("wrong type of MR cert secret is created for %v. Expected %v, Got %v", defaultMRSecretName, defaultIngressSecret.Type, defaultMRSecret.Type) + } + + if string(defaultIngressSecret.Data["tls.crt"]) != string(defaultMRSecret.Data["tls.crt"]) { + return fmt.Errorf("default MR cert secret not expected. Epected %v, Got %v", defaultIngressSecret.Data["tls.crt"], defaultMRSecret.Data["tls.crt"]) + } + + if string(defaultIngressSecret.Data["tls.key"]) != string(defaultMRSecret.Data["tls.key"]) { + return fmt.Errorf("default MR cert secret not expected. Epected %v, Got %v", defaultIngressSecret.Data["tls.crt"], defaultMRSecret.Data["tls.crt"]) + } + return nil +} + func (tc *testContext) testUpdateComponentReconcile() error { // Test Updating Dashboard Replicas