Skip to content

Commit

Permalink
feat: add default cert for model registry, fixes RHOAIENG-9909 (opend…
Browse files Browse the repository at this point in the history
…atahub-io#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
  • Loading branch information
dhirajsb authored Aug 26, 2024
1 parent 843e7e5 commit 4c411a6
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 1 deletion.
47 changes: 46 additions & 1 deletion components/modelregistry/modelregistry.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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"
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
}
}
Expand All @@ -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
Expand Down Expand Up @@ -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
}
46 changes: 46 additions & 0 deletions tests/e2e/dsc_creation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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

Expand Down

0 comments on commit 4c411a6

Please sign in to comment.