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/dag: Implement TLSRoute mode:terminate #3801

Merged
merged 9 commits into from
Jun 28, 2021
Merged

internal/dag: Implement TLSRoute mode:terminate #3801

merged 9 commits into from
Jun 28, 2021

Conversation

stevesloka
Copy link
Member

Implements support for GatewayAPI TLSRoute mode: terminate which terminates TLS
at the Gateway.

Updates #3440

Signed-off-by: Steve Sloka slokas@vmware.com

@stevesloka stevesloka added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 11, 2021
@stevesloka stevesloka requested a review from a team as a code owner June 11, 2021 18:31
@stevesloka stevesloka requested review from danehans and sunjayBhatia and removed request for a team June 11, 2021 18:31
@codecov
Copy link

codecov bot commented Jun 11, 2021 •
edited
Loading

Codecov Report

Merging #3801 (582758a) into main (71c4cb8) will decrease coverage by 0.02%.
The diff coverage is 81.25%.

❗ Current head 582758a differs from pull request most recent head 9dee885. Consider uploading reports for the commit 9dee885 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3801      +/-   ##
==========================================
- Coverage   75.81%   75.78%   -0.03%     
==========================================
  Files         107      107              
  Lines        7340     7344       +4     
==========================================
+ Hits         5565     5566       +1     
- Misses       1654     1656       +2     
- Partials      121      122       +1
Impacted Files Coverage Δ
internal/dag/gatewayapi_processor.go 93.07% <81.25%> (-0.84%) ⬇️

sunjayBhatia
sunjayBhatia reviewed Jun 11, 2021
View reviewed changes
internal/dag/gatewayapi_processor.go Outdated
@@ -112,6 +112,14 @@ func (p *GatewayAPIProcessor) Run(dag *DAG, source *KubernetesCache) {
}
}
}
} else {
Copy link
Member

Choose a reason for hiding this comment

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

so this case now means a TLSRoute with no TLS field set defaults to terminate, though terminate without a secret doesn't really work? so can this just be an error now if you don't set TLS at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

/shrug The spec says that "Terminate" is the default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

is this intended to be the code for if the mode is nil? rather than the whole TLS block is nil?

sounds like the listener should be rejected if the TLS field is not set: https://github.com/kubernetes-sigs/gateway-api/blob/17a98f6f5aaa1e7fa607c1fc974efeb915b3337c/apis/v1alpha2/gateway_types.go#L184-L197

Copy link
Member Author

Choose a reason for hiding this comment

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

Essentially it is since the cert validation then fails on L118. Part of the problem with our unit tests is that we don't have the kubebuilder defaults applied. So we can create a test that doesn't specify the cert, but would get the mode applied by the api server and wouldn't pass the unit test.

Copy link
Member Author

@stevesloka stevesloka Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ok I rereead the spec and at the top level it does say "This field is required if the Protocol field is “HTTPS” or “TLS” and ignored otherwise.", so that make sense. I've updated.

skriss
skriss reviewed Jun 21, 2021
View reviewed changes
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.

one tiny nit, looking through tests next.

internal/dag/gatewayapi_processor.go Outdated Show resolved Hide resolved
skriss
skriss reviewed Jun 21, 2021
View reviewed changes
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.

One more small comment on the tests, otherwise looks good. Some of the tests are pretty tough to review but I skimmed and it looks reasonable/reasonable coverage.

test/e2e/gateway/008_tlsroute_test.go Outdated Show resolved Hide resolved
skriss
skriss approved these changes Jun 24, 2021
View reviewed changes
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.

no further comments, LGTM pending CI.

@skriss
Copy link
Member

skriss commented Jun 25, 2021

This one needs a rebase now that the E2E changes have gone in

@skriss skriss self-requested a review June 25, 2021 15:50
@stevesloka
internal/dag: Implement TLSRoute mode:terminate
692c17e 
Implements support for GatewayAPI TLSRoute mode: terminate which terminates TLS
at the Gateway.

Updates projectcontour#3440

Signed-off-by: Steve Sloka <slokas@vmware.com>
@stevesloka
review comments
c032010 
Signed-off-by: Steve Sloka <slokas@vmware.com>
@stevesloka
fixup error message
0f71ddd 
Signed-off-by: Steve Sloka <slokas@vmware.com>
@stevesloka
add SNI to terminate E2E test
c73c8cd 
Signed-off-by: Steve Sloka <slokas@vmware.com>
@stevesloka
fix up E2E tests
c0ec0e0 
Signed-off-by: Steve Sloka <slokas@vmware.com>
@stevesloka
rework how validation of bad SNI is done
6bf15b1 
Signed-off-by: Steve Sloka <slokas@vmware.com>
@stevesloka
make the tests run
398ef0f 
Signed-off-by: Steve Sloka <slokas@vmware.com>
@stevesloka
check for nil instead of false
2364162 
Signed-off-by: Steve Sloka <slokas@vmware.com>
@stevesloka
fix up tests after rebase
9dee885 
Signed-off-by: Steve Sloka <slokas@vmware.com>
@stevesloka stevesloka requested a review from sunjayBhatia June 27, 2021 14:22
@stevesloka
Copy link
Member Author

all rebased @skriss @sunjayBhatia, please take a look!

skriss
skriss approved these changes Jun 28, 2021
View reviewed changes
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

sunjayBhatia
sunjayBhatia approved these changes Jun 28, 2021
View reviewed changes
internal/dag/builder_test.go
},
want: listeners(),
},
"TLSRoute with TLS.Mode=Terminate is invalid when TLS is not defined": {
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
"TLSRoute with TLS.Mode=Terminate is invalid when TLS is not defined": {
"TLSRoute with TLS.Mode=Terminate is invalid when TLS certificate reference is not defined": {

@skriss
Copy link
Member

skriss commented Jun 28, 2021

gonna merge, can fix the nit separately

@skriss skriss merged commit 1d4526a into projectcontour:main Jun 28, 2021
skriss added a commit to skriss/contour that referenced this pull request Jun 28, 2021
@skriss
internal/dag: fix test case name
cd2e556 
Updates projectcontour#3801.

Signed-off-by: Steve Kriss <krisss@vmware.com>
@skriss skriss mentioned this pull request Jun 28, 2021
Merged
@youngnick youngnick added this to the 1.17.0 milestone Jul 2, 2021
stevesloka pushed a commit that referenced this pull request Jul 2, 2021
@skriss
internal/dag: fix test case name (#3847)
86c5f13 
Updates #3801.

Signed-off-by: Steve Kriss <krisss@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skriss skriss skriss approved these changes

@sunjayBhatia sunjayBhatia sunjayBhatia approved these changes

@danehans danehans Awaiting requested review from danehans

Assignees
No one assigned
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
1.17.0
Development

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