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: store and push last valid config #4205

Merged
merged 2 commits into from
Jun 28, 2023
Merged

Conversation

mlavacca
Copy link
Member

@mlavacca mlavacca commented Jun 21, 2023

What this PR does / why we need it:

Which issue this PR fixes:

Fixes #4048

Special notes for your reviewer:

E2E run with these changes: https://github.com/Kong/kubernetes-ingress-controller/actions/runs/5336108716/jobs/9670625415

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

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

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

Comparison is base (a1f712d) 62.5% compared to head (07ea022) 62.5%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4205     +/-   ##
=======================================
- Coverage   62.5%   62.5%   -0.1%     
=======================================
  Files        154     154             
  Lines      17114   17123      +9     
=======================================
+ Hits       10700   10704      +4     
- Misses      5729    5732      +3     
- Partials     685     687      +2     
Impacted Files Coverage Δ
internal/dataplane/kong_client.go 67.6% <100.0%> (+0.7%) ⬆️
internal/manager/run.go 50.1% <100.0%> (+0.1%) ⬆️

... 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.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jun 21, 2023
@mlavacca mlavacca force-pushed the mlavacca/last-valid-config branch 5 times, most recently from c902031 to 3f4c1f3 Compare June 21, 2023 16:22
@mlavacca mlavacca marked this pull request as ready for review June 21, 2023 16:22
@mlavacca mlavacca requested a review from a team as a code owner June 21, 2023 16:22
rainest
rainest previously approved these changes Jun 22, 2023
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.

This should indeed not resend configuration if a gateway already has the configuration the fallback wants to send, correct?

It looks like that's the case since PerformUpdate computes a hash from the config provided (so it does compute different hashes for the new, broken config and the fallback) rather than earlier, so we don't get a situation where we think the comparison still finds a change when sending the fallback.

We would want to avoid broken configuration resulting in a situation where we effectively enable reverse sync and always send configuration, since updates are expensive. It looks like we do indeed avoid that without any new logic to avoid it for the fallback, however, and the rest looks fine, so approving with an optional changelog addition.

CHANGELOG.md Outdated Show resolved Hide resolved
@mlavacca
Copy link
Member Author

@rainest @pmalek @mheap I've updated the CHANGELOG as Travis suggested and I think we can merge this one. My intention is to create a new issue for fetching the latest good configuration from the Proxy and tackle it in a different PR (we need to add a go-kong update as well), along with the hash thingy (fundamental when implementing the fetch from proxy feature). Are you ok with this approach?

rainest
rainest previously approved these changes Jun 26, 2023
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
@rainest rainest self-requested a review June 27, 2023 17:07
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 think this is (still) good to go but IDK if ya'll want to continue anything in the open comment threads, so approving again but leaving it up to @mlavacca to close them/merge.

@mlavacca
Copy link
Member Author

I think this is (still) good to go but IDK if ya'll want to continue anything in the open comment threads, so approving again but leaving it up to @mlavacca to close them/merge.

The thread's discussion will be addressed in a separated PR. I added some unit testing to address @czeslavo comment. Waiting for his approval.

@mheap
Copy link
Member

mheap commented Jun 28, 2023

@mflendrich Flagged some potential issues with fetching config from a running DP if running in DB-backed and using both KIC and deck. Agree to merge this PR as-is, but we should consider limiting "fetch from DP" mode to DB-less only in a future PR

@mlavacca mlavacca enabled auto-merge (squash) June 28, 2023 10:17
@mlavacca mlavacca merged commit d73df77 into main Jun 28, 2023
29 checks passed
@mlavacca mlavacca deleted the mlavacca/last-valid-config branch June 28, 2023 10: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.

Store + send last known good configuration
5 participants