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

Fix negated allow chunked length field #4152

Merged

Conversation

sunjayBhatia
Copy link
Member

Takes out NewListenerConfig constructor, back to using a struct

Pulls out parts of that into pkg/config validation and new package we
can use for contourconfiguration crd helpers

Still TODO: we need to do a bigger refactor of how we wire up the xds
server, xdscache components, informers etc., leaving testing of the
contents of cmd/contour to that change

Fixes: #4140

Takes out NewListenerConfig constructor, back to using a struct

Pulls out parts of that into pkg/config validation and new package we
can use for contourconfiguration crd helpers

Still TODO: we need to do a bigger refactor of how we wire up the xds
server, xdscache components, informers etc., leaving testing of the
contents of cmd/contour to that change

Fixes: projectcontour#4140

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@sunjayBhatia sunjayBhatia requested a review from a team as a code owner November 2, 2021 23:35
@sunjayBhatia sunjayBhatia requested review from tsaarni and youngnick and removed request for a team November 2, 2021 23:35
@sunjayBhatia sunjayBhatia added the release-note/small A small change that needs one line of explanation in the release notes. label Nov 2, 2021
@sunjayBhatia sunjayBhatia added this to the 1.20.0 milestone Nov 2, 2021
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #4152 (2ec96bd) into main (6d78450) will increase coverage by 0.53%.
The diff coverage is 59.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4152      +/-   ##
==========================================
+ Coverage   73.16%   73.70%   +0.53%     
==========================================
  Files         113      114       +1     
  Lines        9884     9867      -17     
==========================================
+ Hits         7232     7272      +40     
+ Misses       2498     2441      -57     
  Partials      154      154              
Impacted Files Coverage Δ
cmd/contour/serve.go 12.44% <0.00%> (-0.47%) ⬇️
internal/contourconfig/contourconfiguration.go 100.00% <100.00%> (ø)
internal/xdscache/v3/listener.go 90.53% <100.00%> (+21.13%) ⬆️
pkg/config/parameters.go 92.03% <100.00%> (+0.26%) ⬆️
internal/sorter/sorter.go 98.18% <0.00%> (+0.60%) ⬆️

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple non-blocking comments.

cmd/contour/serve.go Outdated Show resolved Hide resolved
cmd/contour/serve.go Outdated Show resolved Hide resolved
internal/contourconfiguration/contourconfiguration.go Outdated Show resolved Hide resolved
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
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, I like the helper refactor, particularly having the Timeouts in their own struct reads nicely.

internal/contourconfiguration/contourconfiguration.go Outdated Show resolved Hide resolved
@skriss
Copy link
Member

skriss commented Nov 9, 2021

has some conflicts to resolve before merge.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@sunjayBhatia sunjayBhatia merged commit e612be9 into projectcontour:main Nov 9, 2021
@sunjayBhatia sunjayBhatia deleted the fix-allow-chunked-length branch November 9, 2021 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contour config disableAllowChunkedLength setting not working as expected.
3 participants