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: default to speaking xDS v3, but allow for v2 to be spoken upon request #9658

Merged
merged 13 commits into from
Feb 26, 2021

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Jan 27, 2021

Additionally adds in support for envoy 1.17.0

@rboyer rboyer self-assigned this Jan 27, 2021
@vercel
Copy link

vercel bot commented Jan 27, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

consul-ui-staging – ./ui

🔍 Inspect: https://vercel.com/hashicorp/consul-ui-staging/qlhbtiht9
✅ Preview: Canceled

[Deployment for 880dd82 canceled]

@github-actions github-actions bot added pr/dependencies PR specifically updates dependencies of project theme/cli Flags and documentation for the CLI interface theme/envoy/xds Related to Envoy support type/ci Relating to continuous integration (CI) tooling for testing or releases type/docs Documentation needs to be created/updated/clarified labels Jan 27, 2021
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging January 27, 2021 23:22 Inactive
@vercel vercel bot temporarily deployed to Preview – consul January 27, 2021 23:23 Inactive
agent/xds/clusters.go Outdated Show resolved Hide resolved
agent/xds/clusters.go Outdated Show resolved Hide resolved
agent/xds/clusters.go Outdated Show resolved Hide resolved
agent/xds/listeners.go Outdated Show resolved Hide resolved
agent/xds/listeners.go Outdated Show resolved Hide resolved
agent/xds/listeners.go Outdated Show resolved Hide resolved
@@ -0,0 +1,522 @@
package xds
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 the most magical part.

Copy link
Contributor

Choose a reason for hiding this comment

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

When can this code be removed?

It seems like we'll end up having some sort of documentation note for whatever version 'x' this ships in to upgrade all versions of consul to 'x', then do any upgrades to versions > x. Is it correct to think that we'll have everything on v3 after upgrading to version x? Would this code be needed in versions x+1 and greater?

Copy link
Member Author

@rboyer rboyer Feb 19, 2021

Choose a reason for hiding this comment

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

Most likely 1-2 more releases. At worst it's when our envoy support matrix looks like:

  • 1.20.x (v2 deleted)
  • 1.19.x (v2 deleted)
  • 1.18.x (v2 deleted)
  • 1.17.x (v2 opt-in)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe put that in a doc comment here for future readers.

@@ -557,6 +557,7 @@ func (c *BootstrapConfig) generateListenerConfig(args *BootstrapTplArgs, bindAdd

clusterJSON := `{
"name": "` + selfAdminName + `",
"ignore_health_on_host_removal": false,
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 a default setting only set to force envoy to notice this should be definitely v3.

@rboyer rboyer force-pushed the remove-deprecated-xds-usages branch from 7ed3c35 to d42b71b Compare February 8, 2021 20:17
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 8, 2021 20:24 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 8, 2021 20:50 Inactive
@vercel vercel bot temporarily deployed to Preview – consul February 8, 2021 20:50 Inactive
@vercel vercel bot temporarily deployed to Preview – consul February 8, 2021 21:15 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 8, 2021 21:15 Inactive
@rboyer rboyer force-pushed the remove-deprecated-xds-usages branch from d42b71b to 87aa61f Compare February 11, 2021 22:14
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 11, 2021 22:14 Inactive
@rboyer rboyer force-pushed the remove-deprecated-xds-usages branch from 87aa61f to f42cf93 Compare February 11, 2021 22:31
@vercel vercel bot temporarily deployed to Preview – consul February 22, 2021 23:34 Inactive
@rboyer rboyer marked this pull request as ready for review February 22, 2021 23:36
Base automatically changed from shrink-envoy-golden-test-files to master February 24, 2021 20:04
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 24, 2021 20:56 Inactive
@vercel vercel bot temporarily deployed to Preview – consul February 24, 2021 20:56 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 24, 2021 23:31 Inactive
@vercel vercel bot temporarily deployed to Preview – consul February 24, 2021 23:31 Inactive
Copy link
Contributor

@markan markan left a comment

Choose a reason for hiding this comment

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

A few minor nits, but otherwise looks good to me.

}
}
return nil
case *any.Any:
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit, it might make more sense to pull this case to the front, since it is a little special. Not a blocker.

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'll leave it for now. The order of the switches roughly reads top-to-bottom the way they would be first used. So for instance this function is about converting responses so you enter with a DiscoveryResponse. The bulk of a DiscoveryResponse is a list of generic any.Any resources which is why that section comes second, and so on.

@@ -0,0 +1,522 @@
package xds
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe put that in a doc comment here for future readers.

@vercel vercel bot temporarily deployed to Preview – consul February 26, 2021 22:18 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 26, 2021 22:18 Inactive
@rboyer rboyer merged commit 398b766 into master Feb 26, 2021
@rboyer rboyer deleted the xds-v3 branch February 26, 2021 22:23
@hashicorp-ci
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/333097.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/dependencies PR specifically updates dependencies of project theme/cli Flags and documentation for the CLI interface theme/envoy/xds Related to Envoy support type/ci Relating to continuous integration (CI) tooling for testing or releases type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants