Skip to content

Conversation

@johannes-engler-mw
Copy link

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches
  • Documentation
  • Refactor
  • Chore
  • CI/CD or Tests

What this PR does / why we need it:

This change implements support for port-based routing in HTTPRoute and GRPCRoute by leveraging the 'server_port' variable in APISIX. When a route targets a specific Gateway listener (via 'sectionName' or matching), the translator now adds a condition to ensure the route only matches traffic on that specific port.

Key changes:

  • Update translator to add 'server_port' matching variables to routes.
  • Enhance controller to track all matching listeners for a route.
  • Update E2E test framework to support multi-port APISIX and in-cluster testing.
  • Add comprehensive E2E tests for port-based routing scenarios.

It fixes #2639.

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible?

johannes-engler-mw and others added 5 commits January 16, 2026 09:11
This change implements support for port-based routing in HTTPRoute and GRPCRoute
by leveraging the 'server_port' variable in APISIX. When a route targets a
specific Gateway listener (via 'sectionName' or matching), the translator now
adds a condition to ensure the route only matches traffic on that specific port.

Key changes:
- Update translator to add 'server_port' matching variables to routes.
- Enhance controller to track all matching listeners for a route.
- Update E2E test framework to support multi-port APISIX and in-cluster testing.
- Add comprehensive E2E tests for port-based routing scenarios.
Reflect the new port-based routing capability in Gateway API documentation.
- Updated concepts/gateway-api.md to mark listener port as partially supported.
- Updated reference/example.md to explain that port is used for matching.
This fixes a linting error flagged by errcheck where the return value of
fmt.Sscanf was being ignored.
This ensures the alpine/curl image is present locally before attempting to
load it into the Kind cluster, preventing build failures in CI.
- Add comprehensive unit tests for addServerPortVars function
- Fix two-port test to use expectedPortList + ElementsMatch for non-deterministic map ordering
- Consolidate redundant single-port tests (removed HTTP/HTTPS duplicates)
- Fix image tag mismatch: alpine/curl:flattened -> alpine/curl:latest
- Fix comment typo: port 9100 -> port 9081

Co-Authored-By: Claude <noreply@anthropic.com>
@johannes-engler-mw
Copy link
Author

Hi there,

While reviewing the feat/2639 changes, I noticed a bug in the status propagation logic that affects all route controllers.

Issue

The acceptStatus variable is shared across all parent gateways in the reconciliation loop. If one gateway fails processing, all parent references are marked as "Not Accepted", which violates the Gateway API specification that requires per-parent status tracking.

Example from httproute_controller.go:163-184:

acceptStatus := ResourceStatus{
    status: true,
    msg:    "Route is accepted",
}
// ...
for _, gateway := range gateways {
    if err := ProcessGatewayProxy(...); err != nil {
        acceptStatus.status = false  // Overwrites status for ALL parents
        acceptStatus.msg = err.Error()
    }
}

Affected Controllers

This pattern exists in all 5 route controllers:

  • httproute_controller.go
  • grpcroute_controller.go
  • tcproute_controller.go
  • tlsroute_controller.go
  • udproute_controller.go

Impact

  • HTTPRoute (highest impact): The feat/2639 port-based routing feature encourages multi-listener/multi-gateway topologies, making partial failures more likely
  • Other route types: Lower impact in practice, but still a spec violation

Proposed Fix

Track status per-gateway using a map:

gatewayStatuses := make(map[string]ResourceStatus)
for _, gateway := range gateways {
    if err := ProcessGatewayProxy(...); err != nil {
        gatewayStatuses[gateway.Gateway.Name] = ResourceStatus{status: false, msg: err.Error()}
    } else {
        gatewayStatuses[gateway.Gateway.Name] = ResourceStatus{status: true, msg: "Route is accepted"}
    }
}
// Use per-gateway status when setting parent status conditions

Question

How would you like to proceed?

  1. Fix only HTTPRoute as part of feat/2639 (since it's most impacted by the new feature)
  2. Fix all route controllers in a separate PR
  3. Fix all route controllers as part of feat/2639

Happy to submit a fix either way.

@johannes-engler-mw
Copy link
Author

Hi again,

I just realized this PR probably adds a breaking change:

The Change
In internal/controller/utils.go, I updated ParseRouteParentRefs to align with the Gateway API Specification (https://gateway-api.sigs.k8s.io/api-types/httproute/#attaching-to-gateways). Specifically, if a Route's ParentRef does
not specify a sectionName, the spec requires it to attach to all compatible listeners.

My implementation now does exactly this:

1 // internal/controller/utils.go
2
3 // Always add to the list of matched listeners
4 matchedListeners = append(matchedListeners, listener)
5 matched = true
6
7 // Only break if sectionName was explicitly specified
8 if sectionNameSpecified {
9     break
10 }

The Risk
Previously, the controller would stop after the first match. This change means that if a user has a Gateway with multiple listeners (e.g., public-80 and internal-8080) and a Route without a sectionName, that Route will now
automatically be exposed on both listeners.

This is technically "correct" per the spec, but for users relying on the old "first match only" behavior, this could inadvertently expose internal services to public ports upon upgrade.

Proposal
I believe we should address this to avoid surprised users. I see two main options:

  1. Feature Flag: I can add a flag like enable_legacy_listener_matching (defaulting to false or true depending on our safety preference) to let users opt-in to the old behavior.
  2. Documentation/Release Note: We explicitly document this as a breaking change required for Gateway API conformance.

I'm leaning towards option 1 for safety, or at least a very prominent warning. Let me know your preference and I can update the PR accordingly.

@johannes-engler-mw
Copy link
Author

Anyone got time to review my PR and answer my questions?

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.

bug: sectionname in parentrefs is ignored

1 participant