Skip to content

fix ListenerSetHostnameConflict/ListenerSetProtocolConflict conformance test#8361

Closed
zirain wants to merge 3 commits into
envoyproxy:mainfrom
zirain:listenerset-hostname
Closed

fix ListenerSetHostnameConflict/ListenerSetProtocolConflict conformance test#8361
zirain wants to merge 3 commits into
envoyproxy:mainfrom
zirain:listenerset-hostname

Conversation

@zirain
Copy link
Copy Markdown
Member

@zirain zirain commented Feb 27, 2026

Fix ListenerSetHostnameConflict conformance test

  • fix IncreaseAttachedListenerSets only increase when ListenerSet is valid

@zirain zirain requested a review from a team as a code owner February 27, 2026 08:13
@netlify
Copy link
Copy Markdown

netlify Bot commented Feb 27, 2026

Deploy Preview for cerulean-figolla-1f9435 ready!

Name Link
🔨 Latest commit c82d97e
🔍 Latest deploy log https://app.netlify.com/projects/cerulean-figolla-1f9435/deploys/69eaa48aa8d9ac0008be4405
😎 Deploy Preview https://deploy-preview-8361--cerulean-figolla-1f9435.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 27, 2026

Codecov Report

❌ Patch coverage is 88.50174% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.39%. Comparing base (c3d06fa) to head (68da39b).
⚠️ Report is 40 commits behind head on main.

Files with missing lines Patch % Lines
internal/gatewayapi/validate.go 90.00% 13 Missing and 6 partials ⚠️
internal/gatewayapi/listener.go 85.54% 7 Missing and 5 partials ⚠️
internal/gatewayapi/contexts.go 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8361      +/-   ##
==========================================
+ Coverage   74.34%   74.39%   +0.04%     
==========================================
  Files         244      244              
  Lines       38768    38935     +167     
==========================================
+ Hits        28824    28967     +143     
- Misses       7950     7962      +12     
- Partials     1994     2006      +12     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread internal/cmd/egctl/testdata/translate/out/backend-endpoint.all.yaml Outdated
@zirain zirain changed the title fix ListenerSetHostnameConflict conformance test fix ListenerSetHostnameConflict/ListenerSetProtocolConflict conformance test Feb 27, 2026
@zirain zirain force-pushed the listenerset-hostname branch 5 times, most recently from 1663b4e to 668e7f7 Compare March 2, 2026 14:44
Comment thread internal/gatewayapi/listener.go Outdated
@zirain zirain added this to the v1.8.0-rc.1 Release milestone Mar 2, 2026
Comment thread internal/gatewayapi/contexts.go Outdated
@zhaohuabing
Copy link
Copy Markdown
Member

Hi @zirain can we split ListenerSetHostnameConflict/ListenerSetProtocolConflict fixes into two PRs, it will be easier to review.

@zirain zirain force-pushed the listenerset-hostname branch from 9ee8b95 to c861521 Compare March 4, 2026 02:44
@zirain zirain changed the title fix ListenerSetHostnameConflict/ListenerSetProtocolConflict conformance test fix ListenerSetHostnameConflict conformance test Mar 4, 2026
@zirain zirain force-pushed the listenerset-hostname branch from c861521 to a6d8112 Compare March 4, 2026 02:49
@zirain zirain force-pushed the listenerset-hostname branch from 5ae7a19 to d1c0767 Compare March 5, 2026 03:56
@zirain
Copy link
Copy Markdown
Member Author

zirain commented Mar 5, 2026

Hi @zirain can we split ListenerSetHostnameConflict/ListenerSetProtocolConflict fixes into two PRs, it will be easier to review.

Splited.

@zirain zirain force-pushed the listenerset-hostname branch from d1c0767 to 8d5a5d9 Compare March 5, 2026 04:36
@zhaohuabing
Copy link
Copy Markdown
Member

zhaohuabing commented Mar 6, 2026

kubernetes-sigs/gateway-api#4618

I don't see any gateway conformance tests checking the conflicted condition but thats probably because validation ensures no conflicts on gateway listeners

Hi @zirain can you confirm if the conformance test could pass without adding the conflicted=false condition to the non-conflicted listener?

@zirain
Copy link
Copy Markdown
Member Author

zirain commented Mar 6, 2026

kubernetes.GatewayListenersMustHaveConditions(t, suite.Client, suite.TimeoutConfig, gwNN, generateAcceptedListenerConditions(), "gateway-listener")

func generateAcceptedListenerConditions() []metav1.Condition {
	return []metav1.Condition{
		{
			Type:   string(gatewayv1.ListenerConditionResolvedRefs),
			Status: metav1.ConditionTrue,
			Reason: "", // any reason
		},
		{
			Type:   string(gatewayv1.ListenerConditionAccepted),
			Status: metav1.ConditionTrue,
			Reason: "", // any reason
		},
		{
			Type:   string(gatewayv1.ListenerConditionProgrammed),
			Status: metav1.ConditionTrue,
			Reason: "", // any reason
		},
		{
			Type:   string(gatewayv1.ListenerConditionConflicted), // this's required.
			Status: metav1.ConditionFalse,
			Reason: "", // any reason
		},
	}
}

@zirain zirain force-pushed the listenerset-hostname branch 2 times, most recently from bfd2b62 to 36fa1bc Compare March 9, 2026 08:09
Comment thread internal/gatewayapi/validate.go Outdated
@zirain zirain force-pushed the listenerset-hostname branch 2 times, most recently from 43de278 to 513cee2 Compare March 16, 2026 08:24
@zirain zirain force-pushed the listenerset-hostname branch from 513cee2 to 3795153 Compare March 17, 2026 01:18
@zirain zirain force-pushed the listenerset-hostname branch from 3349e94 to 19c01ac Compare March 26, 2026 11:56
@zirain zirain requested review from jukie and zhaohuabing April 2, 2026 22:53
@zirain zirain force-pushed the listenerset-hostname branch 2 times, most recently from 6fc7803 to 7bc5d9c Compare April 3, 2026 06:10
@zirain
Copy link
Copy Markdown
Member Author

zirain commented Apr 3, 2026

@jukie @zhaohuabing can you take a review again?

for _, gateway := range gateways {
portListenerInfo := map[gwapiv1.PortNumber]*portListeners{}
for _, listener := range gateway.listeners {
if listener.Protocol == gwapiv1.UDPProtocolType || listener.Protocol == gwapiv1.TCPProtocolType {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unsupported listeners should probably be skipped in hostname conflict checks too. Otherwise an invalid-first listener can still win hostname precedence and cause a later valid listener on the same hostname/port to be marked HostnameConflict.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Let's do #8577 first.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not opposed to it being in a follow-up either

jukie
jukie previously approved these changes Apr 4, 2026
@zirain zirain requested a review from a team April 4, 2026 03:12
if countInt32 > 0 {
g.Status.AttachedListenerSets = ptr.To(countInt32)
}
return
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we return here, arent we avoid an increment ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

if we reach here, that means new count is 0 and AttachedListenerSets is nil, so it's safe to skip.

@zirain zirain force-pushed the listenerset-hostname branch 2 times, most recently from 59f59cb to 58a2796 Compare April 6, 2026 11:51
@zirain zirain marked this pull request as draft April 6, 2026 11:52
@zirain zirain force-pushed the listenerset-hostname branch 4 times, most recently from e37f5c2 to 9b7180d Compare April 6, 2026 13:38
zirain added 2 commits April 7, 2026 17:43
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain zirain force-pushed the listenerset-hostname branch from 9b7180d to 68da39b Compare April 7, 2026 09:43
Comment thread internal/gatewayapi/contexts.go Outdated
Comment on lines +261 to +280
specValid := t.validateAllowedNamespaces(listener)
if !validateProtocolRules(listener) {
specValid = false
}

// Phase 2: Validate allowed routes based on protocol
if listener.Protocol == gwapiv1.HTTPProtocolType ||
listener.Protocol == gwapiv1.HTTPSProtocolType ||
listener.Protocol == gwapiv1.TCPProtocolType ||
listener.Protocol == gwapiv1.UDPProtocolType ||
listener.Protocol == gwapiv1.TLSProtocolType {
var tlsMode *gwapiv1.TLSModeType
if listener.TLS != nil {
tlsMode = listener.TLS.Mode
}
allowedKinds := allowedRouteKindsForProtocol(listener.Protocol, tlsMode)
if !t.validateAllowedRoutes(listener, allowedKinds...) {
specValid = false
}
} else {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this be reworked or simplified? The else block effectively covers the !validateProtocolRules case if I'm reading correctly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

that's why I want merge #8577 first, will make this one cleaner.

Co-authored-by: Isaac Wilson <isaac.wilson514@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
@jukie
Copy link
Copy Markdown
Contributor

jukie commented Apr 25, 2026

Can you add a release note too

@zirain
Copy link
Copy Markdown
Member Author

zirain commented May 6, 2026

replaced by #8910

@zirain zirain closed this May 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants