From 8cae64fd6e956473e53c178e271ac20ed2d92245 Mon Sep 17 00:00:00 2001 From: chaosi-zju Date: Wed, 13 Nov 2024 18:30:36 +0800 Subject: [PATCH] refactor operator module validation logic for Karmada objects Signed-off-by: chaosi-zju --- operator/pkg/controller/karmada/controller.go | 40 +--- operator/pkg/controller/karmada/validating.go | 122 +++++++++++ .../pkg/controller/karmada/validating_test.go | 194 ++++++++++++++++++ operator/pkg/init.go | 23 +-- operator/pkg/tasks/init/etcd.go | 4 - 5 files changed, 323 insertions(+), 60 deletions(-) create mode 100644 operator/pkg/controller/karmada/validating.go create mode 100644 operator/pkg/controller/karmada/validating_test.go diff --git a/operator/pkg/controller/karmada/controller.go b/operator/pkg/controller/karmada/controller.go index 498707821..4f057c452 100644 --- a/operator/pkg/controller/karmada/controller.go +++ b/operator/pkg/controller/karmada/controller.go @@ -18,15 +18,11 @@ package karmada import ( "context" - "fmt" "reflect" "strconv" "time" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/rest" "k8s.io/client-go/tools/record" @@ -41,7 +37,6 @@ import ( operatorv1alpha1 "github.com/karmada-io/karmada/operator/pkg/apis/operator/v1alpha1" operatorscheme "github.com/karmada-io/karmada/operator/pkg/scheme" - "github.com/karmada-io/karmada/operator/pkg/util" ) const ( @@ -85,11 +80,6 @@ func (ctrl *Controller) Reconcile(ctx context.Context, req controllerruntime.Req return controllerruntime.Result{}, err } - if err := ctrl.validateKarmada(karmada); err != nil { - klog.Error(err, "Validation failed for karmada") - return controllerruntime.Result{}, nil - } - // The object is being deleted if !karmada.DeletionTimestamp.IsZero() { val, ok := karmada.Labels[DisableCascadingDeletionLabel] @@ -102,6 +92,11 @@ func (ctrl *Controller) Reconcile(ctx context.Context, req controllerruntime.Req return ctrl.removeFinalizer(ctx, karmada) } + if err := ctrl.validateKarmada(ctx, karmada); err != nil { + klog.Errorf("Validation failed for karmada: %+v", err) + return controllerruntime.Result{}, nil + } + if err := ctrl.ensureKarmada(ctx, karmada); err != nil { return controllerruntime.Result{}, err } @@ -109,31 +104,6 @@ func (ctrl *Controller) Reconcile(ctx context.Context, req controllerruntime.Req return controllerruntime.Result{}, ctrl.syncKarmada(karmada) } -// validateKarmada ensures the Karmada resource adheres to validation rules -func (ctrl *Controller) validateKarmada(karmada *operatorv1alpha1.Karmada) error { - if karmada.Spec.Components.Etcd != nil && karmada.Spec.Components.Etcd.External != nil { - expectedSecretName := util.EtcdCertSecretName(karmada.Name) - if karmada.Spec.Components.Etcd.External.SecretRef.Name != expectedSecretName { - errorMessage := fmt.Sprintf("Secret name for external etcd client must be %s, but got %s", expectedSecretName, karmada.Spec.Components.Etcd.External.SecretRef.Name) - ctrl.EventRecorder.Event(karmada, corev1.EventTypeWarning, ValidationErrorReason, errorMessage) - - newCondition := metav1.Condition{ - Type: string(operatorv1alpha1.Ready), - Status: metav1.ConditionFalse, - Reason: ValidationErrorReason, - Message: errorMessage, - LastTransitionTime: metav1.Now(), - } - meta.SetStatusCondition(&karmada.Status.Conditions, newCondition) - if err := ctrl.Status().Update(context.TODO(), karmada); err != nil { - return err - } - return fmt.Errorf(errorMessage) - } - } - return nil -} - func (ctrl *Controller) syncKarmada(karmada *operatorv1alpha1.Karmada) error { klog.V(2).InfoS("Reconciling karmada", "name", karmada.Name) planner, err := NewPlannerFor(karmada, ctrl.Client, ctrl.Config) diff --git a/operator/pkg/controller/karmada/validating.go b/operator/pkg/controller/karmada/validating.go new file mode 100644 index 000000000..a1830145b --- /dev/null +++ b/operator/pkg/controller/karmada/validating.go @@ -0,0 +1,122 @@ +/* +Copyright 2024 The Karmada Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package karmada + +import ( + "context" + "fmt" + "net/url" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/klog/v2" + + operatorv1alpha1 "github.com/karmada-io/karmada/operator/pkg/apis/operator/v1alpha1" + "github.com/karmada-io/karmada/operator/pkg/util" +) + +func validateCRDTarball(crdTarball *operatorv1alpha1.CRDTarball, fldPath *field.Path) (errs field.ErrorList) { + if crdTarball == nil || crdTarball.HTTPSource == nil { + return nil + } + + if _, err := url.ParseRequestURI(crdTarball.HTTPSource.URL); err != nil { + errs = append(errs, field.Invalid(fldPath.Child("httpSource").Child("url"), crdTarball.HTTPSource.URL, "invalid CRDs remote URL")) + } + + return errs +} + +func validateKarmadaAPIServer(karmadaAPIServer *operatorv1alpha1.KarmadaAPIServer, hostCluster *operatorv1alpha1.HostCluster, fldPath *field.Path) (errs field.ErrorList) { + if karmadaAPIServer == nil { + return nil + } + + serviceType := karmadaAPIServer.ServiceType + if serviceType != corev1.ServiceTypeClusterIP && serviceType != corev1.ServiceTypeNodePort && serviceType != corev1.ServiceTypeLoadBalancer { + errs = append(errs, field.Invalid(fldPath.Child("serviceType"), serviceType, "unsupported service type for Karmada API server")) + } + if !util.IsInCluster(hostCluster) && serviceType == corev1.ServiceTypeClusterIP { + errs = append(errs, field.Invalid(fldPath.Child("serviceType"), serviceType, "if karmada is installed in a remote cluster, the service type of karmada-apiserver must be either NodePort or LoadBalancer")) + } + + return errs +} + +func validateETCD(etcd *operatorv1alpha1.Etcd, karmadaName string, fldPath *field.Path) (errs field.ErrorList) { + if etcd == nil || (etcd.Local == nil && etcd.External == nil) { + errs = append(errs, field.Invalid(fldPath, etcd, "unexpected empty etcd configuration")) + return errs + } + + if etcd.External != nil { + expectedSecretName := util.EtcdCertSecretName(karmadaName) + if etcd.External.SecretRef.Name != expectedSecretName { + errs = append(errs, field.Invalid(fldPath.Child("external").Child("secretRef").Child("name"), etcd.External.SecretRef.Name, "secret name for external etcd client must be "+expectedSecretName)) + } + } + + if etcd.Local != nil && etcd.Local.CommonSettings.Replicas != nil { + replicas := *etcd.Local.CommonSettings.Replicas + + if (replicas % 2) == 0 { + klog.Warningf("invalid etcd replicas %d, expected an odd number", replicas) + } + } + + return errs +} + +func validate(karmada *operatorv1alpha1.Karmada) error { + var errs field.ErrorList + + errs = append(errs, validateCRDTarball(karmada.Spec.CRDTarball, field.NewPath("spec").Child("crdTarball"))...) + + if karmada.Spec.Components != nil { + components, fldPath := karmada.Spec.Components, field.NewPath("spec").Child("components") + + errs = append(errs, validateKarmadaAPIServer(components.KarmadaAPIServer, karmada.Spec.HostCluster, fldPath.Child("karmadaAPIServer"))...) + errs = append(errs, validateETCD(components.Etcd, karmada.Name, fldPath.Child("etcd"))...) + } + + if len(errs) > 0 { + return fmt.Errorf("validation errors: %v", errs) + } + return nil +} + +func (ctrl *Controller) validateKarmada(ctx context.Context, karmada *operatorv1alpha1.Karmada) error { + if err := validate(karmada); err != nil { + ctrl.EventRecorder.Event(karmada, corev1.EventTypeWarning, ValidationErrorReason, err.Error()) + + newCondition := metav1.Condition{ + Type: string(operatorv1alpha1.Ready), + Status: metav1.ConditionFalse, + Reason: ValidationErrorReason, + Message: err.Error(), + LastTransitionTime: metav1.Now(), + } + meta.SetStatusCondition(&karmada.Status.Conditions, newCondition) + if updateErr := ctrl.Status().Update(ctx, karmada); updateErr != nil { + return fmt.Errorf("failed to update validate condition, validate error: %+v, update err: %+v", err, updateErr) + } + return err + } + return nil +} diff --git a/operator/pkg/controller/karmada/validating_test.go b/operator/pkg/controller/karmada/validating_test.go new file mode 100644 index 000000000..5dae97fba --- /dev/null +++ b/operator/pkg/controller/karmada/validating_test.go @@ -0,0 +1,194 @@ +package karmada + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + operatorv1alpha1 "github.com/karmada-io/karmada/operator/pkg/apis/operator/v1alpha1" + "github.com/karmada-io/karmada/operator/pkg/util" +) + +func Test_validate(t *testing.T) { + karmadaType := metav1.TypeMeta{Kind: "Karmada", APIVersion: "operator.karmada.io/v1alpha1"} + testObj := metav1.ObjectMeta{Name: "test", Namespace: "test"} + + tests := []struct { + name string + karmada *operatorv1alpha1.Karmada + wantErr bool + }{ + { + name: "KarmadaSpec is empty", + karmada: &operatorv1alpha1.Karmada{ + TypeMeta: karmadaType, + ObjectMeta: testObj, + Spec: operatorv1alpha1.KarmadaSpec{}, + }, + wantErr: false, + }, + { + name: "CRDTarball HTTPSource is invalid", + karmada: &operatorv1alpha1.Karmada{ + TypeMeta: karmadaType, + ObjectMeta: testObj, + Spec: operatorv1alpha1.KarmadaSpec{ + CRDTarball: &operatorv1alpha1.CRDTarball{ + HTTPSource: &operatorv1alpha1.HTTPSource{ + URL: "test", + }, + }, + }, + }, + wantErr: true, + }, + { + name: "CRDTarball HTTPSource is valid", + karmada: &operatorv1alpha1.Karmada{ + TypeMeta: karmadaType, + ObjectMeta: testObj, + Spec: operatorv1alpha1.KarmadaSpec{ + CRDTarball: &operatorv1alpha1.CRDTarball{ + HTTPSource: &operatorv1alpha1.HTTPSource{ + URL: "http://localhost", + }, + }, + }, + }, + wantErr: false, + }, + { + name: "KarmadaAPIServer ServiceType unsupported", + karmada: &operatorv1alpha1.Karmada{ + TypeMeta: karmadaType, + ObjectMeta: testObj, + Spec: operatorv1alpha1.KarmadaSpec{ + Components: &operatorv1alpha1.KarmadaComponents{ + Etcd: &operatorv1alpha1.Etcd{ + Local: &operatorv1alpha1.LocalEtcd{ + CommonSettings: operatorv1alpha1.CommonSettings{ + Image: operatorv1alpha1.Image{}, + }, + }, + }, + KarmadaAPIServer: &operatorv1alpha1.KarmadaAPIServer{ + ServiceType: "ExternalName", + }, + }, + }, + }, + wantErr: true, + }, + { + name: "KarmadaAPIServer ServiceType is invalid when using remote cluster", + karmada: &operatorv1alpha1.Karmada{ + TypeMeta: karmadaType, + ObjectMeta: testObj, + Spec: operatorv1alpha1.KarmadaSpec{ + Components: &operatorv1alpha1.KarmadaComponents{ + Etcd: &operatorv1alpha1.Etcd{ + Local: &operatorv1alpha1.LocalEtcd{ + CommonSettings: operatorv1alpha1.CommonSettings{ + Image: operatorv1alpha1.Image{}, + }, + }, + }, + KarmadaAPIServer: &operatorv1alpha1.KarmadaAPIServer{ + ServiceType: "ClusterIP", + }, + }, + HostCluster: &operatorv1alpha1.HostCluster{ + SecretRef: &operatorv1alpha1.LocalSecretReference{ + Name: "test", + }, + }, + }, + }, + wantErr: true, + }, + { + name: "KarmadaAPIServer ServiceType is valid", + karmada: &operatorv1alpha1.Karmada{ + TypeMeta: karmadaType, + ObjectMeta: testObj, + Spec: operatorv1alpha1.KarmadaSpec{ + Components: &operatorv1alpha1.KarmadaComponents{ + Etcd: &operatorv1alpha1.Etcd{ + Local: &operatorv1alpha1.LocalEtcd{ + CommonSettings: operatorv1alpha1.CommonSettings{ + Image: operatorv1alpha1.Image{}, + }, + }, + }, + KarmadaAPIServer: &operatorv1alpha1.KarmadaAPIServer{ + ServiceType: "NodePort", + }, + }, + HostCluster: &operatorv1alpha1.HostCluster{ + SecretRef: &operatorv1alpha1.LocalSecretReference{ + Name: "test", + }, + }, + }, + }, + wantErr: false, + }, + { + name: "ETCD empty configuration", + karmada: &operatorv1alpha1.Karmada{ + TypeMeta: karmadaType, + ObjectMeta: testObj, + Spec: operatorv1alpha1.KarmadaSpec{ + Components: &operatorv1alpha1.KarmadaComponents{}, + }, + }, + wantErr: true, + }, + { + name: "ExternalETCD secretref unexpected name", + karmada: &operatorv1alpha1.Karmada{ + TypeMeta: karmadaType, + ObjectMeta: testObj, + Spec: operatorv1alpha1.KarmadaSpec{ + Components: &operatorv1alpha1.KarmadaComponents{ + Etcd: &operatorv1alpha1.Etcd{ + External: &operatorv1alpha1.ExternalEtcd{ + SecretRef: operatorv1alpha1.LocalSecretReference{ + Name: "karmada-xx", + }, + }, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "ExternalETCD secretref is valid", + karmada: &operatorv1alpha1.Karmada{ + TypeMeta: karmadaType, + ObjectMeta: testObj, + Spec: operatorv1alpha1.KarmadaSpec{ + Components: &operatorv1alpha1.KarmadaComponents{ + Etcd: &operatorv1alpha1.Etcd{ + External: &operatorv1alpha1.ExternalEtcd{ + SecretRef: operatorv1alpha1.LocalSecretReference{ + Name: util.EtcdCertSecretName(testObj.Name), + }, + }, + }, + }, + }, + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validate(tt.karmada) + if (err != nil && !tt.wantErr) || (err == nil && tt.wantErr) { + t.Errorf("validate() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/operator/pkg/init.go b/operator/pkg/init.go index 3c91c69b6..6a8fa3368 100644 --- a/operator/pkg/init.go +++ b/operator/pkg/init.go @@ -19,11 +19,9 @@ package karmada import ( "errors" "fmt" - "net/url" "sync" corev1 "k8s.io/api/core/v1" - utilerrors "k8s.io/apimachinery/pkg/util/errors" utilversion "k8s.io/apimachinery/pkg/util/version" clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" @@ -55,33 +53,16 @@ type InitOptions struct { // Validate is used to validate the initOptions before creating initJob. func (opt *InitOptions) Validate() error { - var errs []error - if len(opt.Name) == 0 || len(opt.Namespace) == 0 { return errors.New("unexpected empty name or namespace") } - if opt.CRDTarball.HTTPSource != nil { - if _, err := url.Parse(opt.CRDTarball.HTTPSource.URL); err != nil { - return fmt.Errorf("unexpected invalid crds remote url %s", opt.CRDTarball.HTTPSource.URL) - } - } - if !util.IsInCluster(opt.Karmada.Spec.HostCluster) && opt.Karmada.Spec.Components.KarmadaAPIServer.ServiceType == corev1.ServiceTypeClusterIP { - return fmt.Errorf("if karmada is installed in a remote cluster, the service type of karmada-apiserver must be either NodePort or LoadBalancer") - } + _, err := utilversion.ParseGeneric(opt.KarmadaVersion) if err != nil { return fmt.Errorf("unexpected karmada invalid version %s", opt.KarmadaVersion) } - if opt.Karmada.Spec.Components.Etcd.Local != nil && opt.Karmada.Spec.Components.Etcd.Local.CommonSettings.Replicas != nil { - replicas := *opt.Karmada.Spec.Components.Etcd.Local.CommonSettings.Replicas - - if (replicas % 2) == 0 { - klog.Warningf("invalid etcd replicas %d, expected an odd number", replicas) - } - } - - return utilerrors.NewAggregate(errs) + return nil } // InitOpt defines a type of function to set InitOptions values. diff --git a/operator/pkg/tasks/init/etcd.go b/operator/pkg/tasks/init/etcd.go index 757347bdc..a73c1f0bf 100644 --- a/operator/pkg/tasks/init/etcd.go +++ b/operator/pkg/tasks/init/etcd.go @@ -68,10 +68,6 @@ func runDeployEtcd(r workflow.RunData) error { return nil } - if cfg.Etcd.Local == nil { - return errors.New("unexpected empty etcd local configuration") - } - err := etcd.EnsureKarmadaEtcd(data.RemoteClient(), cfg.Etcd.Local, data.GetName(), data.GetNamespace()) if err != nil { return fmt.Errorf("failed to install etcd component, err: %w", err)