Fix 932 - ingress annotations accept external changes

closes #932
related to #1018

 - Implementation: In some cases, when the ingress was changed, reconciliation was triggered, which changed the annotations back to the values calculated with GSLB. I modified the merge function to filter which values should be synced and which should not. In fact, it is only `k8gb.io/strategy` and `k8gb.io/primary-geotag` that are changed on initialization.
 - Utit tests update
 - Terratests are focused on repeated patching of annotations in ingress and examining ingress when patching annotations in GSLB

Signed-off-by: kuritka <kuritka@gmail.com>

controllers/gslb_controller_reconciliation_test.go

@@ -754,7 +754,6 @@ func TestReturnsExternalRecordsUsingFailoverStrategyAndFallbackDNSserver(t *test
 
 func TestGslbProperlyPropagatesAnnotationDownToIngress(t *testing.T) {
 	// arrange
-	expectedAnnotations := map[string]string{"annotation": "test", "k8gb.io/strategy": "roundRobin"}
 	settings := provideSettings(t, predefinedConfig)
 	settings.gslb.Annotations = map[string]string{"annotation": "test"}
 	err := settings.client.Update(context.TODO(), settings.gslb)
@@ -764,8 +763,29 @@ func TestGslbProperlyPropagatesAnnotationDownToIngress(t *testing.T) {
 	err2 := settings.client.Get(context.TODO(), settings.request.NamespacedName, settings.ingress)
 	// assert
 	assert.NoError(t, err2, "Failed to get expected ingress")
-	assert.Equal(t, expectedAnnotations, settings.ingress.Annotations)
-	assert.Equal(t, expectedAnnotations, settings.gslb.ObjectMeta.Annotations)
+	// only k8gb annotations are allowed to be merged into ingress
+	assert.True(t, utils.EqualAnnotations(map[string]string{"k8gb.io/strategy": "roundRobin"}, settings.ingress.Annotations))
+	assert.True(t, utils.EqualAnnotations(map[string]string{"annotation": "test", "k8gb.io/strategy": "roundRobin"}, settings.gslb.Annotations))
+}
+
+func TestIngressDoesntPropagateAnnotationsToGslb(t *testing.T) {
+	// arrange
+	mgslb := map[string]string{"k8gb.io/primary-geotag": "us", "k8gb.io/strategy": "roundRobin"}
+	ming := map[string]string{"k8gb.io/strategy": "roundRobin", "k8gb.io/primary-geotag": "na", "k8gb.io/protocol": "HTTP"}
+	settings := provideSettings(t, predefinedConfig)
+	settings.gslb.Annotations = mgslb
+	err := settings.client.Update(context.TODO(), settings.gslb)
+	require.NoError(t, err, "Can't update gslb")
+	settings.ingress.Annotations = ming
+	err = settings.client.Update(context.TODO(), settings.ingress)
+	require.NoError(t, err, "Can't update ingress")
+	// act
+	reconcileAndUpdateGslb(t, settings)
+	err2 := settings.client.Get(context.TODO(), settings.request.NamespacedName, settings.gslb)
+	// assert
+	assert.NoError(t, err2, "Failed to get expected ingress")
+	assert.True(t, utils.EqualAnnotations(mgslb, settings.gslb.Annotations))
+	assert.True(t, utils.EqualAnnotations(ming, settings.ingress.Annotations))
 }
 
 func TestReflectGeoTagInStatusAsUnsetByDefault(t *testing.T) {

controllers/ingress.go

@@ -87,7 +87,9 @@ 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)
+		// target (found) is taken from the cluster while source (i) is calculated from gslb
+		// only allowed to merge fields to be merged from source to target.
+		found.Annotations = utils.MergeAnnotations(found.Annotations, i.Annotations, primaryGeoTagAnnotation, strategyAnnotation)
 		err = r.Update(context.TODO(), found)
 		if errors.IsConflict(err) {
 			log.Info().

controllers/internal/utils/annotations.go

@@ -19,17 +19,38 @@ 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 {
+// allowedToMerge would be k8gb related annotations which are allowed to be merged
+// ONLY those values in target will be merged if they are allowed and exists in the source!
+func MergeAnnotations(target map[string]string, source map[string]string, allowedToMerge ...string) map[string]string {
+	allowSet := make(map[string]bool, len(allowedToMerge))
+	for _, v := range allowedToMerge {
+		allowSet[v] = true
+	}
 	if target == nil {
 		target = make(map[string]string)
 	}
 	if source == nil {
 		source = make(map[string]string)
 	}
 	for k, v := range source {
+		if !allowSet[k] {
+			continue
+		}
 		if target[k] != v {
 			target[k] = v
 		}
 	}
 	return target
 }
+
+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/internal/utils/annotations_test.go

@@ -26,11 +26,42 @@ 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 allowed = []string{"k8gb.io/primary-geotag", "k8gb.io/strategy", "field.cattle.io/publicEndpoints"}
+
+func TestTryAddDeniedAnnotation(t *testing.T) {
+	// arrange
+	// act
+	source := map[string]string{
+		"k8gb.io/primary-geotag":  "us",
+		"k8gb.io/strategy":        "failover",
+		"k8gb.io/port":            "8080",
+		"k8gb.io/protocol":        "TCP",
+		"k8gb.io/dns-ttl-seconds": "100",
+		"kubectl.kubernetes.io/last-applied-configuration": "{}",
+		"k8gb.io/override": "blah",
+	}
+	target := map[string]string{"k8gb.io/primary-geotag": "na",
+		"k8gb.io/strategy": "failover",
+		"k8gb.io/port":     "80",
+		"k8gb.io/tls":      "true",
+		"k8gb.io/override": "foo",
+	}
+
+	repaired := MergeAnnotations(target, source, "k8gb.io/primary-geotag", "k8gb.io/dns-ttl-seconds", "k8gb.io/override")
+	// assert
+	assert.Equal(t, 6, len(repaired))
+	assert.Equal(t, "us", repaired["k8gb.io/primary-geotag"])
+	assert.Equal(t, "failover", repaired["k8gb.io/strategy"])
+	assert.Equal(t, "true", repaired["k8gb.io/tls"])
+	assert.Equal(t, "100", repaired["k8gb.io/dns-ttl-seconds"])
+	assert.Equal(t, "80", repaired["k8gb.io/port"])
+	assert.Equal(t, "blah", repaired["k8gb.io/override"])
+}
 
 func TestAddNewAnnotations(t *testing.T) {
 	// arrange
 	// act
-	repaired := MergeAnnotations(a1, a2)
+	repaired := MergeAnnotations(a1, a2, allowed...)
 	// assert
 	assert.Equal(t, 3, len(repaired))
 	assert.Equal(t, "eu", repaired["k8gb.io/primary-geotag"])
@@ -58,7 +89,7 @@ func TestUpdateExistingRecords(t *testing.T) {
 	}
 	a1["k8gb.io/primary-geotag"] = "us"
 	// act
-	repaired := MergeAnnotations(a1, a2)
+	repaired := MergeAnnotations(a1, a2, allowed...)
 	// assert
 	assert.Equal(t, 3, len(repaired))
 	assert.Equal(t, "eu", repaired["k8gb.io/primary-geotag"])
@@ -69,7 +100,7 @@ func TestUpdateExistingRecords(t *testing.T) {
 func TestEqualAnnotationsWithNilA1(t *testing.T) {
 	// arrange
 	// act
-	repaired := MergeAnnotations(nil, a2)
+	repaired := MergeAnnotations(nil, a2, allowed...)
 	// assert
 	assert.True(t, assert.ObjectsAreEqual(a2, repaired))
 }

terratest/test/k8gb_ingress_annotation_update_test.go

@@ -0,0 +1,143 @@
+package test
+
+/*
+Copyright 2022 The k8gb Contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+
+Generated by GoLic, for more details see: https://github.com/AbsaOSS/golic
+*/
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"k8gbterratest/utils"
+)
+
+func TestIngressAnnotationUpdateWhenClusterIsReady(t *testing.T) {
+	t.Parallel()
+	const host = "terratest-roundrobin.cloud.example.com"
+	const endpointDNSNameEU = "gslb-ns-eu-cloud.example.com"
+	const gslbPath = "../examples/roundrobin_weight1.yaml"
+	testGslb1, err := utils.NewWorkflow(t, "k3d-test-gslb1", 5053).
+		WithGslb(gslbPath, host).
+		WithTestApp("eu").
+		Start()
+	require.NoError(t, err)
+	defer testGslb1.Kill()
+
+	err = testGslb1.WaitForAppIsRunning()
+	require.NoError(t, err)
+
+	err = testGslb1.WaitForExternalDNSEndpointExists()
+	require.NoError(t, err)
+
+	err = testGslb1.WaitForLocalDNSEndpointExists()
+	require.NoError(t, err)
+
+	err = testGslb1.Resources().WaitForExternalDNSEndpointHasTargets(endpointDNSNameEU)
+	require.NoError(t, err)
+
+	patchAnnotations := map[string]string{}
+	patchAnnotations["k8gb.io/protocol"] = "undefined"
+	patchAnnotations["k8gb.io/primary-geotag"] = "na"
+	err = testGslb1.Resources().Ingress().PatchAnnotations(patchAnnotations)
+	require.NoError(t, err)
+	time.Sleep(5 * time.Second)
+	newAnnotations := testGslb1.Resources().Ingress().GetAnnotations()
+	assert.Equal(t, 4, len(newAnnotations))
+	assert.Equal(t, "undefined", newAnnotations["k8gb.io/protocol"])
+	assert.Equal(t, "na", newAnnotations["k8gb.io/primary-geotag"])
+	assert.Equal(t, "roundRobin", newAnnotations["k8gb.io/strategy"])
+	assert.NotEmpty(t, newAnnotations["kubectl.kubernetes.io/last-applied-configuration"])
+}
+
+func TestIngressAnnotationUpdateRepeatedly(t *testing.T) {
+	t.Parallel()
+	const host = "terratest-roundrobin.cloud.example.com"
+	const gslbPath = "../examples/roundrobin_weight1.yaml"
+	const key = "k8gb.io/ep.ips"
+	testGslb1, err := utils.NewWorkflow(t, "k3d-test-gslb1", 5053).
+		WithGslb(gslbPath, host).
+		WithTestApp("eu").
+		Start()
+	require.NoError(t, err)
+	defer testGslb1.Kill()
+
+	err = testGslb1.WaitForAppIsRunning()
+	require.NoError(t, err)
+
+	for _, v := range []string{"10.0.0.1", "10.0.0.2", "10.0.0.3", "10.0.0.4"} {
+		patchAnnotations := map[string]string{}
+		patchAnnotations[key] = v
+		err = testGslb1.Resources().Ingress().PatchAnnotations(patchAnnotations)
+		require.NoError(t, err)
+		time.Sleep(3 * time.Second)
+		newAnnotations := testGslb1.Resources().Ingress().GetAnnotations()
+		assert.Equal(t, 3, len(newAnnotations))
+		assert.Equal(t, v, newAnnotations["k8gb.io/ep.ips"])
+	}
+}
+
+func TestGslbModifiesAllowedIngressAnnotationRepeatedly(t *testing.T) {
+	t.Parallel()
+	const host = "terratest-roundrobin.cloud.example.com"
+	const gslbPath = "../examples/roundrobin_weight1.yaml"
+	const ipKey = "k8gb.io/ep.ips"
+	const strategyKey = "k8gb.io/strategy"
+	testGslb1, err := utils.NewWorkflow(t, "k3d-test-gslb1", 5053).
+		WithGslb(gslbPath, host).
+		WithTestApp("eu").
+		Start()
+	require.NoError(t, err)
+	defer testGslb1.Kill()
+
+	err = testGslb1.WaitForAppIsRunning()
+	require.NoError(t, err)
+	initialStrategy := testGslb1.Resources().Gslb().GslbSpecProperty(".spec.strategy.type")
+	data := []struct {
+		ip       string
+		strategy string
+	}{{"10.0.0.1", "unknown1"}, {"10.100.0.1", "unknown2"}, {"10.200.0.1", "unknown3"}}
+
+	for _, v := range data {
+		patchAnnotations := map[string]string{}
+		// "k8gb.io/ep.ips" is not synced with ingress. However I change the value in GSLB, it will not pass the allowedMerge filter
+		// "k8gb.io/strategy" is synchronized with gslb, but the annotation is forcibly changed to `.spec.strategy.type` during reconciliation.
+		// However I change the annotation in GSLB, the computed annotation in the ingress will always come from .spec.strategy.type
+		patchAnnotations[ipKey] = v.ip
+		patchAnnotations[strategyKey] = v.strategy
+		err = testGslb1.Resources().Gslb().PatchAnnotations(patchAnnotations)
+		require.NoError(t, err)
+
+		time.Sleep(3 * time.Second)
+
+		ingressAnnotations := testGslb1.Resources().Ingress().GetAnnotations()
+		assert.Equal(t, 2, len(ingressAnnotations))
+		ips := ingressAnnotations[ipKey]
+		assert.Equal(t, "", ips)
+		strategy := ingressAnnotations[strategyKey]
+		assert.Equal(t, initialStrategy, strategy)
+
+		gslbAnnotations := testGslb1.Resources().Gslb().GetAnnotations()
+		assert.Equal(t, 3, len(gslbAnnotations))
+		ips = gslbAnnotations[ipKey]
+		assert.Equal(t, v.ip, ips)
+		strategy = gslbAnnotations[strategyKey]
+		assert.Equal(t, strategy, strategy)
+	}
+}

terratest/utils/extensions.go

@@ -553,15 +553,50 @@ type Resources struct {
 	i *Instance
 }
 
+type Gslb struct {
+	i *Instance
+}
+
+func (r *Resources) Gslb() *Gslb {
+	return &Gslb{
+		i: r.i,
+	}
+}
+
 // GslbSpecProperty returns actual value of one Spec property, e.g: `spec.ingress.rules[0].host`
-func (r *Resources) GslbSpecProperty(specPath string) string {
-	actualValue, _ := k8s.RunKubectlAndGetOutputE(r.i.w.t, r.i.w.k8sOptions, "get", "gslb", r.i.w.state.gslb.name,
+func (g *Gslb) GslbSpecProperty(specPath string) string {
+	actualValue, _ := k8s.RunKubectlAndGetOutputE(g.i.w.t, g.i.w.k8sOptions, "get", "gslb", g.i.w.state.gslb.name,
 		"-o", fmt.Sprintf("custom-columns=SERVICESTATUS:%s", specPath), "--no-headers")
 	return actualValue
 }
 
-func (r *Resources) Ingress() *networkingv1.Ingress {
-	return k8s.GetIngress(r.i.w.t, r.i.w.k8sOptions, r.i.w.settings.ingressName)
+func (g *Gslb) GetAnnotations() (a map[string]string) {
+	m := struct {
+		Metadata struct {
+			Annotations map[string]string `yaml:"annotations"`
+		} `yaml:"metadata"`
+	}{}
+	strValue, err := k8s.RunKubectlAndGetOutputE(g.i.w.t, g.i.w.k8sOptions, "get", "gslb", g.i.w.state.gslb.name, "-ojson")
+	require.NoError(g.i.w.t, err)
+	err = json.Unmarshal([]byte(strValue), &m)
+	require.NoError(g.i.w.t, err)
+	return m.Metadata.Annotations
+}
+
+func (g *Gslb) PatchAnnotations(a map[string]string) (err error) {
+	return g.i.patchAnnotations(g.i.w.state.gslb.name, "gslb", a)
+}
+
+type Ingress struct {
+	networkingv1.Ingress
+	i *Instance
+}
+
+func (r *Resources) Ingress() *Ingress {
+	return &Ingress{
+		*k8s.GetIngress(r.i.w.t, r.i.w.k8sOptions, r.i.w.settings.ingressName),
+		r.i,
+	}
 }
 
 func (r *Resources) GetLocalDNSEndpoint() DNSEndpoint {
@@ -570,6 +605,10 @@ func (r *Resources) GetLocalDNSEndpoint() DNSEndpoint {
 	return ep
 }
 
+func (ing *Ingress) PatchAnnotations(a map[string]string) (err error) {
+	return ing.i.patchAnnotations(ing.GetName(), "ingress", a)
+}
+
 func (r *Resources) GetExternalDNSEndpoint() DNSEndpoint {
 	ep, err := r.getDNSEndpoint("k8gb-ns-extdns", "k8gb")
 	r.i.continueIfK8sResourceNotFound(err)
@@ -592,3 +631,13 @@ func (r *Resources) getDNSEndpoint(epName, ns string) (ep DNSEndpoint, err error
 	err = json.Unmarshal([]byte(j), &ep)
 	return ep, err
 }
+
+func (i *Instance) patchAnnotations(name, ktype string, a map[string]string) (err error) {
+	var data []string
+	for k, v := range a {
+		data = append(data, fmt.Sprintf(`"%s":"%s"`, k, v))
+	}
+	annotations := fmt.Sprintf("{\"metadata\":{\"annotations\":{%s}}}", strings.Join(data, ","))
+	_, err = k8s.RunKubectlAndGetOutputE(i.w.t, i.w.k8sOptions, "patch", ktype, name, "-p", annotations, "--type=merge")
+	return err
+}