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

Allow non-IPs in service spec to avoid noop updates #5663

Merged
merged 6 commits into from
May 25, 2022
Merged

Allow non-IPs in service spec to avoid noop updates #5663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
373522f
Allow non-IPs in service spec to avoid noop updates
pebrc May 12, 2022
27ba67a
Update pkg/controller/common/service_control.go
pebrc May 24, 2022
6bdeaea
add unit test for internalTrafficPolicy
pebrc May 24, 2022
501358c
remove lint
pebrc May 25, 2022
7b0314b
Apply suggestions from code review
pebrc May 25, 2022
42728e6
review input
pebrc May 25, 2022
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
30 changes: 11 additions & 19 deletions pkg/controller/common/service_control.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package common

import (
"context"
"net"
"reflect"

"go.elastic.co/apm"
Expand Down Expand Up @@ -90,20 +89,17 @@ func applyServerSideValues(expected, reconciled *corev1.Service) {
if expected.Spec.Type == "" {
expected.Spec.Type = reconciled.Spec.Type
}
// ClusterIP might not exist in the expected service,
// ClusterIPs might not exist in the expected service,
// but might have been set after creation by k8s on the actual resource.
// In such case, we want to use these values for comparison.
// But only if we are not changing the type of service and the api server has assigned an IP
if expected.Spec.Type == reconciled.Spec.Type && expected.Spec.ClusterIP == "" && net.ParseIP(reconciled.Spec.ClusterIP) != nil {
expected.Spec.ClusterIP = reconciled.Spec.ClusterIP
}

// ClusterIPs also might not exist in the expected service,
// but might have been set after creation by k8s on the actual resource.
// In such case, we want to use these values for comparison.
// But only if we are not changing the type of service and the api server has assigned IPs
if expected.Spec.Type == reconciled.Spec.Type && len(expected.Spec.ClusterIPs) == 0 && validClusterIPs(reconciled.Spec.ClusterIPs) {
expected.Spec.ClusterIPs = reconciled.Spec.ClusterIPs
if expected.Spec.Type == reconciled.Spec.Type {
if expected.Spec.ClusterIP == "" {
expected.Spec.ClusterIP = reconciled.Spec.ClusterIP
}
if len(expected.Spec.ClusterIPs) == 0 {
expected.Spec.ClusterIPs = reconciled.Spec.ClusterIPs
}
}

// SessionAffinity may be defaulted by the api server
Expand Down Expand Up @@ -140,15 +136,11 @@ func applyServerSideValues(expected, reconciled *corev1.Service) {
if expected.Spec.IPFamilyPolicy == nil {
expected.Spec.IPFamilyPolicy = reconciled.Spec.IPFamilyPolicy
}
}

func validClusterIPs(clusterIPs []string) bool {
for _, ip := range clusterIPs {
if net.ParseIP(ip) == nil {
return false
}
// InternalTrafficPolicy may be defaulted by the api server starting K8S v1.22
if expected.Spec.InternalTrafficPolicy == nil {
expected.Spec.InternalTrafficPolicy = reconciled.Spec.InternalTrafficPolicy
}
return true
}

// hasNodePort returns for a given service type, if the service ports have a NodePort or not.
Expand Down
33 changes: 32 additions & 1 deletion pkg/controller/common/service_control_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,9 @@ func Test_needsDelete(t *testing.T) {
}

func Test_applyServerSideValues(t *testing.T) {
ptr := func(policyType corev1.ServiceInternalTrafficPolicyType) *corev1.ServiceInternalTrafficPolicyType {
return &policyType
}
type args struct {
expected corev1.Service
reconciled corev1.Service
Expand Down Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

args: args{
expected: corev1.Service{Spec: corev1.ServiceSpec{}},
reconciled: corev1.Service{Spec: corev1.ServiceSpec{
Expand All @@ -297,6 +300,8 @@ func Test_applyServerSideValues(t *testing.T) {
},
want: corev1.Service{Spec: corev1.ServiceSpec{
Type: corev1.ServiceTypeClusterIP,
ClusterIP: "None",
ClusterIPs: []string{"None"},
SessionAffinity: corev1.ServiceAffinityClientIP,
}},
},
Expand Down Expand Up @@ -515,6 +520,32 @@ func Test_applyServerSideValues(t *testing.T) {
IPFamilies: []corev1.IPFamily{corev1.IPv6Protocol},
}},
},
{
name: "Reconciled InternalTrafficPolicy is used if the expected one is empty",
args: args{
expected: corev1.Service{Spec: corev1.ServiceSpec{}},
reconciled: corev1.Service{Spec: corev1.ServiceSpec{
InternalTrafficPolicy: ptr(corev1.ServiceInternalTrafficPolicyCluster),
}},
},
want: corev1.Service{Spec: corev1.ServiceSpec{
InternalTrafficPolicy: ptr(corev1.ServiceInternalTrafficPolicyCluster),
}},
},
{
name: "Expected InternalTrafficPolicy is used if not empty",
args: args{
expected: corev1.Service{Spec: corev1.ServiceSpec{
InternalTrafficPolicy: ptr(corev1.ServiceInternalTrafficPolicyLocal),
}},
reconciled: corev1.Service{Spec: corev1.ServiceSpec{
InternalTrafficPolicy: ptr(corev1.ServiceInternalTrafficPolicyCluster),
}},
},
want: corev1.Service{Spec: corev1.ServiceSpec{
InternalTrafficPolicy: ptr(corev1.ServiceInternalTrafficPolicyLocal),
}},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down