Add support for Port in ParentReference

[sarthyparty]

Proposed changes

Problem: ParentRefs didn't support Port
While adding this feature, I noticed that buildSectionRefs and findAttachableListeners had duplicated logic. I first tried to refactor findAttachableListeners, but that seemed to create much more complexities, so I decided to refactor buildSectionNameRefs and then solve the bug differently in policies. In policies I noticed a really small issue where we weren't looping through hostnames correctly so I fixed that and also added in the fix for the bug.

Solution: I added support for it and enabled conformance

Testing: Unit tests and conformance test

Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide
specific feedback, add them here.

Closes #2946

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have checked that all unit tests pass after adding my changes
• I have updated necessary documentation
• I have rebased my branch onto main
• I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.

[codecov]

Codecov Report

❌ Patch coverage is 97.70115% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.11%. Comparing base (d9d1181) to head (6ce774c).

Files with missing lines	Patch %	Lines
internal/controller/state/graph/policies.go	95.55%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3778      +/-   ##
==========================================
+ Coverage   87.00%   87.11%   +0.10%     
==========================================
  Files         128      128              
  Lines       16404    16442      +38     
  Branches       62       62              
==========================================
+ Hits        14273    14324      +51     
+ Misses       1956     1944      -12     
+ Partials      175      174       -1     

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

[sjberman]

Nice work! You'll also need to update our compatibility doc in the docs repo (merge to branch ngf-release-2.2) with these updates.

[sarthyparty]

@sjberman I chose to refactor the attachToListener side, which was weird because we're actually sorting the listeners downstream. I'm not sure the original intention of sorting, but I removed it now. This shouldn't make any functionality changes (besides the port addition)

[sjberman]

Usually when we are sorting something, it's for good reason :) but I'll take a look.

[sjberman]

As part of your recent changes, have you verified that the bug that was fixed as part of #3418 was not reintroduced? @bjee19 should probably take a closer look as well.

[salonichf5]

Are we only attaching a single listener based on the port? Meaning, if a port is specified, we attach to the matching port. But if the port is not specified, then the existing rules still apply — such as using the winning hostname or attaching to multiple listeners.

I just want to confirm that listener isolation rules remain unchanged.

[sarthyparty]

If a port is specified, the rules should be the same, with the additional port filtering added

[sarthyparty]

I refactored the code and did fixed the bug from that issue. The reason why I refactored the code was because that the previous fix seemed to introduce bugs, so I had to do it differently, in policies

[bjee19]

I refactored the code and did fixed the bug from that issue. The reason why I refactored the code was because that the previous fix seemed to introduce bugs, so I had to do it differently, in policies

@sarthyparty Could you explain a little more about the refactoring that will be introduced in this pr? Also, could you explain some more on what bugs the previous fix seemed to have introduced? Usually that is included in the PR description or just a comment on the PR, and will help us if we ever need to trace back to this PR.

[salonichf5]

If a port is specified, the rules should be the same, with the additional port filtering added

but what is preferred first?

[sarthyparty]

While adding the Port parentRef feature, I noticed that buildSectionRefs and findAttachableListeners had duplicated logic. I first tried to refactor findAttachableListeners, but that seemed to create much more complexities, so I decided to refactor buildSectionNameRefs and then solve the bug differently in policies. In policies I noticed a really small issue where we weren't looping through hostnames correctly so I fixed that and also added in the fix for the bug.

[sarthyparty]

The complexities that seemed difficult to address were sorting the L4Route attachable listeners, which didn't happen when the sectionName was nil, and also the parentRef attachment status logic was going to have issues since the parentsRefs were split.

[sarthyparty]

but what is preferred first?

Wdym by this? My main code change was just filtering the attachable listeners by port if port was specified. I don't mess with any of the rules.

[salonichf5]

but what is preferred first?

Wdym by this? My main code change was just filtering the attachable listeners by port if port was specified. I don't mess with any of the rules.

Not the rules, but hostnames. So for example if we have two listeners with same hostnames/wildcards and 2 routes also having 2 hostnames with both those hostnames

and port is also specified, we will select listeners based on port or hostname first?

For example

Gateway

  listeners:
  - name: wildcard-example-com
    port: 81
    protocol: HTTP
    hostname: "*.example.com"
    allowedRoutes:
      namespaces:
        from: All
  - name: wildcard-foo-example-com
    port: 81
    protocol: HTTP
    hostname: "*.foo.example.com"
    allowedRoutes:
      namespaces:
        from: All

2 HTTPRoutes

apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: route1
spec:
  parentRefs:
  - name: gateway
    sectionName: empty-hostname
    port: 81
  hostnames:
# both hostnames could match both the listener hostname
  - "*.foo.example.com" 
  - "abc.foo.example.com" 
  rules:
  - matches:
    - path:
        type: PathPrefix
        value: /empty-hostname
    backendRefs:
    - name: coffee
      port: 80
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: route2
spec:
  parentRefs:
  - name: gateway
    sectionName: wildcard-example-com
    port: 82
  hostnames:
# both hostnames could match both the listener hostname
  - "*.foo.example.com" 
  - "abc.foo.example.com" 
  rules:
  - matches:
    - path:
        type: PathPrefix
        value: /wildcard-example-com
    backendRefs:
    - name: coffee
      port: 82

which listener would attach to which route?

do we prioritize port match or hostname match?

[sarthyparty]

The first HTTPRoute would attach to no listeners because the sectionName does not match any listener names. The second HTTPRoute would not attach to any listener because the listener that matches the section name doesn't have the port matching.

[sarthyparty]

Assuming you meant to make the sectionName nil for the first HTTPRoute, then my changes would see that the port matches both listeners on the gateway and return them both as attachable listeners. What happens next based on the hostname, is unchanged.

[salonichf5]

Assuming you meant to make the sectionName nil for the first HTTPRoute, then my changes would see that the port matches both listeners on the gateway and return them both as attachable listeners. What happens next based on the hostname, is unchanged.

oh yeah my bad I wanted to remove the sectionaName. Thank you for the clarification