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

xds: version sniff envoy and switch regular expressions from 'regex' to 'safe_regex' on newer envoy versions #8222

Merged
merged 10 commits into from
Jul 9, 2020
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
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
2 changes: 1 addition & 1 deletion agent/xds/clusters.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
)

// clustersFromSnapshot returns the xDS API representation of the "clusters" in the snapshot.
func (s *Server) clustersFromSnapshot(cfgSnap *proxycfg.ConfigSnapshot, _ string) ([]proto.Message, error) {
func (s *Server) clustersFromSnapshot(_ connectionInfo, cfgSnap *proxycfg.ConfigSnapshot) ([]proto.Message, error) {
if cfgSnap == nil {
return nil, errors.New("nil config given")
}
Expand Down
91 changes: 55 additions & 36 deletions agent/xds/clusters_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package xds

import (
"bytes"
"path"
"path/filepath"
"sort"
"testing"
"text/template"
Expand Down Expand Up @@ -527,44 +527,53 @@ func TestClustersFromSnapshot(t *testing.T) {
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
require := require.New(t)
for _, envoyVersion := range supportedEnvoyVersions {
sf := determineSupportedProxyFeaturesFromString(envoyVersion)
t.Run("envoy-"+envoyVersion, func(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
require := require.New(t)

// Sanity check default with no overrides first
snap := tt.create(t)
// Sanity check default with no overrides first
snap := tt.create(t)

// We need to replace the TLS certs with deterministic ones to make golden
// files workable. Note we don't update these otherwise they'd change
// golder files for every test case and so not be any use!
setupTLSRootsAndLeaf(t, snap)
// We need to replace the TLS certs with deterministic ones to make golden
// files workable. Note we don't update these otherwise they'd change
// golder files for every test case and so not be any use!
setupTLSRootsAndLeaf(t, snap)

if tt.setup != nil {
tt.setup(snap)
}
if tt.setup != nil {
tt.setup(snap)
}

// Need server just for logger dependency
logger := testutil.Logger(t)
s := Server{
Logger: logger,
}
// Need server just for logger dependency
logger := testutil.Logger(t)
s := Server{
Logger: logger,
}

clusters, err := s.clustersFromSnapshot(snap, "my-token")
require.NoError(err)
sort.Slice(clusters, func(i, j int) bool {
return clusters[i].(*envoy.Cluster).Name < clusters[j].(*envoy.Cluster).Name
})
r, err := createResponse(ClusterType, "00000001", "00000001", clusters)
require.NoError(err)
cInfo := connectionInfo{
Token: "my-token",
ProxyFeatures: sf,
}
clusters, err := s.clustersFromSnapshot(cInfo, snap)
require.NoError(err)
sort.Slice(clusters, func(i, j int) bool {
return clusters[i].(*envoy.Cluster).Name < clusters[j].(*envoy.Cluster).Name
})
r, err := createResponse(ClusterType, "00000001", "00000001", clusters)
require.NoError(err)

gotJSON := responseToJSON(t, r)
gotJSON := responseToJSON(t, r)

gName := tt.name
if tt.overrideGoldenName != "" {
gName = tt.overrideGoldenName
}
gName := tt.name
if tt.overrideGoldenName != "" {
gName = tt.overrideGoldenName
}

require.JSONEq(golden(t, path.Join("clusters", gName), gotJSON), gotJSON)
require.JSONEq(goldenEnvoy(t, filepath.Join("clusters", gName), envoyVersion, gotJSON), gotJSON)
})
}
})
}
}
Expand Down Expand Up @@ -717,14 +726,24 @@ func setupTLSRootsAndLeaf(t *testing.T, snap *proxycfg.ConfigSnapshot) {
if snap.Leaf() != nil {
switch snap.Kind {
case structs.ServiceKindConnectProxy:
snap.ConnectProxy.Leaf.CertPEM = golden(t, "test-leaf-cert", "")
snap.ConnectProxy.Leaf.PrivateKeyPEM = golden(t, "test-leaf-key", "")
snap.ConnectProxy.Leaf.CertPEM = golden(t, "test-leaf-cert", "", "")
snap.ConnectProxy.Leaf.PrivateKeyPEM = golden(t, "test-leaf-key", "", "")
case structs.ServiceKindIngressGateway:
snap.IngressGateway.Leaf.CertPEM = golden(t, "test-leaf-cert", "")
snap.IngressGateway.Leaf.PrivateKeyPEM = golden(t, "test-leaf-key", "")
snap.IngressGateway.Leaf.CertPEM = golden(t, "test-leaf-cert", "", "")
snap.IngressGateway.Leaf.PrivateKeyPEM = golden(t, "test-leaf-key", "", "")
}
}
if snap.Roots != nil {
snap.Roots.Roots[0].RootCert = golden(t, "test-root-cert", "")
snap.Roots.Roots[0].RootCert = golden(t, "test-root-cert", "", "")
}
}

// supportedEnvoyVersions lists the versions that we generated golden tests for
//
// see: https://www.consul.io/docs/connect/proxies/envoy#supported-versions
var supportedEnvoyVersions = []string{
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder to move these to somewhere that an agent API can also pull them

Copy link
Member Author

Choose a reason for hiding this comment

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

I figure the moving can happen in the other PR that surfaces the feature.

"1.14.2",
"1.13.2",
"1.12.4",
"1.11.2",
}
2 changes: 1 addition & 1 deletion agent/xds/endpoints.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const (
)

// endpointsFromSnapshot returns the xDS API representation of the "endpoints"
func (s *Server) endpointsFromSnapshot(cfgSnap *proxycfg.ConfigSnapshot, _ string) ([]proto.Message, error) {
func (s *Server) endpointsFromSnapshot(_ connectionInfo, cfgSnap *proxycfg.ConfigSnapshot) ([]proto.Message, error) {
if cfgSnap == nil {
return nil, errors.New("nil config given")
}
Expand Down
71 changes: 40 additions & 31 deletions agent/xds/endpoints_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package xds

import (
"path"
"path/filepath"
"sort"
"testing"

Expand Down Expand Up @@ -554,44 +554,53 @@ func Test_endpointsFromSnapshot(t *testing.T) {
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
require := require.New(t)
for _, envoyVersion := range supportedEnvoyVersions {
sf := determineSupportedProxyFeaturesFromString(envoyVersion)
t.Run("envoy-"+envoyVersion, func(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
require := require.New(t)

// Sanity check default with no overrides first
snap := tt.create(t)
// Sanity check default with no overrides first
snap := tt.create(t)

// We need to replace the TLS certs with deterministic ones to make golden
// files workable. Note we don't update these otherwise they'd change
// golden files for every test case and so not be any use!
setupTLSRootsAndLeaf(t, snap)
// We need to replace the TLS certs with deterministic ones to make golden
// files workable. Note we don't update these otherwise they'd change
// golden files for every test case and so not be any use!
setupTLSRootsAndLeaf(t, snap)

if tt.setup != nil {
tt.setup(snap)
}
if tt.setup != nil {
tt.setup(snap)
}

// Need server just for logger dependency
logger := testutil.Logger(t)
s := Server{
Logger: logger,
}
// Need server just for logger dependency
logger := testutil.Logger(t)
s := Server{
Logger: logger,
}

endpoints, err := s.endpointsFromSnapshot(snap, "my-token")
sort.Slice(endpoints, func(i, j int) bool {
return endpoints[i].(*envoy.ClusterLoadAssignment).ClusterName < endpoints[j].(*envoy.ClusterLoadAssignment).ClusterName
})
require.NoError(err)
r, err := createResponse(EndpointType, "00000001", "00000001", endpoints)
require.NoError(err)
cInfo := connectionInfo{
Token: "my-token",
ProxyFeatures: sf,
}
endpoints, err := s.endpointsFromSnapshot(cInfo, snap)
sort.Slice(endpoints, func(i, j int) bool {
return endpoints[i].(*envoy.ClusterLoadAssignment).ClusterName < endpoints[j].(*envoy.ClusterLoadAssignment).ClusterName
})
require.NoError(err)
r, err := createResponse(EndpointType, "00000001", "00000001", endpoints)
require.NoError(err)

gotJSON := responseToJSON(t, r)
gotJSON := responseToJSON(t, r)

gName := tt.name
if tt.overrideGoldenName != "" {
gName = tt.overrideGoldenName
}
gName := tt.name
if tt.overrideGoldenName != "" {
gName = tt.overrideGoldenName
}

require.JSONEq(golden(t, path.Join("endpoints", gName), gotJSON), gotJSON)
require.JSONEq(goldenEnvoy(t, filepath.Join("endpoints", gName), envoyVersion, gotJSON), gotJSON)
})
}
})
}
}
83 changes: 83 additions & 0 deletions agent/xds/envoy_versioning.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package xds

import (
"fmt"
"regexp"

envoycore "github.com/envoyproxy/go-control-plane/envoy/api/v2/core"
"github.com/hashicorp/go-version"
)

var (
// minSafeRegexVersion reflects the minimum version where we could use safe_regex instead of regex
//
// NOTE: the first version that no longer supported the old style was 1.13.0
minSafeRegexVersion = version.Must(version.NewVersion("1.11.2"))
Copy link
Member Author

Choose a reason for hiding this comment

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

Conveniently for 1.8.x all of the officially supported versions are covered by this pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could have elected to avoid doing any sniffing at all here, but upcoming work would require us to add it right back in again anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

One thing we could start doing after this lands is to put in guardrails when an older or newer envoy dials in to a consul agent. Maybe not going so far as to reject the connection, but we could start logging a "this envoy is unsupported" warning message for stuff outside of our support matrix.

)

type supportedProxyFeatures struct {
RouterMatchSafeRegex bool // use safe_regex instead of regex in http.router rules
}

func determineSupportedProxyFeatures(node *envoycore.Node) supportedProxyFeatures {
version := determineEnvoyVersionFromNode(node)
if version == nil {
return supportedProxyFeatures{}
}

return supportedProxyFeatures{
RouterMatchSafeRegex: !version.LessThan(minSafeRegexVersion),
}
}

// example: 1580db37e9a97c37e410bad0e1507ae1a0fd9e77/1.12.4/Clean/RELEASE/BoringSSL
var buildVersionPattern = regexp.MustCompile(`^[a-f0-9]{40}/([^/]+)/Clean/RELEASE/BoringSSL$`)

func determineEnvoyVersionFromNode(node *envoycore.Node) *version.Version {
if node == nil {
return nil
}

if node.UserAgentVersionType == nil {
if node.BuildVersion == "" {
return nil
}

// Must be an older pre-1.13 envoy
m := buildVersionPattern.FindStringSubmatch(node.BuildVersion)
if m == nil {
return nil
}

return version.Must(version.NewVersion(m[1]))
}

if node.UserAgentName != "envoy" {
return nil
}

bv, ok := node.UserAgentVersionType.(*envoycore.Node_UserAgentBuildVersion)
if !ok {
// NOTE: we could sniff for *envoycore.Node_UserAgentVersion and do more regex but official builds don't have this problem.
return nil
}
if bv.UserAgentBuildVersion == nil {
return nil
}
v := bv.UserAgentBuildVersion.Version

return version.Must(version.NewVersion(
fmt.Sprintf("%d.%d.%d",
v.GetMajorNumber(),
v.GetMinorNumber(),
v.GetPatch(),
),
))
}

func determineSupportedProxyFeaturesFromString(vs string) supportedProxyFeatures {
version := version.Must(version.NewVersion(vs))
return supportedProxyFeatures{
RouterMatchSafeRegex: !version.LessThan(minSafeRegexVersion),
}
}
70 changes: 70 additions & 0 deletions agent/xds/envoy_versioning_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package xds

import (
"testing"

envoycore "github.com/envoyproxy/go-control-plane/envoy/api/v2/core"
envoytype "github.com/envoyproxy/go-control-plane/envoy/type"
"github.com/hashicorp/go-version"
"github.com/stretchr/testify/require"
)

func TestDetermineEnvoyVersionFromNode(t *testing.T) {
cases := map[string]struct {
node *envoycore.Node
expect *version.Version
}{
"empty": {
node: &envoycore.Node{},
expect: nil,
},
"only build version": {
node: &envoycore.Node{
BuildVersion: "1580db37e9a97c37e410bad0e1507ae1a0fd9e77/1.9.0/Clean/RELEASE/BoringSSL",
},
expect: version.Must(version.NewVersion("1.9.0")),
},
"user agent build version but no user agent": {
node: &envoycore.Node{
UserAgentName: "",
UserAgentVersionType: &envoycore.Node_UserAgentBuildVersion{
UserAgentBuildVersion: &envoycore.BuildVersion{
Version: &envoytype.SemanticVersion{
MajorNumber: 1,
MinorNumber: 14,
Patch: 4,
},
},
},
},
expect: nil,
},
"user agent build version with user agent": {
node: &envoycore.Node{
UserAgentName: "envoy",
UserAgentVersionType: &envoycore.Node_UserAgentBuildVersion{
UserAgentBuildVersion: &envoycore.BuildVersion{
Version: &envoytype.SemanticVersion{
MajorNumber: 1,
MinorNumber: 14,
Patch: 4,
},
},
},
},
expect: version.Must(version.NewVersion("1.14.4")),
},
}

for name, tc := range cases {
tc := tc
t.Run(name, func(t *testing.T) {
got := determineEnvoyVersionFromNode(tc.node)
if tc.expect != nil {
require.Equal(t, tc.expect, got)
} else {
require.Nil(t, got)
}
})
}
}
Loading