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

Enhancement: Consul Compatibility Checking #15818

Merged
merged 5 commits into from
Dec 20, 2022

Conversation

wilkermichael
Copy link
Contributor

@wilkermichael wilkermichael commented Dec 15, 2022

Description

Note: docs team see last commit for docs change

This PR introduces an enhancement to enable users to detect incompatible versions of Envoy early. This PR was motivated by issue #13131.

With the addition of these code changes, when using the consul connect envoy command the version of envoy being connected to is compared against the supported versions coded into the Consul binary. If the version of Envoy is deemed to be incompatible, then an error will be logged and the service will exit.

Since this is a potentially breaking change, and there may be reasons that a user is using a version of Consul that we don't recognize as compatible (such as custom builds of Envoy), a flag has also been added to disable the check: --ignore-envoy-compatibility. By default this flag will be set to false and we will perform the check and error out as necessary.

Example: consul connect envoy -admin-bind=0.0.0.0:19000 -sidecar-for=svc1 --ignore-envoy-compatibility

How do we determine compatibility?

Compatibility is defined in proxysupport.go

var EnvoyVersions = []string{
	"1.24.0",
	"1.23.2",
	"1.22.5",
	"1.21.5",
}

For the above list, Consul is compatible with any version >= 1.21.0 and <1.25. For example, we are compatible with 1.21.1, 1.21.2, etc. and anything in between up to any future patches to 1.24.

A list of explicitly unsupported Envoy versions has also been added to proxysupport.go in case there is a requirement to explicitly callout patches for breaking compatibility (e.g. if 1.21.2 had a known compatibility breaking change).

Log Message Example

dc1svc1    | Envoy version 1.18.3 is not supported. If there is a reason you need to use this version of envoy use the ignore-envoy-compatibility flag. Using an unsupported version of Envoy is not recommended and your experience may vary. For more information on compatibility see https://developer.hashicorp.com/consul/docs/connect/proxies/envoy#envoy-and-consul-client-agent

This resolves #13131

As always, I try to add some extra context to my commits, so that's usually the best way to go through my PRs.

Testing & Reproduction steps

Manual Testing Scenarios

  • Lower Bound Test: Tested a patch version below 1.21.5, confirmed no error
  • In Between Test: Tested a patch version between 1.21 and 1.24, confirmed no error
  • Upper Bound Test: Modified the list in proxysupport.go so that 1.23.1 was the top of the list, confirmed that 1.23.2 resulted in no error
  • Upper Out Of Bounds Test: Modified the list in proxysupport.go so that 1.23.1 was the top of the list, confirmed that 1.24.0 resulted in error and exited
  • Lower Out Of Bounds Test: Tested with Envoy 1.18, confirmed that this resulted in an error and exited
  • Flag Testing: Tested with 1.18 using --ignore-envoy-compatibility confirmed that there was no error for compatibility
  • Incompatibility List Testing: added 1.21.1 to the incompatibility list, confirmed connecting to this version resulted in an error and exit

Links

N/A

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

@wilkermichael wilkermichael added the type/enhancement Proposed improvement or new feature label Dec 15, 2022
@wilkermichael wilkermichael requested review from hashi-derek and a team December 15, 2022 23:03
@github-actions github-actions bot added theme/cli Flags and documentation for the CLI interface theme/envoy/xds Related to Envoy support labels Dec 15, 2022
@wilkermichael wilkermichael force-pushed the mw/net-1303-ensure-envoy-compatible branch 3 times, most recently from 1a1a2cd to cf9bb2e Compare December 15, 2022 23:40
Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp left a comment

Choose a reason for hiding this comment

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

Added some comments to consider. I defer to the eng team for a formal review.

agent/xds/proxysupport/proxysupport.go Show resolved Hide resolved
agent/xds/proxysupport/proxysupport.go Show resolved Hide resolved
c.UI.Error(fmt.Sprintf("Envoy version %s is not supported. If there is a reason you need to use "+
"this version of envoy use the ignore-envoy-compatibility flag. Using an unsupported version of Envoy "+
"is not recommended and your experience may vary. For more information on compatibility "+
"see https://developer.hashicorp.com/consul/docs/connect/proxies/envoy#envoy-and-consul-client-agent", v))
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: if anyone moves this page or renames the subsection with this anchor tag, this link will no longer work. When I write the docs PR, I should make sure the docs page has an internal comment that calls out this dependency from the code that we should aim to preserve.

.changelog/15818.txt Show resolved Hide resolved
command/connect/envoy/envoy.go Show resolved Hide resolved
@vercel
Copy link

vercel bot commented Dec 16, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
consul 🔄 Building (Inspect) Dec 19, 2022 at 6:49PM (UTC)
consul-ui-staging 🔄 Building (Inspect) Dec 19, 2022 at 6:49PM (UTC)

@wilkermichael wilkermichael force-pushed the mw/net-1303-ensure-envoy-compatible branch 3 times, most recently from 968eae9 to 7cca4e6 Compare December 16, 2022 18:02
@wilkermichael
Copy link
Contributor Author

@hashi-derek This is ready for review

There's currently a failing integration test, but it's not related to these changes (It's failing in other PRs)

command/connect/envoy/envoy_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@trujillo-adam trujillo-adam left a comment

Choose a reason for hiding this comment

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

I made a few adjustments to the flag description. Also, it seems that there should be an overview section somewhere that describes the default behavior when registering a sidecar proxy. If we don't already have this content, we should make a note to consider creating it.

Comment on lines 107 to 110
- `-ignore-envoy-compatibility` - Specifies whether or not the program should check the envoy version for compatibility.
If this flag is set to true, the program will not check the envoy version for compatibility.
It is recommended in most situations to leave this flag set to false, as ensuring compatibility with the
Envoy version can prevent potential issues. By default this is set to `false`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `-ignore-envoy-compatibility` - Specifies whether or not the program should check the envoy version for compatibility.
If this flag is set to true, the program will not check the envoy version for compatibility.
It is recommended in most situations to leave this flag set to false, as ensuring compatibility with the
Envoy version can prevent potential issues. By default this is set to `false`.
- `-ignore-envoy-compatibility` - If set to `true`, this flag ignores the Envoy version compatibility check. We recommend setting this flag set to `false` to ensure compatibility with Envoy and prevent potential issues. Default is `false`.

Just a few tweaks to simplify the description and consistently use monospace. Did we update the documentation under /connect/proxies/envoy or wherever we describe the Envoy startup process? I was going to add a link, but I'm not sure where it would go. Maybe this would be a better place to document this behavior? /connect/registration/service-registration

Copy link
Contributor Author

@wilkermichael wilkermichael Dec 19, 2022

Choose a reason for hiding this comment

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

Sorry @trujillo-adam, I'm not sure which paths you're referring to? Where exactly is /connect/registration/service-registration or /connect/proxies/envoy?

This is described here because this is a flag for and specific to the consul connect envoy command

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking one of these pages would be the place to describe the default bootstrap process:
https://developer.hashicorp.com/consul/docs/connect/proxies/envoy
https://developer.hashicorp.com/consul/docs/connect/registration/service-registration

Maybe we don't need it, though. The beginning of this page says "The default behavior is to generate the necessary bootstrap configuration for Envoy based on the environment variables and options provided and by talking to the local Consul agent." Maybe we should add a sentence there letting people know about the version check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add a sentence about the version check at an overview level?

It seems like a thing that you don't need to be told about. If you run into it (because you're using an incompatible version), the error message you get will tell you what went wrong and why. It seems like something that's already very discoverable in-context in Consul itself, so doesn't need to be present at an overview level in the docs... Freeing us up to focus on other important, high-level info in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that makes sense to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jkirschner-hashicorp and @trujillo-adam are you okay with me resolving this discussing and merging this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to close this PR for now, we can always create another one if we want to address the documentation updates

@wilkermichael wilkermichael force-pushed the mw/net-1303-ensure-envoy-compatible branch 3 times, most recently from 42a39ba to 46d79be Compare December 19, 2022 18:49
- added an UnsupportedEnvoyVersions list
- removed an unused error from TestDetermineSupportedProxyFeaturesFromString
- modified minSupportedVersion to use the function for getting the Min Envoy major version. Using just the major version without the patch is equivalent to using `.0`
- added a new exec.go file to not be locked to unix system
@wilkermichael wilkermichael force-pushed the mw/net-1303-ensure-envoy-compatible branch from 46d79be to 5eae945 Compare December 20, 2022 17:46
@wilkermichael wilkermichael merged commit 1b28b89 into main Dec 20, 2022
@wilkermichael wilkermichael deleted the mw/net-1303-ensure-envoy-compatible branch December 20, 2022 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/cli Flags and documentation for the CLI interface theme/envoy/xds Related to Envoy support type/enhancement Proposed improvement or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dropping support for envoy 1.18 in consul 1.12 should be listed as a breaking change
4 participants