From 7e20a5e4f99fdfec770bc4f8a375c3cf16f7bc73 Mon Sep 17 00:00:00 2001 From: Evan Culver Date: Wed, 22 Sep 2021 11:48:50 -0700 Subject: [PATCH 1/3] connect: remove support for Envoy 1.15 --- .circleci/config.yml | 21 ++------------ agent/xds/clusters.go | 35 ++-------------------- agent/xds/delta.go | 6 ---- agent/xds/delta_test.go | 2 +- agent/xds/envoy_versioning.go | 40 ++++++++++---------------- agent/xds/envoy_versioning_test.go | 9 +----- agent/xds/proxysupport/proxysupport.go | 2 -- 7 files changed, 22 insertions(+), 93 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 5f35f8ba4b..91e04262cb 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -792,14 +792,14 @@ jobs: command: make test-coverage-ci - run: *notify-slack-failure - envoy-integration-test-1_15_5: &ENVOY_TESTS + envoy-integration-test-1_16_5: &ENVOY_TESTS docker: # We only really need bash and docker-compose which is installed on all # Circle images but pick Go since we have to pick one of them. - image: *GOLANG_IMAGE parallelism: 2 environment: - ENVOY_VERSION: "1.15.5" + ENVOY_VERSION: "1.16.5" steps: &ENVOY_INTEGRATION_TEST_STEPS - checkout # Get go binary from workspace @@ -832,17 +832,6 @@ jobs: path: *TEST_RESULTS_DIR - run: *notify-slack-failure - envoy-integration-test-1_15_5-v2compat: - <<: *ENVOY_TESTS - environment: - ENVOY_VERSION: "1.15.5" - TEST_V2_XDS: "1" - - envoy-integration-test-1_16_5: - <<: *ENVOY_TESTS - environment: - ENVOY_VERSION: "1.16.5" - envoy-integration-test-1_16_5-v2compat: <<: *ENVOY_TESTS environment: @@ -1104,12 +1093,6 @@ workflows: - nomad-integration-0_8: requires: - dev-build - - envoy-integration-test-1_15_5: - requires: - - dev-build - - envoy-integration-test-1_15_5-v2compat: - requires: - - dev-build - envoy-integration-test-1_16_5: requires: - dev-build diff --git a/agent/xds/clusters.go b/agent/xds/clusters.go index d2ba03efb6..eac2ac561f 100644 --- a/agent/xds/clusters.go +++ b/agent/xds/clusters.go @@ -38,53 +38,24 @@ func (s *ResourceGenerator) clustersFromSnapshot(cfgSnap *proxycfg.ConfigSnapsho if err != nil { return nil, err } - return s.maybeInjectStubClusterForGateways(res) + return res, nil case structs.ServiceKindMeshGateway: res, err := s.clustersFromSnapshotMeshGateway(cfgSnap) if err != nil { return nil, err } - return s.maybeInjectStubClusterForGateways(res) + return res, nil case structs.ServiceKindIngressGateway: res, err := s.clustersFromSnapshotIngressGateway(cfgSnap) if err != nil { return nil, err } - return s.maybeInjectStubClusterForGateways(res) + return res, nil default: return nil, fmt.Errorf("Invalid service kind: %v", cfgSnap.Kind) } } -func (s *ResourceGenerator) maybeInjectStubClusterForGateways(resources []proto.Message) ([]proto.Message, error) { - switch { - case !s.IncrementalXDS: - return resources, nil - case !s.ProxyFeatures.GatewaysNeedStubClusterWhenEmptyWithIncrementalXDS: - return resources, nil - case len(resources) > 0: - return resources, nil - } - - // For more justification for this hacky fix, check the comments associated - // with s.ProxyFeatures.GatewaysNeedStubClusterWhenEmptyWithIncrementalXDS - - const stubName = "consul-stub-cluster-working-around-envoy-bug-ignore" - return []proto.Message{ - &envoy_cluster_v3.Cluster{ - Name: stubName, - ConnectTimeout: ptypes.DurationProto(5 * time.Second), - ClusterDiscoveryType: &envoy_cluster_v3.Cluster_Type{Type: envoy_cluster_v3.Cluster_STATIC}, - LoadAssignment: &envoy_endpoint_v3.ClusterLoadAssignment{ - ClusterName: stubName, - Endpoints: []*envoy_endpoint_v3.LocalityLbEndpoints{ - {LbEndpoints: []*envoy_endpoint_v3.LbEndpoint{}}, - }, - }, - }, - }, nil -} - // clustersFromSnapshot returns the xDS API representation of the "clusters" // (upstreams) in the snapshot. func (s *ResourceGenerator) clustersFromSnapshotConnectProxy(cfgSnap *proxycfg.ConfigSnapshot) ([]proto.Message, error) { diff --git a/agent/xds/delta.go b/agent/xds/delta.go index 4009e9de93..71c97af933 100644 --- a/agent/xds/delta.go +++ b/agent/xds/delta.go @@ -310,12 +310,6 @@ func (s *Server) processDelta(stream ADSDeltaStream, reqCh <-chan *envoy_discove } if sent { sentType[op.TypeUrl] = struct{}{} - if generator.ProxyFeatures.IncrementalXDSUpdatesMustBeSerial { - // For more justification for this hacky fix, check the - // comments associated with - // generator.ProxyFeatures.IncrementalXDSUpdatesMustBeSerial - break - } } } } diff --git a/agent/xds/delta_test.go b/agent/xds/delta_test.go index 7e1746812b..56d1c93ec3 100644 --- a/agent/xds/delta_test.go +++ b/agent/xds/delta_test.go @@ -1228,7 +1228,7 @@ func TestServer_DeltaAggregatedResources_v3_IngressEmptyResponse(t *testing.T) { // REQ: clusters envoy.SendDeltaReq(t, ClusterType, nil) - // RESP: clustesr + // RESP: cluster assertDeltaResponseSent(t, envoy.deltaStream.sendCh, &envoy_discovery_v3.DeltaDiscoveryResponse{ TypeUrl: ClusterType, Nonce: hexString(1), diff --git a/agent/xds/envoy_versioning.go b/agent/xds/envoy_versioning.go index 0e218ad659..d8f10a3e86 100644 --- a/agent/xds/envoy_versioning.go +++ b/agent/xds/envoy_versioning.go @@ -11,10 +11,10 @@ import ( var ( // minSupportedVersion is the oldest mainline version we support. This should always be // the zero'th point release of the last element of proxysupport.EnvoyVersions. - minSupportedVersion = version.Must(version.NewVersion("1.15.0")) + minSupportedVersion = version.Must(version.NewVersion("1.16.0")) - minVersionAllowingEmptyGatewayClustersWithIncrementalXDS = version.Must(version.NewVersion("1.16.0")) - minVersionAllowingMultipleIncrementalXDSChanges = version.Must(version.NewVersion("1.16.0")) + // add min version constraints for associated feature flags when necessary, for example: + // minVersionAllowingEmptyGatewayClustersWithIncrementalXDS = version.Must(version.NewVersion("1.16.0")) specificUnsupportedVersions = []unsupportedVersion{} ) @@ -27,24 +27,15 @@ type unsupportedVersion struct { type supportedProxyFeatures struct { // add version dependent feature flags here - - // GatewaysNeedStubClusterWhenEmptyWithIncrementalXDS is needed to paper - // over some weird envoy behavior. // - // For some reason Envoy versions prior to 1.16.0 when sent an empty CDS - // list via the incremental xDS protocol will correctly ack the message and - // just never request LDS resources. - GatewaysNeedStubClusterWhenEmptyWithIncrementalXDS bool - - // IncrementalXDSUpdatesMustBeSerial is needed to avoid an envoy crash. + // For example, we previously had flags for Envoy < 1.16 called: // - // Versions of Envoy prior to 1.16.0 could crash if multiple in-flight - // changes to resources were happening during incremental xDS. To prevent - // that we force serial updates on those older versions. + // GatewaysNeedStubClusterWhenEmptyWithIncrementalXDS + // IncrementalXDSUpdatesMustBeSerial // - // issue: https://github.com/envoyproxy/envoy/issues/11877 - // PR: https://github.com/envoyproxy/envoy/pull/12069 - IncrementalXDSUpdatesMustBeSerial bool + // Which then manifested in the code for checks with this struct populated. + // By dropping support for 1.15, we no longer have any special flags here + // but leaving this flagging functionality for future one-offs. } func determineSupportedProxyFeatures(node *envoy_core_v3.Node) (supportedProxyFeatures, error) { @@ -82,13 +73,12 @@ func determineSupportedProxyFeaturesFromVersion(version *version.Version) (suppo sf := supportedProxyFeatures{} - if version.LessThan(minVersionAllowingEmptyGatewayClustersWithIncrementalXDS) { - sf.GatewaysNeedStubClusterWhenEmptyWithIncrementalXDS = true - } - - if version.LessThan(minVersionAllowingMultipleIncrementalXDSChanges) { - sf.IncrementalXDSUpdatesMustBeSerial = true - } + // add version contraints to poipulate feature flags here when necessary, for example: + /* + if version.LessThan(minVersionAllowingEmptyGatewayClustersWithIncrementalXDS) { + sf.GatewaysNeedStubClusterWhenEmptyWithIncrementalXDS = true + } + */ return sf, nil } diff --git a/agent/xds/envoy_versioning_test.go b/agent/xds/envoy_versioning_test.go index bbcd9c7c52..dc1e2bcc49 100644 --- a/agent/xds/envoy_versioning_test.go +++ b/agent/xds/envoy_versioning_test.go @@ -109,14 +109,7 @@ func TestDetermineSupportedProxyFeaturesFromString(t *testing.T) { } // Insert a bunch of valid versions. - for _, v := range []string{ - "1.15.0", "1.15.1", "1.15.2", "1.15.3", "1.15.4", "1.15.5", - } { - cases[v] = testcase{expect: supportedProxyFeatures{ - GatewaysNeedStubClusterWhenEmptyWithIncrementalXDS: true, - IncrementalXDSUpdatesMustBeSerial: true, - }} - } + // Populate feature flags here when appropriate. See consul 1.10.x for reference. for _, v := range []string{ "1.16.0", "1.16.1", "1.16.2", "1.16.3", "1.16.4", "1.16.5", "1.17.0", "1.17.1", "1.17.2", "1.17.3", "1.17.4", diff --git a/agent/xds/proxysupport/proxysupport.go b/agent/xds/proxysupport/proxysupport.go index 908a7ba401..530ff5d017 100644 --- a/agent/xds/proxysupport/proxysupport.go +++ b/agent/xds/proxysupport/proxysupport.go @@ -11,10 +11,8 @@ var EnvoyVersions = []string{ "1.18.4", "1.17.4", "1.16.5", - "1.15.5", } var EnvoyVersionsV2 = []string{ "1.16.5", - "1.15.5", } From 4f1a8d4ea6c0db60f8f488a59308d8161a962e01 Mon Sep 17 00:00:00 2001 From: Evan Culver Date: Wed, 29 Sep 2021 01:05:45 +0200 Subject: [PATCH 2/3] Fix typo Co-authored-by: Freddy --- agent/xds/envoy_versioning.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/xds/envoy_versioning.go b/agent/xds/envoy_versioning.go index d8f10a3e86..33849ebb49 100644 --- a/agent/xds/envoy_versioning.go +++ b/agent/xds/envoy_versioning.go @@ -73,7 +73,7 @@ func determineSupportedProxyFeaturesFromVersion(version *version.Version) (suppo sf := supportedProxyFeatures{} - // add version contraints to poipulate feature flags here when necessary, for example: + // add version constraints to populate feature flags here when necessary, for example: /* if version.LessThan(minVersionAllowingEmptyGatewayClustersWithIncrementalXDS) { sf.GatewaysNeedStubClusterWhenEmptyWithIncrementalXDS = true From db397d62c5ea3bfda09b436d1aa13993de46868e Mon Sep 17 00:00:00 2001 From: Evan Culver Date: Fri, 1 Oct 2021 11:28:26 -0700 Subject: [PATCH 3/3] Add 1.15 versions to too old list --- agent/xds/envoy_versioning_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/agent/xds/envoy_versioning_test.go b/agent/xds/envoy_versioning_test.go index dc1e2bcc49..1efd10da3b 100644 --- a/agent/xds/envoy_versioning_test.go +++ b/agent/xds/envoy_versioning_test.go @@ -106,6 +106,12 @@ func TestDetermineSupportedProxyFeaturesFromString(t *testing.T) { "1.14.5": {expectErr: "Envoy 1.14.5 " + errTooOld}, "1.14.6": {expectErr: "Envoy 1.14.6 " + errTooOld}, "1.14.7": {expectErr: "Envoy 1.14.7 " + errTooOld}, + "1.15.0": {expectErr: "Envoy 1.15.0 " + errTooOld}, + "1.15.1": {expectErr: "Envoy 1.15.1 " + errTooOld}, + "1.15.2": {expectErr: "Envoy 1.15.2 " + errTooOld}, + "1.15.3": {expectErr: "Envoy 1.15.3 " + errTooOld}, + "1.15.4": {expectErr: "Envoy 1.15.4 " + errTooOld}, + "1.15.5": {expectErr: "Envoy 1.15.5 " + errTooOld}, } // Insert a bunch of valid versions.