Skip to content

Conversation

@cheran-senthil
Copy link
Contributor

@cheran-senthil cheran-senthil commented Oct 28, 2021

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.

Copy link
Contributor

@csjiang csjiang left a 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.

@cheran-senthil
Copy link
Contributor Author

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")
Copy link
Contributor

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

Copy link
Contributor Author

@cheran-senthil cheran-senthil Nov 2, 2021

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

@cheran-senthil cheran-senthil merged commit 695454d into master Nov 2, 2021
@cheran-senthil cheran-senthil deleted the vss-openresty-check-fix branch November 2, 2021 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants