Fixed unexpected behaviors in webhooks (#580)

.github/workflows/ci-tests.yml

@@ -0,0 +1,41 @@
+name: run tests
+
+on:
+  pull_request:
+    branches:
+      - master
+      - "*_release"
+    paths:
+      - '**/*.go'
+  push:
+    branches:
+      - master
+      - "*_release"
+    paths:
+      - '**/*.go'
+
+env:
+  GO_VERSION: "1.22"
+
+jobs:
+  run-tests:
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v3
+
+      - name: Set up Go 1.x
+        uses: actions/setup-go@v3
+        with:
+          go-version: ${{ env.GO_VERSION }}
+
+      - name: go mod vendor
+        run: go mod vendor
+
+      - name: install tools
+        run: make tools
+
+      - name: test webhooks
+        run: make test-webhooks
+

api/v1alpha1/obcluster_types.go

@@ -88,5 +88,7 @@ func init() {
 }
 
 func (c *OBCluster) SupportStaticIP() bool {
-	return c.Annotations[oceanbaseconst.AnnotationsSupportStaticIP] == "true"
+	return c.Annotations[oceanbaseconst.AnnotationsSupportStaticIP] == "true" ||
+		c.Annotations[oceanbaseconst.AnnotationsMode] == oceanbaseconst.ModeService ||
+		c.Annotations[oceanbaseconst.AnnotationsMode] == oceanbaseconst.ModeStandalone
 }

api/v1alpha1/obcluster_webhook_test.go

@@ -148,7 +148,7 @@ var _ = Describe("Test OBCluster Webhook", Label("webhook"), func() {
 
 	It("Validate existence of secrets", func() {
 		By("Create normal cluster")
-		cluster := newOBCluster("test", 1, 1)
+		cluster := newOBCluster("test3", 1, 1)
 		cluster.Spec.UserSecrets.Monitor = ""
 		cluster.Spec.UserSecrets.ProxyRO = ""
 		cluster.Spec.UserSecrets.Operator = ""
@@ -158,17 +158,19 @@ var _ = Describe("Test OBCluster Webhook", Label("webhook"), func() {
 		cluster2.Spec.UserSecrets.Monitor = "secret-that-does-not-exist"
 		cluster2.Spec.UserSecrets.ProxyRO = ""
 		cluster2.Spec.UserSecrets.Operator = ""
-		Expect(k8sClient.Create(ctx, cluster)).ShouldNot(Succeed())
+		Expect(k8sClient.Create(ctx, cluster2)).Should(Succeed())
 
+		cluster3 := newOBCluster("test3", 1, 1)
 		cluster2.Spec.UserSecrets.Monitor = wrongKeySecret
-		Expect(k8sClient.Create(ctx, cluster)).ShouldNot(Succeed())
+		Expect(k8sClient.Create(ctx, cluster3)).ShouldNot(Succeed())
 
 		Expect(k8sClient.Delete(ctx, cluster)).Should(Succeed())
+		Expect(k8sClient.Delete(ctx, cluster2)).Should(Succeed())
 	})
 
 	It("Validate secrets creation and fetch them", func() {
 		By("Create normal cluster")
-		cluster := newOBCluster("test", 1, 1)
+		cluster := newOBCluster("test-create-secrets", 1, 1)
 		cluster.Spec.UserSecrets.Monitor = ""
 		cluster.Spec.UserSecrets.ProxyRO = ""
 		cluster.Spec.UserSecrets.Operator = ""
@@ -178,6 +180,7 @@ var _ = Describe("Test OBCluster Webhook", Label("webhook"), func() {
 		Expect(cluster.Spec.UserSecrets.Monitor).ShouldNot(BeEmpty())
 		Expect(cluster.Spec.UserSecrets.ProxyRO).ShouldNot(BeEmpty())
 		Expect(cluster.Spec.UserSecrets.Operator).ShouldNot(BeEmpty())
+		Expect(k8sClient.Delete(ctx, cluster)).Should(Succeed())
 	})
 
 	It("Validate single pvc with multiple storage classes", func() {

api/v1alpha1/obtenantbackuppolicy_webhook.go

@@ -304,44 +304,46 @@ func (r *OBTenantBackupPolicy) validateDestination(cluster *OBCluster, dest *api
 	if !pattern.MatchString(dest.Path) {
 		return field.Invalid(field.NewPath("spec").Child(fieldName).Child("destination"), dest.Path, "invalid backup destination path, the path format should be "+pattern.String())
 	}
-	if dest.Type != constants.BackupDestTypeNFS && dest.OSSAccessSecret == "" {
-		return field.Invalid(field.NewPath("spec").Child(fieldName).Child("destination"), dest.OSSAccessSecret, "OSSAccessSecret is required when backing up data to OSS, COS or S3")
-	}
-	secret := &v1.Secret{}
-	err := bakClt.Get(context.Background(), types.NamespacedName{
-		Namespace: r.GetNamespace(),
-		Name:      dest.OSSAccessSecret,
-	}, secret)
-	fieldPath := field.NewPath("spec").Child(fieldName).Child("destination").Child("ossAccessSecret")
-	if err != nil {
-		if apierrors.IsNotFound(err) {
-			return field.Invalid(fieldPath, dest.OSSAccessSecret, "Given OSSAccessSecret not found")
-		}
-		return field.InternalError(fieldPath, err)
-	}
-	// All the following types need accessId and accessKey
-	switch dest.Type {
-	case
-		constants.BackupDestTypeCOS,
-		constants.BackupDestTypeOSS,
-		constants.BackupDestTypeS3,
-		constants.BackupDestTypeS3Compatible:
-		if _, ok := secret.Data["accessId"]; !ok {
-			return field.Invalid(fieldPath, dest.OSSAccessSecret, "accessId field not found in given OSSAccessSecret")
+	if dest.Type != constants.BackupDestTypeNFS {
+		if dest.OSSAccessSecret == "" {
+			return field.Invalid(field.NewPath("spec").Child(fieldName).Child("destination"), dest.OSSAccessSecret, "OSSAccessSecret is required when backing up data to OSS, COS or S3")
 		}
-		if _, ok := secret.Data["accessKey"]; !ok {
-			return field.Invalid(fieldPath, dest.OSSAccessSecret, "accessKey field not found in given OSSAccessSecret")
+		secret := &v1.Secret{}
+		err := bakClt.Get(context.Background(), types.NamespacedName{
+			Namespace: r.GetNamespace(),
+			Name:      dest.OSSAccessSecret,
+		}, secret)
+		fieldPath := field.NewPath("spec").Child(fieldName).Child("destination").Child("ossAccessSecret")
+		if err != nil {
+			if apierrors.IsNotFound(err) {
+				return field.Invalid(fieldPath, dest.OSSAccessSecret, "Given OSSAccessSecret not found")
+			}
+			return field.InternalError(fieldPath, err)
 		}
-	}
-	// The following types need additional fields
-	switch dest.Type {
-	case constants.BackupDestTypeCOS:
-		if _, ok := secret.Data["appId"]; !ok {
-			return field.Invalid(fieldPath, dest.OSSAccessSecret, "appId field not found in given OSSAccessSecret")
+		// All the following types need accessId and accessKey
+		switch dest.Type {
+		case
+			constants.BackupDestTypeCOS,
+			constants.BackupDestTypeOSS,
+			constants.BackupDestTypeS3,
+			constants.BackupDestTypeS3Compatible:
+			if _, ok := secret.Data["accessId"]; !ok {
+				return field.Invalid(fieldPath, dest.OSSAccessSecret, "accessId field not found in given OSSAccessSecret")
+			}
+			if _, ok := secret.Data["accessKey"]; !ok {
+				return field.Invalid(fieldPath, dest.OSSAccessSecret, "accessKey field not found in given OSSAccessSecret")
+			}
 		}
-	case constants.BackupDestTypeS3:
-		if _, ok := secret.Data["s3Region"]; !ok {
-			return field.Invalid(fieldPath, dest.OSSAccessSecret, "s3Region field not found in given OSSAccessSecret")
+		// The following types need additional fields
+		switch dest.Type {
+		case constants.BackupDestTypeCOS:
+			if _, ok := secret.Data["appId"]; !ok {
+				return field.Invalid(fieldPath, dest.OSSAccessSecret, "appId field not found in given OSSAccessSecret")
+			}
+		case constants.BackupDestTypeS3:
+			if _, ok := secret.Data["s3Region"]; !ok {
+				return field.Invalid(fieldPath, dest.OSSAccessSecret, "s3Region field not found in given OSSAccessSecret")
+			}
 		}
 	}
 	return nil

api/v1alpha1/obtenantoperation_webhook.go

@@ -226,15 +226,23 @@ func (r *OBTenantOperation) validateMutation() error {
 				allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("targetTenant"), r.Spec.TargetTenant, "The target tenant is not a standby"))
 			}
 		}
+	case constants.TenantOpSetUnitNumber,
+		constants.TenantOpSetConnectWhiteList,
+		constants.TenantOpAddResourcePools,
+		constants.TenantOpModifyResourcePools,
+		constants.TenantOpDeleteResourcePools:
+		return r.validateNewOperations()
 	default:
-		if r.Spec.TargetTenant == nil {
-			allErrs = append(allErrs, field.Required(field.NewPath("spec").Child("targetTenant"), "name of targetTenant is required"))
-		}
-	}
-	if len(allErrs) != 0 {
-		return allErrs.ToAggregate()
+		allErrs = append(allErrs, field.Required(field.NewPath("spec").Child("type"), string(r.Spec.Type)+" type of operation is not supported"))
 	}
+	return allErrs.ToAggregate()
+}
 
+func (r *OBTenantOperation) validateNewOperations() error {
+	if r.Spec.TargetTenant == nil {
+		return field.Required(field.NewPath("spec").Child("targetTenant"), "name of targetTenant is required")
+	}
+	allErrs := field.ErrorList{}
 	obtenant := &OBTenant{}
 	err := clt.Get(context.Background(), types.NamespacedName{Name: *r.Spec.TargetTenant, Namespace: r.Namespace}, obtenant)
 	if err != nil {
@@ -274,9 +282,11 @@ func (r *OBTenantOperation) validateMutation() error {
 			} else {
 				allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("targetTenant"), r.Spec.TargetTenant, "The target tenant's cluster "+obtenant.Spec.ClusterName+" does not exist"))
 			}
+			break
 		}
 		if obcluster.Spec.Topology == nil {
 			allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("targetTenant"), r.Spec.TargetTenant, "The target tenant's cluster "+obtenant.Spec.ClusterName+" does not have a topology"))
+			break
 		}
 		pools := make(map[string]any)
 		for _, pool := range obtenant.Spec.Pools {
@@ -287,6 +297,9 @@ func (r *OBTenantOperation) validateMutation() error {
 				allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("addResourcePools"), r.Spec.AddResourcePools, "The resource pool already exists"))
 			}
 		}
+		if len(allErrs) != 0 {
+			return allErrs.ToAggregate()
+		}
 		zonesInOBCluster := make(map[string]any, len(obcluster.Spec.Topology))
 		for _, zone := range obcluster.Spec.Topology {
 			zonesInOBCluster[zone.Zone] = struct{}{}
@@ -313,6 +326,7 @@ func (r *OBTenantOperation) validateMutation() error {
 	case constants.TenantOpDeleteResourcePools:
 		if len(r.Spec.DeleteResourcePools) == 0 {
 			allErrs = append(allErrs, field.Required(field.NewPath("spec").Child("deleteResourcePools"), "deleteResourcePools is required"))
+			break
 		}
 		pools := make(map[string]any)
 		for _, pool := range obtenant.Spec.Pools {
@@ -324,7 +338,6 @@ func (r *OBTenantOperation) validateMutation() error {
 			}
 		}
 	default:
-		allErrs = append(allErrs, field.Required(field.NewPath("spec").Child("type"), string(r.Spec.Type)+" type of operation is not supported"))
 	}
 	return allErrs.ToAggregate()
 }

api/v1alpha1/obtenantoperation_webhook_test.go

@@ -13,10 +13,11 @@ See the Mulan PSL v2 for more details.
 package v1alpha1
 
 import (
-	apiconsts "github.com/oceanbase/ob-operator/api/constants"
-
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/api/resource"
+
+	apiconsts "github.com/oceanbase/ob-operator/api/constants"
 )
 
 var _ = Describe("Test OBTenantOperation Webhook", Label("webhook"), Serial, func() {
@@ -25,7 +26,7 @@ var _ = Describe("Test OBTenantOperation Webhook", Label("webhook"), Serial, fun
 	tenantStandby := "test-tenant-for-operation2"
 
 	It("Create cluster and tenants", func() {
-		c := newOBCluster(clusterName, 1, 1)
+		c := newOBCluster(clusterName, 3, 1)
 		t := newOBTenant(tenantPrimary, clusterName)
 		t2 := newOBTenant(tenantStandby, clusterName)
 		t2.Spec.TenantRole = apiconsts.TenantRoleStandby
@@ -35,6 +36,10 @@ var _ = Describe("Test OBTenantOperation Webhook", Label("webhook"), Serial, fun
 		Expect(k8sClient.Create(ctx, c)).Should(Succeed())
 		Expect(k8sClient.Create(ctx, t)).Should(Succeed())
 		Expect(k8sClient.Create(ctx, t2)).Should(Succeed())
+
+		t.Status.TenantRole = apiconsts.TenantRolePrimary
+		t.Status.Pools = []ResourcePoolStatus{}
+		Expect(k8sClient.Status().Update(ctx, t)).Should(Succeed())
 	})
 
 	It("Check operation types", func() {
@@ -119,6 +124,9 @@ var _ = Describe("Test OBTenantOperation Webhook", Label("webhook"), Serial, fun
 		notexist := "tenant-not-exist"
 		op.Spec.TargetTenant = &notexist
 		Expect(k8sClient.Create(ctx, op)).ShouldNot(Succeed())
+
+		op.Spec.TargetTenant = &tenantPrimary
+		Expect(k8sClient.Create(ctx, op)).Should(Succeed())
 	})
 
 	It("Check operation replay log", func() {
@@ -144,4 +152,90 @@ var _ = Describe("Test OBTenantOperation Webhook", Label("webhook"), Serial, fun
 		}
 		Expect(k8sClient.Create(ctx, op)).ShouldNot(Succeed())
 	})
+
+	It("Check adding resource pools", func() {
+		op := newTenantOperation(tenantPrimary)
+		op.Spec.Type = apiconsts.TenantOpAddResourcePools
+		op.Spec.AddResourcePools = []ResourcePoolSpec{{
+			Zone: "zone1",
+			Type: &LocalityType{
+				Name:     "Full",
+				Replica:  1,
+				IsActive: true,
+			},
+			UnitConfig: &UnitConfig{
+				MaxCPU:      resource.MustParse("1"),
+				MemorySize:  resource.MustParse("5Gi"),
+				MinCPU:      resource.MustParse("1"),
+				MaxIops:     1024,
+				MinIops:     1024,
+				IopsWeight:  2,
+				LogDiskSize: resource.MustParse("12Gi"),
+			},
+		}}
+
+		notexist := "tenant-not-exist"
+		op.Spec.TargetTenant = &notexist
+
+		Expect(k8sClient.Create(ctx, op)).ShouldNot(Succeed())
+
+		op.Spec.TargetTenant = &tenantPrimary
+		Expect(k8sClient.Create(ctx, op)).ShouldNot(Succeed())
+
+		op.Spec.Force = true
+		Expect(k8sClient.Create(ctx, op)).Should(Succeed())
+
+		// Delete resource pool
+		opDel := newTenantOperation(tenantPrimary)
+		opDel.Spec.Type = apiconsts.TenantOpDeleteResourcePools
+		opDel.Spec.DeleteResourcePools = []string{"zone0"}
+		opDel.Spec.TargetTenant = &tenantPrimary
+		Expect(k8sClient.Create(ctx, opDel)).ShouldNot(Succeed())
+		opDel.Spec.Force = true
+		Expect(k8sClient.Create(ctx, opDel)).Should(Succeed())
+	})
+
+	It("Check modifying resource pools", func() {
+		op := newTenantOperation(tenantPrimary)
+		op.Spec.Type = apiconsts.TenantOpModifyResourcePools
+		op.Spec.ModifyResourcePools = []ResourcePoolSpec{{
+			Zone: "zone0",
+			Type: &LocalityType{
+				Name:     "Full",
+				Replica:  1,
+				IsActive: true,
+			},
+			UnitConfig: &UnitConfig{
+				MaxCPU:      resource.MustParse("6"),
+				MemorySize:  resource.MustParse("6Gi"),
+				MinCPU:      resource.MustParse("2"),
+				MaxIops:     1024,
+				MinIops:     1024,
+				IopsWeight:  2,
+				LogDiskSize: resource.MustParse("12Gi"),
+			},
+		}}
+
+		op.Spec.TargetTenant = &tenantPrimary
+		Expect(k8sClient.Create(ctx, op)).ShouldNot(Succeed())
+
+		op.Spec.Force = true
+		Expect(k8sClient.Create(ctx, op)).Should(Succeed())
+	})
+
+	It("Check setting connection white list", func() {
+		op := newTenantOperation(tenantPrimary)
+		op.Spec.Type = apiconsts.TenantOpSetConnectWhiteList
+		op.Spec.ConnectWhiteList = "%,127.0.0.1"
+		op.Spec.Force = true
+		Expect(k8sClient.Create(ctx, op)).Should(Succeed())
+	})
+
+	It("Check setting unit number", func() {
+		op := newTenantOperation(tenantPrimary)
+		op.Spec.Type = apiconsts.TenantOpSetUnitNumber
+		op.Spec.UnitNumber = 2
+		op.Spec.Force = true
+		Expect(k8sClient.Create(ctx, op)).Should(Succeed())
+	})
 })

api/v1alpha1/webhook_suite_test.go

@@ -210,6 +210,8 @@ var _ = AfterSuite(func() {
 	By("Clean auxiliary resources")
 	cancel()
 	By("tearing down the test environment")
-	err := testEnv.Stop()
-	Expect(err).NotTo(HaveOccurred())
+	if testEnv != nil {
+		err := testEnv.Stop()
+		Expect(err).NotTo(HaveOccurred())
+	}
 })

make/deps.mk

@@ -39,7 +39,7 @@ install-delve: ## Install delve, a debugger for the Go programming language. Mor
 	go install github.com/go-delve/delve/cmd/dlv@master
 
 .PHONY: tools 
-tools: kustomize controller-gen envtest install-delve ## Download all tools
+tools: kustomize controller-gen envtest ## Download all tools
 
 .PHONY: init-generator
 init-generator: ## Install generator tools