Commit 35860222 authored by Miciah Masters's avatar Miciah Masters Committed by Miciah Dashiel Butler Masters
Browse files

Set the "local-with-fallback" service annotation

Set the "traffic-policy.network.alpha.openshift.io/local-with-fallback"
annotation on LoadBalancer- and NodePort-type services that use the "Local"
external traffic policy.

This commit is related to bug 1960284.

https://bugzilla.redhat.com/show_bug.cgi?id=1960284

* pkg/operator/controller/ingress/load_balancer_service.go
(localWithFallbackAnnotation): New const.  Define the annotation key for
the "local-with-fallback" service annotation.
(desiredLoadBalancerService): Set the "local-with-fallback" annotation when
using the "Local" external traffic policy.
(managedLoadBalancerServiceAnnotations): New variable.  Define the keys for
annotations that the operator manages on LoadBalancer-type services, which
now include the "local-with-fallback" annotation.
(loadBalancerServiceChanged): Fix the update logic to properly handle
annotations with empty values using go-cmp and
managedLoadBalancerServiceAnnotations.
* pkg/operator/controller/ingress/load_balancer_service_test.go
(TestDesiredLoadBalancerService): Verify that the "local-with-fallback"
annotation is set when appropriate.
(TestLoadBalancerServiceChanged): Verify that loadBalancerServiceChanged
properly handles updates to the "local-with-fallback" annotation.
* pkg/operator/controller/ingress/nodeport_service.go
(desiredNodePortService): Set the "local-with-fallback" annotation.
(managedNodePortServiceAnnotations): New variable.  Define the annotation
keys that the operator manages on NodePort-type services.
(nodePortServiceChanged): Check if any of the annotations in
managedNodePortServiceAnnotations have changed.  Update managed annotations
if they have changed.
* pkg/operator/controller/ingress/nodeport_service_test.go
(TestDesiredNodePortService): Verify that the expected annotations are set.
(TestNodePortServiceChanged): Add a test case to verify that
nodePortServiceChanged properly handles updates to the
"local-with-fallback" annotation.
parent 2a9e3c24
......@@ -21,6 +21,7 @@ import (
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"
crclient "sigs.k8s.io/controller-runtime/pkg/client"
)
......@@ -104,6 +105,12 @@ const (
// openstackInternalLBAnnotation is the annotation used on a service to specify an
// OpenStack load balancer as being internal.
openstackInternalLBAnnotation = "service.beta.kubernetes.io/openstack-internal-load-balancer"
// localWithFallbackAnnotation is the annotation used on a service that
// has "Local" external traffic policy to indicate that the service
// proxy should prefer using a local endpoint but forward traffic to any
// available endpoint if no local endpoint is available.
localWithFallbackAnnotation = "traffic-policy.network.alpha.openshift.io/local-with-fallback"
)
var (
......@@ -298,6 +305,10 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef
}
// Azure load balancers are not customizable and are set to (2 fail @ 5s interval, 2 healthy)
// GCP load balancers are not customizable and are set to (3 fail @ 8s interval, 1 healthy)
if service.Spec.ExternalTrafficPolicy == corev1.ServiceExternalTrafficPolicyTypeLocal {
service.Annotations[localWithFallbackAnnotation] = ""
}
}
service.SetOwnerReferences([]metav1.OwnerReference{deploymentRef})
......@@ -394,14 +405,31 @@ func (r *reconciler) updateLoadBalancerService(current, desired *corev1.Service,
return true, nil
}
// managedLoadBalancerServiceAnnotations is a set of annotation keys for
// annotations that the operator manages for LoadBalancer-type services.
var managedLoadBalancerServiceAnnotations = sets.NewString(
awsLBHealthCheckIntervalAnnotation,
GCPGlobalAccessAnnotation,
localWithFallbackAnnotation,
)
// loadBalancerServiceChanged checks if the current load balancer service
// matches the expected and if not returns an updated one.
func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev1.Service) {
annotationCmpOpts := []cmp.Option{
cmpopts.IgnoreMapEntries(func(k, _ string) bool {
return !managedLoadBalancerServiceAnnotations.Has(k)
}),
}
if cmp.Equal(current.Annotations, expected.Annotations, annotationCmpOpts...) {
return false, nil
}
updated := current.DeepCopy()
changed := false
// Preserve everything but the AWS LB health check interval annotation &
// GCP Global Access internal Load Balancer annotation.
// Preserve everything but the AWS LB health check interval annotation,
// GCP Global Access internal Load Balancer annotation, and
// local-with-fallback annotation
// (see <https://bugzilla.redhat.com/show_bug.cgi?id=1908758>).
// Updating annotations and spec fields cannot be done unless the
// previous release blocks upgrades when the user has modified those
......@@ -409,18 +437,15 @@ func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev
if updated.Annotations == nil {
updated.Annotations = map[string]string{}
}
if current.Annotations[awsLBHealthCheckIntervalAnnotation] != expected.Annotations[awsLBHealthCheckIntervalAnnotation] {
updated.Annotations[awsLBHealthCheckIntervalAnnotation] = expected.Annotations[awsLBHealthCheckIntervalAnnotation]
changed = true
}
if current.Annotations[GCPGlobalAccessAnnotation] != expected.Annotations[GCPGlobalAccessAnnotation] {
updated.Annotations[GCPGlobalAccessAnnotation] = expected.Annotations[GCPGlobalAccessAnnotation]
changed = true
}
if !changed {
return false, nil
for annotation := range managedLoadBalancerServiceAnnotations {
currentVal, have := current.Annotations[annotation]
expectedVal, want := expected.Annotations[annotation]
if want && (!have || currentVal != expectedVal) {
updated.Annotations[annotation] = expected.Annotations[annotation]
} else if have && !want {
delete(updated.Annotations, annotation)
}
}
return true, updated
......
......@@ -319,6 +319,9 @@ func TestDesiredLoadBalancerService(t *testing.T) {
if err := checkServiceHasAnnotation(svc, awsLBHealthCheckHealthyThresholdAnnotation, true, awsLBHealthCheckHealthyThresholdDefault); err != nil {
t.Errorf("annotation check for test %q failed: %v", tc.description, err)
}
if err := checkServiceHasAnnotation(svc, localWithFallbackAnnotation, true, ""); err != nil {
t.Errorf("local-with-fallback annotation check for test %q failed: %v", tc.description, err)
}
classicLB := tc.lbStrategy.ProviderParameters == nil || tc.lbStrategy.ProviderParameters.AWS.Type == operatorv1.AWSClassicLoadBalancer
switch {
case classicLB:
......@@ -388,6 +391,9 @@ func TestDesiredLoadBalancerService(t *testing.T) {
t.Errorf("annotation check for test %q failed; unexpected annotation %s", tc.description, azureInternalLBAnnotation)
}
}
if err := checkServiceHasAnnotation(svc, localWithFallbackAnnotation, true, ""); err != nil {
t.Errorf("local-with-fallback annotation check for test %q failed: %v", tc.description, err)
}
case configv1.GCPPlatformType:
if isInternal {
if err := checkServiceHasAnnotation(svc, gcpLBTypeAnnotation, true, "Internal"); err != nil {
......@@ -399,6 +405,9 @@ func TestDesiredLoadBalancerService(t *testing.T) {
t.Errorf("annotation check for test %q failed; unexpected annotation %s", tc.description, gcpLBTypeAnnotation)
}
}
if err := checkServiceHasAnnotation(svc, localWithFallbackAnnotation, true, ""); err != nil {
t.Errorf("local-with-fallback annotation check for test %q failed: %v", tc.description, err)
}
case configv1.OpenStackPlatformType:
if isInternal {
if err := checkServiceHasAnnotation(svc, openstackInternalLBAnnotation, true, "true"); err != nil {
......@@ -410,6 +419,9 @@ func TestDesiredLoadBalancerService(t *testing.T) {
t.Errorf("annotation check for test %q failed; unexpected annotation %s", tc.description, openstackInternalLBAnnotation)
}
}
if err := checkServiceHasAnnotation(svc, localWithFallbackAnnotation, true, ""); err != nil {
t.Errorf("local-with-fallback annotation check for test %q failed: %v", tc.description, err)
}
}
}
}
......@@ -478,6 +490,13 @@ func TestLoadBalancerServiceChanged(t *testing.T) {
},
expect: false,
},
{
description: "if the local-with-fallback annotation is added",
mutate: func(svc *corev1.Service) {
svc.Annotations[localWithFallbackAnnotation] = ""
},
expect: true,
},
{
description: "if .spec.healthCheckNodePort changes",
mutate: func(svc *corev1.Service) {
......@@ -548,6 +567,13 @@ func TestLoadBalancerServiceChanged(t *testing.T) {
},
expect: true,
},
{
description: "if the service.beta.kubernetes.io/aws-load-balancer-healthcheck-interval annotation is deleted",
mutate: func(svc *corev1.Service) {
delete(svc.Annotations, "service.beta.kubernetes.io/aws-load-balancer-healthcheck-interval")
},
expect: true,
},
}
for _, tc := range testCases {
......
......@@ -15,6 +15,7 @@ import (
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/intstr"
)
......@@ -87,6 +88,9 @@ func desiredNodePortService(ic *operatorv1.IngressController, deploymentRef meta
name := controller.NodePortServiceName(ic)
service := &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
localWithFallbackAnnotation: "",
},
Namespace: name.Namespace,
Name: name.Name,
Labels: map[string]string{
......@@ -160,9 +164,17 @@ func (r *reconciler) updateNodePortService(current, desired *corev1.Service) (bo
return true, nil
}
// managedNodePortServiceAnnotations is a set of annotation keys for annotations
// that the operator manages for NodePort-type services.
var managedNodePortServiceAnnotations = sets.NewString(
localWithFallbackAnnotation,
)
// nodePortServiceChanged checks if the current NodePort service spec matches
// the expected spec and if not returns an updated one.
func nodePortServiceChanged(current, expected *corev1.Service) (bool, *corev1.Service) {
changed := false
serviceCmpOpts := []cmp.Option{
// Ignore fields that the API, other controllers, or user may
// have modified.
......@@ -171,13 +183,38 @@ func nodePortServiceChanged(current, expected *corev1.Service) (bool, *corev1.Se
cmp.Comparer(cmpServiceAffinity),
cmpopts.EquateEmpty(),
}
if cmp.Equal(current.Spec, expected.Spec, serviceCmpOpts...) {
if !cmp.Equal(current.Spec, expected.Spec, serviceCmpOpts...) {
changed = true
}
annotationCmpOpts := []cmp.Option{
cmpopts.IgnoreMapEntries(func(k, _ string) bool {
return !managedNodePortServiceAnnotations.Has(k)
}),
}
if !cmp.Equal(current.Annotations, expected.Annotations, annotationCmpOpts...) {
changed = true
}
if !changed {
return false, nil
}
updated := current.DeepCopy()
updated.Spec = expected.Spec
if updated.Annotations == nil {
updated.Annotations = map[string]string{}
}
for annotation := range managedNodePortServiceAnnotations {
currentVal, have := current.Annotations[annotation]
expectedVal, want := expected.Annotations[annotation]
if want && (!have || currentVal != expectedVal) {
updated.Annotations[annotation] = expected.Annotations[annotation]
} else if have && !want {
delete(updated.Annotations, annotation)
}
}
// Preserve fields that the API, other controllers, or user may have
// modified.
updated.Spec.ClusterIP = current.Spec.ClusterIP
......
......@@ -37,6 +37,9 @@ func TestDesiredNodePortService(t *testing.T) {
expect: true,
expectService: corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
localWithFallbackAnnotation: "",
},
Namespace: "openshift-ingress",
Name: "router-nodeport-default",
Labels: map[string]string{
......@@ -75,6 +78,9 @@ func TestDesiredNodePortService(t *testing.T) {
expect: true,
expectService: corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
localWithFallbackAnnotation: "",
},
Namespace: "openshift-ingress",
Name: "router-nodeport-default",
Labels: map[string]string{
......@@ -175,6 +181,20 @@ func TestNodePortServiceChanged(t *testing.T) {
},
expect: true,
},
{
description: "if the local-with-fallback annotation changes",
mutate: func(svc *corev1.Service) {
svc.Annotations["traffic-policy.network.alpha.openshift.io/local-with-fallback"] = "x"
},
expect: true,
},
{
description: "if the local-with-fallback annotation is deleted",
mutate: func(svc *corev1.Service) {
delete(svc.Annotations, "traffic-policy.network.alpha.openshift.io/local-with-fallback")
},
expect: true,
},
{
description: "if .spec.healthCheckNodePort changes",
mutate: func(svc *corev1.Service) {
......@@ -236,6 +256,9 @@ func TestNodePortServiceChanged(t *testing.T) {
for _, tc := range testCases {
original := corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
"traffic-policy.network.alpha.openshift.io/local-with-fallback": "",
},
Namespace: "openshift-ingress",
Name: "router-original",
UID: "1",
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment