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

docs: Can I use Envoy as a front-end proxy? #8001

Merged
merged 22 commits into from
Oct 31, 2019

Conversation

PiotrSikora
Copy link
Contributor

Signed-off-by: Piotr Sikora piotrsikora@google.com

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Contributor Author

cc @mattklein123 @yanavlasov @htuch

docs/root/faq/edge.rst Outdated Show resolved Hide resolved
Copy link
Member

@mattklein123 mattklein123 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 comments to get started. Thanks for excited to see us do this.

/wait

docs/root/faq/edge.rst Outdated Show resolved Hide resolved
docs/root/faq/edge.rst Outdated Show resolved Hide resolved
docs/root/faq/edge.rst Outdated Show resolved Hide resolved
docs/root/faq/edge.rst Outdated Show resolved Hide resolved
docs/root/faq/edge.rst Outdated Show resolved Hide resolved
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Great. I think we can land something pretty soon and iterate. Thanks @PiotrSikora !

docs/root/faq/edge.rst Outdated Show resolved Hide resolved
docs/root/faq/edge.rst Outdated Show resolved Hide resolved
docs/root/faq/edge.rst Outdated Show resolved Hide resolved
docs/root/faq/edge.rst Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Aug 30, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 30, 2019
@mattklein123 mattklein123 added the no stalebot Disables stalebot from closing an issue label Aug 30, 2019
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 30, 2019
@mattklein123
Copy link
Member

@PiotrSikora I would really love to land this and iterate given the number of questions we get on this topic. Do you think you will have time to action on the comments? If not I can potentially take this over next week.

Copy link
Contributor

@yanavlasov yanavlasov left a comment

Choose a reason for hiding this comment

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

+1 on explaining the need for the use_remote_address setting.

@mattklein123
Copy link
Member

@PiotrSikora friendly ping. I would like to land this for 1.12.0 by the end of the month. Will you be able to finish this out? If not I can take it over.

@@ -0,0 +1,85 @@
.. _faq_edge:
Copy link
Member

Choose a reason for hiding this comment

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

Should we treat #2763 as well? I feel folks need some guidance on "best practices for secure configuration", and even though we warn about locking down the admin endpoint elsewhere, having this in one place would make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Protecting /admin endpoint is not exclusive to front-end proxies, so I think that it should be documented in a separate entry, and linked from here when it's ready.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but in the interim can we at least show an admin config that only listens on localhost with a small comment?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, please do, and at least say a few sentences about this. This isn't one of the places we're aiming for minimalism and zero redundancy; it's a really major security hole if misconfigured and operators should be reminded about this pretty much everywhere that there is discussion of security + Envoy.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
address: 127.0.0.1
port_value: 8080
http2_protocol_options:
max_concurrent_streams: 100
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that we want this for upstream. IMO the default is fine and have circuit breakers to limit this on a per-cluster basis as well as the downstream limits.

Copy link
Member

Choose a reason for hiding this comment

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

What is the scope here? Do we want to over edge use cases where Envoy is acting as an egress gateway and speaking to untrusted backends? In this situation, we also need to be defensive on the upstream.

Copy link
Member

Choose a reason for hiding this comment

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

We don't support push, so this provides no protection against the upstream. IIRC it might also limit the number of outgoing streams, but that is already controlled by downstream limits and circuit breakers.

Copy link
Member

Choose a reason for hiding this comment

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

Right. I guess I'd suggest that we do configure all the other defensive limits that we have, or at least provide some nuanced discussion on trust model here. In fact, I think there should be a short section on trust model. By edge, are we really saying "edge ingress"? Because, for "edge egress", we know today that there are some missing features that came up during the HTTP/2 headers CVE handling for example, that are needed when dealing with untrusted upstreams.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, let's be explicit and also call out a warning against untrusted upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you want to see here? It's unclear to me from the comments.

Copy link
Member

Choose a reason for hiding this comment

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

I would delete this line. I don't think it does anything useful.

@@ -0,0 +1,85 @@
.. _faq_edge:
Copy link
Member

Choose a reason for hiding this comment

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

I agree, but in the interim can we at least show an admin config that only listens on localhost with a small comment?

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM with some small comments. Let's ship and iterate after this.

I know that @alyssawilk has some additional thoughts on recommendations but we can add those later.

/wait

docs/root/faq/configuration/edge.rst Outdated Show resolved Hide resolved
docs/root/configuration/best_practices/edge.rst Outdated Show resolved Hide resolved
address: 127.0.0.1
port_value: 8080
http2_protocol_options:
max_concurrent_streams: 100
Copy link
Member

Choose a reason for hiding this comment

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

I would delete this line. I don't think it does anything useful.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Contributor Author

@mattklein123 I believe all comments were addressed, in case you still want to ship this in 1.12.0.

* :ref:`connection and stream timeouts <faq_configuration_timeouts>`,
* :ref:`HTTP/2 maximum concurrent streams limit <envoy_api_field_core.Http2ProtocolOptions.max_concurrent_streams>` to 100,
* :ref:`HTTP/2 initial stream window size limit <envoy_api_field_core.Http2ProtocolOptions.initial_stream_window_size>` to 64 KiB,
* :ref:`HTTP/2 initial connection window size limit <envoy_api_field_core.Http2ProtocolOptions.initial_connection_window_size>` to 1 MiB.
Copy link
Member

Choose a reason for hiding this comment

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

Can you comment on the relative relationship between connection and stream window default sizes? Is there an idea l ratio?

for details),
* :ref:`connection and stream timeouts <faq_configuration_timeouts>`,
* :ref:`HTTP/2 maximum concurrent streams limit <envoy_api_field_core.Http2ProtocolOptions.max_concurrent_streams>` to 100,
* :ref:`HTTP/2 initial stream window size limit <envoy_api_field_core.Http2ProtocolOptions.initial_stream_window_size>` to 64 KiB,
Copy link
Member

Choose a reason for hiding this comment

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

How much difference does it make if your perimeter on the public Internet vs. Cloud? Do we need to consider bandwidth-delay product?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't really matter, it's here to disable Envoy's default which is 256 MiB (HTTP/2 default is 65535 B).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but the meta point is that this document currently lacks the conveying of this intuition. These docs need to help educate the reader about what is important, why we do things the way we do them, not only to provide some suggested config settings (although the latter is useful as a start).

* :ref:`use_remote_address <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.use_remote_address>`
to true (to avoid consuming HTTP headers from external clients, see :ref:`HTTP header sanitizing <config_http_conn_man_header_sanitizing>`
for details),
* :ref:`connection and stream timeouts <faq_configuration_timeouts>`,
Copy link
Member

Choose a reason for hiding this comment

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

What is the guidance here? Is the idea that "anything, as long as it's not effectively infinite" is fine? Which are the timeouts that need to be added, which are optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The linked FAQ has a few pages about timeouts, so I didn't see a point in repeating it all that.

Copy link
Member

Choose a reason for hiding this comment

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

I think even a short sentence "see the FAQ at for details on how to configure" is a useful addition. I agree we don't want redundancy and we want a single source of truth, but we can also try and guide the reader.

In general, I think it's instructive to put oneself in the shoes of the average reader. They arrive at this page with the following thought: Envoy timeouts, limits, configuration knobs, WTF ¯\_(ツ)_/¯

Our goal here is not just to tell them truthful and correct things, but to help demystify and have them leave with a feeling that they fully grok the mystery.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

This LGTM as a first cut, thanks. I think @htuch comments can be looked as follow ups but will defer to him on further review and merge.

@htuch
Copy link
Member

htuch commented Oct 31, 2019

If you want to merge for 1.12.0, this is OK. I would appreciate followup on my comments. To me, I think we still have a ton of magic in how to configure Envoy for the edge, but this doc is a useful beach head and starting point.

@mattklein123
Copy link
Member

To me, I think we still have a ton of magic in how to configure Envoy for the edge, but this doc is a useful beach head and starting point.

I agree completely, but I would rather ship and iterate on this as this certainly puts us in a much better place. @htuch will defer to you to approve/merge if you want.

@mattklein123
Copy link
Member

Let's merge this to get this into the 1.12.0 docs and we can iterate from here.

@mattklein123 mattklein123 merged commit 6a5e23c into envoyproxy:master Oct 31, 2019
junr03 pushed a commit that referenced this pull request Nov 20, 2019
Description: use_proxy_proto is part of the listener.FilterChain (not HttpConnectionManager)
Risk Level: None
Testing: Manually tested with envoyproxy/envoy-alpine:v1.12.1 (Docker image)
Docs Changes: see description
Release Notes: docs: corrected use of use_proxy_proto in edge-proxy best-practice example

Fix for #8001

Signed-off-by: Dominik <dominik-k@mailbox.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants