Backport of [OSS] Improve Gateway Test Coverage of Catalog Health into release/1.16.x (#18014)

* backport of commit 954bd6ab1f1a2a00f549b10ad435cdead8d2cae2

* backport of commit 85c32d8f2e7e2c3a2855fe7a8fc4d10e3865b81f

* backport of commit 7ea3d622d75b4a69b8fc51d181b79c6b170ea47a

* backport of commit 127ae69c6dc967d575929e920813e7fe0d3fdef1

* backport of commit e04099b6cdd5dc20a36a19897816069669b2ef92

---------

Co-authored-by: DanStough <dan.stough@hashicorp.com>

.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{
+			Check:           check,
-				Name:                           fmt.Sprintf("%s listening", c.gatewayKind),
-				TCP:                            ipaddr.FormatAddressPort(tcpCheckAddr, lanAddr.Port),
-				Interval:                       "10s",
-				DeregisterCriticalServiceAfter: c.deregAfterCritical,
-			},
 		}
 
 		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"
 )
 
@@ -40,7 +39,21 @@ func CatalogServiceExists(t *testing.T, c *api.Client, svc string, opts *api.Que
 	})
 }
 
-// CatalogServiceExists verifies the node name exists in the Consul catalog
+// CatalogServiceHasInstanceCount verifies the service name exists in the Consul catalog and has the specified
+// number of instances.
+func CatalogServiceHasInstanceCount(t *testing.T, c *api.Client, svc string, count int, opts *api.QueryOptions) {
+	retry.Run(t, func(r *retry.R) {
+		services, _, err := c.Catalog().Service(svc, "", opts)
+		if err != nil {
+			r.Fatal("error reading service data")
+		}
+		if len(services) != count {
+			r.Fatalf("did not find %d catalog entries for %s", count, svc)
+		}
+	})
+}
+
+// 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)
@@ -53,26 +66,55 @@ func CatalogNodeExists(t *testing.T, c *api.Client, nodeName string) {
 	})
 }
 
-func HTTPServiceEchoes(t *testing.T, ip string, port int, path string) {
+// CatalogServiceIsHealthy verifies the service name exists and all instances pass healthchecks
-	doHTTPServiceEchoes(t, ip, port, path, nil)
+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, 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(
@@ -80,6 +122,7 @@ func doHTTPServiceEchoesWithClient(
 	client *http.Client,
 	addr string,
 	path string,
+	requestHeaders map[string]string,
 	expectedResHeader map[string]string,
 ) {
 	const phrase = "hello"
@@ -96,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 {
+	if config.ExposedPorts != nil {
-		cluster, err = libcluster.New(t, []libcluster.Config{*serverConf}, config.Ports...)
+		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,

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 <service>.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)
+}