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 configuration_hash in /status in dbless #8214

Merged
merged 5 commits into from
Feb 10, 2022

Conversation

shaneutt
Copy link
Contributor

Summary

In #8152 it was reported that if the Kong Gateway crashes the KIC will not be aware and unless something changes in the control plane, it will not push a new configuration.

This patch adds a configuration_hash to the /status endpoint, which would help solve this problem (in DBLESS mode) by allowing the control-plane to keep track of changes to the hash on the data-plane and use that to make decisions about pushing new /config updates.

Issues resolved

Partially resolves Kong/kubernetes-ingress-controller#2107

@CLAassistant
Copy link

CLAassistant commented Dec 17, 2021

CLA assistant check
All committers have signed the CLA.

@shaneutt
Copy link
Contributor Author

shaneutt commented Dec 17, 2021

I tested this locally and it worked properly: I was able to verify that the hash and empty status message was populated only in dbless mode.

However there was some kind of issue with the #off strategy which I may need help with: it seemed as if the #off strategy did not turn on dbless mode because the /status output did not include the field when running the tests (but again, it did work when I did a manual test).

EDIT: well it ended up working in CI so perhaps it was a local environment issue?

@shaneutt shaneutt force-pushed the shaneutt/configuration-hash-dbless-status branch from d416f7d to 9b9bf79 Compare December 17, 2021 23:03
@dndx dndx added the core/db label Dec 22, 2021
@shaneutt
Copy link
Contributor Author

shaneutt commented Jan 3, 2022

Closing this in favor of #8256

@shaneutt shaneutt closed this Jan 3, 2022
@shaneutt shaneutt deleted the shaneutt/configuration-hash-dbless-status branch January 3, 2022 15:06
@shaneutt shaneutt restored the shaneutt/configuration-hash-dbless-status branch January 19, 2022 18:36
@shaneutt
Copy link
Contributor Author

@dndx suggested may still want this implementation for @Kong/team-k8s, re-opening.

@shaneutt shaneutt reopened this Jan 19, 2022
Copy link
Member

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

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

+1 from an API perspective

kong/api/routes/health.lua Show resolved Hide resolved
spec/02-integration/04-admin_api/02-kong_routes_spec.lua Outdated Show resolved Hide resolved
@shaneutt shaneutt force-pushed the shaneutt/configuration-hash-dbless-status branch 2 times, most recently from 1acca49 to ed05a1d Compare January 24, 2022 20:25
@shaneutt shaneutt marked this pull request as ready for review January 24, 2022 20:35
@shaneutt shaneutt requested a review from hbagdi January 24, 2022 20:35
@shaneutt shaneutt force-pushed the shaneutt/configuration-hash-dbless-status branch 4 times, most recently from 2b699c0 to 54f5a16 Compare January 25, 2022 16:13
@shaneutt shaneutt requested a review from hbagdi January 25, 2022 16:14
@shaneutt shaneutt force-pushed the shaneutt/configuration-hash-dbless-status branch from 54f5a16 to 859d81b Compare January 25, 2022 20:23
@shaneutt shaneutt force-pushed the shaneutt/configuration-hash-dbless-status branch 4 times, most recently from 0aa7eb9 to 5f8b296 Compare January 26, 2022 16:31
@shaneutt shaneutt requested a review from dndx January 26, 2022 17:03
@hbagdi
Copy link
Member

hbagdi commented Jan 26, 2022

The code should add a test to assert that the hash is populated correctly on the Admin API /status endpoint as well (the code tests for Status API /status).
KIC plans to use Admin API so if there is a regression here, it would be problematic for you, won't it?

@shaneutt shaneutt force-pushed the shaneutt/configuration-hash-dbless-status branch from f8e18cc to d4e27bc Compare January 26, 2022 20:22
Copy link
Member

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

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

This looks good from an API perspective.
Someone in team gateway who has more familiarity with this code can review this further and approve.

spec/02-integration/08-status_api/01-core_routes_spec.lua Outdated Show resolved Hide resolved
spec/02-integration/08-status_api/01-core_routes_spec.lua Outdated Show resolved Hide resolved
spec/02-integration/08-status_api/01-core_routes_spec.lua Outdated Show resolved Hide resolved
shaneutt and others added 3 commits January 26, 2022 16:23
Co-authored-by: Harry <harrybagdi@gmail.com>
Co-authored-by: Harry <harrybagdi@gmail.com>
Co-authored-by: Harry <harrybagdi@gmail.com>
@shaneutt shaneutt requested a review from hbagdi January 26, 2022 21:31
@RobSerafini
Copy link
Contributor

RobSerafini commented Feb 10, 2022

@shaneutt @locao - It looks like this PR is ready to be merged. Any reason why it is not?

@locao locao merged commit 4f8c99d into Kong:master Feb 10, 2022
@locao locao deleted the shaneutt/configuration-hash-dbless-status branch February 10, 2022 19:26
@locao
Copy link
Contributor

locao commented Feb 10, 2022

Merged! Thanks for pointing out, @RobSerafini 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ingress controller leaves replacement proxy containers without pushed configuration
6 participants