fix: do not compare all svc.spec for user modified scene by spwangxp · Pull Request #1342 · envoyproxy/gateway

spwangxp · 2023-04-23T07:02:38Z

What type of PR is this?

fix: #1340

What this PR does / why we need it:
if not, the ir will encounter an error when update svc.
for example, user A deployed an envoyproxy, and set the svc type to NodePort instead of LoadBalancer, then the gateway restart, the check will always be error.

Which issue(s) this PR fixes:

Fixes #1340

internal/infrastructure/kubernetes/applier/apply.go

codecov · 2023-04-23T09:17:00Z

Codecov Report

Merging #1342 (8f2695c) into main (297fcf1) will increase coverage by 0.11%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##             main    #1342      +/-   ##
==========================================
+ Coverage   62.44%   62.55%   +0.11%     
==========================================
  Files          79       79              
  Lines       11084    11088       +4     
==========================================
+ Hits         6921     6936      +15     
+ Misses       3707     3697      -10     
+ Partials      456      455       -1

Impacted Files	Coverage Δ
...ternal/infrastructure/kubernetes/infra_resource.go	`83.69% <40.00%> (ø)`
internal/infrastructure/kubernetes/infra_client.go	`75.00% <100.00%> (ø)`
...nal/infrastructure/kubernetes/resource/resource.go	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

internal/infrastructure/kubernetes/utils/utils_test.go

zirain

LGTM

cc @arkodg for second review

qicz · 2023-04-24T08:24:26Z

#1337 follow-up infrastructure refactoring may be the best choice to follow up this pr. @spwangxp

arkodg · 2023-04-24T17:36:48Z

internal/infrastructure/kubernetes/applier/apply.go

there are two bugs associated with this logic

we are comparing the entire Spec here, instead we should be comparing all the fields the infra layer sets or skip the fields the infra layer does not set https://pkg.go.dev/k8s.io/api/core/v1#ServiceSpec.

when updating the service, we need to set immutable fields like ClusterIP and ClusterIPs

// clusterIP is the IP address of the service and is usually assigned // randomly. If an address is specified manually, is in-range (as per // system configuration), and is not in use, it will be allocated to the // service; otherwise creation of the service will fail. This field may not // be changed through updates unless the type field is also being changed // to ExternalName (which requires this field to be blank) or the type // field is being changed from ExternalName (in which case this field may // optionally be specified, as describe above). Valid values are "None", // empty string (""), or a valid IP address. Setting this to "None" makes a // "headless service" (no virtual IP), which is useful when direct endpoint // connections are preferred and proxying is not required. Only applies to // types ClusterIP, NodePort, and LoadBalancer. If this field is specified // when creating a Service of type ExternalName, creation will fail. This // field will be wiped when updating a Service to type ExternalName. // More info: https://kubernetes.io/docs/concepts/services-networking/service/#virtual-ips-and-service-proxies // +optional ClusterIP [string](https://pkg.go.dev/builtin#string) `json:"clusterIP,omitempty" protobuf:"bytes,3,opt,name=clusterIP"`

1, ok, i'll add imutable fields compare we set.
2, when updating the svc, is these be better that just patch what we need rather than set everything to the new svc?
3, actually，when we use loadbalacer by default, svc.Spec.ports[*].nodePort will be set an random number by k8s, so it's not just about user modified the svc type.

Reg 2. Patch is fine, my only request is to keep the code simple so it can be enhanced with more fields in the future
Reg 3. its okay to undo what user did, to solve this, we must

support Nodeport natively

support BYO Envoy Svc and Envoy Deployment and disable managed infra

raise another pr for support NodePort natively might be a better way?

sure please go ahead @spwangxp

arkodg · 2023-04-24T17:37:45Z

internal/infrastructure/kubernetes/utils/utils.go

the infra layer also sets Type and SessionAffinity

gateway/internal/infrastructure/kubernetes/utils/utils.go

Line 23 in 690a045

func ExpectedServiceSpec(serviceType *egcfgv1a1.ServiceType) corev1.ServiceSpec {

something for the future, we should find a sane way to ensure we check what we set

is there any need to update svc type rather than follow user's setting?

added some comments above, Patch is fine as long as its easy to mantain

arkodg · 2023-04-24T17:39:06Z

thanks for highlighting this bug @spwangxp, added some comments

arkodg · 2023-04-24T20:52:33Z

also @spwangxp if you are using service type NodePort, can you please raise a issue to add support for it ?

arkodg · 2023-04-26T16:30:07Z

@qicz can you take a first pass ? with the recent refactor want to make sure the logic lives in the right place, TIA

internal/infrastructure/kubernetes/applier/apply.go

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

qicz

LGTM, pls approve ci @arkodg

Xunzhuo · 2023-04-27T06:28:00Z

I got this @qicz

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

zirain

@arkodg for final review.

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

qicz

LGTM, thanks

arkodg · 2023-04-28T16:34:30Z

/hold can we compare the entire Spec here, and ignore any fields (NodePort) . This approach will be easier to maintain in the future

I've raised two issues to track your use case @spwangxp

Support NodePort Support NodePort Type Envoy Proxy service #1378
Support user provisioned envoy fleet Support User Provisioned Envoy Proxy fleet #1379

spwangxp · 2023-04-29T02:31:47Z

/hold can we compare the entire Spec here, and ignore any fields (NodePort) . This approach will be easier to maintain in the future

I've raised two issues to track your use case @spwangxp

Support NodePort Support NodePort Type Envoy Proxy service #1378

Support user provisioned envoy fleet Support User Provisioned Envoy Proxy fleet #1379

1, actual gw just need ensure the field unchanged we set before.
2, field should exclude more than need compare, compare entire spec is harder to maintain.

arkodg · 2023-04-29T02:35:23Z

/hold can we compare the entire Spec here, and ignore any fields (NodePort) . This approach will be easier to maintain in the future
I've raised two issues to track your use case @spwangxp

Support NodePort Support NodePort Type Envoy Proxy service #1378

Support user provisioned envoy fleet Support User Provisioned Envoy Proxy fleet #1379

1, actual gw just need ensure the field unchanged we set before. 2, field should exclude more than need compare, compare entire spec is harder to maintain.

every time an API change is made to the KubernetesServiceSpec, the reviewer/maintainer will need to ensure that the comparison field is added in the infra layer which is extra cognitive load for the reviewer, else it will cause a bug
Moving the compare to Spec is simpler to maintain long term

spwangxp · 2023-04-29T02:41:56Z

/hold can we compare the entire Spec here, and ignore any fields (NodePort) . This approach will be easier to maintain in the future
I've raised two issues to track your use case @spwangxp

Support NodePort Support NodePort Type Envoy Proxy service #1378

Support user provisioned envoy fleet Support User Provisioned Envoy Proxy fleet #1379

1, actual gw just need ensure the field unchanged we set before. 2, field should exclude more than need compare, compare entire spec is harder to maintain.

every time an API change is made to the KubernetesServiceSpec, the reviewer/maintainer will need to ensure that the comparison field is added in the infra layer which is extra cognitive load for the reviewer, else it will cause a bug Moving the compare to Spec is simpler to maintain long term

check and remove ignored field should de done when KubernetesServiceSpec changed, so there is no difference.
or u hava a ignore field list already？

arkodg · 2023-04-29T02:47:00Z

The ignore list shouldnt change in the codebase, but should only only increase whenever upstream K8s API adds a dynamic field into the Spec, and shouldnt change whenever a contributor updates the internal KubernetesServiceSpec struct in this project

spwangxp · 2023-04-29T03:22:38Z

1, add some comment on KubernetesServiceSpec to notice that.
2, pls help check the below list, will use cmp.Equal(newSvc.Spec, oldSvc.Spec, cmpopts.IgnoreFields(ignore list)) to compare two svc.
ignore list:

Ports.NodePort
ClusterIP
ClusterIPs
ExternalIPs
SessionAffinity
LoadBalancerIP
LoadBalancerSourceRanges
ExternalTrafficPolicy
HealthCheckNodePort
SessionAffinityConfig
IPFamilies
IPFamilyPolicy
AllocateLoadBalancerNodePorts
LoadBalancerClass
InternalTrafficPolicy

check list:

Ports
Selector
Type
ExternalName
PublishNotReadyAddresses

The ignore list shouldnt change in the codebase, but should only only increase whenever upstream K8s API adds a dynamic field into the Spec, and shouldnt change whenever a contributor updates the internal KubernetesServiceSpec struct in this project

arkodg · 2023-05-01T20:46:02Z

afaik we should only ignore

Ports.NodePort
ClusterIP
ClusterIPs

since they are generated by K8s
LoadBalancerIP has been deprecated, so we can ignore it

Alice-Lilith · 2023-05-02T23:28:32Z

/hold can we compare the entire Spec here, and ignore any fields (NodePort) . This approach will be easier to maintain in the future

afaik we should only ignore
Ports.NodePort
ClusterIP
ClusterIPs
since they are generated by K8s LoadBalancerIP has been deprecated, so we can ignore it

+1 since this makes it easy to maintain / not need to remember this, otherwise lgtm

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

qicz · 2023-05-04T07:06:39Z

pls @Xunzhuo @zirain @arkodg approve the ci

Xunzhuo · 2023-05-04T07:07:16Z

done

Xunzhuo

/lgtm
/approve

Thanks for working on it @spwangxp

internal/infrastructure/kubernetes/resource/resource.go

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

qicz · 2023-05-05T14:49:18Z

ci needs to retrigger

arkodg

LGTM thanks for considering all the suggestions !

spwangxp requested a review from a team as a code owner April 23, 2023 07:02

zirain reviewed Apr 23, 2023

View reviewed changes

internal/infrastructure/kubernetes/applier/apply.go Outdated Show resolved Hide resolved

spwangxp force-pushed the bugfix-1340-update-svc-after-gateway-restart branch from d163cca to 1ddf9d3 Compare April 23, 2023 08:46

zirain reviewed Apr 23, 2023

View reviewed changes

internal/infrastructure/kubernetes/applier/apply.go Outdated Show resolved Hide resolved

spwangxp force-pushed the bugfix-1340-update-svc-after-gateway-restart branch 2 times, most recently from 7ea2c83 to 79b9cf8 Compare April 23, 2023 10:12

zirain reviewed Apr 24, 2023

View reviewed changes

internal/infrastructure/kubernetes/utils/utils_test.go Outdated Show resolved Hide resolved

spwangxp force-pushed the bugfix-1340-update-svc-after-gateway-restart branch from 79b9cf8 to 4cbb1a0 Compare April 24, 2023 01:43

zirain reviewed Apr 24, 2023

View reviewed changes

spwangxp force-pushed the bugfix-1340-update-svc-after-gateway-restart branch from 4cbb1a0 to f2595e9 Compare April 24, 2023 06:44

qicz mentioned this pull request Apr 24, 2023

refactor: followup kubernetes/infrastruture refactoring. #1337

Merged

arkodg reviewed Apr 24, 2023

View reviewed changes

spwangxp force-pushed the bugfix-1340-update-svc-after-gateway-restart branch from f2595e9 to aae2408 Compare April 26, 2023 06:00

qicz reviewed Apr 26, 2023

View reviewed changes

internal/infrastructure/kubernetes/applier/apply.go Outdated Show resolved Hide resolved

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

87488d9

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

spwangxp force-pushed the bugfix-1340-update-svc-after-gateway-restart branch from aae2408 to 87488d9 Compare April 27, 2023 02:23

qicz previously approved these changes Apr 27, 2023

View reviewed changes

Xunzhuo and others added 2 commits April 27, 2023 14:28

Merge branch 'main' into bugfix-1340-update-svc-after-gateway-restart

65c099b

lint code(internal/infrastructure/kubernetes)

2ac09c7

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

spwangxp dismissed qicz’s stale review via 2ac09c7 April 27, 2023 08:29

qicz previously approved these changes Apr 27, 2023

View reviewed changes

zirain changed the title ~~fix: #1340 not compare all svc.spec for user modified scene~~ fix: do not compare all svc.spec for user modified scene Apr 27, 2023

zirain previously approved these changes Apr 27, 2023

View reviewed changes

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

1500e2c

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

spwangxp dismissed stale reviews from zirain and qicz via 1500e2c April 28, 2023 02:44

arkodg previously approved these changes Apr 28, 2023

View reviewed changes

qicz previously approved these changes Apr 28, 2023

View reviewed changes

arkodg self-requested a review April 28, 2023 16:32

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

982272f

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

spwangxp dismissed stale reviews from qicz and arkodg via 982272f May 4, 2023 02:23

Xunzhuo enabled auto-merge (squash) May 4, 2023 07:07

Xunzhuo disabled auto-merge May 4, 2023 07:07

Merge branch 'main' into bugfix-1340-update-svc-after-gateway-restart

e115409

Xunzhuo previously approved these changes May 4, 2023

View reviewed changes

qicz reviewed May 4, 2023

View reviewed changes

internal/infrastructure/kubernetes/resource/resource.go Outdated Show resolved Hide resolved

update comment for CompareSvc

171b1ca

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

spwangxp dismissed Xunzhuo’s stale review via 171b1ca May 5, 2023 01:30

Merge branch 'main' into bugfix-1340-update-svc-after-gateway-restart

8f2695c

arkodg approved these changes May 5, 2023

View reviewed changes

arkodg merged commit ea2a688 into envoyproxy:main May 5, 2023

Conversation

spwangxp commented Apr 23, 2023

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

zirain left a comment

Choose a reason for hiding this comment

Uh oh!

qicz commented Apr 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg commented Apr 24, 2023

Uh oh!

arkodg commented Apr 24, 2023

Uh oh!

arkodg commented Apr 26, 2023

Uh oh!

Uh oh!

qicz left a comment

Choose a reason for hiding this comment

Uh oh!

Xunzhuo commented Apr 27, 2023

Uh oh!

zirain left a comment

Choose a reason for hiding this comment

Uh oh!

qicz left a comment

Choose a reason for hiding this comment

Uh oh!

arkodg commented Apr 28, 2023

Uh oh!

spwangxp commented Apr 29, 2023

Uh oh!

arkodg commented Apr 29, 2023

Uh oh!

spwangxp commented Apr 29, 2023

Uh oh!

arkodg commented Apr 29, 2023

Uh oh!

spwangxp commented Apr 29, 2023

Uh oh!

arkodg commented May 1, 2023

Uh oh!

Alice-Lilith commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qicz commented May 4, 2023

Uh oh!

Xunzhuo commented May 4, 2023

Uh oh!

Xunzhuo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qicz commented May 5, 2023

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 23, 2023 •

edited

Loading

Alice-Lilith commented May 2, 2023 •

edited

Loading