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.

Sign up for GitHub

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
merged 8 commits into from
May 5, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
87488d9
bugfix #1340: not compare all svc.spec for user modified scene
Apr 27, 2023
65c099b
Merge branch 'main' into bugfix-1340-update-svc-after-gateway-restart
Xunzhuo Apr 27, 2023
2ac09c7
lint code(internal/infrastructure/kubernetes)
Apr 27, 2023
1500e2c
bugfix #1340: not compare all svc.spec for user modified scene
Apr 28, 2023
982272f
bugfix #1340: not compare all svc.spec for user modified scene
May 4, 2023
e115409
Merge branch 'main' into bugfix-1340-update-svc-after-gateway-restart
Xunzhuo May 4, 2023
171b1ca
update comment for CompareSvc
May 5, 2023
8f2695c
Merge branch 'main' into bugfix-1340-update-svc-after-gateway-restart
arkodg May 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/infrastructure/kubernetes/infra_client.go
View file
Open in desktop
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
11 changes: 6 additions & 5 deletions internal/infrastructure/kubernetes/infra_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
})
}
Expand All @@ -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)
})
}
Expand All @@ -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)
})
}
Expand All @@ -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)
})
}

Expand Down
11 changes: 9 additions & 2 deletions internal/infrastructure/kubernetes/resource/resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) &&
Copy link
Contributor

@arkodg arkodg Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about service Type, since we support configuring it, shouldnt we also ensure it is not being changed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my other concern with this change, is how do we track and ensure changes made to the API

gateway/api/config/v1alpha1/shared_types.go

Line 117 in 85e61d2

type KubernetesServiceSpec struct {
are checked here
rather than doing this, prefer if we allow EG to build the default spec, compare the entire spec and skip comparing fields within the spec that are generated by k8s such as ClusterIP & NodePort

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i'll made svc'type compare if my question solved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1373 I will fix it ASAP

cmp.Equal(currentSvc.Spec.Ports, originalSvc.Spec.Ports, cmpopts.IgnoreFields(corev1.ServicePort{}, "NodePort"))
}
147 changes: 147 additions & 0 deletions internal/infrastructure/kubernetes/resource/resource_test.go
View file
Open in desktop
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 @@ -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)
})
}
}