Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not compare all svc.spec for user modified scene #1342

Merged
2 changes: 1 addition & 1 deletion internal/infrastructure/kubernetes/infra_client.go
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
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
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
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)
})
}
}