From d96c0281a7c1d21f616c00be62bf24f28fe874fb Mon Sep 17 00:00:00 2001
From: Stephen Greene <sgreene@redhat.com>
Date: Tue, 11 May 2021 13:33:26 -0400
Subject: [PATCH] ingress: Fix up openshift-ingress namespace reconciliation

Commit `ea085e7` added logic to reconcile the Ingress namespace during
cluster upgrades. This follow-up commit fixes some mistakes introduced
by that commit.

pkg/operator/controller/ingress/namespace.go:

Fix a typo: `opensift.io/cluster-monitoring`.
We are, after all, in the computing business, and not the baking
business.

Check if annotation/label map values exist since comparing a
non-existent map value to the empty string will always return true
in this case.

pkg/operator/controller/ingress/namespace_test.go:

Add unit test to prove that a map value that does not exist
is equivalent to the empty string.

Rename the `original` namespace to `desired`.
Switch ordering of `desired` and `mutated` in `routerNamespaceChanged`
calling sites to better reflect the intent of the unit tests.
---
 pkg/operator/controller/ingress/namespace.go      |  6 +++---
 pkg/operator/controller/ingress/namespace_test.go | 15 +++++++++++----
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/pkg/operator/controller/ingress/namespace.go b/pkg/operator/controller/ingress/namespace.go
index df75d250c9..b50b0dfb43 100644
--- a/pkg/operator/controller/ingress/namespace.go
+++ b/pkg/operator/controller/ingress/namespace.go
@@ -86,7 +86,7 @@ func routerNamespaceChanged(current, expected *corev1.Namespace) (bool, *corev1.
 	}
 
 	knownLabels := []string{
-		"opensift.io/cluster-monitoring",
+		"openshift.io/cluster-monitoring",
 		"name",
 		"network.openshift.io/policy-group",
 		"policy-group.network.openshift.io/ingress",
@@ -104,14 +104,14 @@ func routerNamespaceChanged(current, expected *corev1.Namespace) (bool, *corev1.
 	}
 
 	for _, annotation := range knownAnnotations {
-		if current.Annotations[annotation] != expected.Annotations[annotation] {
+		if val, ok := current.Annotations[annotation]; !ok || val != expected.Annotations[annotation] {
 			updated.Annotations[annotation] = expected.Annotations[annotation]
 			changed = true
 		}
 	}
 
 	for _, label := range knownLabels {
-		if current.Labels[label] != expected.Labels[label] {
+		if val, ok := current.Labels[label]; !ok || val != expected.Labels[label] {
 			updated.Labels[label] = expected.Labels[label]
 			changed = true
 		}
diff --git a/pkg/operator/controller/ingress/namespace_test.go b/pkg/operator/controller/ingress/namespace_test.go
index 14b970eafb..31deb0f118 100644
--- a/pkg/operator/controller/ingress/namespace_test.go
+++ b/pkg/operator/controller/ingress/namespace_test.go
@@ -41,16 +41,23 @@ func TestRouterNamespaceChanged(t *testing.T) {
 			},
 			expect: false,
 		},
+		{
+			description: "if a managed label with an empty string value is deleted",
+			mutate: func(ns *corev1.Namespace) {
+				delete(ns.Labels, "policy-group.network.openshift.io/ingress")
+			},
+			expect: true,
+		},
 	}
 
 	for _, tc := range testCases {
-		original := manifests.RouterNamespace()
-		mutated := original.DeepCopy()
+		desired := manifests.RouterNamespace()
+		mutated := desired.DeepCopy()
 		tc.mutate(mutated)
-		if changed, updated := routerNamespaceChanged(original, mutated); changed != tc.expect {
+		if changed, updated := routerNamespaceChanged(mutated, desired); changed != tc.expect {
 			t.Errorf("%s, expect routerNamespaceChanged to be %t, got %t", tc.description, tc.expect, changed)
 		} else if changed {
-			if changedAgain, _ := routerNamespaceChanged(mutated, updated); changedAgain {
+			if changedAgain, _ := routerNamespaceChanged(desired, updated); changedAgain {
 				t.Errorf("%s, routerNamespaceChanged does not behave as a fixed point function", tc.description)
 			}
 		}