diff --git a/controllers/gslb_controller_reconciliation_test.go b/controllers/gslb_controller_reconciliation_test.go index 6d36169865..0b07bc5faf 100644 --- a/controllers/gslb_controller_reconciliation_test.go +++ b/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) { diff --git a/controllers/ingress.go b/controllers/ingress.go index 67fc315ea9..634833c04d 100644 --- a/controllers/ingress.go +++ b/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(). diff --git a/controllers/internal/utils/annotations.go b/controllers/internal/utils/annotations.go index 755a71a964..e958e13535 100644 --- a/controllers/internal/utils/annotations.go +++ b/controllers/internal/utils/annotations.go @@ -19,7 +19,13 @@ 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) } @@ -27,9 +33,24 @@ func MergeAnnotations(target map[string]string, source map[string]string) map[st 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 +} diff --git a/controllers/internal/utils/annotations_test.go b/controllers/internal/utils/annotations_test.go index 0f16e95cd9..7f555041d3 100644 --- a/controllers/internal/utils/annotations_test.go +++ b/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)) } diff --git a/terratest/test/k8gb_ingress_annotation_update_test.go b/terratest/test/k8gb_ingress_annotation_update_test.go new file mode 100644 index 0000000000..e01ae8f5c1 --- /dev/null +++ b/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) + } +} diff --git a/terratest/utils/extensions.go b/terratest/utils/extensions.go index 79986703f5..c858e7334b 100644 --- a/terratest/utils/extensions.go +++ b/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) @@ -582,6 +621,7 @@ func (i *Instance) logIfError(err error, message string, args ...interface{}) { } } +// kubectl -n k8gb patch deployment k8gb -p '{".spec.containers[0].image":"docker.io/kuritka/k8gb:temp005"}' func (r *Resources) getDNSEndpoint(epName, ns string) (ep DNSEndpoint, err error) { ep = DNSEndpoint{} opts := k8s.NewKubectlOptions(r.i.w.k8sOptions.ContextName, r.i.w.k8sOptions.ConfigPath, ns) @@ -592,3 +632,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 +}