diff --git a/controllers/apps/apimanager_controller.go b/controllers/apps/apimanager_controller.go index 4e29a63dd..d184aef49 100644 --- a/controllers/apps/apimanager_controller.go +++ b/controllers/apps/apimanager_controller.go @@ -40,7 +40,6 @@ import ( subController "github.com/3scale/3scale-operator/controllers/subscription" "github.com/3scale/3scale-operator/pkg/3scale/amp/component" "github.com/3scale/3scale-operator/pkg/3scale/amp/operator" - "github.com/3scale/3scale-operator/pkg/handlers" "github.com/3scale/3scale-operator/pkg/helper" "github.com/3scale/3scale-operator/pkg/reconcilers" "github.com/3scale/3scale-operator/version" @@ -315,10 +314,10 @@ func (r *APIManagerReconciler) SetupWithManager(mgr ctrl.Manager) error { Namespace: r.WatchedNamespace, } - handlers := &handlers.APIManagerRoutesEventMapper{ + zyncRouteMapper := &ZyncRouteToAPIManagerMapper{ Context: r.Context(), K8sClient: r.Client(), - Logger: r.Logger().WithName("APIManagerRoutesHandler"), + Logger: r.Logger().WithName("zyncRouteToAPIManagerMapper"), } operatorNamespace, err := helper.GetOperatorNamespace() @@ -341,7 +340,10 @@ func (r *APIManagerReconciler) SetupWithManager(mgr ctrl.Manager) error { builder.WithPredicates(labelSelectorPredicate), ). Owns(&k8sappsv1.Deployment{}). - Watches(&routev1.Route{}, handler.EnqueueRequestsFromMapFunc(handlers.Map)). + Watches( + &routev1.Route{}, + handler.EnqueueRequestsFromMapFunc(zyncRouteMapper.Map), + ). Watches( &v1.ConfigMap{ ObjectMeta: apimachinerymetav1.ObjectMeta{ diff --git a/controllers/apps/apimanager_status_reconciler_test.go b/controllers/apps/apimanager_status_reconciler_test.go index 04664bb9b..2844dab40 100644 --- a/controllers/apps/apimanager_status_reconciler_test.go +++ b/controllers/apps/apimanager_status_reconciler_test.go @@ -174,11 +174,15 @@ func getRequiredSecrets(namespace string) []runtime.Object { } } -// makeAdmittedRoute returns a Route with the Admitted condition set to True. +// makeAdmittedRoute returns a Route with the Admitted condition set to True and the Zync created-by label. func makeAdmittedRoute(name, namespace, host string) *routev1.Route { return &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, - Spec: routev1.RouteSpec{Host: host}, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: map[string]string{zyncCreatedByLabel: zyncCreatedByValue}, + }, + Spec: routev1.RouteSpec{Host: host}, Status: routev1.RouteStatus{ Ingress: []routev1.RouteIngress{ {Conditions: []routev1.RouteIngressCondition{ diff --git a/controllers/apps/zync_route_to_apimanager_event_mapper.go b/controllers/apps/zync_route_to_apimanager_event_mapper.go new file mode 100644 index 000000000..10e810c2b --- /dev/null +++ b/controllers/apps/zync_route_to_apimanager_event_mapper.go @@ -0,0 +1,57 @@ +package controllers + +import ( + "context" + + "github.com/go-logr/logr" + routev1 "github.com/openshift/api/route/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + appsv1alpha1 "github.com/3scale/3scale-operator/apis/apps/v1alpha1" +) + +const zyncCreatedByLabel = "3scale.net/created-by" +const zyncCreatedByValue = "zync" + +type ZyncRouteToAPIManagerMapper struct { + Context context.Context + K8sClient client.Client + Logger logr.Logger +} + +func (h *ZyncRouteToAPIManagerMapper) Map(ctx context.Context, o client.Object) []reconcile.Request { + route, ok := o.(*routev1.Route) + if !ok { + return nil + } + + apimanagerList := &appsv1alpha1.APIManagerList{} + if err := h.K8sClient.List(ctx, apimanagerList, client.InNamespace(o.GetNamespace())); err != nil { + h.Logger.Error(err, "failed to list APIManagers") + return nil + } + + var requests []reconcile.Request + for i := range apimanagerList.Items { + if routeHostMatchesAPIManager(route.Spec.Host, &apimanagerList.Items[i]) { + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: apimanagerList.Items[i].Name, + Namespace: apimanagerList.Items[i].Namespace, + }, + }) + } + } + return requests +} + +func routeHostMatchesAPIManager(routeHost string, apimanager *appsv1alpha1.APIManager) bool { + for _, expectedHost := range apimanagerExpectedRouteHosts(apimanager) { + if expectedHost == routeHost { + return true + } + } + return false +} diff --git a/controllers/apps/zync_route_to_apimanager_event_mapper_test.go b/controllers/apps/zync_route_to_apimanager_event_mapper_test.go new file mode 100644 index 000000000..e0aab29da --- /dev/null +++ b/controllers/apps/zync_route_to_apimanager_event_mapper_test.go @@ -0,0 +1,132 @@ +package controllers + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + routev1 "github.com/openshift/api/route/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + appsv1alpha1 "github.com/3scale/3scale-operator/apis/apps/v1alpha1" +) + +func TestZyncRouteToAPIManagerMapperMap(t *testing.T) { + const namespace = "test-ns" + const otherNamespace = "other-ns" + + am := getTestAPIManager(namespace) + am.Name = "main-apimanager" + + amOtherNs := getTestAPIManager(otherNamespace) + amOtherNs.Name = "other-apimanager" + + s := scheme.Scheme + s.AddKnownTypes(appsv1alpha1.GroupVersion, am, &appsv1alpha1.APIManagerList{}) + if err := routev1.Install(s); err != nil { + t.Fatal(err) + } + + newRoute := func(host, ns string) *routev1.Route { + return &routev1.Route{ + TypeMeta: metav1.TypeMeta{Kind: "Route", APIVersion: "route.openshift.io/v1"}, + ObjectMeta: metav1.ObjectMeta{Name: "a-route", Namespace: ns}, + Spec: routev1.RouteSpec{Host: host}, + } + } + + enqueuesMainAM := []reconcile.Request{ + {NamespacedName: types.NamespacedName{Name: "main-apimanager", Namespace: namespace}}, + } + + tests := []struct { + name string + objects []runtime.Object + route *routev1.Route + want []reconcile.Request + }{ + { + name: "backend listener route host enqueues APIManager", + objects: []runtime.Object{am}, + route: newRoute("backend-3scale.test.example.com", namespace), + want: enqueuesMainAM, + }, + { + name: "apicast production route host enqueues APIManager", + objects: []runtime.Object{am}, + route: newRoute("api-3scale-apicast-production.test.example.com", namespace), + want: enqueuesMainAM, + }, + { + name: "apicast staging route host enqueues APIManager", + objects: []runtime.Object{am}, + route: newRoute("api-3scale-apicast-staging.test.example.com", namespace), + want: enqueuesMainAM, + }, + { + name: "master portal route host enqueues APIManager", + objects: []runtime.Object{am}, + route: newRoute("master.test.example.com", namespace), + want: enqueuesMainAM, + }, + { + name: "developer portal route host enqueues APIManager", + objects: []runtime.Object{am}, + route: newRoute("3scale.test.example.com", namespace), + want: enqueuesMainAM, + }, + { + name: "admin portal route host enqueues APIManager", + objects: []runtime.Object{am}, + route: newRoute("3scale-admin.test.example.com", namespace), + want: enqueuesMainAM, + }, + { + name: "route host does not match any APIManager — no requests", + objects: []runtime.Object{am}, + route: newRoute("unrelated.example.com", namespace), + want: nil, + }, + { + name: "no APIManagers in namespace — no requests", + objects: []runtime.Object{}, + route: newRoute("backend-3scale.test.example.com", namespace), + want: nil, + }, + { + name: "route in different namespace matches only that namespace's APIManager", + objects: []runtime.Object{am, amOtherNs}, + route: newRoute("backend-3scale.test.example.com", otherNamespace), + want: []reconcile.Request{ + {NamespacedName: types.NamespacedName{Name: "other-apimanager", Namespace: otherNamespace}}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cl := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(tt.objects...).Build() + mapper := &ZyncRouteToAPIManagerMapper{ + Context: context.TODO(), + K8sClient: cl, + Logger: logr.Discard(), + } + + got := mapper.Map(context.TODO(), tt.route) + + if len(got) != len(tt.want) { + t.Fatalf("Map() returned %d requests, want %d: got %v", len(got), len(tt.want), got) + } + for i := range tt.want { + if got[i] != tt.want[i] { + t.Errorf("Map()[%d] = %v, want %v", i, got[i], tt.want[i]) + } + } + }) + } +} diff --git a/pkg/handlers/apimanager_managed_routes_event_mapper.go b/pkg/handlers/apimanager_managed_routes_event_mapper.go deleted file mode 100644 index a82583853..000000000 --- a/pkg/handlers/apimanager_managed_routes_event_mapper.go +++ /dev/null @@ -1,101 +0,0 @@ -package handlers - -import ( - "context" - - appscommon "github.com/3scale/3scale-operator/apis/apps" - appsv1alpha1 "github.com/3scale/3scale-operator/apis/apps/v1alpha1" - "github.com/3scale/3scale-operator/pkg/3scale/amp/component" - "github.com/3scale/3scale-operator/pkg/reconcilers" - - "github.com/go-logr/logr" - k8sappsv1 "k8s.io/api/apps/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/reconcile" -) - -// APIManagerRoutesEventMapper is an EventHandler that maps an existing OpenShift -// route to an APIManager. This handler should only be used on Route objects -// and when APIManager is used. -type APIManagerRoutesEventMapper struct { - Context context.Context - K8sClient client.Client - Logger logr.Logger -} - -func (h *APIManagerRoutesEventMapper) Map(ctx context.Context, o client.Object) []reconcile.Request { - var res []reconcile.Request - apimanagerReconcileRequest := h.getAPIManagerOwnerReconcileRequest(o) - if apimanagerReconcileRequest != nil { - res = append(res, *apimanagerReconcileRequest) - } - return res -} - -func (h *APIManagerRoutesEventMapper) getAPIManagerOwnerReconcileRequest(object metav1.Object) *reconcile.Request { - h.Logger.V(2).Info("Processing meta object", "Name", object.GetName(), "Namespace", object.GetNamespace()) - - for _, ref := range object.GetOwnerReferences() { - refGV, err := schema.ParseGroupVersion(ref.APIVersion) - if err != nil { - h.Logger.Error(err, "Could not parse OwnerReference APIVersion", - "api version", ref.APIVersion) - return nil - } - - h.Logger.V(2).Info("Evaluating OwnerReference", "GroupVersion", refGV, "Kind", ref.Kind, "Name", ref.Name) - // Compare the OwnerReference Group and Kind against the APIManager Kind - // or the name of the Zync Que Deployment. - - // If the OwnerReference of the received object is an APIManager we - // return a reconcile Request using the name from the OwnerReference and the - // Namespace from the object in the event - apimanagerKind := appscommon.APIManagerKind - apimanagerGroup := appsv1alpha1.GroupVersion.Group - if ref.Kind == apimanagerKind && refGV.Group == apimanagerGroup { - h.Logger.V(2).Info("APIManager OwnerReference detected. Reenqueuing as APIManager event", "APIManager name", ref.Name, "APIManager namespace", object.GetNamespace()) - // Match found - add a Request for the object referred to in the OwnerReference - request := &reconcile.Request{NamespacedName: types.NamespacedName{ - Name: ref.Name, - Namespace: object.GetNamespace(), - }} - return request - } - - // If the OwnerReference of the received object is a Deployment and - // its name is Zync Que's name then we fetch that Object and recursively - // try to find an OwnerReference that is an APIManager. If it is found - // we return it. - zyncQueKind := reconcilers.DeploymentKind - zyncQueGroup := k8sappsv1.GroupName - // An alternative to hardcode Zync-Que name would be just try to recurse - // OwnerReferences until there are no more of them. That would be - // potentially more costly. - zyncQueDeploymentName := component.ZyncQueDeploymentName - if ref.Kind == zyncQueKind && refGV.Group == zyncQueGroup && ref.Name == zyncQueDeploymentName { - h.Logger.V(2).Info("OwnerReference to Zync-Que detected. Recursively looking for APIManager OwnerReferences...") - existing := &k8sappsv1.Deployment{} - getErr := h.K8sClient.Get(context.Background(), types.NamespacedName{Name: ref.Name, Namespace: object.GetNamespace()}, existing) - if getErr != nil { - // If there's an error getting the object it might be due to - // it might have been deleted already or any other kind of error. - // In both cases we log it and ignore it and we continue the processing. - h.Logger.Error(getErr, "Could not get object", - "Kind", ref.Kind, "APIVersion", ref.APIVersion, "Name", ref.Name, "Namespace", object.GetNamespace()) - } else { - // Recursively try to find an APIManager OwnerReference - request := h.getAPIManagerOwnerReconcileRequest(existing) - if request != nil { - return request - } - } - } - } - - h.Logger.V(3).Info("No APIManager OwnerReference detected") - - return nil -} diff --git a/pkg/handlers/apimanager_managed_routes_event_mapper_test.go b/pkg/handlers/apimanager_managed_routes_event_mapper_test.go deleted file mode 100644 index c869fb927..000000000 --- a/pkg/handlers/apimanager_managed_routes_event_mapper_test.go +++ /dev/null @@ -1,185 +0,0 @@ -package handlers - -import ( - "context" - "reflect" - "testing" - - "github.com/go-logr/logr" - routev1 "github.com/openshift/api/route/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/kubernetes/scheme" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - - appscommon "github.com/3scale/3scale-operator/apis/apps" - appsv1alpha1 "github.com/3scale/3scale-operator/apis/apps/v1alpha1" - "github.com/3scale/3scale-operator/pkg/3scale/amp/component" - "github.com/3scale/3scale-operator/pkg/reconcilers" - k8sappsv1 "k8s.io/api/apps/v1" -) - -func TestAPIManagerRoutesEventMapperMap(t *testing.T) { - apimanagerName := "apimanagerName" - apimanagerNamespace := "examplenamespace" - apimanager := &appsv1alpha1.APIManager{ - ObjectMeta: metav1.ObjectMeta{ - Name: apimanagerName, - Namespace: apimanagerNamespace, - }, - TypeMeta: metav1.TypeMeta{ - APIVersion: appsv1alpha1.GroupVersion.String(), - Kind: appscommon.APIManagerKind, - }, - } - - zyncQue := &k8sappsv1.Deployment{ - TypeMeta: metav1.TypeMeta{ - Kind: reconcilers.DeploymentKind, - APIVersion: reconcilers.DeploymentAPIVersion, - }, - ObjectMeta: metav1.ObjectMeta{ - Name: component.ZyncQueDeploymentName, - Namespace: apimanagerNamespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: appsv1alpha1.GroupVersion.String(), - Kind: appscommon.APIManagerKind, - Name: apimanager.Name, - }, - }, - }, - } - - objs := []runtime.Object{zyncQue} - - s := scheme.Scheme - s.AddKnownTypes(appsv1alpha1.GroupVersion, apimanager) - err := k8sappsv1.AddToScheme(s) - if err != nil { - t.Fatal(err) - } - - err = routev1.Install(s) - if err != nil { - t.Fatal(err) - } - - // Create a fake client to mock API calls. - cl := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(objs...).Build() - - apimanagerRoutesEventMapper := APIManagerRoutesEventMapper{ - K8sClient: cl, - Logger: logr.Discard(), - } - - cases := []struct { - testName string - input client.Object - expected []reconcile.Request - }{ - { - testName: "Event with route directly owned by APIManager is converted to an APIManager event", - input: &routev1.Route{ - TypeMeta: metav1.TypeMeta{ - Kind: "Route", - APIVersion: "route.openshift.io/v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "routeDirectlyManagedByAPIManager", - Namespace: apimanagerNamespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: "v1", - Kind: "Secret", - Name: "asecret", - }, - { - APIVersion: appsv1alpha1.GroupVersion.String(), - Kind: appscommon.APIManagerKind, - Name: apimanager.Name, - }, - }, - }, - }, - expected: []reconcile.Request{ - {NamespacedName: types.NamespacedName{Namespace: apimanagerNamespace, Name: apimanagerName}}, - }, - }, - { - testName: "Event with route owned by zync-que managed by APIManager is converted to an APIManager event", - input: &routev1.Route{ - TypeMeta: metav1.TypeMeta{ - Kind: "Route", - APIVersion: "route.openshift.io/v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "routeManagedByZyncQue", - Namespace: apimanagerNamespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: "v1", - Kind: "Secret", - Name: "asecret", - }, - { - APIVersion: reconcilers.DeploymentAPIVersion, - Kind: reconcilers.DeploymentKind, - Name: component.ZyncQueDeploymentName, - }, - }, - }, - }, - expected: []reconcile.Request{ - {NamespacedName: types.NamespacedName{Namespace: apimanagerNamespace, Name: apimanagerName}}, - }, - }, - { - testName: "Event with route without OwnerReferences is discarded", - input: &routev1.Route{ - TypeMeta: metav1.TypeMeta{ - Kind: "Route", - APIVersion: "route.openshift.io/v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "aroute", - Namespace: apimanagerNamespace, - }, - }, - expected: nil, - }, - { - testName: "Event with route with non-APIManager OwnerReference (directly or indirectly) is discarded", - input: &routev1.Route{ - TypeMeta: metav1.TypeMeta{ - Kind: "Route", - APIVersion: "route.openshift.io/v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "aroute", - Namespace: apimanagerNamespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: "v1", - Kind: "Secret", - Name: "asecret", - }, - }, - }, - }, - expected: nil, - }, - } - - for _, tc := range cases { - t.Run(tc.testName, func(subT *testing.T) { - res := apimanagerRoutesEventMapper.Map(context.TODO(), tc.input) - if !reflect.DeepEqual(res, tc.expected) { - subT.Errorf("Unexpected result: %v. Expected: %v", res, tc.expected) - } - }) - } -} diff --git a/test/integration/apimanager_controller_test.go b/test/integration/apimanager_controller_test.go index b0b0c4bd8..0e106213e 100644 --- a/test/integration/apimanager_controller_test.go +++ b/test/integration/apimanager_controller_test.go @@ -211,6 +211,26 @@ var _ = Describe("APIManager controller", func() { waitForAPIManagerAvailableCondition(5*time.Second, 15*time.Minute, apimanager, GinkgoWriter) fmt.Fprintf(GinkgoWriter, "APIManager 'Available' condition is true\n") + // Verify that route watch events trigger status reconciliation for routes + // not created by Zync (regression test for THREESCALE-14653: the old ownerRef-chain + // mapper could not map route events to an APIManager when Zync was absent). + // Use a Zync-managed route so that deleting and recreating it without the + // Zync label exercises the domain-name mapper path (THREESCALE-14653). + zyncRouteHost := "api-3scale-apicast-production." + wildcardDomain + fmt.Fprintf(GinkgoWriter, "Deleting Zync-managed route '%s' to trigger unavailability\n", zyncRouteHost) + routeTo := deleteRouteByHost(testNamespace, zyncRouteHost) + + fmt.Fprintf(GinkgoWriter, "Waiting until APIManager's 'Available' condition is false after route deletion\n") + waitForAPIManagerUnavailableCondition(5*time.Second, 2*time.Minute, apimanager, GinkgoWriter) + fmt.Fprintf(GinkgoWriter, "APIManager 'Available' condition is false\n") + + fmt.Fprintf(GinkgoWriter, "Recreating route '%s' manually (no Zync label)\n", zyncRouteHost) + createRouteManually(testNamespace, zyncRouteHost, routeTo) + + fmt.Fprintf(GinkgoWriter, "Waiting until APIManager's 'Available' condition is true after manual route creation\n") + waitForAPIManagerAvailableCondition(5*time.Second, 5*time.Minute, apimanager, GinkgoWriter) + fmt.Fprintf(GinkgoWriter, "APIManager 'Available' condition is true after manual route creation\n") + elapsed := time.Since(start) fmt.Fprintf(GinkgoWriter, "APIManager creation and availability took '%s'\n", elapsed) }) @@ -965,6 +985,42 @@ func waitForAPIManagerAvailableCondition(retryInterval, timeout time.Duration, a }, timeout, retryInterval).Should(BeTrue()) } +func waitForAPIManagerUnavailableCondition(retryInterval, timeout time.Duration, apimanager *appsv1alpha1.APIManager, w io.Writer) { + Eventually(func() bool { + err := testK8sClient.Get(context.Background(), types.NamespacedName{Name: apimanager.Name, Namespace: apimanager.Namespace}, apimanager) + if err != nil { + fmt.Fprintf(w, "Error getting APIManager '%s': %v\n", apimanager.Name, err) + return false + } + return !apimanager.Status.Conditions.IsTrueFor(appsv1alpha1.APIManagerAvailableConditionType) + }, timeout, retryInterval).Should(BeTrue()) +} + +// deleteRouteByHost deletes the route with the given host and returns its spec.to +// so the caller can recreate the route pointing at the same backend service. +func deleteRouteByHost(namespace, host string) routev1.RouteTargetReference { + routeList := &routev1.RouteList{} + Expect(testK8sAPIClient.List(context.Background(), routeList, &client.ListOptions{ + Namespace: namespace, + FieldSelector: fields.OneTermEqualSelector("spec.host", host), + })).To(Succeed()) + Expect(routeList.Items).To(HaveLen(1)) + to := routeList.Items[0].Spec.To + Expect(testK8sClient.Delete(context.Background(), &routeList.Items[0])).To(Succeed()) + return to +} + +func createRouteManually(namespace, host string, to routev1.RouteTargetReference) { + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "manual-" + host, + Namespace: namespace, + }, + Spec: routev1.RouteSpec{Host: host, To: to}, + } + Expect(testK8sClient.Create(context.Background(), route)).To(Succeed()) +} + func waitForAPIManagerLabels(namespace string, retryInterval time.Duration, timeout time.Duration, apimanager *appsv1alpha1.APIManager, customEnvSecret *corev1.Secret, w io.Writer) { Eventually(func() bool { reconciledApimanager := &appsv1alpha1.APIManager{}