From 89fa4fdfeec2d58fc5a4da3d2733ef65e9a00cb5 Mon Sep 17 00:00:00 2001 From: Jacek Ewertowski Date: Fri, 12 Apr 2024 15:43:50 +0200 Subject: [PATCH] OSSM-6295: Copy all annotations and labels added by ArgoCD except `argocd.argoproj.io/instance` (#980) * OSSM-6295: Copy ArgoCD annotations from Gateway to Route Signed-off-by: Jacek Ewertowski * OSSM-6295: Copy all annotations and labels added by ArgoCD except argocd.argoproj.io/instance Signed-off-by: Jacek Ewertowski * Fix lint errors Signed-off-by: Jacek Ewertowski --------- Co-authored-by: Jacek Ewertowski Signed-off-by: Yann Liu --- pilot/pkg/config/kube/ior/controller.go | 3 +- pilot/pkg/config/kube/ior/ior_test.go | 296 +++++++++++++----------- 2 files changed, 157 insertions(+), 142 deletions(-) diff --git a/pilot/pkg/config/kube/ior/controller.go b/pilot/pkg/config/kube/ior/controller.go index c6314a3d5d..322aa4480d 100644 --- a/pilot/pkg/config/kube/ior/controller.go +++ b/pilot/pkg/config/kube/ior/controller.go @@ -125,7 +125,6 @@ func filteredRouteAnnotation(routeAnnotation string) bool { filteredPrefixes := [...]string{ ShouldManageRouteAnnotation, "kubectl.kubernetes.io", - "argocd.argoproj.io", } for _, prefix := range filteredPrefixes { @@ -139,7 +138,7 @@ func filteredRouteAnnotation(routeAnnotation string) bool { func filteredRouteLabel(routeLabel string) bool { filteredPrefixes := [...]string{ maistraPrefix, - "argocd.argoproj.io", + "argocd.argoproj.io/instance", } for _, prefix := range filteredPrefixes { diff --git a/pilot/pkg/config/kube/ior/ior_test.go b/pilot/pkg/config/kube/ior/ior_test.go index f635d0c8a4..8395f860ca 100644 --- a/pilot/pkg/config/kube/ior/ior_test.go +++ b/pilot/pkg/config/kube/ior/ior_test.go @@ -94,154 +94,157 @@ func initClients( func TestCreate(t *testing.T) { cases := []struct { - testName string - ns string - hosts []string - gwSelector map[string]string - expectedRoutes int - expectedError string - tls bool - annotations map[string]string + testName string + ns string + hosts []string + gwSelector map[string]string + expectedRoutes int + expectedError string + tls bool + gatewayAnnotations map[string]string + routeAnnotations map[string]string + gatewayLabels map[string]string + routeLabels map[string]string }{ { - "One host", - "istio-system", - []string{"one.org"}, - map[string]string{"istio": "ingressgateway"}, - 1, - "", - false, - nil, + testName: "One host", + ns: "istio-system", + hosts: []string{"one.org"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + expectedRoutes: 1, }, { - "Two hosts", - "istio-system", - []string{"two.org", "three.com"}, - map[string]string{"istio": "ingressgateway"}, - 2, - "", - false, - nil, + testName: "Two hosts", + ns: "istio-system", + hosts: []string{"two.org", "three.com"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + expectedRoutes: 2, }, { - "Wildcard 1", - "istio-system", - []string{"*"}, - map[string]string{"istio": "ingressgateway"}, - 1, - "", - false, - nil, + testName: "Wildcard 1", + ns: "istio-system", + hosts: []string{"*"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + expectedRoutes: 1, }, { - "Wildcard 2", - "istio-system", - []string{"*.a.com"}, - map[string]string{"istio": "ingressgateway"}, - 1, - "", - false, - nil, + testName: "Wildcard 2", + ns: "istio-system", + hosts: []string{"*.a.com"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + expectedRoutes: 1, }, { - "Invalid gateway", - "istio-system", - []string{"fail.com"}, - map[string]string{"istio": "nonexistent"}, - 0, - "could not find a service that matches the gateway selector `istio=nonexistent'", - false, - nil, + testName: "Invalid gateway", + ns: "istio-system", + hosts: []string{"fail.com"}, + gwSelector: map[string]string{"istio": "nonexistent"}, + expectedError: "could not find a service that matches the gateway selector `istio=nonexistent'", }, { - "TLS 1", - "istio-system", - []string{"one.org"}, - map[string]string{"istio": "ingressgateway"}, - 1, - "", - true, - nil, + testName: "TLS 1", + ns: "istio-system", + hosts: []string{"one.org"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + expectedRoutes: 1, + tls: true, }, { - "Gateway not managed", - "istio-system", - []string{"notmanaged.org"}, - map[string]string{"istio": "ingressgateway"}, - 0, - "", - false, - map[string]string{ShouldManageRouteAnnotation: "false", "foo": "bar"}, + testName: "Gateway not managed", + ns: "istio-system", + hosts: []string{"notmanaged.org"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + gatewayAnnotations: map[string]string{ShouldManageRouteAnnotation: "false", "foo": "bar"}, + routeAnnotations: map[string]string{"foo": "bar"}, }, { - "Gateway explicitly managed", - "istio-system", - []string{"explicitlymanaged.org"}, - map[string]string{"istio": "ingressgateway"}, - 1, - "", - false, - map[string]string{ShouldManageRouteAnnotation: "TRUE", "foo": "bar"}, + testName: "Gateway explicitly managed", + ns: "istio-system", + hosts: []string{"explicitlymanaged.org"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + expectedRoutes: 1, + gatewayAnnotations: map[string]string{ShouldManageRouteAnnotation: "TRUE", "foo": "bar"}, + routeAnnotations: map[string]string{originalHostAnnotation: "explicitlymanaged.org", "foo": "bar"}, }, { - "Gateway explicitly managed with an invalid value", - "istio-system", - []string{"explicitlymanaged.org"}, - map[string]string{"istio": "ingressgateway"}, - 0, - fmt.Sprintf("could not parse annotation %q:", ShouldManageRouteAnnotation), - false, - map[string]string{ShouldManageRouteAnnotation: "ABC", "foo": "bar"}, + testName: "Gateway explicitly managed with an invalid value", + ns: "istio-system", + hosts: []string{"explicitlymanaged.org"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + expectedError: fmt.Sprintf("could not parse annotation %q:", ShouldManageRouteAnnotation), + gatewayAnnotations: map[string]string{ShouldManageRouteAnnotation: "ABC", "foo": "bar"}, + routeAnnotations: map[string]string{"foo": "bar"}, }, { - "Egress gateway must be ignored", - "istio-system", - []string{"egress.org"}, - map[string]string{"istio": "egressgateway"}, - 0, - "", - false, - nil, + testName: "all annotations except kubectl.kubernetes.io should be copied from Gateway to Route", + ns: "istio-system", + hosts: []string{"istio.io"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + expectedRoutes: 1, + gatewayAnnotations: map[string]string{ + "foo": "bar", "argocd.argoproj.io/sync-options": "Prune=false", + "kubectl.kubernetes.io/last-applied-configuration": "{}", + }, + routeAnnotations: map[string]string{ + "foo": "bar", "argocd.argoproj.io/sync-options": "Prune=false", + originalHostAnnotation: "istio.io", + }, }, { - "Host with all namespaces", - "istio-system", - []string{"*/one.org"}, - map[string]string{"istio": "ingressgateway"}, - 1, - "", - false, - nil, + testName: "all labels except maistra.io and argocd.argoproj.io/instance should be copied from Gateway to Route", + ns: "istio-system", + hosts: []string{"istio.io"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + expectedRoutes: 1, + gatewayLabels: map[string]string{ + "argocd.argoproj.io/instance": "app", + "argocd.argoproj.io/secret-type": "cluster", + "foo": "bar", + "maistra.io/manageRoute": "false", + "maistra.io/gateway-name": "random", + }, + routeLabels: map[string]string{ + "argocd.argoproj.io/secret-type": "cluster", + "foo": "bar", + "maistra.io/gateway-name": "gw10", + "maistra.io/gateway-namespace": "istio-system", + "maistra.io/gateway-resourceVersion": "1", + "maistra.io/generated-by": "ior", + }, }, { - "Host with current namespace", - "istio-system", - []string{"./one.org"}, - map[string]string{"istio": "ingressgateway"}, - 1, - "", - false, - nil, + testName: "Egress gateway must be ignored", + ns: "istio-system", + hosts: []string{"egress.org"}, + gwSelector: map[string]string{"istio": "egressgateway"}, }, { - "Host with a specific namespace", - "istio-system", - []string{"ns1/one.org"}, - map[string]string{"istio": "ingressgateway"}, - 1, - "", - false, - nil, + testName: "Host with all namespaces", + ns: "istio-system", + hosts: []string{"*/one.org"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + expectedRoutes: 1, }, { - "Host with a namespace and wildcard", - "istio-system", - []string{"*/*.one.org"}, - map[string]string{"istio": "ingressgateway"}, - 1, - "", - false, - nil, + testName: "Host with current namespace", + ns: "istio-system", + hosts: []string{"./one.org"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + expectedRoutes: 1, + }, + { + testName: "Host with a specific namespace", + ns: "istio-system", + hosts: []string{"ns1/one.org"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + expectedRoutes: 1, + }, + { + testName: "Host with a namespace and wildcard", + ns: "istio-system", + hosts: []string{"*/*.one.org"}, + gwSelector: map[string]string{"istio": "ingressgateway"}, + expectedRoutes: 1, }, } @@ -257,13 +260,13 @@ func TestCreate(t *testing.T) { for i, c := range cases { t.Run(c.testName, func(t *testing.T) { gatewayName := fmt.Sprintf("gw%d", i) - createGateway(t, store, c.ns, gatewayName, c.hosts, c.gwSelector, c.tls, c.annotations) + createGateway(t, store, c.ns, gatewayName, c.hosts, c.gwSelector, c.tls, c.gatewayAnnotations, c.gatewayLabels) list := getRoutes(t, routerClient, controlPlaneNs, c.expectedRoutes, time.Second) // Only continue the validation if any route is expected to be created if c.expectedRoutes > 0 { - validateRoutes(t, c.hosts, list, gatewayName, c.tls) + validateRoutes(t, c.hosts, list, gatewayName, c.tls, c.routeAnnotations, c.routeLabels) // Remove the gateway and expect all routes get removed deleteGateway(t, k8sClient.GetActualClient().Istio(), c.ns, gatewayName) @@ -273,7 +276,9 @@ func TestCreate(t *testing.T) { } } -func validateRoutes(t *testing.T, hosts []string, list *[]*routeapiv1.Route, gatewayName string, tls bool) { +func validateRoutes(t *testing.T, hosts []string, list *[]*routeapiv1.Route, gatewayName string, tls bool, + expectedAnnotations map[string]string, expectedLabels map[string]string, +) { for _, host := range hosts { route := findRouteByHost(list, host) if route == nil { @@ -281,17 +286,31 @@ func validateRoutes(t *testing.T, hosts []string, list *[]*routeapiv1.Route, gat } // Check metadata - if route.Labels[gatewayNameLabel] != gatewayName { - t.Fatalf("wrong label, expecting %s, got %s", gatewayName, route.Annotations[gatewayNameLabel]) - } - if route.Annotations["foo"] != "bar" { - t.Fatal("gateway annotations were not copied to route") + if expectedAnnotations != nil { + if len(route.Annotations) != len(expectedAnnotations) { + t.Fatalf("route was created with unexpected annotations: %s", route.Annotations) + } + for key, val := range expectedAnnotations { + if route.Annotations[key] != val { + t.Fatalf("annotation '%s' was not copied to route: expected '%s', got '%s'", key, val, route.Annotations[key]) + } + } } if _, found := route.Annotations[ShouldManageRouteAnnotation]; found { t.Fatalf("annotation %q should not be copied to the route", ShouldManageRouteAnnotation) } - if route.Labels["foo"] != "bar" { - t.Fatal("gateway labels were not copied to route") + if expectedLabels != nil { + if len(route.Labels) != len(expectedLabels) { + t.Fatalf("route was created with unexpected labels: %s", route.Labels) + } + for key, val := range expectedLabels { + if route.Labels[key] != val { + t.Fatalf("label '%s' was not copied to route: expected '%s', got '%s'", key, val, route.Labels[key]) + } + } + } + if route.Labels[gatewayNameLabel] != gatewayName { + t.Fatalf("wrong label, expecting %s, got %s", gatewayName, route.Annotations[gatewayNameLabel]) } if _, found := route.Labels[prefixedLabel]; found { t.Fatalf("label %q should not be copied to the route", prefixedLabel) @@ -382,7 +401,7 @@ func TestEdit(t *testing.T) { controlPlane := "istio-system" createIngressGateway(t, k8sClient.GetActualClient(), controlPlane, map[string]string{"istio": "ingressgateway"}) - createGateway(t, store, controlPlane, "gw", []string{"abc.com"}, map[string]string{"istio": "ingressgateway"}, false, nil) + createGateway(t, store, controlPlane, "gw", []string{"abc.com"}, map[string]string{"istio": "ingressgateway"}, false, nil, nil) list := getRoutes(t, routerClient, controlPlane, 1, time.Second) @@ -391,7 +410,7 @@ func TestEdit(t *testing.T) { editGateway(t, store, c.ns, "gw", c.hosts, c.gwSelector, c.tls, fmt.Sprintf("%d", i+2)) list = getRoutes(t, routerClient, controlPlane, c.expectedRoutes, time.Second) - validateRoutes(t, c.hosts, list, "gw", c.tls) + validateRoutes(t, c.hosts, list, "gw", c.tls, nil, nil) }) } } @@ -451,10 +470,10 @@ func TestStatelessness(t *testing.T) { runClients(store, kubeClient, stop) createIngressGateway(t, kubeClient.GetActualClient(), watchedNamespace, map[string]string{"istio": "ingressgateway"}) - createGateway(t, store, initialState.ns, initialState.name, initialState.hosts, map[string]string{"istio": "ingressgateway"}, initialState.tls, nil) + createGateway(t, store, initialState.ns, initialState.name, initialState.hosts, map[string]string{"istio": "ingressgateway"}, initialState.tls, nil, nil) list := getRoutes(t, routerClient, watchedNamespace, 2, time.Second) - validateRoutes(t, initialState.hosts, list, initialState.name, initialState.tls) + validateRoutes(t, initialState.hosts, list, initialState.name, initialState.tls, nil, nil) close(iorStop) @@ -473,6 +492,7 @@ func createGateways(t *testing.T, store model.ConfigStoreController, begin, end []string{fmt.Sprintf("d%d.com", i)}, map[string]string{"istio": "ingressgateway"}, false, + nil, nil) } } @@ -541,7 +561,7 @@ func createService(t *testing.T, client kube.Client, ns string, labels map[strin } func createGateway(t *testing.T, store model.ConfigStoreController, ns string, name string, hosts []string, gwSelector map[string]string, - tls bool, annotations map[string]string, + tls bool, annotations map[string]string, labels map[string]string, ) { t.Helper() @@ -550,17 +570,13 @@ func createGateway(t *testing.T, store model.ConfigStoreController, ns string, n tlsConfig = &networking.ServerTLSSettings{HttpsRedirect: true} } - if annotations == nil { - annotations = map[string]string{"foo": "bar"} - } - _, err := store.Create(config.Config{ Meta: config.Meta{ GroupVersionKind: collections.Gateway.GroupVersionKind(), Namespace: ns, Name: name, Annotations: annotations, - Labels: map[string]string{"foo": "bar", prefixedLabel: "present"}, + Labels: labels, ResourceVersion: "1", }, Spec: &networking.Gateway{