[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

@@ -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.
+```

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 {

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,33 +66,63 @@ 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(
 	t *testing.T,
 	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)
 		}

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)
 	}

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
-}

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})

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,