operator: Fix logic used to sync Cilium's IngressClass on startup

This commit introduces changes to the ingress class manager piece of the
ingress controller, in order to address bugs impacting the proper
synching of Cilium's IngressClass during start up. The following changes
are made:

* Replace use of an Informer with Resource[T] for IngressClass. This
  helps simplify the logic used to perform the initial sync.
* Move the responsibility of tracking if Cilium should act as the default
  IngressClass into the ingress class manager, rather than having the
  ingress controller track this itself when processing IngressClass
  events.

After the ingress class manager is constructed, the ingress controller
will be able to determine if Cilium is the default IngressClass for a
cluster through the ingress class manager. The ingress controller no
longer was to wait to process an event for Cilium's IngressClass to
learn if Cilium should be the default.

Before this commit, the ingress controller would process all Ingress
resources before processing IngressClass resources. This is because the
Ingress resource informer would be started before the ingress class
manager, so all events related to Ingress resources would appear in the
ingress controller's event queue before events relating to IngressClass
resources. This presented a problem, because the ingress controller
would always believe that it was not the default IngressClass for a
cluster on startup while processing each Ingress resource for the first
time. This could lead to the following situation:

1. The ingress controller processes all Ingress resources.
2. The ingress controller processes IngressClass resources, and learns
   that it should act as the default IngressClass for the cluster.
3. A resync of Ingress resources is triggered.

This double-sync overhead can act as a problem for large-scale clusters.

Signed-off-by: Ryan Drew <ryan.drew@isovalent.com>

operator/cmd/root.go

@@ -714,6 +714,7 @@ func (legacy *legacyOnLeader) onStart(_ hive.HookContext) error {
 	if operatorOption.Config.EnableIngressController {
 		ingressController, err := ingress.NewController(
 			legacy.clientset,
+			legacy.resources.IngressClasses,
 			ingress.WithHTTPSEnforced(operatorOption.Config.EnforceIngressHTTPS),
 			ingress.WithSecretsSyncEnabled(operatorOption.Config.EnableIngressSecretsSync),
 			ingress.WithSecretsNamespace(operatorOption.Config.IngressSecretsNamespace),

operator/k8s/resource_ctors.go

@@ -16,6 +16,7 @@ import (
 	cilium_api_v2alpha1 "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2alpha1"
 	"github.com/cilium/cilium/pkg/k8s/client"
 	"github.com/cilium/cilium/pkg/k8s/resource"
+	slim_networkingv1 "github.com/cilium/cilium/pkg/k8s/slim/k8s/api/networking/v1"
 	"github.com/cilium/cilium/pkg/k8s/utils"
 )
 
@@ -57,3 +58,13 @@ func CiliumEndpointSliceResource(lc hive.Lifecycle, cs client.Clientset, opts ..
 	)
 	return resource.New[*cilium_api_v2alpha1.CiliumEndpointSlice](lc, lw, resource.WithMetric("CiliumEndpointSlice")), nil
 }
+
+func IngressClassResource(lc hive.Lifecycle, cs client.Clientset, opts ...func(*metav1.ListOptions)) (resource.Resource[*slim_networkingv1.IngressClass], error) {
+	if !cs.IsEnabled() {
+		return nil, nil
+	}
+	lw := utils.ListerWatcherWithModifiers(
+		utils.ListerWatcherFromTyped[*slim_networkingv1.IngressClassList](cs.Slim().NetworkingV1().IngressClasses()), opts...,
+	)
+	return resource.New[*slim_networkingv1.IngressClass](lc, lw, resource.WithMetric("IngressClass")), nil
+}

operator/k8s/resources.go

@@ -10,6 +10,7 @@ import (
 	cilium_api_v2alpha1 "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2alpha1"
 	"github.com/cilium/cilium/pkg/k8s/resource"
 	slim_corev1 "github.com/cilium/cilium/pkg/k8s/slim/k8s/api/core/v1"
+	slim_networkingv1 "github.com/cilium/cilium/pkg/k8s/slim/k8s/api/networking/v1"
 )
 
 const (
@@ -37,6 +38,7 @@ var (
 			CiliumEndpointSliceResource,
 			k8s.CiliumNodeResource,
 			k8s.PodResource,
+			IngressClassResource,
 		),
 	)
 )
@@ -54,4 +56,5 @@ type Resources struct {
 	CiliumEndpointSlices resource.Resource[*cilium_api_v2alpha1.CiliumEndpointSlice]
 	CiliumNodes          resource.Resource[*cilium_api_v2.CiliumNode]
 	Pods                 resource.Resource[*slim_corev1.Pod]
+	IngressClasses       resource.Resource[*slim_networkingv1.IngressClass]
 }

operator/pkg/ingress/ingress.go

@@ -6,7 +6,6 @@ package ingress
 import (
 	"context"
 	"fmt"
-	"strconv"
 	"strings"
 
 	"github.com/sirupsen/logrus"
@@ -26,6 +25,7 @@ import (
 	ciliumv2 "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2"
 	k8sClient "github.com/cilium/cilium/pkg/k8s/client"
 	"github.com/cilium/cilium/pkg/k8s/informer"
+	"github.com/cilium/cilium/pkg/k8s/resource"
 	slim_corev1 "github.com/cilium/cilium/pkg/k8s/slim/k8s/api/core/v1"
 	slim_networkingv1 "github.com/cilium/cilium/pkg/k8s/slim/k8s/api/networking/v1"
 	"github.com/cilium/cilium/pkg/k8s/utils"
@@ -92,15 +92,19 @@ type Controller struct {
 	sharedLBServiceName     string
 	ciliumNamespace         string
 	defaultLoadbalancerMode string
-	isDefaultIngressClass   bool
-	defaultSecretNamespace  string
-	defaultSecretName       string
+
+	defaultSecretNamespace string
+	defaultSecretName      string
 
 	sharedLBStatus *slim_corev1.LoadBalancerStatus
 }
 
 // NewController returns a controller for ingress objects having ingressClassName as cilium
-func NewController(clientset k8sClient.Clientset, options ...Option) (*Controller, error) {
+func NewController(
+	clientset k8sClient.Clientset,
+	ingressClasses resource.Resource[*slim_networkingv1.IngressClass],
+	options ...Option,
+) (*Controller, error) {
 	opts := DefaultIngressOptions
 	for _, opt := range options {
 		if err := opt(&opts); err != nil {
@@ -157,7 +161,7 @@ func NewController(clientset k8sClient.Clientset, options ...Option) (*Controlle
 		nil,
 	)
 
-	ingressClassManager, err := newIngressClassManager(clientset, ic.queue, opts.MaxRetries)
+	ingressClassManager, err := newIngressClassManager(context.Background(), ic.queue, ingressClasses)
 	if err != nil {
 		return nil, err
 	}
@@ -197,12 +201,16 @@ func NewController(clientset k8sClient.Clientset, options ...Option) (*Controlle
 // Run kicks off the controlled loop
 func (ic *Controller) Run() {
 	defer ic.queue.ShutDown()
+
+	ctx := context.Background()
+
+	go ic.ingressClassManager.Run(ctx)
+
 	go ic.ingressInformer.Run(wait.NeverStop)
 	if !cache.WaitForCacheSync(wait.NeverStop, ic.ingressInformer.HasSynced) {
 		return
 	}
 
-	go ic.ingressClassManager.Run()
 	go ic.serviceManager.Run()
 	go ic.secretManager.Run()
 
@@ -246,7 +254,7 @@ func hasEmptyIngressClass(ingress *slim_networkingv1.Ingress) bool {
 func (ic *Controller) isCiliumIngressEntry(ingress *slim_networkingv1.Ingress) bool {
 	className := getIngressClassName(ingress)
 
-	if (className == nil || *className == "") && ic.isDefaultIngressClass {
+	if (className == nil || *className == "") && ic.ingressClassManager.IsDefault() {
 		return true
 	}
 
@@ -354,40 +362,30 @@ func (ic *Controller) handleIngressServiceUpdatedEvent(ingressServiceUpdated ing
 }
 
 func (ic *Controller) handleCiliumIngressClassUpdatedEvent(event ciliumIngressClassUpdatedEvent) error {
-	log.Debugf("Cilium IngressClass updated")
-	previousValue := ic.isDefaultIngressClass
-	if val, ok := event.ingressClass.GetAnnotations()[slim_networkingv1.AnnotationIsDefaultIngressClass]; ok {
-		isDefault, err := strconv.ParseBool(val)
+	if !event.changed {
+		return nil
+	}
+
+	log.WithField(CiliumIngressClassIsDefault, event.isDefault).Info(
+		"Cilium IngressClass default value changed, re-syncing ingresses",
+	)
+	// ensure that all ingresses are in the correct state
+	for _, k := range ic.ingressStore.ListKeys() {
+		ing, err := ic.getByKey(k)
 		if err != nil {
-			log.WithError(err).Warnf("Failed to parse annotation value for %q", slim_networkingv1.AnnotationIsDefaultIngressClass)
 			return err
 		}
-		ic.isDefaultIngressClass = isDefault
-	} else {
-		// if the annotation is not set we are not the default ingress class
-		ic.isDefaultIngressClass = false
-	}
 
-	if previousValue != ic.isDefaultIngressClass {
-		log.Debugf("Cilium IngressClass default value changed, re-syncing ingresses")
-		// ensure that all ingresses are in the correct state
-		for _, k := range ic.ingressStore.ListKeys() {
-			ing, err := ic.getByKey(k)
-			if err != nil {
+		if ic.isCiliumIngressEntry(ing) {
+			// make sure that the ingress is in the correct state
+			if err := ic.ensureResources(ing, false); err != nil {
 				return err
 			}
-
-			if ic.isCiliumIngressEntry(ing) {
-				// make sure that the ingress is in the correct state
-				if err := ic.ensureResources(ing, false); err != nil {
-					return err
-				}
-			} else if hasEmptyIngressClass(ing) && !ic.isDefaultIngressClass {
-				// if we are no longer the default ingress class, we need to clean up
-				// the resources that we created for the ingress
-				if err := ic.deleteResources(ing); err != nil {
-					return err
-				}
+		} else if hasEmptyIngressClass(ing) && !event.isDefault {
+			// if we are no longer the default ingress class, we need to clean up
+			// the resources that we created for the ingress
+			if err := ic.deleteResources(ing); err != nil {
+				return err
 			}
 		}
 	}
@@ -396,28 +394,27 @@ func (ic *Controller) handleCiliumIngressClassUpdatedEvent(event ciliumIngressCl
 }
 
 func (ic *Controller) handleCiliumIngressClassDeletedEvent(event ciliumIngressClassDeletedEvent) error {
-	log.Debug("Cilium IngressClass deleted")
+	if !event.isDefault {
+		return nil
+	}
 
-	if ic.isDefaultIngressClass {
-		// if we were the default ingress class, we need to clean up all ingresses
-		for _, k := range ic.ingressStore.ListKeys() {
-			ing, err := ic.getByKey(k)
-			if err != nil {
-				return err
-			}
+	log.Debug("Cilium IngressClass deleted, performing cleanup")
+	// if we were the default ingress class, we need to clean up all ingresses
+	for _, k := range ic.ingressStore.ListKeys() {
+		ing, err := ic.getByKey(k)
+		if err != nil {
+			return err
+		}
 
-			if hasEmptyIngressClass(ing) {
-				// if we are no longer the default ingress class, we need to clean up
-				// the resources that we created for the ingress
-				if err := ic.deleteResources(ing); err != nil {
-					return err
-				}
+		if hasEmptyIngressClass(ing) {
+			// if we are no longer the default ingress class, we need to clean up
+			// the resources that we created for the ingress
+			if err := ic.deleteResources(ing); err != nil {
+				return err
 			}
 		}
-
-		// disable the default ingress class behavior
-		ic.isDefaultIngressClass = false
 	}
+
 	return nil
 }
 
@@ -506,10 +503,10 @@ func (ic *Controller) handleEvent(event interface{}) error {
 		log.WithField(logfields.ServiceKey, ev.ingressService.Name).WithField(logfields.K8sNamespace, ev.ingressService.Namespace).Debug("Handling ingress service updated event")
 		err = ic.handleIngressServiceUpdatedEvent(ev)
 	case ciliumIngressClassUpdatedEvent:
-		log.WithField(logfields.IngressClass, ev.ingressClass.Name).Debug("Handling cilium ingress class updated event")
+		log.Debug("Handling cilium ingress class updated event")
 		err = ic.handleCiliumIngressClassUpdatedEvent(ev)
 	case ciliumIngressClassDeletedEvent:
-		log.WithField(logfields.IngressClass, ev.ingressClass.Name).Debug("Handling cilium ingress class deleted event")
+		log.Debug("Handling cilium ingress class deleted event")
 		err = ic.handleCiliumIngressClassDeletedEvent(ev)
 	default:
 		err = fmt.Errorf("received an unknown event: %t", ev)