From aae2408c447026d32c4dbdae96dd318eeda885ad 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 | 0 .../infrastructure/kubernetes/infra_client.go | 2 +- .../kubernetes/infra_resource.go | 11 +- .../kubernetes/resource/resource.go | 11 +- .../kubernetes/resource/resource_test.go | 147 ++++++++++++++++++ 5 files changed, 163 insertions(+), 8 deletions(-) create mode 100644 internal/infrastructure/kubernetes/applier/apply.go diff --git a/internal/infrastructure/kubernetes/applier/apply.go b/internal/infrastructure/kubernetes/applier/apply.go new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/internal/infrastructure/kubernetes/infra_client.go b/internal/infrastructure/kubernetes/infra_client.go index a94757549e77..76425f1ce883 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 798d5650bd6c..c8a1307bd73f 100644 --- a/internal/infrastructure/kubernetes/infra_resource.go +++ b/internal/infrastructure/kubernetes/infra_resource.go @@ -9,6 +9,7 @@ import ( "context" "reflect" + "github.com/envoyproxy/gateway/internal/infrastructure/kubernetes/resource" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -29,7 +30,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 +49,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 +68,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 +87,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 8f71150fc65e..671c1a2bb135 100644 --- a/internal/infrastructure/kubernetes/resource/resource.go +++ b/internal/infrastructure/kubernetes/resource/resource.go @@ -6,10 +6,11 @@ package resource import ( + egcfgv1a1 "github.com/envoyproxy/gateway/api/config/v1alpha1" + "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" - - egcfgv1a1 "github.com/envoyproxy/gateway/api/config/v1alpha1" ) // GetSelector returns a label selector used to select resources @@ -31,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(currentSvc, originalSvc *corev1.Service) bool { + return cmp.Equal(currentSvc.Spec.Selector, originalSvc.Spec.Selector) && + cmp.Equal(currentSvc.Spec.Ports, originalSvc.Spec.Ports, cmpopts.IgnoreFields(corev1.ServicePort{}, "NodePort")) +} diff --git a/internal/infrastructure/kubernetes/resource/resource_test.go b/internal/infrastructure/kubernetes/resource/resource_test.go index f9fd40c52473..ae4963e3bfb8 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" @@ -76,3 +79,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) + }) + } +}