Skip to content

Commit

Permalink
Refactor proxy list command, ensuring api-gateway Pods are included (…
Browse files Browse the repository at this point in the history 
…#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
  • Loading branch information
@nathancoleman
nathancoleman authored Nov 18, 2024
1 parent 20fb198 commit acb1af8
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 72 deletions.
3 changes: 3 additions & 0 deletions .changelog/4426.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
cli: fix issue where the `consul-k8s proxy list` command does not include API gateways.
```
117 changes: 59 additions & 58 deletions cli/cmd/proxy/list/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 "<namespace>/<name>".
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
}
Expand All @@ -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")
Expand All @@ -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"
Expand Down Expand Up @@ -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)
}

Expand Down
41 changes: 27 additions & 14 deletions cli/cmd/proxy/list/command_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,24 @@ 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"
"github.com/stretchr/testify/require"
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) {
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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",
},
},
},
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit acb1af8

Please sign in to comment.