Skip to content

Conversation

@pmalek
Copy link
Member

@pmalek pmalek commented Nov 4, 2022

What this PR does / why we need it:

This PR aims to implement GEP-957: Destination Port Matching

Which issue this PR fixes:

Fixes #2709

Special notes for your reviewer:

  • Which Accepted reason to use:

    For now, the routes that will end up not being attached to a Gateway because of unmet port criteria (missing listener in the Gateway with the specified Port) will get an Accepted condition with NoMatchingListenerHostname reason.

    This should be addressed also in this PR IMHO and the proposed course of action is:

    - use a const defined in our codebase
    - submit an issue with the above described proposal to https://github.com/kubernetes-sigs/gateway-api/
    - swap the self defined const with the one defined in upstream whenever (if) it gets released there.

    The above has been mitigated for now with RouteReasonNoMatchingListenerPort which has been defined in our codebase.

  • Open question: since only HTTPRoute was graduated in beta would it be fine to add tests just for that route type and leave the other ones without (while tracking that in a separate issue: to add whenever they graduate to beta)?

  • I also added a call to r.DataplaneClient.DeleteObject(httproute) when the route "is not accepted". This way I was able to get the route to get pruned after changing a parent ref port which wouldn't match the Gateway. I believe that's the way it should be but I can be proven wrong.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR
  • add integration tests for this change for
    • HTTPRoute

@pmalek pmalek added area/feature New feature or request area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API labels Nov 4, 2022
@pmalek pmalek added this to the KIC v2.8.0 milestone Nov 4, 2022
@pmalek pmalek self-assigned this Nov 4, 2022
@pmalek pmalek temporarily deployed to Configure ci November 4, 2022 17:56 Inactive
@pmalek pmalek force-pushed the gep-957-port-matching branch from 0bfb5a9 to bc04535 Compare November 4, 2022 18:03
@pmalek pmalek temporarily deployed to Configure ci November 4, 2022 18:03 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 4, 2022 18:29 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 4, 2022 18:37 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 4, 2022 18:56 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 4, 2022 18:56 Inactive
@pmalek pmalek marked this pull request as ready for review November 4, 2022 21:18
@pmalek pmalek requested a review from a team as a code owner November 4, 2022 21:18
Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Deleting from the store is the same thing #3119 does, with the same caveat as its notes. It's a break from what we've been doing, but I think it is probably what we should be doing.

One issue with test logging and testify not providing the tools we need to report issues concisely. Implementation looks fine.

@pmalek pmalek force-pushed the gep-957-port-matching branch from bc04535 to 4d2566b Compare November 5, 2022 12:08
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Nov 5, 2022
@pmalek pmalek temporarily deployed to Configure ci November 5, 2022 12:08 Inactive
@pmalek pmalek force-pushed the gep-957-port-matching branch from 4d2566b to 3a39843 Compare November 5, 2022 12:14
@pmalek pmalek temporarily deployed to Configure ci November 5, 2022 12:15 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 5, 2022 12:38 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 5, 2022 12:38 Inactive
@pmalek pmalek force-pushed the gep-957-port-matching branch from 3a39843 to 7f3c3f8 Compare November 7, 2022 11:05
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 11:05 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 11:30 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 11:38 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 12:01 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 12:01 Inactive
@pmalek pmalek force-pushed the gep-957-port-matching branch from 7f3c3f8 to fe073c7 Compare November 7, 2022 12:41
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 12:41 Inactive
@pmalek pmalek requested a review from rainest November 7, 2022 12:41
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 13:04 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 13:04 Inactive
@pmalek pmalek force-pushed the gep-957-port-matching branch from fe073c7 to 29b0e3e Compare November 7, 2022 17:58
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 17:58 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 18:23 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 18:23 Inactive
Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Open question: since only HTTPRoute was graduated in beta would it be fine to add tests just for that route type and leave the other ones without (while tracking that in a separate issue: to add whenever they graduate to beta)?

In general, since we do have implementations of the other route types and do test with our alpha flag on, I'd say apply like changes to all unless we expect something to change (if there's an existing GEP or an area of the spec we know is incomplete). Here we should have reasonable confidence that the other route types can (or always will) have ports and will bind to Listeners with those ports, so we could proceed with adding the rest of these immediately.

@rainest rainest enabled auto-merge (squash) November 7, 2022 19:42
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 19:43 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 19:43 Inactive
@pmalek pmalek temporarily deployed to Configure ci November 7, 2022 19:49 Inactive
@rainest rainest merged commit 04f4d3b into main Nov 7, 2022
@rainest rainest deleted the gep-957-port-matching branch November 7, 2022 19:49
@pmalek
Copy link
Member Author

pmalek commented Nov 7, 2022

Open question: since only HTTPRoute was graduated in beta would it be fine to add tests just for that route type and leave the other ones without (while tracking that in a separate issue: to add whenever they graduate to beta)?

In general, since we do have implementations of the other route types and do test with our alpha flag on, I'd say apply like changes to all unless we expect something to change (if there's an existing GEP or an area of the spec we know is incomplete). Here we should have reasonable confidence that the other route types can (or always will) have ports and will bind to Listeners with those ports, so we could proceed with adding the rest of these immediately.

Good, in such case I'll leave #2709 open for that purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/feature New feature or request area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GEP-957: Gateway port matching for L4 Gateway API Routes

3 participants