Revert #94

pkg/operator/controller/controller.go

@@ -1,9 +1,8 @@
-package controller
+package clusteringress
 
 import (
 	"context"
 	"fmt"
-	"reflect"
 
 	"github.com/sirupsen/logrus"
 
@@ -16,7 +15,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 
@@ -28,9 +26,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/source"
 )
 
-// resourceMatcherFunc checks if the two objects match.
-type resourceMatcherFunc func(old, new runtime.Object) bool
-
 const (
 	// ClusterIngressFinalizer is applied to all ClusterIngresses before they are
 	// considered for processing; this ensures the operator has a chance to handle
@@ -153,20 +148,40 @@ func (r *reconciler) ensureRouterNamespace() error {
 	if err != nil {
 		return fmt.Errorf("failed to build router cluster role: %v", err)
 	}
-	_, err = r.verifyRouterAssetExists(types.NamespacedName{Namespace: cr.Namespace, Name: cr.Name}, cr, nil)
+	err = r.Client.Get(context.TODO(), types.NamespacedName{Namespace: cr.Namespace, Name: cr.Name}, cr)
 	if err != nil {
-		return err
+		if !errors.IsNotFound(err) {
+			return fmt.Errorf("failed to get router cluster role %s: %v", cr.Name, err)
+		}
+		err = r.Client.Create(context.TODO(), cr)
+		if err == nil {
+			logrus.Infof("created router cluster role %s", cr.Name)
+		} else if !errors.IsAlreadyExists(err) {
+			return fmt.Errorf("failed to create router cluster role %s: %v", cr.Name, err)
+		}
 	}
 
-	if err := r.ensureRouterNamespaceAsset(); err != nil {
-		return err
+	ns, err := r.ManifestFactory.RouterNamespace()
+	if err != nil {
+		return fmt.Errorf("failed to build router namespace: %v", err)
+	}
+	err = r.Client.Get(context.TODO(), types.NamespacedName{Name: ns.Name}, ns)
+	if err != nil {
+		if !errors.IsNotFound(err) {
+			return fmt.Errorf("failed to get router namespace %q: %v", ns.Name, err)
+		}
+		err = r.Client.Create(context.TODO(), ns)
+		if err == nil {
+			logrus.Infof("created router namespace %s", ns.Name)
+		} else if !errors.IsAlreadyExists(err) {
+			return fmt.Errorf("failed to create router namespace %s: %v", ns.Name, err)
+		}
 	}
 
 	sa, err := r.ManifestFactory.RouterServiceAccount()
 	if err != nil {
 		return fmt.Errorf("failed to build router service account: %v", err)
 	}
-	// TODO: switch this to use verifyRouterAssetExists(key, obj, nil)
 	err = r.Client.Get(context.TODO(), types.NamespacedName{Namespace: sa.Namespace, Name: sa.Name}, sa)
 	if err != nil {
 		if !errors.IsNotFound(err) {
@@ -184,7 +199,6 @@ func (r *reconciler) ensureRouterNamespace() error {
 	if err != nil {
 		return fmt.Errorf("failed to build router cluster role binding: %v", err)
 	}
-	// TODO: switch this to use verifyRouterAssetExists(key, obj, nil)
 	err = r.Client.Get(context.TODO(), types.NamespacedName{Name: crb.Name}, crb)
 	if err != nil {
 		if !errors.IsNotFound(err) {
@@ -204,9 +218,33 @@ func (r *reconciler) ensureRouterNamespace() error {
 // ensureRouterForIngress ensures all necessary router resources exist for a
 // given clusteringress.
 func (r *reconciler) ensureRouterForIngress(ci *ingressv1alpha1.ClusterIngress) error {
-	deployment, err := r.ensureRouterDeployment(ci)
+	expected, err := r.ManifestFactory.RouterDeployment(ci)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to build router deployment: %v", err)
+	}
+	current := expected.DeepCopy()
+	err = r.Client.Get(context.TODO(), types.NamespacedName{Namespace: expected.Namespace, Name: expected.Name}, current)
+	if err != nil {
+		if !errors.IsNotFound(err) {
+			return fmt.Errorf("failed to get router deployment %s/%s, %v", expected.Namespace, expected.Name, err)
+		}
+
+		err = r.Client.Create(context.TODO(), current)
+		if err == nil {
+			logrus.Infof("created router deployment %s/%s", current.Namespace, current.Name)
+		} else if !errors.IsAlreadyExists(err) {
+			return fmt.Errorf("failed to create router deployment %s/%s: %v", current.Namespace, current.Name, err)
+		}
+	}
+
+	if changed, updated := deploymentConfigChanged(current, expected); changed {
+		err = r.Client.Update(context.TODO(), updated)
+		if err == nil {
+			logrus.Infof("updated router deployment %s/%s", updated.Namespace, updated.Name)
+			current = updated
+		} else {
+			return fmt.Errorf("failed to update router deployment %s/%s, %v", updated.Namespace, updated.Name, err)
+		}
 	}
 
 	if ci.Spec.HighAvailability != nil {
@@ -226,8 +264,8 @@ func (r *reconciler) ensureRouterForIngress(ci *ingressv1alpha1.ClusterIngress)
 				deploymentRef := metav1.OwnerReference{
 					APIVersion: "apps/v1",
 					Kind:       "Deployment",
-					Name:       deployment.Name,
-					UID:        deployment.UID,
+					Name:       current.Name,
+					UID:        current.UID,
 					Controller: &trueVar,
 				}
 				service.SetOwnerReferences([]metav1.OwnerReference{deploymentRef})
@@ -247,12 +285,12 @@ func (r *reconciler) ensureRouterForIngress(ci *ingressv1alpha1.ClusterIngress)
 		}
 	}
 
-	if err := r.ensureInternalRouterServiceForIngress(deployment, ci); err != nil {
+	if err := r.ensureInternalRouterServiceForIngress(current, ci); err != nil {
 		return fmt.Errorf("failed to create internal router service for clusteringress %s: %v", ci.Name, err)
 	}
 
-	if err := r.syncClusterIngressStatus(deployment, ci); err != nil {
-		return fmt.Errorf("failed to update status of clusteringress %s/%s: %v", deployment.Namespace, deployment.Name, err)
+	if err := r.syncClusterIngressStatus(current, ci); err != nil {
+		return fmt.Errorf("failed to update status of clusteringress %s/%s: %v", current.Namespace, current.Name, err)
 	}
 
 	return nil
@@ -279,41 +317,6 @@ func (r *reconciler) syncClusterIngressStatus(deployment *appsv1.Deployment, ci
 	return nil
 }
 
-// verifyRouterAssetExists verifies that the desired router asset exists and
-// matches the desired object.
-func (r *reconciler) verifyRouterAssetExists(key types.NamespacedName, desired runtime.Object, matcherFunc resourceMatcherFunc) (runtime.Object, error) {
-	current := desired.DeepCopyObject()
-	err := r.Client.Get(context.TODO(), key, current)
-	if err != nil {
-		if !errors.IsNotFound(err) {
-			return nil, fmt.Errorf("failed to get router %T %s: %v", desired, key.String(), err)
-		}
-
-		if err := r.Client.Create(context.TODO(), current); err == nil {
-			logrus.Infof("created router asset %T %s", current, key.String())
-			return current, nil
-		} else if !errors.IsAlreadyExists(err) {
-			return nil, fmt.Errorf("failed to create router asset %T %s: %v", desired, key.String(), err)
-		}
-	}
-
-	if matcherFunc != nil && !matcherFunc(current, desired) {
-		if err := r.Client.Update(context.TODO(), desired); err != nil {
-			return nil, err
-		}
-
-		logrus.Infof("updated router asset %T %s", desired, key.String())
-		// Fetch back the updated asset.
-		err = r.Client.Get(context.TODO(), key, current)
-		if err != nil {
-			return nil, err
-		}
-		return current, nil
-	}
-
-	return current, nil
-}
-
 // ensureInternalRouterServiceForIngress ensures that an internal service exists
 // for a given ClusterIngress.
 func (r *reconciler) ensureInternalRouterServiceForIngress(deployment *appsv1.Deployment, ci *ingressv1alpha1.ClusterIngress) error {
@@ -348,69 +351,6 @@ func (r *reconciler) ensureInternalRouterServiceForIngress(deployment *appsv1.De
 	return nil
 }
 
-// ensureRouterNamespaceAsset ensures that the router namespace exists and
-// matches the expected namespace.
-func (r *reconciler) ensureRouterNamespaceAsset() error {
-	ns, err := r.ManifestFactory.RouterNamespace()
-	if err != nil {
-		return fmt.Errorf("failed to build router namespace: %v", err)
-	}
-
-	namespaceMatcher := func(obj1, obj2 runtime.Object) bool {
-		ns1 := obj1.(*corev1.Namespace)
-		ns2 := obj2.(*corev1.Namespace)
-
-		if !reflect.DeepEqual(ns1.Labels, ns2.Labels) {
-			return false
-		}
-		if !reflect.DeepEqual(ns1.Annotations, ns2.Annotations) {
-			return false
-		}
-
-		return true
-	}
-
-	key := types.NamespacedName{Name: ns.Name}
-	if _, err := r.verifyRouterAssetExists(key, ns, namespaceMatcher); err != nil {
-		return err
-	}
-
-	return nil
-}
-
-// ensureRouterDeployment ensures that the router deployment exists and matches
-// the expected deployment.
-func (r *reconciler) ensureRouterDeployment(ci *ingressv1alpha1.ClusterIngress) (*appsv1.Deployment, error) {
-	deployment, err := r.ManifestFactory.RouterDeployment(ci)
-	if err != nil {
-		return nil, fmt.Errorf("failed to build router deployment: %v", err)
-	}
-
-	clusterIngressMatcher := func(obj1, obj2 runtime.Object) bool {
-		ci1 := obj1.(*appsv1.Deployment)
-		ci2 := obj2.(*appsv1.Deployment)
-
-		if !reflect.DeepEqual(ci1.Labels, ci2.Labels) {
-			return false
-		}
-		if !reflect.DeepEqual(ci1.Annotations, ci2.Annotations) {
-			return false
-		}
-		if !reflect.DeepEqual(ci1.Spec, ci2.Spec) {
-			return false
-		}
-		return true
-	}
-
-	key := types.NamespacedName{Namespace: deployment.Namespace, Name: deployment.Name}
-	actual, err := r.verifyRouterAssetExists(key, deployment, clusterIngressMatcher)
-	if err != nil {
-		return nil, err
-	}
-
-	return actual.(*appsv1.Deployment), nil
-}
-
 // ensureDNSForLoadBalancer configures a wildcard DNS alias for a ClusterIngress
 // targeting the given service.
 func (r *reconciler) ensureDNSForLoadBalancer(ci *ingressv1alpha1.ClusterIngress, service *corev1.Service) error {

pkg/operator/controller/status.go

@@ -1,4 +1,4 @@
-package controller
+package clusteringress
 
 import (
 	"context"

pkg/operator/controller/status_test.go

@@ -1,4 +1,4 @@
-package controller
+package clusteringress
 
 import (
 	"fmt"

test/e2e/operator_test.go

@@ -188,24 +188,6 @@ func TestClusterIngressUpdate(t *testing.T) {
 		t.Fatalf("failed to get default router deployment: %v", err)
 	}
 
-	// update router namespace with a label.
-	routerNS := &corev1.Namespace{}
-	if err := cl.Get(context.TODO(), types.NamespacedName{Name: "openshift-ingress"}, routerNS); err != nil {
-		t.Fatalf("failed to get default router namespace: %v", err)
-	}
-
-	if routerNS.Labels == nil {
-		routerNS.Labels = map[string]string{}
-	}
-
-	e2eLabel := fmt.Sprintf("e2e-label-%v", time.Now().UnixNano())
-	routerNS.Labels[e2eLabel] = "e2e-test"
-
-	err = cl.Update(context.TODO(), routerNS)
-	if err != nil {
-		t.Fatalf("failed to update router namespace with e2e test label %s: %v", e2eLabel, err)
-	}
-
 	originalSecret := ci.Spec.DefaultCertificateSecret
 	expectedSecretName := fmt.Sprintf("router-certs-%s", ci.Name)
 	if originalSecret != nil {
@@ -241,20 +223,6 @@ func TestClusterIngressUpdate(t *testing.T) {
 		t.Fatalf("failed to get updated router deployment %s/%s: %v", deployment.Namespace, deployment.Name, err)
 	}
 
-	updatedNamespace := &corev1.Namespace{}
-	err = wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) {
-		if err := cl.Get(context.TODO(), types.NamespacedName{Name: "openshift-ingress"}, updatedNamespace); err != nil {
-			return false, err
-		}
-		if _, ok := updatedNamespace.Labels[e2eLabel]; ok {
-			return false, nil
-		}
-		return true, nil
-	})
-	if err != nil {
-		t.Fatalf("expected router namespace to be updated without the e2e test label %s: %v", e2eLabel, err)
-	}
-
 	ci.Spec.DefaultCertificateSecret = originalSecret
 	err = cl.Update(context.TODO(), ci)
 	if err != nil {