Skip to content

lds: Add TCP_FASTOPEN listener option#539

Merged
htuch merged 1 commit intoenvoyproxy:masterfrom
bmetzdorf:tfo
Apr 5, 2018
Merged

lds: Add TCP_FASTOPEN listener option#539
htuch merged 1 commit intoenvoyproxy:masterfrom
bmetzdorf:tfo

Conversation

@bmetzdorf
Copy link
Copy Markdown
Contributor

Add TCP_FASTOPEN listener option

API changes for: envoyproxy/envoy#2793

Signed-off-by: Bjoern Metzdorf bmetzdorf@apple.com

mattklein123
mattklein123 previously approved these changes Mar 13, 2018
Copy link
Copy Markdown
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.

LGTM.

bool transparent = 10;

// Whether the listener should accept TCP Fast Open (TFO) connections.
// Supports Linux and OSX. Defaults to false.
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.

I'd drop the OS whitelist and only add a blacklist if needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will do.


// Whether the listener should accept TCP Fast Open (TFO) connections.
// Supports Linux and OSX. Defaults to false.
bool enable_tcp_fast_open = 12;
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.

Why is this 12 and not 11?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There's another unrelated PR pending that claims 11. But I can set this to 11 for now and let the faster PR win :)

@bmetzdorf
Copy link
Copy Markdown
Contributor Author

@htuch @mattklein123 @ggreenway

How do you feel about the naming convention for these listener options? Some are use_ prefixed, some have no prefix, and this one is enable_. I chose enable_ to indicate it is a bool, but I don't feel strongly about it at all.

@mattklein123
Copy link
Copy Markdown
Member

Optimally we should have option names be consistent, but I don't feel very strongly about which way. Will defer to others and @wora on this.

@htuch
Copy link
Copy Markdown
Member

htuch commented Mar 14, 2018

Wouldn't accept_tcp_fast_open be most descriptive in this case? </bike shed>

@rlenglet
Copy link
Copy Markdown
Contributor

rlenglet commented Mar 16, 2018

FYI, based on the discussions in #522 and envoyproxy/envoy#2734, we're going to use google.protobuf.BoolValue instead of bool for listen socket options: #558.

@bmetzdorf
Copy link
Copy Markdown
Contributor Author

@htuch I like accept_ better. Good idea, updated!

I'll also wait for the generic setOptions framework to be merged before I update envoyproxy/envoy#2793

@mattklein123
Copy link
Copy Markdown
Member

LGTM. Let's hold on merging this until the Envoy side change merges. This might not make 1.6.0.

@htuch
Copy link
Copy Markdown
Member

htuch commented Apr 4, 2018

We need to merge this before the Envoy-side merges. I think it's fine to go ahead for now, the upstream PR should merge soon.

1.7.0
=====

* listeners: added :ref:`tcp_fast_open_queue_length <envoy_api_field_Listener.tcp_fast_open_queue_length>` 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.

Let's split this into two PRs. One without release notes, just the proto changes, and another to followup post-merge.

// Whether the listener should accept TCP Fast Open (TFO) connections.
// When this flag is set to a value greater than 0, the option TCP_FASTOPEN is enabled on
// the socket, with a queue length of the specified size
// (see https://tools.ietf.org/html/rfc7413#section-5.1).
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.

Use Sphinx RST links.

// When this flag is not set (default), the socket is not modified,
// i.e. the option is neither enabled nor disabled.
//
// On linux, the net.ipv4.tcp_fastopen kernel parameter must include flag 0x2 to enable
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: Linux and MacOS below, thanks.

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.

macOS is supposed to have a lower-case m.

@bmetzdorf bmetzdorf force-pushed the tfo branch 2 times, most recently from 5534c90 to e0f6f83 Compare April 4, 2018 23:15
Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>
@bmetzdorf
Copy link
Copy Markdown
Contributor Author

@htuch Thanks for the review! I have addressed your comments (besides macOS, which is the actual marketing name instead of MacOS).

@bmetzdorf
Copy link
Copy Markdown
Contributor Author

(And sorry for squashing. I was not aware of the policy here yet.)

@htuch htuch merged commit f9f5c41 into envoyproxy:master Apr 5, 2018
htuch pushed a commit to envoyproxy/envoy that referenced this pull request Apr 18, 2018
This allows accepting TCP_FASTOPEN based connections. Works on both Linux and OSX.

API Changes: envoyproxy/data-plane-api#539
Risk Level: Low

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.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.

5 participants