From 9bc197c5b3dbaa09498c82d5b6a68c80845e8513 Mon Sep 17 00:00:00 2001 From: sarahalsmiller <100602640+sarahalsmiller@users.noreply.github.com> Date: Tue, 16 Jan 2024 16:16:58 -0600 Subject: [PATCH] Net 6289 improve consul api gateway annotations (#3437) * Add api-gateway to allow list for gateway-kind annotation * Update godoc for AnnotationGatewayKind to list correct allowable values * Exclude api-gateway kind when checking if endpoints controller should act on gateway * Add gateway-kind and gateway-consul-service-name annotations to API gateway pods * Add appropriate component label to api-gateway pods * Update consul-k8s CLI to rely on standard component label for api-gateway This makes api-gateway behave the same as ingress, mesh and terminating gateways; however, it won't pick up pods created by the legacy consul-api-gateway controller. * add backwards compatabiilty, additional tests * add changelog * Update .changelog/3437.txt Co-authored-by: Nathan Coleman * Update cli/cmd/proxy/list/command.go Co-authored-by: Nathan Coleman * Update cli/cmd/proxy/list/command_test.go Co-authored-by: Nathan Coleman * Update cli/cmd/proxy/list/command_test.go Co-authored-by: Nathan Coleman * Update cli/cmd/proxy/list/command.go Co-authored-by: Nathan Coleman * cleanup * Update cli/cmd/proxy/list/command.go Co-authored-by: Nathan Coleman --------- Co-authored-by: Nathan Coleman --- .changelog/3437.txt | 3 + cli/cmd/proxy/list/command.go | 61 ++++++++++++++----- cli/cmd/proxy/list/command_test.go | 44 ++++++++++++- control-plane/api-gateway/common/labels.go | 2 + .../api-gateway/gatekeeper/deployment.go | 5 +- .../api-gateway/gatekeeper/gatekeeper_test.go | 8 ++- .../constants/annotations_and_labels.go | 3 +- .../endpoints/endpoints_controller.go | 12 ++-- 8 files changed, 115 insertions(+), 23 deletions(-) create mode 100644 .changelog/3437.txt diff --git a/.changelog/3437.txt b/.changelog/3437.txt new file mode 100644 index 0000000000..3e70f0d4c0 --- /dev/null +++ b/.changelog/3437.txt @@ -0,0 +1,3 @@ +```release-note:bug-fix +api-gateways: API Gateway pods now include `gateway-kind` and `gateway-consul-service-name` annotations consistent with other Consul gateway types. +``` \ No newline at end of file diff --git a/cli/cmd/proxy/list/command.go b/cli/cmd/proxy/list/command.go index 49881417a3..e0211ebe37 100644 --- a/cli/cmd/proxy/list/command.go +++ b/cli/cmd/proxy/list/command.go @@ -10,15 +10,16 @@ import ( "strings" "sync" - "github.com/hashicorp/consul-k8s/cli/common" - "github.com/hashicorp/consul-k8s/cli/common/flag" - "github.com/hashicorp/consul-k8s/cli/common/terminal" "github.com/posener/complete" helmCLI "helm.sh/helm/v3/pkg/cli" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/validation" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" + + "github.com/hashicorp/consul-k8s/cli/common" + "github.com/hashicorp/consul-k8s/cli/common/flag" + "github.com/hashicorp/consul-k8s/cli/common/terminal" ) const ( @@ -211,21 +212,51 @@ func (c *ListCommand) fetchPods() ([]v1.Pod, error) { // Fetch all pods in the namespace with labels matching the gateway component names. gatewaypods, err := c.kubernetes.CoreV1().Pods(c.namespace()).List(c.Ctx, metav1.ListOptions{ - LabelSelector: "component in (ingress-gateway, mesh-gateway, terminating-gateway), chart=consul-helm", + LabelSelector: "component in (api-gateway, ingress-gateway, mesh-gateway, terminating-gateway), chart=consul-helm", }) if err != nil { return nil, err } pods = append(pods, gatewaypods.Items...) - // Fetch all pods in the namespace with a label indicating they are an API gateway. + // Fetch API Gateway pods with deprecated label and append if they aren't already in the list + // TODO this block can be deleted if and when we decide we are ok with no longer listing pods of people using previous API Gateway + // versions. apigatewaypods, err := c.kubernetes.CoreV1().Pods(c.namespace()).List(c.Ctx, metav1.ListOptions{ LabelSelector: "api-gateway.consul.hashicorp.com/managed=true", }) + + namespacedName := func(pod v1.Pod) string { + return pod.Namespace + pod.Name + } if err != nil { return nil, err } - pods = append(pods, apigatewaypods.Items...) + if len(apigatewaypods.Items) > 0 { + //Deduplicated pod list + seenPods := map[string]struct{}{} + for _, pod := range apigatewaypods.Items { + if _, ok := seenPods[namespacedName(pod)]; ok { + continue + } + found := false + for _, gatewayPod := range gatewaypods.Items { + //note that we already have this pod in the list so we can exit early. + seenPods[namespacedName(gatewayPod)] = struct{}{} + + if (namespacedName(gatewayPod)) == namespacedName(pod) { + found = true + break + } + } + //pod isn't in the list already, we can add it. + if !found { + pods = append(pods, pod) + } + + } + } + //--- // Fetch all pods in the namespace with a label indicating they are a service networked by Consul. sidecarpods, err := c.kubernetes.CoreV1().Pods(c.namespace()).List(c.Ctx, metav1.ListOptions{ @@ -268,22 +299,22 @@ func (c *ListCommand) output(pods []v1.Pod) { // Get the type for ingress, mesh, and terminating gateways. switch pod.Labels["component"] { + case "api-gateway": + proxyType = "API Gateway" case "ingress-gateway": proxyType = "Ingress Gateway" case "mesh-gateway": proxyType = "Mesh Gateway" case "terminating-gateway": proxyType = "Terminating Gateway" - } - - // Determine if the pod is an API Gateway. - if pod.Labels["api-gateway.consul.hashicorp.com/managed"] == "true" { - proxyType = "API Gateway" - } - - // Fallback to "Sidecar" as a default - if proxyType == "" { + default: + // Fallback to "Sidecar" as a default proxyType = "Sidecar" + + // Determine if deprecated API Gateway pod. + if pod.Labels["api-gateway.consul.hashicorp.com/managed"] == "true" { + proxyType = "API Gateway" + } } if c.flagAllNamespaces { diff --git a/cli/cmd/proxy/list/command_test.go b/cli/cmd/proxy/list/command_test.go index 9e7104886d..5493ab88a9 100644 --- a/cli/cmd/proxy/list/command_test.go +++ b/cli/cmd/proxy/list/command_test.go @@ -274,12 +274,33 @@ func TestListCommandOutput(t *testing.T) { }, }, }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "depricated-api-gateway", + Namespace: "consul", + Labels: map[string]string{ + "api-gateway.consul.hashicorp.com/managed": "true", + }, + }, + }, { ObjectMeta: metav1.ObjectMeta{ Name: "api-gateway", Namespace: "consul", + Labels: map[string]string{ + "component": "api-gateway", + "chart": "consul-helm", + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "both-labels-api-gateway", + Namespace: "consul", Labels: map[string]string{ "api-gateway.consul.hashicorp.com/managed": "true", + "component": "api-gateway", + "chart": "consul-helm", }, }, }, @@ -321,7 +342,7 @@ func TestListCommandOutput(t *testing.T) { func TestListCommandOutputInJsonFormat(t *testing.T) { // These regular expressions must be present in the output. - expected := ".*Name.*mesh-gateway.*\n.*Namespace.*consul.*\n.*Type.*Mesh Gateway.*\n.*\n.*\n.*Name.*terminating-gateway.*\n.*Namespace.*consul.*\n.*Type.*Terminating Gateway.*\n.*\n.*\n.*Name.*ingress-gateway.*\n.*Namespace.*default.*\n.*Type.*Ingress Gateway.*\n.*\n.*\n.*Name.*api-gateway.*\n.*Namespace.*consul.*\n.*Type.*API Gateway.*\n.*\n.*\n.*Name.*pod1.*\n.*Namespace.*default.*\n.*Type.*Sidecar.*" + expected := ".*Name.*api-gateway.*\n.*Namespace.*consul.*\n.*Type.*API Gateway.*\n.*\n.*\n.*Name.*both-labels-api-gateway.*\n.*Namespace.*consul.*\n.*Type.*API Gateway.*\n.*\n.*\n.*Name.*mesh-gateway.*\n.*Namespace.*consul.*\n.*Type.*Mesh Gateway.*\n.*\n.*\n.*Name.*terminating-gateway.*\n.*Namespace.*consul.*\n.*Type.*Terminating Gateway.*\n.*\n.*\n.*Name.*ingress-gateway.*\n.*Namespace.*default.*\n.*Type.*Ingress Gateway.*\n.*\n.*\n.*Name.*deprecated-api-gateway.*\n.*Namespace.*consul.*\n.*Type.*API Gateway.*\n.*\n.*\n.*Name.*pod1.*\n.*Namespace.*default.*\n.*Type.*Sidecar.*" notExpected := "default.*dont-fetch.*Sidecar" pods := []v1.Pod{ @@ -359,6 +380,27 @@ func TestListCommandOutputInJsonFormat(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "api-gateway", Namespace: "consul", + Labels: map[string]string{ + "component": "api-gateway", + "chart": "consul-helm", + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "both-labels-api-gateway", + Namespace: "consul", + Labels: map[string]string{ + "api-gateway.consul.hashicorp.com/managed": "true", + "component": "api-gateway", + "chart": "consul-helm", + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "deprecated-api-gateway", + Namespace: "consul", Labels: map[string]string{ "api-gateway.consul.hashicorp.com/managed": "true", }, diff --git a/control-plane/api-gateway/common/labels.go b/control-plane/api-gateway/common/labels.go index cba13a603e..06f7857c30 100644 --- a/control-plane/api-gateway/common/labels.go +++ b/control-plane/api-gateway/common/labels.go @@ -12,6 +12,7 @@ import ( ) const ( + componentLabel = "component" nameLabel = "gateway.consul.hashicorp.com/name" namespaceLabel = "gateway.consul.hashicorp.com/namespace" createdAtLabel = "gateway.consul.hashicorp.com/created" @@ -21,6 +22,7 @@ const ( // LabelsForGateway formats the default labels that appear on objects managed by the controllers. func LabelsForGateway(gateway *gwv1beta1.Gateway) map[string]string { return map[string]string{ + componentLabel: "api-gateway", nameLabel: gateway.Name, namespaceLabel: gateway.Namespace, createdAtLabel: fmt.Sprintf("%d", gateway.CreationTimestamp.Unix()), diff --git a/control-plane/api-gateway/gatekeeper/deployment.go b/control-plane/api-gateway/gatekeeper/deployment.go index 38207dc819..d85fbf8ef1 100644 --- a/control-plane/api-gateway/gatekeeper/deployment.go +++ b/control-plane/api-gateway/gatekeeper/deployment.go @@ -19,6 +19,7 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/api-gateway/common" "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" + "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" ) const ( @@ -111,7 +112,9 @@ func (g *Gatekeeper) deployment(gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayC ObjectMeta: metav1.ObjectMeta{ Labels: common.LabelsForGateway(&gateway), Annotations: map[string]string{ - "consul.hashicorp.com/connect-inject": "false", + constants.AnnotationInject: "false", + constants.AnnotationGatewayConsulServiceName: gateway.Name, + constants.AnnotationGatewayKind: "api-gateway", }, }, Spec: corev1.PodSpec{ diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index 79a3050e04..8e212344b5 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -26,6 +26,7 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/api-gateway/common" "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" + "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" ) var ( @@ -35,6 +36,7 @@ var ( name = "test" namespace = "default" labels = map[string]string{ + "component": "api-gateway", "gateway.consul.hashicorp.com/name": name, "gateway.consul.hashicorp.com/namespace": namespace, createdAtLabelKey: createdAtLabelValue, @@ -1018,6 +1020,8 @@ func validateResourcesExist(t *testing.T, client client.Client, helmConfig commo require.NotNil(t, actual.Spec.Replicas) require.EqualValues(t, *expected.Spec.Replicas, *actual.Spec.Replicas) } + require.Equal(t, expected.Spec.Template.ObjectMeta.Annotations, actual.Spec.Template.ObjectMeta.Annotations) + require.Equal(t, expected.Spec.Template.ObjectMeta.Labels, actual.Spec.Template.Labels) // Ensure there is an init container hasInitContainer := false @@ -1215,7 +1219,9 @@ func configureDeployment(name, namespace string, labels map[string]string, repli ObjectMeta: metav1.ObjectMeta{ Labels: labels, Annotations: map[string]string{ - "consul.hashicorp.com/connect-inject": "false", + constants.AnnotationInject: "false", + constants.AnnotationGatewayConsulServiceName: name, + constants.AnnotationGatewayKind: "api-gateway", }, }, Spec: corev1.PodSpec{ diff --git a/control-plane/connect-inject/constants/annotations_and_labels.go b/control-plane/connect-inject/constants/annotations_and_labels.go index f3824bd071..82d2e01ce0 100644 --- a/control-plane/connect-inject/constants/annotations_and_labels.go +++ b/control-plane/connect-inject/constants/annotations_and_labels.go @@ -25,7 +25,8 @@ const ( // AnnotationGatewayKind is the key of the annotation that indicates pods // that represent Consul Connect Gateways. This should be set to a - // value that is either "mesh", "ingress" or "terminating". + // value that is either "mesh-gateway", "ingress-gateway", "terminating-gateway", + // or "api-gateway". AnnotationGatewayKind = "consul.hashicorp.com/gateway-kind" // AnnotationGatewayConsulServiceName is the key of the annotation whose value diff --git a/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go b/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go index 269f42f0a2..886b8e5e8b 100644 --- a/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go +++ b/control-plane/connect-inject/controllers/endpoints/endpoints_controller.go @@ -43,6 +43,7 @@ const ( meshGateway = "mesh-gateway" terminatingGateway = "terminating-gateway" ingressGateway = "ingress-gateway" + apiGateway = "api-gateway" kubernetesSuccessReasonMsg = "Kubernetes health checks passing" envoyPrometheusBindAddr = "envoy_prometheus_bind_addr" @@ -786,9 +787,11 @@ func (r *Controller) createGatewayRegistrations(pod corev1.Pod, serviceEndpoints "address": "0.0.0.0", }, } - + case apiGateway: + // Do nothing. This is only here so that API gateway pods have annotations + // consistent with other gateway types but don't return an error below. default: - return nil, fmt.Errorf("%s must be one of %s, %s, or %s", constants.AnnotationGatewayKind, meshGateway, terminatingGateway, ingressGateway) + return nil, fmt.Errorf("%s must be one of %s, %s, %s, or %s", constants.AnnotationGatewayKind, meshGateway, terminatingGateway, ingressGateway, apiGateway) } if r.MetricsConfig.DefaultEnableMetrics && r.MetricsConfig.EnableGatewayMetrics { @@ -1448,10 +1451,11 @@ func hasBeenInjected(pod corev1.Pod) bool { return false } -// isGateway checks the value of the gateway annotation and returns true if the Pod represents a Gateway. +// isGateway checks the value of the gateway annotation and returns true if the Pod +// represents a Gateway kind that should be acted upon by the endpoints controller. func isGateway(pod corev1.Pod) bool { anno, ok := pod.Annotations[constants.AnnotationGatewayKind] - return ok && anno != "" + return ok && anno != "" && anno != apiGateway } // isTelemetryCollector checks whether a pod is part of a deployment for a Consul Telemetry Collector. If so,