From 8fcfb7e9fdbd5b4561ff306ac1c3ee65baab5081 Mon Sep 17 00:00:00 2001 From: Dhiraj Bokde Date: Mon, 4 Nov 2024 22:22:56 -0800 Subject: [PATCH 1/4] fix: cleanup runtime default properties on first reconcile, fixes RHOAIENG-15033 Signed-off-by: Dhiraj Bokde --- api/v1alpha1/modelregistry_webhook.go | 91 ++++++++++++++++++++++++++- 1 file changed, 90 insertions(+), 1 deletion(-) diff --git a/api/v1alpha1/modelregistry_webhook.go b/api/v1alpha1/modelregistry_webhook.go index eb1a637..10f729c 100644 --- a/api/v1alpha1/modelregistry_webhook.go +++ b/api/v1alpha1/modelregistry_webhook.go @@ -23,11 +23,14 @@ import ( "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" + "maps" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "slices" + "strings" ) // log is for logging in this package. @@ -41,6 +44,9 @@ const ( DefaultTlsMode = IstioMutualTlsMode IstioMutualTlsMode = "ISTIO_MUTUAL" DefaultIstioGateway = "ingressgateway" + + tagSeparator = ":" + emptyValue = "" ) func (r *ModelRegistry) SetupWebhookWithManager(mgr ctrl.Manager) error { @@ -63,7 +69,7 @@ var ( // Default implements webhook.Defaulter so a webhook will be registered for the type func (r *ModelRegistry) Default() { - modelregistrylog.Info("default", "name", r.Name) + modelregistrylog.Info("default", "name", r.Name, "status.specDefaults", r.Status.SpecDefaults) // handle annotation mutations r.HandleAnnotations() @@ -117,6 +123,89 @@ func (r *ModelRegistry) Default() { } } } + + // handle runtime default properties for https://issues.redhat.com/browse/RHOAIENG-15033 + r.cleanupRuntimeDefaults() +} + +// cleanupRuntimeDefaults removes runtime defaults on first reconcile, when specDefaults is empty, +// also including model registries reconciled by older operator versions before adding specDefaults support. +// It removes images if they are the same as the operator defaults (ignoring version tag), +// and it removes default runtime values that match default runtime properties set in the operator +// since they are redundant as custom property values. +func (r *ModelRegistry) cleanupRuntimeDefaults() { + if len(r.Status.SpecDefaults) != 0 { + // nothing to do as model registries with specDefaults set + // will only have custom properties set by users + return + } + + // check grpc image against operator default grpc image repo + if len(r.Spec.Grpc.Image) != 0 { + defaultGrpcImage := config.GetStringConfigWithDefault(config.GrpcImage, config.DefaultGrpcImage) + defaultGrpcImageRepo := strings.Split(defaultGrpcImage, tagSeparator)[0] + + grpcImageRepo := strings.Split(r.Spec.Grpc.Image, tagSeparator)[0] + if grpcImageRepo == defaultGrpcImageRepo { + modelregistrylog.V(4).Info("reset image", "grpc repo", grpcImageRepo) + // remove image altogether as the MR repo matches operator repo, + // so that future operator version upgrades don't have to handle a hardcoded default + r.Spec.Grpc.Image = emptyValue + + // also reset resource requirements + r.Spec.Grpc.Resources = nil + } + } + + // check rest image against operator default rest image repo + if len(r.Spec.Rest.Image) != 0 { + defaultRestImage := config.GetStringConfigWithDefault(config.RestImage, config.DefaultRestImage) + defaultRestImageRepo := strings.Split(defaultRestImage, tagSeparator)[0] + + restImageRepo := strings.Split(r.Spec.Rest.Image, tagSeparator)[0] + if restImageRepo == defaultRestImageRepo { + modelregistrylog.V(4).Info("reset image", "rest repo", restImageRepo) + // remove image altogether as the MR repo matches operator repo, + // so that future operator version upgrades don't have to handle a hardcoded default + r.Spec.Rest.Image = emptyValue + + // also reset resource requirements + r.Spec.Rest.Resources = nil + } + } + + // reset istio defaults + if r.Spec.Istio != nil { + // reset default audiences + if len(r.Spec.Istio.Audiences) != 0 && slices.Equal(r.Spec.Istio.Audiences, config.GetDefaultAudiences()) { + r.Spec.Istio.Audiences = make([]string, 0) + } + // reset default authprovider + if r.Spec.Istio.AuthProvider == config.GetDefaultAuthProvider() { + r.Spec.Istio.AuthProvider = emptyValue + } + // reset default authconfig labels + if len(r.Spec.Istio.AuthConfigLabels) != 0 && maps.Equal(r.Spec.Istio.AuthConfigLabels, config.GetDefaultAuthConfigLabels()) { + r.Spec.Istio.AuthConfigLabels = make(map[string]string) + } + + if r.Spec.Istio.Gateway != nil { + // reset default domain + if r.Spec.Istio.Gateway.Domain == config.GetDefaultDomain() { + r.Spec.Istio.Gateway.Domain = emptyValue + } + + // reset default cert + if r.Spec.Istio.Gateway.Rest.TLS != nil && r.Spec.Istio.Gateway.Rest.TLS.Mode != DefaultTlsMode && + (r.Spec.Istio.Gateway.Rest.TLS.CredentialName != nil && *r.Spec.Istio.Gateway.Rest.TLS.CredentialName == config.GetDefaultCert()) { + r.Spec.Istio.Gateway.Rest.TLS.CredentialName = nil + } + if r.Spec.Istio.Gateway.Grpc.TLS != nil && r.Spec.Istio.Gateway.Grpc.TLS.Mode != DefaultTlsMode && + (r.Spec.Istio.Gateway.Grpc.TLS.CredentialName != nil && *r.Spec.Istio.Gateway.Grpc.TLS.CredentialName == config.GetDefaultCert()) { + r.Spec.Istio.Gateway.Grpc.TLS.CredentialName = nil + } + } + } } // RuntimeDefaults sets default values from the operator environment, which could change at runtime From 223a739096c5e505edb266bcaf78d7abfd7f9420 Mon Sep 17 00:00:00 2001 From: Dhiraj Bokde Date: Thu, 7 Nov 2024 16:19:11 -0800 Subject: [PATCH 2/4] fix: cleanup default properties when specDefaults=={} Signed-off-by: Dhiraj Bokde --- api/v1alpha1/modelregistry_webhook.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/v1alpha1/modelregistry_webhook.go b/api/v1alpha1/modelregistry_webhook.go index 10f729c..853513b 100644 --- a/api/v1alpha1/modelregistry_webhook.go +++ b/api/v1alpha1/modelregistry_webhook.go @@ -134,7 +134,7 @@ func (r *ModelRegistry) Default() { // and it removes default runtime values that match default runtime properties set in the operator // since they are redundant as custom property values. func (r *ModelRegistry) cleanupRuntimeDefaults() { - if len(r.Status.SpecDefaults) != 0 { + if r.Status.SpecDefaults != "" && r.Status.SpecDefaults != "{}" { // nothing to do as model registries with specDefaults set // will only have custom properties set by users return From 00d0683eee4a10492c9b453ab990fd5d14f9f32f Mon Sep 17 00:00:00 2001 From: Dhiraj Bokde Date: Thu, 7 Nov 2024 17:02:09 -0800 Subject: [PATCH 3/4] fix: handle stale specDefaults Signed-off-by: Dhiraj Bokde --- api/v1alpha1/modelregistry_webhook.go | 30 +++++++++++++++---- .../modelregistry_controller_status.go | 2 +- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/api/v1alpha1/modelregistry_webhook.go b/api/v1alpha1/modelregistry_webhook.go index 853513b..367c890 100644 --- a/api/v1alpha1/modelregistry_webhook.go +++ b/api/v1alpha1/modelregistry_webhook.go @@ -134,10 +134,15 @@ func (r *ModelRegistry) Default() { // and it removes default runtime values that match default runtime properties set in the operator // since they are redundant as custom property values. func (r *ModelRegistry) cleanupRuntimeDefaults() { + // if specDefaults hasn't been set for new MRs or all properties were set in a previous version if r.Status.SpecDefaults != "" && r.Status.SpecDefaults != "{}" { - // nothing to do as model registries with specDefaults set - // will only have custom properties set by users - return + // also check if any runtime default properties will be set in the next reconcile + // this is required because specDefaults could be stale + deepCopy := r.DeepCopy() // do not modify this registry + if deepCopy.RuntimeDefaults() { + // model registry has custom values set for runtime properties + return + } } // check grpc image against operator default grpc image repo @@ -208,22 +213,29 @@ func (r *ModelRegistry) cleanupRuntimeDefaults() { } } -// RuntimeDefaults sets default values from the operator environment, which could change at runtime -func (r *ModelRegistry) RuntimeDefaults() { +// RuntimeDefaults sets default values from the operator environment, which could change at runtime. +// Returns true if any properties were set to a default value, false if all default values are overridden. +func (r *ModelRegistry) RuntimeDefaults() bool { modelregistrylog.Info("runtime defaults", "name", r.Name) + modified := false + if r.Spec.Grpc.Resources == nil { r.Spec.Grpc.Resources = config.MlmdGRPCResourceRequirements.DeepCopy() + modified = true } if len(r.Spec.Grpc.Image) == 0 { r.Spec.Grpc.Image = config.GetStringConfigWithDefault(config.GrpcImage, config.DefaultGrpcImage) + modified = true } if r.Spec.Rest.Resources == nil { r.Spec.Rest.Resources = config.MlmdRestResourceRequirements.DeepCopy() + modified = true } if len(r.Spec.Rest.Image) == 0 { r.Spec.Rest.Image = config.GetStringConfigWithDefault(config.RestImage, config.DefaultRestImage) + modified = true } // istio defaults @@ -231,20 +243,24 @@ func (r *ModelRegistry) RuntimeDefaults() { // set default audiences if len(r.Spec.Istio.Audiences) == 0 { r.Spec.Istio.Audiences = config.GetDefaultAudiences() + modified = true } // set default authprovider if len(r.Spec.Istio.AuthProvider) == 0 { r.Spec.Istio.AuthProvider = config.GetDefaultAuthProvider() + modified = true } // set default authconfig labels if len(r.Spec.Istio.AuthConfigLabels) == 0 { r.Spec.Istio.AuthConfigLabels = config.GetDefaultAuthConfigLabels() + modified = true } if r.Spec.Istio.Gateway != nil { // set default domain if len(r.Spec.Istio.Gateway.Domain) == 0 { r.Spec.Istio.Gateway.Domain = config.GetDefaultDomain() + modified = true } // set default cert @@ -252,14 +268,18 @@ func (r *ModelRegistry) RuntimeDefaults() { (r.Spec.Istio.Gateway.Rest.TLS.CredentialName == nil || len(*r.Spec.Istio.Gateway.Rest.TLS.CredentialName) == 0) { cert := config.GetDefaultCert() r.Spec.Istio.Gateway.Rest.TLS.CredentialName = &cert + modified = true } if r.Spec.Istio.Gateway.Grpc.TLS != nil && r.Spec.Istio.Gateway.Grpc.TLS.Mode != DefaultTlsMode && (r.Spec.Istio.Gateway.Grpc.TLS.CredentialName == nil || len(*r.Spec.Istio.Gateway.Grpc.TLS.CredentialName) == 0) { cert := config.GetDefaultCert() r.Spec.Istio.Gateway.Grpc.TLS.CredentialName = &cert + modified = true } } } + + return modified } // ValidateRegistry validates registry spec diff --git a/internal/controller/modelregistry_controller_status.go b/internal/controller/modelregistry_controller_status.go index e0c0aef..1ccfbe0 100644 --- a/internal/controller/modelregistry_controller_status.go +++ b/internal/controller/modelregistry_controller_status.go @@ -20,7 +20,7 @@ import ( "bufio" "context" "fmt" - jsonpatch "github.com/evanphx/json-patch/v5" + "github.com/evanphx/json-patch/v5" "github.com/go-logr/logr" authorino "github.com/kuadrant/authorino/api/v1beta2" modelregistryv1alpha1 "github.com/opendatahub-io/model-registry-operator/api/v1alpha1" From 74e4fa463f49ebea75bd92ab4946aa0006c0b086 Mon Sep 17 00:00:00 2001 From: Dhiraj Bokde Date: Thu, 7 Nov 2024 17:31:38 -0800 Subject: [PATCH 4/4] fix: cleanup runtime properties in reconcile for older registries because mutatingwebhook is only called on create or update Signed-off-by: Dhiraj Bokde --- api/v1alpha1/modelregistry_webhook.go | 34 ++++--------------- .../modelregistry_controller_status.go | 10 ++++++ 2 files changed, 17 insertions(+), 27 deletions(-) diff --git a/api/v1alpha1/modelregistry_webhook.go b/api/v1alpha1/modelregistry_webhook.go index 367c890..89cadb3 100644 --- a/api/v1alpha1/modelregistry_webhook.go +++ b/api/v1alpha1/modelregistry_webhook.go @@ -125,24 +125,19 @@ func (r *ModelRegistry) Default() { } // handle runtime default properties for https://issues.redhat.com/browse/RHOAIENG-15033 - r.cleanupRuntimeDefaults() + r.CleanupRuntimeDefaults() } -// cleanupRuntimeDefaults removes runtime defaults on first reconcile, when specDefaults is empty, -// also including model registries reconciled by older operator versions before adding specDefaults support. +// CleanupRuntimeDefaults removes runtime defaults. Usually on first reconcile, when specDefaults is empty, +// or for model registries reconciled by older operator versions before adding specDefaults support. // It removes images if they are the same as the operator defaults (ignoring version tag), // and it removes default runtime values that match default runtime properties set in the operator // since they are redundant as custom property values. -func (r *ModelRegistry) cleanupRuntimeDefaults() { +func (r *ModelRegistry) CleanupRuntimeDefaults() { // if specDefaults hasn't been set for new MRs or all properties were set in a previous version if r.Status.SpecDefaults != "" && r.Status.SpecDefaults != "{}" { - // also check if any runtime default properties will be set in the next reconcile - // this is required because specDefaults could be stale - deepCopy := r.DeepCopy() // do not modify this registry - if deepCopy.RuntimeDefaults() { - // model registry has custom values set for runtime properties - return - } + // model registry has custom values set for runtime properties + return } // check grpc image against operator default grpc image repo @@ -214,28 +209,21 @@ func (r *ModelRegistry) cleanupRuntimeDefaults() { } // RuntimeDefaults sets default values from the operator environment, which could change at runtime. -// Returns true if any properties were set to a default value, false if all default values are overridden. -func (r *ModelRegistry) RuntimeDefaults() bool { +func (r *ModelRegistry) RuntimeDefaults() { modelregistrylog.Info("runtime defaults", "name", r.Name) - modified := false - if r.Spec.Grpc.Resources == nil { r.Spec.Grpc.Resources = config.MlmdGRPCResourceRequirements.DeepCopy() - modified = true } if len(r.Spec.Grpc.Image) == 0 { r.Spec.Grpc.Image = config.GetStringConfigWithDefault(config.GrpcImage, config.DefaultGrpcImage) - modified = true } if r.Spec.Rest.Resources == nil { r.Spec.Rest.Resources = config.MlmdRestResourceRequirements.DeepCopy() - modified = true } if len(r.Spec.Rest.Image) == 0 { r.Spec.Rest.Image = config.GetStringConfigWithDefault(config.RestImage, config.DefaultRestImage) - modified = true } // istio defaults @@ -243,24 +231,20 @@ func (r *ModelRegistry) RuntimeDefaults() bool { // set default audiences if len(r.Spec.Istio.Audiences) == 0 { r.Spec.Istio.Audiences = config.GetDefaultAudiences() - modified = true } // set default authprovider if len(r.Spec.Istio.AuthProvider) == 0 { r.Spec.Istio.AuthProvider = config.GetDefaultAuthProvider() - modified = true } // set default authconfig labels if len(r.Spec.Istio.AuthConfigLabels) == 0 { r.Spec.Istio.AuthConfigLabels = config.GetDefaultAuthConfigLabels() - modified = true } if r.Spec.Istio.Gateway != nil { // set default domain if len(r.Spec.Istio.Gateway.Domain) == 0 { r.Spec.Istio.Gateway.Domain = config.GetDefaultDomain() - modified = true } // set default cert @@ -268,18 +252,14 @@ func (r *ModelRegistry) RuntimeDefaults() bool { (r.Spec.Istio.Gateway.Rest.TLS.CredentialName == nil || len(*r.Spec.Istio.Gateway.Rest.TLS.CredentialName) == 0) { cert := config.GetDefaultCert() r.Spec.Istio.Gateway.Rest.TLS.CredentialName = &cert - modified = true } if r.Spec.Istio.Gateway.Grpc.TLS != nil && r.Spec.Istio.Gateway.Grpc.TLS.Mode != DefaultTlsMode && (r.Spec.Istio.Gateway.Grpc.TLS.CredentialName == nil || len(*r.Spec.Istio.Gateway.Grpc.TLS.CredentialName) == 0) { cert := config.GetDefaultCert() r.Spec.Istio.Gateway.Grpc.TLS.CredentialName = &cert - modified = true } } } - - return modified } // ValidateRegistry validates registry spec diff --git a/internal/controller/modelregistry_controller_status.go b/internal/controller/modelregistry_controller_status.go index 1ccfbe0..b6f434c 100644 --- a/internal/controller/modelregistry_controller_status.go +++ b/internal/controller/modelregistry_controller_status.go @@ -89,6 +89,16 @@ func (r *ModelRegistryReconciler) setRegistryStatus(ctx context.Context, req ctr // log error but continue updating rest of the status since it's not a blocker log.Error(err, "Failed to set registry status defaults") } + // if specDefaults is {}, cleanup runtime properties + if modelRegistry.Status.SpecDefaults == "{}" { + // this is an exception to the rule to not modify a resource in reconcile, + // because mutatingwebhook is not triggered on status update since it's a subresource + modelRegistry.CleanupRuntimeDefaults() + if err := r.Client.Update(ctx, modelRegistry); err != nil { + log.Error(err, "Failed to update modelRegistry runtime defaults") + return false, err + } + } status := metav1.ConditionTrue reason := ReasonDeploymentCreated