Skip to content

[http] add upstreamSocketOptions and hook them into http connection pool creation#6804

Merged
mattklein123 merged 10 commits intoenvoyproxy:masterfrom
klarose:upstream_options
May 8, 2019
Merged

[http] add upstreamSocketOptions and hook them into http connection pool creation#6804
mattklein123 merged 10 commits intoenvoyproxy:masterfrom
klarose:upstream_options

Conversation

@klarose
Copy link
Copy Markdown
Contributor

@klarose klarose commented May 3, 2019

Description: Add the ability to push sock options into the upstream connection pool from an http filter.

In order to create an HTTP filter to set the source address of an upstream connection based on the downstreamRemoteAddress of a stream, we need to be able to communicate to the connection pool creating the upstream connection the IP address we want to set. We have existing infrastructure to do this using socket options, but no way to pass socket options to the upstream on a per-http-request basis.

This PR adds methods to the the StreamDecoderFilterCallbacks so that an HTTP filter can add socket options to the callbacks. It extends the LoadBalancerContext to expose the desired upstream socket options to the connection pool logic.

For the HTTP case, the socket options flow from the callback (which is an ActiveStreamDecoderFilter) to its ActiveStream parent. By having the parent track the socket options, every HTTP filter may add to or get the options previously added. In particular, the HTTP filter has access to them. Since it's also the LoadBalancerContext for HTTP connection pools, this completes the circle.

The PR also uses the LoadBalancerContext to stitch the upstreamSocketOptions together with the downstream connection options to build the full set of options to set on the upstream.

This work was split out from #6790.

Risk Level: Medium
Testing: UT + Manual
Docs Changes: None
Release Notes: None

klarose added 5 commits May 3, 2019 16:25
We need these to configure per-request socket options from the http
filters.

Signed-off-by: Kyle Larose <kyle@agilicus.com>
We use the existing mechanism used to pass socket options to the
connection pool creation logic to pass the upstream socket options. We
do this by creating a new socket options object, then placing both sets
of socket options into it.

Signed-off-by: Kyle Larose <kyle@agilicus.com>
The active stream is what's shared between the various filters. Push the
options up to it so they're actually shared!

Signed-off-by: Kyle Larose <kyle@agilicus.com>
Signed-off-by: Kyle Larose <kyle@agilicus.com>
Fix from earlier review

Signed-off-by: Kyle Larose <kyle@agilicus.com>
@klarose
Copy link
Copy Markdown
Contributor Author

klarose commented May 3, 2019

@snowp As discussed, here's the PR for the router changes.

Copy link
Copy Markdown
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, looks pretty good.

 - Clarify Comments
 - Fix grammar
 - Fix style

Signed-off-by: Kyle Larose <kyle@agilicus.com>
@klarose
Copy link
Copy Markdown
Contributor Author

klarose commented May 6, 2019

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🔨 rebuilding ci/circleci: docker (failed build)

🐱

Caused by: a #6804 (comment) was created by @klarose.

see: more, trace.

Signed-off-by: Kyle Larose <kyle@agilicus.com>
snowp
snowp previously approved these changes May 7, 2019
Copy link
Copy Markdown
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.

Nice, this looks good to me!

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.

Thanks this looks great. 1 question/comment.

/wait

Remove mention of downstream connections from http filter.

Signed-off-by: Kyle Larose <kyle@agilicus.com>
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.

Thanks. Can you merge master to bring in CI fixes?

Signed-off-by: Kyle Larose <kyle@agilicus.com>
The DCO is stuck :(. Try forcing it.

Signed-off-by: Kyle Larose <kyle@agilicus.com>
@mattklein123 mattklein123 merged commit 57721ad into envoyproxy:master May 8, 2019
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.

3 participants