Backport of [OSS] Fix initial_fetch_timeout to wait for all xDS resou…

…rces into release/1.16.x (#18065) * backport of commit 8a2f60d * backport of commit e17e53c * backport of commit d919d55 --------- Co-authored-by: DanStough <[email protected]>
hashicorp · Jul 10, 2023 · 2ef95b6 · 2ef95b6
1 parent 5432554
commit 2ef95b6
Show file tree

Hide file tree

Showing 47 changed files with 137 additions and 21 deletions.
diff --git a/.changelog/18024.txt b/.changelog/18024.txt
@@ -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.
+```
diff --git a/agent/proxycfg/api_gateway.go b/agent/proxycfg/api_gateway.go
@@ -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
 	}
 

diff --git a/agent/proxycfg/ingress_gateway.go b/agent/proxycfg/ingress_gateway.go
@@ -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

diff --git a/agent/proxycfg/snapshot.go b/agent/proxycfg/snapshot.go
@@ -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 {
@@ -964,17 +988,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

diff --git a/agent/proxycfg/state.go b/agent/proxycfg/state.go
@@ -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:"

diff --git a/agent/proxycfg/testing_api_gateway.go b/agent/proxycfg/testing_api_gateway.go
@@ -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.
 			},

diff --git a/command/connect/envoy/bootstrap_tpl.go b/command/connect/envoy/bootstrap_tpl.go
@@ -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": {

diff --git a/command/connect/envoy/testdata/CONSUL_GRPC_ADDR-with-https-scheme-enables-tls.golden b/command/connect/envoy/testdata/CONSUL_GRPC_ADDR-with-https-scheme-enables-tls.golden
@@ -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": {

diff --git a/...connect/envoy/testdata/CONSUL_HTTP_ADDR-with-https-scheme-does-not-affect-grpc-tls.golden b/...connect/envoy/testdata/CONSUL_HTTP_ADDR-with-https-scheme-does-not-affect-grpc-tls.golden
@@ -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": {

diff --git a/command/connect/envoy/testdata/access-log-path.golden b/command/connect/envoy/testdata/access-log-path.golden
@@ -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": {

diff --git a/command/connect/envoy/testdata/access-logs-enabled-custom.golden b/command/connect/envoy/testdata/access-logs-enabled-custom.golden
@@ -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": {

diff --git a/command/connect/envoy/testdata/access-logs-enabled.golden b/command/connect/envoy/testdata/access-logs-enabled.golden
@@ -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": {

diff --git a/command/connect/envoy/testdata/acl-enabled-and-token.golden b/command/connect/envoy/testdata/acl-enabled-and-token.golden
@@ -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": {

diff --git a/command/connect/envoy/testdata/acl-enabled-but-no-token.golden b/command/connect/envoy/testdata/acl-enabled-but-no-token.golden
@@ -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": {

diff --git a/...connect/envoy/testdata/both-CONSUL_HTTP_ADDR-PLAIN-and-CONSUL_GRPC_ADDR-TLS-is-tls.golden b/...connect/envoy/testdata/both-CONSUL_HTTP_ADDR-PLAIN-and-CONSUL_GRPC_ADDR-TLS-is-tls.golden
@@ -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": {

diff --git a/...nnect/envoy/testdata/both-CONSUL_HTTP_ADDR-TLS-and-CONSUL_GRPC_ADDR-PLAIN-is-plain.golden b/...nnect/envoy/testdata/both-CONSUL_HTTP_ADDR-TLS-and-CONSUL_GRPC_ADDR-PLAIN-is-plain.golden
@@ -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": {

diff --git a/command/connect/envoy/testdata/defaults-nodemeta.golden b/command/connect/envoy/testdata/defaults-nodemeta.golden
@@ -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": {

diff --git a/command/connect/envoy/testdata/defaults.golden b/command/connect/envoy/testdata/defaults.golden
@@ -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": {

diff --git a/command/connect/envoy/testdata/deprecated-grpc-addr-config.golden b/command/connect/envoy/testdata/deprecated-grpc-addr-config.golden
@@ -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": {

diff --git a/command/connect/envoy/testdata/envoy-readiness-probe.golden b/command/connect/envoy/testdata/envoy-readiness-probe.golden
@@ -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": {

diff --git a/command/connect/envoy/testdata/existing-ca-file.golden b/command/connect/envoy/testdata/existing-ca-file.golden
@@ -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": {