Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: set proper destination port for TCPRoute and UDPRoute #4928

Merged
merged 6 commits into from
Oct 25, 2023

Conversation

programmer04
Copy link
Member

@programmer04 programmer04 commented Oct 20, 2023

What this PR does / why we need it:

As described in #4864 TCPRoute sets the incorrect destinations port on the Routes it creates (sets the desired backend service port instead of the listener port). The same happens for UDPRoute. It ignores the port set by specifying SectionName, it always requires the same port for the backend service as for the listener.

This PR fixes it for both cases. When SectionName is specified take config from it. Otherwise, it works now as described (a little bit vague) in spec

When unspecified (empty string), this will reference the entire resource. (src)

so every listener that matches the protocol (UDP or TCP) is used.

It breaks current behavior that automatically matches ports by number from target services to listening ports on Gateway with the same numbers.

It is interesting that no conformance test from gateway-api catches that problem - a test for this should exist in gateway-api project.

There is also another field for this purpose PortNumber it is part of experimental and extended gateway-api, currently KIC doesn't care about it at all. A separate issue has been created to provide support for it and will be prioritized accordingly see

Which issue this PR fixes:

Fixes #4864

Special notes for your reviewer:

The first commit 1100749 introduces adjustments for TCPRoute and TLSRoute integration tests to check such a scenario. Test for TCPRoute does not pass, for TLSRoute bug doesn't exist because matching is done with hostname.

The second commit b385f15 introduces actual fix and unit test cases, also for UDPRoute. Integration test for UDPRoute requires refactoring (to make it similar to TCP one) which is not in the scope of this PR due to a major rework of it in PR #4823. After it should be adjusted to test changes introduced in this PR too.

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

@programmer04 programmer04 added area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API fix labels Oct 20, 2023
@programmer04 programmer04 added this to the KIC v3.0.0 milestone Oct 20, 2023
@programmer04 programmer04 self-assigned this Oct 20, 2023
@programmer04 programmer04 requested a review from a team as a code owner October 20, 2023 16:36
@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (4efc818) 77.6% compared to head (91b4c66) 77.6%.

❗ Current head 91b4c66 differs from pull request most recent head e224eea. Consider uploading reports for the commit e224eea to get more accurate results

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #4928   +/-   ##
=====================================
  Coverage   77.6%   77.6%           
=====================================
  Files        166     166           
  Lines      18640   18652   +12     
=====================================
+ Hits       14473   14485   +12     
- Misses      3329    3330    +1     
+ Partials     838     837    -1     
Files Coverage Δ
internal/dataplane/parser/parser.go 89.6% <88.8%> (-2.2%) ⬇️
internal/dataplane/parser/translate_tcproute.go 64.2% <75.0%> (+5.6%) ⬆️
internal/dataplane/parser/translate_tlsroute.go 56.9% <66.6%> (+0.6%) ⬆️
...ernal/dataplane/parser/translate_routes_helpers.go 86.2% <84.2%> (+1.8%) ⬆️
internal/dataplane/parser/translate_udproute.go 70.1% <72.7%> (-1.6%) ⬇️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

test/integration/tlsroute_test.go Outdated Show resolved Hide resolved
test/integration/tcproute_test.go Outdated Show resolved Hide resolved
@programmer04
Copy link
Member Author

@pmalek major rework was done, PR description is updated accordingly, PTAL Let's see if I understand gateway-api specification correctly 😛

@programmer04 programmer04 marked this pull request as ready for review October 24, 2023 14:44
internal/dataplane/parser/translate_udproute_test.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translate_udproute_test.go Outdated Show resolved Hide resolved
internal/dataplane/parser/parser.go Outdated Show resolved Hide resolved
internal/dataplane/parser/parser.go Outdated Show resolved Hide resolved
internal/dataplane/parser/parser.go Show resolved Hide resolved
@programmer04 programmer04 enabled auto-merge (squash) October 25, 2023 10:37
@programmer04 programmer04 merged commit 5ab69eb into main Oct 25, 2023
33 checks passed
@programmer04 programmer04 deleted the fix-bug-backup branch October 25, 2023 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API breaking change fix size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TCPRoute & UDPRoute: destination port taken from backendRef and not from listener config
2 participants