search

openshift / oauth-proxy

A reverse proxy that provides authentication with OpenShift via OAuth and Kubernetes service accounts
MIT License
261 stars 136 forks source link

APPSRE-7970: Add support for configuring upstream timeout #258

Closed kwilczynski closed 1 year ago

kwilczynski commented 1 year ago

Currently, there is no support for configuring the timeout between the proxy and the backends, and as such, the default timeout of 30 seconds is applied (as per Go's standard library defaults). When the timeout occurs, the user will see an "http: proxy error: context canceled" in the logs, and the remote client will receive a 502 error.

The default timeout might not be sufficient for some services, especially as every upstream service differs, and the abrupt connection termination might not be desirable. Typically, most upstream services only send response headers after they have gathered/processed all necessary data.

Thus, add a new command-line switch called --upstream-timeout, allowing users to configure the timeout setting.

Note: this change is, almost entirely, a backport of a change that was added to the upstream OAuth2 Proxy project some time ago, per:

Signed-off-by: Krzysztof Wilczyński kwilczynski@redhat.com

openshift-ci-robot commented 1 year ago

@kwilczynski: This pull request references APPSRE-7970 which is a valid jira issue.

In response to [this](https://github.com/openshift/oauth-proxy/pull/258): > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
openshift-ci[bot] commented 1 year ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

kwilczynski commented 1 year ago

/test all

kwilczynski commented 1 year ago

/retest

kwilczynski commented 1 year ago

To the reviewer:

The currently failing test seems to be failing for reasons that aren't related to the changes here.

Update:

Tests seem flaky. Things eventually passed.

openshift-ci-robot commented 1 year ago

@kwilczynski: This pull request references APPSRE-7970 which is a valid jira issue.

In response to [this](https://github.com/openshift/oauth-proxy/pull/258): >Currently, there is no support for configuring the timeout between the proxy and the backends, and as such, the default timeout of **30 seconds** is applied (as per Go's standard library defaults). When the timeout occurs, the user will see an "http: proxy error: context canceled" in the logs, and the remote client will receive a **502** error. > >The default timeout might not be sufficient for some services, especially as every upstream service differs, and the abrupt connection termination might not be desirable. Typically, most upstream services only send response headers after they have gathered/processed all necessary data. > >Thus, add a new command-line switch called `--upstream-timeout`, allowing users to configure the timeout setting. > >Related: >- [APPSRE-7875](https://issues.redhat.com/browse/APPSRE-7875) >- [APPSRE-7970](https://issues.redhat.com/browse/APPSRE-7970) > >Note: this change is, almost entirely, a backport of a change that was added to the upstream [OAuth2 Proxy](https://github.com/oauth2-proxy/oauth2-proxy) project some time ago, per: > >- PR#1638: [Configure upstream timeout](https://github.com/oauth2-proxy/oauth2-proxy/pull/1638) > >Signed-off-by: Krzysztof Wilczyński Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
kwilczynski commented 1 year ago

@stlaz, this should be ready for review. Thank you!

kwilczynski commented 1 year ago

/retest-required

kwilczynski commented 1 year ago

Hi @sttts, @stlaz and @deads2k,

Would any of you have a moment to look at this change? I would appreciate it.

kwilczynski commented 1 year ago

I had a brief chat with @stlaz, who requested several changes:

This will be done in due course.

openshift-ci-robot commented 1 year ago

@kwilczynski: This pull request references APPSRE-7970 which is a valid jira issue.

In response to [this](https://github.com/openshift/oauth-proxy/pull/258): >Currently, there is no support for configuring the timeout between the proxy and the backends, and as such, the default timeout of **30 seconds** is applied (as per Go's standard library defaults). When the timeout occurs, the user will see an "http: proxy error: context canceled" in the logs, and the remote client will receive a **502** error. > >The default timeout might not be sufficient for some services, especially as every upstream service differs, and the abrupt connection termination might not be desirable. Typically, most upstream services only send response headers after they have gathered/processed all necessary data. > >Thus, add a new command-line switch called `--upstream-timeout`, allowing users to configure the timeout setting. > >Note: this change is, almost entirely, a backport of a change that was added to the upstream [OAuth2 Proxy](https://github.com/oauth2-proxy/oauth2-proxy) project some time ago, per: > >- PR#1638: [Configure upstream timeout](https://github.com/oauth2-proxy/oauth2-proxy/pull/1638) > >Signed-off-by: Krzysztof Wilczyński Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
kwilczynski commented 1 year ago

/retest-required

kwilczynski commented 1 year ago

/test e2e-aws

openshift-ci[bot] commented 1 year ago

@kwilczynski: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
stlaz commented 1 year ago

/lgtm

openshift-ci[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kwilczynski, stlaz

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

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openshift/oauth-proxy/blob/master/OWNERS)~~ [stlaz] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment