Skip to content

upstream: support max connection duration for upstream HTTP connections#17932

Merged
alyssawilk merged 53 commits intoenvoyproxy:mainfrom
esmet:max-upstream-connection-duration
Oct 14, 2021
Merged

upstream: support max connection duration for upstream HTTP connections#17932
alyssawilk merged 53 commits intoenvoyproxy:mainfrom
esmet:max-upstream-connection-duration

Conversation

@esmet
Copy link
Contributor

@esmet esmet commented Aug 31, 2021 •
edited by alyssawilk
Loading

Commit Message: upstream: support max connection duration for upstream connections
Risk Level: low, isolated opt-in feature
Testing: new unit tests
Docs Changes: updated protobuf definition to no longer mention that max lifetime is not implemented for upstream connections
Release Notes: upstream: support max connection duration for upstream connections
Platform Specific Features:
Fixes #15107

esmet added 6 commits February 17, 2021 00:19
@esmet
conn_pool: support max stream duration for upstream connections
f143d0c 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Formatting
4e8e73d 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Only drain non-closed connections
9d7d810 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Pipe down maxConnectionDuration from CommonHttpProtocolOptions
0964e31 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Merge remote-tracking branch 'upstream/main' into max-upstream-connec…
7909f3f 
…tion-duration

Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Fix proto comments
301e2a0 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Aug 31, 2021

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #17932 was opened by esmet.

see: more, trace.

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Aug 31, 2021

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17932 was opened by esmet.

see: more, trace.

@esmet esmet mentioned this pull request Aug 31, 2021
Closed
esmet added 2 commits August 31, 2021 18:44
@esmet
Merge remote-tracking branch 'upstream/main' into max-upstream-connec…
4a776fe 
…tion-duration

Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Fix ClusterInfo to contain maxConnectionDuration
d61139e 
Signed-off-by: John Esmet <john.esmet@gmail.com>
htuch
htuch reviewed Sep 1, 2021
View reviewed changes
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.

/lgtm api

@repokitteh-read-only repokitteh-read-only bot removed the api label Sep 1, 2021
@htuch htuch removed their assignment Sep 1, 2021
@htuch
Copy link
Member

htuch commented Sep 1, 2021

/assign-from @envoyproxy/first-pass-reviewers

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Sep 1, 2021

@envoyproxy/first-pass-reviewers assignee is @jmarantz

🐱

Caused by: a #17932 (comment) was created by @htuch.

see: more, trace.

esmet added 4 commits September 1, 2021 22:36
@esmet
Add tests
8276bb6 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Fix
51ecc21 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Add back some bits
3abf8cd 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Fix changelog
e65ce1d 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet esmet marked this pull request as ready for review September 2, 2021 00:06
jmarantz
jmarantz reviewed Sep 2, 2021
View reviewed changes
Copy link
Contributor

@jmarantz jmarantz 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 but would like a test that pulls everything together with SImulatedTime.

esmet added 3 commits September 6, 2021 15:54
@esmet
Update max connection duration tests to use a real dispatcher and sim…
be1493b 
…ulated time.

Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Fix format
6934dff 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Add back two expects
7976234 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Copy link
Contributor Author

esmet commented Sep 7, 2021

I'm working on a fix for the ASAN failure in the new test.

@esmet
Remove the addition of closed_, since the lifetime of that bit is not…
e0b793a 
… reliable. Also, use a better source for host() for checking cluster stats.

Signed-off-by: John Esmet <john.esmet@gmail.com>
esmet added 3 commits October 6, 2021 21:33
@esmet
Add a basic integration test
0c64dcb 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Merge remote-tracking branch 'upstream/main' into max-upstream-connec…
c0f413d 
…tion-duration

Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Fix format
230a177 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Copy link
Contributor Author

esmet commented Oct 7, 2021 •
edited
Loading

Unfortunately looks like this needs a main merge which should hopefully fix both the conflict and the coverage flake.

Yep and I still need to address your comment about adding an integration test

@alyssawilk done. I added an integration test to verify the basic flow.

@esmet
Remove unused boilerplate
926989c 
Signed-off-by: John Esmet <john.esmet@gmail.com>
alyssawilk
alyssawilk previously approved these changes Oct 7, 2021
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Awesome! I think unfortunately you need one more main merge (release notes are conflict prone just after the release)

esmet added 2 commits October 7, 2021 12:07
@esmet
Merge remote-tracking branch 'upstream/main' into max-upstream-connec…
8b99184 
…tion-duration

Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Fix changelog merge
5b23fb5 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Copy link
Contributor Author

esmet commented Oct 7, 2021

@alyssawilk done!

alyssawilk
alyssawilk previously approved these changes Oct 11, 2021
View reviewed changes
@alyssawilk
Copy link
Contributor

alyssawilk commented Oct 11, 2021

/assign-from @envoyproxy/api-shepherds for comment update

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Oct 11, 2021

@envoyproxy/api-shepherds assignee is @lizan
for assignee is @None
comment assignee is @None
update assignee is @None

🐱

Caused by: a #17932 (comment) was created by @alyssawilk.

see: more, trace.

@alyssawilk
Copy link
Contributor

alyssawilk commented Oct 12, 2021

sorry about the lag: Lizan was running EnvoyCon yesterday and may be behind today due to in person con stuff as well
@markdroth could you take a look instead?

@markdroth
Copy link
Contributor

markdroth commented Oct 13, 2021

/lgtm api

Please change the PR title and the commit message to say "max connection duration" instead of "max stream duration". Thanks!

@alyssawilk alyssawilk changed the title upstream: support max stream duration for upstream connections upstream: support max connection duration for upstream connections Oct 13, 2021
@alyssawilk alyssawilk changed the title upstream: support max connection duration for upstream connections upstream: support max connection duration for upstream HTTP connections Oct 13, 2021
@alyssawilk
Copy link
Contributor

alyssawilk commented Oct 13, 2021

Super sorry but this now needs a main merge because of version history conflicts :-/

esmet added 2 commits October 13, 2021 13:51
@esmet
Merge remote-tracking branch 'upstream/main' into max-upstream-connec…
6474914 
…tion-duration

Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Fix changelog
ca9fa76 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Merge remote-tracking branch 'upstream/main' into max-upstream-connec…
2bbef6c 
…tion-duration

Signed-off-by: John Esmet <john.esmet@gmail.com>
@alyssawilk alyssawilk merged commit 9a4a861 into envoyproxy:main Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmarantz jmarantz jmarantz left review comments

+2 more reviewers

@htuch htuch htuch left review comments

@alyssawilk alyssawilk alyssawilk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lizan lizan

@jmarantz jmarantz

@adisuissa adisuissa

@alyssawilk alyssawilk

@markdroth markdroth

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Upstream max connection duration should be configurable.

7 participants

@esmet @htuch @jmarantz @alyssawilk @markdroth @lizan @adisuissa