Skip to content

Commit

Permalink
fix: do not compare all svc.spec for user modified scene (#1342)
Browse files Browse the repository at this point in the history 
* bugfix #1340: not compare all svc.spec for user modified scene

Signed-off-by: spwangxp <wangshengpeng@cestc.cn>

* lint code(internal/infrastructure/kubernetes)

Signed-off-by: spwangxp <wangshengpeng@cestc.cn>

* bugfix #1340: not compare all svc.spec for user modified scene

Signed-off-by: spwangxp <wangshengpeng@cestc.cn>

* bugfix #1340: not compare all svc.spec for user modified scene

Signed-off-by: spwangxp <wangshengpeng@cestc.cn>

* update comment for CompareSvc

Signed-off-by: spwangxp <wangshengpeng@cestc.cn>

---------

Signed-off-by: spwangxp <wangshengpeng@cestc.cn>
Co-authored-by: spwangxp <wangshengpeng@cestc.cn>
Co-authored-by: Xunzhuo <bitliu@tencent.com>
Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com>
  • Loading branch information
@spwangxp @Xunzhuo @arkodg
4 people authored May 5, 2023
1 parent 297fcf1 commit ea2a688
Show file tree
Hide file tree
Showing 4 changed files with 164 additions and 6 deletions.
2 changes: 1 addition & 1 deletion internal/infrastructure/kubernetes/infra_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
12 changes: 7 additions & 5 deletions internal/infrastructure/kubernetes/infra_resource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
})
}
Expand All @@ -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)
})
}
Expand All @@ -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)
})
}
Expand All @@ -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)
})
}

Expand Down
9 changes: 9 additions & 0 deletions internal/infrastructure/kubernetes/resource/resource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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"))
}
147 changes: 147 additions & 0 deletions internal/infrastructure/kubernetes/resource/resource_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
})
}
}

0 comments on commit ea2a688

Please sign in to comment.