kubernetes-ingress-controller icon indicating copy to clipboard operation
kubernetes-ingress-controller copied to clipboard

GEP-957: Gateway port matching for Gateway API Routes

Open pmalek opened this issue 2 years ago • 1 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

Problem Statement

Implement gateway port matching for Gateway API Routes as it was defined in https://gateway-api.sigs.k8s.io/geps/gep-957/ and implemented in https://github.com/kubernetes-sigs/gateway-api/pull/1002.

Proposed Solution

No response

Additional information

Relevant comment in translation code: https://github.com/Kong/kubernetes-ingress-controller/blob/7cb5e8a29483ca4968169665c2a0296d73fd6b6f/internal/dataplane/parser/translate_routes_helpers.go#L126-L129

Acceptance Criteria

  • [ ] Any route With the port field set results in a kong route with that port in the destination.
  • [ ] check that it actually works with HTTPRoute, and if not document it. The Kong destinations field is intended primarily for L4 routes; it may not work with HTTP routes.

pmalek avatar Jul 18 '22 09:07 pmalek

Because https://github.com/kubernetes-sigs/gateway-api/pull/1002 is experimental, moving to milestone 4 as it isn't a blocker for us to get to beta.

scseanchow avatar Jul 19 '22 13:07 scseanchow

@pmalek Isn't this issue solved by the PR https://github.com/Kong/kubernetes-ingress-controller/pull/2708/ ?

jrsmroz avatar Oct 19 '22 13:10 jrsmroz

#2708 only intended to make the implementation more DRY 🤔 Why do you think it solves this issue?

pmalek avatar Oct 20 '22 11:10 pmalek

I've misread. The destination port is taken from backends refs for now. If I read the GEP correctly, we need to take it from a parentRef.port, validate if listener with port like this exist in parent gateway, validate if backend ref has a matching port (). The rest would be mostly the same.

However, how it should behave if backend ref have different port specified then the parentRef.port?

jrsmroz avatar Oct 20 '22 11:10 jrsmroz

#3129 implemented it for HTTPRoute (and it also should work for other routes but no relevant tests have been added for those hence leaving those open).

pmalek avatar Nov 07 '22 21:11 pmalek

I wonder if https://github.com/Kong/kubernetes-ingress-controller/pull/3129 is all that's needed. It deals with matching route to the listener in the controller, but it does not limit the routes by port in the data plane proxy. For example, if dataplane proxy listens on multiple destination ports and gateway listener is configured with any one those, the matching kong route will be generated from gateway api route. However, the generated route will not be limited to particular destination port. Wouldn't in this case kong route be served on any dataplane proxy port?

jrsmroz avatar Nov 09 '22 20:11 jrsmroz

However, the generated route will not be limited to particular destination port. Wouldn't in this case kong route be served on any dataplane proxy port?

For L7 (and thus the only version currently tested) yes, though we do still need to add a not accepted condition if we can't match to the requested port--even if an HTTP route is served on all HTTP ports, we do still need a listen for the requested port. Matching a Listener should mean that we do have one, as we're separately validating the (unmanaged) Gateway to confirm that all Listeners are satisfied by appropriate Kong configuration.

L4 are served only on the port they indicate.

rainest avatar Nov 10 '22 17:11 rainest

Unassigned @pmalek - because it seems that the #3129 as a self-contained part is already completed, and the rest of the AC hasn't been started yet.

mflendrich avatar Nov 15 '22 16:11 mflendrich

TLS and TCP routes have been fixed and covered in https://github.com/Kong/kubernetes-ingress-controller/pull/3226. For UDPRoute we made a decision to postpone it to the next release due to missing dependencies (tracking issue: https://github.com/Kong/kubernetes-ingress-controller/issues/3237).

Docs reflecting the limitation for UDPRoute: https://github.com/Kong/docs.konghq.com/pull/4889

@pmalek do you think we may close this as soon as the docs get merged?

czeslavo avatar Dec 08 '22 14:12 czeslavo

Sure thing! Thank you 🙇.

I've put #3237 into 2.9 milestone so that we don't miss it. I've also updated the description of this issue to reflect the current state of affairs and what has been done.

pmalek avatar Dec 08 '22 14:12 pmalek