From 2ee2f81992e017af68b2f9b7bdfc65751e464320 Mon Sep 17 00:00:00 2001 From: d-kuro Date: Fri, 25 Feb 2022 04:00:21 +0900 Subject: [PATCH 1/2] Copy values to replicaServiceTemplate in conversion webhook Signed-off-by: d-kuro --- api/v1beta1/conversion.go | 101 +++++++++++++++++++++++---------- api/v1beta1/conversion_test.go | 69 +++++++++++++++++++--- 2 files changed, 132 insertions(+), 38 deletions(-) diff --git a/api/v1beta1/conversion.go b/api/v1beta1/conversion.go index 8041138dc..9dbb036ba 100644 --- a/api/v1beta1/conversion.go +++ b/api/v1beta1/conversion.go @@ -5,6 +5,7 @@ import ( "unsafe" "github.com/cybozu-go/moco/api/v1beta2" + "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apiconversion "k8s.io/apimachinery/pkg/conversion" "k8s.io/apimachinery/pkg/runtime" @@ -12,6 +13,7 @@ import ( ) const ( + SpecPrimaryServiceTemplateAnnotation = "mysqlcluster.v1beta2.moco.cybozu.com/spec.primaryServiceTemplate" SpecReplicaServiceTemplateAnnotation = "mysqlcluster.v1beta2.moco.cybozu.com/spec.replicaServiceTemplate" ) @@ -51,7 +53,7 @@ func Convert__MySQLCluster_To_v1beta2_MySQLCluster(in *MySQLCluster, out *v1beta return err } - if _, err := unmarshalReplicaServiceTemplate(in, out); err != nil { + if err := unmarshalServiceTemplate(in, out); err != nil { return err } @@ -64,7 +66,7 @@ func Convert_v1beta2_MySQLCluster_To__MySQLCluster(in *v1beta2.MySQLCluster, out return err } - if err := marshalReplicaServiceTemplate(&in.Spec, out); err != nil { + if err := marshalServiceTemplate(&in.Spec, out); err != nil { return err } @@ -76,8 +78,6 @@ func Convert__MySQLClusterSpec_To_v1beta2_MySQLClusterSpec(in *MySQLClusterSpec, return err } - out.PrimaryServiceTemplate = (*v1beta2.ServiceTemplate)(unsafe.Pointer(in.ServiceTemplate)) - return nil } @@ -91,20 +91,14 @@ func Convert_v1beta2_MySQLClusterSpec_To__MySQLClusterSpec(in *v1beta2.MySQLClus return nil } -// marshalReplicaServiceTemplate stores the service template as json data in the destination object annotations. -func marshalReplicaServiceTemplate(spec *v1beta2.MySQLClusterSpec, dst metav1.Object) error { - if spec.ReplicaServiceTemplate == nil { +// marshalServiceTemplate stores the service template as json data in the destination object annotations. +func marshalServiceTemplate(spec *v1beta2.MySQLClusterSpec, dst metav1.Object) error { + if spec.PrimaryServiceTemplate == nil && spec.ReplicaServiceTemplate == nil { return nil } - u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(spec.ReplicaServiceTemplate) - if err != nil { - return err - } - - data, err := json.Marshal(u) - if err != nil { - return err + if equality.Semantic.DeepEqual(spec.PrimaryServiceTemplate, spec.ReplicaServiceTemplate) { + return nil } annotations := dst.GetAnnotations() @@ -112,34 +106,81 @@ func marshalReplicaServiceTemplate(spec *v1beta2.MySQLClusterSpec, dst metav1.Ob annotations = map[string]string{} } - annotations[SpecReplicaServiceTemplateAnnotation] = string(data) - dst.SetAnnotations(annotations) + if spec.PrimaryServiceTemplate != nil { + u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(spec.PrimaryServiceTemplate) + if err != nil { + return err + } - return nil -} + data, err := json.Marshal(u) + if err != nil { + return err + } -// unmarshalReplicaServiceTemplate tries to retrieve the data from the annotation and unmarshal it into the service template passed as input. -func unmarshalReplicaServiceTemplate(src metav1.Object, dst *v1beta2.MySQLCluster) (bool, error) { - data, ok := src.GetAnnotations()[SpecReplicaServiceTemplateAnnotation] - if !ok { - return false, nil + annotations[SpecPrimaryServiceTemplateAnnotation] = string(data) } - var s *v1beta2.ServiceTemplate + if spec.ReplicaServiceTemplate != nil { + u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(spec.ReplicaServiceTemplate) + if err != nil { + return err + } + + data, err := json.Marshal(u) + if err != nil { + return err + } - if err := json.Unmarshal([]byte(data), &s); err != nil { - return false, err + annotations[SpecReplicaServiceTemplateAnnotation] = string(data) } - dst.Spec.ReplicaServiceTemplate = s + dst.SetAnnotations(annotations) + return nil +} + +// unmarshalServiceTemplate tries to retrieve the data from the annotation and unmarshal it into the service template passed as input. +func unmarshalServiceTemplate(src *MySQLCluster, dst *v1beta2.MySQLCluster) error { dstAnnotation := dst.GetAnnotations() - delete(dstAnnotation, SpecReplicaServiceTemplateAnnotation) + if hasServiceTemplateAnnotation(src) { + if primary, ok := src.GetAnnotations()[SpecPrimaryServiceTemplateAnnotation]; ok { + var s *v1beta2.ServiceTemplate + if err := json.Unmarshal([]byte(primary), &s); err != nil { + return err + } + + dst.Spec.PrimaryServiceTemplate = s + delete(dstAnnotation, SpecPrimaryServiceTemplateAnnotation) + } + + if replica, ok := src.GetAnnotations()[SpecReplicaServiceTemplateAnnotation]; ok { + var s *v1beta2.ServiceTemplate + if err := json.Unmarshal([]byte(replica), &s); err != nil { + return err + } + + dst.Spec.ReplicaServiceTemplate = s + delete(dstAnnotation, SpecReplicaServiceTemplateAnnotation) + } + } else { + // If the annotation does not exist, copy the same value to primary and replica. + serviceTemplate := (*v1beta2.ServiceTemplate)(unsafe.Pointer(src.Spec.ServiceTemplate)) + dst.Spec.PrimaryServiceTemplate = serviceTemplate.DeepCopy() + dst.Spec.ReplicaServiceTemplate = serviceTemplate.DeepCopy() + } if len(dstAnnotation) == 0 { dst.SetAnnotations(nil) } - return true, nil + return nil +} + +// hasServiceTemplateAnnotation checks if the given object has the service template annotation. +func hasServiceTemplateAnnotation(obj metav1.Object) bool { + _, primaryFound := obj.GetAnnotations()[SpecPrimaryServiceTemplateAnnotation] + _, replicaFound := obj.GetAnnotations()[SpecReplicaServiceTemplateAnnotation] + + return primaryFound || replicaFound } diff --git a/api/v1beta1/conversion_test.go b/api/v1beta1/conversion_test.go index 666f15f1a..b8c67833a 100644 --- a/api/v1beta1/conversion_test.go +++ b/api/v1beta1/conversion_test.go @@ -10,6 +10,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/apitesting/roundtrip" "k8s.io/apimachinery/pkg/runtime" + corev1ac "k8s.io/client-go/applyconfigurations/core/v1" ) func TestCompatibility(t *testing.T) { @@ -29,31 +30,83 @@ func TestCompatibility(t *testing.T) { t.Run("MySQLCluster v1beta1 => v1beta2 => v1beta1", func(t *testing.T) { for i := 0; i < 10000; i++ { - var oldCluster1, oldCluster2 MySQLCluster - var cluster mocov1beta2.MySQLCluster - f.Fuzz(&oldCluster1) + var v1beta1Cluster1, v1beta1Cluster2 MySQLCluster + var v1beta2Cluster mocov1beta2.MySQLCluster + f.Fuzz(&v1beta1Cluster1) var tmp1, tmp2 mocov1beta2.MySQLCluster - if err := scheme.Convert(oldCluster1.DeepCopy(), &tmp1, nil); err != nil { + if err := scheme.Convert(v1beta1Cluster1.DeepCopy(), &tmp1, nil); err != nil { t.Fatal(err) } - if err := scheme.Convert(&tmp1, &cluster, nil); err != nil { + if err := scheme.Convert(&tmp1, &v1beta2Cluster, nil); err != nil { t.Fatal(err) } - if err := scheme.Convert(&cluster, &tmp2, nil); err != nil { + if err := scheme.Convert(&v1beta2Cluster, &tmp2, nil); err != nil { t.Fatal(err) } - if err := scheme.Convert(&tmp2, &oldCluster2, nil); err != nil { + if err := scheme.Convert(&tmp2, &v1beta1Cluster2, nil); err != nil { t.Fatal(err) } - if diff := cmp.Diff(oldCluster1, oldCluster2, cmpopts.EquateEmpty()); diff != "" { + if diff := cmp.Diff(v1beta1Cluster1, v1beta1Cluster2, cmpopts.EquateEmpty()); diff != "" { t.Fatalf("compatibility error case #%d (-want +got):\n%s", i, diff) } } }) + t.Run("MySQLCluster v1beta2 => v1beta1 => v1beta2", func(t *testing.T) { + for i := 0; i < 10000; i++ { + var v1beta2Cluster1, v1beta2Cluster2 mocov1beta2.MySQLCluster + var v1beta1Cluster MySQLCluster + f.Fuzz(&v1beta2Cluster1) + + var tmp1, tmp2 MySQLCluster + + if err := scheme.Convert(v1beta2Cluster1.DeepCopy(), &tmp1, nil); err != nil { + t.Fatal(err) + } + if err := scheme.Convert(&tmp1, &v1beta1Cluster, nil); err != nil { + t.Fatal(err) + } + if err := scheme.Convert(&v1beta1Cluster, &tmp2, nil); err != nil { + t.Fatal(err) + } + if err := scheme.Convert(&tmp2, &v1beta2Cluster2, nil); err != nil { + t.Fatal(err) + } + + if diff := cmp.Diff(v1beta2Cluster1, v1beta2Cluster2, cmpopts.EquateEmpty()); diff != "" { + t.Fatalf("compatibility error case #%d (-want +got):\n%s", i, diff) + } + } + }) + + t.Run("MySQLCluster v1beta1 => v1beta2 ServiceTemplate will be copied", func(t *testing.T) { + var v1beta2Cluster mocov1beta2.MySQLCluster + var v1beta1Cluster MySQLCluster + f.Fuzz(&v1beta1Cluster) + + v1beta1Cluster.Spec.ServiceTemplate = &ServiceTemplate{ + ObjectMeta: ObjectMeta{}, + Spec: (*ServiceSpecApplyConfiguration)( + corev1ac.ServiceSpec(). + WithPorts(corev1ac.ServicePort(). + WithName("dummy"). + WithPort(8080), + ), + ), + } + + if err := scheme.Convert(&v1beta1Cluster, &v1beta2Cluster, nil); err != nil { + t.Fatal(err) + } + + if diff := cmp.Diff(v1beta2Cluster.Spec.PrimaryServiceTemplate, v1beta2Cluster.Spec.ReplicaServiceTemplate, cmpopts.EquateEmpty()); diff != "" { + t.Fatalf("compatibility error case (-want +got):\n%s", diff) + } + }) + t.Run("BackupPolicy v1beta1 => v1beta2 => v1beta1", func(t *testing.T) { for i := 0; i < 10000; i++ { var oldPolicy1, oldPolicy2 BackupPolicy From f37e8b654b58511114215cede55c7f2bfea7fbff Mon Sep 17 00:00:00 2001 From: d-kuro Date: Tue, 1 Mar 2022 03:42:30 +0900 Subject: [PATCH 2/2] Run conversion tests in parallel. Signed-off-by: d-kuro --- api/v1beta1/conversion_test.go | 58 ++++++++++++++++++++++++++-------- 1 file changed, 45 insertions(+), 13 deletions(-) diff --git a/api/v1beta1/conversion_test.go b/api/v1beta1/conversion_test.go index b8c67833a..ed971abd0 100644 --- a/api/v1beta1/conversion_test.go +++ b/api/v1beta1/conversion_test.go @@ -14,21 +14,12 @@ import ( ) func TestCompatibility(t *testing.T) { - scheme := runtime.NewScheme() - _ = AddToScheme(scheme) - _ = mocov1beta2.AddToScheme(scheme) - - // Suppress typed nil. - fn := func(l **corev1.ResourceList, c fuzz.Continue) { - if l != nil && *l == nil || **l == nil { - *l = nil - } - } + t.Run("MySQLCluster v1beta1 => v1beta2 => v1beta1", func(t *testing.T) { + t.Parallel() - f := roundtrip.CompatibilityTestFuzzer(scheme, []interface{}{fn}) - f.NilChance(0.5).NumElements(0, 3) + scheme := newScheme(t) + f := newCompatibilityTestFuzzer(t, scheme) - t.Run("MySQLCluster v1beta1 => v1beta2 => v1beta1", func(t *testing.T) { for i := 0; i < 10000; i++ { var v1beta1Cluster1, v1beta1Cluster2 MySQLCluster var v1beta2Cluster mocov1beta2.MySQLCluster @@ -56,6 +47,11 @@ func TestCompatibility(t *testing.T) { }) t.Run("MySQLCluster v1beta2 => v1beta1 => v1beta2", func(t *testing.T) { + t.Parallel() + + scheme := newScheme(t) + f := newCompatibilityTestFuzzer(t, scheme) + for i := 0; i < 10000; i++ { var v1beta2Cluster1, v1beta2Cluster2 mocov1beta2.MySQLCluster var v1beta1Cluster MySQLCluster @@ -83,6 +79,11 @@ func TestCompatibility(t *testing.T) { }) t.Run("MySQLCluster v1beta1 => v1beta2 ServiceTemplate will be copied", func(t *testing.T) { + t.Parallel() + + scheme := newScheme(t) + f := newCompatibilityTestFuzzer(t, scheme) + var v1beta2Cluster mocov1beta2.MySQLCluster var v1beta1Cluster MySQLCluster f.Fuzz(&v1beta1Cluster) @@ -108,6 +109,11 @@ func TestCompatibility(t *testing.T) { }) t.Run("BackupPolicy v1beta1 => v1beta2 => v1beta1", func(t *testing.T) { + t.Parallel() + + scheme := newScheme(t) + f := newCompatibilityTestFuzzer(t, scheme) + for i := 0; i < 10000; i++ { var oldPolicy1, oldPolicy2 BackupPolicy var policy mocov1beta2.BackupPolicy @@ -134,3 +140,29 @@ func TestCompatibility(t *testing.T) { } }) } + +func newScheme(t *testing.T) *runtime.Scheme { + t.Helper() + + scheme := runtime.NewScheme() + _ = AddToScheme(scheme) + _ = mocov1beta2.AddToScheme(scheme) + + return scheme +} + +func newCompatibilityTestFuzzer(t *testing.T, scheme *runtime.Scheme) *fuzz.Fuzzer { + t.Helper() + + // Suppress typed nil. + fn := func(l **corev1.ResourceList, c fuzz.Continue) { + if l != nil && *l == nil || **l == nil { + *l = nil + } + } + + f := roundtrip.CompatibilityTestFuzzer(scheme, []interface{}{fn}) + f.NilChance(0.5).NumElements(0, 3) + + return f +}