Skip to content

Commit

Permalink
ingress: Fix up openshift-ingress namespace reconciliation
Browse files Browse the repository at this point in the history
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.

Switch oredering of `original` and `mutated` in `routerNamespaceChanged`
calling sites to better reflect the intent of the unit tests.
  • Loading branch information
sgreene570 committed May 11, 2021
1 parent b7f990e commit 7d469a7
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 5 deletions.
6 changes: 3 additions & 3 deletions pkg/operator/controller/ingress/namespace.go
Expand Up @@ -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",
Expand All @@ -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
}
Expand Down
11 changes: 9 additions & 2 deletions pkg/operator/controller/ingress/namespace_test.go
Expand Up @@ -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()
tc.mutate(mutated)
if changed, updated := routerNamespaceChanged(original, mutated); changed != tc.expect {
if changed, updated := routerNamespaceChanged(mutated, original); 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(original, updated); changedAgain {
t.Errorf("%s, routerNamespaceChanged does not behave as a fixed point function", tc.description)
}
}
Expand Down

0 comments on commit 7d469a7

Please sign in to comment.