From b11e6ee2b8f31d551385c0c659fb043f067a4aa0 Mon Sep 17 00:00:00 2001 From: Howard Gao Date: Sun, 30 Jun 2024 22:28:13 +0800 Subject: [PATCH] [#967] Operator fails to recover route/ingress when labels/host modified --- .../activemqartemis_controller2_test.go | 91 ++++++++++++++++++- controllers/common_util_test.go | 8 ++ pkg/resources/ingresses/ingress.go | 18 ++-- pkg/resources/routes/route.go | 30 ++---- 4 files changed, 114 insertions(+), 33 deletions(-) diff --git a/controllers/activemqartemis_controller2_test.go b/controllers/activemqartemis_controller2_test.go index 87b0716e2..17fb50d09 100644 --- a/controllers/activemqartemis_controller2_test.go +++ b/controllers/activemqartemis_controller2_test.go @@ -20,6 +20,7 @@ package controllers import ( "os" + "reflect" "github.com/artemiscloud/activemq-artemis-operator/pkg/resources/volumes" "github.com/artemiscloud/activemq-artemis-operator/pkg/utils/common" @@ -29,10 +30,12 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + netv1 "k8s.io/api/networking/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/types" brokerv1beta1 "github.com/artemiscloud/activemq-artemis-operator/api/v1beta1" + routev1 "github.com/openshift/api/route/v1" ) var _ = Describe("artemis controller 2", func() { @@ -49,7 +52,18 @@ var _ = Describe("artemis controller 2", func() { It("controller resource recover test", Label("controller-resource-recover-test"), func() { By("deploy a broker cr") - _, crd := DeployCustomBroker(defaultNamespace, nil) + acceptorName := "amqp" + _, crd := DeployCustomBroker(defaultNamespace, func(candidate *brokerv1beta1.ActiveMQArtemis) { + candidate.Spec.Acceptors = []brokerv1beta1.AcceptorType{ + { + Name: acceptorName, + Protocols: "amqp", + Port: 5672, + Expose: true, + }, + } + candidate.Spec.IngressDomain = "artemiscloud.io" + }) brokerKey := types.NamespacedName{ Name: crd.Name, @@ -78,8 +92,83 @@ var _ = Describe("artemis controller 2", func() { g.Expect(newSecretId).ShouldNot(Equal(secretId)) }, timeout, interval).Should(Succeed()) + if os.Getenv("USE_EXISTING_CLUSTER") == "true" && isOpenshift { + By("modify route labels") + routeKey := types.NamespacedName{ + Name: crd.Name + "-" + acceptorName + "-0-" + "svc-rte", + Namespace: defaultNamespace, + } + + acceptorRoute := routev1.Route{} + var originalLables map[string]string + // compare resource version as there will be an update + var routeVersion string + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, routeKey, &acceptorRoute)).Should(Succeed()) + originalLables = CloneStringMap(acceptorRoute.Labels) + routeVersion = acceptorRoute.ResourceVersion + g.Expect(len(acceptorRoute.Labels) > 0).To(BeTrue()) + for k, v := range acceptorRoute.Labels { + acceptorRoute.Labels[k] = v + "change" + } + g.Expect(k8sClient.Update(ctx, &acceptorRoute)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, routeKey, &acceptorRoute)).Should(Succeed()) + newRouteVersion := acceptorRoute.ResourceVersion + g.Expect(newRouteVersion).ShouldNot(Equal(routeVersion)) + g.Expect(reflect.DeepEqual(originalLables, acceptorRoute.Labels)).Should(BeTrue()) + }, timeout, interval).Should(Succeed()) + } else { + By("modify ingress labels") + ingKey := types.NamespacedName{ + Name: crd.Name + "-" + acceptorName + "-0-" + "svc-ing", + Namespace: defaultNamespace, + } + + acceptorIng := netv1.Ingress{} + var originalLables map[string]string + // compare resource version as there will be an update + var ingVersion string + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, ingKey, &acceptorIng)).Should(Succeed()) + originalLables = CloneStringMap(acceptorIng.Labels) + ingVersion = acceptorIng.ResourceVersion + g.Expect(len(acceptorIng.Labels) > 0).To(BeTrue()) + for k, v := range acceptorIng.Labels { + acceptorIng.Labels[k] = v + "change" + } + g.Expect(k8sClient.Update(ctx, &acceptorIng)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, ingKey, &acceptorIng)).Should(Succeed()) + newIngVersion := acceptorIng.ResourceVersion + g.Expect(newIngVersion).ShouldNot(Equal(ingVersion)) + g.Expect(reflect.DeepEqual(originalLables, acceptorIng.Labels)).Should(BeTrue()) + }, timeout, interval).Should(Succeed()) + + By("modifying ingress host") + originalHost := "" + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, ingKey, &acceptorIng)).Should(Succeed()) + originalHost = acceptorIng.Spec.Rules[0].Host + ingVersion = acceptorIng.ResourceVersion + acceptorIng.Spec.Rules[0].Host = originalHost + "s" + g.Expect(k8sClient.Update(ctx, &acceptorIng)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, ingKey, &acceptorIng)).Should(Succeed()) + newIngVersion := acceptorIng.ResourceVersion + g.Expect(newIngVersion).ShouldNot(Equal(ingVersion)) + g.Expect(acceptorIng.Spec.Rules[0].Host).Should(Equal(originalHost)) + }, timeout, interval).Should(Succeed()) + } CleanResource(crd, crd.Name, defaultNamespace) }) + It("external volumes attach", func() { if os.Getenv("USE_EXISTING_CLUSTER") == "true" { diff --git a/controllers/common_util_test.go b/controllers/common_util_test.go index 3edf55591..50ed50987 100644 --- a/controllers/common_util_test.go +++ b/controllers/common_util_test.go @@ -1131,3 +1131,11 @@ func (m *NillCluster) GetAPIReader() client.Reader { func (m *NillCluster) Start(ctx context.Context) error { return nil } + +func CloneStringMap(original map[string]string) map[string]string { + copy := make(map[string]string) + for key, value := range original { + copy[key] = value + } + return copy +} diff --git a/pkg/resources/ingresses/ingress.go b/pkg/resources/ingresses/ingress.go index 927b8bc29..34e9dad1b 100644 --- a/pkg/resources/ingresses/ingress.go +++ b/pkg/resources/ingresses/ingress.go @@ -11,23 +11,21 @@ func NewIngressForCRWithSSL(existing *netv1.Ingress, namespacedName types.Namesp pathType := netv1.PathTypePrefix - var desired *netv1.Ingress - if existing == nil { + var desired *netv1.Ingress = existing + if desired == nil { desired = &netv1.Ingress{ TypeMeta: metav1.TypeMeta{ APIVersion: "networking.k8s.io/v1", Kind: "Ingress", }, - ObjectMeta: metav1.ObjectMeta{ - Labels: labels, - Name: targetServiceName + "-ing", - Namespace: namespacedName.Namespace, - }, - Spec: netv1.IngressSpec{}, + ObjectMeta: metav1.ObjectMeta{}, + Spec: netv1.IngressSpec{}, } - } else { - desired = existing } + //apply desired + desired.ObjectMeta.Labels = labels + desired.ObjectMeta.Name = targetServiceName + "-ing" + desired.ObjectMeta.Namespace = namespacedName.Namespace desired.Spec.Rules = []netv1.IngressRule{ { diff --git a/pkg/resources/routes/route.go b/pkg/resources/routes/route.go index 3760eba5e..e07e03b87 100644 --- a/pkg/resources/routes/route.go +++ b/pkg/resources/routes/route.go @@ -9,35 +9,21 @@ import ( func NewRouteDefinitionForCR(existing *routev1.Route, namespacedName types.NamespacedName, labels map[string]string, targetServiceName string, targetPortName string, passthroughTLS bool, domain string, brokerHost string) *routev1.Route { - var desired *routev1.Route = nil - if existing == nil { + desired := existing + if desired == nil { desired = &routev1.Route{ TypeMeta: metav1.TypeMeta{ APIVersion: "v1", Kind: "Route", }, - ObjectMeta: metav1.ObjectMeta{ - Labels: labels, - Name: targetServiceName + "-rte", - Namespace: namespacedName.Namespace, - }, - Spec: routev1.RouteSpec{}, - } - - } else { - desired = &routev1.Route{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "v1", - Kind: "Route", - }, - ObjectMeta: metav1.ObjectMeta{ - Labels: existing.Labels, - Name: existing.Name, - Namespace: namespacedName.Namespace, - }, - Spec: existing.Spec, + ObjectMeta: metav1.ObjectMeta{}, + Spec: routev1.RouteSpec{}, } } + //apply desired + desired.ObjectMeta.Labels = labels + desired.ObjectMeta.Name = targetServiceName + "-rte" + desired.ObjectMeta.Namespace = namespacedName.Namespace if brokerHost != "" { desired.Spec.Host = brokerHost