From d0e9ac4aac19304711f167a15390119f7673f404 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Mon, 31 Oct 2022 13:22:14 -0700 Subject: [PATCH 1/9] Moved and renamed pkg/operator/controller/ingress/router_status.go -> pkg/operator/controller/route-status/route_status.go --- .../{ingress/router_status.go => route-status/route_status.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename pkg/operator/controller/{ingress/router_status.go => route-status/route_status.go} (100%) diff --git a/pkg/operator/controller/ingress/router_status.go b/pkg/operator/controller/route-status/route_status.go similarity index 100% rename from pkg/operator/controller/ingress/router_status.go rename to pkg/operator/controller/route-status/route_status.go From d62a15ffe9788b63891f65e5d2ad2bc5bf407a1b Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Mon, 31 Oct 2022 13:23:54 -0700 Subject: [PATCH 2/9] OCPBUGS-1689: Add the route-status control loop for clearing route admitted status when labels change on routes and namespaces. Refactored route_status.go and functions to be organized under the route-status directory. Added unit testing for route_status.go with the addition of the fake_client.go to simplify future creation of fake clients for unit testing. - `pkg/operator/controller/ingress/controller.go`: Moving functions from/to route_status.go. - `pkg/operator/controller/ingress/status.go`: Moved routeSelectorsUpdated here. - `pkg/operator/controller/route-metrics/controller.go`: De-duplicate logic between route-status and route-metric. - `pkg/operator/controller/route-metrics/controller_test.go`: Deleted because unit test for routeStatusAdmitted moved to `route-status/router_status_test.go`. - `pkg/operator/controller/route-status/controller.go`: New control loop that reconciles route status if a route or namespace label changes. - `pkg/operator/controller/route-status/controller_test.go`: Unit testing for the new control loop using fake client. - `pkg/operator/controller/route-status/route_status.go`: Moved from `pkg/operator/controller/ingress` to be co-located with route status control loop. Added various functions for support of route stale status clearing. - `pkg/operator/controller/route-status/router_status_test.go`: New route_status.go unit testing using fake clients. - `pkg/operator/operator.go`: Start the new route_status control loop and share the cache with the route_metric control loop. - `test/e2e/all_test.go`: Invoke new E2E tests - `test/e2e/router_status_test.go`: E2E tests for route status control loop. - `test/unit/fake_client.go`: Added common fake client initialization functions. --- pkg/operator/controller/ingress/controller.go | 48 +- pkg/operator/controller/ingress/status.go | 10 + .../controller/route-metrics/controller.go | 86 +--- .../route-metrics/controller_test.go | 111 ----- .../controller/route-status/controller.go | 110 ++++ .../route-status/controller_test.go | 197 ++++++++ .../controller/route-status/route_status.go | 274 ++++++---- .../route-status/route_status_test.go | 469 ++++++++++++++++++ pkg/operator/operator.go | 23 +- test/e2e/all_test.go | 2 + test/e2e/router_status_test.go | 172 ++++++- test/unit/fake_client.go | 46 ++ vendor/modules.txt | 2 + .../pkg/cache/informertest/fake_cache.go | 141 ++++++ .../pkg/controller/controllertest/doc.go | 20 + .../pkg/controller/controllertest/testing.go | 62 +++ .../unconventionallisttypecrd.go | 76 +++ .../pkg/controller/controllertest/util.go | 118 +++++ 18 files changed, 1672 insertions(+), 295 deletions(-) delete mode 100644 pkg/operator/controller/route-metrics/controller_test.go create mode 100644 pkg/operator/controller/route-status/controller.go create mode 100644 pkg/operator/controller/route-status/controller_test.go create mode 100644 pkg/operator/controller/route-status/route_status_test.go create mode 100644 test/unit/fake_client.go create mode 100644 vendor/sigs.k8s.io/controller-runtime/pkg/cache/informertest/fake_cache.go create mode 100644 vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/doc.go create mode 100644 vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/testing.go create mode 100644 vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/unconventionallisttypecrd.go create mode 100644 vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/util.go diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index fb9ef7a199..1cf56cf14b 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -14,6 +14,7 @@ import ( "github.com/openshift/cluster-ingress-operator/pkg/manifests" operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" routemetrics "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/route-metrics" + routestatus "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/route-status" "github.com/openshift/cluster-ingress-operator/pkg/util/ingresscontroller" retryable "github.com/openshift/cluster-ingress-operator/pkg/util/retryableerror" "github.com/openshift/cluster-ingress-operator/pkg/util/slice" @@ -918,7 +919,7 @@ func (r *reconciler) ensureIngressDeleted(ingress *operatorv1.IngressController) } else if allDeleted { // Deployment has been deleted and there are no more pods left. // Clear all routes status for this ingress controller. - statusErrs := r.clearAllRoutesStatusForIngressController(ingress.ObjectMeta.Name) + statusErrs := routestatus.ClearAllRoutesStatusForIngressController(r.client, ingress.ObjectMeta.Name) errs = append(errs, statusErrs...) } else { errs = append(errs, retryable.New(fmt.Errorf("not all router pods have been deleted for %s/%s", ingress.Namespace, ingress.Name), 15*time.Second)) @@ -1064,7 +1065,7 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d errs = append(errs, syncStatusErr) // If syncIngressControllerStatus updated our ingress status, it's important we query for that new object. - // If we don't, then the next function syncRouteStatus would always fail because it has a stale ingress object. + // If we don't, then the next function syncRouteStatusWithSelectorChange would always fail because it has a stale ingress object. if updated { updatedIc := &operatorv1.IngressController{} if err := r.client.Get(context.TODO(), types.NamespacedName{Namespace: ci.Namespace, Name: ci.Name}, updatedIc); err != nil { @@ -1075,7 +1076,7 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d SetIngressControllerNLBMetric(ci) - errs = append(errs, r.syncRouteStatus(ci)...) + errs = append(errs, r.syncRouteStatusWithSelectorChange(ci)...) return retryable.NewMaybeRetryableAggregate(errs) } @@ -1141,3 +1142,44 @@ func (r *reconciler) allRouterPodsDeleted(ingress *operatorv1.IngressController) return true, nil } + +// isRouterDeploymentRolloutComplete determines whether the rollout of the ingress router deployment is complete. +func (r *reconciler) isRouterDeploymentRolloutComplete(ic *operatorv1.IngressController) (bool, error) { + deployment := appsv1.Deployment{} + deploymentName := operatorcontroller.RouterDeploymentName(ic) + if err := r.client.Get(context.TODO(), deploymentName, &deployment); err != nil { + return false, fmt.Errorf("failed to get deployment %s: %w", deploymentName, err) + } + + if deployment.Generation != deployment.Status.ObservedGeneration { + return false, nil + } + if deployment.Status.Replicas != deployment.Status.UpdatedReplicas { + return false, nil + } + + return true, nil +} + +// syncRouteStatusWithSelectorChange ensures that all routes status have been synced with the ingress controller's state. +func (r *reconciler) syncRouteStatusWithSelectorChange(ic *operatorv1.IngressController) []error { + // Clear routes that are not admitted by this ingress controller if route selectors have been updated. + if routeSelectorsUpdated(ic) { + // Only clear once we are done rolling out routers. + // We want to avoid race condition in which we clear status and an old router re-admits it before terminated. + if done, err := r.isRouterDeploymentRolloutComplete(ic); err != nil { + return []error{err} + } else if done { + // Clear routes status not admitted by this ingress controller. + if errs := routestatus.ClearRoutesNotAdmittedByIngress(r.client, ic); len(errs) > 0 { + return errs + } + + // Now sync the selectors from the spec to the status, so we indicate we are done clearing status. + if err := r.syncIngressControllerSelectorStatus(ic); err != nil { + return []error{err} + } + } + } + return nil +} diff --git a/pkg/operator/controller/ingress/status.go b/pkg/operator/controller/ingress/status.go index 7195b18f8c..b89eeae554 100644 --- a/pkg/operator/controller/ingress/status.go +++ b/pkg/operator/controller/ingress/status.go @@ -120,6 +120,16 @@ func (r *reconciler) syncIngressControllerSelectorStatus(ic *operatorv1.IngressC return nil } +// routeSelectorsUpdated returns whether any of the route selectors have been updated by comparing +// the status selector fields to the spec selector fields. +func routeSelectorsUpdated(ingress *operatorv1.IngressController) bool { + if !reflect.DeepEqual(ingress.Spec.RouteSelector, ingress.Status.RouteSelector) || + !reflect.DeepEqual(ingress.Spec.NamespaceSelector, ingress.Status.NamespaceSelector) { + return true + } + return false +} + // MergeConditions adds or updates matching conditions, and updates // the transition time if details of a condition have changed. Returns // the updated condition array. diff --git a/pkg/operator/controller/route-metrics/controller.go b/pkg/operator/controller/route-metrics/controller.go index ef97c32e7e..9beb987178 100644 --- a/pkg/operator/controller/route-metrics/controller.go +++ b/pkg/operator/controller/route-metrics/controller.go @@ -5,15 +5,15 @@ import ( "fmt" "time" + "golang.org/x/time/rate" + operatorv1 "github.com/openshift/api/operator/v1" routev1 "github.com/openshift/api/route/v1" logf "github.com/openshift/cluster-ingress-operator/pkg/log" - "golang.org/x/time/rate" + routestatus "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/route-status" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/util/workqueue" @@ -38,18 +38,9 @@ var ( // New creates the route metrics controller. This is the controller // that handles all the logic for gathering and exporting // metrics related to route resources. -func New(mgr manager.Manager, namespace string) (controller.Controller, error) { - // Create a new cache to watch on Route objects from every namespace. - newCache, err := cache.New(mgr.GetConfig(), cache.Options{ - Scheme: mgr.GetScheme(), - }) - if err != nil { - return nil, err - } - // Add the cache to the manager so that the cache is started along with the other runnables. - mgr.Add(newCache) +func New(mgr manager.Manager, namespace string, cache cache.Cache) (controller.Controller, error) { reconciler := &reconciler{ - cache: newCache, + cache: cache, namespace: namespace, routeToIngresses: make(map[types.NamespacedName]sets.String), } @@ -72,7 +63,7 @@ func New(mgr manager.Manager, namespace string) (controller.Controller, error) { return nil, err } // add watch for changes in Route - if err := c.Watch(source.NewKindWithCache(&routev1.Route{}, newCache), + if err := c.Watch(source.NewKindWithCache(&routev1.Route{}, cache), handler.EnqueueRequestsFromMapFunc(reconciler.routeToIngressController)); err != nil { return nil, err } @@ -172,55 +163,26 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( return reconcile.Result{}, nil } - // NOTE: Even though the route admitted status should reflect validity of the namespace and route labelselectors, we still will validate - // the namespace and route labels as there are still edge scenarios where the route status may be inaccurate. - // List all the Namespaces filtered by our ingress's Namespace selector. - namespaceMatchingLabelsSelector := client.MatchingLabelsSelector{Selector: labels.Everything()} - if ingressController.Spec.NamespaceSelector != nil { - namespaceSelector, err := metav1.LabelSelectorAsSelector(ingressController.Spec.NamespaceSelector) - if err != nil { - log.Error(err, "ingresscontroller has an invalid namespace selector", "ingresscontroller", - ingressController.Name, "namespaceSelector", ingressController.Spec.NamespaceSelector) - return reconcile.Result{}, nil - } - namespaceMatchingLabelsSelector = client.MatchingLabelsSelector{Selector: namespaceSelector} - } - - namespaceList := corev1.NamespaceList{} - if err := r.cache.List(ctx, &namespaceList, namespaceMatchingLabelsSelector); err != nil { - return reconcile.Result{}, fmt.Errorf("failed to list Namespaces %q: %w", request, err) - } - // Create a set of Namespaces to easily look up Namespaces that matches the Routes assigned to the Ingress Controller. - namespacesSet := sets.NewString() - for i := range namespaceList.Items { - namespacesSet.Insert(namespaceList.Items[i].Name) + namespacesInShard, err := routestatus.GetNamespacesSelectedByIngressController(r.cache, ingressController) + if err != nil { + return reconcile.Result{}, err } - // List routes filtered by our ingress's route selector. - routeMatchingLabelsSelector := client.MatchingLabelsSelector{Selector: labels.Everything()} - if ingressController.Spec.RouteSelector != nil { - routeSelector, err := metav1.LabelSelectorAsSelector(ingressController.Spec.RouteSelector) - if err != nil { - log.Error(err, "ingresscontroller has an invalid route selector", "ingresscontroller", - ingressController.Name, "routeSelector", ingressController.Spec.RouteSelector) - return reconcile.Result{}, nil - } - routeMatchingLabelsSelector = client.MatchingLabelsSelector{Selector: routeSelector} - } - routeList := routev1.RouteList{} - if err := r.cache.List(ctx, &routeList, routeMatchingLabelsSelector); err != nil { - return reconcile.Result{}, fmt.Errorf("failed to list Routes for the Shard %q: %w", request, err) + // List all the Namespaces filtered by our ingress's Namespace selector. + routesInShard, err := routestatus.GetRoutesSelectedByIngressController(r.cache, ingressController) + if err != nil { + return reconcile.Result{}, err } // Variable to store the number of routes admitted by the Shard (Ingress Controller). routesAdmitted := 0 // Iterate through the list Routes. - for _, route := range routeList.Items { + for _, route := range routesInShard.Items { // Check if the Route's Namespace matches one of the Namespaces in the set namespacesSet and // the Route is admitted by the Ingress Controller. - if namespacesSet.Has(route.Namespace) && routeStatusAdmitted(route, ingressController.Name) { + if namespacesInShard.Has(route.Namespace) && routestatus.IsRouteStatusAdmitted(route, ingressController.Name) { // If the Route is admitted then, the routesAdmitted should be incremented by 1 for the Shard. routesAdmitted++ } @@ -231,21 +193,3 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( return reconcile.Result{}, nil } - -// routeStatusAdmitted returns true if a given route's status shows admitted by the Ingress Controller. -func routeStatusAdmitted(route routev1.Route, ingressControllerName string) bool { - // Iterate through the related Ingress Controllers. - for _, ingress := range route.Status.Ingress { - // Check if the RouterName matches the name of the Ingress Controller. - if ingress.RouterName == ingressControllerName { - // Check if the Route was admitted by the Ingress Controller. - for _, cond := range ingress.Conditions { - if cond.Type == routev1.RouteAdmitted && cond.Status == corev1.ConditionTrue { - return true - } - } - return false - } - } - return false -} diff --git a/pkg/operator/controller/route-metrics/controller_test.go b/pkg/operator/controller/route-metrics/controller_test.go deleted file mode 100644 index bda6d78f1a..0000000000 --- a/pkg/operator/controller/route-metrics/controller_test.go +++ /dev/null @@ -1,111 +0,0 @@ -package routemetrics - -import ( - "testing" - - routev1 "github.com/openshift/api/route/v1" - corev1 "k8s.io/api/core/v1" -) - -// Test_routeStatusAdmitted verifies that routeStatusAdmitted behaves correctly. -func Test_routeStatusAdmitted(t *testing.T) { - testCases := []struct { - name string - route routev1.Route - ingressControllerName string - expectedResult bool - }{ - { - name: "route admitted by default", - route: routev1.Route{ - Status: routev1.RouteStatus{ - Ingress: []routev1.RouteIngress{ - { - RouterName: "default", - Conditions: []routev1.RouteIngressCondition{ - { - Type: routev1.RouteAdmitted, - Status: corev1.ConditionTrue, - }, - }, - }, - }, - }, - }, - ingressControllerName: "default", - expectedResult: true, - }, - { - name: "route not admitted by sharded", - route: routev1.Route{ - Status: routev1.RouteStatus{ - Ingress: []routev1.RouteIngress{ - { - RouterName: "sharded", - Conditions: []routev1.RouteIngressCondition{ - { - Type: routev1.RouteAdmitted, - Status: corev1.ConditionFalse, - }, - }, - }, - }, - }, - }, - ingressControllerName: "sharded", - expectedResult: false, - }, - { - name: "route admitted by default, not admitted by sharded", - route: routev1.Route{ - Status: routev1.RouteStatus{ - Ingress: []routev1.RouteIngress{ - { - RouterName: "default", - Conditions: []routev1.RouteIngressCondition{ - { - Type: routev1.RouteAdmitted, - Status: corev1.ConditionTrue, - }, - }, - }, - }, - }, - }, - ingressControllerName: "sharded", - expectedResult: false, - }, - { - name: "route not admitted by sharded without Conditions", - route: routev1.Route{ - Status: routev1.RouteStatus{ - Ingress: []routev1.RouteIngress{ - { - RouterName: "sharded", - }, - }, - }, - }, - ingressControllerName: "sharded", - expectedResult: false, - }, - { - name: "route not admitted by any shard", - route: routev1.Route{ - Status: routev1.RouteStatus{ - Ingress: []routev1.RouteIngress{}, - }, - }, - ingressControllerName: "default", - expectedResult: false, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - if actualResult := routeStatusAdmitted(tc.route, tc.ingressControllerName); actualResult != tc.expectedResult { - t.Errorf("expected result %v, got %v", tc.expectedResult, actualResult) - } - }) - } -} diff --git a/pkg/operator/controller/route-status/controller.go b/pkg/operator/controller/route-status/controller.go new file mode 100644 index 0000000000..e677f8ee84 --- /dev/null +++ b/pkg/operator/controller/route-status/controller.go @@ -0,0 +1,110 @@ +package routestatus + +import ( + "context" + "fmt" + + routev1 "github.com/openshift/api/route/v1" + logf "github.com/openshift/cluster-ingress-operator/pkg/log" + corev1 "k8s.io/api/core/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + "sigs.k8s.io/controller-runtime/pkg/source" +) + +const ( + controllerName = "route_status_controller" +) + +var ( + log = logf.Logger.WithName(controllerName) +) + +// New creates the route status controller. This is the controller that handles reconciling route status and keeping +// it in sync. +func New(mgr manager.Manager, namespace string, cache cache.Cache) (controller.Controller, error) { + reconciler := &reconciler{ + cache: cache, + client: mgr.GetClient(), + namespace: namespace, + } + c, err := controller.New(controllerName, mgr, controller.Options{ + Reconciler: reconciler, + }) + if err != nil { + return nil, err + } + // Add watch for changes in Route + if err := c.Watch(source.NewKindWithCache(&routev1.Route{}, cache), &handler.EnqueueRequestForObject{}); err != nil { + return nil, err + } + // Add watch for changes in Namespace + if err := c.Watch(source.NewKindWithCache(&corev1.Namespace{}, cache), + handler.EnqueueRequestsFromMapFunc(reconciler.namespaceToRoutes)); err != nil { + return nil, err + } + return c, nil +} + +// namespaceToRoutes creates a reconcile.Request for all the routes in the namespace. +func (r *reconciler) namespaceToRoutes(obj client.Object) []reconcile.Request { + var requests []reconcile.Request + // Cast the received object into a Namespace object. + ns := obj.(*corev1.Namespace) + + routeList := routev1.RouteList{} + if err := r.cache.List(context.Background(), &routeList, client.InNamespace(ns.Name)); err != nil { + log.Error(err, "failed to list routes for namespace", "related", obj.GetSelfLink()) + return requests + } + + for _, route := range routeList.Items { + log.Info("queueing route", "name", route.Name, "related", obj.GetSelfLink()) + request := reconcile.Request{ + NamespacedName: types.NamespacedName{ + Namespace: route.Namespace, + Name: route.Name, + }, + } + requests = append(requests, request) + } + + return requests +} + +// reconciler handles the actual route reconciliation logic in response to events. +type reconciler struct { + cache cache.Cache + client client.Client + namespace string +} + +// Reconcile expects request to refer to a Route object, which will clear route status if it is no longer admitted +// by each ingress controller it claims to be. +func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { + log.Info("reconciling", "request", request) + + // Fetch the Route object. + route := &routev1.Route{} + if err := r.cache.Get(ctx, request.NamespacedName, route); err != nil { + if kerrors.IsNotFound(err) { + // This means the rout object was already deleted/finalized. + log.Info("route not found; reconciliation will be skipped", "request", request) + return reconcile.Result{}, nil + } + return reconcile.Result{}, fmt.Errorf("failed to get route %q: %w", request, err) + } + + // Validate and clean up route status for given route if any status is stale. + if err := clearStaleRouteAdmittedStatus(r.client, route); err != nil { + return reconcile.Result{}, err + } + + return reconcile.Result{}, nil +} diff --git a/pkg/operator/controller/route-status/controller_test.go b/pkg/operator/controller/route-status/controller_test.go new file mode 100644 index 0000000000..9ba16031b2 --- /dev/null +++ b/pkg/operator/controller/route-status/controller_test.go @@ -0,0 +1,197 @@ +package routestatus + +import ( + "context" + v12 "github.com/openshift/api/operator/v1" + routev1 "github.com/openshift/api/route/v1" + "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" + "github.com/openshift/cluster-ingress-operator/test/unit" + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + "testing" +) + +// Test_Reconcile verifies Reconcile +// Note: This effectively tests clearStaleRouteAdmittedStatus as well +func Test_Reconcile(t *testing.T) { + reconcileRequestRoute := reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "route", + Namespace: "ns", + }, + } + testCases := []struct { + name string + request reconcile.Request + route *routev1.Route + ingressController *v12.IngressController + namespace *corev1.Namespace + expectedRoute *routev1.Route + expectedErr bool + }{ + { + name: "admitted with no routeSelector and no namespaceSelector with route with no labels in namespace with no labels", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + namespace: newNamespace("ns", ""), + ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + }, + { + name: "admitted with no routeSelector and no namespaceSelector with route with correct labels in namespace with no labels", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + namespace: newNamespace("ns", ""), + ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + }, + { + name: "admitted with no routeSelector and no namespaceSelector with route with no labels in namespace with correct labels", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + namespace: newNamespace("ns", "shard-label"), + ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + }, + { + name: "admitted with routeSelector and no namespaceSelector with route with correct labels in namespace with no labels", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "shard-label", "default-ic"), + namespace: newNamespace("ns", ""), + ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + }, + { + name: "not admitted with routeSelector and no namespaceSelector with route with no labels in namespace with no labels", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + namespace: newNamespace("ns", ""), + ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", ""), + }, + { + name: "not admitted with routeSelector and no namespaceSelector with route with incorrect labels in namespace with no labels", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "not-shard-label", "default-ic"), + namespace: newNamespace("ns", ""), + ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", ""), + }, + { + name: "admitted with routeSelector and namespaceSelector with route with correct labels in namespace with correct labels", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "shard-label", "default-ic"), + namespace: newNamespace("ns", "shard-label"), + ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "shard-label", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + }, + { + name: "not admitted with routeSelector and namespaceSelector with route with incorrect labels in namespace with correct labels", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "not-shard-label", "default-ic"), + namespace: newNamespace("ns", "shard-label"), + ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "shard-label", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", ""), + }, + { + name: "not admitted with routeSelector and namespaceSelector with route with correct labels in namespace with incorrect labels", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "shard-label", "default-ic"), + namespace: newNamespace("ns", "not-shard-label"), + ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "shard-label", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", ""), + }, + { + name: "admitted with expression routeSelector and no namespaceSelector with route with correct labels in namespace with no labels", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "shard-label", "default-ic"), + namespace: newNamespace("ns", ""), + ingressController: newIngressControllerWithSelectors("default-ic", "", "shard-label", "", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + }, + { + name: "not admitted with expression routeSelector and no namespaceSelector with route with incorrect labels in namespace with no labels", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "not-shard-label", "default-ic"), + namespace: newNamespace("ns", ""), + ingressController: newIngressControllerWithSelectors("default-ic", "", "shard-label", "", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", ""), + }, + { + name: "admitted with no routeSelector and expression namespaceSelector with route with correct labels in namespace with no labels", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + namespace: newNamespace("ns", "shard-label"), + ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", "shard-label"), + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + }, + { + name: "not admitted with no routeSelector and expression namespaceSelector with route with incorrect labels in namespace with no labels", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + namespace: newNamespace("ns", "not-shard-label"), + ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", "shard-label"), + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", ""), + }, + { + name: "route is nil", + request: reconcileRequestRoute, + route: nil, + namespace: newNamespace("ns", "not-shard-label"), + ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", "shard-label"), + expectedErr: false, + }, + { + name: "admitted by ingress controller that doesn't exist anymore", + request: reconcileRequestRoute, + route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "missing-ic"), + namespace: newNamespace("ns", "shard-label"), + ingressController: nil, + expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "missing-ic"), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + r := newFakeReconciler(tc.namespace) + if tc.route != nil { + if err := r.client.Create(context.Background(), tc.route); err != nil { + t.Errorf("error creating route: %v", err) + } + } + if tc.ingressController != nil { + if err := r.client.Create(context.Background(), tc.ingressController); err != nil { + t.Errorf("error creating ingress controller: %v", err) + } + } + + if _, err := r.Reconcile(context.Background(), tc.request); tc.expectedErr && err == nil { + t.Errorf("expected error, got no error") + } else if !tc.expectedErr && err != nil { + t.Errorf("did not expected error: %v", err) + } else if tc.expectedRoute != nil && !tc.expectedErr { + actualRoute := routev1.Route{} + actualRouteName := types.NamespacedName{Name: tc.route.Name, Namespace: tc.route.Namespace} + if err := r.client.Get(context.Background(), actualRouteName, &actualRoute); err != nil { + t.Errorf("error retrieving route from client: %v", err) + } + assert.Equal(t, tc.expectedRoute.Status, actualRoute.Status, "route name", tc.expectedRoute.Name) + } + }) + } +} + +// newFakeReconciler builds a reconciler object for configurable-route based on fake clients and caches. +func newFakeReconciler(initObjs ...client.Object) *reconciler { + client := unit.NewFakeClient(initObjs...) + cache := unit.NewFakeCache(client) + r := reconciler{ + client: client, + cache: cache, + namespace: controller.DefaultOperandNamespace, + } + return &r +} diff --git a/pkg/operator/controller/route-status/route_status.go b/pkg/operator/controller/route-status/route_status.go index 9ecd663f36..81558cd306 100644 --- a/pkg/operator/controller/route-status/route_status.go +++ b/pkg/operator/controller/route-status/route_status.go @@ -1,23 +1,21 @@ -package ingress +package routestatus import ( "context" "fmt" - "k8s.io/apimachinery/pkg/labels" - "reflect" "time" - operatorv1 "github.com/openshift/api/operator/v1" - routev1 "github.com/openshift/api/route/v1" - operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" - - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - + kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" - "sigs.k8s.io/controller-runtime/pkg/client" + + operatorv1 "github.com/openshift/api/operator/v1" + routev1 "github.com/openshift/api/route/v1" + operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" ) // A brief overview of the operator's interactions with route status: @@ -27,67 +25,36 @@ import ( // status when it stops managing the route. Here are the scenarios where the operator steps in: // #1 When the ingress controller, the corresponding router deployment, and its pods are deleted. // - The operator knows when a router is deleted because it is the one responsible for deleting it. So it -// simply calls clearRouteStatus to clear status of routes that openshift-router has admitted. +// simply calls ClearRouteStatus to clear status of routes that openshift-router has admitted. +// - Handled by the ingress control loop. // #2 When the ingress controller sharding configuration (i.e., selectors) is changed. // - When the selectors (routeSelector and namespaceSelector) are updated, the operator simply clears the status of // any route that it is no longer selecting using the updated selectors. // - We determine what routes are admitted by the current state of the selectors (just like the openshift-router). +// - Handled by the ingress control loop. +// # 3 When the route's labels are changed. +// - When the route labels are updated, the operator will reconcile that specific route and clean up any admitted +// status that is now stale. +// - Handled by the route-status control loop. +// # 4 When the namespace's labels are changed. +// - When the namespace labels are updated, the operator will reconcile all the routes in the namespace and clean up +// stale route statuses. +// - Handled by the route-status control loop. -// syncRouteStatus ensures that all routes status have been synced with the ingress controller's state. -func (r *reconciler) syncRouteStatus(ic *operatorv1.IngressController) []error { - // Clear routes that are not admitted by this ingress controller if route selectors have been updated. - if routeSelectorsUpdated(ic) { - // Only clear once we are done rolling out routers. - // We want to avoid race condition in which we clear status and an old router re-admits it before terminated. - if done, err := r.isRouterDeploymentRolloutComplete(ic); err != nil { - return []error{err} - } else if done { - // Clear routes status not admitted by this ingress controller. - if errs := r.clearRoutesNotAdmittedByIngress(ic); len(errs) > 0 { - return errs - } - - // Now sync the selectors from the spec to the status, so we indicate we are done clearing status. - if err := r.syncIngressControllerSelectorStatus(ic); err != nil { - return []error{err} - } - } - } - return nil -} - -// isRouterDeploymentRolloutComplete determines whether the rollout of the ingress router deployment is complete. -func (r *reconciler) isRouterDeploymentRolloutComplete(ic *operatorv1.IngressController) (bool, error) { - deployment := appsv1.Deployment{} - deploymentName := operatorcontroller.RouterDeploymentName(ic) - if err := r.client.Get(context.TODO(), deploymentName, &deployment); err != nil { - return false, fmt.Errorf("failed to get deployment %s: %w", deploymentName, err) - } - - if deployment.Generation != deployment.Status.ObservedGeneration { - return false, nil - } - if deployment.Status.Replicas != deployment.Status.UpdatedReplicas { - return false, nil - } - - return true, nil -} - -// clearAllRoutesStatusForIngressController clears any route status that have been -// admitted by provided ingress controller. -func (r *reconciler) clearAllRoutesStatusForIngressController(icName string) []error { +// ClearAllRoutesStatusForIngressController clears any route status that has been admitted by the ingress controller, +// regardless if it is actually admitted or not. This function should be used for deletions of ingress controllers. +func ClearAllRoutesStatusForIngressController(kclient client.Client, icName string) []error { // List all routes. errs := []error{} start := time.Now() routeList := &routev1.RouteList{} routesCleared := 0 - if err := r.client.List(context.TODO(), routeList); err != nil { + if err := kclient.List(context.TODO(), routeList); err != nil { return append(errs, fmt.Errorf("failed to list all routes in order to clear route status for deployment %s: %w", icName, err)) } // Clear status on the routes that belonged to icName. for i := range routeList.Items { - if cleared, err := r.clearRouteStatus(&routeList.Items[i], icName); err != nil { + if cleared, err := ClearRouteStatus(kclient, &routeList.Items[i], icName); err != nil { errs = append(errs, err) } else if cleared { routesCleared++ @@ -100,8 +67,11 @@ func (r *reconciler) clearAllRoutesStatusForIngressController(icName string) []e return errs } -// clearRouteStatus clears a route's status that is admitted by a specific ingress controller. -func (r *reconciler) clearRouteStatus(route *routev1.Route, icName string) (bool, error) { +// ClearRouteStatus clears a route's status that is admitted by a specific ingress controller. +func ClearRouteStatus(kclient client.Client, route *routev1.Route, icName string) (bool, error) { + if route == nil { + return false, fmt.Errorf("failed to clear route status: route is nil") + } // Go through each route and clear status if admitted by this ingress controller. var updated routev1.Route for i := range route.Status.Ingress { @@ -110,7 +80,7 @@ func (r *reconciler) clearRouteStatus(route *routev1.Route, icName string) (bool // Remove this status since it matches our routerName. route.DeepCopyInto(&updated) updated.Status.Ingress = append(route.Status.Ingress[:i], route.Status.Ingress[i+1:]...) - if err := r.client.Status().Update(context.TODO(), &updated); err != nil { + if err := kclient.Status().Update(context.TODO(), &updated); err != nil { return false, fmt.Errorf("failed to clear route status of %s/%s for routerName %s: %w", route.Namespace, route.Name, icName, err) } @@ -124,42 +94,25 @@ func (r *reconciler) clearRouteStatus(route *routev1.Route, icName string) (bool return false, nil } -// routeSelectorsUpdated returns whether any of the route selectors have been updated by comparing -// the status selector fields to the spec selector fields. -func routeSelectorsUpdated(ingress *operatorv1.IngressController) bool { - if !reflect.DeepEqual(ingress.Spec.RouteSelector, ingress.Status.RouteSelector) || - !reflect.DeepEqual(ingress.Spec.NamespaceSelector, ingress.Status.NamespaceSelector) { - return true +// ClearRoutesNotAdmittedByIngress clears routes status that are not selected by a specific ingress controller. +func ClearRoutesNotAdmittedByIngress(kclient client.Client, ingress *operatorv1.IngressController) []error { + var errs []error + if ingress == nil { + return append(errs, fmt.Errorf("ingress controller is nil")) } - return false -} -// clearRoutesNotAdmittedByIngress clears routes status that are not selected by a specific ingress controller. -func (r *reconciler) clearRoutesNotAdmittedByIngress(ingress *operatorv1.IngressController) []error { start := time.Now() - errs := []error{} // List all routes. routeList := &routev1.RouteList{} - if err := r.client.List(context.TODO(), routeList); err != nil { + if err := kclient.List(context.TODO(), routeList); err != nil { return append(errs, fmt.Errorf("failed to list all routes in order to clear route status: %w", err)) } - // List namespaces filtered by our ingress's namespace selector. - namespaceSelector, err := metav1.LabelSelectorAsSelector(ingress.Spec.NamespaceSelector) + // List all the Namespaces filtered by our ingress's Namespace selector. + namespacesInShard, err := GetNamespacesSelectedByIngressController(kclient, ingress) if err != nil { - return append(errs, fmt.Errorf("ingresscontroller %s has an invalid namespace selector: %w", ingress.Name, err)) - } - filteredNamespaceList := &corev1.NamespaceList{} - if err := r.client.List(context.TODO(), filteredNamespaceList, - client.MatchingLabelsSelector{Selector: namespaceSelector}); err != nil { - return append(errs, fmt.Errorf("failed to list all namespaces in order to clear route status for %s: %w", ingress.Name, err)) - } - - // Create a set of namespaces to easily look up namespaces in this shard. - namespacesInShard := sets.NewString() - for i := range filteredNamespaceList.Items { - namespacesInShard.Insert(filteredNamespaceList.Items[i].Name) + return append(errs, err) } // List routes filtered by our ingress's route selector. @@ -168,7 +121,8 @@ func (r *reconciler) clearRoutesNotAdmittedByIngress(ingress *operatorv1.Ingress return append(errs, fmt.Errorf("ingresscontroller %s has an invalid route selector: %w", ingress.Name, err)) } - // Iterate over the entire route list and clear if not selected by route selector OR namespace selector. + // Iterate over the entire route list and clear if either the route selector OR the namespace selector does not + // select it. routesCleared := 0 for i := range routeList.Items { route := &routeList.Items[i] @@ -177,7 +131,7 @@ func (r *reconciler) clearRoutesNotAdmittedByIngress(ingress *operatorv1.Ingress namespaceInShard := namespacesInShard.Has(route.Namespace) if !routeInShard || !namespaceInShard { - if cleared, err := r.clearRouteStatus(route, ingress.ObjectMeta.Name); err != nil { + if cleared, err := ClearRouteStatus(kclient, route, ingress.ObjectMeta.Name); err != nil { errs = append(errs, err) } else if cleared { routesCleared++ @@ -190,6 +144,150 @@ func (r *reconciler) clearRoutesNotAdmittedByIngress(ingress *operatorv1.Ingress return errs } +// clearStaleRouteAdmittedStatus cleans up a single route's admitted status by verifying it is actually admitted by +// the ingress controller its status claims to be admitted by. +func clearStaleRouteAdmittedStatus(kclient client.Client, route *routev1.Route) error { + if route == nil { + return fmt.Errorf("route is nil") + } + // Iterate through the Route's Ingresses. + for _, ri := range route.Status.Ingress { + // Check if the Route was admitted by the RouteIngress. + for _, cond := range ri.Conditions { + if cond.Type == routev1.RouteAdmitted && cond.Status == corev1.ConditionTrue { + // Get the ingress controller that this route status claims to be admitted by. + icName := types.NamespacedName{Name: ri.RouterName, Namespace: operatorcontroller.DefaultOperatorNamespace} + ic := &operatorv1.IngressController{} + if err := kclient.Get(context.TODO(), icName, ic); err != nil { + // If the Ingress Controller doesn't exist at all, skip status clear, as it should have been cleared + // upon Ingress Controller deletion. If it made it here, then it's probably a router status test. + if kerrors.IsNotFound(err) { + continue + } + return fmt.Errorf("failed to get ingresscontroller %q: %v", icName, err) + } + + // If it is no longer admitted by this ingress controller, clear the status. + if admitted, err := isRouteAdmittedByIngressController(kclient, route, ic); err != nil { + return err + } else if !admitted { + ClearRouteStatus(kclient, route, ri.RouterName) + } + } + } + } + return nil +} + +// isRouteAdmittedByIngressController returns a boolean if the route is admitted by the ingress +// controller as determined by routeSelectors and namespaceSelectors on the ingress controller. +// Note: This is not using route status to determine admitted (see IsRouteStatusAdmitted) +func isRouteAdmittedByIngressController(kclient client.Reader, route *routev1.Route, ic *operatorv1.IngressController) (bool, error) { + if route == nil { + return false, fmt.Errorf("failed to check if route is admitted: route is nil") + } + if ic == nil { + return false, fmt.Errorf("failed to check if route is admitted: ingress controller is nil") + } + // First check if the route's labels match the RouteSelector on the Ingress Controller. + if ic.Spec.RouteSelector != nil { + routeSelector, err := metav1.LabelSelectorAsSelector(ic.Spec.RouteSelector) + if err != nil { + return false, fmt.Errorf("ingresscontroller %s has an invalid route selector: %w", ic.Name, err) + } + if !routeSelector.Matches(labels.Set(route.Labels)) { + return false, nil + } + } + + // Next let's check if the route's namespace labels match the NamespaceSelector on the Ingress Controller. + if ic.Spec.NamespaceSelector != nil { + ns := &corev1.Namespace{} + nsName := types.NamespacedName{Name: route.Namespace} + if err := kclient.Get(context.Background(), nsName, ns); err != nil { + return false, fmt.Errorf("failed to get namespace %q: %v", nsName, err) + } + namespaceSelector, err := metav1.LabelSelectorAsSelector(ic.Spec.NamespaceSelector) + if err != nil { + return false, fmt.Errorf("ingresscontroller %s has an invalid namespace selector: %w", ic.Name, err) + } + if !namespaceSelector.Matches(labels.Set(ns.Labels)) { + return false, nil + } + } + + return true, nil +} + +// IsRouteStatusAdmitted returns true if a given route's status shows admitted by the Ingress Controller. +func IsRouteStatusAdmitted(route routev1.Route, ingressControllerName string) bool { + // Iterate through the related Ingress Controllers. + for _, ingress := range route.Status.Ingress { + // Check if the RouterName matches the name of the Ingress Controller. + if ingress.RouterName == ingressControllerName { + // Check if the Route was admitted by the Ingress Controller. + for _, cond := range ingress.Conditions { + if cond.Type == routev1.RouteAdmitted && cond.Status == corev1.ConditionTrue { + return true + } + } + return false + } + } + return false +} + +// GetNamespacesSelectedByIngressController gets a set of strings that represents the namespaces that were selected by +// the ingress controller's namespacesSelector. If ingress controller's namespace selector is empty, then it returns all. +func GetNamespacesSelectedByIngressController(kclient client.Reader, ic *operatorv1.IngressController) (sets.String, error) { + if ic == nil { + return nil, fmt.Errorf("failed to get selected namespaces: ingress controller is nil") + } + // List all the Namespaces filtered by our ingress's Namespace selector. + namespaceMatchingLabelsSelector := client.MatchingLabelsSelector{Selector: labels.Everything()} + if ic.Spec.NamespaceSelector != nil { + namespaceSelector, err := metav1.LabelSelectorAsSelector(ic.Spec.NamespaceSelector) + if err != nil { + return nil, fmt.Errorf("ingresscontroller %s has an invalid namespace selector %q: %w", + ic.Name, ic.Spec.NamespaceSelector, err) + } + namespaceMatchingLabelsSelector = client.MatchingLabelsSelector{Selector: namespaceSelector} + } + filteredNamespaceList := &corev1.NamespaceList{} + if err := kclient.List(context.TODO(), filteredNamespaceList, namespaceMatchingLabelsSelector); err != nil { + return nil, fmt.Errorf("failed to list all namespaces in order to clear route status for %s: %w", ic.Name, err) + } + // Create a set of namespaces to easily look up namespaces in this shard. + namespacesInShard := sets.NewString() + for i := range filteredNamespaceList.Items { + namespacesInShard.Insert(filteredNamespaceList.Items[i].Name) + } + return namespacesInShard, nil +} + +// GetRoutesSelectedByIngressController gets route list that represents the routes that were selected by +// the ingress controller's routeSelector. If ingress controller's route selector is empty, then it returns all. +func GetRoutesSelectedByIngressController(kclient client.Reader, ic *operatorv1.IngressController) (routev1.RouteList, error) { + if ic == nil { + return routev1.RouteList{}, fmt.Errorf("failed to get selected routes: ingress controller is nil") + } + // List routes filtered by our ingress's route selector. + routeMatchingLabelsSelector := client.MatchingLabelsSelector{Selector: labels.Everything()} + if ic.Spec.RouteSelector != nil { + routeSelector, err := metav1.LabelSelectorAsSelector(ic.Spec.RouteSelector) + if err != nil { + return routev1.RouteList{}, fmt.Errorf("ingresscontroller %s has an invalid route selector %q: %w", + ic.Name, ic.Spec.RouteSelector, err) + } + routeMatchingLabelsSelector = client.MatchingLabelsSelector{Selector: routeSelector} + } + routeList := routev1.RouteList{} + if err := kclient.List(context.Background(), &routeList, routeMatchingLabelsSelector); err != nil { + return routev1.RouteList{}, fmt.Errorf("failed to list routes for the ingresscontroller %s: %w", ic.Name, err) + } + return routeList, nil +} + // findCondition locates the first condition that corresponds to the requested type. func findCondition(ingress *routev1.RouteIngress, t routev1.RouteIngressConditionType) *routev1.RouteIngressCondition { for i := range ingress.Conditions { diff --git a/pkg/operator/controller/route-status/route_status_test.go b/pkg/operator/controller/route-status/route_status_test.go new file mode 100644 index 0000000000..c71e0f5e5a --- /dev/null +++ b/pkg/operator/controller/route-status/route_status_test.go @@ -0,0 +1,469 @@ +package routestatus + +import ( + "context" + "testing" + + operatorv1 "github.com/openshift/api/operator/v1" + v1 "github.com/openshift/api/operator/v1" + routev1 "github.com/openshift/api/route/v1" + operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" + "github.com/openshift/cluster-ingress-operator/test/unit" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// Test_ClearAllRoutesStatusForIngressController verifies that ClearAllRoutesStatusForIngressController behaves correctly. +func Test_ClearAllRoutesStatusForIngressController(t *testing.T) { + testCases := []struct { + name string + routes routev1.RouteList + ingressController *v1.IngressController + expectedRoutes routev1.RouteList + expectedErr bool + }{ + { + name: "clear ingress controller foo status that admitted route test", + routes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithAdmittedStatuses("test", "foo-ic", "bar-ic"), + *newRouteWithAdmittedStatuses("test2", "baz-ic", "bar-ic"), + }, + }, + ingressController: &operatorv1.IngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo-ic", + Namespace: operatorcontroller.DefaultOperatorNamespace, + }, + }, + expectedRoutes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithAdmittedStatuses("test", "bar-ic"), + *newRouteWithAdmittedStatuses("test2", "baz-ic", "bar-ic"), + }, + }, + }, + { + name: "don't clear ingress controller foo status that didn't admit anything", + routes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithAdmittedStatuses("test", "dog-ic", "bar-ic"), + *newRouteWithAdmittedStatuses("test2", "baz-ic", "bar-ic"), + }, + }, + ingressController: &operatorv1.IngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo-ic", + Namespace: operatorcontroller.DefaultOperatorNamespace, + }, + }, + expectedRoutes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithAdmittedStatuses("test", "dog-ic", "bar-ic"), + *newRouteWithAdmittedStatuses("test2", "baz-ic", "bar-ic"), + }, + }, + }, + { + name: "clear ingress controller foo status that admitted everything", + routes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithAdmittedStatuses("test", "foo-ic"), + *newRouteWithAdmittedStatuses("test2", "foo-ic"), + }, + }, + ingressController: &operatorv1.IngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo-ic", + Namespace: operatorcontroller.DefaultOperatorNamespace, + }, + }, + expectedRoutes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithAdmittedStatuses("test"), + *newRouteWithAdmittedStatuses("test2"), + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Load up client with test objects. + client := unit.NewFakeClient(tc.ingressController) + for _, route := range tc.routes.Items { + if err := client.Create(context.Background(), &route); err != nil { + t.Errorf("error creating route: %v", err) + } + } + + if errs := ClearAllRoutesStatusForIngressController(client, tc.ingressController.Name); tc.expectedErr && len(errs) == 0 { + t.Errorf("expected errors, got no errors") + } else if !tc.expectedErr && len(errs) != 0 { + t.Errorf("did not expected errors: %v", errs) + } + + actualRoutes := routev1.RouteList{} + if err := client.List(context.Background(), &actualRoutes); err == nil { + for _, expectedRoute := range tc.expectedRoutes.Items { + // Find the actual route that we should compare status with this expected route + var actualRoute routev1.Route + for _, route := range actualRoutes.Items { + if route.Name == expectedRoute.Name { + actualRoute = route + } + } + assert.Equal(t, expectedRoute.Status, actualRoute.Status, "route name", expectedRoute.Name) + } + } else { + t.Errorf("error retrieving routes from client: %v", err) + } + }) + } +} + +// Test_ClearRoutesNotAdmittedByIngress verifies that ClearRoutesNotAdmittedByIngress behaves correctly. +func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { + testCases := []struct { + name string + routes routev1.RouteList + namespace corev1.NamespaceList + ingressController *v1.IngressController + expectedRoutes routev1.RouteList + expectedErr bool + }{ + { + name: "don't clear anything: all IC admissions are valid", + routes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "shard-label", "baz-ic", "foo-ic", "bar-ic"), + }, + }, + namespace: corev1.NamespaceList{ + Items: []corev1.Namespace{ + *newNamespace("ns", "shard-label"), + }, + }, + ingressController: newIngressControllerWithSelectors("foo-ic", "shard-label", "", "shard-label", ""), + expectedRoutes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "shard-label", "baz-ic", "foo-ic", "bar-ic"), + }, + }, + }, + { + name: "clear route that is no longer admitted by ingress controller by routeSelectors", + routes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "foo-ic", "bar-ic"), + }, + }, + namespace: corev1.NamespaceList{ + Items: []corev1.Namespace{ + *newNamespace("ns", ""), + }, + }, + ingressController: newIngressControllerWithSelectors("foo-ic", "shard-label", "", "", ""), + expectedRoutes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "bar-ic"), + }, + }, + }, + { + name: "clear route that is no longer admitted by ingress controller by routeSelectors expression", + routes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "foo-ic", "bar-ic"), + }, + }, + namespace: corev1.NamespaceList{ + Items: []corev1.Namespace{ + *newNamespace("ns", ""), + }, + }, + ingressController: newIngressControllerWithSelectors("foo-ic", "", "shard-label", "", ""), + expectedRoutes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "bar-ic"), + }, + }, + }, + { + name: "clear route that is no longer admitted by ingress controller by namespaceSelectors", + routes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns2", "", "baz-ic", "foo-ic", "bar-ic"), + }, + }, + namespace: corev1.NamespaceList{ + Items: []corev1.Namespace{ + *newNamespace("ns", "shard-label"), + *newNamespace("ns2", ""), + }, + }, + ingressController: newIngressControllerWithSelectors("foo-ic", "shard-label", "", "shard-label", ""), + expectedRoutes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "bar-ic"), + }, + }, + }, + { + name: "clear route that is no longer admitted by ingress controller by namespaceSelectors expression", + routes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns2", "", "baz-ic", "foo-ic", "bar-ic"), + }, + }, + namespace: corev1.NamespaceList{ + Items: []corev1.Namespace{ + *newNamespace("ns", "shard-label"), + *newNamespace("ns2", ""), + }, + }, + ingressController: newIngressControllerWithSelectors("foo-ic", "shard-label", "", "", "shard-label"), + expectedRoutes: routev1.RouteList{ + Items: []routev1.Route{ + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "bar-ic"), + }, + }, + }, + { + name: "nil ingress controller", + routes: routev1.RouteList{}, + namespace: corev1.NamespaceList{}, + ingressController: nil, + expectedRoutes: routev1.RouteList{}, + expectedErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Load up client with test objects. + client := unit.NewFakeClient() + if tc.ingressController != nil { + if err := client.Create(context.Background(), tc.ingressController); err != nil { + t.Errorf("error ingress controller route: %v", err) + } + } + for _, route := range tc.routes.Items { + if err := client.Create(context.Background(), &route); err != nil { + t.Errorf("error creating route: %v", err) + } + } + for _, ns := range tc.namespace.Items { + if err := client.Create(context.Background(), &ns); err != nil { + t.Errorf("error creating ns: %v", err) + } + } + + if errs := ClearRoutesNotAdmittedByIngress(client, tc.ingressController); tc.expectedErr && len(errs) == 0 { + t.Errorf("expected errors, got no errors") + } else if !tc.expectedErr && len(errs) != 0 { + t.Errorf("did not expected errors: %v", errs) + } + + actualRoutes := routev1.RouteList{} + if err := client.List(context.Background(), &actualRoutes); err == nil { + for _, expectedRoute := range tc.expectedRoutes.Items { + // Find the actual route that we should compare status with this expected route + var actualRoute routev1.Route + for _, route := range actualRoutes.Items { + if route.Name == expectedRoute.Name { + actualRoute = route + } + } + assert.Equal(t, expectedRoute.Status, actualRoute.Status, "route name", expectedRoute.Name) + } + } else { + t.Errorf("error retrieving all from client: %v", err) + } + }) + } +} + +// Test_IsRouteStatusAdmitted verifies that IsRouteStatusAdmitted behaves correctly. +func Test_IsRouteStatusAdmitted(t *testing.T) { + testCases := []struct { + name string + route routev1.Route + ingressControllerName string + expectedResult bool + }{ + { + name: "route admitted by default", + route: *newRouteWithAdmittedStatuses("foo-route", "default-ic"), + ingressControllerName: "default-ic", + expectedResult: true, + }, + { + name: "route not admitted by sharded", + route: routev1.Route{ + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + RouterName: "sharded", + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: corev1.ConditionFalse, + }, + }, + }, + }, + }, + }, + ingressControllerName: "sharded", + expectedResult: false, + }, + { + name: "route admitted by default, not admitted by sharded", + route: *newRouteWithAdmittedStatuses("foo-route", "default-ic"), + ingressControllerName: "sharded", + expectedResult: false, + }, + { + name: "route not admitted by sharded without Conditions", + route: routev1.Route{ + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + RouterName: "sharded", + }, + }, + }, + }, + ingressControllerName: "sharded", + expectedResult: false, + }, + { + name: "route not admitted by any shard", + route: routev1.Route{ + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{}, + }, + }, + ingressControllerName: "default-ic", + expectedResult: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if actualResult := IsRouteStatusAdmitted(tc.route, tc.ingressControllerName); actualResult != tc.expectedResult { + t.Errorf("expected result %v, got %v", tc.expectedResult, actualResult) + } + }) + } +} + +// newIngressControllerWithSelectors returns a new ingresscontroller with the specified name, +// routeSelectors, and namespaceSelectors based on the parameters. +func newIngressControllerWithSelectors(name, routeMatchLabel, routeMatchExpression, namespaceMatchLabel, namespaceMatchExpression string) *operatorv1.IngressController { + ingresscontroller := operatorv1.IngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: operatorcontroller.DefaultOperatorNamespace, + }, + } + if len(routeMatchLabel) != 0 { + ingresscontroller.Spec.RouteSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "type": routeMatchLabel, + }, + } + } + if len(routeMatchExpression) != 0 { + ingresscontroller.Spec.RouteSelector = &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "type", + Operator: metav1.LabelSelectorOpIn, + Values: []string{routeMatchExpression}, + }, + }, + } + } + if len(namespaceMatchLabel) != 0 { + ingresscontroller.Spec.NamespaceSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "type": namespaceMatchLabel, + }, + } + } + if len(namespaceMatchExpression) != 0 { + ingresscontroller.Spec.NamespaceSelector = &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "type", + Operator: metav1.LabelSelectorOpIn, + Values: []string{namespaceMatchExpression}, + }, + }, + } + } + return &ingresscontroller +} + +// newNamespace returns a new namespace with the specified name +// and if label exists, with that label +func newNamespace(name, label string) *corev1.Namespace { + namespace := corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + } + if len(label) != 0 { + namespace.ObjectMeta.Labels = map[string]string{"type": label} + } + return &namespace +} + +// newRouteWithAdmittedStatuses returns a new route that is admitted by ingress controllers. +func newRouteWithLabelWithAdmittedStatuses(name, namespace, label string, icAdmitted ...string) *routev1.Route { + route := newRouteWithAdmittedStatuses(name, icAdmitted...) + route.ObjectMeta.Namespace = namespace + if len(label) != 0 { + route.Labels = map[string]string{ + "type": label, + } + } + return route +} + +// newRouteWithAdmittedStatuses returns a new route that is admitted by ingress controllers. +func newRouteWithAdmittedStatuses(name string, icAdmitted ...string) *routev1.Route { + route := routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + } + if len(icAdmitted) != 0 { + for _, ic := range icAdmitted { + route.Status.Ingress = append(route.Status.Ingress, routev1.RouteIngress{ + RouterName: ic, + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: "True", + }, + }, + }) + } + } + return &route +} diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index 2bf20963a0..297359e04d 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -7,8 +7,6 @@ import ( "github.com/openshift/library-go/pkg/operator/v1helpers" - routemetricscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/route-metrics" - errorpageconfigmapcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/sync-http-error-code-configmap" "github.com/openshift/library-go/pkg/operator/onepodpernodeccontroller" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/informers" @@ -31,7 +29,10 @@ import ( ingress "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress" ingresscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress" ingressclasscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingressclass" + routemetricscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/route-metrics" + routestatuscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/route-status" statuscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/status" + errorpageconfigmapcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/sync-http-error-code-configmap" "github.com/openshift/library-go/pkg/operator/events" "k8s.io/apimachinery/pkg/api/errors" @@ -209,11 +210,27 @@ func New(config operatorconfig.Config, kubeConfig *rest.Config) (*Operator, erro } } + // Create a new cache to watch on Route objects from every namespace for the route metrics and the route status + // controller to share. + globalCache, err := cache.New(mgr.GetConfig(), cache.Options{ + Scheme: mgr.GetScheme(), + }) + if err != nil { + return nil, err + } + // Add the cache to the manager so that the cache is started along with the other runnables. + mgr.Add(globalCache) + // Set up the route metrics controller. - if _, err := routemetricscontroller.New(mgr, config.Namespace); err != nil { + if _, err := routemetricscontroller.New(mgr, config.Namespace, globalCache); err != nil { return nil, fmt.Errorf("failed to create route metrics controller: %w", err) } + // Set up the route status controller. + if _, err := routestatuscontroller.New(mgr, config.Namespace, globalCache); err != nil { + return nil, fmt.Errorf("failed to create route status controller: %w", err) + } + return &Operator{ manager: mgr, // TODO: These are only needed for the default ingress controller stuff, which diff --git a/test/e2e/all_test.go b/test/e2e/all_test.go index 2f70de3d7e..a18ab3fc90 100644 --- a/test/e2e/all_test.go +++ b/test/e2e/all_test.go @@ -74,6 +74,8 @@ func TestAll(t *testing.T) { t.Run("TestRouteMetricsControllerOnlyRouteSelector", TestRouteMetricsControllerOnlyRouteSelector) t.Run("TestRouteMetricsControllerOnlyNamespaceSelector", TestRouteMetricsControllerOnlyNamespaceSelector) t.Run("TestRouteMetricsControllerRouteAndNamespaceSelector", TestRouteMetricsControllerRouteAndNamespaceSelector) + t.Run("TestNamespaceLabelShouldClearRouteStatus", TestNamespaceLabelShouldClearRouteStatus) + t.Run("TestRouteLabelShouldClearRouteStatus", TestRouteLabelShouldClearRouteStatus) }) t.Run("serial", func(t *testing.T) { diff --git a/test/e2e/router_status_test.go b/test/e2e/router_status_test.go index 460943f72d..209e6e500c 100644 --- a/test/e2e/router_status_test.go +++ b/test/e2e/router_status_test.go @@ -16,6 +16,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/apiserver/pkg/storage/names" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -28,7 +29,7 @@ var ( func TestDeleteIngressControllerShouldClearRouteStatus(t *testing.T) { t.Parallel() // Create an ingress controller that can admit our route. - icName := types.NamespacedName{Namespace: operatorNamespace, Name: "ic-delete-test"} + icName := types.NamespacedName{Namespace: operatorNamespace, Name: names.SimpleNameGenerator.GenerateName("e2e-ic-routestatus-delete-ic-")} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) ic.Spec.RouteSelector = &metav1.LabelSelector{ @@ -43,7 +44,7 @@ func TestDeleteIngressControllerShouldClearRouteStatus(t *testing.T) { // Create a route to be admitted by this ingress controller. // Use openshift-console namespace to get a namespace outside the ingress-operator's cache. - routeName := types.NamespacedName{Namespace: "openshift-console", Name: "route-" + icName.Name} + routeName := types.NamespacedName{Namespace: "openshift-console", Name: names.SimpleNameGenerator.GenerateName("route-")} route := newRouteWithLabel(routeName, icName.Name) if err := kclient.Create(context.TODO(), route); err != nil { t.Fatalf("failed to create route: %v", err) @@ -73,12 +74,12 @@ func TestDeleteIngressControllerShouldClearRouteStatus(t *testing.T) { func TestIngressControllerRouteSelectorUpdateShouldClearRouteStatus(t *testing.T) { t.Parallel() // Create an ingress controller that can admit our route. - icName := types.NamespacedName{Namespace: operatorNamespace, Name: "ic-route-selector-test"} + icName := types.NamespacedName{Namespace: operatorNamespace, Name: names.SimpleNameGenerator.GenerateName("e2e-ic-routestatus-route-selector-")} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) ic.Spec.RouteSelector = &metav1.LabelSelector{ MatchLabels: map[string]string{ - "type": "foo", + "type": "foo-" + icName.Name, }, } @@ -90,8 +91,8 @@ func TestIngressControllerRouteSelectorUpdateShouldClearRouteStatus(t *testing.T // Create a route to be immediately admitted by this ingress controller and then when the IC label selectors are // updated, the status should clear. // Use openshift-console namespace to get a namespace outside the ingress-operator's cache. - routeFooLabelName := types.NamespacedName{Namespace: "openshift-console", Name: "route-foo-label"} - routeFooLabel := newRouteWithLabel(routeFooLabelName, "foo") + routeFooLabelName := types.NamespacedName{Namespace: "openshift-console", Name: names.SimpleNameGenerator.GenerateName("route-foo-")} + routeFooLabel := newRouteWithLabel(routeFooLabelName, "foo-"+icName.Name) if err := kclient.Create(context.TODO(), routeFooLabel); err != nil { t.Fatalf("failed to create route: %v", err) } @@ -104,8 +105,8 @@ func TestIngressControllerRouteSelectorUpdateShouldClearRouteStatus(t *testing.T // Create a route that will NOT be immediately admitted by the ingress controller, but will be admitted AFTER // the IC selectors are updated. The status SHOULD be successfully admitted. // Use openshift-console namespace to get a namespace outside the ingress-operator's cache. - routeBarLabelName := types.NamespacedName{Namespace: "openshift-console", Name: "route-bar-label"} - routeBarLabel := newRouteWithLabel(routeBarLabelName, "bar") + routeBarLabelName := types.NamespacedName{Namespace: "openshift-console", Name: names.SimpleNameGenerator.GenerateName("route-bar-")} + routeBarLabel := newRouteWithLabel(routeBarLabelName, "bar-"+icName.Name) if err := kclient.Create(context.TODO(), routeBarLabel); err != nil { t.Fatalf("failed to create route: %v", err) } @@ -127,7 +128,7 @@ func TestIngressControllerRouteSelectorUpdateShouldClearRouteStatus(t *testing.T } ic.Spec.RouteSelector = &metav1.LabelSelector{ MatchLabels: map[string]string{ - "type": "bar", + "type": "bar-" + icName.Name, }, } if err := kclient.Update(context.TODO(), ic); err != nil { @@ -148,12 +149,12 @@ func TestIngressControllerRouteSelectorUpdateShouldClearRouteStatus(t *testing.T func TestIngressControllerNamespaceSelectorUpdateShouldClearRouteStatus(t *testing.T) { t.Parallel() // Create an ingress controller that can admit our route. - icName := types.NamespacedName{Namespace: operatorNamespace, Name: "ic-namespace-selector-test"} + icName := types.NamespacedName{Namespace: operatorNamespace, Name: names.SimpleNameGenerator.GenerateName("e2e-ic-routestatus-namespace-selector-")} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) ic.Spec.NamespaceSelector = &metav1.LabelSelector{ MatchLabels: map[string]string{ - "type": "foo", + "type": "foo-" + icName.Name, }, } @@ -165,9 +166,9 @@ func TestIngressControllerNamespaceSelectorUpdateShouldClearRouteStatus(t *testi // Create a new namespace for the route that we can immediately match with the IC's namespace selector. nsFoo := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo-namespace-selector-test", + Name: names.SimpleNameGenerator.GenerateName("foo-namespace-selector-test-"), Labels: map[string]string{ - "type": "foo", + "type": "foo-" + icName.Name, }, }, } @@ -183,9 +184,9 @@ func TestIngressControllerNamespaceSelectorUpdateShouldClearRouteStatus(t *testi // Create a new namespace for the route that we can NOT immediately match with the IC's namespace selector. nsBar := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "bar-namespace-selector-test", + Name: names.SimpleNameGenerator.GenerateName("bar-namespace-selector-test-"), Labels: map[string]string{ - "type": "bar", + "type": "bar-" + icName.Name, }, }, } @@ -200,7 +201,7 @@ func TestIngressControllerNamespaceSelectorUpdateShouldClearRouteStatus(t *testi // Create a route to be immediately admitted by this ingress controller and then when the IC label selectors are // updated, the status should clear. - routeFooLabelName := types.NamespacedName{Namespace: nsFoo.Name, Name: "route-foo-label"} + routeFooLabelName := types.NamespacedName{Namespace: nsFoo.Name, Name: names.SimpleNameGenerator.GenerateName("route-foo-")} routeFooLabel := newRouteWithLabel(routeFooLabelName, "") if err := kclient.Create(context.TODO(), routeFooLabel); err != nil { t.Fatalf("failed to create route: %v", err) @@ -213,8 +214,8 @@ func TestIngressControllerNamespaceSelectorUpdateShouldClearRouteStatus(t *testi // Create a route that will NOT be immediately admitted by the ingress controller, but will be admitted AFTER // the IC selectors are updated. The status SHOULD be successfully admitted. - routeBarLabelName := types.NamespacedName{Namespace: nsBar.Name, Name: "route-bar-label"} - routeBarLabel := newRouteWithLabel(routeBarLabelName, "bar") + routeBarLabelName := types.NamespacedName{Namespace: nsBar.Name, Name: names.SimpleNameGenerator.GenerateName("route-bar-")} + routeBarLabel := newRouteWithLabel(routeBarLabelName, "bar-"+icName.Name) if err := kclient.Create(context.TODO(), routeBarLabel); err != nil { t.Fatalf("failed to create route: %v", err) } @@ -241,7 +242,7 @@ func TestIngressControllerNamespaceSelectorUpdateShouldClearRouteStatus(t *testi } ic.Spec.NamespaceSelector = &metav1.LabelSelector{ MatchLabels: map[string]string{ - "type": "bar", + "type": "bar-" + icName.Name, }, } if err := kclient.Update(context.TODO(), ic); err != nil { @@ -259,6 +260,139 @@ func TestIngressControllerNamespaceSelectorUpdateShouldClearRouteStatus(t *testi } } +func TestNamespaceLabelShouldClearRouteStatus(t *testing.T) { + t.Parallel() + + // Create an ingress controller that can admit our route. + icName := types.NamespacedName{Namespace: operatorNamespace, Name: names.SimpleNameGenerator.GenerateName("e2e-ic-routestatus-namespace-label-")} + domain := icName.Name + "." + dnsConfig.Spec.BaseDomain + ic := newPrivateController(icName, domain) + ic.Spec.NamespaceSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "type": "foo-" + icName.Name, + }, + } + + if err := kclient.Create(context.TODO(), ic); err != nil { + t.Fatalf("failed to create ingresscontroller: %v", err) + } + defer assertIngressControllerDeleted(t, kclient, ic) + + // Create a new namespace for the Route. + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: names.SimpleNameGenerator.GenerateName("test-route-namespace-label-"), + Labels: map[string]string{ + "type": "foo-" + icName.Name, + }, + }, + } + if err := kclient.Create(context.TODO(), ns); err != nil { + t.Fatalf("failed to create namespace: %v", err) + } + defer func() { + if err := kclient.Delete(context.TODO(), ns); err != nil { + t.Fatalf("failed to delete test namespace %v: %v", ns.Name, err) + } + }() + + // Create a route to be immediately admitted by this ingress controller. + routeName := types.NamespacedName{Namespace: ns.Name, Name: names.SimpleNameGenerator.GenerateName("route-")} + route := newRouteWithLabel(routeName, "") + if err := kclient.Create(context.TODO(), route); err != nil { + t.Fatalf("failed to create route: %v", err) + } + defer func() { + if err := kclient.Delete(context.TODO(), route); err != nil { + t.Fatalf("failed to delete route %s: %v", routeName, err) + } + }() + + // Wait for route to be admitted upon creation. + if err := waitForRouteIngressConditions(t, kclient, routeName, ic.Name, admittedCondition); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } + + // Update namespace to not match the namespaceSelector. + if err := kclient.Get(context.TODO(), types.NamespacedName{Name: ns.Name}, ns); err != nil { + t.Fatalf("failed to get namespace: %v", err) + } + ns.Labels = map[string]string{} + if err := kclient.Update(context.TODO(), ns); err != nil { + t.Fatalf("failed to update namespace: %v", err) + } + + // Ensure route clears status. + if err := waitForRouteStatusClear(t, kclient, routeName, ic.Name); err != nil { + t.Fatalf("failed to observe route has cleared status: %v", err) + } +} + +func TestRouteLabelShouldClearRouteStatus(t *testing.T) { + t.Parallel() + + // Create an ingress controller that can admit our route. + icName := types.NamespacedName{Namespace: operatorNamespace, Name: names.SimpleNameGenerator.GenerateName("e2e-ic-routestatus-route-label-")} + domain := icName.Name + "." + dnsConfig.Spec.BaseDomain + ic := newPrivateController(icName, domain) + ic.Spec.RouteSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "type": "foo-" + icName.Name, + }, + } + + if err := kclient.Create(context.TODO(), ic); err != nil { + t.Fatalf("failed to create ingresscontroller: %v", err) + } + defer assertIngressControllerDeleted(t, kclient, ic) + + // Create a new namespace for the Route. + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: names.SimpleNameGenerator.GenerateName("test-route-label-"), + }, + } + if err := kclient.Create(context.TODO(), ns); err != nil { + t.Fatalf("failed to create namespace: %v", err) + } + defer func() { + if err := kclient.Delete(context.TODO(), ns); err != nil { + t.Fatalf("failed to delete test namespace %v: %v", ns.Name, err) + } + }() + + // Create a route to be immediately admitted by this ingress controller. + routeName := types.NamespacedName{Namespace: ns.Name, Name: names.SimpleNameGenerator.GenerateName("route-")} + route := newRouteWithLabel(routeName, "foo-"+icName.Name) + if err := kclient.Create(context.TODO(), route); err != nil { + t.Fatalf("failed to create route: %v", err) + } + defer func() { + if err := kclient.Delete(context.TODO(), route); err != nil { + t.Fatalf("failed to delete route %s: %v", routeName, err) + } + }() + + // Wait for route to be admitted upon creation. + if err := waitForRouteIngressConditions(t, kclient, routeName, ic.Name, admittedCondition); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } + + // Update namespace to not match the namespaceSelector. + if err := kclient.Get(context.TODO(), routeName, route); err != nil { + t.Fatalf("failed to get route: %v", err) + } + route.Labels = map[string]string{} + if err := kclient.Update(context.TODO(), route); err != nil { + t.Fatalf("failed to update route: %v", err) + } + + // Ensure route clears status. + if err := waitForRouteStatusClear(t, kclient, routeName, ic.Name); err != nil { + t.Fatalf("failed to observe route has cleared status: %v", err) + } +} + // waitForRouteStatusClear waits for route to be unadmitted (e.g. admitted status cleared) by given ingress controller. func waitForRouteStatusClear(t *testing.T, cl client.Client, routeName types.NamespacedName, routerName string) error { t.Helper() diff --git a/test/unit/fake_client.go b/test/unit/fake_client.go new file mode 100644 index 0000000000..b758bfd295 --- /dev/null +++ b/test/unit/fake_client.go @@ -0,0 +1,46 @@ +package unit + +import ( + operatorv1 "github.com/openshift/api/operator/v1" + routev1 "github.com/openshift/api/route/v1" + + "k8s.io/client-go/kubernetes/scheme" + + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/cache/informertest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// Fake Cache struct that implements the cache.Cache interface. +type fakeCache struct { + cache.Informers + client.Reader +} + +// NewFakeClientBuilder creates a new fake client builder with the schema installed to support cluster-ingress-operator +// unit testing. +func NewFakeClientBuilder() *fake.ClientBuilder { + clientBuilder := fake.NewClientBuilder() + s := scheme.Scheme + routev1.Install(s) + operatorv1.Install(s) + + return clientBuilder +} + +// NewFakeClient creates a fake client for cluster-ingress-operator unit testing. +func NewFakeClient(initObjs ...client.Object) client.Client { + clientBuilder := NewFakeClientBuilder() + clientBuilder.WithObjects(initObjs...) + return clientBuilder.Build() +} + +// NewFakeCache creates a fake cache object that abides by the controller runtime cache interface so that it can be +// populated into a reconciler object. The cache is essentially just the fake client with a fake informer. +func NewFakeCache(client client.Client) fakeCache { + informer := informertest.FakeInformers{ + Scheme: client.Scheme(), + } + return fakeCache{Informers: &informer, Reader: client} +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 5bae3439bb..5f2e67859d 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1062,6 +1062,7 @@ k8s.io/utils/trace # sigs.k8s.io/controller-runtime v0.13.0 ## explicit; go 1.17 sigs.k8s.io/controller-runtime/pkg/cache +sigs.k8s.io/controller-runtime/pkg/cache/informertest sigs.k8s.io/controller-runtime/pkg/cache/internal sigs.k8s.io/controller-runtime/pkg/certwatcher sigs.k8s.io/controller-runtime/pkg/certwatcher/metrics @@ -1073,6 +1074,7 @@ sigs.k8s.io/controller-runtime/pkg/cluster sigs.k8s.io/controller-runtime/pkg/config sigs.k8s.io/controller-runtime/pkg/config/v1alpha1 sigs.k8s.io/controller-runtime/pkg/controller +sigs.k8s.io/controller-runtime/pkg/controller/controllertest sigs.k8s.io/controller-runtime/pkg/event sigs.k8s.io/controller-runtime/pkg/handler sigs.k8s.io/controller-runtime/pkg/healthz diff --git a/vendor/sigs.k8s.io/controller-runtime/pkg/cache/informertest/fake_cache.go b/vendor/sigs.k8s.io/controller-runtime/pkg/cache/informertest/fake_cache.go new file mode 100644 index 0000000000..da3bf8e0d4 --- /dev/null +++ b/vendor/sigs.k8s.io/controller-runtime/pkg/cache/informertest/fake_cache.go @@ -0,0 +1,141 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package informertest + +import ( + "context" + + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/kubernetes/scheme" + toolscache "k8s.io/client-go/tools/cache" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllertest" +) + +var _ cache.Cache = &FakeInformers{} + +// FakeInformers is a fake implementation of Informers. +type FakeInformers struct { + InformersByGVK map[schema.GroupVersionKind]toolscache.SharedIndexInformer + Scheme *runtime.Scheme + Error error + Synced *bool +} + +// GetInformerForKind implements Informers. +func (c *FakeInformers) GetInformerForKind(ctx context.Context, gvk schema.GroupVersionKind) (cache.Informer, error) { + if c.Scheme == nil { + c.Scheme = scheme.Scheme + } + obj, err := c.Scheme.New(gvk) + if err != nil { + return nil, err + } + return c.informerFor(gvk, obj) +} + +// FakeInformerForKind implements Informers. +func (c *FakeInformers) FakeInformerForKind(ctx context.Context, gvk schema.GroupVersionKind) (*controllertest.FakeInformer, error) { + if c.Scheme == nil { + c.Scheme = scheme.Scheme + } + obj, err := c.Scheme.New(gvk) + if err != nil { + return nil, err + } + i, err := c.informerFor(gvk, obj) + if err != nil { + return nil, err + } + return i.(*controllertest.FakeInformer), nil +} + +// GetInformer implements Informers. +func (c *FakeInformers) GetInformer(ctx context.Context, obj client.Object) (cache.Informer, error) { + if c.Scheme == nil { + c.Scheme = scheme.Scheme + } + gvks, _, err := c.Scheme.ObjectKinds(obj) + if err != nil { + return nil, err + } + gvk := gvks[0] + return c.informerFor(gvk, obj) +} + +// WaitForCacheSync implements Informers. +func (c *FakeInformers) WaitForCacheSync(ctx context.Context) bool { + if c.Synced == nil { + return true + } + return *c.Synced +} + +// FakeInformerFor implements Informers. +func (c *FakeInformers) FakeInformerFor(obj runtime.Object) (*controllertest.FakeInformer, error) { + if c.Scheme == nil { + c.Scheme = scheme.Scheme + } + gvks, _, err := c.Scheme.ObjectKinds(obj) + if err != nil { + return nil, err + } + gvk := gvks[0] + i, err := c.informerFor(gvk, obj) + if err != nil { + return nil, err + } + return i.(*controllertest.FakeInformer), nil +} + +func (c *FakeInformers) informerFor(gvk schema.GroupVersionKind, _ runtime.Object) (toolscache.SharedIndexInformer, error) { + if c.Error != nil { + return nil, c.Error + } + if c.InformersByGVK == nil { + c.InformersByGVK = map[schema.GroupVersionKind]toolscache.SharedIndexInformer{} + } + informer, ok := c.InformersByGVK[gvk] + if ok { + return informer, nil + } + + c.InformersByGVK[gvk] = &controllertest.FakeInformer{} + return c.InformersByGVK[gvk], nil +} + +// Start implements Informers. +func (c *FakeInformers) Start(ctx context.Context) error { + return c.Error +} + +// IndexField implements Cache. +func (c *FakeInformers) IndexField(ctx context.Context, obj client.Object, field string, extractValue client.IndexerFunc) error { + return nil +} + +// Get implements Cache. +func (c *FakeInformers) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + return nil +} + +// List implements Cache. +func (c *FakeInformers) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + return nil +} diff --git a/vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/doc.go b/vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/doc.go new file mode 100644 index 0000000000..91c5a3e35e --- /dev/null +++ b/vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/doc.go @@ -0,0 +1,20 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package controllertest contains fake informers for testing controllers +// When in doubt, it's almost always better to test against a real API server +// using envtest.Environment. +package controllertest diff --git a/vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/testing.go b/vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/testing.go new file mode 100644 index 0000000000..b9f97d5289 --- /dev/null +++ b/vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/testing.go @@ -0,0 +1,62 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controllertest + +import ( + "time" + + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/util/workqueue" +) + +var _ runtime.Object = &ErrorType{} + +// ErrorType implements runtime.Object but isn't registered in any scheme and should cause errors in tests as a result. +type ErrorType struct{} + +// GetObjectKind implements runtime.Object. +func (ErrorType) GetObjectKind() schema.ObjectKind { return nil } + +// DeepCopyObject implements runtime.Object. +func (ErrorType) DeepCopyObject() runtime.Object { return nil } + +var _ workqueue.RateLimitingInterface = Queue{} + +// Queue implements a RateLimiting queue as a non-ratelimited queue for testing. +// This helps testing by having functions that use a RateLimiting queue synchronously add items to the queue. +type Queue struct { + workqueue.Interface +} + +// AddAfter implements RateLimitingInterface. +func (q Queue) AddAfter(item interface{}, duration time.Duration) { + q.Add(item) +} + +// AddRateLimited implements RateLimitingInterface. TODO(community): Implement this. +func (q Queue) AddRateLimited(item interface{}) { + q.Add(item) +} + +// Forget implements RateLimitingInterface. TODO(community): Implement this. +func (q Queue) Forget(item interface{}) {} + +// NumRequeues implements RateLimitingInterface. TODO(community): Implement this. +func (q Queue) NumRequeues(item interface{}) int { + return 0 +} diff --git a/vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/unconventionallisttypecrd.go b/vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/unconventionallisttypecrd.go new file mode 100644 index 0000000000..d0f5017154 --- /dev/null +++ b/vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/unconventionallisttypecrd.go @@ -0,0 +1,76 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controllertest + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +var _ runtime.Object = &UnconventionalListType{} +var _ runtime.Object = &UnconventionalListTypeList{} + +// UnconventionalListType is used to test CRDs with List types that +// have a slice of pointers rather than a slice of literals. +type UnconventionalListType struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + Spec string `json:"spec,omitempty"` +} + +// DeepCopyObject implements runtime.Object +// Handwritten for simplicity. +func (u *UnconventionalListType) DeepCopyObject() runtime.Object { + return u.DeepCopy() +} + +// DeepCopy implements *UnconventionalListType +// Handwritten for simplicity. +func (u *UnconventionalListType) DeepCopy() *UnconventionalListType { + return &UnconventionalListType{ + TypeMeta: u.TypeMeta, + ObjectMeta: *u.ObjectMeta.DeepCopy(), + Spec: u.Spec, + } +} + +// UnconventionalListTypeList is used to test CRDs with List types that +// have a slice of pointers rather than a slice of literals. +type UnconventionalListTypeList struct { + metav1.TypeMeta `json:",inline"` + metav1.ListMeta `json:"metadata,omitempty"` + Items []*UnconventionalListType `json:"items"` +} + +// DeepCopyObject implements runtime.Object +// Handwritten for simplicity. +func (u *UnconventionalListTypeList) DeepCopyObject() runtime.Object { + return u.DeepCopy() +} + +// DeepCopy implements *UnconventionalListTypeListt +// Handwritten for simplicity. +func (u *UnconventionalListTypeList) DeepCopy() *UnconventionalListTypeList { + out := &UnconventionalListTypeList{ + TypeMeta: u.TypeMeta, + ListMeta: *u.ListMeta.DeepCopy(), + } + for _, item := range u.Items { + out.Items = append(out.Items, item.DeepCopy()) + } + return out +} diff --git a/vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/util.go b/vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/util.go new file mode 100644 index 0000000000..b638b4976c --- /dev/null +++ b/vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllertest/util.go @@ -0,0 +1,118 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controllertest + +import ( + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/cache" +) + +var _ cache.SharedIndexInformer = &FakeInformer{} + +// FakeInformer provides fake Informer functionality for testing. +type FakeInformer struct { + // Synced is returned by the HasSynced functions to implement the Informer interface + Synced bool + + // RunCount is incremented each time RunInformersAndControllers is called + RunCount int + + handlers []cache.ResourceEventHandler +} + +// AddIndexers does nothing. TODO(community): Implement this. +func (f *FakeInformer) AddIndexers(indexers cache.Indexers) error { + return nil +} + +// GetIndexer does nothing. TODO(community): Implement this. +func (f *FakeInformer) GetIndexer() cache.Indexer { + return nil +} + +// Informer returns the fake Informer. +func (f *FakeInformer) Informer() cache.SharedIndexInformer { + return f +} + +// HasSynced implements the Informer interface. Returns f.Synced. +func (f *FakeInformer) HasSynced() bool { + return f.Synced +} + +// AddEventHandler implements the Informer interface. Adds an EventHandler to the fake Informers. +func (f *FakeInformer) AddEventHandler(handler cache.ResourceEventHandler) { + f.handlers = append(f.handlers, handler) +} + +// Run implements the Informer interface. Increments f.RunCount. +func (f *FakeInformer) Run(<-chan struct{}) { + f.RunCount++ +} + +// Add fakes an Add event for obj. +func (f *FakeInformer) Add(obj metav1.Object) { + for _, h := range f.handlers { + h.OnAdd(obj) + } +} + +// Update fakes an Update event for obj. +func (f *FakeInformer) Update(oldObj, newObj metav1.Object) { + for _, h := range f.handlers { + h.OnUpdate(oldObj, newObj) + } +} + +// Delete fakes an Delete event for obj. +func (f *FakeInformer) Delete(obj metav1.Object) { + for _, h := range f.handlers { + h.OnDelete(obj) + } +} + +// AddEventHandlerWithResyncPeriod does nothing. TODO(community): Implement this. +func (f *FakeInformer) AddEventHandlerWithResyncPeriod(handler cache.ResourceEventHandler, resyncPeriod time.Duration) { + +} + +// GetStore does nothing. TODO(community): Implement this. +func (f *FakeInformer) GetStore() cache.Store { + return nil +} + +// GetController does nothing. TODO(community): Implement this. +func (f *FakeInformer) GetController() cache.Controller { + return nil +} + +// LastSyncResourceVersion does nothing. TODO(community): Implement this. +func (f *FakeInformer) LastSyncResourceVersion() string { + return "" +} + +// SetWatchErrorHandler does nothing. TODO(community): Implement this. +func (f *FakeInformer) SetWatchErrorHandler(cache.WatchErrorHandler) error { + return nil +} + +// SetTransform does nothing. TODO(community): Implement this. +func (f *FakeInformer) SetTransform(t cache.TransformFunc) error { + return nil +} From 2eead9f07221fe958b413afb18ee7c5062104f49 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Fri, 20 Jan 2023 11:03:32 -0500 Subject: [PATCH 3/9] Update TestRouteMetricsController*Selector-type E2E tests to be more resilent on updates `test/e2e/route_metrics_test.go`: - Added update*WithRetryOnConflict-type functions to ensure resilient updates. Route status has more churn due to recent route-status controller addition. - Added test for removing and adding namespace labels, which was previously left out due to limitation, but now has been fixed. `test/e2e/util_test.go`: - Added `updateRouteLabelsWithRetryOnConflict` and `updateNamespaceLabelsWithRetryOnConflict` --- test/e2e/route_metrics_test.go | 233 +++++++++++++++++---------------- test/e2e/util_test.go | 48 +++++++ 2 files changed, 167 insertions(+), 114 deletions(-) diff --git a/test/e2e/route_metrics_test.go b/test/e2e/route_metrics_test.go index 9aad89a8ee..48eb05ddf7 100644 --- a/test/e2e/route_metrics_test.go +++ b/test/e2e/route_metrics_test.go @@ -160,9 +160,10 @@ func testRouteMetricsControllerLabelSelector(t *testing.T, testRS, testNS bool) } // Create a new namespace for the Route. + nsName := types.NamespacedName{Name: names.SimpleNameGenerator.GenerateName("test-e2e-metrics-")} ns := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: names.SimpleNameGenerator.GenerateName("test-e2e-metrics-"), + Name: nsName.Name, }, } @@ -224,121 +225,151 @@ func testRouteMetricsControllerLabelSelector(t *testing.T, testRS, testNS bool) } if testNS { - // Fetch the latest version of the Ingress Controller resource. - if err := kclient.Get(context.TODO(), icName, ic); err != nil { - t.Fatalf("failed to get ingress resource: %v", err) - } // Update the NamespaceSelector of the Ingress Controller so that the Route gets un-admitted. - ic.Spec.NamespaceSelector = &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "label": incorrectLabel, - }, - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "expression", - Operator: metav1.LabelSelectorOpIn, - Values: []string{incorrectLabel}, + if err := updateIngressControllerSpecWithRetryOnConflict(t, icName, 5*time.Minute, func(ic *operatorv1.IngressControllerSpec) { + ic.NamespaceSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "label": incorrectLabel, }, - }, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "expression", + Operator: metav1.LabelSelectorOpIn, + Values: []string{incorrectLabel}, + }, + }, + } + }); err != nil { + t.Fatalf("failed to update ingresscontroller %s: %v", icName, err) } - // Update the NamespaceSelector of the Ingress Controller and wait for metrics to be updated to 0 as - // the Route will get un-admitted by the IC. - updateICAndWaitForMetricsUpdate(t, ic, prometheusClient, 0) - - // Fetch the latest version of the Ingress Controller resource. - if err := kclient.Get(context.TODO(), icName, ic); err != nil { - t.Fatalf("failed to get ingress resource: %v", err) + // Wait for metrics to be updated to 0 as the Route will get un-admitted by the IC. + if err := waitForRouteMetricsAddorUpdate(t, prometheusClient, ic.Name, 0); err != nil { + t.Fatalf("failed to fetch expected metrics: %v", err) } + // Update the NamespaceSelector of the Ingress Controller so that the Route gets admitted again. - ic.Spec.NamespaceSelector = &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "label": correctLabel, - }, - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "expression", - Operator: metav1.LabelSelectorOpIn, - Values: []string{correctLabel}, + if err := updateIngressControllerSpecWithRetryOnConflict(t, icName, 5*time.Minute, func(ic *operatorv1.IngressControllerSpec) { + ic.NamespaceSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "label": correctLabel, }, - }, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "expression", + Operator: metav1.LabelSelectorOpIn, + Values: []string{correctLabel}, + }, + }, + } + }); err != nil { + t.Fatalf("failed to update ingresscontroller %s: %v", icName, err) } - // Update the NamespaceSelector of the Ingress Controller and wait for metrics to be updated to 1 as - // the Route will get admitted by the IC again. - updateICAndWaitForMetricsUpdate(t, ic, prometheusClient, 1) + // Wait for metrics to be updated to 1 as the Route will get admitted by the IC again. + if err := waitForRouteMetricsAddorUpdate(t, prometheusClient, ic.Name, 1); err != nil { + t.Fatalf("failed to fetch expected metrics: %v", err) + } + + // Update the label of the namespace so that Route gets un-admitted from the Ingress Controller. + if err := updateNamespaceLabelsWithRetryOnConflict(t, nsName, 5*time.Minute, func(labels map[string]string) { + labels["label"] = incorrectLabel + labels["expression"] = incorrectLabel + }); err != nil { + t.Fatalf("failed to update namespace %s: %v", routeFooLabelName, err) + } + + // Wait for metrics to be updated to 0 as the Route will get un-admitted by the IC. + if err := waitForRouteMetricsAddorUpdate(t, prometheusClient, ic.Name, 0); err != nil { + t.Fatalf("failed to fetch expected metrics: %v", err) + } + + // Update the label of the namespace so that Route gets admitted from the Ingress Controller. + if err := updateNamespaceLabelsWithRetryOnConflict(t, nsName, 5*time.Minute, func(labels map[string]string) { + labels["label"] = correctLabel + labels["expression"] = correctLabel + }); err != nil { + t.Fatalf("failed to update namespace %s: %v", routeFooLabelName, err) + } + + // Wait for metrics to be updated to 1 as the Route will get admitted by the IC again. + if err := waitForRouteMetricsAddorUpdate(t, prometheusClient, ic.Name, 1); err != nil { + t.Fatalf("failed to fetch expected metrics: %v", err) + } } if testRS { - // Fetch the latest version of the Ingress Controller resource. - if err := kclient.Get(context.TODO(), icName, ic); err != nil { - t.Fatalf("failed to get ingress resource: %v", err) - } // Update the RouteSelector of the Ingress Controller so that the Route gets un-admitted again. - ic.Spec.RouteSelector = &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "label": incorrectLabel, - }, - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "expression", - Operator: metav1.LabelSelectorOpIn, - Values: []string{incorrectLabel}, + if err := updateIngressControllerSpecWithRetryOnConflict(t, icName, 5*time.Minute, func(ic *operatorv1.IngressControllerSpec) { + ic.RouteSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "label": incorrectLabel, }, - }, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "expression", + Operator: metav1.LabelSelectorOpIn, + Values: []string{incorrectLabel}, + }, + }, + } + }); err != nil { + t.Fatalf("failed to update ingresscontroller %s: %v", icName, err) } - // Update the RouteSelector of the Ingress Controller and wait for metrics to be updated to 1 as - // the Route will get un-admitted by the IC again. - updateICAndWaitForMetricsUpdate(t, ic, prometheusClient, 0) - - // Fetch the latest version of the Ingress Controller resource. - if err := kclient.Get(context.TODO(), icName, ic); err != nil { - t.Fatalf("failed to get ingress resource: %v", err) + // Wait for metrics to be updated to 0 as the Route will get un-admitted by the IC again. + if err := waitForRouteMetricsAddorUpdate(t, prometheusClient, ic.Name, 0); err != nil { + t.Fatalf("failed to fetch expected metrics: %v", err) } + // Update the RouteSelector of the Ingress Controller so that the Route gets admitted again. - ic.Spec.RouteSelector = &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "label": correctLabel, - }, - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "expression", - Operator: metav1.LabelSelectorOpIn, - Values: []string{correctLabel}, + if err := updateIngressControllerSpecWithRetryOnConflict(t, icName, 5*time.Minute, func(ic *operatorv1.IngressControllerSpec) { + ic.RouteSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "label": correctLabel, }, - }, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "expression", + Operator: metav1.LabelSelectorOpIn, + Values: []string{correctLabel}, + }, + }, + } + }); err != nil { + t.Fatalf("failed to update ingresscontroller %s: %v", icName, err) } - // Update the RouteSelector of the Ingress Controller and wait for metrics to be updated to 1 as - // the Route will get admitted by the IC again. - updateICAndWaitForMetricsUpdate(t, ic, prometheusClient, 1) - - // Fetch the latest version of the Route resource. - if err := kclient.Get(context.TODO(), routeFooLabelName, routeFooLabel); err != nil { - t.Fatalf("failed to get route resource: %v", err) + // Wait for metrics to be updated to 1 as the Route will get admitted by the IC again. + if err := waitForRouteMetricsAddorUpdate(t, prometheusClient, ic.Name, 1); err != nil { + t.Fatalf("failed to fetch expected metrics: %v", err) } + // Update the label of the route so that it gets un-admitted from the Ingress Controller. - routeFooLabel.Labels = map[string]string{ - "label": incorrectLabel, - "expression": incorrectLabel, + if err := updateRouteLabelsWithRetryOnConflict(t, routeFooLabelName, 5*time.Minute, func(labels map[string]string) { + labels["label"] = incorrectLabel + labels["expression"] = incorrectLabel + }); err != nil { + t.Fatalf("failed to update route %s: %v", routeFooLabelName, err) } - // Update the label of the route and wait for metrics to be updated to 0 as the Route will get un-admitted by the IC. - updateRouteAndWaitForMetricsUpdate(t, routeFooLabel, prometheusClient, ic.Name, 0) - - // Fetch the latest version of the Route resource. - if err := kclient.Get(context.TODO(), routeFooLabelName, routeFooLabel); err != nil { - t.Fatalf("failed to get route resource: %v", err) + // Wait for metrics to be updated to 0 as the Route will get un-admitted by the IC. + if err := waitForRouteMetricsAddorUpdate(t, prometheusClient, ic.Name, 0); err != nil { + t.Fatalf("failed to fetch expected metrics: %v", err) } - // Update the label of the route so that it gets admitted to the Ingress Controller again. - routeFooLabel.Labels = map[string]string{ - "label": correctLabel, - "expression": correctLabel, + + // Update the label of the route so that it gets admitted by the Ingress Controller. + if err := updateRouteLabelsWithRetryOnConflict(t, routeFooLabelName, 5*time.Minute, func(labels map[string]string) { + labels["label"] = correctLabel + labels["expression"] = correctLabel + }); err != nil { + t.Fatalf("failed to update route %s: %v", routeFooLabelName, err) } - // Update the Route label and wait for metrics to be updated to 1 as the Route will get admitted by the IC again. - updateRouteAndWaitForMetricsUpdate(t, routeFooLabel, prometheusClient, ic.Name, 1) + // Wait for metrics to be updated to 1 as the Route will get admitted by the IC again. + if err := waitForRouteMetricsAddorUpdate(t, prometheusClient, ic.Name, 1); err != nil { + t.Fatalf("failed to fetch expected metrics: %v", err) + } } // Delete the Route routeFooLabel. @@ -366,32 +397,6 @@ func testRouteMetricsControllerLabelSelector(t *testing.T, testRS, testNS bool) } } -// updateICAndWaitForMetricsUpdate updates the Ingress Controller and waits for metric to be updated to the expected value. -func updateICAndWaitForMetricsUpdate(t *testing.T, ic *operatorv1.IngressController, prometheusClient prometheusv1.API, value int) { - // Update the Ingress Controller resource. - if err := kclient.Update(context.TODO(), ic); err != nil { - t.Fatalf("failed to update ingresscontroller: %v", err) - } - - // Wait for metrics to be updated to the expected value. - if err := waitForRouteMetricsAddorUpdate(t, prometheusClient, ic.Name, value); err != nil { - t.Fatalf("failed to fetch expected metrics: %v", err) - } -} - -// updateRouteAndWaitForMetricsUpdate updates the Route and waits for metric to be updated to the expected value. -func updateRouteAndWaitForMetricsUpdate(t *testing.T, route *routev1.Route, prometheusClient prometheusv1.API, shardName string, value int) { - // Update the Route resource. - if err := kclient.Update(context.TODO(), route); err != nil { - t.Fatalf("failed to update route: %v", err) - } - - // Wait for metrics to be updated to the expected value. - if err := waitForRouteMetricsAddorUpdate(t, prometheusClient, shardName, value); err != nil { - t.Fatalf("failed to fetch expected metrics: %v", err) - } -} - // waitForRouteMetricsAddorUpdate waits for the metrics for the corresponding shard to be added or updated to the expected value. func waitForRouteMetricsAddorUpdate(t *testing.T, prometheusClient prometheusv1.API, shardName string, value int) error { t.Logf("waiting for route_metrics_controller_routes_per_shard{shard_name=%s} to become %d", shardName, value) diff --git a/test/e2e/util_test.go b/test/e2e/util_test.go index 2bd4b0fd9c..b70b9d2942 100644 --- a/test/e2e/util_test.go +++ b/test/e2e/util_test.go @@ -389,6 +389,54 @@ func updateIngressControllerSpecWithRetryOnConflict(t *testing.T, name types.Nam }) } +// updateRouteLabelsWithRetryOnConflict gets a fresh copy of +// the named route, calls mutateSpecFn() where callers can +// modify the labels, and then updates the route. If there +// is a conflict error on update then the complete sequence +// of get, mutate, and update is retried until timeout is reached. +func updateRouteLabelsWithRetryOnConflict(t *testing.T, name types.NamespacedName, timeout time.Duration, mutateSpecFn func(map[string]string)) error { + route := routev1.Route{} + return wait.PollImmediate(1*time.Second, timeout, func() (bool, error) { + if err := kclient.Get(context.TODO(), name, &route); err != nil { + t.Logf("error getting route %v: %v, retrying...", name, err) + return false, nil + } + mutateSpecFn(route.Labels) + if err := kclient.Update(context.TODO(), &route); err != nil { + if errors.IsConflict(err) { + t.Logf("conflict when updating route %v: %v, retrying...", name, err) + return false, nil + } + return false, err + } + return true, nil + }) +} + +// updateNamespaceLabelsWithRetryOnConflict gets a fresh copy of +// the named namespace, calls mutateSpecFn() where callers can +// modify the labels, and then updates the namespace. If there +// is a conflict error on update then the complete sequence +// of get, mutate, and update is retried until timeout is reached. +func updateNamespaceLabelsWithRetryOnConflict(t *testing.T, name types.NamespacedName, timeout time.Duration, mutateSpecFn func(map[string]string)) error { + ns := corev1.Namespace{} + return wait.PollImmediate(1*time.Second, timeout, func() (bool, error) { + if err := kclient.Get(context.TODO(), name, &ns); err != nil { + t.Logf("error getting namespace %v: %v, retrying...", name, err) + return false, nil + } + mutateSpecFn(ns.Labels) + if err := kclient.Update(context.TODO(), &ns); err != nil { + if errors.IsConflict(err) { + t.Logf("conflict when updating namespace %v: %v, retrying...", name, err) + return false, nil + } + return false, err + } + return true, nil + }) +} + // updateIngressConfigSpecWithRetryOnConflict gets a fresh copy of the // name ingress config, calls updateSpecFn() where callers can modify // fields of the spec, and then updates the ingress config object. If From eacbbc5118cea3ac822e064b3cffc70acf09c587 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Fri, 17 Feb 2023 11:25:02 -0500 Subject: [PATCH 4/9] Fix ClearRoutesNotAdmittedByIngress always clearing status if routeSelector was Nil - `pkg/operator/controller/route-status/route_status.go`: Update ClearRoutesNotAdmittedByIngress to match everything if routeSelector is Nil - `pkg/operator/controller/route-status/route_status_test.go`: Update unit test to expose the issue --- pkg/operator/controller/route-status/route_status.go | 12 ++++++++---- .../controller/route-status/route_status_test.go | 12 ++++++------ 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/pkg/operator/controller/route-status/route_status.go b/pkg/operator/controller/route-status/route_status.go index 81558cd306..916e03cfd8 100644 --- a/pkg/operator/controller/route-status/route_status.go +++ b/pkg/operator/controller/route-status/route_status.go @@ -116,9 +116,13 @@ func ClearRoutesNotAdmittedByIngress(kclient client.Client, ingress *operatorv1. } // List routes filtered by our ingress's route selector. - routeSelector, err := metav1.LabelSelectorAsSelector(ingress.Spec.RouteSelector) - if err != nil { - return append(errs, fmt.Errorf("ingresscontroller %s has an invalid route selector: %w", ingress.Name, err)) + routeMatchingLabelsSelector := client.MatchingLabelsSelector{Selector: labels.Everything()} + if ingress.Spec.RouteSelector != nil { + routeSelector, err := metav1.LabelSelectorAsSelector(ingress.Spec.RouteSelector) + if err != nil { + return append(errs, fmt.Errorf("ingresscontroller %s has an invalid route selector: %w", ingress.Name, err)) + } + routeMatchingLabelsSelector = client.MatchingLabelsSelector{Selector: routeSelector} } // Iterate over the entire route list and clear if either the route selector OR the namespace selector does not @@ -127,7 +131,7 @@ func ClearRoutesNotAdmittedByIngress(kclient client.Client, ingress *operatorv1. for i := range routeList.Items { route := &routeList.Items[i] - routeInShard := routeSelector.Matches(labels.Set(route.Labels)) + routeInShard := routeMatchingLabelsSelector.Matches(labels.Set(route.Labels)) namespaceInShard := namespacesInShard.Has(route.Namespace) if !routeInShard || !namespaceInShard { diff --git a/pkg/operator/controller/route-status/route_status_test.go b/pkg/operator/controller/route-status/route_status_test.go index c71e0f5e5a..76908968df 100644 --- a/pkg/operator/controller/route-status/route_status_test.go +++ b/pkg/operator/controller/route-status/route_status_test.go @@ -201,7 +201,7 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { name: "clear route that is no longer admitted by ingress controller by namespaceSelectors", routes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "", "foo-ic", "bar-ic"), *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns2", "", "baz-ic", "foo-ic", "bar-ic"), }, }, @@ -211,10 +211,10 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { *newNamespace("ns2", ""), }, }, - ingressController: newIngressControllerWithSelectors("foo-ic", "shard-label", "", "shard-label", ""), + ingressController: newIngressControllerWithSelectors("foo-ic", "", "", "shard-label", ""), expectedRoutes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "", "foo-ic", "bar-ic"), *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "bar-ic"), }, }, @@ -223,7 +223,7 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { name: "clear route that is no longer admitted by ingress controller by namespaceSelectors expression", routes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "", "foo-ic", "bar-ic"), *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns2", "", "baz-ic", "foo-ic", "bar-ic"), }, }, @@ -233,10 +233,10 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { *newNamespace("ns2", ""), }, }, - ingressController: newIngressControllerWithSelectors("foo-ic", "shard-label", "", "", "shard-label"), + ingressController: newIngressControllerWithSelectors("foo-ic", "", "", "", "shard-label"), expectedRoutes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "", "foo-ic", "bar-ic"), *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "bar-ic"), }, }, From b2c735188137a3b44a1a0a8a18b40d33bf945188 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Fri, 17 Feb 2023 11:37:29 -0500 Subject: [PATCH 5/9] Update route status functions to accept context as an argument --- pkg/operator/controller/ingress/controller.go | 4 +- .../controller/route-metrics/controller.go | 4 +- .../controller/route-status/controller.go | 2 +- .../controller/route-status/route_status.go | 38 +++++++++---------- .../route-status/route_status_test.go | 4 +- 5 files changed, 26 insertions(+), 26 deletions(-) diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index 1cf56cf14b..5d4b6e1030 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -919,7 +919,7 @@ func (r *reconciler) ensureIngressDeleted(ingress *operatorv1.IngressController) } else if allDeleted { // Deployment has been deleted and there are no more pods left. // Clear all routes status for this ingress controller. - statusErrs := routestatus.ClearAllRoutesStatusForIngressController(r.client, ingress.ObjectMeta.Name) + statusErrs := routestatus.ClearAllRoutesStatusForIngressController(context.TODO(), r.client, ingress.ObjectMeta.Name) errs = append(errs, statusErrs...) } else { errs = append(errs, retryable.New(fmt.Errorf("not all router pods have been deleted for %s/%s", ingress.Namespace, ingress.Name), 15*time.Second)) @@ -1171,7 +1171,7 @@ func (r *reconciler) syncRouteStatusWithSelectorChange(ic *operatorv1.IngressCon return []error{err} } else if done { // Clear routes status not admitted by this ingress controller. - if errs := routestatus.ClearRoutesNotAdmittedByIngress(r.client, ic); len(errs) > 0 { + if errs := routestatus.ClearRoutesNotAdmittedByIngress(context.TODO(), r.client, ic); len(errs) > 0 { return errs } diff --git a/pkg/operator/controller/route-metrics/controller.go b/pkg/operator/controller/route-metrics/controller.go index 9beb987178..5c649c26ab 100644 --- a/pkg/operator/controller/route-metrics/controller.go +++ b/pkg/operator/controller/route-metrics/controller.go @@ -164,13 +164,13 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( } // List all the Namespaces filtered by our ingress's Namespace selector. - namespacesInShard, err := routestatus.GetNamespacesSelectedByIngressController(r.cache, ingressController) + namespacesInShard, err := routestatus.GetNamespacesSelectedByIngressController(ctx, r.cache, ingressController) if err != nil { return reconcile.Result{}, err } // List all the Namespaces filtered by our ingress's Namespace selector. - routesInShard, err := routestatus.GetRoutesSelectedByIngressController(r.cache, ingressController) + routesInShard, err := routestatus.GetRoutesSelectedByIngressController(ctx, r.cache, ingressController) if err != nil { return reconcile.Result{}, err } diff --git a/pkg/operator/controller/route-status/controller.go b/pkg/operator/controller/route-status/controller.go index e677f8ee84..d5c113089e 100644 --- a/pkg/operator/controller/route-status/controller.go +++ b/pkg/operator/controller/route-status/controller.go @@ -102,7 +102,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( } // Validate and clean up route status for given route if any status is stale. - if err := clearStaleRouteAdmittedStatus(r.client, route); err != nil { + if err := clearStaleRouteAdmittedStatus(ctx, r.client, route); err != nil { return reconcile.Result{}, err } diff --git a/pkg/operator/controller/route-status/route_status.go b/pkg/operator/controller/route-status/route_status.go index 916e03cfd8..4b05d94b16 100644 --- a/pkg/operator/controller/route-status/route_status.go +++ b/pkg/operator/controller/route-status/route_status.go @@ -43,18 +43,18 @@ import ( // ClearAllRoutesStatusForIngressController clears any route status that has been admitted by the ingress controller, // regardless if it is actually admitted or not. This function should be used for deletions of ingress controllers. -func ClearAllRoutesStatusForIngressController(kclient client.Client, icName string) []error { +func ClearAllRoutesStatusForIngressController(ctx context.Context, kclient client.Client, icName string) []error { // List all routes. errs := []error{} start := time.Now() routeList := &routev1.RouteList{} routesCleared := 0 - if err := kclient.List(context.TODO(), routeList); err != nil { + if err := kclient.List(ctx, routeList); err != nil { return append(errs, fmt.Errorf("failed to list all routes in order to clear route status for deployment %s: %w", icName, err)) } // Clear status on the routes that belonged to icName. for i := range routeList.Items { - if cleared, err := ClearRouteStatus(kclient, &routeList.Items[i], icName); err != nil { + if cleared, err := ClearRouteStatus(ctx, kclient, &routeList.Items[i], icName); err != nil { errs = append(errs, err) } else if cleared { routesCleared++ @@ -68,7 +68,7 @@ func ClearAllRoutesStatusForIngressController(kclient client.Client, icName stri } // ClearRouteStatus clears a route's status that is admitted by a specific ingress controller. -func ClearRouteStatus(kclient client.Client, route *routev1.Route, icName string) (bool, error) { +func ClearRouteStatus(ctx context.Context, kclient client.Client, route *routev1.Route, icName string) (bool, error) { if route == nil { return false, fmt.Errorf("failed to clear route status: route is nil") } @@ -80,7 +80,7 @@ func ClearRouteStatus(kclient client.Client, route *routev1.Route, icName string // Remove this status since it matches our routerName. route.DeepCopyInto(&updated) updated.Status.Ingress = append(route.Status.Ingress[:i], route.Status.Ingress[i+1:]...) - if err := kclient.Status().Update(context.TODO(), &updated); err != nil { + if err := kclient.Status().Update(ctx, &updated); err != nil { return false, fmt.Errorf("failed to clear route status of %s/%s for routerName %s: %w", route.Namespace, route.Name, icName, err) } @@ -95,7 +95,7 @@ func ClearRouteStatus(kclient client.Client, route *routev1.Route, icName string } // ClearRoutesNotAdmittedByIngress clears routes status that are not selected by a specific ingress controller. -func ClearRoutesNotAdmittedByIngress(kclient client.Client, ingress *operatorv1.IngressController) []error { +func ClearRoutesNotAdmittedByIngress(ctx context.Context, kclient client.Client, ingress *operatorv1.IngressController) []error { var errs []error if ingress == nil { return append(errs, fmt.Errorf("ingress controller is nil")) @@ -105,12 +105,12 @@ func ClearRoutesNotAdmittedByIngress(kclient client.Client, ingress *operatorv1. // List all routes. routeList := &routev1.RouteList{} - if err := kclient.List(context.TODO(), routeList); err != nil { + if err := kclient.List(ctx, routeList); err != nil { return append(errs, fmt.Errorf("failed to list all routes in order to clear route status: %w", err)) } // List all the Namespaces filtered by our ingress's Namespace selector. - namespacesInShard, err := GetNamespacesSelectedByIngressController(kclient, ingress) + namespacesInShard, err := GetNamespacesSelectedByIngressController(ctx, kclient, ingress) if err != nil { return append(errs, err) } @@ -135,7 +135,7 @@ func ClearRoutesNotAdmittedByIngress(kclient client.Client, ingress *operatorv1. namespaceInShard := namespacesInShard.Has(route.Namespace) if !routeInShard || !namespaceInShard { - if cleared, err := ClearRouteStatus(kclient, route, ingress.ObjectMeta.Name); err != nil { + if cleared, err := ClearRouteStatus(ctx, kclient, route, ingress.ObjectMeta.Name); err != nil { errs = append(errs, err) } else if cleared { routesCleared++ @@ -150,7 +150,7 @@ func ClearRoutesNotAdmittedByIngress(kclient client.Client, ingress *operatorv1. // clearStaleRouteAdmittedStatus cleans up a single route's admitted status by verifying it is actually admitted by // the ingress controller its status claims to be admitted by. -func clearStaleRouteAdmittedStatus(kclient client.Client, route *routev1.Route) error { +func clearStaleRouteAdmittedStatus(ctx context.Context, kclient client.Client, route *routev1.Route) error { if route == nil { return fmt.Errorf("route is nil") } @@ -162,7 +162,7 @@ func clearStaleRouteAdmittedStatus(kclient client.Client, route *routev1.Route) // Get the ingress controller that this route status claims to be admitted by. icName := types.NamespacedName{Name: ri.RouterName, Namespace: operatorcontroller.DefaultOperatorNamespace} ic := &operatorv1.IngressController{} - if err := kclient.Get(context.TODO(), icName, ic); err != nil { + if err := kclient.Get(ctx, icName, ic); err != nil { // If the Ingress Controller doesn't exist at all, skip status clear, as it should have been cleared // upon Ingress Controller deletion. If it made it here, then it's probably a router status test. if kerrors.IsNotFound(err) { @@ -172,10 +172,10 @@ func clearStaleRouteAdmittedStatus(kclient client.Client, route *routev1.Route) } // If it is no longer admitted by this ingress controller, clear the status. - if admitted, err := isRouteAdmittedByIngressController(kclient, route, ic); err != nil { + if admitted, err := isRouteAdmittedByIngressController(ctx, kclient, route, ic); err != nil { return err } else if !admitted { - ClearRouteStatus(kclient, route, ri.RouterName) + ClearRouteStatus(ctx, kclient, route, ri.RouterName) } } } @@ -186,7 +186,7 @@ func clearStaleRouteAdmittedStatus(kclient client.Client, route *routev1.Route) // isRouteAdmittedByIngressController returns a boolean if the route is admitted by the ingress // controller as determined by routeSelectors and namespaceSelectors on the ingress controller. // Note: This is not using route status to determine admitted (see IsRouteStatusAdmitted) -func isRouteAdmittedByIngressController(kclient client.Reader, route *routev1.Route, ic *operatorv1.IngressController) (bool, error) { +func isRouteAdmittedByIngressController(ctx context.Context, kclient client.Reader, route *routev1.Route, ic *operatorv1.IngressController) (bool, error) { if route == nil { return false, fmt.Errorf("failed to check if route is admitted: route is nil") } @@ -208,7 +208,7 @@ func isRouteAdmittedByIngressController(kclient client.Reader, route *routev1.Ro if ic.Spec.NamespaceSelector != nil { ns := &corev1.Namespace{} nsName := types.NamespacedName{Name: route.Namespace} - if err := kclient.Get(context.Background(), nsName, ns); err != nil { + if err := kclient.Get(ctx, nsName, ns); err != nil { return false, fmt.Errorf("failed to get namespace %q: %v", nsName, err) } namespaceSelector, err := metav1.LabelSelectorAsSelector(ic.Spec.NamespaceSelector) @@ -243,7 +243,7 @@ func IsRouteStatusAdmitted(route routev1.Route, ingressControllerName string) bo // GetNamespacesSelectedByIngressController gets a set of strings that represents the namespaces that were selected by // the ingress controller's namespacesSelector. If ingress controller's namespace selector is empty, then it returns all. -func GetNamespacesSelectedByIngressController(kclient client.Reader, ic *operatorv1.IngressController) (sets.String, error) { +func GetNamespacesSelectedByIngressController(ctx context.Context, kclient client.Reader, ic *operatorv1.IngressController) (sets.String, error) { if ic == nil { return nil, fmt.Errorf("failed to get selected namespaces: ingress controller is nil") } @@ -258,7 +258,7 @@ func GetNamespacesSelectedByIngressController(kclient client.Reader, ic *operato namespaceMatchingLabelsSelector = client.MatchingLabelsSelector{Selector: namespaceSelector} } filteredNamespaceList := &corev1.NamespaceList{} - if err := kclient.List(context.TODO(), filteredNamespaceList, namespaceMatchingLabelsSelector); err != nil { + if err := kclient.List(ctx, filteredNamespaceList, namespaceMatchingLabelsSelector); err != nil { return nil, fmt.Errorf("failed to list all namespaces in order to clear route status for %s: %w", ic.Name, err) } // Create a set of namespaces to easily look up namespaces in this shard. @@ -271,7 +271,7 @@ func GetNamespacesSelectedByIngressController(kclient client.Reader, ic *operato // GetRoutesSelectedByIngressController gets route list that represents the routes that were selected by // the ingress controller's routeSelector. If ingress controller's route selector is empty, then it returns all. -func GetRoutesSelectedByIngressController(kclient client.Reader, ic *operatorv1.IngressController) (routev1.RouteList, error) { +func GetRoutesSelectedByIngressController(ctx context.Context, kclient client.Reader, ic *operatorv1.IngressController) (routev1.RouteList, error) { if ic == nil { return routev1.RouteList{}, fmt.Errorf("failed to get selected routes: ingress controller is nil") } @@ -286,7 +286,7 @@ func GetRoutesSelectedByIngressController(kclient client.Reader, ic *operatorv1. routeMatchingLabelsSelector = client.MatchingLabelsSelector{Selector: routeSelector} } routeList := routev1.RouteList{} - if err := kclient.List(context.Background(), &routeList, routeMatchingLabelsSelector); err != nil { + if err := kclient.List(ctx, &routeList, routeMatchingLabelsSelector); err != nil { return routev1.RouteList{}, fmt.Errorf("failed to list routes for the ingresscontroller %s: %w", ic.Name, err) } return routeList, nil diff --git a/pkg/operator/controller/route-status/route_status_test.go b/pkg/operator/controller/route-status/route_status_test.go index 76908968df..5ea14f5dcc 100644 --- a/pkg/operator/controller/route-status/route_status_test.go +++ b/pkg/operator/controller/route-status/route_status_test.go @@ -99,7 +99,7 @@ func Test_ClearAllRoutesStatusForIngressController(t *testing.T) { } } - if errs := ClearAllRoutesStatusForIngressController(client, tc.ingressController.Name); tc.expectedErr && len(errs) == 0 { + if errs := ClearAllRoutesStatusForIngressController(context.Background(), client, tc.ingressController.Name); tc.expectedErr && len(errs) == 0 { t.Errorf("expected errors, got no errors") } else if !tc.expectedErr && len(errs) != 0 { t.Errorf("did not expected errors: %v", errs) @@ -271,7 +271,7 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { } } - if errs := ClearRoutesNotAdmittedByIngress(client, tc.ingressController); tc.expectedErr && len(errs) == 0 { + if errs := ClearRoutesNotAdmittedByIngress(context.Background(), client, tc.ingressController); tc.expectedErr && len(errs) == 0 { t.Errorf("expected errors, got no errors") } else if !tc.expectedErr && len(errs) != 0 { t.Errorf("did not expected errors: %v", errs) From be502863d5cbe841cede90ad2da9bb385a10bd9f Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Fri, 17 Feb 2023 11:44:43 -0500 Subject: [PATCH 6/9] Add InvalidSelectorError error type for distinguishing invalid selector errorsdistinguishing --- .../controller/route-metrics/controller.go | 12 +++++++-- .../controller/route-status/route_status.go | 21 ++++++++++++--- .../route-status/route_status_test.go | 27 +++++++++++++++++++ 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/pkg/operator/controller/route-metrics/controller.go b/pkg/operator/controller/route-metrics/controller.go index 5c649c26ab..d75efcdfec 100644 --- a/pkg/operator/controller/route-metrics/controller.go +++ b/pkg/operator/controller/route-metrics/controller.go @@ -165,13 +165,21 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( // List all the Namespaces filtered by our ingress's Namespace selector. namespacesInShard, err := routestatus.GetNamespacesSelectedByIngressController(ctx, r.cache, ingressController) - if err != nil { + if routestatus.IsInvalidSelectorError(err) { + log.Error(err, "ingresscontroller has an invalid namespace selector", "ingresscontroller", + ingressController.Name, "namespaceSelector", ingressController.Spec.NamespaceSelector) + return reconcile.Result{}, nil + } else if err != nil { return reconcile.Result{}, err } // List all the Namespaces filtered by our ingress's Namespace selector. routesInShard, err := routestatus.GetRoutesSelectedByIngressController(ctx, r.cache, ingressController) - if err != nil { + if routestatus.IsInvalidSelectorError(err) { + log.Error(err, "ingresscontroller has an invalid route selector", "ingresscontroller", + ingressController.Name, "namespaceSelector", ingressController.Spec.RouteSelector) + return reconcile.Result{}, nil + } else if err != nil { return reconcile.Result{}, err } diff --git a/pkg/operator/controller/route-status/route_status.go b/pkg/operator/controller/route-status/route_status.go index 4b05d94b16..38ade01b3b 100644 --- a/pkg/operator/controller/route-status/route_status.go +++ b/pkg/operator/controller/route-status/route_status.go @@ -2,6 +2,7 @@ package routestatus import ( "context" + "errors" "fmt" "time" @@ -41,6 +42,14 @@ import ( // stale route statuses. // - Handled by the route-status control loop. +// InvalidSelectorError is an error type for namespace or route selector errors. +type InvalidSelectorError struct { + msg string // description of error +} + +// Error returns the message describing the invalid selector. +func (e *InvalidSelectorError) Error() string { return e.msg } + // ClearAllRoutesStatusForIngressController clears any route status that has been admitted by the ingress controller, // regardless if it is actually admitted or not. This function should be used for deletions of ingress controllers. func ClearAllRoutesStatusForIngressController(ctx context.Context, kclient client.Client, icName string) []error { @@ -252,8 +261,7 @@ func GetNamespacesSelectedByIngressController(ctx context.Context, kclient clien if ic.Spec.NamespaceSelector != nil { namespaceSelector, err := metav1.LabelSelectorAsSelector(ic.Spec.NamespaceSelector) if err != nil { - return nil, fmt.Errorf("ingresscontroller %s has an invalid namespace selector %q: %w", - ic.Name, ic.Spec.NamespaceSelector, err) + return nil, &InvalidSelectorError{err.Error()} } namespaceMatchingLabelsSelector = client.MatchingLabelsSelector{Selector: namespaceSelector} } @@ -280,8 +288,7 @@ func GetRoutesSelectedByIngressController(ctx context.Context, kclient client.Re if ic.Spec.RouteSelector != nil { routeSelector, err := metav1.LabelSelectorAsSelector(ic.Spec.RouteSelector) if err != nil { - return routev1.RouteList{}, fmt.Errorf("ingresscontroller %s has an invalid route selector %q: %w", - ic.Name, ic.Spec.RouteSelector, err) + return routev1.RouteList{}, &InvalidSelectorError{err.Error()} } routeMatchingLabelsSelector = client.MatchingLabelsSelector{Selector: routeSelector} } @@ -292,6 +299,12 @@ func GetRoutesSelectedByIngressController(ctx context.Context, kclient client.Re return routeList, nil } +// IsInvalidSelectorError determines if an error returned is of type InvalidSelectorError. +func IsInvalidSelectorError(err error) bool { + invalidSelectorError := &InvalidSelectorError{} + return errors.As(err, &invalidSelectorError) +} + // findCondition locates the first condition that corresponds to the requested type. func findCondition(ingress *routev1.RouteIngress, t routev1.RouteIngressConditionType) *routev1.RouteIngressCondition { for i := range ingress.Conditions { diff --git a/pkg/operator/controller/route-status/route_status_test.go b/pkg/operator/controller/route-status/route_status_test.go index 5ea14f5dcc..4a0682f299 100644 --- a/pkg/operator/controller/route-status/route_status_test.go +++ b/pkg/operator/controller/route-status/route_status_test.go @@ -2,6 +2,7 @@ package routestatus import ( "context" + "fmt" "testing" operatorv1 "github.com/openshift/api/operator/v1" @@ -371,6 +372,32 @@ func Test_IsRouteStatusAdmitted(t *testing.T) { } } +// Test_IsInvalidSelectorError verifies that IsInvalidSelectorError behaves correctly. +func Test_IsInvalidSelectorError(t *testing.T) { + testCases := []struct { + name string + error error + expectedInvalidSelectorError bool + }{ + { + name: "Is not InvalidSelectorError", + error: fmt.Errorf("not InvalidSelectorError"), + }, + { + name: "Is InvalidSelectorError", + error: &InvalidSelectorError{"InvalidSelectorError"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if isInvalidSelectorError := IsInvalidSelectorError(tc.error); tc.expectedInvalidSelectorError && !isInvalidSelectorError { + t.Errorf("expected InvalidSelectorError, was not InvalidSelectorError") + } + }) + } +} + // newIngressControllerWithSelectors returns a new ingresscontroller with the specified name, // routeSelectors, and namespaceSelectors based on the parameters. func newIngressControllerWithSelectors(name, routeMatchLabel, routeMatchExpression, namespaceMatchLabel, namespaceMatchExpression string) *operatorv1.IngressController { From a4a87208a9a82e7cacee93bfb2449f983306c5ca Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Fri, 17 Feb 2023 11:49:29 -0500 Subject: [PATCH 7/9] Update clearStaleRouteAdmittedStatus to use cache when possible --- pkg/operator/controller/route-status/controller.go | 2 +- pkg/operator/controller/route-status/route_status.go | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/operator/controller/route-status/controller.go b/pkg/operator/controller/route-status/controller.go index d5c113089e..1ffa0d5e61 100644 --- a/pkg/operator/controller/route-status/controller.go +++ b/pkg/operator/controller/route-status/controller.go @@ -102,7 +102,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( } // Validate and clean up route status for given route if any status is stale. - if err := clearStaleRouteAdmittedStatus(ctx, r.client, route); err != nil { + if err := clearStaleRouteAdmittedStatus(ctx, r.client, r.cache, route); err != nil { return reconcile.Result{}, err } diff --git a/pkg/operator/controller/route-status/route_status.go b/pkg/operator/controller/route-status/route_status.go index 38ade01b3b..4de11e8c81 100644 --- a/pkg/operator/controller/route-status/route_status.go +++ b/pkg/operator/controller/route-status/route_status.go @@ -12,6 +12,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" + "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" operatorv1 "github.com/openshift/api/operator/v1" @@ -159,7 +160,7 @@ func ClearRoutesNotAdmittedByIngress(ctx context.Context, kclient client.Client, // clearStaleRouteAdmittedStatus cleans up a single route's admitted status by verifying it is actually admitted by // the ingress controller its status claims to be admitted by. -func clearStaleRouteAdmittedStatus(ctx context.Context, kclient client.Client, route *routev1.Route) error { +func clearStaleRouteAdmittedStatus(ctx context.Context, kclient client.Client, kcache cache.Cache, route *routev1.Route) error { if route == nil { return fmt.Errorf("route is nil") } @@ -171,7 +172,7 @@ func clearStaleRouteAdmittedStatus(ctx context.Context, kclient client.Client, r // Get the ingress controller that this route status claims to be admitted by. icName := types.NamespacedName{Name: ri.RouterName, Namespace: operatorcontroller.DefaultOperatorNamespace} ic := &operatorv1.IngressController{} - if err := kclient.Get(ctx, icName, ic); err != nil { + if err := kcache.Get(ctx, icName, ic); err != nil { // If the Ingress Controller doesn't exist at all, skip status clear, as it should have been cleared // upon Ingress Controller deletion. If it made it here, then it's probably a router status test. if kerrors.IsNotFound(err) { @@ -181,7 +182,7 @@ func clearStaleRouteAdmittedStatus(ctx context.Context, kclient client.Client, r } // If it is no longer admitted by this ingress controller, clear the status. - if admitted, err := isRouteAdmittedByIngressController(ctx, kclient, route, ic); err != nil { + if admitted, err := isRouteAdmittedByIngressController(ctx, kcache, route, ic); err != nil { return err } else if !admitted { ClearRouteStatus(ctx, kclient, route, ri.RouterName) From 161f28334ba4b472dae3611a42f4722d762aea6a Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Fri, 17 Feb 2023 11:54:26 -0500 Subject: [PATCH 8/9] Unit test cleanup, comment cleanup, and unused variable cleanup --- .../controller/route-status/controller.go | 22 ++- .../route-status/controller_test.go | 160 ++++++++---------- .../route-status/route_status_test.go | 144 +++++++++++----- test/e2e/router_status_test.go | 2 +- 4 files changed, 187 insertions(+), 141 deletions(-) diff --git a/pkg/operator/controller/route-status/controller.go b/pkg/operator/controller/route-status/controller.go index 1ffa0d5e61..83235ada9c 100644 --- a/pkg/operator/controller/route-status/controller.go +++ b/pkg/operator/controller/route-status/controller.go @@ -26,13 +26,12 @@ var ( log = logf.Logger.WithName(controllerName) ) -// New creates the route status controller. This is the controller that handles reconciling route status and keeping -// it in sync. +// New creates the route status controller. This is the controller that handles reconciling route status and removing +// stale status entries when a route is removed from a shard or a shard is deleted. func New(mgr manager.Manager, namespace string, cache cache.Cache) (controller.Controller, error) { reconciler := &reconciler{ - cache: cache, - client: mgr.GetClient(), - namespace: namespace, + cache: cache, + client: mgr.GetClient(), } c, err := controller.New(controllerName, mgr, controller.Options{ Reconciler: reconciler, @@ -60,12 +59,12 @@ func (r *reconciler) namespaceToRoutes(obj client.Object) []reconcile.Request { routeList := routev1.RouteList{} if err := r.cache.List(context.Background(), &routeList, client.InNamespace(ns.Name)); err != nil { - log.Error(err, "failed to list routes for namespace", "related", obj.GetSelfLink()) + log.Error(err, "failed to list routes for namespace") return requests } for _, route := range routeList.Items { - log.Info("queueing route", "name", route.Name, "related", obj.GetSelfLink()) + log.Info("queueing route", "name", route.Name) request := reconcile.Request{ NamespacedName: types.NamespacedName{ Namespace: route.Namespace, @@ -80,13 +79,12 @@ func (r *reconciler) namespaceToRoutes(obj client.Object) []reconcile.Request { // reconciler handles the actual route reconciliation logic in response to events. type reconciler struct { - cache cache.Cache - client client.Client - namespace string + cache cache.Cache + client client.Client } -// Reconcile expects request to refer to a Route object, which will clear route status if it is no longer admitted -// by each ingress controller it claims to be. +// Reconcile expects request to refer to a Route object and clears stale route status entries if the route is no longer +// admitted by each ingress controller it claims to be. func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { log.Info("reconciling", "request", request) diff --git a/pkg/operator/controller/route-status/controller_test.go b/pkg/operator/controller/route-status/controller_test.go index 9ba16031b2..851f0a5e11 100644 --- a/pkg/operator/controller/route-status/controller_test.go +++ b/pkg/operator/controller/route-status/controller_test.go @@ -4,7 +4,6 @@ import ( "context" v12 "github.com/openshift/api/operator/v1" routev1 "github.com/openshift/api/route/v1" - "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" "github.com/openshift/cluster-ingress-operator/test/unit" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" @@ -14,143 +13,127 @@ import ( "testing" ) -// Test_Reconcile verifies Reconcile -// Note: This effectively tests clearStaleRouteAdmittedStatus as well +// Test_Reconcile verifies Reconcile behaves as expected. +// Note: This effectively tests clearStaleRouteAdmittedStatus as well. func Test_Reconcile(t *testing.T) { + routeName := "route" + routeNs := "ns" reconcileRequestRoute := reconcile.Request{ NamespacedName: types.NamespacedName{ - Name: "route", - Namespace: "ns", + Name: routeName, + Namespace: routeNs, }, } testCases := []struct { name string - request reconcile.Request route *routev1.Route ingressController *v12.IngressController namespace *corev1.Namespace expectedRoute *routev1.Route - expectedErr bool }{ { - name: "admitted with no routeSelector and no namespaceSelector with route with no labels in namespace with no labels", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), - namespace: newNamespace("ns", ""), + name: "don't clear admitted status with no routeSelector and no namespaceSelector with route with no labels in namespace with no labels", + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), + namespace: newNamespace(routeNs, ""), ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), }, { - name: "admitted with no routeSelector and no namespaceSelector with route with correct labels in namespace with no labels", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), - namespace: newNamespace("ns", ""), + name: "don't clear admitted status with no routeSelector and no namespaceSelector with route with labels in namespace with no labels", + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), + namespace: newNamespace(routeNs, ""), ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), }, { - name: "admitted with no routeSelector and no namespaceSelector with route with no labels in namespace with correct labels", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), - namespace: newNamespace("ns", "shard-label"), + name: "don't clear admitted status with no routeSelector and no namespaceSelector with route with no labels in namespace with labels", + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), + namespace: newNamespace(routeNs, "shard-label"), ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), }, { - name: "admitted with routeSelector and no namespaceSelector with route with correct labels in namespace with no labels", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "shard-label", "default-ic"), - namespace: newNamespace("ns", ""), + name: "don't clear admitted status with routeSelector and no namespaceSelector with route with matching labels in namespace with no labels", + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), + namespace: newNamespace(routeNs, ""), ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), }, { - name: "not admitted with routeSelector and no namespaceSelector with route with no labels in namespace with no labels", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), - namespace: newNamespace("ns", ""), + name: "clear stale admitted status with routeSelector and no namespaceSelector with route with no labels in namespace with no labels", + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), + namespace: newNamespace(routeNs, ""), ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, ""), }, { - name: "not admitted with routeSelector and no namespaceSelector with route with incorrect labels in namespace with no labels", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "not-shard-label", "default-ic"), - namespace: newNamespace("ns", ""), + name: "clear stale admitted status with routeSelector and no namespaceSelector with route with different labels in namespace with no labels", + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "not-shard-label", "default-ic"), + namespace: newNamespace(routeNs, ""), ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "not-shard-label"), }, { - name: "admitted with routeSelector and namespaceSelector with route with correct labels in namespace with correct labels", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "shard-label", "default-ic"), - namespace: newNamespace("ns", "shard-label"), + name: "don't clear admitted status with routeSelector and namespaceSelector with route with matching labels in namespace with matching labels", + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), + namespace: newNamespace(routeNs, "shard-label"), ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "shard-label", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), }, { - name: "not admitted with routeSelector and namespaceSelector with route with incorrect labels in namespace with correct labels", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "not-shard-label", "default-ic"), - namespace: newNamespace("ns", "shard-label"), + name: "clear stale admitted status with routeSelector and namespaceSelector with route with different labels in namespace with matching labels", + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "not-shard-label", "default-ic"), + namespace: newNamespace(routeNs, "shard-label"), ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "shard-label", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "not-shard-label"), }, { - name: "not admitted with routeSelector and namespaceSelector with route with correct labels in namespace with incorrect labels", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "shard-label", "default-ic"), - namespace: newNamespace("ns", "not-shard-label"), + name: "clear stale admitted status with routeSelector and namespaceSelector with route with matching labels in namespace with different labels", + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), + namespace: newNamespace(routeNs, "not-shard-label"), ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "shard-label", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label"), }, { - name: "admitted with expression routeSelector and no namespaceSelector with route with correct labels in namespace with no labels", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "shard-label", "default-ic"), - namespace: newNamespace("ns", ""), + name: "don't clear admitted status with expression routeSelector and no namespaceSelector with route with matching labels in namespace with no labels", + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), + namespace: newNamespace(routeNs, ""), ingressController: newIngressControllerWithSelectors("default-ic", "", "shard-label", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), }, { - name: "not admitted with expression routeSelector and no namespaceSelector with route with incorrect labels in namespace with no labels", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "not-shard-label", "default-ic"), - namespace: newNamespace("ns", ""), + name: "clear stale admitted status with expression routeSelector and no namespaceSelector with route with different labels in namespace with no labels", + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "not-shard-label", "default-ic"), + namespace: newNamespace(routeNs, ""), ingressController: newIngressControllerWithSelectors("default-ic", "", "shard-label", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "not-shard-label"), }, { - name: "admitted with no routeSelector and expression namespaceSelector with route with correct labels in namespace with no labels", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), - namespace: newNamespace("ns", "shard-label"), + name: "don't clear admitted status with no routeSelector and expression namespaceSelector with route with matching labels in namespace with matching labels", + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), + namespace: newNamespace(routeNs, "shard-label"), ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", "shard-label"), - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), }, { - name: "not admitted with no routeSelector and expression namespaceSelector with route with incorrect labels in namespace with no labels", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "default-ic"), - namespace: newNamespace("ns", "not-shard-label"), + name: "clear stale admitted status with no routeSelector and expression namespaceSelector with route with no labels in namespace with different labels", + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), + namespace: newNamespace(routeNs, "not-shard-label"), ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", "shard-label"), - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", ""), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, ""), }, { name: "route is nil", - request: reconcileRequestRoute, route: nil, - namespace: newNamespace("ns", "not-shard-label"), + namespace: newNamespace(routeNs, "not-shard-label"), ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", "shard-label"), - expectedErr: false, }, { name: "admitted by ingress controller that doesn't exist anymore", - request: reconcileRequestRoute, - route: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "missing-ic"), - namespace: newNamespace("ns", "shard-label"), + route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "missing-ic"), + namespace: newNamespace(routeNs, "shard-label"), ingressController: nil, - expectedRoute: newRouteWithLabelWithAdmittedStatuses("route", "ns", "", "missing-ic"), + expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, "ns", "", "missing-ic"), }, } @@ -159,24 +142,22 @@ func Test_Reconcile(t *testing.T) { r := newFakeReconciler(tc.namespace) if tc.route != nil { if err := r.client.Create(context.Background(), tc.route); err != nil { - t.Errorf("error creating route: %v", err) + t.Fatalf("error creating route: %v", err) } } if tc.ingressController != nil { if err := r.client.Create(context.Background(), tc.ingressController); err != nil { - t.Errorf("error creating ingress controller: %v", err) + t.Fatalf("error creating ingress controller: %v", err) } } - if _, err := r.Reconcile(context.Background(), tc.request); tc.expectedErr && err == nil { - t.Errorf("expected error, got no error") - } else if !tc.expectedErr && err != nil { - t.Errorf("did not expected error: %v", err) - } else if tc.expectedRoute != nil && !tc.expectedErr { + if _, err := r.Reconcile(context.Background(), reconcileRequestRoute); err != nil { + t.Fatalf("did not expected error: %v", err) + } else if tc.expectedRoute != nil { actualRoute := routev1.Route{} actualRouteName := types.NamespacedName{Name: tc.route.Name, Namespace: tc.route.Namespace} if err := r.client.Get(context.Background(), actualRouteName, &actualRoute); err != nil { - t.Errorf("error retrieving route from client: %v", err) + t.Fatalf("error retrieving route from client: %v", err) } assert.Equal(t, tc.expectedRoute.Status, actualRoute.Status, "route name", tc.expectedRoute.Name) } @@ -189,9 +170,8 @@ func newFakeReconciler(initObjs ...client.Object) *reconciler { client := unit.NewFakeClient(initObjs...) cache := unit.NewFakeCache(client) r := reconciler{ - client: client, - cache: cache, - namespace: controller.DefaultOperandNamespace, + client: client, + cache: cache, } return &r } diff --git a/pkg/operator/controller/route-status/route_status_test.go b/pkg/operator/controller/route-status/route_status_test.go index 4a0682f299..dfefbc909f 100644 --- a/pkg/operator/controller/route-status/route_status_test.go +++ b/pkg/operator/controller/route-status/route_status_test.go @@ -23,7 +23,6 @@ func Test_ClearAllRoutesStatusForIngressController(t *testing.T) { routes routev1.RouteList ingressController *v1.IngressController expectedRoutes routev1.RouteList - expectedErr bool }{ { name: "clear ingress controller foo status that admitted route test", @@ -96,30 +95,27 @@ func Test_ClearAllRoutesStatusForIngressController(t *testing.T) { client := unit.NewFakeClient(tc.ingressController) for _, route := range tc.routes.Items { if err := client.Create(context.Background(), &route); err != nil { - t.Errorf("error creating route: %v", err) + t.Fatalf("error creating route: %v", err) } } - if errs := ClearAllRoutesStatusForIngressController(context.Background(), client, tc.ingressController.Name); tc.expectedErr && len(errs) == 0 { - t.Errorf("expected errors, got no errors") - } else if !tc.expectedErr && len(errs) != 0 { - t.Errorf("did not expected errors: %v", errs) + if errs := ClearAllRoutesStatusForIngressController(context.Background(), client, tc.ingressController.Name); len(errs) != 0 { + t.Fatalf("did not expected errors: %v", errs) } actualRoutes := routev1.RouteList{} - if err := client.List(context.Background(), &actualRoutes); err == nil { - for _, expectedRoute := range tc.expectedRoutes.Items { - // Find the actual route that we should compare status with this expected route - var actualRoute routev1.Route - for _, route := range actualRoutes.Items { - if route.Name == expectedRoute.Name { - actualRoute = route - } + if err := client.List(context.Background(), &actualRoutes); err != nil { + t.Fatalf("error retrieving routes from client: %v", err) + } + for _, expectedRoute := range tc.expectedRoutes.Items { + // Find the actual route that we should compare status with this expected route + var actualRoute routev1.Route + for _, route := range actualRoutes.Items { + if route.Name == expectedRoute.Name { + actualRoute = route } - assert.Equal(t, expectedRoute.Status, actualRoute.Status, "route name", expectedRoute.Name) } - } else { - t.Errorf("error retrieving routes from client: %v", err) + assert.Equal(t, expectedRoute.Status, actualRoute.Status, "route name", expectedRoute.Name) } }) } @@ -216,7 +212,7 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { expectedRoutes: routev1.RouteList{ Items: []routev1.Route{ *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "", "foo-ic", "bar-ic"), - *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns2", "", "baz-ic", "bar-ic"), }, }, }, @@ -237,7 +233,7 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { ingressController: newIngressControllerWithSelectors("foo-ic", "", "", "", "shard-label"), expectedRoutes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "", "foo-ic", "bar-ic"), + *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "bar-ic"), }, }, @@ -250,6 +246,78 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { expectedRoutes: routev1.RouteList{}, expectedErr: true, }, + { + name: "ingress controller with invalid route selector", + routes: routev1.RouteList{}, + namespace: corev1.NamespaceList{}, + ingressController: &operatorv1.IngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: operatorcontroller.DefaultOperatorNamespace, + }, + Spec: v1.IngressControllerSpec{ + RouteSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "type", + Operator: "test", + Values: []string{"test"}, + }, + }, + }, + }, + }, + expectedRoutes: routev1.RouteList{}, + expectedErr: true, + }, + { + name: "ingress controller with invalid RouteSelector", + routes: routev1.RouteList{}, + namespace: corev1.NamespaceList{}, + ingressController: &operatorv1.IngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: operatorcontroller.DefaultOperatorNamespace, + }, + Spec: v1.IngressControllerSpec{ + RouteSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "type", + Operator: "invalid-operator", + Values: []string{"test"}, + }, + }, + }, + }, + }, + expectedRoutes: routev1.RouteList{}, + expectedErr: true, + }, + { + name: "ingress controller with invalid NamespaceSelector", + routes: routev1.RouteList{}, + namespace: corev1.NamespaceList{}, + ingressController: &operatorv1.IngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: operatorcontroller.DefaultOperatorNamespace, + }, + Spec: v1.IngressControllerSpec{ + NamespaceSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "type", + Operator: "invalid-operator", + Values: []string{"test"}, + }, + }, + }, + }, + }, + expectedRoutes: routev1.RouteList{}, + expectedErr: true, + }, } for _, tc := range testCases { @@ -258,40 +326,39 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { client := unit.NewFakeClient() if tc.ingressController != nil { if err := client.Create(context.Background(), tc.ingressController); err != nil { - t.Errorf("error ingress controller route: %v", err) + t.Fatalf("error ingress controller route: %v", err) } } for _, route := range tc.routes.Items { if err := client.Create(context.Background(), &route); err != nil { - t.Errorf("error creating route: %v", err) + t.Fatalf("error creating route: %v", err) } } for _, ns := range tc.namespace.Items { if err := client.Create(context.Background(), &ns); err != nil { - t.Errorf("error creating ns: %v", err) + t.Fatalf("error creating ns: %v", err) } } if errs := ClearRoutesNotAdmittedByIngress(context.Background(), client, tc.ingressController); tc.expectedErr && len(errs) == 0 { - t.Errorf("expected errors, got no errors") + t.Fatal("expected errors, got no errors") } else if !tc.expectedErr && len(errs) != 0 { - t.Errorf("did not expected errors: %v", errs) + t.Fatalf("did not expected errors: %v", errs) } actualRoutes := routev1.RouteList{} - if err := client.List(context.Background(), &actualRoutes); err == nil { - for _, expectedRoute := range tc.expectedRoutes.Items { - // Find the actual route that we should compare status with this expected route - var actualRoute routev1.Route - for _, route := range actualRoutes.Items { - if route.Name == expectedRoute.Name { - actualRoute = route - } + if err := client.List(context.Background(), &actualRoutes); err != nil { + t.Fatalf("error retrieving routes from client: %v", err) + } + for _, expectedRoute := range tc.expectedRoutes.Items { + // Find the actual route that we should compare status with this expected route + var actualRoute routev1.Route + for _, route := range actualRoutes.Items { + if route.Name == expectedRoute.Name { + actualRoute = route } - assert.Equal(t, expectedRoute.Status, actualRoute.Status, "route name", expectedRoute.Name) } - } else { - t.Errorf("error retrieving all from client: %v", err) + assert.Equal(t, expectedRoute.Status, actualRoute.Status, "route name", expectedRoute.Name) } }) } @@ -366,7 +433,7 @@ func Test_IsRouteStatusAdmitted(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { if actualResult := IsRouteStatusAdmitted(tc.route, tc.ingressControllerName); actualResult != tc.expectedResult { - t.Errorf("expected result %v, got %v", tc.expectedResult, actualResult) + t.Fatalf("expected result %v, got %v", tc.expectedResult, actualResult) } }) } @@ -392,7 +459,7 @@ func Test_IsInvalidSelectorError(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { if isInvalidSelectorError := IsInvalidSelectorError(tc.error); tc.expectedInvalidSelectorError && !isInvalidSelectorError { - t.Errorf("expected InvalidSelectorError, was not InvalidSelectorError") + t.Fatal("expected InvalidSelectorError, was not InvalidSelectorError") } }) } @@ -460,7 +527,8 @@ func newNamespace(name, label string) *corev1.Namespace { return &namespace } -// newRouteWithAdmittedStatuses returns a new route that is admitted by ingress controllers. +// newRouteWithLabelWithAdmittedStatuses returns a new route that is admitted by ingress controllers +// and has the "type" label set with the specified value. func newRouteWithLabelWithAdmittedStatuses(name, namespace, label string, icAdmitted ...string) *routev1.Route { route := newRouteWithAdmittedStatuses(name, icAdmitted...) route.ObjectMeta.Namespace = namespace diff --git a/test/e2e/router_status_test.go b/test/e2e/router_status_test.go index 209e6e500c..1dfb821458 100644 --- a/test/e2e/router_status_test.go +++ b/test/e2e/router_status_test.go @@ -378,7 +378,7 @@ func TestRouteLabelShouldClearRouteStatus(t *testing.T) { t.Fatalf("failed to observe expected conditions: %v", err) } - // Update namespace to not match the namespaceSelector. + // Update route to not match the routeSelector. if err := kclient.Get(context.TODO(), routeName, route); err != nil { t.Fatalf("failed to get route: %v", err) } From efc08f91fc67ffb060cb4616f78894171f08ccb7 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Fri, 17 Feb 2023 15:54:55 -0500 Subject: [PATCH 9/9] Added IngressController, Route, and Namespace builders for unit test --- .../route-status/controller_test.go | 131 ++++--- .../route-status/route_status_test.go | 366 +++++------------- test/unit/util.go | 174 +++++++++ 3 files changed, 340 insertions(+), 331 deletions(-) create mode 100644 test/unit/util.go diff --git a/pkg/operator/controller/route-status/controller_test.go b/pkg/operator/controller/route-status/controller_test.go index 851f0a5e11..8fb1b55072 100644 --- a/pkg/operator/controller/route-status/controller_test.go +++ b/pkg/operator/controller/route-status/controller_test.go @@ -2,28 +2,27 @@ package routestatus import ( "context" + "testing" + v12 "github.com/openshift/api/operator/v1" routev1 "github.com/openshift/api/route/v1" "github.com/openshift/cluster-ingress-operator/test/unit" - "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" - "testing" + + "github.com/stretchr/testify/assert" ) // Test_Reconcile verifies Reconcile behaves as expected. // Note: This effectively tests clearStaleRouteAdmittedStatus as well. func Test_Reconcile(t *testing.T) { - routeName := "route" - routeNs := "ns" reconcileRequestRoute := reconcile.Request{ - NamespacedName: types.NamespacedName{ - Name: routeName, - Namespace: routeNs, - }, + NamespacedName: fooRouteNsName, } + testCases := []struct { name string route *routev1.Route @@ -33,107 +32,107 @@ func Test_Reconcile(t *testing.T) { }{ { name: "don't clear admitted status with no routeSelector and no namespaceSelector with route with no labels in namespace with no labels", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), - namespace: newNamespace(routeNs, ""), - ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcNsName.Name).Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).Build(), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcNsName.Name).Build(), }, { name: "don't clear admitted status with no routeSelector and no namespaceSelector with route with labels in namespace with no labels", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), - namespace: newNamespace(routeNs, ""), - ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcNsName.Name).Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).Build(), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcNsName.Name).Build(), }, { name: "don't clear admitted status with no routeSelector and no namespaceSelector with route with no labels in namespace with labels", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), - namespace: newNamespace(routeNs, "shard-label"), - ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcNsName.Name).Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).WithLabels(shardLabel).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).Build(), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcNsName.Name).Build(), }, { name: "don't clear admitted status with routeSelector and no namespaceSelector with route with matching labels in namespace with no labels", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), - namespace: newNamespace(routeNs, ""), - ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcNsName.Name).Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithRouteSelectors(shardLabel).Build(), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcNsName.Name).Build(), }, { name: "clear stale admitted status with routeSelector and no namespaceSelector with route with no labels in namespace with no labels", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), - namespace: newNamespace(routeNs, ""), - ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, ""), + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcNsName.Name).Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithRouteSelectors(shardLabel).Build(), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).Build(), }, { name: "clear stale admitted status with routeSelector and no namespaceSelector with route with different labels in namespace with no labels", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "not-shard-label", "default-ic"), - namespace: newNamespace(routeNs, ""), - ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "not-shard-label"), + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(notShardLabel).WithAdmittedICs(fooIcNsName.Name).Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithRouteSelectors(shardLabel).Build(), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(notShardLabel).Build(), }, { name: "don't clear admitted status with routeSelector and namespaceSelector with route with matching labels in namespace with matching labels", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), - namespace: newNamespace(routeNs, "shard-label"), - ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "shard-label", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcNsName.Name).Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).WithLabels(shardLabel).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithRouteSelectors(shardLabel).WithNamespaceSelectors(shardLabel).Build(), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcNsName.Name).Build(), }, { name: "clear stale admitted status with routeSelector and namespaceSelector with route with different labels in namespace with matching labels", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "not-shard-label", "default-ic"), - namespace: newNamespace(routeNs, "shard-label"), - ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "shard-label", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "not-shard-label"), + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(notShardLabel).WithAdmittedICs(fooIcNsName.Name).Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).WithLabels(shardLabel).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithRouteSelectors(shardLabel).WithNamespaceSelectors(shardLabel).Build(), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(notShardLabel).Build(), }, { name: "clear stale admitted status with routeSelector and namespaceSelector with route with matching labels in namespace with different labels", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), - namespace: newNamespace(routeNs, "not-shard-label"), - ingressController: newIngressControllerWithSelectors("default-ic", "shard-label", "", "shard-label", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label"), + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcNsName.Name).Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).WithLabels(notShardLabel).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithRouteSelectors(shardLabel).WithNamespaceSelectors(shardLabel).Build(), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).Build(), }, { name: "don't clear admitted status with expression routeSelector and no namespaceSelector with route with matching labels in namespace with no labels", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), - namespace: newNamespace(routeNs, ""), - ingressController: newIngressControllerWithSelectors("default-ic", "", "shard-label", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "shard-label", "default-ic"), + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcNsName.Name).Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithRouteExpressionSelector(shardLabelExpression).Build(), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcNsName.Name).Build(), }, { name: "clear stale admitted status with expression routeSelector and no namespaceSelector with route with different labels in namespace with no labels", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "not-shard-label", "default-ic"), - namespace: newNamespace(routeNs, ""), - ingressController: newIngressControllerWithSelectors("default-ic", "", "shard-label", "", ""), - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "not-shard-label"), + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(notShardLabel).WithAdmittedICs(fooIcNsName.Name).Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithRouteExpressionSelector(shardLabelExpression).Build(), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(notShardLabel).Build(), }, { name: "don't clear admitted status with no routeSelector and expression namespaceSelector with route with matching labels in namespace with matching labels", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), - namespace: newNamespace(routeNs, "shard-label"), - ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", "shard-label"), - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcNsName.Name).Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).WithLabels(shardLabel).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithNamespaceExpressionSelector(shardLabelExpression).Build(), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcNsName.Name).Build(), }, { name: "clear stale admitted status with no routeSelector and expression namespaceSelector with route with no labels in namespace with different labels", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "default-ic"), - namespace: newNamespace(routeNs, "not-shard-label"), - ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", "shard-label"), - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, ""), + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcNsName.Name).Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).WithLabels(notShardLabel).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithNamespaceExpressionSelector(shardLabelExpression).Build(), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).Build(), }, { name: "route is nil", route: nil, - namespace: newNamespace(routeNs, "not-shard-label"), - ingressController: newIngressControllerWithSelectors("default-ic", "", "", "", "shard-label"), + namespace: unit.NewNamespaceBuilder().WithName(fooRouteNsName.Name).Build(), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).Build(), }, { - name: "admitted by ingress controller that doesn't exist anymore", - route: newRouteWithLabelWithAdmittedStatuses(routeName, routeNs, "", "missing-ic"), - namespace: newNamespace(routeNs, "shard-label"), + name: "don't clear admitted by ingress controller that doesn't exist anymore", + route: unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs("missing-ic").Build(), + namespace: unit.NewNamespaceBuilder().WithName(ns1Name).Build(), ingressController: nil, - expectedRoute: newRouteWithLabelWithAdmittedStatuses(routeName, "ns", "", "missing-ic"), + expectedRoute: unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs("missing-ic").Build(), }, } diff --git a/pkg/operator/controller/route-status/route_status_test.go b/pkg/operator/controller/route-status/route_status_test.go index dfefbc909f..7ef4506c8c 100644 --- a/pkg/operator/controller/route-status/route_status_test.go +++ b/pkg/operator/controller/route-status/route_status_test.go @@ -5,15 +5,38 @@ import ( "fmt" "testing" - operatorv1 "github.com/openshift/api/operator/v1" v1 "github.com/openshift/api/operator/v1" routev1 "github.com/openshift/api/route/v1" operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" "github.com/openshift/cluster-ingress-operator/test/unit" - "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + "github.com/stretchr/testify/assert" +) + +var ( + ns1Name = "ns1" + ns2Name = "ns2" + fooRouteNsName = types.NamespacedName{Name: "foo-route", Namespace: ns1Name} + barRouteNsName = types.NamespacedName{Name: "bar-route", Namespace: ns1Name} + barRouteNs2Name = types.NamespacedName{Name: "bar-route", Namespace: ns2Name} + fooIcName = "foo-ic" + fooIcNsName = types.NamespacedName{Name: fooIcName, Namespace: operatorcontroller.DefaultOperatorNamespace} + shardLabel = map[string]string{"type": "shard-label"} + notShardLabel = map[string]string{"type": "not-shard-label"} + shardLabelExpression = metav1.LabelSelectorRequirement{ + Key: "type", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"shard-label"}, + } + invalidExpression = metav1.LabelSelectorRequirement{ + Key: "type", + Operator: "invalid-operator", + Values: []string{"shard-label"}, + } ) // Test_ClearAllRoutesStatusForIngressController verifies that ClearAllRoutesStatusForIngressController behaves correctly. @@ -28,20 +51,15 @@ func Test_ClearAllRoutesStatusForIngressController(t *testing.T) { name: "clear ingress controller foo status that admitted route test", routes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithAdmittedStatuses("test", "foo-ic", "bar-ic"), - *newRouteWithAdmittedStatuses("test2", "baz-ic", "bar-ic"), - }, - }, - ingressController: &operatorv1.IngressController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo-ic", - Namespace: operatorcontroller.DefaultOperatorNamespace, + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcName, "bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNsName).WithAdmittedICs("baz-ic", "bar-ic").Build(), }, }, + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).Build(), expectedRoutes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithAdmittedStatuses("test", "bar-ic"), - *newRouteWithAdmittedStatuses("test2", "baz-ic", "bar-ic"), + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs("bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNsName).WithAdmittedICs("baz-ic", "bar-ic").Build(), }, }, }, @@ -49,20 +67,15 @@ func Test_ClearAllRoutesStatusForIngressController(t *testing.T) { name: "don't clear ingress controller foo status that didn't admit anything", routes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithAdmittedStatuses("test", "dog-ic", "bar-ic"), - *newRouteWithAdmittedStatuses("test2", "baz-ic", "bar-ic"), - }, - }, - ingressController: &operatorv1.IngressController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo-ic", - Namespace: operatorcontroller.DefaultOperatorNamespace, + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs("dog-ic", "bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNsName).WithAdmittedICs("baz-ic", "bar-ic").Build(), }, }, + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).Build(), expectedRoutes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithAdmittedStatuses("test", "dog-ic", "bar-ic"), - *newRouteWithAdmittedStatuses("test2", "baz-ic", "bar-ic"), + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs("dog-ic", "bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNsName).WithAdmittedICs("baz-ic", "bar-ic").Build(), }, }, }, @@ -70,20 +83,15 @@ func Test_ClearAllRoutesStatusForIngressController(t *testing.T) { name: "clear ingress controller foo status that admitted everything", routes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithAdmittedStatuses("test", "foo-ic"), - *newRouteWithAdmittedStatuses("test2", "foo-ic"), - }, - }, - ingressController: &operatorv1.IngressController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo-ic", - Namespace: operatorcontroller.DefaultOperatorNamespace, + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcName).Build(), + *unit.NewRouteBuilder().WithName(barRouteNsName).WithAdmittedICs(fooIcName).Build(), }, }, + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).Build(), expectedRoutes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithAdmittedStatuses("test"), - *newRouteWithAdmittedStatuses("test2"), + *unit.NewRouteBuilder().WithName(fooRouteNsName).Build(), + *unit.NewRouteBuilder().WithName(barRouteNsName).Build(), }, }, }, @@ -135,20 +143,20 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { name: "don't clear anything: all IC admissions are valid", routes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), - *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "shard-label", "baz-ic", "foo-ic", "bar-ic"), + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcName, "bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcName, "baz-ic", "bar-ic").Build(), }, }, namespace: corev1.NamespaceList{ Items: []corev1.Namespace{ - *newNamespace("ns", "shard-label"), + *unit.NewNamespaceBuilder().WithName(ns1Name).WithLabels(shardLabel).Build(), }, }, - ingressController: newIngressControllerWithSelectors("foo-ic", "shard-label", "", "shard-label", ""), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithRouteSelectors(shardLabel).WithNamespaceSelectors(shardLabel).Build(), expectedRoutes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), - *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "shard-label", "baz-ic", "foo-ic", "bar-ic"), + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcName, "bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcName, "baz-ic", "bar-ic").Build(), }, }, }, @@ -156,20 +164,20 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { name: "clear route that is no longer admitted by ingress controller by routeSelectors", routes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), - *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "foo-ic", "bar-ic"), + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcName, "bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNsName).WithAdmittedICs(fooIcName, "baz-ic", "bar-ic").Build(), }, }, namespace: corev1.NamespaceList{ Items: []corev1.Namespace{ - *newNamespace("ns", ""), + *unit.NewNamespaceBuilder().WithName(ns1Name).Build(), }, }, - ingressController: newIngressControllerWithSelectors("foo-ic", "shard-label", "", "", ""), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithRouteSelectors(shardLabel).Build(), expectedRoutes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), - *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "bar-ic"), + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcName, "bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNsName).WithAdmittedICs("baz-ic", "bar-ic").Build(), }, }, }, @@ -177,20 +185,20 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { name: "clear route that is no longer admitted by ingress controller by routeSelectors expression", routes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), - *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "foo-ic", "bar-ic"), + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcName, "bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNsName).WithAdmittedICs(fooIcName, "baz-ic", "bar-ic").Build(), }, }, namespace: corev1.NamespaceList{ Items: []corev1.Namespace{ - *newNamespace("ns", ""), + *unit.NewNamespaceBuilder().WithName(ns1Name).Build(), }, }, - ingressController: newIngressControllerWithSelectors("foo-ic", "", "shard-label", "", ""), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithRouteExpressionSelector(shardLabelExpression).Build(), expectedRoutes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), - *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "bar-ic"), + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithLabels(shardLabel).WithAdmittedICs(fooIcName, "bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNsName).WithAdmittedICs("baz-ic", "bar-ic").Build(), }, }, }, @@ -198,21 +206,21 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { name: "clear route that is no longer admitted by ingress controller by namespaceSelectors", routes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "", "foo-ic", "bar-ic"), - *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns2", "", "baz-ic", "foo-ic", "bar-ic"), + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcName, "bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNs2Name).WithAdmittedICs(fooIcName, "baz-ic", "bar-ic").Build(), }, }, namespace: corev1.NamespaceList{ Items: []corev1.Namespace{ - *newNamespace("ns", "shard-label"), - *newNamespace("ns2", ""), + *unit.NewNamespaceBuilder().WithName(ns1Name).WithLabels(shardLabel).Build(), + *unit.NewNamespaceBuilder().WithName(ns2Name).Build(), }, }, - ingressController: newIngressControllerWithSelectors("foo-ic", "", "", "shard-label", ""), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithNamespaceSelectors(shardLabel).Build(), expectedRoutes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "", "foo-ic", "bar-ic"), - *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns2", "", "baz-ic", "bar-ic"), + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcName, "bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNs2Name).WithAdmittedICs("baz-ic", "bar-ic").Build(), }, }, }, @@ -220,21 +228,21 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { name: "clear route that is no longer admitted by ingress controller by namespaceSelectors expression", routes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "", "foo-ic", "bar-ic"), - *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns2", "", "baz-ic", "foo-ic", "bar-ic"), + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcName, "bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNs2Name).WithAdmittedICs(fooIcName, "baz-ic", "bar-ic").Build(), }, }, namespace: corev1.NamespaceList{ Items: []corev1.Namespace{ - *newNamespace("ns", "shard-label"), - *newNamespace("ns2", ""), + *unit.NewNamespaceBuilder().WithName(ns1Name).WithLabels(shardLabel).Build(), + *unit.NewNamespaceBuilder().WithName(ns2Name).Build(), }, }, - ingressController: newIngressControllerWithSelectors("foo-ic", "", "", "", "shard-label"), + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithNamespaceExpressionSelector(shardLabelExpression).Build(), expectedRoutes: routev1.RouteList{ Items: []routev1.Route{ - *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), - *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns", "", "baz-ic", "bar-ic"), + *unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcName, "bar-ic").Build(), + *unit.NewRouteBuilder().WithName(barRouteNs2Name).WithAdmittedICs("baz-ic", "bar-ic").Build(), }, }, }, @@ -247,76 +255,20 @@ func Test_ClearRoutesNotAdmittedByIngress(t *testing.T) { expectedErr: true, }, { - name: "ingress controller with invalid route selector", - routes: routev1.RouteList{}, - namespace: corev1.NamespaceList{}, - ingressController: &operatorv1.IngressController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: operatorcontroller.DefaultOperatorNamespace, - }, - Spec: v1.IngressControllerSpec{ - RouteSelector: &metav1.LabelSelector{ - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "type", - Operator: "test", - Values: []string{"test"}, - }, - }, - }, - }, - }, - expectedRoutes: routev1.RouteList{}, - expectedErr: true, - }, - { - name: "ingress controller with invalid RouteSelector", - routes: routev1.RouteList{}, - namespace: corev1.NamespaceList{}, - ingressController: &operatorv1.IngressController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: operatorcontroller.DefaultOperatorNamespace, - }, - Spec: v1.IngressControllerSpec{ - RouteSelector: &metav1.LabelSelector{ - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "type", - Operator: "invalid-operator", - Values: []string{"test"}, - }, - }, - }, - }, - }, - expectedRoutes: routev1.RouteList{}, - expectedErr: true, + name: "ingress controller with invalid route selector", + routes: routev1.RouteList{}, + namespace: corev1.NamespaceList{}, + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithRouteExpressionSelector(invalidExpression).Build(), + expectedRoutes: routev1.RouteList{}, + expectedErr: true, }, { - name: "ingress controller with invalid NamespaceSelector", - routes: routev1.RouteList{}, - namespace: corev1.NamespaceList{}, - ingressController: &operatorv1.IngressController{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: operatorcontroller.DefaultOperatorNamespace, - }, - Spec: v1.IngressControllerSpec{ - NamespaceSelector: &metav1.LabelSelector{ - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "type", - Operator: "invalid-operator", - Values: []string{"test"}, - }, - }, - }, - }, - }, - expectedRoutes: routev1.RouteList{}, - expectedErr: true, + name: "ingress controller with invalid route selector", + routes: routev1.RouteList{}, + namespace: corev1.NamespaceList{}, + ingressController: unit.NewIngressControllerBuilder().WithName(fooIcNsName).WithNamespaceExpressionSelector(invalidExpression).Build(), + expectedRoutes: routev1.RouteList{}, + expectedErr: true, }, } @@ -373,59 +325,41 @@ func Test_IsRouteStatusAdmitted(t *testing.T) { expectedResult bool }{ { - name: "route admitted by default", - route: *newRouteWithAdmittedStatuses("foo-route", "default-ic"), - ingressControllerName: "default-ic", + name: "route admitted", + route: *unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcName).Build(), + ingressControllerName: fooIcName, expectedResult: true, }, { - name: "route not admitted by sharded", - route: routev1.Route{ - Status: routev1.RouteStatus{ - Ingress: []routev1.RouteIngress{ - { - RouterName: "sharded", - Conditions: []routev1.RouteIngressCondition{ - { - Type: routev1.RouteAdmitted, - Status: corev1.ConditionFalse, - }, - }, - }, - }, - }, - }, - ingressControllerName: "sharded", + name: "route with false admitted condition", + route: *unit.NewRouteBuilder().WithName(fooRouteNsName).WithUnAdmittedICs(fooIcName).Build(), + ingressControllerName: fooIcName, expectedResult: false, }, { - name: "route admitted by default, not admitted by sharded", - route: *newRouteWithAdmittedStatuses("foo-route", "default-ic"), - ingressControllerName: "sharded", + name: "route admitted by foo, not admitted by bar", + route: *unit.NewRouteBuilder().WithName(fooRouteNsName).WithAdmittedICs(fooIcName).Build(), + ingressControllerName: "bar-ic", expectedResult: false, }, { - name: "route not admitted by sharded without Conditions", + name: "route not admitted without conditions", route: routev1.Route{ Status: routev1.RouteStatus{ Ingress: []routev1.RouteIngress{ { - RouterName: "sharded", + RouterName: fooIcName, }, }, }, }, - ingressControllerName: "sharded", + ingressControllerName: fooIcName, expectedResult: false, }, { - name: "route not admitted by any shard", - route: routev1.Route{ - Status: routev1.RouteStatus{ - Ingress: []routev1.RouteIngress{}, - }, - }, - ingressControllerName: "default-ic", + name: "route not admitted by any ic", + route: *unit.NewRouteBuilder().WithName(fooRouteNsName).Build(), + ingressControllerName: fooIcName, expectedResult: false, }, } @@ -447,11 +381,11 @@ func Test_IsInvalidSelectorError(t *testing.T) { expectedInvalidSelectorError bool }{ { - name: "Is not InvalidSelectorError", + name: "is not InvalidSelectorError", error: fmt.Errorf("not InvalidSelectorError"), }, { - name: "Is InvalidSelectorError", + name: "is InvalidSelectorError", error: &InvalidSelectorError{"InvalidSelectorError"}, }, } @@ -464,101 +398,3 @@ func Test_IsInvalidSelectorError(t *testing.T) { }) } } - -// newIngressControllerWithSelectors returns a new ingresscontroller with the specified name, -// routeSelectors, and namespaceSelectors based on the parameters. -func newIngressControllerWithSelectors(name, routeMatchLabel, routeMatchExpression, namespaceMatchLabel, namespaceMatchExpression string) *operatorv1.IngressController { - ingresscontroller := operatorv1.IngressController{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: operatorcontroller.DefaultOperatorNamespace, - }, - } - if len(routeMatchLabel) != 0 { - ingresscontroller.Spec.RouteSelector = &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "type": routeMatchLabel, - }, - } - } - if len(routeMatchExpression) != 0 { - ingresscontroller.Spec.RouteSelector = &metav1.LabelSelector{ - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "type", - Operator: metav1.LabelSelectorOpIn, - Values: []string{routeMatchExpression}, - }, - }, - } - } - if len(namespaceMatchLabel) != 0 { - ingresscontroller.Spec.NamespaceSelector = &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "type": namespaceMatchLabel, - }, - } - } - if len(namespaceMatchExpression) != 0 { - ingresscontroller.Spec.NamespaceSelector = &metav1.LabelSelector{ - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "type", - Operator: metav1.LabelSelectorOpIn, - Values: []string{namespaceMatchExpression}, - }, - }, - } - } - return &ingresscontroller -} - -// newNamespace returns a new namespace with the specified name -// and if label exists, with that label -func newNamespace(name, label string) *corev1.Namespace { - namespace := corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - }, - } - if len(label) != 0 { - namespace.ObjectMeta.Labels = map[string]string{"type": label} - } - return &namespace -} - -// newRouteWithLabelWithAdmittedStatuses returns a new route that is admitted by ingress controllers -// and has the "type" label set with the specified value. -func newRouteWithLabelWithAdmittedStatuses(name, namespace, label string, icAdmitted ...string) *routev1.Route { - route := newRouteWithAdmittedStatuses(name, icAdmitted...) - route.ObjectMeta.Namespace = namespace - if len(label) != 0 { - route.Labels = map[string]string{ - "type": label, - } - } - return route -} - -// newRouteWithAdmittedStatuses returns a new route that is admitted by ingress controllers. -func newRouteWithAdmittedStatuses(name string, icAdmitted ...string) *routev1.Route { - route := routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - }, - } - if len(icAdmitted) != 0 { - for _, ic := range icAdmitted { - route.Status.Ingress = append(route.Status.Ingress, routev1.RouteIngress{ - RouterName: ic, - Conditions: []routev1.RouteIngressCondition{ - { - Type: routev1.RouteAdmitted, - Status: "True", - }, - }, - }) - } - } - return &route -} diff --git a/test/unit/util.go b/test/unit/util.go new file mode 100644 index 0000000000..8c87bdcb76 --- /dev/null +++ b/test/unit/util.go @@ -0,0 +1,174 @@ +package unit + +import ( + v1 "github.com/openshift/api/operator/v1" + routev1 "github.com/openshift/api/route/v1" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +type routeBuilder struct { + name types.NamespacedName + labels map[string]string + admittedICs []string + unAdmittedICs []string +} + +func NewRouteBuilder() *routeBuilder { + return &routeBuilder{ + name: types.NamespacedName{Name: "sample", Namespace: "openshift-ingress"}, + } +} + +func (b *routeBuilder) WithName(name types.NamespacedName) *routeBuilder { + b.name = name + return b +} + +func (b *routeBuilder) WithLabels(labels map[string]string) *routeBuilder { + b.labels = labels + return b +} + +func (b *routeBuilder) WithAdmittedICs(admittedICs ...string) *routeBuilder { + b.admittedICs = admittedICs + return b +} + +func (b *routeBuilder) WithUnAdmittedICs(unAdmittedICs ...string) *routeBuilder { + b.unAdmittedICs = unAdmittedICs + return b +} + +func (b routeBuilder) Build() *routev1.Route { + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: b.name.Name, + Namespace: b.name.Namespace, + Labels: b.labels, + }, + Spec: routev1.RouteSpec{}, + Status: routev1.RouteStatus{}, + } + + if len(b.admittedICs) != 0 { + for _, ic := range b.admittedICs { + route.Status.Ingress = append(route.Status.Ingress, routev1.RouteIngress{ + RouterName: ic, + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: corev1.ConditionTrue, + }, + }, + }) + } + } + + if len(b.unAdmittedICs) != 0 { + for _, ic := range b.unAdmittedICs { + route.Status.Ingress = append(route.Status.Ingress, routev1.RouteIngress{ + RouterName: ic, + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: corev1.ConditionFalse, + }, + }, + }) + } + } + + return route +} + +type ingressControllerBuilder struct { + name types.NamespacedName + namespaceSelectors map[string]string + routeSelectors map[string]string + namespaceExpressionSelector []metav1.LabelSelectorRequirement + routeExpressionSelector []metav1.LabelSelectorRequirement +} + +func NewIngressControllerBuilder() *ingressControllerBuilder { + return &ingressControllerBuilder{ + name: types.NamespacedName{Name: "sample", Namespace: "openshift-ingress"}, + } +} + +func (b *ingressControllerBuilder) WithName(name types.NamespacedName) *ingressControllerBuilder { + b.name = name + return b +} + +func (b *ingressControllerBuilder) WithNamespaceSelectors(namespaceSelectors map[string]string) *ingressControllerBuilder { + b.namespaceSelectors = namespaceSelectors + return b +} + +func (b *ingressControllerBuilder) WithRouteSelectors(routeSelectors map[string]string) *ingressControllerBuilder { + b.routeSelectors = routeSelectors + return b +} + +func (b *ingressControllerBuilder) WithRouteExpressionSelector(routeExpressionSelectors metav1.LabelSelectorRequirement) *ingressControllerBuilder { + b.routeExpressionSelector = []metav1.LabelSelectorRequirement{routeExpressionSelectors} + return b +} + +func (b *ingressControllerBuilder) WithNamespaceExpressionSelector(namespaceExpressionSelectors metav1.LabelSelectorRequirement) *ingressControllerBuilder { + b.namespaceExpressionSelector = []metav1.LabelSelectorRequirement{namespaceExpressionSelectors} + return b +} + +func (b ingressControllerBuilder) Build() *v1.IngressController { + ic := &v1.IngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: b.name.Name, + Namespace: b.name.Namespace, + }, + Spec: v1.IngressControllerSpec{ + RouteSelector: &metav1.LabelSelector{ + MatchLabels: b.routeSelectors, + MatchExpressions: b.routeExpressionSelector, + }, + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: b.namespaceSelectors, + MatchExpressions: b.namespaceExpressionSelector, + }, + }, + } + return ic +} + +type namespaceBuilder struct { + name string + labels map[string]string +} + +func NewNamespaceBuilder() *namespaceBuilder { + return &namespaceBuilder{ + name: "name", + } +} + +func (b *namespaceBuilder) WithName(name string) *namespaceBuilder { + b.name = name + return b +} + +func (b *namespaceBuilder) WithLabels(labels map[string]string) *namespaceBuilder { + b.labels = labels + return b +} + +func (b namespaceBuilder) Build() *corev1.Namespace { + return &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: b.name, + Labels: b.labels, + }, + } +}