Fix ingress Annotations replacement loop (#1652)

Annotation implementation change, terratests covered, test change.

When I create GSLB - not k8gb annotations are not copied to ingress. Annotations that are directly related to k8gb are now created in ingress.

When I change an annotation in ingress, the annotation is not copied to the GSLB

If I change an annotation that is related to k8gb in ingress, the GSLB as ource of truth returns the annotation

When I change an annotation in ingress that is not related to k8gb, reconciliation is not triggered and the annotation remains in ingress and is ignored by k8gb

When I create a GSLB from an ingress, the annotations are not copied to the GSLB.

Signed-off-by: Michal Kuritka <[email protected]>

controllers/gslb_controller_reconciliation.go

@@ -59,6 +59,8 @@ const (
 	splitBrainThresholdSecondsAnnotation = "k8gb.io/splitbrain-threshold-seconds"
 )
 
+var k8gbAnnotations = []string{strategyAnnotation, primaryGeoTagAnnotation, dnsTTLSecondsAnnotation, splitBrainThresholdSecondsAnnotation}
+
 var log = logging.Logger()
 
 var m = metrics.Metrics()

controllers/gslb_controller_reconciliation_test.go

@@ -826,7 +826,7 @@ func TestGslbProperlyPropagatesAnnotationDownToIngress(t *testing.T) {
 		Start().
 		RunTestFunc(func() {
 			// arrange
-			expectedAnnotations := map[string]string{"annotation": "test", "k8gb.io/strategy": "roundRobin"}
+			expectedAnnotations := map[string]string{"k8gb.io/strategy": "roundRobin"}
 			settings := provideSettings(t, predefinedConfig)
 			settings.gslb.Annotations = map[string]string{"annotation": "test"}
 			err := settings.client.Update(context.TODO(), settings.gslb)

controllers/gslb_controller_setup.go

@@ -23,17 +23,20 @@ import (
 	"fmt"
 	"strconv"
 
-	"github.com/k8gb-io/k8gb/controllers/depresolver"
+	"github.com/k8gb-io/k8gb/controllers/utils"
 
 	k8gbv1beta1 "github.com/k8gb-io/k8gb/api/v1beta1"
+	"github.com/k8gb-io/k8gb/controllers/depresolver"
 	corev1 "k8s.io/api/core/v1"
 	netv1 "k8s.io/api/networking/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"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/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	externaldns "sigs.k8s.io/external-dns/endpoint"
 )
@@ -90,6 +93,18 @@ func (r *GslbReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Owns(&externaldns.DNSEndpoint{}).
 		Watches(&corev1.Endpoints{}, endpointMapHandler).
 		Watches(&netv1.Ingress{}, ingressMapHandler).
+		WithEventFilter(predicate.Funcs{
+			UpdateFunc: func(e event.TypedUpdateEvent[client.Object]) bool {
+				if e.ObjectOld.GetGeneration() != e.ObjectNew.GetGeneration() {
+					return true
+				}
+				// Ignore reconciliation in case nothing has changed in k8gb annotations
+				oldAnnotations := e.ObjectOld.GetAnnotations()
+				newAnnotations := e.ObjectNew.GetAnnotations()
+				reconcile := !utils.EqualPredefinedAnnotations(oldAnnotations, newAnnotations, k8gbAnnotations...)
+				return reconcile
+			},
+		}).
 		Complete(r)
 }
 
@@ -123,9 +138,8 @@ func (r *GslbReconciler) createGSLBFromIngress(c client.Client, a client.Object,
 	}
 	gslb := &k8gbv1beta1.Gslb{
 		ObjectMeta: metav1.ObjectMeta{
-			Namespace:   a.GetNamespace(),
-			Name:        a.GetName(),
-			Annotations: a.GetAnnotations(),
+			Namespace: a.GetNamespace(),
+			Name:      a.GetName(),
 		},
 		Spec: k8gbv1beta1.GslbSpec{
 			Ingress: k8gbv1beta1.FromV1IngressSpec(ingressToReuse.Spec),

controllers/ingress.go

@@ -33,15 +33,16 @@ import (
 )
 
 func (r *GslbReconciler) gslbIngress(gslb *k8gbv1beta1.Gslb) (*netv1.Ingress, error) {
-	metav1.SetMetaDataAnnotation(&gslb.ObjectMeta, strategyAnnotation, gslb.Spec.Strategy.Type)
+	annotations := make(map[string]string)
+	annotations[strategyAnnotation] = gslb.Spec.Strategy.Type
 	if gslb.Spec.Strategy.PrimaryGeoTag != "" {
-		metav1.SetMetaDataAnnotation(&gslb.ObjectMeta, primaryGeoTagAnnotation, gslb.Spec.Strategy.PrimaryGeoTag)
+		annotations[primaryGeoTagAnnotation] = gslb.Spec.Strategy.PrimaryGeoTag
 	}
 	ingress := &netv1.Ingress{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        gslb.Name,
 			Namespace:   gslb.Namespace,
-			Annotations: gslb.Annotations,
+			Annotations: annotations,
 		},
 		Spec: k8gbv1beta1.ToV1IngressSpec(gslb.Spec.Ingress),
 	}
@@ -87,7 +88,7 @@ func (r *GslbReconciler) saveIngress(instance *k8gbv1beta1.Gslb, i *netv1.Ingres
 	// Update existing object with new spec and annotations
 	if !ingressEqual(found, i) {
 		found.Spec = i.Spec
-		found.Annotations = utils.MergeAnnotations(found.Annotations, i.Annotations)
+		found.Annotations = utils.MergeAnnotations(found.Annotations, i.Annotations, k8gbAnnotations...)
 		err = r.Update(context.TODO(), found)
 		if errors.IsConflict(err) {
 			log.Info().
@@ -110,10 +111,8 @@ func (r *GslbReconciler) saveIngress(instance *k8gbv1beta1.Gslb, i *netv1.Ingres
 }
 
 func ingressEqual(ing1 *netv1.Ingress, ing2 *netv1.Ingress) bool {
-	for k, v := range ing2.Annotations {
-		if ing1.Annotations[k] != v {
-			return false
-		}
+	if !utils.EqualPredefinedAnnotations(ing1.Annotations, ing2.Annotations, k8gbAnnotations...) {
+		return false
 	}
 	return reflect.DeepEqual(ing1.Spec, ing2.Spec)
 }

controllers/utils/annotations.go

@@ -19,17 +19,47 @@ Generated by GoLic, for more details see: https://github.com/AbsaOSS/golic
 */
 
 // MergeAnnotations adds or updates annotations from source to target and returns merge
-func MergeAnnotations(target map[string]string, source map[string]string) map[string]string {
+func MergeAnnotations(target map[string]string, source map[string]string, controlledAnnotations ...string) map[string]string {
 	if target == nil {
 		target = make(map[string]string)
 	}
 	if source == nil {
 		source = make(map[string]string)
 	}
-	for k, v := range source {
-		if target[k] != v {
-			target[k] = v
+	for _, key := range controlledAnnotations {
+		if val, ok := source[key]; ok {
+			if target[key] != val {
+				target[key] = val
+			}
 		}
 	}
+
 	return target
 }
+
+// EqualPredefinedAnnotations checks if there has been a change in controlledAnnotations, it ignores the rest
+func EqualPredefinedAnnotations(a1, a2 map[string]string, controlledAnnotations ...string) bool {
+	a1K8gbValues := map[string]string{}
+	a2K8gbValues := map[string]string{}
+	for _, v := range controlledAnnotations {
+		if val, ok := a1[v]; ok {
+			a1K8gbValues[v] = val
+		}
+		if val, ok := a2[v]; ok {
+			a2K8gbValues[v] = val
+		}
+	}
+	return EqualAnnotations(a1K8gbValues, a2K8gbValues)
+}
+
+func EqualAnnotations(a, b map[string]string) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for k := range a {
+		if b[k] != a[k] {
+			return false
+		}
+	}
+	return true
+}

controllers/utils/annotations_test.go

@@ -26,11 +26,12 @@ import (
 
 var a2 = map[string]string{"k8gb.io/primary-geotag": "eu", "k8gb.io/strategy": "failover"}
 var a1 = map[string]string{"field.cattle.io/publicEndpoints": "dummy"}
+var annotations = []string{"k8gb.io/primary-geotag", "k8gb.io/strategy", "k8gb.io/dns-ttl-seconds", "k8gb.io/splitbrain-threshold-seconds"}
 
 func TestAddNewAnnotations(t *testing.T) {
 	// arrange
 	// act
-	repaired := MergeAnnotations(a1, a2)
+	repaired := MergeAnnotations(a1, a2, annotations...)
 	// assert
 	assert.Equal(t, 3, len(repaired))
 	assert.Equal(t, "eu", repaired["k8gb.io/primary-geotag"])
@@ -43,7 +44,7 @@ func TestAddExistingAnnotations(t *testing.T) {
 		a1[k] = v
 	}
 	// act
-	repaired := MergeAnnotations(a1, a2)
+	repaired := MergeAnnotations(a1, a2, annotations...)
 	// assert
 	assert.Equal(t, 3, len(repaired))
 	assert.Equal(t, "eu", repaired["k8gb.io/primary-geotag"])
@@ -58,7 +59,7 @@ func TestUpdateExistingRecords(t *testing.T) {
 	}
 	a1["k8gb.io/primary-geotag"] = "us"
 	// act
-	repaired := MergeAnnotations(a1, a2)
+	repaired := MergeAnnotations(a1, a2, annotations...)
 	// assert
 	assert.Equal(t, 3, len(repaired))
 	assert.Equal(t, "eu", repaired["k8gb.io/primary-geotag"])
@@ -69,7 +70,7 @@ func TestUpdateExistingRecords(t *testing.T) {
 func TestEqualAnnotationsWithNilA1(t *testing.T) {
 	// arrange
 	// act
-	repaired := MergeAnnotations(nil, a2)
+	repaired := MergeAnnotations(nil, a2, annotations...)
 	// assert
 	assert.True(t, assert.ObjectsAreEqual(a2, repaired))
 }

controllers/utils/dns_test.go

@@ -64,6 +64,10 @@ func TestValidDig(t *testing.T) {
 	fqdn := defaultFqdn
 	// act
 	result, err := Dig(fqdn, defaultEdgeDNSServer)
+	if err != nil && strings.HasSuffix(err.Error(), "->8.8.8.8:53: i/o timeout") {
+		// udp 8.8.8.8:53 may be blocked on some local environments
+		t.Skip()
+	}
 	// assert
 	assert.NoError(t, err)
 	assert.NotEmpty(t, result)
@@ -118,6 +122,10 @@ func TestOneValidEdgeDNSInTheList(t *testing.T) {
 	// act
 	result, err := Dig(fqdn, edgeDNSServers...)
 	// assert
+	if err != nil && strings.HasSuffix(err.Error(), "->8.8.8.8:253: i/o timeout") {
+		// udp 8.8.8.8:253 may be blocked on some local environments
+		t.Skip()
+	}
 	assert.NoError(t, err)
 	assert.NotEmpty(t, result)
 	assert.NotEmpty(t, result[0])

terratest/examples/gslb-annotation.yaml

@@ -0,0 +1,24 @@
+apiVersion: k8gb.absa.oss/v1beta1
+kind: Gslb
+metadata:
+  name: test-gslb
+  annotations:
+      example.io/origin: gslb
+spec:
+  ingress:
+    ingressClassName: nginx
+    rules:
+      - host: test-gslb-annotation.cloud.example.com # This is the GSLB enabled host that clients would use
+        http: # This section mirrors the same structure as that of an Ingress resource and will be used verbatim when creating the corresponding Ingress resource that will match the GSLB host
+          paths:
+            - path: /
+              pathType: Prefix
+              backend:
+                service:
+                  name: frontend-podinfo # Gslb should reflect NotFound status
+                  port:
+                    name: http
+  strategy:
+    type: failover # Use a round robin load balancing strategy, when deciding which downstream clusters to route clients too
+    dnsTtlSeconds: 5
+    primaryGeoTag: "eu"

terratest/examples/ingress-annotation.yaml

@@ -0,0 +1,20 @@
+apiVersion: networking.k8s.io/v1
+kind: Ingress
+metadata:
+  annotations:
+    k8gb.io/strategy: failover
+    k8gb.io/primary-geotag: "eu"
+  name: test-gslb
+spec:
+  ingressClassName: nginx
+  rules:
+    - host: test-ingress-annotation-failover.cloud.example.com
+      http:
+        paths:
+          - path: /
+            pathType: Prefix
+            backend:
+              service:
+                name: frontend-podinfo
+                port:
+                  name: http