Skip to content

Feature/timeouts#289

Merged
k8s-ci-robot merged 24 commits intokubernetes-sigs:mainfrom
Stevenjin8:feature/timeouts
Feb 6, 2026
Merged

Feature/timeouts#289
k8s-ci-robot merged 24 commits intokubernetes-sigs:mainfrom
Stevenjin8:feature/timeouts

Conversation

@Stevenjin8
Copy link
Contributor

depends on #288

What type of PR is this?

/kind feature
What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

adds best-effort translation for
* `nginx.ingress.kubernetes.io/proxy-connect-timeout`
* `nginx.ingress.kubernetes.io/proxy-send-timeout`
* `nginx.ingress.kubernetes.io/proxy-read-timeout`

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 5, 2026
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 5, 2026
Comment on lines 98 to 99
"ingress-nginx only supports TCP-level timeouts; i2gw has made a best-effort translation to Gateway API timeouts.request." +
" Please verify that this meets your needs. See documentation: https://gateway-api.sigs.k8s.io/guides/http-timeouts/",
Copy link

Choose a reason for hiding this comment

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

So essentially you're taking the highest timeout value out of connect/send/read and multiplying by 10 to use that as the full HTTP request timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but definitely open to any other suggestions.

Copy link

Choose a reason for hiding this comment

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

Since nginx doesn't natively support full HTTP request timeouts, there's really no great solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct

Copy link
Contributor

@mikemorris mikemorris Jan 15, 2026

Choose a reason for hiding this comment

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

@sjberman How did y'all implement the Gateway API timeouts in https://github.com/nginx/nginx-gateway-fabric? (Or have you not implemented timeouts due to this difficulty?)

Choose a reason for hiding this comment

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

We haven't implemented it yet.

Users can inject raw nginx config, so they can set the TCP level timeouts (and we'll probably expose these soon in our own Policy), but no support for the Gateway API timeouts.

Copy link
Contributor

@mikemorris mikemorris Jan 15, 2026

Choose a reason for hiding this comment

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

Reference material in https://gateway-api.sigs.k8s.io/geps/gep-1742/ and kubernetes-sigs/gateway-api#1741 might be helpful for suggesting anything we could do to get a closer approximation, @kflynn any ideas from your research a while back?

Choose a reason for hiding this comment

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

If the ultimate goal in Gateway API is to define a timeout from the point at which a client's request hits the gateway to the time that the gateway responds back to the client, that's just something that nginx won't be able to support today. Now, we may be able to work with the core nginx team to see if it can be built, or write our own custom nginx module, but that has yet to be prioritized.

The existing nginx timeouts just won't suffice for the desire of a true HTTP request timeout.

proxy_connect_timeout is the time that an upstream has to accept a connection
proxy_read_timeout is the time for reading a response from the upstream. It's set only between two successive read operations, not for the transmission of the whole response. So if a response comes back in parts, as long as each part is within that timeout, we'll read the response forever.
proxy_send_timeout is the time for transmitting a request to the upstream. Same rules as read timeout.

It's more likely that a client side connection would timeout first.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2026
@Stevenjin8 Stevenjin8 self-assigned this Jan 13, 2026
@mikemorris
Copy link
Contributor

Refs #270

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 28, 2026
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2026
@Stevenjin8 Stevenjin8 requested a review from kkk777-7 February 4, 2026 20:59
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2026
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kkk777-7, Stevenjin8

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2026
@kkk777-7
Copy link
Member

kkk777-7 commented Feb 5, 2026

LGTM, thanks!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2026
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2026
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2026
@kkk777-7
Copy link
Member

kkk777-7 commented Feb 6, 2026

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2026
@k8s-ci-robot k8s-ci-robot merged commit 2802595 into kubernetes-sigs:main Feb 6, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants