Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat!: Update
aws
tov23.3.1
#264feat!: Update
aws
tov23.3.1
#264Changes from all commits
221f04d
7ad758f
eac98bf
e1018d9
482310a
b0aa0ce
303dd10
46914ba
33b2ea2
c434921
f560430
fb81760
88d0ad5
d5bfa30
e6d3f6e
7e86bb9
521a791
938b427
df1b25d
14a92a5
a8b7dc5
441e013
add1149
c7cc14b
a9df666
6cd4b81
75a11a7
1c49bc9
b7afc23
ab8b274
935a032
b705ea1
80855d1
b387f58
cce6d8b
b6fb421
fb9320e
e26d2a5
005aab9
794808f
355ed43
46441bc
a0c64e2
9c52378
191fdd2
4e5cc69
35c0fd0
bdfb813
4bdcce2
6b2f7cc
3c80494
2f9cf11
946f63f
5be873d
a90cbd7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why are we using the
request_*
values here? For services that can aggregate across region and account this can flag wouldn't this flag detectors in other accounts and regions?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.
Previously we used
account_id
®ion
columns that were propagated with the same data.We could, however, parse
account_id
®ion
values from thearn
column, but IDK if that's in the scope of this upgrade or not.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.
I might be mistaken, but I believe we used
account_id
andregion
I believe at the time of our plugin release Guard Duty didn't support cross region/ cross account aggregationThere 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.
I updated the PK in cloudquery/cloudquery#15468.
I might've misinterpreted cloudquery/cloudquery#15468 (comment) as a suggestion to include
request_
columns to the PK, as we already had ARN there...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.
The ARN should contain the region/account of the resource that actually owns the resource, while the
request_*
should contain information about where the request was made... If we didn't add therequest_*
fields to the PK it would be non-deterministic about which requests actually made it to the DBThere 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.
Judging by the docs we could parse ARN instead. Do we want this?
cc: @jsonpr
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.
Additionally, it doesn't seem that the
request_
prefix is required here: https://docs.aws.amazon.com/guardduty/latest/ug/guardduty_concepts.htmlIt seems to me that each acc/region will have its own 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.
So maybe there should be a fix moving back to
acocunt_id
®ion
+ actually including them into ARN: https://github.com/cloudquery/cloudquery/blob/main/plugins/source/aws/resources/services/guardduty/detectors.go#L97-L101