Allow non-IPs in service spec to avoid noop updates #5663
Conversation
There was a problem hiding this comment.
It is unclear to me why we made this exclusion originally.
Same 🤷♂️
Doing some tests I noticed that InternalTrafficPolicy is set by the api server starting K8S 1.22 (documentation states that it is beta since v1.23 but I can clearly see it enabled by default on v1.22.9-gke.1300):
v1.ServiceSpec{
... // 16 identical fields
AllocateLoadBalancerNodePorts: nil,
LoadBalancerClass: nil,
- InternalTrafficPolicy: nil,
+ InternalTrafficPolicy: &"Cluster",
}
Co-authored-by: Michael Morello <[email protected]>
| @@ -285,7 +288,7 @@ func Test_applyServerSideValues(t *testing.T) { | |||
| }}, | |||
| }, | |||
| { | |||
| name: "Reconciled ClusterIP[s] is not used if the reconciled ClusterIP[s] are not valid IPs", | |||
| name: "Reconciled ClusterIP[s] is also used if the reconciled ClusterIP[s] are not valid IPs", | |||
There was a problem hiding this comment.
Not sure what we are testing now with this case. It's equivalent to the previous case 'Reconciled ClusterIP/ClusterIPs/Type/SessionAffinity is used if expected ClusterIP/Type/SessionAffinity is empty'.
There was a problem hiding this comment.
Not quite because this test would have failed in the previous implementation because None is not a valid IP address. This test documents this new behaviour.
Co-authored-by: Thibault Richard <[email protected]>
We are copying parts of the service spec populated on the service side into the expected service spec to avoid unnecessary updates of services. For some reason that is unfortunately not documented an exception was made for the ClusterIP[s] values that not valid IPs like None. By not including those we always update the service on each reconciliation. This change addresses this gap and a similar one for internalTrafficPolicy Co-authored-by: Michael Morello <[email protected]> Co-authored-by: Thibault Richard <[email protected]>
Fixes #5657
We are copying parts of the service spec populated on the service side into the
expectedservice spec to avoid unnecessary updates of services. For some reason that is unfortunately not documented an exception was made for theClusterIP[s]values that not valid IPs likeNone.By not including those we always update the service on each reconciliation.
Prior relevant work on in this area:
#2691
#4929
I am still investigatingIt is unclear to me why we made this exclusion originally. I have not been able to create a situation where this became a problem. It seemsNoneis perfectly acceptable to create a headless service, as expected.