NE-406: ingress: Add aws-elb-idle-timeout enhancement #461
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Miciah The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Miciah
force-pushed
the
add-aws-elb-idle-timeout-enhancement
branch
from
061987e
to
9a0f9ac
Compare
Miciah
changed the title
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci-robot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/reopen |
|
@Miciah: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci
bot
removed
the
lifecycle/rotten
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten enhancement proposals close after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Reopen the proposal by commenting /close |
|
@openshift-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/reopen |
|
@Miciah: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/remove-lifecycle rotten |
Miciah
force-pushed
the
add-aws-elb-idle-timeout-enhancement
branch
from
9a0f9ac
to
504a5c4
Compare
openshift-ci
bot
removed
the
lifecycle/rotten
Miciah
force-pushed
the
add-aws-elb-idle-timeout-enhancement
branch
from
504a5c4
to
14009aa
Compare
| Route annotation for the application's Route: | ||
|
|
||
| ```shell | ||
| oc patch -n my-project routes/my-route haproxy.router.openshift.io/timeout=5m |
There was a problem hiding this comment.
this is supposed to be the annotate command right? patch doesn't work like this.
| be added: | ||
|
|
||
| 1. Create an IngressController that specifies a short timeout (for example, 10 seconds). | ||
| 2. Create a pod with a simple HTTP application that sends static responses. |
There was a problem hiding this comment.
nit semantics I think you also need to specify it intentionally holds a connection open so it does indeed timeout, but I understand what you are getting at here.
There was a problem hiding this comment.
Good point, this test plan has problems. When I wrote this test plan way back when, I had in mind that the client would open the connection and wait before sending the request. I realize now that the client would reach the tcp-request inspect-delay timeout of 5 seconds with this approach. I've updated the test plan to say that the client sends a request and the server imposes a delay for the response.
| field of the corresponding IngressController to the desired value to restore the | ||
| configuration. Otherwise, the default ELB timeout period remains in effect | ||
| after an upgrade. Because modifying operator-managed resources is generally | ||
| unsupported, it should suffice to address this issue through a release note. |
| annotation on operator-managed Services. This would require less work to | ||
| implement. However, an explicit API can be more easily validated. | ||
| Additionally, allowing users to set the annotation would set a precedent of | ||
| allowing users to modify operator-managed resources, which we want to avoid. |
There was a problem hiding this comment.
I also add the idea of centralized configuration. In our current version, you'd turn on the classic load balance service via spec.endpointPublishingStrategy.loadBalancer.providerParameters.aws.classicLoadBalancer, but then users need to go tweak the timeout on a different object (service) that is created after you enable the classic load balancer. This enhancement seems more congruent to me. You configure it where you turn enable it.
There was a problem hiding this comment.
Thanks! Very good point. I've added it to the enhancement.
| annotation on operator-managed Services. This would require less work to | ||
| implement. However, an explicit API can be more easily validated. | ||
| Additionally, allowing users to set the annotation would set a precedent of | ||
| allowing users to modify operator-managed resources, which we want to avoid. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Miciah
force-pushed
the
add-aws-elb-idle-timeout-enhancement
branch
from
14009aa
to
9f0ffa7
Compare
|
Changes look good to me @Miciah thanks! |
Miciah
force-pushed
the
add-aws-elb-idle-timeout-enhancement
branch
from
9f0ffa7
to
3feb104
Compare
Miciah
force-pushed
the
add-aws-elb-idle-timeout-enhancement
branch
from
3feb104
to
b070b03
Compare
| IngressController's | ||
| `spec.endpointPublishingStrategy.loadBalancer.providerParameters.aws.classicLoadBalancer.connectionIdleTimeout` | ||
| field to `5m` or longer, and the project administrator must similarly set a | ||
| Route annotation for the application's Route: |
There was a problem hiding this comment.
If the connection idle timeout value is set and you forget (or omit) the route setting is there any feedback that the latter hasn't been done?
There was a problem hiding this comment.
No, there is no feedback. Application developers usually shouldn't need to worry about this sort of thing anyway; configuring timeouts on routes is an advanced use-case.
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frobware 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@Miciah: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Miciah
changed the title
This enhancement extends the IngressController API to allow the user to configure the timeout period for idle connections to an IngressController that is published using an AWS Classic Elastic Load Balancer.