Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OSS] Improve Gateway Test Coverage of Catalog Health #18011

Merged
merged 5 commits into from
Jul 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .changelog/18011.txt
Original file line number Diff line number Diff line change
@@ -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.
```
19 changes: 13 additions & 6 deletions command/connect/envoy/envoy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 {
Expand Down
61 changes: 51 additions & 10 deletions test/integration/consul-container/libs/assert/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
Expand All @@ -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"
Expand All @@ -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)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -40,7 +37,7 @@ func TestBasicConnectService(t *testing.T) {
},
})

clientService := createServices(t, cluster)
_, clientService := topology.CreateServices(t, cluster)
_, port := clientService.GetAddr()
_, adminPort := clientService.GetAdminAddr()

Expand All @@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -47,7 +46,7 @@ func TestAPIGatewayCreate(t *testing.T) {
InjectGossipEncryption: true,
AllowHTTPAnyway: true,
},
Ports: []int{
ExposedPorts: []int{
listenerPortOne,
serviceHTTPPort,
serviceGRPCPort,
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func TestHTTPRouteFlattening(t *testing.T) {
InjectGossipEncryption: true,
AllowHTTPAnyway: true,
},
Ports: []int{
ExposedPorts: []int{
listenerPort,
serviceOneHTTPPort,
serviceOneGRPCPort,
Expand Down Expand Up @@ -298,7 +298,7 @@ func TestHTTPRoutePathRewrite(t *testing.T) {
InjectGossipEncryption: true,
AllowHTTPAnyway: true,
},
Ports: []int{
ExposedPorts: []int{
listenerPort,
fooHTTPPort,
fooGRPCPort,
Expand Down Expand Up @@ -525,7 +525,7 @@ func TestHTTPRouteParentRefChange(t *testing.T) {
InjectGossipEncryption: true,
AllowHTTPAnyway: true,
},
Ports: []int{
ExposedPorts: []int{
listenerOnePort,
listenerTwoPort,
serviceHTTPPort,
Expand Down
Loading