-
Notifications
You must be signed in to change notification settings - Fork 98
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
Update whitelist file with support for S3 #9
Conversation
Thank you for the contribution and we really appreciate it. We require some unit test coverage for any new PR if applicable. Could you add one like this existing one https://github.com/aws/aws-xray-sdk-java/blob/master/aws-xray-recorder-sdk-aws-sdk/src/test/java/com/amazonaws/xray/handlers/TracingHandlerTest.java#L50 which simply checks the whitelisted parameter is properly captured? (You can ignore the print statement on that test...) |
Just did a commit with 2 unit tests for s3. I also moved the http mock logic to a generic method to reduce duplicate code between the tests. |
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.
Thanks a lot for going through this long list of APIs to add this additional whitelist JSON.
I've added a couple of nice-to-have comments but will approve this change as-is as well as it is a huge value add to the SDK.
"BucketName" | ||
] | ||
}, | ||
"DeleteObjects": { |
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 would be interesting to add
"response_descriptors": {
"DeletedObjects": {
"list": true,
"get_count": true,
"rename_to": "deleted_object_count"
}
}
here.
"BucketName" | ||
] | ||
}, | ||
"ListObjectsV2": { |
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.
Would be interesting to add KeyCount
as a response_parameter
:
"response_parameters": [
"KeyCount"
]
Likely won't work on the regular ListObjects
API (non v2) because of the legacy ObjectListing
return type.
Thanks for the feedback. To generate the full list of api calls, I wrote a small class that uses reflection to get the full list. I just added this on a separate repository which I put up to help with adding whitelist parameters: https://github.com/functionalone/aws-xray-parameter-whitelist-java/blob/master/src/test/java/com/github/functionalone/xray/handlers/GenerateParameterWhitelist.java . If you think this is useful, I can add this class to this project too (probably under the test branch). |
Hi, just checking. When will this be merged in? Anything blocking the merge? |
Hi,
This the whitelist file I use to monitor s3 access. It provides information regarding the Bucket and Key being used.
Fields I've included in the request_parameters: BucketName, Key, VersionId, Prefix.
Please consider merging this in.
Best,
Guy