Merge pull request #608 from sgreene570/sync-ingress-namespace

Bug 1954330: Reconcile openshift-ingress namespace on upgrade

pkg/operator/controller/ingress/controller.go

@@ -699,10 +699,18 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d
 		return fmt.Errorf("failed to ensure cluster role: %v", err)
 	}
 
-	if err := r.ensureRouterNamespace(); err != nil {
+	if _, _, err := r.ensureRouterNamespace(); err != nil {
 		return fmt.Errorf("failed to ensure namespace: %v", err)
 	}
 
+	if err := r.ensureRouterServiceAccount(); err != nil {
+		return fmt.Errorf("failed to ensure service account: %v", err)
+	}
+
+	if err := r.ensureRouterClusterRoleBinding(); err != nil {
+		return fmt.Errorf("failed to ensure cluster role binding: %v", err)
+	}
+
 	var errs []error
 	if _, _, err := r.ensureServiceCAConfigMap(); err != nil {
 		// Even if we were unable to create the configmap at this time,

pkg/operator/controller/ingress/namespace.go

@@ -4,26 +4,127 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+
+	corev1 "k8s.io/api/core/v1"
+
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 
 	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
+	operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
 )
 
-// ensureRouterNamespace ensures all the necessary scaffolding exists for
-// routers generally, including a namespace and all RBAC setup.
-func (r *reconciler) ensureRouterNamespace() error {
-	ns := manifests.RouterNamespace()
-	if err := r.client.Get(context.TODO(), types.NamespacedName{Name: ns.Name}, ns); err != nil {
-		if !errors.IsNotFound(err) {
-			return fmt.Errorf("failed to get router namespace %q: %v", ns.Name, err)
+// ensureRouterNamespace ensures that the router namespace exists.
+func (r *reconciler) ensureRouterNamespace() (bool, *corev1.Namespace, error) {
+	desired := manifests.RouterNamespace()
+
+	haveNamespace, current, err := r.currentRouterNamespace()
+	if err != nil {
+		return false, nil, err
+	}
+
+	switch {
+	case !haveNamespace:
+		if err := r.client.Create(context.TODO(), desired); err != nil {
+			return false, nil, fmt.Errorf("failed to create router namespace: %v", err)
+		}
+		log.Info("created router namespace", "desired", desired)
+		return r.currentRouterNamespace()
+	case haveNamespace:
+		if updated, err := r.updateRouterNamespace(current, desired); err != nil {
+			return true, current, fmt.Errorf("failed to update router namespace: %v", err)
+		} else if updated {
+			return r.currentRouterNamespace()
+		}
+	}
+	return true, current, nil
+}
+
+// currentRouterNamespace gets the current router namespace resource.
+func (r *reconciler) currentRouterNamespace() (bool, *corev1.Namespace, error) {
+	namespace := &corev1.Namespace{}
+	name := types.NamespacedName{
+		Name: operatorcontroller.DefaultOperandNamespace,
+	}
+	if err := r.client.Get(context.TODO(), name, namespace); err != nil {
+		if errors.IsNotFound(err) {
+			return false, nil, nil
+		}
+		return false, nil, err
+	}
+	return true, namespace, nil
+}
+
+// updateRouterNamespace updates the router namespace if an appropriate change
+// has been detected.
+func (r *reconciler) updateRouterNamespace(current, desired *corev1.Namespace) (bool, error) {
+	changed, updated := routerNamespaceChanged(current, desired)
+	if !changed {
+		return false, nil
+	}
+	// Diff before updating because the client may mutate the object.
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
+	if err := r.client.Update(context.TODO(), updated); err != nil {
+		if errors.IsAlreadyExists(err) {
+			return false, nil
+		}
+		return false, err
+	}
+	log.Info("updated router namespace", "namespace", updated.Name, "diff", diff)
+	return true, nil
+}
+
+// routerNamespaceChanged returns true if current and expected differ by any
+// of the annotations or the labels managed by the operator.
+// Other namespace labels and annotations are not overwritten so that
+// values written by other controllers are preserved.
+func routerNamespaceChanged(current, expected *corev1.Namespace) (bool, *corev1.Namespace) {
+	knownAnnotations := []string{
+		"workload.openshift.io/allowed",
+	}
+
+	knownLabels := []string{
+		"opensift.io/cluster-monitoring",
+		"name",
+		"network.openshift.io/policy-group",
+		"policy-group.network.openshift.io/ingress",
+	}
+
+	updated := current.DeepCopy()
+	changed := false
+
+	if updated.Annotations == nil {
+		updated.Annotations = map[string]string{}
+	}
+
+	if updated.Labels == nil {
+		updated.Labels = map[string]string{}
+	}
+
+	for _, annotation := range knownAnnotations {
+		if current.Annotations[annotation] != expected.Annotations[annotation] {
+			updated.Annotations[annotation] = expected.Annotations[annotation]
+			changed = true
 		}
-		if err := r.client.Create(context.TODO(), ns); err != nil {
-			return fmt.Errorf("failed to create router namespace %s: %v", ns.Name, err)
+	}
+
+	for _, label := range knownLabels {
+		if current.Labels[label] != expected.Labels[label] {
+			updated.Labels[label] = expected.Labels[label]
+			changed = true
 		}
-		log.Info("created router namespace", "name", ns.Name)
 	}
 
+	if !changed {
+		return false, nil
+	}
+
+	return true, updated
+}
+
+func (r *reconciler) ensureRouterServiceAccount() error {
 	sa := manifests.RouterServiceAccount()
 	if err := r.client.Get(context.TODO(), types.NamespacedName{Namespace: sa.Namespace, Name: sa.Name}, sa); err != nil {
 		if !errors.IsNotFound(err) {
@@ -35,6 +136,10 @@ func (r *reconciler) ensureRouterNamespace() error {
 		log.Info("created router service account", "namespace", sa.Namespace, "name", sa.Name)
 	}
 
+	return nil
+}
+
+func (r *reconciler) ensureRouterClusterRoleBinding() error {
 	crb := manifests.RouterClusterRoleBinding()
 	if err := r.client.Get(context.TODO(), types.NamespacedName{Name: crb.Name}, crb); err != nil {
 		if !errors.IsNotFound(err) {
@@ -45,6 +150,5 @@ func (r *reconciler) ensureRouterNamespace() error {
 		}
 		log.Info("created router cluster role binding", "name", crb.Name)
 	}
-
 	return nil
 }

pkg/operator/controller/ingress/namespace_test.go

+package ingress
+
+import (
+	"testing"
+
+	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
+
+	corev1 "k8s.io/api/core/v1"
+)
+
+func TestRouterNamespaceChanged(t *testing.T) {
+	testCases := []struct {
+		description string
+		mutate      func(*corev1.Namespace)
+		expect      bool
+	}{
+		{
+			description: "if nothing changes",
+			mutate:      func(_ *corev1.Namespace) {},
+			expect:      false,
+		},
+		{
+			description: "if namespace workload annotation changes",
+			mutate: func(ns *corev1.Namespace) {
+				ns.Annotations["workload.openshift.io/allowed"] = "foo"
+			},
+			expect: true,
+		},
+		{
+			description: "if namespace network-policy annotation changes",
+			mutate: func(ns *corev1.Namespace) {
+				ns.Labels["policy-group.network.openshift.io/ingress"] = "foo"
+			},
+			expect: true,
+		},
+		{
+			description: "if an unmanaged label and unmanaged annotation change",
+			mutate: func(ns *corev1.Namespace) {
+				ns.Annotations["unmanaged.annotation.io"] = "foo"
+				ns.Labels["unmanaged.label.io"] = "foo"
+			},
+			expect: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		original := manifests.RouterNamespace()
+		mutated := original.DeepCopy()
+		tc.mutate(mutated)
+		if changed, updated := routerNamespaceChanged(original, mutated); 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 {
+				t.Errorf("%s, routerNamespaceChanged does not behave as a fixed point function", tc.description)
+			}
+		}
+	}
+}