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.

Sign up for GitHub

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

Jump to bottom

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

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

merged 10 commits into from
Jul 9, 2020

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Jul 1, 2020 •
edited
Loading

  • look into combining the proxy features with the token
  • docs update for service-router to link to a different page for describing the regex syntax (which is now re2)

Fixes: #8205

rboyer
rboyer commented Jul 6, 2020
View reviewed changes
agent/xds/envoy_versioning.go
// 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.

@rboyer
Copy link
Member Author

rboyer commented Jul 6, 2020

The 1.8.x backport is going to depend on #8247

@rboyer
Copy link
Member Author

rboyer commented Jul 6, 2020

The 1.7.x backport is going to depend on #8249

@rboyer rboyer changed the title version sniff envoy and switch regular expressions from 'regex' to 'safe_regex' on newer envoy versions xds: version sniff envoy and switch regular expressions from 'regex' to 'safe_regex' on newer envoy versions Jul 6, 2020
@rboyer rboyer requested a review from a team July 6, 2020 21:20
@rboyer rboyer self-assigned this Jul 6, 2020
@rboyer rboyer added theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies theme/envoy/xds Related to Envoy support labels Jul 6, 2020
@rboyer rboyer marked this pull request as ready for review July 6, 2020 21:20
@rboyer rboyer force-pushed the envoy-versions branch 3 times, most recently from 6b95ee8 to 3a58925 Compare July 8, 2020 14:58
@rboyer
xds: version sniff envoy and switch regular expressions from 'regex' …
f6e7ca7 
…to 'safe_regex' on newer envoy versions

- cut down on extra node metadata transmission
- split the golden file generation to compare all envoy version
This was referenced Jul 8, 2020
Merged
Merged
rboyer
rboyer commented Jul 8, 2020
View reviewed changes
test/integration/connect/envoy/test-envoy-versions.sh
@@ -0,0 +1,34 @@
#!/bin/bash
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a script to assist with running the current cases through a version matrix to deduce envoy versions for plugging into those version checks.

@rboyer rboyer mentioned this pull request Jul 8, 2020
Merged
14 tasks
freddygv
freddygv reviewed Jul 8, 2020
View reviewed changes
Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

This mostly looks good to me. Left some minor questions/comments inline.

agent/xds/clusters_test.go Outdated Show resolved Hide resolved
agent/xds/clusters.go Outdated Show resolved Hide resolved
agent/xds/clusters_test.go
// 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.

agent/xds/listeners.go Outdated Show resolved Hide resolved
agent/xds/server.go Show resolved Hide resolved
test/integration/connect/envoy/helpers.bash Outdated Show resolved Hide resolved
test/integration/connect/envoy/helpers.bash Outdated Show resolved Hide resolved
test/integration/connect/envoy/case-cfg-router-features/config_entries.hcl Outdated Show resolved Hide resolved
test/integration/connect/envoy/case-cfg-router-features/verify.bats Show resolved Hide resolved
agent/xds/envoy_versioning.go Outdated Show resolved Hide resolved
@rboyer rboyer requested a review from freddygv July 9, 2020 21:35
freddygv
freddygv approved these changes Jul 9, 2020
View reviewed changes
Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

:shipit:

rboyer added a commit that referenced this pull request Jul 9, 2020
@rboyer
[backport: 1.7.x] xds: version sniff envoy and switch regular express…
1b7a8ba 
…ions from 'regex' to 'safe_regex' on newer envoy versions (#8266)

cherry-pick of #8222 onto origin/release/1.7.x

Fixes: #8205
rboyer added a commit that referenced this pull request Jul 9, 2020
@rboyer
[backport: 1.8.x] xds: version sniff envoy and switch regular express…
2142a69 
…ions from 'regex' to 'safe_regex' on newer envoy versions (#8265)

cherry-pick of #8222 onto origin/release/1.8.x

Fixes: #8205
@rboyer rboyer merged commit 1eef096 into master Jul 9, 2020
@rboyer rboyer deleted the envoy-versions branch July 9, 2020 22:04
@dependabot dependabot bot mentioned this pull request Mar 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freddygv freddygv freddygv approved these changes

Assignees

@rboyer rboyer

Labels
theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies theme/envoy/xds Related to Envoy support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle deprecated envoy fields with version sniffing
2 participants
@rboyer @freddygv