Merge pull request #5814 from chaosi-zju/validate

refactor operator module validation logic for Karmada objects

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)

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
+}

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)
+			}
+		})
+	}
+}

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 {
+	return 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)
 }
 
 // InitOpt defines a type of function to set InitOptions values.

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)