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

[DRAFT] [NEED FEEDBACK] Support websocket through envoy #13418

Closed
wants to merge 1 commit into from

Conversation

davide-baldo
Copy link

This PR aim to implement websocket support, referenced here #8283 .

Rather than always enable the websocket support for everything (like in this old PR #9639), I've created a new websocket parameter which can be used wherever protocol can be used, usually when you specify the service protocol of the service or when you override the path.
In this patch, websocket it's disabled by default.

Basic unit testing was added but I couldn't validate them since i couldn't perform a clean test run with intellij, make test or make test-docker.
I've manually tested in a 2 node server installation and envoy configuration gets created correctly and it's routing correctly.

Since this is the first time I approach consul codebase i ask, if possible, for guidance before committing more time, especially if the overall approach is correct.

@hashicorp-cla
Copy link

hashicorp-cla commented Jun 10, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added theme/api Relating to the HTTP API interface theme/config Relating to Consul Agent configuration, including reloading theme/envoy/xds Related to Envoy support labels Jun 10, 2022
@Amier3
Copy link
Contributor

Amier3 commented Jun 13, 2022

Hey @davide-baldo

Really excited to see this come through, as you can see in #8283 this would help a lot of people. We'll try to get some feedback on this shortly. Since this is a draft it might help us to know what you think is left on the TODO list?

@davide-baldo
Copy link
Author

Hi @Amier3,

Unfortunately the config guide doesn't cover the service-defaults and apply only partially (I've added websocket attribute in the exposed paths in proxy configuration as well).

CLI and struct changes and envoy generation should be complete AFAIK.

  • Tests
  • PR Changelog
  • Docs
    • Update docs in website/source/docs/agent/services.html.md
    • Consider if it's worth adding examples to feature docs or API docs
      that show the new field's usage.

@davide-baldo
Copy link
Author

just rebased against lastest main branch.
@Amier3 Could you give me some feedback on whether this is the right approach?
thanks,

@davide-baldo davide-baldo changed the title [DRAFT] Support websocket through envoy [DRAFT] [NEED FEEDBACK] Support websocket through envoy Aug 29, 2022
@simonctrlz
Copy link

@davide-baldo how do you deal with this requirement internally? do you maintain a fork with your patches?
Embarrassing show on Hashicorp's part on that front, really.

@davide-baldo
Copy link
Author

@simonctrlz
If you don't need any routing/filter you can proxy websocket as TCP, if you need more control you need to patch consul (and rebuild it, luckily in go it's really easy to compile/cross-compile) in order to generate the right envoy configuration.

the smallest patch you can apply is this: https://github.com/hashicorp/consul/pull/9639/files#diff-3f5a22a74e450c254308e27dccb905bdce627f0a29ec602ca66df792d1dbf5ce

		UpgradeConfigs: []*envoyhttp.HttpConnectionManager_UpgradeConfig{
			{
				UpgradeType: "websocket",
			},
		},

This PR is mainly about enabling/disabling websocket capabilities through service configuration.
You might also find a way to modify envoy configuration directly from the consul proxy configuration to avoid any consul patch, but it's undocumented I don't know how to do it, or even if it's possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/api Relating to the HTTP API interface theme/config Relating to Consul Agent configuration, including reloading theme/envoy/xds Related to Envoy support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants