Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug 1954330: ingress: Fix up openshift-ingress namespace reconciliation #611

Merged
merged 1 commit into from May 12, 2021
Merged

Bug 1954330: ingress: Fix up openshift-ingress namespace reconciliation #611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/operator/controller/ingress/namespace.go
View file
Open in desktop
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
15 changes: 11 additions & 4 deletions pkg/operator/controller/ingress/namespace_test.go
View file
Open in desktop
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()
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my thinking, the names were fine before: routerNamespaceChanged has parameters current and expected, and its logic copies current and updates it using expected. In actual use, current is the object that's in the API, and expected is the new desired object, which mutates the current API object based on some update (changes to the ingresscontroller, or changes to the operator itself as in the case of these new annotations and labels). I don't think the order really matters here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In actual use, current is the object that's in the API, and expected is the new desired object

Right, the only caveat being in the case of the ingress namespace resource, desired is effectively manifests.RouterNamespace. I think that's where the prior naming was throwing me off.

If you don't mind, I would prefer to use the new naming and ordering in this PR, since IMO, it makes the test added in this PR simpler.

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)
}
}
Expand Down