-
Notifications
You must be signed in to change notification settings - Fork 712
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
Conditional report censoring #3571
Conversation
1bb2b03
to
eb69f6a
Compare
832c271
to
3c5320e
Compare
ac4b6b8
to
353ab75
Compare
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 looks really good to me 💯
I know there have been some concerns about performance when manipulating reports etc, but perhaps that was more about JSON which is a bit unrelated. We can keep an scope resource usage on dev.
Tested it locally w/
diff --git a/client/app/scripts/utils/web-api-utils.js b/client/app/scripts/utils/web-api-utils.js
index 7cbd1ff1..df72a881 100644
--- a/client/app/scripts/utils/web-api-utils.js
+++ b/client/app/scripts/utils/web-api-utils.js
@@ -51,6 +51,8 @@ let continuePolling = true;
export function buildUrlQuery(params = makeMap(), state) {
// Attach the time travel timestamp to every request to the backend.
params = params.set('timestamp', state.get('pausedAt'));
+ params = params.set('hideCommandLineArguments', 'true');
+ params = params.set('hideEnvironmentVariables', 'true');
// Ignore the entries with values `null` or `undefined`.
return params.map((value, param) => {
and works great w/ xhr/ws. Nice one!
metadata := []report.MetadataRow{} | ||
for _, row := range s.Metadata { | ||
if report.IsCommandEntry(row.ID) { | ||
row.Value = report.StripCommandArgs(row.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.
Is this mutating the existing row?
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.
It shouldn't since:
range
in the loop above returns a copy ofrow
and we're changing that copy.- The test is passing and normally it should fail if any of these operations are mutating since we're not resetting the test set between test cases for this test.
While I was looking at the code, I noticed that reports normally get copied/transformed a few times until they get to JSON. My bet would be that most of the performance drags would be either due to the lowest level data structures or indeed sending through the JSON. Having said that, we might expect a certain resource usage percentage bump, so it's probably a good idea to track it. |
Support for hiding certain parts of report in API responses through query params:
hideCommandLineArguments
- hides the command line arguments for all nodes in the specific request (only matters ifnoCommandLineArguments
if setfalse
on probe) - defaultfalse
hideEnvironmentVariables
- hides the environment variables for all nodes in the specific request (only matters ifnoEnvironmentVariables
if setfalse
on probe) - defaultfalse
All the relevant API endpoints exposing any node states will now support these query params:
/api/report
/api/topology/[TOPOLOGY_ID]
/api/topology/[TOPOLOGY_ID]/ws
/api/topology/[TOPOLOGY_ID]/[NODE_ID]
It's a step in enabling finer control of information exposed through the API.
Unfortunately, the censoring logic needed to be implemented both on the report level (for
/api/report
endpoint) and the rendering level (for all the other endpoints, to be as close to the response level as possible to avoid renderers adding their own sensitive data to the reports).To test this PR, probe should report both the env vars and cmd args so make sure to run it via
scope launch --probe.omit.env-vars=false --probe.omit.cmd-args=false
.