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.

Sign up for GitHub

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

Jump to bottom

internal/timeout: return parse errors instead of defaulting to infinity #2905

Merged
merged 2 commits into from
Sep 16, 2020
Merged

internal/timeout: return parse errors instead of defaulting to infinity #2905

merged 2 commits into from
Sep 16, 2020

Conversation

skriss
Copy link
Member

@skriss skriss commented Sep 11, 2020

Returns errors when parsing timeout strings so they can be reported to
the user, either as invalid HTTPProxies, or in Contour logs. Previously,
such errors were swallowed and the timeout in question was disabled/set
to infinity.

Updates #2728

Signed-off-by: Steve Kriss krisss@vmware.com

@skriss skriss added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Sep 11, 2020
@skriss skriss force-pushed the timeout-parse-errors branch from 64af0d9 to 432c228 Compare September 11, 2020 22:41
@jpeach
Copy link
Contributor

jpeach commented Sep 11, 2020

Quick comments:

  • bumping the Go version should be in a separate PR
  • we should enforce the timeout syntax with a validation regex in the CRD
  • once we enforce validation, do we still need the parse errors?

@codecov
Copy link

codecov bot commented Sep 11, 2020 •
edited
Loading

Codecov Report

Merging #2905 into main will decrease coverage by 0.07%.
The diff coverage is 71.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2905      +/-   ##
==========================================
- Coverage   76.40%   76.32%   -0.08%     
==========================================
  Files          83       83              
  Lines        6298     6345      +47     
==========================================
+ Hits         4812     4843      +31     
- Misses       1392     1408      +16     
  Partials       94       94
Impacted Files Coverage Δ
cmd/contour/serve.go 2.21% <13.04%> (-0.12%) ⬇️
internal/dag/policy.go 94.44% <88.46%> (-1.62%) ⬇️
internal/annotation/annotations.go 100.00% <100.00%> (ø)
internal/dag/extension_processor.go 96.55% <100.00%> (+0.25%) ⬆️
internal/dag/httpproxy_processor.go 94.74% <100.00%> (+0.10%) ⬆️
internal/dag/ingress_processor.go 93.13% <100.00%> (+0.20%) ⬆️
internal/timeout/timeout.go 100.00% <100.00%> (ø)
internal/dag/cache.go 96.58% <0.00%> (+0.68%) ⬆️

@skriss
Copy link
Member Author

skriss commented Sep 11, 2020

Quick comments:

  • bumping the Go version should be in a separate PR

See #2906 :)

  • we should enforce the timeout syntax with a validation regex in the CRD

Yep, just haven't gotten to that part yet

  • once we enforce validation, do we still need the parse errors?

It's still an issue for (a) contour serve config file, and (b) Ingress annotations, so I think yes.

@skriss skriss force-pushed the timeout-parse-errors branch 2 times, most recently from dee45f7 to 435eca4 Compare September 15, 2020 00:34
youngnick
youngnick reviewed Sep 15, 2020
View reviewed changes
Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

I like the approach, looks good.

internal/featuretests/timeoutpolicy_test.go Show resolved Hide resolved
internal/dag/policy.go Show resolved Hide resolved
internal/featuretests/timeoutpolicy_test.go Show resolved Hide resolved
@skriss skriss force-pushed the timeout-parse-errors branch from 435eca4 to 1db0e33 Compare September 15, 2020 15:29
@skriss skriss requested a review from stevesloka September 15, 2020 15:35
@skriss skriss marked this pull request as ready for review September 15, 2020 15:36
@skriss
Copy link
Member Author

skriss commented Sep 15, 2020

rebased for auth work

stevesloka
stevesloka approved these changes Sep 15, 2020
View reviewed changes
Copy link
Member

@stevesloka stevesloka left a comment

Choose a reason for hiding this comment

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

/lgtm 🎉

@skriss
Copy link
Member Author

skriss commented Sep 15, 2020

I'll have to rebase this post #2900 as well

@youngnick youngnick self-requested a review September 16, 2020 08:07
youngnick
youngnick approved these changes Sep 16, 2020
View reviewed changes
Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM, with one small testing nit.

internal/dag/httpproxy_processor.go Show resolved Hide resolved
@skriss
internal/timeout: return parse errors instead of defaulting to infinity
5102ed1 
Returns errors when parsing timeout strings so they can be reported to
the user, either as invalid HTTPProxies, or in Contour logs. Previously,
such errors were swallowed and the timeout in question was disabled/set
to infinity.

Updates projectcontour#2728

Signed-off-by: Steve Kriss <krisss@vmware.com>
@skriss skriss force-pushed the timeout-parse-errors branch from 1db0e33 to 5102ed1 Compare September 16, 2020 14:52
@skriss
add test for invalid auth timeout
bf14fc2 
Signed-off-by: Steve Kriss <krisss@vmware.com>
@skriss skriss merged commit 301fb4e into projectcontour:main Sep 16, 2020
@skriss skriss deleted the timeout-parse-errors branch September 16, 2020 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youngnick youngnick youngnick approved these changes

@stevesloka stevesloka stevesloka approved these changes

Assignees
No one assigned
Labels
release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@skriss @jpeach @stevesloka @youngnick