Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move serveContext.RequestTimeout into TimeoutConfig and drop old field #2690

Closed
skriss opened this issue Jul 15, 2020 · 9 comments · Fixed by #3005
Closed

move serveContext.RequestTimeout into TimeoutConfig and drop old field #2690

skriss opened this issue Jul 15, 2020 · 9 comments · Fixed by #3005
Assignees
Labels
area/httpproxy Issues or PRs related to the HTTPProxy API. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action.
Milestone

Comments

@skriss
Copy link
Member

skriss commented Jul 15, 2020

Spun out from #2225:

Once this work is complete, we will move the current request-timeout value in the configuration file into this stanza, and add request-timeout-max and request-timeout-min optional parameters that specify the bounds in which HTTPProxy or other objects may configure the timeout. This will be the pattern from now for all timeouts that have Contour-tunable defaults with a HTTPProxy override. This will be done under a separate issue.

@skriss skriss self-assigned this Jul 15, 2020
@skriss skriss added this to Unprioritized in Contour Project Board via automation Jul 15, 2020
@skriss skriss moved this from Unprioritized to Prioritized Backlog in Contour Project Board Jul 15, 2020
@youngnick youngnick added area/httpproxy Issues or PRs related to the HTTPProxy API. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jul 15, 2020
@skriss skriss moved this from Prioritized Backlog to In progress in Contour Project Board Jul 17, 2020
@skriss
Copy link
Member Author

skriss commented Jul 23, 2020

Note that per #2697 (comment), the new field will use the standard timeout syntax, which is slightly different from the existing syntax for RequestTimeout. Make sure to call this out in docs.

@jpeach
Copy link
Contributor

jpeach commented Jul 28, 2020

Did anyone investigate using an admission controller (e.g. gatekeeper) to enforce this policy? If we add policy controls for every field, configuration will get rapidly out of hand. It's also way better to give policy errors before a resource enters the cluster if possible.

@skriss
Copy link
Member Author

skriss commented Jul 29, 2020

Did anyone investigate using an admission controller (e.g. gatekeeper) to enforce this policy? If we add policy controls for every field, configuration will get rapidly out of hand. It's also way better to give policy errors before a resource enters the cluster if possible.

I did not; I was going off of @youngnick's spec. I can play around with gatekeeper if we're interested.

@stevesloka
Copy link
Member

I think you probably still need the validation checks in Contour even with an admission controller. Folks would have to buy into that configuration and it sort of feels like client-side validation where you'd still want the check on the backend.

@skriss
Copy link
Member Author

skriss commented Jul 30, 2020

There are also concerns that an operator who adds new limits has no built-in way of ensuring that they're not going to break existing proxies. It sounds like we need to take a step back here and think more holistically about the design for adding these types of controls. I'm going to close out the associated PR until we get to a consensus on the approach.

@youngnick
Copy link
Member

Let's talk about where we go from here and come back around about how we do min/max settings. I agree that my initial ideas about this are not sustainable; we need a more general solution for policy control over settings.

@skriss skriss moved this from In progress to Prioritized Backlog in Contour Project Board Aug 4, 2020
@skriss skriss changed the title move serveContext.RequestTimeout into TimeoutConfig and add min/max settings move serveContext.RequestTimeout into TimeoutConfig and drop old field Aug 24, 2020
@skriss
Copy link
Member Author

skriss commented Aug 24, 2020

The min/max limits will be implemented via optional Gatekeeper policies.

Leaving this open to track removing the old config file field after a few releases.

@skriss
Copy link
Member Author

skriss commented Sep 15, 2020

I'd like to remove the deprecated field in 1.10 - 3 minor releases after it was deprecated. That sound reasonable to everyone?

@youngnick
Copy link
Member

Yes, that's quite reasonable.

@stevesloka stevesloka added this to the 1.10.0 milestone Sep 16, 2020
@skriss skriss moved this from Prioritized Backlog to In progress in Contour Project Board Sep 30, 2020
skriss added a commit to skriss/contour that referenced this issue Oct 7, 2020
Removes the request-timeout field from the root of the config file. This
field has been replaced by timeouts.request-timeout, and the original
field has been deprectated for several releases now.

Closes projectcontour#2690.

Signed-off-by: Steve Kriss <krisss@vmware.com>
Contour Project Board automation moved this from In progress to 1.10 Release Oct 7, 2020
skriss added a commit that referenced this issue Oct 7, 2020
…#3005)

Removes the request-timeout field from the root of the config file. This
field has been replaced by timeouts.request-timeout, and the original
field has been deprectated for several releases now.

Closes #2690.

Signed-off-by: Steve Kriss <krisss@vmware.com>
@skriss skriss added release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/httpproxy Issues or PRs related to the HTTPProxy API. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action.
Projects
None yet
4 participants