-
Notifications
You must be signed in to change notification settings - Fork 1
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
to v23.3.1
#264
Merged
kodiakhq
merged 55 commits into
main
from
fix/aws_cloudtrail_trail_event_selectors/v23-schema
Dec 28, 2023
+404
−212
Merged
Changes from 1 commit
Commits
Show all changes
55 commits
Select commit
Hold shift + click to select a range
221f04d
feat: Update `aws_cloudtrail_trail_event_selectors` schema
candiduslynx 7ad758f
Merge branch 'main' into fix/aws_cloudtrail_trail_event_selectors/v23…
candiduslynx eac98bf
bump aws in data-resilience
candiduslynx e1018d9
Merge branch 'main' into fix/aws_cloudtrail_trail_event_selectors/v23…
candiduslynx 482310a
bump aws in asset-free
candiduslynx b0aa0ce
bump aws-pg in compliance free
candiduslynx 303dd10
bumop aws in compliance-free-bq
candiduslynx 46914ba
bump aws in compliance-free snowflake
candiduslynx 33b2ea2
Update bigquery.yml
candiduslynx c434921
Update postgres.yml
candiduslynx f560430
Update snowflake.yml
candiduslynx fb81760
Update postgres.yml
candiduslynx 88d0ad5
Update postgres.yml
candiduslynx d5bfa30
Update bucket_access_logging.sql
candiduslynx e6d3f6e
Update detector_enabled.sql
candiduslynx 7e86bb9
Update detector_enabled.sql
candiduslynx 521a791
Update detector_enabled.sql
candiduslynx 938b427
Update unused_directconntect_connections.sql
candiduslynx df1b25d
Update snowflake.yml
candiduslynx 14a92a5
Update snowflake.yml
candiduslynx a8b7dc5
Update snowflake.yml
candiduslynx 441e013
Update transformations/aws/compliance-free/tests/snowflake.yml
candiduslynx add1149
force migration inf compliance-free/bq
candiduslynx c7cc14b
upd sf for compliance-premium
candiduslynx a9df666
use request_ cols
candiduslynx 6cd4b81
use aws_iam_policy_versions
candiduslynx 75a11a7
use aws_s3_bucket_policies
candiduslynx 1c49bc9
Merge branch 'main' into fix/aws_cloudtrail_trail_event_selectors/v23…
candiduslynx b7afc23
aws_s3_buckets relations
candiduslynx ab8b274
aws_s3_bucket_replications
candiduslynx 935a032
aws_s3_bucket_policies
candiduslynx b705ea1
rm extra forced mode
candiduslynx 80855d1
cloudtrail/bucket_access_logging:snowflake
candiduslynx b387f58
cloudtrail/enabled_in_all_regions:snowflake
candiduslynx cce6d8b
Updated log_metric and enabled_in_all_regions
ronsh12 b6fb421
change ref to aws@v23.2.0
candiduslynx fb9320e
Updated queries - no_star, policies_have_wildcard_actions, policies_w…
ronsh12 e26d2a5
Merge branch 'main' into fix/aws_cloudtrail_trail_event_selectors/v23…
candiduslynx 005aab9
force migrate for tests
candiduslynx 794808f
use v23.3.0
candiduslynx 355ed43
use v23.3.0
candiduslynx 46441bc
Revert "force migrate for tests"
candiduslynx a0c64e2
Updated queries cloudtrail_enabled_all_regions, bucket_access_logging
ronsh12 9c52378
Updated manifest compliances
ronsh12 191fdd2
Updated queries elastic_beanstalk_stream_logs_to_cloudwatch, s3_bucke…
ronsh12 4e5cc69
check query
ronsh12 35c0fd0
Update aws to `v23.3.1`
candiduslynx bdfb813
Updated query s3_bucket_logging_enabled
ronsh12 4bdcce2
Update transformations/aws/compliance-premium/tests/snowflake.yml
candiduslynx 6b2f7cc
Updated query elastic_beanstalk_stream_logs_to_cloudwatch
ronsh12 3c80494
Update transformations/aws/compliance-premium/tests/snowflake.yml
candiduslynx 2f9cf11
Merge branch 'main' into fix/aws_cloudtrail_trail_event_selectors/v23…
candiduslynx 946f63f
tmp force migration
candiduslynx 5be873d
no forced migration
candiduslynx a90cbd7
Merge branch 'main' into fix/aws_cloudtrail_trail_event_selectors/v23…
kodiakhq[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Update detector_enabled.sql
commit e6d3f6e05cb5c5f001cbe2270deb46846256309e
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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