refactor ingress backend pod detection (#11)

Move the detection at the ingress level rather than per rule Goals --- - Prepare the case to support multiple ingress controllers through CRDs - Increase code readability and testeability Change-Id: I14936b3ef0558e5d5b5fd6f251264ddfa7b8ad65 Co-authored-by: Thibault Jamet <thibault.jamet@adevinta.com>
adevinta · Dec 13, 2024 · 8c217ad · 8c217ad
1 parent 559c285
commit 8c217ad
Show file tree

Hide file tree

Showing 3 changed files with 159 additions and 84 deletions.
diff --git a/pkg/controllers/ingress_controller.go b/pkg/controllers/ingress_controller.go
@@ -19,7 +19,6 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/source"
@@ -100,16 +99,6 @@ func (r *IngressReconciler) filterIngressRulesByHost(rules []netv1.IngressRule)
 	return rulesToBind
 }
 
-func (r *IngressReconciler) getTargetFromIngress(ingress netv1.Ingress) (string, error) {
-	if r.DevMode {
-		return "devmode", nil
-	}
-	if len(ingress.Status.LoadBalancer.Ingress) == 0 {
-		return "", fmt.Errorf("Ingress has no status")
-	}
-	return ingress.Status.LoadBalancer.Ingress[0].Hostname, nil
-}
-
 func (r *IngressReconciler) isIngressWeighted(ingress netv1.Ingress) bool {
 	return r.ingressHasAnnotationKey(ingress, r.annotationKey("traffic-weight"))
 }
@@ -168,7 +157,19 @@ func setGlobalHealthCheckID(endpoint *externaldnsk8siov1alpha1.Endpoint) {
 	}
 }
 
-func (r *IngressReconciler) newDnsEndpoint(ctx context.Context, dnsEndpoint *externaldnsk8siov1alpha1.DNSEndpoint, target string, ingress netv1.Ingress) {
+func (r *IngressReconciler) addIngressTargetsToEndpoint(endpoint *externaldnsk8siov1alpha1.Endpoint, ingress netv1.Ingress) {
+	for _, lb := range ingress.Status.LoadBalancer.Ingress {
+		if lb.Hostname != "" {
+			hostName := lb.Hostname
+			if r.DevMode {
+				hostName = "devmode"
+			}
+			endpoint.Targets = append(endpoint.Targets, hostName)
+		}
+	}
+}
+
+func (r *IngressReconciler) newDnsEndpoint(ctx context.Context, dnsEndpoint *externaldnsk8siov1alpha1.DNSEndpoint, ingress netv1.Ingress) {
 	var desiredWeight uint
 	var err error
 
@@ -188,21 +189,19 @@ func (r *IngressReconciler) newDnsEndpoint(ctx context.Context, dnsEndpoint *ext
 			dnsEndpoint.ObjectMeta.Annotations = make(map[string]string)
 		}
 	}
+
+	if !r.allIngressBackendsHavePods(ctx, &ingress) {
+		desiredWeight = 0
+	}
 	dnsEndpoint.Spec = externaldnsk8siov1alpha1.DNSEndpointSpec{Endpoints: []*externaldnsk8siov1alpha1.Endpoint{}}
 	for _, rule := range r.filterIngressRulesByHost(ingress.Spec.Rules) {
 
-		if !r.ingressRuleHasPods(ctx, ingress.ObjectMeta.Namespace, &rule) {
-			desiredWeight = 0
-		}
-
 		ep := &externaldnsk8siov1alpha1.Endpoint{
-			DNSName: rule.Host,
-			Targets: externaldnsk8siov1alpha1.Targets{
-				target,
-			},
+			DNSName:       rule.Host,
 			RecordType:    "CNAME",
 			SetIdentifier: r.ClusterName,
 		}
+		r.addIngressTargetsToEndpoint(ep, ingress)
 
 		setEndpointProviderSpecificProperty(ep, WeightProperty, strconv.FormatUint(uint64(desiredWeight), 10))
 		setGlobalHealthCheckID(ep)
@@ -218,12 +217,6 @@ func (r *IngressReconciler) reconcileDNSEntries(ctx context.Context, ingress net
 		return nil
 	}
 
-	target, err := r.getTargetFromIngress(ingress)
-	if err != nil {
-		log.Info("Ingress object doesn't have target assigned. Skipping")
-		return nil
-	}
-
 	// Reconcile uses this property that an ingress has a single matching dnsendpoint
 	// with the same name. Shall this be changed, we should also change the Reconcile code
 	var dnsEndpoint = &externaldnsk8siov1alpha1.DNSEndpoint{
@@ -232,11 +225,10 @@ func (r *IngressReconciler) reconcileDNSEntries(ctx context.Context, ingress net
 			Namespace: ingress.GetNamespace(),
 		},
 	}
-	var f controllerutil.MutateFn = func() error {
-		r.newDnsEndpoint(ctx, dnsEndpoint, target, ingress)
+	_, err := ctrl.CreateOrUpdate(ctx, r.Client, dnsEndpoint, func() error {
+		r.newDnsEndpoint(ctx, dnsEndpoint, ingress)
 		return nil
-	}
-	_, err = ctrl.CreateOrUpdate(ctx, r.Client, dnsEndpoint, f)
+	})
 	return err
 }
 
@@ -255,23 +247,23 @@ func (r *IngressReconciler) endpointBeingDeleted(ctx context.Context, obj types.
 in DNSEndpoint Object we set its weight based on hostname level. in case a host in an ingress has more than 1 service for different paths,
 we will only set the weight to 0 if all services do not have backing pods.
 */
-func (r *IngressReconciler) ingressRuleHasPods(ctx context.Context, namespace string, rule *netv1.IngressRule) bool {
-
-	// there are some edge cases that the rules does not have HTTP property defined
-	// keeping the same behavior
-	if rule.HTTP == nil {
-		return false
-	}
-	paths := rule.HTTP.Paths
-
+func (r *IngressReconciler) allIngressBackendsHavePods(ctx context.Context, ingress *netv1.Ingress) bool {
 	var servicesName = make(map[string]types.NamespacedName)
 
-	for _, path := range paths {
-		servicesName[path.Backend.Service.Name] = types.NamespacedName{Namespace: namespace, Name: path.Backend.Service.Name}
+	for _, rule := range ingress.Spec.Rules {
+		// there are some edge cases that the rules does not have HTTP property defined
+		// keeping the same behavior
+		if rule.HTTP == nil {
+			continue
+		}
+		paths := rule.HTTP.Paths
+
+		for _, path := range paths {
+			servicesName[path.Backend.Service.Name] = types.NamespacedName{Namespace: ingress.Namespace, Name: path.Backend.Service.Name}
+		}
 	}
 
 	for _, svc := range servicesName {
-
 		var endpoints v1.Endpoints
 
 		if err := r.Get(ctx, svc, &endpoints); err != nil {
@@ -281,12 +273,6 @@ func (r *IngressReconciler) ingressRuleHasPods(ctx context.Context, namespace st
 			if apierrors.IsNotFound(err) {
 				return false
 			}
-			return true
-		}
-
-		if !endpoints.DeletionTimestamp.IsZero() {
-			// If the endpoint is being deleted, it will eventually disappear, and ingresses will not have pods pretty soon.
-			// Handling this case here helps reduce downtime in some cases
 			return false
 		}
 
@@ -299,6 +285,11 @@ func (r *IngressReconciler) ingressRuleHasPods(ctx context.Context, namespace st
 }
 
 func (r *IngressReconciler) endpointsHasPods(endpoints *v1.Endpoints) bool {
+	if !endpoints.DeletionTimestamp.IsZero() {
+		// If the endpoint is being deleted, it will eventually disappear, and ingresses will not have pods pretty soon.
+		// Handling this case here helps reduce downtime in some cases
+		return false
+	}
 	if len(endpoints.Subsets) == 0 {
 		return false
 	}

diff --git a/pkg/controllers/ingress_controller_internal_test.go b/pkg/controllers/ingress_controller_internal_test.go
@@ -122,39 +122,6 @@ func TestFilterIngressRulesByHost(t *testing.T) {
 	})
 }
 
-func TestGetTargetFromIngress(t *testing.T) {
-	t.Run("with dev mode, devmode should be returned", func(t *testing.T) {
-		reconciler := IngressReconciler{
-			DevMode: true,
-		}
-		target, err := reconciler.getTargetFromIngress(netv1.Ingress{})
-		assert.NoError(t, err)
-		assert.Equal(t, "devmode", target)
-	})
-	t.Run("with a Hostname target, the host name should be returned", func(t *testing.T) {
-		reconciler := IngressReconciler{}
-		target, err := reconciler.getTargetFromIngress(netv1.Ingress{
-			Status: netv1.IngressStatus{
-				LoadBalancer: netv1.IngressLoadBalancerStatus{
-					Ingress: []netv1.IngressLoadBalancerIngress{
-						{
-							Hostname: "hello.world",
-						},
-					},
-				},
-			},
-		})
-		assert.NoError(t, err)
-		assert.Equal(t, "hello.world", target)
-	})
-	t.Run("no target, an error should be returned", func(t *testing.T) {
-		reconciler := IngressReconciler{}
-		target, err := reconciler.getTargetFromIngress(netv1.Ingress{})
-		assert.Error(t, err)
-		assert.Equal(t, "", target)
-	})
-}
-
 func TestReconcileIngressShouldCreateDNSEndpointsWithCorrectWeight(t *testing.T) {
 	extendedScheme := NewScheme()
 

diff --git a/pkg/controllers/ingress_controller_test.go b/pkg/controllers/ingress_controller_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	v1 "k8s.io/api/core/v1"
 	netv1 "k8s.io/api/networking/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -31,6 +32,9 @@ func TestIngressController(t *testing.T) {
 	testutils.IntegrationTest(t)
 	scheme := NewScheme()
 
+	trafficweight.Store.AWSHealthCheckID = ""
+	trafficweight.Store.DesiredWeight = 0
+
 	kubeconfig, err := k8s.NewClientConfigBuilder().WithKubeConfigPath(testEnvConf.KubeconfigFile()).Build()
 	require.NoError(t, err)
 
@@ -127,7 +131,7 @@ func TestIngressController(t *testing.T) {
 
 	t.Run("Should forge DNS Endpoints", func(t *testing.T) {
 		forged := &externaldnsk8siov1alpha1.DNSEndpoint{}
-		reconciler.newDnsEndpoint(context.Background(), forged, "bar-celona", ing)
+		reconciler.newDnsEndpoint(context.Background(), forged, ing)
 		assert.Equal(t, expected, *forged)
 	})
 
@@ -191,7 +195,7 @@ func TestIngressController(t *testing.T) {
 	t.Run("if we dont set --aws-health-check-id ingress shouldnt have health property", func(t *testing.T) {
 		trafficweight.Store.AWSHealthCheckID = ""
 		forged := &externaldnsk8siov1alpha1.DNSEndpoint{}
-		reconciler.newDnsEndpoint(context.Background(), forged, "bar-celona", ing)
+		reconciler.newDnsEndpoint(context.Background(), forged, ing)
 		assert.Equal(t, expected, *forged)
 	})
 
@@ -226,7 +230,7 @@ func TestIngressController(t *testing.T) {
 		}
 
 		forged := &externaldnsk8siov1alpha1.DNSEndpoint{}
-		reconciler.newDnsEndpoint(context.Background(), forged, "bar-celona", ing)
+		reconciler.newDnsEndpoint(context.Background(), forged, ing)
 		assert.Equal(t, expected, *forged)
 		ing.Spec.Rules = oldRules
 	})
@@ -378,3 +382,116 @@ func TestSetGlobalHealthcheckID(t *testing.T) {
 		assert.Len(t, ep.Spec.Endpoints[0].ProviderSpecific, 0)
 	})
 }
+
+func TestAddIngressTargets(t *testing.T) {
+	t.Run("When not in devmode", func(t *testing.T) {
+		ingress := netv1.Ingress{
+			Status: netv1.IngressStatus{
+				LoadBalancer: netv1.IngressLoadBalancerStatus{
+					Ingress: []netv1.IngressLoadBalancerIngress{
+						{Hostname: "test-hostname"},
+						{Hostname: "test-hostname-2"},
+					},
+				},
+			},
+		}
+		ep := externaldnsk8siov1alpha1.Endpoint{}
+
+		r := IngressReconciler{}
+
+		r.addIngressTargetsToEndpoint(&ep, ingress)
+
+		require.Len(t, ep.Targets, 2)
+		assert.Contains(t, ep.Targets, "test-hostname")
+		assert.Contains(t, ep.Targets, "test-hostname-2")
+	})
+	t.Run("When in devmode", func(t *testing.T) {
+		ingress := netv1.Ingress{
+			Status: netv1.IngressStatus{
+				LoadBalancer: netv1.IngressLoadBalancerStatus{
+					Ingress: []netv1.IngressLoadBalancerIngress{
+						{Hostname: "test-hostname"},
+						{Hostname: "test-hostname-2"},
+					},
+				},
+			},
+		}
+		ep := externaldnsk8siov1alpha1.Endpoint{}
+
+		r := IngressReconciler{DevMode: true}
+
+		r.addIngressTargetsToEndpoint(&ep, ingress)
+
+		require.Len(t, ep.Targets, 2)
+		assert.Contains(t, ep.Targets, "devmode")
+		assert.Contains(t, ep.Targets, "devmode")
+	})
+}
+
+func TestAllIngressBackendsHavePods(t *testing.T) {
+	t.Run("When all backends have pods", func(t *testing.T) {
+		ingress := MockIngress(
+			WithObjectNamespace[*netv1.Ingress]("test-namespace"),
+			IngressWithRules(
+				NewRule(RuleWithHTTPPaths(
+					NewHTTPIngressPath(PathWithBackendServiceName("rule-1-service-1")), NewHTTPIngressPath(PathWithBackendServiceName("rule-1-service-2")),
+				)),
+				NewRule(RuleWithHTTPPaths(
+					NewHTTPIngressPath(PathWithBackendServiceName("rule-2-service-1")),
+				)),
+			),
+		)
+		client := fake.NewClientBuilder().WithObjects(
+			MockEndpoints(EndpointsWithName("rule-1-service-1"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithSubsets(NewSubsetWithAddressIPs("1.1.1.1"))),
+			MockEndpoints(EndpointsWithName("rule-1-service-2"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithSubsets(NewSubsetWithAddressIPs("1.1.1.2"))),
+			MockEndpoints(EndpointsWithName("rule-2-service-1"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithSubsets(NewSubsetWithAddressIPs("1.1.1.3"))),
+		).Build()
+		r := IngressReconciler{Client: client}
+		assert.True(t, r.allIngressBackendsHavePods(context.Background(), ingress))
+	})
+	t.Run("When a backend does not have pods", func(t *testing.T) {
+		ingress := MockIngress(
+			WithObjectNamespace[*netv1.Ingress]("test-namespace"),
+			IngressWithRules(
+				NewRule(RuleWithHTTPPaths(
+					NewHTTPIngressPath(PathWithBackendServiceName("rule-1-service-1")), NewHTTPIngressPath(PathWithBackendServiceName("rule-1-service-2")),
+				)),
+				NewRule(RuleWithHTTPPaths(
+					NewHTTPIngressPath(PathWithBackendServiceName("rule-2-service-1")),
+				)),
+			),
+		)
+		client := fake.NewClientBuilder().WithObjects(
+			MockEndpoints(EndpointsWithName("rule-1-service-1"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithSubsets(NewSubsetWithAddressIPs("1.1.1.1"))),
+			MockEndpoints(EndpointsWithName("rule-1-service-2"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithoutSubset()),
+			MockEndpoints(EndpointsWithName("rule-2-service-1"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithSubsets(NewSubsetWithAddressIPs("1.1.1.3"))),
+		).Build()
+		r := IngressReconciler{Client: client}
+		assert.False(t, r.allIngressBackendsHavePods(context.Background(), ingress))
+	})
+	t.Run("When a backend service is missing", func(t *testing.T) {
+		ingress := MockIngress(
+			WithObjectNamespace[*netv1.Ingress]("test-namespace"),
+			IngressWithRules(
+				NewRule(RuleWithHTTPPaths(
+					NewHTTPIngressPath(PathWithBackendServiceName("rule-1-service-1")), NewHTTPIngressPath(PathWithBackendServiceName("rule-1-service-2")),
+				)),
+			),
+		)
+		client := fake.NewClientBuilder().WithObjects(
+			MockEndpoints(EndpointsWithName("rule-1-service-1"), WithObjectNamespace[*v1.Endpoints]("test-namespace"), EndpointsWithSubsets(NewSubsetWithAddressIPs("1.1.1.1"))),
+		).Build()
+		r := IngressReconciler{Client: client}
+		assert.False(t, r.allIngressBackendsHavePods(context.Background(), ingress))
+	})
+}
+
+func TestEndpointsHasPods(t *testing.T) {
+	r := IngressReconciler{}
+
+	assert.True(t, r.endpointsHasPods(MockEndpoints()))
+	assert.False(t, r.endpointsHasPods(MockEndpoints(EndpointsWithSubsets(NewSubsetWithAddressIPs()))))
+	assert.False(t, r.endpointsHasPods(MockEndpoints(EndpointsWithoutSubset())))
+
+	assert.False(t, r.endpointsHasPods(MockEndpoints(WithObjectFinalizers[*v1.Endpoints]("test.adevinta.com"), WithObjectDeletionTimestamp[*v1.Endpoints](metav1.Now()))))
+}