Skip to content

lds: Make listen socket options optional and improve docs#558

Merged
ggreenway merged 5 commits intoenvoyproxy:masterfrom
rlenglet:make-listen-socket-options-optional
Mar 19, 2018
Merged

lds: Make listen socket options optional and improve docs#558
ggreenway merged 5 commits intoenvoyproxy:masterfrom
rlenglet:make-listen-socket-options-optional

Conversation

@rlenglet
Copy link
Copy Markdown
Contributor

@rlenglet rlenglet commented Mar 16, 2018

Redefine the transparent and freebind listener options using
google.protobuf.BoolValue instead of bool.

Document the behavior when each option is set to true, set to false,
or unset.
Document that the transparent option should be used in conjunction
the original_dst_filter to restore the original destination address.

Mention the transparent option in version history.

Signed-off-by: Romain Lenglet romain@covalent.io

Document the behavior when each option is set to true, set to false,
or unset.
Document that the transparent option should be used in conjunction
the original_dst_filter to restore the original destination address.

Signed-off-by: Romain Lenglet <romain@covalent.io>
Signed-off-by: Romain Lenglet <romain@covalent.io>
rlenglet referenced this pull request Mar 16, 2018
…#522)

Add a "transparent" option to Listener to set the SOL_IP/IP_TRANSPARENT option on listen sockets, which allows using Envoy with the iptables TPROXY target.
Unlike the iptables REDIRECT target, TPROXY allows preserving both the source and destination IP addresses and ports of accepted connections.

API changes for: envoyproxy/envoy#2719

Signed-off-by: Romain Lenglet <romain@covalent.io>
ggreenway
ggreenway previously approved these changes Mar 16, 2018
Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

Signed-off-by: Romain Lenglet <romain@covalent.io>
@rlenglet rlenglet changed the title lds: Make listen socket options and improve docs lds: Make listen socket options optional and improve docs Mar 16, 2018
@rlenglet
Copy link
Copy Markdown
Contributor Author

@ggreenway please review again. The checks are passing now.

@jrajahalme
Copy link
Copy Markdown
Contributor

I'd clarify that:
"The transparent option should be used in conjunction
the original_dst_filter to mark the destination address as 'restored' so that the original destination cluster will not block opening the upstream connection."

@jrajahalme
Copy link
Copy Markdown
Contributor

Also, I'd prefer merging this before envoyproxy/envoy#2734.

@rlenglet
Copy link
Copy Markdown
Contributor Author

I'd clarify that:
"The transparent option should be used in conjunction
the original_dst_filter to mark the destination address as 'restored' so that the original destination cluster will not block opening the upstream connection."

That's less clear to me. The comment I added about original_dst_filter was nearly copied-and-pasted from the comment on the use_original_dst field.

Signed-off-by: Romain Lenglet <romain@covalent.io>
@rlenglet
Copy link
Copy Markdown
Contributor Author

rlenglet commented Mar 17, 2018

Clarified the behavior of "restored" addresses for TPROXYed connections, to address @jrajahalme's comment.

mattklein123
mattklein123 previously approved these changes Mar 17, 2018
@ggreenway
Copy link
Copy Markdown
Member

@rlenglet There's a merge conflict :(

@rlenglet
Copy link
Copy Markdown
Contributor Author

rlenglet commented Mar 19, 2018

@ggreenway Merged from master. Sorry for the delay.

* tracing: when using the zipkin tracer, it is no longer necessary to propagate the
:ref:`x-ot-span-context <config_http_conn_man_headers_x-ot-span-context>` header.
See more on trace context propagation :ref:`here <arch_overview_tracing>`.
* listeners: added :ref:`transparent <envoy_api_field_Listener.transparent>` option.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: in the future please alpha order the functional area ("listeners"). Don't worry about it for now though. I will fix in the larger release note merge I am doing.

@rlenglet
Copy link
Copy Markdown
Contributor Author

@ggreenway @mattklein123 Could you please re-approve? Your approvals get dismissed after I merged master into my branch.

@ggreenway ggreenway merged commit 24c90e9 into envoyproxy:master Mar 19, 2018
mattklein123 added a commit that referenced this pull request Mar 20, 2018
…)"

This reverts commit 24c90e9.

Signed-off-by: Matt Klein <mklein@lyft.com>
htuch added a commit to htuch/envoy-api that referenced this pull request Mar 27, 2018
This follows up from envoyproxy#558 which made IP_FREEBIND a BoolValue for LDS but
not for upstream. I think it makes sense to have it in both places given
the new socket options setup introduce in
envoyproxy/envoy#2734.

Some bonus docs fixups thrown in.

Signed-off-by: Harvey Tuch <htuch@google.com>
htuch added a commit that referenced this pull request Mar 27, 2018
This follows up from #558 which made IP_FREEBIND a BoolValue for LDS but
not for upstream. I think it makes sense to have it in both places given
the new socket options setup introduce in
envoyproxy/envoy#2734.

Some bonus docs fixups thrown in.

Signed-off-by: Harvey Tuch <htuch@google.com>
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