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

Remove support for Envoy 1.15 #11118

Merged
merged 7 commits into from
Oct 4, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
21 changes: 2 additions & 19 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down
35 changes: 3 additions & 32 deletions agent/xds/clusters.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay!

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) {
Expand Down
6 changes: 0 additions & 6 deletions agent/xds/delta.go
Original file line number Diff line number Diff line change
Expand Up @@ -310,12 +310,6 @@ func (s *Server) processDelta(stream ADSDeltaStream, reqCh <-chan *envoy_discove
}
if sent {
sentType[op.TypeUrl] = struct{}{}
if generator.ProxyFeatures.IncrementalXDSUpdatesMustBeSerial {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay!

// For more justification for this hacky fix, check the
// comments associated with
// generator.ProxyFeatures.IncrementalXDSUpdatesMustBeSerial
break
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion agent/xds/delta_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
40 changes: 15 additions & 25 deletions agent/xds/envoy_versioning.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
)
Expand All @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this. Prior to these 2 flags existing the struct was empty for a bit. Prior to that it had completely different features available. When I went to add these two flags I had to look at my own file history to see how to re-add a feature flag.

//
// 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) {
Expand Down Expand Up @@ -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:
eculver marked this conversation as resolved.
Show resolved Hide resolved
/*
if version.LessThan(minVersionAllowingEmptyGatewayClustersWithIncrementalXDS) {
sf.GatewaysNeedStubClusterWhenEmptyWithIncrementalXDS = true
}
*/

return sf, nil
}
Expand Down
9 changes: 1 addition & 8 deletions agent/xds/envoy_versioning_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,14 +109,7 @@ func TestDetermineSupportedProxyFeaturesFromString(t *testing.T) {
}
eculver marked this conversation as resolved.
Show resolved Hide resolved

// 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",
Expand Down
2 changes: 0 additions & 2 deletions agent/xds/proxysupport/proxysupport.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
}