From b94095d92e3257a8b22a4a6610fd8c4c29d437f7 Mon Sep 17 00:00:00 2001 From: Dan Stough Date: Wed, 5 Jul 2023 11:30:48 -0400 Subject: [PATCH] [OSS] Improve Gateway Test Coverage of Catalog Health (#18011) * fix(cli): remove failing check from 'connect envoy' registration for api gateway * test(integration): add tests to check catalog statsus of gateways on startup * remove extra sleep comment * Update test/integration/consul-container/libs/assert/service.go * changelog --- .changelog/18011.txt | 4 + command/connect/envoy/envoy.go | 19 ++- .../consul-container/libs/assert/service.go | 61 +++++++-- .../libs/topology/peering_topology.go | 10 +- .../test/basic/connect_service_test.go | 32 +---- .../test/gateways/gateway_endpoint_test.go | 30 ++-- .../test/gateways/http_route_test.go | 6 +- .../test/gateways/ingress_gateway_test.go | 129 ++++++++++++++++++ 8 files changed, 226 insertions(+), 65 deletions(-) create mode 100644 .changelog/18011.txt create mode 100644 test/integration/consul-container/test/gateways/ingress_gateway_test.go diff --git a/.changelog/18011.txt b/.changelog/18011.txt new file mode 100644 index 000000000000..d6c989f00e9f --- /dev/null +++ b/.changelog/18011.txt @@ -0,0 +1,4 @@ +```release-note:bug +connect: Removes the default health check from the `consul connect envoy` command when starting an API Gateway. +This health check would always fail. +``` diff --git a/command/connect/envoy/envoy.go b/command/connect/envoy/envoy.go index 4c1ff6b80a19..a6212ae4ca42 100644 --- a/command/connect/envoy/envoy.go +++ b/command/connect/envoy/envoy.go @@ -440,6 +440,18 @@ func (c *cmd) run(args []string) int { meta = map[string]string{structs.MetaWANFederationKey: "1"} } + // API gateways do not have a default listener or ready endpoint, + // so adding any check to the registration will fail + var check *api.AgentServiceCheck + if c.gatewayKind != api.ServiceKindAPIGateway { + check = &api.AgentServiceCheck{ + Name: fmt.Sprintf("%s listening", c.gatewayKind), + TCP: ipaddr.FormatAddressPort(tcpCheckAddr, lanAddr.Port), + Interval: "10s", + DeregisterCriticalServiceAfter: c.deregAfterCritical, + } + } + svc := api.AgentServiceRegistration{ Kind: c.gatewayKind, Name: c.gatewaySvcName, @@ -449,12 +461,7 @@ func (c *cmd) run(args []string) int { Meta: meta, TaggedAddresses: taggedAddrs, Proxy: proxyConf, - Check: &api.AgentServiceCheck{ - Name: fmt.Sprintf("%s listening", c.gatewayKind), - TCP: ipaddr.FormatAddressPort(tcpCheckAddr, lanAddr.Port), - Interval: "10s", - DeregisterCriticalServiceAfter: c.deregAfterCritical, - }, + Check: check, } if err := c.client.Agent().ServiceRegister(&svc); err != nil { diff --git a/test/integration/consul-container/libs/assert/service.go b/test/integration/consul-container/libs/assert/service.go index f8ff1d0ba9bb..370fbc857db8 100644 --- a/test/integration/consul-container/libs/assert/service.go +++ b/test/integration/consul-container/libs/assert/service.go @@ -13,12 +13,11 @@ import ( "time" "github.com/hashicorp/go-cleanhttp" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/sdk/testutil/retry" - "github.com/stretchr/testify/assert" - libservice "github.com/hashicorp/consul/test/integration/consul-container/libs/service" ) @@ -54,7 +53,7 @@ func CatalogServiceHasInstanceCount(t *testing.T, c *api.Client, svc string, cou }) } -// CatalogServiceExists verifies the node name exists in the Consul catalog +// CatalogNodeExists verifies the node name exists in the Consul catalog func CatalogNodeExists(t *testing.T, c *api.Client, nodeName string) { retry.Run(t, func(r *retry.R) { node, _, err := c.Catalog().Node(nodeName, nil) @@ -67,26 +66,55 @@ func CatalogNodeExists(t *testing.T, c *api.Client, nodeName string) { }) } +// CatalogServiceIsHealthy verifies the service name exists and all instances pass healthchecks +func CatalogServiceIsHealthy(t *testing.T, c *api.Client, svc string, opts *api.QueryOptions) { + CatalogServiceExists(t, c, svc, opts) + + retry.Run(t, func(r *retry.R) { + services, _, err := c.Health().Service(svc, "", false, opts) + if err != nil { + r.Fatal("error reading service health data") + } + if len(services) == 0 { + r.Fatal("did not find catalog entry for ", svc) + } + + for _, svc := range services { + for _, check := range svc.Checks { + if check.Status != api.HealthPassing { + r.Fatal("at least one check is not PASSING for service", svc.Service.Service) + } + } + } + + }) +} + func HTTPServiceEchoes(t *testing.T, ip string, port int, path string) { - doHTTPServiceEchoes(t, ip, port, path, nil) + doHTTPServiceEchoes(t, ip, port, path, nil, nil) } + +func HTTPServiceEchoesWithHeaders(t *testing.T, ip string, port int, path string, headers map[string]string) { + doHTTPServiceEchoes(t, ip, port, path, headers, nil) +} + func HTTPServiceEchoesWithClient(t *testing.T, client *http.Client, addr string, path string) { - doHTTPServiceEchoesWithClient(t, client, addr, path, nil) + doHTTPServiceEchoesWithClient(t, client, addr, path, nil, nil) } func HTTPServiceEchoesResHeader(t *testing.T, ip string, port int, path string, expectedResHeader map[string]string) { - doHTTPServiceEchoes(t, ip, port, path, expectedResHeader) + doHTTPServiceEchoes(t, ip, port, path, nil, expectedResHeader) } func HTTPServiceEchoesResHeaderWithClient(t *testing.T, client *http.Client, addr string, path string, expectedResHeader map[string]string) { - doHTTPServiceEchoesWithClient(t, client, addr, path, expectedResHeader) + doHTTPServiceEchoesWithClient(t, client, addr, path, nil, expectedResHeader) } // HTTPServiceEchoes verifies that a post to the given ip/port combination returns the data // in the response body. Optional path can be provided to differentiate requests. -func doHTTPServiceEchoes(t *testing.T, ip string, port int, path string, expectedResHeader map[string]string) { +func doHTTPServiceEchoes(t *testing.T, ip string, port int, path string, requestHeaders map[string]string, expectedResHeader map[string]string) { client := cleanhttp.DefaultClient() addr := fmt.Sprintf("%s:%d", ip, port) - doHTTPServiceEchoesWithClient(t, client, addr, path, expectedResHeader) + doHTTPServiceEchoesWithClient(t, client, addr, path, requestHeaders, expectedResHeader) } func doHTTPServiceEchoesWithClient( @@ -94,6 +122,7 @@ func doHTTPServiceEchoesWithClient( client *http.Client, addr string, path string, + requestHeaders map[string]string, expectedResHeader map[string]string, ) { const phrase = "hello" @@ -110,8 +139,20 @@ func doHTTPServiceEchoesWithClient( retry.RunWith(failer(), t, func(r *retry.R) { t.Logf("making call to %s", url) + reader := strings.NewReader(phrase) - res, err := client.Post(url, "text/plain", reader) + req, err := http.NewRequest("POST", url, reader) + require.NoError(t, err, "could not construct request") + + for k, v := range requestHeaders { + req.Header.Add(k, v) + + if k == "Host" { + req.Host = v + } + } + + res, err := client.Do(req) if err != nil { r.Fatal("could not make call to service ", url) } diff --git a/test/integration/consul-container/libs/topology/peering_topology.go b/test/integration/consul-container/libs/topology/peering_topology.go index c684764d4851..f51b041a8bcd 100644 --- a/test/integration/consul-container/libs/topology/peering_topology.go +++ b/test/integration/consul-container/libs/topology/peering_topology.go @@ -187,7 +187,11 @@ type ClusterConfig struct { BuildOpts *libcluster.BuildOptions Cmd string LogConsumer *TestLogConsumer - Ports []int + + // Exposed Ports are available on the cluster's pause container for the purposes + // of adding external communication to the cluster. An example would be a listener + // on a gateway. + ExposedPorts []int } // NewCluster creates a cluster with peering enabled. It also creates @@ -234,8 +238,8 @@ func NewCluster( serverConf.Cmd = append(serverConf.Cmd, config.Cmd) } - if config.Ports != nil { - cluster, err = libcluster.New(t, []libcluster.Config{*serverConf}, config.Ports...) + if config.ExposedPorts != nil { + cluster, err = libcluster.New(t, []libcluster.Config{*serverConf}, config.ExposedPorts...) } else { cluster, err = libcluster.NewN(t, *serverConf, config.NumServers) } diff --git a/test/integration/consul-container/test/basic/connect_service_test.go b/test/integration/consul-container/test/basic/connect_service_test.go index 3875ad3528a9..650db9fef079 100644 --- a/test/integration/consul-container/test/basic/connect_service_test.go +++ b/test/integration/consul-container/test/basic/connect_service_test.go @@ -7,11 +7,8 @@ import ( "fmt" "testing" - "github.com/stretchr/testify/require" - libassert "github.com/hashicorp/consul/test/integration/consul-container/libs/assert" libcluster "github.com/hashicorp/consul/test/integration/consul-container/libs/cluster" - libservice "github.com/hashicorp/consul/test/integration/consul-container/libs/service" "github.com/hashicorp/consul/test/integration/consul-container/libs/topology" ) @@ -40,7 +37,7 @@ func TestBasicConnectService(t *testing.T) { }, }) - clientService := createServices(t, cluster) + _, clientService := topology.CreateServices(t, cluster) _, port := clientService.GetAddr() _, adminPort := clientService.GetAdminAddr() @@ -51,30 +48,3 @@ func TestBasicConnectService(t *testing.T) { libassert.HTTPServiceEchoes(t, "localhost", port, "") libassert.AssertFortioName(t, fmt.Sprintf("http://localhost:%d", port), "static-server", "") } - -func createServices(t *testing.T, cluster *libcluster.Cluster) libservice.Service { - node := cluster.Agents[0] - client := node.GetClient() - // Create a service and proxy instance - serviceOpts := &libservice.ServiceOpts{ - Name: libservice.StaticServerServiceName, - ID: "static-server", - HTTPPort: 8080, - GRPCPort: 8079, - } - - // Create a service and proxy instance - _, _, err := libservice.CreateAndRegisterStaticServerAndSidecar(node, serviceOpts) - require.NoError(t, err) - - libassert.CatalogServiceExists(t, client, "static-server-sidecar-proxy", nil) - libassert.CatalogServiceExists(t, client, libservice.StaticServerServiceName, nil) - - // Create a client proxy instance with the server as an upstream - clientConnectProxy, err := libservice.CreateAndRegisterStaticClientSidecar(node, "", false, false) - require.NoError(t, err) - - libassert.CatalogServiceExists(t, client, "static-client-sidecar-proxy", nil) - - return clientConnectProxy -} diff --git a/test/integration/consul-container/test/gateways/gateway_endpoint_test.go b/test/integration/consul-container/test/gateways/gateway_endpoint_test.go index 1f7a00e6830e..659fc4e3fdfe 100644 --- a/test/integration/consul-container/test/gateways/gateway_endpoint_test.go +++ b/test/integration/consul-container/test/gateways/gateway_endpoint_test.go @@ -12,11 +12,10 @@ import ( "testing" "time" + "github.com/hashicorp/go-cleanhttp" "github.com/stretchr/testify/require" "github.com/hashicorp/consul/api" - "github.com/hashicorp/go-cleanhttp" - libassert "github.com/hashicorp/consul/test/integration/consul-container/libs/assert" libcluster "github.com/hashicorp/consul/test/integration/consul-container/libs/cluster" libservice "github.com/hashicorp/consul/test/integration/consul-container/libs/service" @@ -47,7 +46,7 @@ func TestAPIGatewayCreate(t *testing.T) { InjectGossipEncryption: true, AllowHTTPAnyway: true, }, - Ports: []int{ + ExposedPorts: []int{ listenerPortOne, serviceHTTPPort, serviceGRPCPort, @@ -59,6 +58,21 @@ func TestAPIGatewayCreate(t *testing.T) { namespace := getOrCreateNamespace(t, client) + // Create a gateway + // We intentionally do this before creating the config entries + gatewayService, err := libservice.NewGatewayService(context.Background(), libservice.GatewayConfig{ + Kind: "api", + Namespace: namespace, + Name: gatewayName, + }, cluster.Agents[0], listenerPortOne) + require.NoError(t, err) + + // We check this is healthy here because in the case of bringing up a new kube cluster, + // it is not possible to create the config entry in advance. + // The health checks must pass so the pod can start up. + // For API gateways, this should always pass, because there is no default listener for health in Envoy + libassert.CatalogServiceIsHealthy(t, client, gatewayName, &api.QueryOptions{Namespace: namespace}) + // add api gateway config apiGateway := &api.APIGatewayConfigEntry{ Kind: api.APIGateway, @@ -75,7 +89,7 @@ func TestAPIGatewayCreate(t *testing.T) { require.NoError(t, cluster.ConfigEntryWrite(apiGateway)) - _, _, err := libservice.CreateAndRegisterStaticServerAndSidecar(cluster.Agents[0], &libservice.ServiceOpts{ + _, _, err = libservice.CreateAndRegisterStaticServerAndSidecar(cluster.Agents[0], &libservice.ServiceOpts{ ID: serviceName, Name: serviceName, Namespace: namespace, @@ -105,14 +119,6 @@ func TestAPIGatewayCreate(t *testing.T) { require.NoError(t, cluster.ConfigEntryWrite(tcpRoute)) - // Create a gateway - gatewayService, err := libservice.NewGatewayService(context.Background(), libservice.GatewayConfig{ - Kind: "api", - Namespace: namespace, - Name: gatewayName, - }, cluster.Agents[0], listenerPortOne) - require.NoError(t, err) - // make sure the gateway/route come online // make sure config entries have been properly created checkGatewayConfigEntry(t, client, gatewayName, &api.QueryOptions{Namespace: namespace}) diff --git a/test/integration/consul-container/test/gateways/http_route_test.go b/test/integration/consul-container/test/gateways/http_route_test.go index 0a33ef2fae8b..fcdbb7ce1c96 100644 --- a/test/integration/consul-container/test/gateways/http_route_test.go +++ b/test/integration/consul-container/test/gateways/http_route_test.go @@ -70,7 +70,7 @@ func TestHTTPRouteFlattening(t *testing.T) { InjectGossipEncryption: true, AllowHTTPAnyway: true, }, - Ports: []int{ + ExposedPorts: []int{ listenerPort, serviceOneHTTPPort, serviceOneGRPCPort, @@ -298,7 +298,7 @@ func TestHTTPRoutePathRewrite(t *testing.T) { InjectGossipEncryption: true, AllowHTTPAnyway: true, }, - Ports: []int{ + ExposedPorts: []int{ listenerPort, fooHTTPPort, fooGRPCPort, @@ -525,7 +525,7 @@ func TestHTTPRouteParentRefChange(t *testing.T) { InjectGossipEncryption: true, AllowHTTPAnyway: true, }, - Ports: []int{ + ExposedPorts: []int{ listenerOnePort, listenerTwoPort, serviceHTTPPort, diff --git a/test/integration/consul-container/test/gateways/ingress_gateway_test.go b/test/integration/consul-container/test/gateways/ingress_gateway_test.go new file mode 100644 index 000000000000..1a8958741c8a --- /dev/null +++ b/test/integration/consul-container/test/gateways/ingress_gateway_test.go @@ -0,0 +1,129 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package gateways + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/docker/go-connections/nat" + "github.com/stretchr/testify/require" + + "github.com/hashicorp/consul/api" + libassert "github.com/hashicorp/consul/test/integration/consul-container/libs/assert" + libcluster "github.com/hashicorp/consul/test/integration/consul-container/libs/cluster" + libservice "github.com/hashicorp/consul/test/integration/consul-container/libs/service" + "github.com/hashicorp/consul/test/integration/consul-container/libs/topology" +) + +// TestIngressGateway Summary +// This test makes sure a cluster service can be reached via and ingress gateway. +// +// Steps: +// - Create a cluster (1 server and 1 client). +// - Create the example static-server and sidecar containers, then register them both with Consul +// - Create an ingress gateway and register it with Consul on the client agent +// - Create a config entry that binds static-server to a new listener on the ingress gateway +// - Verify that static-service is accessible through the ingress gateway port +func TestIngressGateway(t *testing.T) { + t.Parallel() + + // Ingress gateways must have a listener other than 8443, which is used for health checks. + // 9999 is already exposed from consul agents + gatewayListenerPort := 9999 + + cluster, _, _ := topology.NewCluster(t, &topology.ClusterConfig{ + NumServers: 1, + NumClients: 1, + ApplyDefaultProxySettings: true, + BuildOpts: &libcluster.BuildOptions{ + Datacenter: "dc1", + InjectAutoEncryption: true, + InjectGossipEncryption: true, + // TODO(rb): fix the test to not need the service/envoy stack to use :8500 + AllowHTTPAnyway: true, + }, + }) + apiClient := cluster.APIClient(0) + clientNode := cluster.Clients()[0] + + // Set up the "static-server" backend + serverService, _ := topology.CreateServices(t, cluster) + + // Create the ingress gateway service + // We expose this on the client node, which already has port 9999 exposed as part of it's pause "pod" + gwCfg := libservice.GatewayConfig{ + Name: api.IngressGateway, + Kind: "ingress", + } + ingressService, err := libservice.NewGatewayService(context.Background(), gwCfg, clientNode) + require.NoError(t, err) + + // this is deliberate + // internally, ingress gw have a 15s timeout before the /ready endpoint is available, + // then we need to wait for the health check to re-execute and propagate. + time.Sleep(45 * time.Second) + + // We check this is healthy here because in the case of bringing up a new kube cluster, + // it is not possible to create the config entry in advance. + // The health checks must pass so the pod can start up. + libassert.CatalogServiceIsHealthy(t, apiClient, api.IngressGateway, nil) + + // Register a service to the ingress gateway + // **NOTE**: We intentionally wait until after the gateway starts to create the config entry. + // This was a regression that can cause errors when starting up consul-k8s before you have the resource defined. + ingressGwConfig := &api.IngressGatewayConfigEntry{ + Kind: api.IngressGateway, + Name: api.IngressGateway, + Listeners: []api.IngressListener{ + { + Port: gatewayListenerPort, + Protocol: "http", + Services: []api.IngressService{ + { + Name: libservice.StaticServerServiceName, + }, + }, + }, + }, + } + + require.NoError(t, cluster.ConfigEntryWrite(ingressGwConfig)) + + // Wait for the request to persist + checkIngressConfigEntry(t, apiClient, api.IngressGateway, nil) + + _, adminPort := ingressService.GetAdminAddr() + libassert.AssertUpstreamEndpointStatus(t, adminPort, "static-server.default", "HEALTHY", 1) + //libassert.GetEnvoyListenerTCPFilters(t, adminPort) // This won't succeed because the dynamic listener is delayed + + libassert.AssertContainerState(t, ingressService, "running") + libassert.AssertContainerState(t, serverService, "running") + + mappedPort, err := clientNode.GetPod().MappedPort(context.Background(), nat.Port(fmt.Sprintf("%d/tcp", gatewayListenerPort))) + require.NoError(t, err) + + // by default, ingress routes are set per .ingress.* + headers := map[string]string{"Host": fmt.Sprintf("%s.ingress.com", libservice.StaticServerServiceName)} + libassert.HTTPServiceEchoesWithHeaders(t, "localhost", mappedPort.Int(), "", headers) +} + +func checkIngressConfigEntry(t *testing.T, client *api.Client, gatewayName string, opts *api.QueryOptions) { + t.Helper() + + require.Eventually(t, func() bool { + entry, _, err := client.ConfigEntries().Get(api.IngressGateway, gatewayName, opts) + if err != nil { + t.Log("error constructing request", err) + return false + } + if entry == nil { + t.Log("returned entry is nil") + return false + } + return true + }, time.Second*10, time.Second*1) +}