Commit 0faf81ce authored by Stephen Greene's avatar Stephen Greene
Browse files

ingress: Fix up openshift-ingress namespace reconciliation

Commit `ea085e72` 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.
parent b7f990e5
...@@ -86,7 +86,7 @@ func routerNamespaceChanged(current, expected *corev1.Namespace) (bool, *corev1. ...@@ -86,7 +86,7 @@ func routerNamespaceChanged(current, expected *corev1.Namespace) (bool, *corev1.
} }
knownLabels := []string{ knownLabels := []string{
"opensift.io/cluster-monitoring", "openshift.io/cluster-monitoring",
"name", "name",
"network.openshift.io/policy-group", "network.openshift.io/policy-group",
"policy-group.network.openshift.io/ingress", "policy-group.network.openshift.io/ingress",
...@@ -104,14 +104,14 @@ func routerNamespaceChanged(current, expected *corev1.Namespace) (bool, *corev1. ...@@ -104,14 +104,14 @@ func routerNamespaceChanged(current, expected *corev1.Namespace) (bool, *corev1.
} }
for _, annotation := range knownAnnotations { 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] updated.Annotations[annotation] = expected.Annotations[annotation]
changed = true changed = true
} }
} }
for _, label := range knownLabels { 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] updated.Labels[label] = expected.Labels[label]
changed = true changed = true
} }
......
...@@ -41,16 +41,23 @@ func TestRouterNamespaceChanged(t *testing.T) { ...@@ -41,16 +41,23 @@ func TestRouterNamespaceChanged(t *testing.T) {
}, },
expect: false, 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 { for _, tc := range testCases {
original := manifests.RouterNamespace() desired := manifests.RouterNamespace()
mutated := original.DeepCopy() mutated := desired.DeepCopy()
tc.mutate(mutated) 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) t.Errorf("%s, expect routerNamespaceChanged to be %t, got %t", tc.description, tc.expect, changed)
} else if 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) t.Errorf("%s, routerNamespaceChanged does not behave as a fixed point function", tc.description)
} }
} }
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment