-
Notifications
You must be signed in to change notification settings - Fork 24
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
ROX-26516: Introspection endpoint for runtime config #1879
ROX-26516: Introspection endpoint for runtime config #1879
Conversation
02326d9
to
691f393
Compare
I believe that the builder regeneration logic bases its decision on modifications that are part of the PR, which is incorrect here (the base PR modifies builder). Added the |
CollectorConfigInspector::CollectorConfigInspector(const std::shared_ptr<CollectorConfig> config) : config_(config) { | ||
} | ||
|
||
Json::Value CollectorConfigInspector::configToJson() { |
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.
There are a few situations in this method that could be considered errors, like failing to convert the configuration to JSON, it'd be nice if we could change the HTTP status to 500 if one of these occur, maybe we can return a tuple with the result and the json? Or use a std::variant
of sorts?
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.
Done
Json::CharReaderBuilder readerBuilder; | ||
std::string errs; | ||
std::istringstream iss(jsonString); | ||
if (!Json::parseFromStream(readerBuilder, iss, &root, &errs)) { | ||
CLOG(ERROR) << "Failed to parse JSON string: " << errs; | ||
return Json::Value(); | ||
} |
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.
If the caller to this method is just going to grab the Json::Value
object and turn it into a string, why are we grabbing a string and turning it into a Json::Value
here? Why can't we just return the string?
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.
Done
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.
Just some minor nitpicks, but otherwise LGTM!
Co-authored-by: Olivier Valentin <[email protected]> Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]>
Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]>
1aecc1f
to
78fde17
Compare
Description
Makes it possible to find the current state of the runtime configuration by querying a collector endpoint.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Deployed ACS on a GKE cluster. Changed the collector image to the one for this PR. Also added the following environment variable
Ran the following command
Queried the endpoint
and got this response
Set enableExternalIps to false
The result in this case was
Not using a ConfigMap
In this case the response was