-
Notifications
You must be signed in to change notification settings - Fork 430
feat(aws): Add GuardDutyDetector to AWS module #1765
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
base: master
Are you sure you want to change the base?
feat(aws): Add GuardDutyDetector to AWS module #1765
Conversation
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.
cubic analysis
1 issue found across 6 files • Review in cubic
React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai
to give feedback, ask questions, or re-run the review.
4bb553d
to
11937a4
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.
Thanks for the contribution! Left a few suggestions — feel free to make these updates while waiting for the final review.
@d-aggarwal @achantavy , PTAL! I have made some updates. |
Signed-off-by: Priyanshu Harshbodhi <[email protected]>
Signed-off-by: Priyanshu Harshbodhi <[email protected]>
Signed-off-by: Priyanshu Harshbodhi <[email protected]>
Signed-off-by: Priyanshu Harshbodhi <[email protected]>
Signed-off-by: Priyanshu Harshbodhi <[email protected]>
Signed-off-by: Priyanshu Harshbodhi <[email protected]>
Signed-off-by: Priyanshu Harshbodhi <[email protected]>
Signed-off-by: Priyanshu Harshbodhi <[email protected]>
Signed-off-by: Priyanshu Harshbodhi <[email protected]>
- Remove try/except block from get_detector_details (decorator handles exceptions) - Move ARN construction logic from load to transform function - Update transform_detector_details to accept aws_account_id parameter - Remove separate detectors.json file as suggested - Update tests to work with new function signatures Signed-off-by: Priyanshu Harshbodhi <[email protected]>
24ac22f
to
9840942
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.
Thanks for the help, almost there, please add integration tests and if possible a console log showing that you ran this. Let us know if you are not able to run this yourself and we'll find another way to test.
detector_details = get_detector_details(boto3_session, region, detector_ids) | ||
transformed_detectors = transform_detector_details( | ||
detector_details, region, current_aws_account_id | ||
) | ||
load_guardduty_detectors( | ||
neo4j_session, | ||
transformed_detectors, | ||
region, | ||
current_aws_account_id, | ||
update_tag, | ||
) |
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.
Let's move this out to a sync_detectors_function()
and then call it from within sync()
.
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.
oh oops meant to actually hit 'request changes'
- Extract sync_detectors_function() - Add comprehensive integration tests - All tests passing for both unit and integration coverage Signed-off-by: Priyanshu Harshbodhi <[email protected]>
Signed-off-by: Priyanshu Harshbodhi <[email protected]>
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.
Hey, looking forward to having this merged in. The tests are failing, can you please address? Also left a comment on removing an overly verbose test
# Test outcomes - verify ARN construction is correct | ||
detectors = neo4j_session.run( | ||
"MATCH (d:GuardDutyDetector) RETURN d.detector_id as detector_id, d.arn as arn" | ||
).data() | ||
|
||
for detector in detectors: | ||
expected_arn = f"arn:aws:guardduty:{TEST_REGION}:{TEST_ACCOUNT_ID}:detector/{detector['detector_id']}" | ||
assert ( | ||
detector["arn"] == expected_arn | ||
), f"ARN mismatch for detector {detector['detector_id']}" |
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.
Remove this test. It's overly verbose and doesn't use the check_nodes()
pattern
Summary
This PR adds GuardDuty Detector support to AWS modules
Related issues or links
(#1477)
Checklist
Provide proof that this works (this makes reviews move faster). Please perform one or more of the following:
If you are changing a node or relationship:
If you are implementing a new intel module: