[OSS] Fix initial_fetch_timeout to wait for all xDS resources (#18024)

* fix(connect): set initial_fetch_time to wait indefinitely

* changelog

* PR feedback 1
This commit is contained in:
Dan Stough 2023-07-10 17:08:06 -04:00 committed by GitHub
parent f4b08040fd
commit 1b08626358
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
47 changed files with 137 additions and 21 deletions

3
.changelog/18024.txt Normal file
View File

@ -0,0 +1,3 @@
```release-note:bug
connect: fix a bug with Envoy potentially starting with incomplete configuration by not waiting enough for initial xDS configuration.
```

View File

@ -6,6 +6,7 @@ package proxycfg
import (
"context"
"fmt"
"github.com/hashicorp/consul/acl"
"github.com/hashicorp/consul/agent/leafcert"
"github.com/hashicorp/consul/agent/proxycfg/internal/watch"
@ -48,13 +49,13 @@ func (h *handlerAPIGateway) initialize(ctx context.Context) (ConfigSnapshot, err
}
// Watch the api-gateway's config entry
err = h.subscribeToConfigEntry(ctx, structs.APIGateway, h.service, h.proxyID.EnterpriseMeta, gatewayConfigWatchID)
err = h.subscribeToConfigEntry(ctx, structs.APIGateway, h.service, h.proxyID.EnterpriseMeta, apiGatewayConfigWatchID)
if err != nil {
return snap, err
}
// Watch the bound-api-gateway's config entry
err = h.subscribeToConfigEntry(ctx, structs.BoundAPIGateway, h.service, h.proxyID.EnterpriseMeta, gatewayConfigWatchID)
err = h.subscribeToConfigEntry(ctx, structs.BoundAPIGateway, h.service, h.proxyID.EnterpriseMeta, boundGatewayConfigWatchID)
if err != nil {
return snap, err
}
@ -108,9 +109,9 @@ func (h *handlerAPIGateway) handleUpdate(ctx context.Context, u UpdateEvent, sna
if err := h.handleRootCAUpdate(u, snap); err != nil {
return err
}
case u.CorrelationID == gatewayConfigWatchID:
case u.CorrelationID == apiGatewayConfigWatchID || u.CorrelationID == boundGatewayConfigWatchID:
// Handle change in the api-gateway or bound-api-gateway config entry
if err := h.handleGatewayConfigUpdate(ctx, u, snap); err != nil {
if err := h.handleGatewayConfigUpdate(ctx, u, snap, u.CorrelationID); err != nil {
return err
}
case u.CorrelationID == inlineCertificateConfigWatchID:
@ -146,11 +147,20 @@ func (h *handlerAPIGateway) handleRootCAUpdate(u UpdateEvent, snap *ConfigSnapsh
// In particular, we want to make sure that we're subscribing to any attached resources such
// as routes and certificates. These additional subscriptions will enable us to update the
// config snapshot appropriately for any route or certificate changes.
func (h *handlerAPIGateway) handleGatewayConfigUpdate(ctx context.Context, u UpdateEvent, snap *ConfigSnapshot) error {
func (h *handlerAPIGateway) handleGatewayConfigUpdate(ctx context.Context, u UpdateEvent, snap *ConfigSnapshot, correlationID string) error {
resp, ok := u.Result.(*structs.ConfigEntryResponse)
if !ok {
return fmt.Errorf("invalid type for response: %T", u.Result)
} else if resp.Entry == nil {
// A nil response indicates that we have the watch configured and that we are done with further changes
// until a new response comes in. By setting these earlier we allow a minimal xDS snapshot to configure the
// gateway.
if correlationID == apiGatewayConfigWatchID {
snap.APIGateway.GatewayConfigLoaded = true
}
if correlationID == boundGatewayConfigWatchID {
snap.APIGateway.BoundGatewayConfigLoaded = true
}
return nil
}

View File

@ -95,6 +95,11 @@ func (s *handlerIngressGateway) handleUpdate(ctx context.Context, u UpdateEvent,
if !ok {
return fmt.Errorf("invalid type for response: %T", u.Result)
}
// We set this even if the response is empty so that we know the watch is set,
// but we don't block if the ingress config entry is unset for this gateway
snap.IngressGateway.GatewayConfigLoaded = true
if resp.Entry == nil {
return nil
}
@ -103,7 +108,6 @@ func (s *handlerIngressGateway) handleUpdate(ctx context.Context, u UpdateEvent,
return fmt.Errorf("invalid type for config entry: %T", resp.Entry)
}
snap.IngressGateway.GatewayConfigLoaded = true
snap.IngressGateway.TLSConfig = gatewayConf.TLS
if gatewayConf.Defaults != nil {
snap.IngressGateway.Defaults = *gatewayConf.Defaults

View File

@ -824,6 +824,18 @@ DOMAIN_LOOP:
return services, upstreams, compiled, err
}
// valid tests for two valid api gateway snapshot states:
// 1. waiting: the watch on api and bound gateway entries is set, but none were received
// 2. loaded: both the valid config entries AND the leaf certs are set
func (c *configSnapshotAPIGateway) valid() bool {
waiting := c.GatewayConfigLoaded && len(c.Upstreams) == 0 && c.BoundGatewayConfigLoaded && c.Leaf == nil
// If we have a leaf, it implies we successfully watched parent resources
loaded := c.GatewayConfigLoaded && c.BoundGatewayConfigLoaded && c.Leaf != nil
return waiting || loaded
}
type configSnapshotIngressGateway struct {
ConfigSnapshotUpstreams
@ -872,6 +884,18 @@ func (c *configSnapshotIngressGateway) isEmpty() bool {
!c.MeshConfigSet
}
// valid tests for two valid ingress snapshot states:
// 1. waiting: the watch on ingress config entries is set, but none were received
// 2. loaded: both the ingress config entry AND the leaf cert are set
func (c *configSnapshotIngressGateway) valid() bool {
waiting := c.GatewayConfigLoaded && len(c.Upstreams) == 0 && c.Leaf == nil
// If we have a leaf, it implies we successfully watched parent resources
loaded := c.GatewayConfigLoaded && c.Leaf != nil
return waiting || loaded
}
type APIGatewayListenerKey = IngressListenerKey
func APIGatewayListenerKeyFromListener(l structs.APIGatewayListener) APIGatewayListenerKey {
@ -965,17 +989,14 @@ func (s *ConfigSnapshot) Valid() bool {
case structs.ServiceKindIngressGateway:
return s.Roots != nil &&
s.IngressGateway.Leaf != nil &&
s.IngressGateway.GatewayConfigLoaded &&
s.IngressGateway.valid() &&
s.IngressGateway.HostsSet &&
s.IngressGateway.MeshConfigSet
case structs.ServiceKindAPIGateway:
// TODO Is this the proper set of things to validate?
return s.Roots != nil &&
s.APIGateway.Leaf != nil &&
s.APIGateway.GatewayConfigLoaded &&
s.APIGateway.BoundGatewayConfigLoaded &&
s.APIGateway.valid() &&
s.APIGateway.MeshConfigSet
default:
return false

View File

@ -36,6 +36,8 @@ const (
serviceResolversWatchID = "service-resolvers"
gatewayServicesWatchID = "gateway-services"
gatewayConfigWatchID = "gateway-config"
apiGatewayConfigWatchID = "api-gateway-config"
boundGatewayConfigWatchID = "bound-gateway-config"
inlineCertificateConfigWatchID = "inline-certificate-config"
routeConfigWatchID = "route-config"
externalServiceIDPrefix = "external-service:"

View File

@ -6,9 +6,10 @@ package proxycfg
import (
"fmt"
"github.com/mitchellh/go-testing-interface"
"github.com/hashicorp/consul/agent/connect"
"github.com/hashicorp/consul/agent/consul/discoverychain"
"github.com/mitchellh/go-testing-interface"
"github.com/hashicorp/consul/agent/configentry"
"github.com/hashicorp/consul/agent/structs"
@ -49,13 +50,13 @@ func TestConfigSnapshotAPIGateway(
Result: placeholderLeaf,
},
{
CorrelationID: gatewayConfigWatchID,
CorrelationID: apiGatewayConfigWatchID,
Result: &structs.ConfigEntryResponse{
Entry: entry,
},
},
{
CorrelationID: gatewayConfigWatchID,
CorrelationID: boundGatewayConfigWatchID,
Result: &structs.ConfigEntryResponse{
Entry: boundEntry,
},
@ -141,13 +142,13 @@ func TestConfigSnapshotAPIGateway_NilConfigEntry(
Result: roots,
},
{
CorrelationID: gatewayConfigWatchID,
CorrelationID: apiGatewayConfigWatchID,
Result: &structs.ConfigEntryResponse{
Entry: nil, // The first watch on a config entry will return nil if the config entry doesn't exist.
},
},
{
CorrelationID: gatewayConfigWatchID,
CorrelationID: boundGatewayConfigWatchID,
Result: &structs.ConfigEntryResponse{
Entry: nil, // The first watch on a config entry will return nil if the config entry doesn't exist.
},

View File

@ -281,10 +281,12 @@ const bootstrapTemplate = `{
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -197,10 +197,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -197,10 +197,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -219,10 +219,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -197,10 +197,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -185,10 +185,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -273,10 +273,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -197,10 +197,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -197,10 +197,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -206,10 +206,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -197,10 +197,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -196,10 +196,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -183,10 +183,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -197,10 +197,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -273,10 +273,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -273,10 +273,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -274,10 +274,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -273,10 +273,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -273,10 +273,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -273,10 +273,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -310,10 +310,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -310,10 +310,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -273,10 +273,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -62,10 +62,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -222,10 +222,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -184,10 +184,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -217,10 +217,12 @@
"dynamic_resources": {
"lds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"cds_config": {
"ads": {},
"initial_fetch_timeout": "0s",
"resource_api_version": "V3"
},
"ads_config": {

View File

@ -62,11 +62,6 @@ func TestIngressGateway(t *testing.T) {
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.