From 4cbb1a0cfc97bc3641cf523ec4c04e7560de1147 Mon Sep 17 00:00:00 2001 From: spwangxp Date: Sun, 23 Apr 2023 14:07:05 +0800 Subject: [PATCH] bugfix #1340: not compare all svc.spec for user modified scene Signed-off-by: spwangxp --- .../kubernetes/applier/apply.go | 3 +- .../infrastructure/kubernetes/utils/utils.go | 8 + .../kubernetes/utils/utils_test.go | 146 ++++++++++++++++++ 3 files changed, 156 insertions(+), 1 deletion(-) diff --git a/internal/infrastructure/kubernetes/applier/apply.go b/internal/infrastructure/kubernetes/applier/apply.go index 6aa7f751d6ce..f999d6b9d281 100644 --- a/internal/infrastructure/kubernetes/applier/apply.go +++ b/internal/infrastructure/kubernetes/applier/apply.go @@ -8,6 +8,7 @@ package applier import ( "context" "fmt" + "github.com/envoyproxy/gateway/internal/infrastructure/kubernetes/utils" "reflect" appsv1 "k8s.io/api/apps/v1" @@ -122,7 +123,7 @@ func (i *Instance) CreateOrUpdateService(ctx context.Context, svc *corev1.Servic } } else { // Update if current value is different. - if !reflect.DeepEqual(svc.Spec, current.Spec) { + if !utils.CompareSvc(svc, current) { svc.ResourceVersion = current.ResourceVersion svc.UID = current.UID if err := i.Client.Update(ctx, svc); err != nil { diff --git a/internal/infrastructure/kubernetes/utils/utils.go b/internal/infrastructure/kubernetes/utils/utils.go index e7203d3ca89a..2ddce6d57951 100644 --- a/internal/infrastructure/kubernetes/utils/utils.go +++ b/internal/infrastructure/kubernetes/utils/utils.go @@ -6,6 +6,8 @@ package utils 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" @@ -30,3 +32,9 @@ func ExpectedServiceSpec(serviceType *egcfgv1a1.ServiceType) corev1.ServiceSpec } return serviceSpec } + +// CompareSvc Only compare the selector and ports(not include nodePort) in case user have modified for some scene. +func CompareSvc(newSvc, originalSvc *corev1.Service) bool { + return cmp.Equal(newSvc.Spec.Selector, originalSvc.Spec.Selector) && + cmp.Equal(newSvc.Spec.Ports, originalSvc.Spec.Ports, cmpopts.IgnoreFields(corev1.ServicePort{}, "NodePort")) +} diff --git a/internal/infrastructure/kubernetes/utils/utils_test.go b/internal/infrastructure/kubernetes/utils/utils_test.go index 1b5f88f71cb2..becd17b201fc 100644 --- a/internal/infrastructure/kubernetes/utils/utils_test.go +++ b/internal/infrastructure/kubernetes/utils/utils_test.go @@ -6,6 +6,8 @@ package utils import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" "testing" "github.com/stretchr/testify/assert" @@ -76,3 +78,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: "ClusterIP", + }, + }, + }, { + // 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) + }) + } +}