-
Notifications
You must be signed in to change notification settings - Fork 123
Prevent randomness in health check endpoint #2013
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
Conversation
csjiang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed in slack, the cjson lib was causing randomness in the ordering of keys on the data object in response. i suggested, rather than encode the values themselves and then reencode the entire object, perhaps we could try a different library? we consulted links https://github.com/rxi/json.lua and http://lua-users.org/wiki/JsonModules. my reasoning is, since this is logic that runs on every single request, might be worth trying to optimize/keep the code path clean. i think it's worth trying at least json.lua and then falling back to this implementation if that doesn't solve our issue.
|
The main problem is with installing the package in this case, since using luarocks is discouraged, https://openresty.org/en/using-luarocks.html Though there are other json packages in the standard package manager, those are less production-ready than cjson. Therefore, I think we should proceed with cjson for this. |
| def get_openresty_public_key(): | ||
| """Get public key for openresty if it is running""" | ||
| try: | ||
| resp = requests.get("http://localhost:5000/openresty_pubkey") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should never change after a container comes up right? and openresty should always boot up first before web server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right, this function is to just keep the code for this isolated
Description
This fixes the randomness observed in the JSON response of the DP health check
Tests
Tested on Sandbox
How will this change be monitored?
This is a non critical change, and can be seen on the health check endpoint.