From ea2a688f61451b5cfebf2e23fd6a85efc9d4ea69 Mon Sep 17 00:00:00 2001 From: XP <24298044+spwangxp@users.noreply.github.com> Date: Sat, 6 May 2023 01:07:41 +0800 Subject: [PATCH] fix: do not compare all svc.spec for user modified scene (#1342) * bugfix #1340: not compare all svc.spec for user modified scene Signed-off-by: spwangxp * lint code(internal/infrastructure/kubernetes) Signed-off-by: spwangxp * bugfix #1340: not compare all svc.spec for user modified scene Signed-off-by: spwangxp * bugfix #1340: not compare all svc.spec for user modified scene Signed-off-by: spwangxp * update comment for CompareSvc Signed-off-by: spwangxp --------- Signed-off-by: spwangxp Co-authored-by: spwangxp Co-authored-by: Xunzhuo Co-authored-by: Arko Dasgupta --- .../infrastructure/kubernetes/infra_client.go | 2 +- .../kubernetes/infra_resource.go | 12 +- .../kubernetes/resource/resource.go | 9 ++ .../kubernetes/resource/resource_test.go | 147 ++++++++++++++++++ 4 files changed, 164 insertions(+), 6 deletions(-) diff --git a/internal/infrastructure/kubernetes/infra_client.go b/internal/infrastructure/kubernetes/infra_client.go index a94757549e7..76425f1ce88 100644 --- a/internal/infrastructure/kubernetes/infra_client.go +++ b/internal/infrastructure/kubernetes/infra_client.go @@ -22,7 +22,7 @@ func New(cli client.Client) *InfraClient { } } -func (cli *InfraClient) Create(ctx context.Context, key client.ObjectKey, current client.Object, specific client.Object, updateChecker func() bool) error { +func (cli *InfraClient) CreateOrUpdate(ctx context.Context, key client.ObjectKey, current client.Object, specific client.Object, updateChecker func() bool) error { if err := cli.Client.Get(ctx, key, current); err != nil { if kerrors.IsNotFound(err) { // Create if it does not exist. diff --git a/internal/infrastructure/kubernetes/infra_resource.go b/internal/infrastructure/kubernetes/infra_resource.go index 798d5650bd6..e9f95dce38b 100644 --- a/internal/infrastructure/kubernetes/infra_resource.go +++ b/internal/infrastructure/kubernetes/infra_resource.go @@ -13,6 +13,8 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + + "github.com/envoyproxy/gateway/internal/infrastructure/kubernetes/resource" ) // createOrUpdateServiceAccount creates a ServiceAccount in the kube api server based on the @@ -29,7 +31,7 @@ func (i *Infra) createOrUpdateServiceAccount(ctx context.Context, r ResourceRend Name: sa.Name, } - return i.Client.Create(ctx, key, current, sa, func() bool { + return i.Client.CreateOrUpdate(ctx, key, current, sa, func() bool { return true }) } @@ -48,7 +50,7 @@ func (i *Infra) createOrUpdateConfigMap(ctx context.Context, r ResourceRender) e Name: cm.Name, } - return i.Client.Create(ctx, key, current, cm, func() bool { + return i.Client.CreateOrUpdate(ctx, key, current, cm, func() bool { return !reflect.DeepEqual(cm.Data, current.Data) }) } @@ -67,7 +69,7 @@ func (i *Infra) createOrUpdateDeployment(ctx context.Context, r ResourceRender) Name: deployment.Name, } - return i.Client.Create(ctx, key, current, deployment, func() bool { + return i.Client.CreateOrUpdate(ctx, key, current, deployment, func() bool { return !reflect.DeepEqual(deployment.Spec, current.Spec) }) } @@ -86,8 +88,8 @@ func (i *Infra) createOrUpdateService(ctx context.Context, r ResourceRender) err Name: svc.Name, } - return i.Client.Create(ctx, key, current, svc, func() bool { - return !reflect.DeepEqual(svc.Spec, current.Spec) + return i.Client.CreateOrUpdate(ctx, key, current, svc, func() bool { + return !resource.CompareSvc(svc, current) }) } diff --git a/internal/infrastructure/kubernetes/resource/resource.go b/internal/infrastructure/kubernetes/resource/resource.go index 8f71150fc65..1143d167a94 100644 --- a/internal/infrastructure/kubernetes/resource/resource.go +++ b/internal/infrastructure/kubernetes/resource/resource.go @@ -6,6 +6,8 @@ package resource import ( + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -31,3 +33,10 @@ func ExpectedServiceSpec(serviceType *egcfgv1a1.ServiceType) corev1.ServiceSpec } return serviceSpec } + +// CompareSvc compare entire Svc.Spec but ignored the ports[*].nodePort, ClusterIP and ClusterIPs in case user have modified for some scene. +func CompareSvc(currentSvc, originalSvc *corev1.Service) bool { + return cmp.Equal(currentSvc.Spec, originalSvc.Spec, + cmpopts.IgnoreFields(corev1.ServicePort{}, "NodePort"), + cmpopts.IgnoreFields(corev1.ServiceSpec{}, "ClusterIP", "ClusterIPs")) +} diff --git a/internal/infrastructure/kubernetes/resource/resource_test.go b/internal/infrastructure/kubernetes/resource/resource_test.go index b800bd16b33..b211b6392a0 100644 --- a/internal/infrastructure/kubernetes/resource/resource_test.go +++ b/internal/infrastructure/kubernetes/resource/resource_test.go @@ -8,6 +8,9 @@ package resource import ( "testing" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -91,3 +94,147 @@ func TestGetSelector(t *testing.T) { }) } } + +func TestCompareSvc(t *testing.T) { + cases := []struct { + ExpectRet bool + NewSvc *corev1.Service + OriginalSvc *corev1.Service + }{ + { + // Only Spec.Ports[*].NodePort and ClusterType is different + ExpectRet: true, + NewSvc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-service", + Namespace: "default", + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + NodePort: 30000, + TargetPort: intstr.FromInt(8080), + }, + }, + Selector: map[string]string{ + "app": "my-app", + }, + Type: "NodePort", + }, + }, + OriginalSvc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-service", + Namespace: "default", + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + NodePort: 30001, + TargetPort: intstr.FromInt(8080), + }, + }, + Selector: map[string]string{ + "app": "my-app", + }, + Type: "NodePort", + }, + }, + }, { + // Only Spec.Ports[*].Port is different + ExpectRet: false, + NewSvc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-service", + Namespace: "default", + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + TargetPort: intstr.FromInt(8080), + }, + }, + Selector: map[string]string{ + "app": "my-app", + }, + Type: "ClusterIP", + }, + }, + OriginalSvc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-service", + Namespace: "default", + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 90, + TargetPort: intstr.FromInt(8080), + }, + }, + Selector: map[string]string{ + "app": "my-app", + }, + Type: "ClusterIP", + }, + }, + }, + { + // only Spec.ClusterIP is different, NewSvc's ClusterIP is nil build by ResourceRender + ExpectRet: true, + NewSvc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-service", + Namespace: "default", + }, + Spec: corev1.ServiceSpec{ + ClusterIP: "", + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + TargetPort: intstr.FromInt(8080), + }, + }, + Selector: map[string]string{ + "app": "my-app", + }, + Type: "ClusterIP", + }, + }, + OriginalSvc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-service", + Namespace: "default", + }, + Spec: corev1.ServiceSpec{ + ClusterIP: "192.168.1.1", + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + TargetPort: intstr.FromInt(8080), + }, + }, + Selector: map[string]string{ + "app": "my-app", + }, + Type: "ClusterIP", + }, + }, + }, + } + + for _, tc := range cases { + t.Run("", func(t *testing.T) { + assert.Equal(t, tc.ExpectRet, CompareSvc(tc.NewSvc, tc.OriginalSvc), "expectedCompareSvcReturn(%v)", tc.ExpectRet) + }) + } +}