diff --git a/pkg/operator/controller/controller.go b/pkg/operator/controller/controller.go index 66c1e9b6bc..63f256a60f 100644 --- a/pkg/operator/controller/controller.go +++ b/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 { diff --git a/pkg/operator/controller/status.go b/pkg/operator/controller/status.go index c4f09c91f5..758eb13b14 100644 --- a/pkg/operator/controller/status.go +++ b/pkg/operator/controller/status.go @@ -1,4 +1,4 @@ -package controller +package clusteringress import ( "context" diff --git a/pkg/operator/controller/status_test.go b/pkg/operator/controller/status_test.go index b006f77a14..b972d50057 100644 --- a/pkg/operator/controller/status_test.go +++ b/pkg/operator/controller/status_test.go @@ -1,4 +1,4 @@ -package controller +package clusteringress import ( "fmt" diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 2c4a320f03..c65e7243ad 100644 --- a/test/e2e/operator_test.go +++ b/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 {