Skip to content

api: Support connection timeouts in BackendTrafficPolicy#2411

Merged
zirain merged 5 commits intoenvoyproxy:mainfrom
guydc:upstream-timeouts
Jan 16, 2024
Merged

api: Support connection timeouts in BackendTrafficPolicy#2411
zirain merged 5 commits intoenvoyproxy:mainfrom
guydc:upstream-timeouts

Conversation

@guydc
Copy link
Copy Markdown
Contributor

@guydc guydc commented Jan 5, 2024

What this PR does / why we need it:
Allow users to configure various upstream timeout options.

Which issue(s) this PR fixes:
Fixes #2401, relates to #1821

@guydc guydc requested a review from a team as a code owner January 5, 2024 21:00
@guydc guydc force-pushed the upstream-timeouts branch from 0f08873 to 2f65997 Compare January 5, 2024 21:06
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c9d5e33) 64.65% compared to head (d7fd784) 64.65%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2411   +/-   ##
=======================================
  Coverage   64.65%   64.65%           
=======================================
  Files         115      115           
  Lines       17364    17364           
=======================================
  Hits        11227    11227           
  Misses       5425     5425           
  Partials      712      712           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

prefer timeout here, we've used singular terms in all fields in this API.
Also connection prefix is not needed imo

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we create top level tcp and http fields here ?

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.

Seeing as there are several other HTTP (stream, ...) and TCP (proxy, protocol, ...) timeouts that could be relevant in the future, it makes sense to me.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sort imports please

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 this pass ci? let me double check it.

guydc added 2 commits January 8, 2024 13:15
Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
@guydc guydc force-pushed the upstream-timeouts branch from 194a083 to a0f04f4 Compare January 8, 2024 19:23
// Default: unlimited.
//
// +optional
ConnectionDuration *gwapiv1.Duration `json:"connectionDuration,omitempty"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maxConnectionDuration instead ?

// +optional
ConnectionIdleTimeout *gwapiv1.Duration `json:"connectionIdleTimeout,omitempty"`

// The maximum duration of an persistent HTTPTimeout connections.
Copy link
Copy Markdown
Contributor

@arkodg arkodg Jan 9, 2024

Choose a reason for hiding this comment

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

The maximum duration of an HTTP connection.

Copy link
Copy Markdown
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team and shawnh2 January 10, 2024 18:39
Copy link
Copy Markdown
Contributor

@tmsnan tmsnan left a comment

Choose a reason for hiding this comment

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

LGTM

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Jan 11, 2024

/retest

Copy link
Copy Markdown
Contributor

@shawnh2 shawnh2 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

@zirain zirain merged commit a8d34b6 into envoyproxy:main Jan 16, 2024
zesiar0 pushed a commit to zesiar0/gateway that referenced this pull request Jan 22, 2024
…2411)

* API: Support Connection Timeouts in BackendTrafficPolicy

Signed-off-by: Guy Daich <guy.daich@sap.com>

* code review fixes

Signed-off-by: Guy Daich <guy.daich@sap.com>

* code review fixes 2

Signed-off-by: Guy Daich <guy.daich@sap.com>

---------

Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: A3bz <zeng921359373@163.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.

Support upstream HTTP connection timeouts

5 participants