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

http: adding CONNECT example configs and improving docs #11066

Merged
merged 5 commits into from
May 11, 2020

Conversation

alyssawilk
Copy link
Contributor

Risk Level: n/a
Testing: n/a
Docs Changes: connect docs improved
Release Notes: n/a
Part of #1451 and #1630

@@ -0,0 +1,38 @@
admin:
Copy link
Member

Choose a reason for hiding this comment

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

drive by: please make sure these new tests are added to the BUILD file so they get tested as part of config_test. Thank you!

- filters:
- name: tcp
typed_config:
"@type": type.googleapis.com/envoy.config.filter.network.tcp_proxy.v2.TcpProxy
Copy link
Contributor

Choose a reason for hiding this comment

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

Use v3? "@type": "type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy",

transport_socket:
name: envoy.transport_sockets.tls
typed_config:
"@type": type.googleapis.com/envoy.api.v2.auth.UpstreamTlsContext
Copy link
Contributor

Choose a reason for hiding this comment

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

Use v3 config? "@type": "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext"

alyssawilk added 3 commits May 5, 2020 13:33
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks this looks like a great addition! Few comments

.. proxy multiplexed TCP over pre-warmed secure connections and amortize the cost of any TLS handshake.
.. An example set up proxying SMTP would look something like this
..
.. [SMTP Upstream] --- raw STMP --- [L2 Envoy] --- SMTP tunneled over HTTP/2 --- [L1 Envoy] --- SMTP --- [Client]
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: raw STMP - > raw SMTP

@@ -168,6 +168,8 @@ uint32_t run(const std::string& directory) {
ENVOY_LOG_MISC(info, "testing {}.\n", filename);
OptionsImpl options(
Envoy::Server::createTestOptionsImpl(filename, "", Network::Address::IpVersion::v6));
// Try to avoid conflicting with other tests.
Copy link
Contributor

Choose a reason for hiding this comment

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

should we just disable hot restart instead? or do some of the tests rely on that?

@@ -0,0 +1,38 @@
admin:
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 have docs explaining how this feature works as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just what's in this PR. I think folks will mainly only use this for TCP tunneling - while there is a desire to have Envoy generate CONNECT requests AFIK it's only for HTTP/1.1, where for HTTP/2 the only planned use is tunneling TCP (by Google and folks who asked for #1630)

Signed-off-by: Alyssa Wilk <[email protected]>
@alyssawilk
Copy link
Contributor Author

Any other requests?

Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +171 to +172
// Avoid contention issues with other tests over the hot restart domain socket.
options.setHotRestartDisabled(true);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm @zuercher is this the same issue you are trying to fix?

Copy link
Member

Choose a reason for hiding this comment

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

See https://github.com/envoyproxy/envoy/pull/11016/files. I'm guessing this change can be reverted once the other PR lands.

@alyssawilk alyssawilk merged commit 57194f4 into envoyproxy:master May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants