diff --git a/pkg/operator/controller/route-metrics/controller.go b/pkg/operator/controller/route-metrics/controller.go index 1b752eaf41..227d5bd8bb 100644 --- a/pkg/operator/controller/route-metrics/controller.go +++ b/pkg/operator/controller/route-metrics/controller.go @@ -8,6 +8,7 @@ import ( operatorv1 "github.com/openshift/api/operator/v1" routev1 "github.com/openshift/api/route/v1" logf "github.com/openshift/cluster-ingress-operator/pkg/log" + "github.com/openshift/cluster-ingress-operator/pkg/util/ingresscontroller" "golang.org/x/time/rate" corev1 "k8s.io/api/core/v1" @@ -173,6 +174,11 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( return reconcile.Result{}, nil } + // If the Ingress Controller is not admitted, don't provide metrics for it. + if !ingresscontroller.IsAdmitted(ingressController) { + 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. diff --git a/pkg/operator/controller/route-metrics/controller_test.go b/pkg/operator/controller/route-metrics/controller_test.go index bda6d78f1a..fccb574492 100644 --- a/pkg/operator/controller/route-metrics/controller_test.go +++ b/pkg/operator/controller/route-metrics/controller_test.go @@ -1,12 +1,328 @@ package routemetrics import ( + "context" + "fmt" + "strings" "testing" + operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" + "github.com/openshift/cluster-ingress-operator/test/unit" + + "github.com/openshift/api/operator" + 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" + "k8s.io/apimachinery/pkg/util/sets" + "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" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + "github.com/prometheus/client_golang/prometheus/testutil" ) +// Test_Reconcile verifies Reconcile +// Note: This is intended to be a generic unit test. Edge cases will be tested in unit tests for specific functions. +func Test_Reconcile(t *testing.T) { + reconcileRequest := reconcile.Request{NamespacedName: types.NamespacedName{Name: "foo-ic", Namespace: "openshift-ingress-operator"}} + testCases := []struct { + name string + request reconcile.Request + initObjs []client.Object + ingressController *v1.IngressController + routes routev1.RouteList + expectedMetricFormat string + }{ + { + name: "ingress controller doesn't exist", + request: reconcileRequest, + }, + { + name: "ingress controller is being deleted", + initObjs: []client.Object{ + unit.NewIngressControllerBuilder(). + WithName("foo-ic"). + IsDeleting(). + WithAdmitted(false). + Build(), + }, + request: reconcileRequest, + }, + { + name: "ingress controller is not admitted", + initObjs: []client.Object{ + unit.NewIngressControllerBuilder(). + WithName("foo-ic"). + WithAdmitted(false). + Build(), + }, + request: reconcileRequest, + }, + { + name: "ingress controller is admitted with no routes admitted", + initObjs: []client.Object{ + unit.NewIngressControllerBuilder(). + WithName("foo-ic"). + WithAdmitted(true). + Build(), + }, + request: reconcileRequest, + expectedMetricFormat: routePerShardMetric("foo-ic", 0), + }, + { + name: "ingress controller is admitted with a route admitted", + initObjs: []client.Object{ + unit.NewIngressControllerBuilder(). + WithName("foo-ic"). + WithAdmitted(true). + Build(), + unit.NewRouteBuilder(). + WithName("foo-route"). + WithNamespace("foo-ns"). + WithAdmittedICs("foo-ic"). + Build(), + unit.NewNamespaceBuilder(). + WithName("foo-ns"). + Build(), + }, + request: reconcileRequest, + expectedMetricFormat: routePerShardMetric("foo-ic", 1), + }, + { + name: "ingress controller with routeSelector and with no namespaceSelector with 2 routes with correct labels in namespace with no labels", + initObjs: []client.Object{ + unit.NewIngressControllerBuilder(). + WithName("foo-ic"). + WithAdmitted(true). + WithRouteSelector("type", "shard"). + Build(), + unit.NewRouteBuilder(). + WithName("foo-route"). + WithNamespace("foo-ns"). + WithAdmittedICs("foo-ic"). + WithLabel("type", "shard"). + Build(), + unit.NewRouteBuilder(). + WithName("bar-route"). + WithNamespace("foo-ns"). + WithAdmittedICs("foo-ic"). + WithLabel("type", "shard"). + Build(), + unit.NewNamespaceBuilder(). + WithName("foo-ns"). + Build(), + }, + request: reconcileRequest, + expectedMetricFormat: routePerShardMetric("foo-ic", 2), + }, + { + name: "ingress controller with no routeSelector and with namespaceSelector with route with no labels in namespace with correct labels", + initObjs: []client.Object{ + unit.NewIngressControllerBuilder(). + WithName("foo-ic"). + WithAdmitted(true). + WithNamespaceSelector("type", "shard"). + Build(), + unit.NewRouteBuilder(). + WithName("foo-route"). + WithNamespace("foo-ns"). + WithAdmittedICs("foo-ic"). + Build(), + unit.NewNamespaceBuilder(). + WithName("foo-ns"). + WithLabel("type", "shard"). + Build(), + }, + request: reconcileRequest, + expectedMetricFormat: routePerShardMetric("foo-ic", 1), + }, + { + name: "ingress controller with expression routeSelector and with no namespaceSelector with route with correct labels in namespace with no labels", + initObjs: []client.Object{ + unit.NewIngressControllerBuilder(). + WithName("foo-ic"). + WithAdmitted(true). + WithRouteExpressionSelector("type", metav1.LabelSelectorOpIn, []string{"shard"}). + Build(), + unit.NewRouteBuilder(). + WithName("foo-route"). + WithNamespace("foo-ns"). + WithAdmittedICs("foo-ic"). + WithLabel("type", "shard"). + Build(), + unit.NewNamespaceBuilder(). + WithName("foo-ns"). + Build(), + }, + request: reconcileRequest, + expectedMetricFormat: routePerShardMetric("foo-ic", 1), + }, + { + name: "ingress controller with no routeSelector and with expression namespaceSelector with route with no labels in namespace with correct labels", + initObjs: []client.Object{ + unit.NewIngressControllerBuilder(). + WithName("foo-ic"). + WithAdmitted(true). + WithNamespaceExpressionSelector("type", metav1.LabelSelectorOpIn, []string{"shard"}). + Build(), + unit.NewRouteBuilder(). + WithName("foo-route"). + WithNamespace("foo-ns"). + WithAdmittedICs("foo-ic"). + Build(), + unit.NewNamespaceBuilder(). + WithName("foo-ns"). + WithLabel("type", "shard"). + Build(), + }, + request: reconcileRequest, + expectedMetricFormat: routePerShardMetric("foo-ic", 1), + }, + { + name: "ingress controller with routeSelector and with namespaceSelector with route with correct labels in namespace with correct labels", + initObjs: []client.Object{ + unit.NewIngressControllerBuilder(). + WithName("foo-ic"). + WithAdmitted(true). + WithRouteSelector("type", "shard"). + WithNamespaceSelector("type", "shard"). + Build(), + unit.NewRouteBuilder(). + WithName("foo-route"). + WithNamespace("foo-ns"). + WithAdmittedICs("foo-ic"). + WithLabel("type", "shard"). + Build(), + unit.NewNamespaceBuilder(). + WithName("foo-ns"). + WithLabel("type", "shard"). + Build(), + }, + request: reconcileRequest, + expectedMetricFormat: routePerShardMetric("foo-ic", 1), + }, + { + name: "ingress controller with routeSelector and with no namespaceSelector with route with incorrect labels in namespace with no labels", + initObjs: []client.Object{ + unit.NewIngressControllerBuilder(). + WithName("foo-ic"). + WithAdmitted(true). + WithRouteSelector("type", "shard"). + Build(), + unit.NewRouteBuilder(). + WithName("foo-route"). + WithNamespace("foo-ns"). + WithAdmittedICs("foo-ic"). + WithLabel("type", "not-shard"). + Build(), + unit.NewNamespaceBuilder(). + WithName("foo-ns"). + Build(), + }, + request: reconcileRequest, + expectedMetricFormat: routePerShardMetric("foo-ic", 0), + }, + { + name: "ingress controller with no routeSelector and with namespaceSelector with route with no labels in namespace with incorrect labels", + initObjs: []client.Object{ + unit.NewIngressControllerBuilder(). + WithName("foo-ic"). + WithAdmitted(true). + WithNamespaceSelector("type", "shard"). + Build(), + unit.NewRouteBuilder(). + WithName("foo-route"). + WithNamespace("foo-ns"). + WithAdmittedICs("foo-ic"). + Build(), + unit.NewNamespaceBuilder(). + WithName("foo-ns"). + WithLabel("type", "not-shard"). + Build(), + }, + request: reconcileRequest, + expectedMetricFormat: routePerShardMetric("foo-ic", 0), + }, + { + name: "ingress controller with routeSelector and with namespaceSelector with route with correct labels in namespace with incorrect labels", + initObjs: []client.Object{ + unit.NewIngressControllerBuilder(). + WithName("foo-ic"). + WithAdmitted(true). + WithRouteSelector("type", "shard"). + WithNamespaceSelector("type", "shard"). + Build(), + unit.NewRouteBuilder(). + WithName("foo-route"). + WithNamespace("foo-ns"). + WithAdmittedICs("foo-ic"). + WithLabel("type", "shard"). + Build(), + unit.NewNamespaceBuilder(). + WithName("foo-ns"). + WithLabel("type", "not-shard"). + Build(), + }, + request: reconcileRequest, + expectedMetricFormat: routePerShardMetric("foo-ic", 0), + }, + { + name: "ingress controller with expression routeSelector and with expression namespaceSelector with route with correct labels in namespace with correct labels", + initObjs: []client.Object{ + unit.NewIngressControllerBuilder(). + WithName("foo-ic"). + WithAdmitted(true). + WithRouteExpressionSelector("type", metav1.LabelSelectorOpIn, []string{"shard"}). + WithNamespaceExpressionSelector("type", metav1.LabelSelectorOpIn, []string{"shard"}). + Build(), + unit.NewRouteBuilder().WithName("foo-route"). + WithNamespace("foo-ns"). + WithAdmittedICs("foo-ic"). + WithLabel("type", "shard"). + Build(), + unit.NewNamespaceBuilder(). + WithName("foo-ns"). + WithLabel("type", "shard"). + Build(), + }, + request: reconcileRequest, + expectedMetricFormat: routePerShardMetric("foo-ic", 1), + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err, _, cache := newFakeClient(tc.initObjs...) + if err != nil { + t.Fatalf("error creating fake client: %v", err) + } + r := reconciler{ + cache: cache, + routeToIngresses: make(map[types.NamespacedName]sets.String), + namespace: operatorcontroller.DefaultOperatorNamespace, + } + + // Cleanup the routes per shard metrics. + routeMetricsControllerRoutesPerShard.Reset() + + if _, err := r.Reconcile(context.Background(), tc.request); err != nil { + t.Errorf("got unexpected error: %v", err) + } else { + err := testutil.CollectAndCompare(routeMetricsControllerRoutesPerShard, strings.NewReader(tc.expectedMetricFormat)) + if err != nil { + t.Error(err) + } + } + }) + } +} + // Test_routeStatusAdmitted verifies that routeStatusAdmitted behaves correctly. func Test_routeStatusAdmitted(t *testing.T) { testCases := []struct { @@ -109,3 +425,38 @@ func Test_routeStatusAdmitted(t *testing.T) { }) } } + +type fakeCache struct { + cache.Informers + client.Reader +} + +// newFakeClient builds a fake client and cache for testing. +func newFakeClient(initObjs ...client.Object) (error, client.Client, cache.Cache) { + // Create fake client + clientBuilder := fake.NewClientBuilder() + s := scheme.Scheme + if err := routev1.Install(s); err != nil { + return err, nil, nil + } + if err := operator.Install(s); err != nil { + return err, nil, nil + } + client := clientBuilder.WithScheme(s).WithObjects(initObjs...).Build() + informer := informertest.FakeInformers{ + Scheme: client.Scheme(), + } + // Create fake cache + cache := fakeCache{Informers: &informer, Reader: client} + + return nil, client, cache +} + +// routePerShardMetric returns a formatted Prometheus string for comparison +func routePerShardMetric(icName string, routesAdmitted int) string { + return fmt.Sprintf(` + # HELP route_metrics_controller_routes_per_shard Report the number of routes for shards (ingress controllers). + # TYPE route_metrics_controller_routes_per_shard gauge + route_metrics_controller_routes_per_shard{shard_name="%s"} %d + `, icName, routesAdmitted) +} diff --git a/test/unit/util.go b/test/unit/util.go new file mode 100644 index 0000000000..7b1e34a997 --- /dev/null +++ b/test/unit/util.go @@ -0,0 +1,230 @@ +package unit + +import ( + "time" + + "github.com/openshift/cluster-ingress-operator/pkg/manifests" + + 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" +) + +type routeBuilder struct { + name string + namespace string + labels map[string]string + admittedICs []string + unAdmittedICs []string +} + +func NewRouteBuilder() *routeBuilder { + return &routeBuilder{ + name: "sample", + namespace: "openshift-ingress", + labels: map[string]string{}, + } +} + +func (b *routeBuilder) WithName(name string) *routeBuilder { + b.name = name + return b +} + +func (b *routeBuilder) WithNamespace(namespace string) *routeBuilder { + b.namespace = namespace + return b +} + +func (b *routeBuilder) WithLabel(key, value string) *routeBuilder { + b.labels[key] = value + 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, + Namespace: b.namespace, + Labels: b.labels, + }, + Spec: routev1.RouteSpec{}, + Status: routev1.RouteStatus{}, + } + + 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, + }, + }, + }) + } + + 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 string + namespace string + namespaceSelectors map[string]string + routeSelectors map[string]string + namespaceExpressionSelector []metav1.LabelSelectorRequirement + routeExpressionSelector []metav1.LabelSelectorRequirement + deleting bool + admittedStatus *v1.OperatorCondition +} + +func NewIngressControllerBuilder() *ingressControllerBuilder { + return &ingressControllerBuilder{ + name: "sample", + namespace: "openshift-ingress-operator", + namespaceSelectors: map[string]string{}, + routeSelectors: map[string]string{}, + } +} + +func (b *ingressControllerBuilder) WithName(name string) *ingressControllerBuilder { + b.name = name + return b +} + +func (b *ingressControllerBuilder) WithNamespace(namespace string) *ingressControllerBuilder { + b.namespace = namespace + return b +} + +func (b *ingressControllerBuilder) WithNamespaceSelector(key, value string) *ingressControllerBuilder { + b.namespaceSelectors[key] = value + return b +} + +func (b *ingressControllerBuilder) WithRouteSelector(key, value string) *ingressControllerBuilder { + b.routeSelectors[key] = value + return b +} + +func (b *ingressControllerBuilder) WithRouteExpressionSelector(key string, operator metav1.LabelSelectorOperator, values []string) *ingressControllerBuilder { + b.routeExpressionSelector = []metav1.LabelSelectorRequirement{{ + Key: key, + Operator: operator, + Values: values, + }} + return b +} + +func (b *ingressControllerBuilder) WithNamespaceExpressionSelector(key string, operator metav1.LabelSelectorOperator, values []string) *ingressControllerBuilder { + b.namespaceExpressionSelector = []metav1.LabelSelectorRequirement{{ + Key: key, + Operator: operator, + Values: values, + }} + return b +} + +func (b *ingressControllerBuilder) IsDeleting() *ingressControllerBuilder { + b.deleting = true + return b +} + +func (b *ingressControllerBuilder) WithAdmitted(admitted bool) *ingressControllerBuilder { + var admittedStatus v1.ConditionStatus + if admitted { + admittedStatus = v1.ConditionTrue + } else { + admittedStatus = v1.ConditionFalse + } + + b.admittedStatus = &v1.OperatorCondition{ + Type: "Admitted", + Status: admittedStatus, + } + return b +} + +func (b *ingressControllerBuilder) Build() *v1.IngressController { + ic := &v1.IngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: b.name, + Namespace: b.namespace, + }, + Spec: v1.IngressControllerSpec{ + RouteSelector: &metav1.LabelSelector{ + MatchLabels: b.routeSelectors, + MatchExpressions: b.routeExpressionSelector, + }, + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: b.namespaceSelectors, + MatchExpressions: b.namespaceExpressionSelector, + }, + }, + } + if b.deleting { + ic.ObjectMeta.DeletionTimestamp = &metav1.Time{Time: time.Now()} + ic.ObjectMeta.Finalizers = []string{manifests.IngressControllerFinalizer} + } + if b.admittedStatus != nil { + ic.Status = v1.IngressControllerStatus{ + Conditions: append(ic.Status.Conditions, *b.admittedStatus), + } + } + return ic +} + +type namespaceBuilder struct { + name string + labels map[string]string +} + +func NewNamespaceBuilder() *namespaceBuilder { + return &namespaceBuilder{ + name: "name", + labels: map[string]string{}, + } +} + +func (b *namespaceBuilder) WithName(name string) *namespaceBuilder { + b.name = name + return b +} + +func (b *namespaceBuilder) WithLabel(key, value string) *namespaceBuilder { + b.labels[key] = value + return b +} + +func (b *namespaceBuilder) Build() *corev1.Namespace { + return &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: b.name, + Labels: b.labels, + }, + } +}