From acb1af85a012b3600480f0120a9d772cd4ad304f Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Mon, 18 Nov 2024 17:59:52 -0500 Subject: [PATCH] Refactor `proxy list` command, ensuring api-gateway Pods are included (#4426) * Refactor `proxy list` command, ensuring api-gateway Pods are included Proxies are also now output in deterministic order based on their proxy type, namespace and name * Stop printing non-JSON info to terminal when `-o json` is requested * Update test assertions to expect sorted output * Add changelog entry --- .changelog/4426.txt | 3 + cli/cmd/proxy/list/command.go | 117 +++++++++++++++-------------- cli/cmd/proxy/list/command_test.go | 41 ++++++---- 3 files changed, 89 insertions(+), 72 deletions(-) create mode 100644 .changelog/4426.txt diff --git a/.changelog/4426.txt b/.changelog/4426.txt new file mode 100644 index 0000000000..8bad4b8e9d --- /dev/null +++ b/.changelog/4426.txt @@ -0,0 +1,3 @@ +```release-note:bug +cli: fix issue where the `consul-k8s proxy list` command does not include API gateways. +``` diff --git a/cli/cmd/proxy/list/command.go b/cli/cmd/proxy/list/command.go index 0204832c44..7d7fe6e4a7 100644 --- a/cli/cmd/proxy/list/command.go +++ b/cli/cmd/proxy/list/command.go @@ -7,14 +7,17 @@ import ( "encoding/json" "errors" "fmt" + "sort" "strings" "sync" "github.com/posener/complete" + "golang.org/x/exp/maps" 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/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" "github.com/hashicorp/consul-k8s/cli/common" @@ -206,66 +209,66 @@ func (c *ListCommand) namespace() string { } } -// fetchPods fetches all pods in flagNamespace which run Consul proxies. +// fetchPods fetches all pods in flagNamespace which run Consul proxies, +// making sure to return each pod only once even if multiple label selectors may +// return the same pod. The pods in the resulting list are grouped by proxy type +// and then sorted by namespace + name within each group. func (c *ListCommand) fetchPods() ([]v1.Pod, error) { - var pods []v1.Pod - - // 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 (api-gateway, ingress-gateway, mesh-gateway, terminating-gateway), chart=consul-helm", - }) - if err != nil { - return nil, err + var ( + apiGateways = make(map[types.NamespacedName]v1.Pod) + ingressGateways = make(map[types.NamespacedName]v1.Pod) + meshGateways = make(map[types.NamespacedName]v1.Pod) + terminatingGateways = make(map[types.NamespacedName]v1.Pod) + sidecars = make(map[types.NamespacedName]v1.Pod) + ) + + // Map target map for each proxy type. Note that some proxy types + // require multiple selectors and thus target the same map. + proxySelectors := []struct { + Target map[types.NamespacedName]v1.Pod + Selector string + }{ + {Target: apiGateways, Selector: "component=api-gateway, gateway.consul.hashicorp.com/managed=true"}, + {Target: apiGateways, Selector: "api-gateway.consul.hashicorp.com/managed=true"}, // Legacy API gateways + {Target: ingressGateways, Selector: "component=ingress-gateway, chart=consul-helm"}, + {Target: meshGateways, Selector: "component=mesh-gateway, chart=consul-helm"}, + {Target: terminatingGateways, Selector: "component=terminating-gateway, chart=consul-helm"}, + {Target: sidecars, Selector: "consul.hashicorp.com/connect-inject-status=injected"}, } - pods = append(pods, gatewaypods.Items...) - // 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", - }) + // Query all proxy types into their appropriate maps. + for _, selector := range proxySelectors { + pods, err := c.kubernetes.CoreV1().Pods(c.namespace()).List(c.Ctx, metav1.ListOptions{ + LabelSelector: selector.Selector, + }) + if err != nil { + return nil, err + } - namespacedName := func(pod v1.Pod) string { - return pod.Namespace + pod.Name - } - if err != nil { - return nil, err + for _, pod := range pods.Items { + name := types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name} + selector.Target[name] = pod + } } - 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) - } - } + // Collect all proxies into a single list of Pods, ordered by proxy type. + // Within each proxy type subgroup, order by namespace and then name for output readability. + var pods []v1.Pod + var podSources = []map[types.NamespacedName]v1.Pod{ + apiGateways, ingressGateways, meshGateways, terminatingGateways, sidecars, } - //--- + for _, podSource := range podSources { + names := maps.Keys(podSource) - // 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{ - LabelSelector: "consul.hashicorp.com/connect-inject-status=injected", - }) - if err != nil { - return nil, err + // Insert Pods ordered by their NamespacedName which amounts to "/". + sort.SliceStable(names, func(i, j int) bool { + return strings.Compare(names[i].String(), names[j].String()) < 0 + }) + + for _, name := range names { + pods = append(pods, podSource[name]) + } } - pods = append(pods, sidecarpods.Items...) return pods, nil } @@ -281,12 +284,6 @@ func (c *ListCommand) output(pods []v1.Pod) { return } - if c.flagAllNamespaces { - c.UI.Output("Namespace: all namespaces\n") - } else { - c.UI.Output("Namespace: %s\n", c.namespace()) - } - var tbl *terminal.Table if c.flagAllNamespaces { tbl = terminal.NewTable("Namespace", "Name", "Type") @@ -297,7 +294,7 @@ func (c *ListCommand) output(pods []v1.Pod) { for _, pod := range pods { var proxyType string - // Get the type for ingress, mesh, and terminating gateways. + // Get the type for api, ingress, mesh, and terminating gateways + sidecars. switch pod.Labels["component"] { case "api-gateway": proxyType = "API Gateway" @@ -333,6 +330,10 @@ func (c *ListCommand) output(pods []v1.Pod) { c.UI.Output(string(jsonSt)) } } else { + if !c.flagAllNamespaces { + c.UI.Output("Namespace: %s\n", c.namespace()) + } + c.UI.Table(tbl) } diff --git a/cli/cmd/proxy/list/command_test.go b/cli/cmd/proxy/list/command_test.go index 5493ab88a9..d0ad84fa0b 100644 --- a/cli/cmd/proxy/list/command_test.go +++ b/cli/cmd/proxy/list/command_test.go @@ -6,15 +6,13 @@ package list import ( "bytes" "context" + "encoding/json" "flag" "fmt" "io" "os" "testing" - "github.com/hashicorp/consul-k8s/cli/common" - cmnFlag "github.com/hashicorp/consul-k8s/cli/common/flag" - "github.com/hashicorp/consul-k8s/cli/common/terminal" "github.com/hashicorp/go-hclog" "github.com/posener/complete" "github.com/stretchr/testify/assert" @@ -22,6 +20,10 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" + + "github.com/hashicorp/consul-k8s/cli/common" + cmnFlag "github.com/hashicorp/consul-k8s/cli/common/flag" + "github.com/hashicorp/consul-k8s/cli/common/terminal" ) func TestFlagParsing(t *testing.T) { @@ -229,6 +231,9 @@ func TestFetchPods(t *testing.T) { } } +// TestListCommandOutput tests the output of the list command. The output must +// contain the expected regular expressions and not contain the not expected +// regular expressions. func TestListCommandOutput(t *testing.T) { // These regular expressions must be present in the output. expected := []string{ @@ -340,11 +345,10 @@ func TestListCommandOutput(t *testing.T) { } } +// TestListCommandOutputInJsonFormat tests the output of the list command when +// the output format is set to JSON. The proxies must be presented in the appropriate +// order and format. func TestListCommandOutputInJsonFormat(t *testing.T) { - // These regular expressions must be present in the output. - 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{ { ObjectMeta: metav1.ObjectMeta{ @@ -381,8 +385,8 @@ func TestListCommandOutputInJsonFormat(t *testing.T) { Name: "api-gateway", Namespace: "consul", Labels: map[string]string{ - "component": "api-gateway", - "chart": "consul-helm", + "component": "api-gateway", + "gateway.consul.hashicorp.com/managed": "true", }, }, }, @@ -432,12 +436,21 @@ func TestListCommandOutputInJsonFormat(t *testing.T) { out := c.Run([]string{"-A", "-o", "json"}) require.Equal(t, 0, out) - actual := buf.String() - - require.Regexp(t, expected, actual) - for _, expression := range notExpected { - require.NotRegexp(t, expression, actual) + var actual []struct { + Name string `json:"Name"` + Namespace string `json:"Namespace"` + Type string `json:"Type"` } + require.NoErrorf(t, json.Unmarshal(buf.Bytes(), &actual), "failed to parse json output: %s", buf.String()) + + require.Len(t, actual, 7) + assert.Equal(t, "api-gateway", actual[0].Name) + assert.Equal(t, "both-labels-api-gateway", actual[1].Name) + assert.Equal(t, "deprecated-api-gateway", actual[2].Name) + assert.Equal(t, "ingress-gateway", actual[3].Name) + assert.Equal(t, "mesh-gateway", actual[4].Name) + assert.Equal(t, "terminating-gateway", actual[5].Name) + assert.Equal(t, "pod1", actual[6].Name) } func TestNoPodsFound(t *testing.T) {