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

feat: add stub upstream when config is empty in dbless mode #4316

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

czeslavo
Copy link
Contributor

What this PR does / why we need it:

When a translated configuration is empty, the controller adds an empty Upstream named kong to a Kong configuration. It will happen for Gateways in DB-less mode only.

Which issue this PR fixes:

Part of #3499.

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

@czeslavo czeslavo added the area/feature New feature or request label Jul 11, 2023
@czeslavo czeslavo added this to the KIC v2.11.0 milestone Jul 11, 2023
@czeslavo czeslavo self-assigned this Jul 11, 2023
@czeslavo czeslavo marked this pull request as ready for review July 11, 2023 14:19
@czeslavo czeslavo requested a review from a team as a code owner July 11, 2023 14:19
@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Patch coverage: 100.0% and project coverage change: -0.1 ⚠️

Comparison is base (5c4d7d5) 65.0% compared to head (891e03f) 65.0%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4316     +/-   ##
=======================================
- Coverage   65.0%   65.0%   -0.1%     
=======================================
  Files        154     154             
  Lines      17382   17384      +2     
=======================================
- Hits       11310   11301      -9     
- Misses      5353    5364     +11     
  Partials     719     719             
Impacted Files Coverage Δ
internal/dataplane/deckgen/deckgen.go 52.7% <100.0%> (+4.9%) ⬆️
internal/dataplane/deckgen/generate.go 44.5% <100.0%> (+2.2%) ⬆️
internal/dataplane/kong_client.go 84.7% <100.0%> (-0.3%) ⬇️
...nal/dataplane/sendconfig/config_change_detector.go 100.0% <100.0%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

I'll always 🤨 a bit at variable renames without explanations why, but doesn't make any difference functionally. Gateway probably should report ready if it successfully receives an empty config and not require this stub config workaround, but assuming we probably can't get that, so :shipit:

@rainest rainest merged commit d85f53a into main Jul 11, 2023
30 checks passed
@rainest rainest deleted the feat/add-stub-entity branch July 11, 2023 22:24
Comment on lines +101 to +104
- When a translated Kong configuration is empty in DB-less mode, the controller
will now send the configuration with a single empty `Upstream`. This is to make
Gateways using `/status/ready` as their health check ready after receiving the
initial configuration (even if it's empty).
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind reminding me what's the reason for this? I do #3499 but as I see it now is that this will circumvent the /status/ready functionality: i.e. instead of waiting for first non-empty config we'll send the stub to make the GW ready which will serve basically nothing.

Copy link
Contributor Author

@czeslavo czeslavo Jul 12, 2023

Choose a reason for hiding this comment

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

It's to make sure a Gateway always gets ready after getting the first (empty or not) config update from KIC. Effectively, Gateways using /status/ready will stay not-ready until KIC sends the first config update to them. IMO it's expected to have KIC configure Gateways with an empty config when there's nothing in K8s API to translate and to assume these Gateways will be ready to serve traffic in such case, isn't it?

This will also ensure that a typical scenario we run in tutorials etc. is not broken:

  1. Deploy KIC + Kong using helm chart.
  2. GET /path returns 404.

Without this workaround, we'd have no Kong instances ready to serve and respond 404s.

Copy link
Member

Choose a reason for hiding this comment

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

👍

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 size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants