Skip to content

feat: set OverlappingTLSConfig condition for HTTPS listeners with conflicting hostnames or certs#5777

Merged
zhaohuabing merged 8 commits intoenvoyproxy:mainfrom
zhaohuabing:overlapping-tls-config
Apr 30, 2025
Merged

feat: set OverlappingTLSConfig condition for HTTPS listeners with conflicting hostnames or certs#5777
zhaohuabing merged 8 commits intoenvoyproxy:mainfrom
zhaohuabing:overlapping-tls-config

Conversation

@zhaohuabing
Copy link
Copy Markdown
Member

@zhaohuabing zhaohuabing commented Apr 21, 2025 •
edited
Loading

This PR adds support for OverlappingTLSConfig condition in GatewayStatus. This condition is set if there are overlapping hostnames or certificates between listeners. The ALPN protocol is set to HTTP/1.1 for the overlapping listeners to avoid HTTP/2 Connection Coalescing.

Implements: #5598
Part of: #2675
Release Notes: Yes
Reference: https://gateway-api.sigs.k8s.io/geps/gep-3567/

@zhaohuabing zhaohuabing requested a review from a team as a code owner April 21, 2025 08:50
@zhaohuabing zhaohuabing marked this pull request as draft April 21, 2025 08:51
@zhaohuabing zhaohuabing changed the title feat: set OverlappingTLSConfig condition for HTTPS listeners feat: set OverlappingTLSConfig condition for HTTPS listeners with conflicting hostnames Apr 21, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 21, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.41317% with 11 lines in your changes missing coverage. Please review.

Project coverage is 65.37%. Comparing base (7c88e66) to head (9374e77).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
internal/gatewayapi/status/gateway.go 0.00% 5 Missing ⚠️
internal/gatewayapi/listener.go 97.01% 2 Missing and 2 partials ⚠️
internal/gatewayapi/tls.go 81.81% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5777      +/-   ##
==========================================
+ Coverage   65.32%   65.37%   +0.04%     
==========================================
  Files         222      222              
  Lines       35459    35611     +152     
==========================================
+ Hits        23164    23281     +117     
- Misses      10859    10886      +27     
- Partials     1436     1444       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zhaohuabing zhaohuabing force-pushed the overlapping-tls-config branch 6 times, most recently from 881cb50 to 78b68a7 Compare April 22, 2025 04:31
@zhaohuabing zhaohuabing changed the title feat: set OverlappingTLSConfig condition for HTTPS listeners with conflicting hostnames feat: set OverlappingTLSConfig condition for HTTPS listeners with conflicting hostnames or certs Apr 22, 2025
@zhaohuabing zhaohuabing force-pushed the overlapping-tls-config branch 2 times, most recently from 4fd6960 to c632b88 Compare April 22, 2025 04:44
@zhaohuabing zhaohuabing marked this pull request as ready for review April 22, 2025 04:48
@zhaohuabing zhaohuabing requested review from a team and removed request for a team April 22, 2025 04:48
@zhaohuabing zhaohuabing force-pushed the overlapping-tls-config branch from c632b88 to c88d616 Compare April 22, 2025 05:22
@zhaohuabing
set OverlappingTLSConfig condition for HTTPS listeners
e95749f 
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing force-pushed the overlapping-tls-config branch from c88d616 to e95749f Compare April 22, 2025 05:30
@zhaohuabing
Merge branch 'main' into overlapping-tls-config
31760e0 
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
arkodg
arkodg reviewed Apr 28, 2025
View reviewed changes
arkodg
arkodg reviewed Apr 28, 2025
View reviewed changes
arkodg
arkodg reviewed Apr 28, 2025
View reviewed changes
arkodg
arkodg reviewed Apr 28, 2025
View reviewed changes
internal/gatewayapi/listener.go
}

// isOverlappingHostname checks if two hostnames overlap.
func isOverlappingHostname(hostname1, hostname2 *gwapiv1.Hostname) bool {
Copy link
Copy Markdown
Contributor

@arkodg arkodg Apr 28, 2025

Choose a reason for hiding this comment

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

can we reuse / dedup some of

gateway/internal/gatewayapi/route.go

Line 820 in 7b64768

underscoredHost := strings.ReplaceAll(host, ".", "_")
?

Copy link
Copy Markdown
Member Author

@zhaohuabing zhaohuabing Apr 29, 2025

Choose a reason for hiding this comment

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

I don't think we can. These are handling two different logics.

arkodg
arkodg reviewed Apr 28, 2025
View reviewed changes
@zhaohuabing zhaohuabing requested a review from arkodg April 29, 2025 03:57
arkodg
arkodg previously approved these changes Apr 29, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team April 29, 2025 19:29
@guydc
Copy link
Copy Markdown
Contributor

guydc commented Apr 29, 2025

Do we treat merge gateway correctly here and consider overlap across merged GWs? If not, we can do that as a further improvement.

arkodg
arkodg reviewed Apr 30, 2025
View reviewed changes
@arkodg arkodg added this to the v1.4.0-rc.1 milestone Apr 30, 2025
@zhaohuabing
address comment
6871e65 
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
arkodg
arkodg previously approved these changes Apr 30, 2025
View reviewed changes
@arkodg arkodg requested review from a team April 30, 2025 00:31
@zhaohuabing zhaohuabing mentioned this pull request Apr 30, 2025
Closed
@zhaohuabing
Copy link
Copy Markdown
Member Author

zhaohuabing commented Apr 30, 2025

Do we treat merge gateway correctly here and consider overlap across merged GWs? If not, we can do that as a further improvement.

Track this separately here as we're trying to release v1.4 rc today. #5860

@zhaohuabing
fix test
9374e77 
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing requested a review from arkodg April 30, 2025 02:55
@zhaohuabing zhaohuabing merged commit 68a2713 into envoyproxy:main Apr 30, 2025
25 checks passed
@zhaohuabing zhaohuabing deleted the overlapping-tls-config branch April 30, 2025 03:57
@zhaohuabing zhaohuabing mentioned this pull request May 27, 2025
Closed
@lboynton lboynton mentioned this pull request Jun 16, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zirain zirain zirain approved these changes

@arkodg arkodg arkodg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.4.0-rc.1

Development

Successfully merging this pull request may close these issues.

4 participants

@zhaohuabing @guydc @zirain @arkodg