Skip to content

skip invalid listener first in IR#8577

Open
zirain wants to merge 6 commits intoenvoyproxy:mainfrom
zirain:skip-invalid-listener
Open

skip invalid listener first in IR#8577
zirain wants to merge 6 commits intoenvoyproxy:mainfrom
zirain:skip-invalid-listener

Conversation

@zirain
Copy link
Copy Markdown
Member

@zirain zirain commented Mar 23, 2026

This's seperated from #8361

Invalid listeners should probably be skipped before check confilict.
Otherwise an invalid-first listener can still win hostname/protocol precedence and cause a later valid listener on the same hostname/port to be marked HostnameConflict.

@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 23, 2026

Deploy Preview for cerulean-figolla-1f9435 ready!

Name Link
🔨 Latest commit b614a71
🔍 Latest deploy log https://app.netlify.com/projects/cerulean-figolla-1f9435/deploys/69ec270dad8b200008a719fc
😎 Deploy Preview https://deploy-preview-8577--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 Mar 23, 2026

Codecov Report

❌ Patch coverage is 79.73568% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.37%. Comparing base (e098718) to head (b614a71).

Files with missing lines Patch % Lines
internal/gatewayapi/validate.go 74.71% 37 Missing and 7 partials ⚠️
internal/gatewayapi/listener.go 96.15% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8577      +/-   ##
==========================================
+ Coverage   74.35%   74.37%   +0.02%     
==========================================
  Files         246      246              
  Lines       39287    39426     +139     
==========================================
+ Hits        29211    29323     +112     
- Misses       8045     8066      +21     
- Partials     2031     2037       +6     

☔ 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.

@zirain zirain force-pushed the skip-invalid-listener branch from 45bd0eb to e9a458a Compare April 4, 2026 00:53
@zirain zirain marked this pull request as ready for review April 4, 2026 00:54
@zirain zirain requested a review from a team as a code owner April 4, 2026 00:54
Comment thread internal/gatewayapi/validate.go Outdated
Comment thread internal/gatewayapi/validate.go Outdated
Comment thread internal/gatewayapi/listener.go
@zirain zirain force-pushed the skip-invalid-listener branch 2 times, most recently from f5bee1d to 2db4164 Compare April 6, 2026 11:28
message: Route is accepted
reason: Accepted
status: "True"
message: Multiple routes on the same TCP listener
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.

this's because tcp-80 is invalid and skipped now.

@zirain zirain force-pushed the skip-invalid-listener branch from 2db4164 to 1fa2c97 Compare April 6, 2026 11:57
@zirain zirain requested review from arkodg and jukie April 7, 2026 06:28
@zirain zirain force-pushed the skip-invalid-listener branch from 1fa2c97 to 46b11c4 Compare April 13, 2026 06:46
@zirain zirain changed the title skip invalid listener first skip invalid listener first in IR Apr 14, 2026
@zirain
Copy link
Copy Markdown
Member Author

zirain commented Apr 14, 2026

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

if listener.tls.frontendTLSValidation != nil &&
listener.tls.frontendTLSValidation.ValidateError != nil {

P1 Badge Mark frontend TLS validation failures as spec-invalid

validateListenerSpec now depends on validateTLSConfiguration’s boolean to decide whether a listener should be excluded from conflict resolution, but the frontendTLSValidation.ValidateError path only sets conditions and never flips specValid to false. That means listeners with invalid frontend CA refs can still participate in conflict checks and block otherwise valid listeners on the same port/hostname, which defeats the intended invalid-first skip behavior in this change.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread internal/gatewayapi/validate.go Outdated
@zirain zirain force-pushed the skip-invalid-listener branch from f587c83 to 97a571f Compare April 14, 2026 23:35
@zirain zirain added this to the v1.8.0-rc.1 Release milestone Apr 16, 2026
Comment thread internal/gatewayapi/listener.go Outdated
Comment thread internal/gatewayapi/listener.go Outdated
Comment thread internal/gatewayapi/listener.go Outdated
@zirain zirain force-pushed the skip-invalid-listener branch 2 times, most recently from 7f0e72b to b214810 Compare April 18, 2026 10:32
@zirain zirain requested a review from jukie April 19, 2026 14:16
@cnvergence
Copy link
Copy Markdown
Member

cnvergence commented Apr 21, 2026

This might solve #8519, wdyt @zirain?

@zirain
Copy link
Copy Markdown
Member Author

zirain commented Apr 21, 2026

This might solve #8519, wdyt @zirain?

possible not

xref: https://kubernetes.slack.com/archives/CR0H13KGA/p1776730527700729?thread_ts=1776560870.202129&cid=CR0H13KGA

The thing is, we always have to treat an object update like a new object, because otherwise we are implicitly forcing controllers to track state between reconciliations, which is fragile across controller reboots.In Kubernetes, it's vital that we always only consider the current state of objects, not previous states, because if we do track state, then that state will be lost when the controller restarts.

So, an update to a Gateway must be treated the same as a new Gateway with the same Listeners.

And yes, Mike and Zac are correct, the reason for the different behavior is that Gateway Listeners are all in the same object, so we have no way to choose one over the other - and even more, we can be confident that having two identical Listeners is user error, so we need to drop both. It's user error because, for the user, the whole object is visible, so if you're adding two entries with the same Port and different Protocol, then that's a mistake on your part.

name: http-80
protocol: HTTP
servicePort: 80
- name: envoy-gateway/gateway-2/https
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.

why is existing logic for Gateway listeners changing ?

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.

- attachedRoutes: 1
      conditions:
      - lastTransitionTime: null
        message: Listener must have TLS set when protocol is HTTPS.
        reason: Invalid
        status: "False"
        type: Programmed
      - lastTransitionTime: null
        message: Listener references have been resolved
        reason: ResolvedRefs
        status: "True"
        type: ResolvedRefs
      name: https
      supportedKinds:
      - group: gateway.networking.k8s.io
        kind: HTTPRoute
      - group: gateway.networking.k8s.io
        kind: GRPCRoute

Signed-off-by: zirain <zirain2009@gmail.com>
zirain added 4 commits April 23, 2026 06:57
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain zirain force-pushed the skip-invalid-listener branch from 89a7c66 to b77d5dc Compare April 22, 2026 22:57
@zirain zirain requested a review from arkodg April 22, 2026 22:58
Copy link
Copy Markdown
Contributor

@jukie jukie 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!

@jukie jukie requested a review from a team April 25, 2026 02:35
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