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

Use Service port in generated Kong services #4503

Merged
merged 2 commits into from
Aug 15, 2023
Merged

Use Service port in generated Kong services #4503

merged 2 commits into from
Aug 15, 2023

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Aug 10, 2023

What this PR does / why we need it:

Uses the Service port from the Ingress(like) backend for the Kong service port. Previously we used 80 as a universal default.

This is a cosmetic change. As all KIC-generated services use upstreams, the service port is ignored in favor of the upstream's targets' ports, which are set to the Service's Endpoints' ports.

Which issue this PR fixes:

Fix #4470

Special notes for your reviewer:

This change still uses the 80 default when initially generating services, e.g. in the Ingress translator. Because Ingress(like) backends may use port names instead of numbers, we don't necessarily know the port number until we look up the Service.

We look up Services when generating upstreams, so this moves upstream generation before service generation. I don't love updating the map values in place and returning a new copy, but we were already passing the map by value and don't use the Kong service during upstream generation (we use the attached ServicePort instead).

Moving upstream generation before the skipped services list also feels a bit weird, but apparently we were never using the skipped service list during upstream generation anyway? We were apparently generating some upstreams we weren't actually using, but this probably doesn't matter--worst case I guess you get a bunch of config bloat if you make tons of misconfigured Services, and the fix is "don't do that, it's silly".

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

When generating Kong services, use the Service port from the Ingress
backend instead of the default HTTP port.

This is a cosmetic change. These ports are not actually used by the
routing engine, as all KIC-generated services use upstreams and the
ports associated with their targets (which are the target ports on the
Service's Endpoints).
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Patch coverage: 100.0% and no project coverage change.

Comparison is base (758be7c) 67.6% compared to head (ba5a2e5) 67.7%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #4503   +/-   ##
=====================================
  Coverage   67.6%   67.7%           
=====================================
  Files        161     161           
  Lines      18839   18846    +7     
=====================================
+ Hits       12751   12763   +12     
+ Misses      5327    5324    -3     
+ Partials     761     759    -2     
Files Changed Coverage Δ
internal/dataplane/parser/parser.go 83.0% <100.0%> (-0.6%) ⬇️

... and 3 files with indirect coverage changes

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

@rainest rainest marked this pull request as ready for review August 10, 2023 20:25
@rainest rainest requested a review from a team as a code owner August 10, 2023 20:25
@rainest rainest enabled auto-merge (squash) August 10, 2023 21:12
@rainest rainest changed the title Use Service port in generated Kong services. Use Service port in generated Kong services Aug 10, 2023
@rainest rainest merged commit e02d8ee into main Aug 15, 2023
33 checks passed
@rainest rainest deleted the fix/port branch August 15, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set proper host and port in generated Kong services
2 participants