feat(aws): Add PolicyStatus to WrappedBucket #11657
Conversation
This PR has the following changes to source plugin(s) tables:
|
tsutsun17
changed the title
tsutsun17
changed the title
tsutsun17
changed the title
hermanschaaf
left a comment
hermanschaaf
left a comment
There was a problem hiding this comment.
Hi @tsutsun17, thanks so much for this PR. What you did here looks good, but I have one request if you don't mind, outlined in the comment (and I tried to provide the reasoning).
| LoggingTargetBucket *string | ||
| LoggingTargetPrefix *string | ||
| Policy map[string]any | ||
| IsPublicByPolicy bool |
There was a problem hiding this comment.
Could we please change this to contain the entire PolicyStatus object, so it gets stored as a JSON column called policy_status?
I'm aware this may seem like an odd request. This will mean queries will need to be written as policy_status->is_public instead of is_public_by_policy, but it conforms better to our current standards, which are:
- stick as closely as possible to the original AWS SDK method / CLI command names
- store nested objects as JSON columns rather than unnesting them
Both these are in place so that it's easier to map from the AWS API to CloudQuery table columns.
The S3 table happens to be one of our oldest tables, so for backwards-compatibility reasons we haven't updated all columns to these standards yet, and what you've done here is totally reasonable when looking at the other columns in the table. But I think for setting the precedent, new columns we should try and follow the rules above, which would mean storing this additional information as a JSON column. One other advantage of this approach is also that, if AWS were ever to add new fields to that API response, they will automatically be included when we upgrade the AWS SDK version without requiring a manual update here.
There was a problem hiding this comment.
Thank you for review and I got it!
if AWS were ever to add new fields to that API response, they will automatically be included when we upgrade the AWS SDK version without requiring a manual update here.
It means I should not use new struct, but use map[string]any, right?
so I'll change it as below.
PolicyStatus map[string]any <- the result of `json.Unmarshal`There was a problem hiding this comment.
@tsutsun17 I think you should be able to do:
PolicyStatus *types.PolicyStatus
and set with:
resource.PolicyStatus = policyStatusOutput.PolicyStatus
Worth testing this though of course :)
There was a problem hiding this comment.
I see :)
I'll try them !
| |logging_target_bucket|`utf8`| | ||
| |logging_target_prefix|`utf8`| | ||
| |policy|`json`| | ||
| |policy_status|`json`| |
There was a problem hiding this comment.
Should I describe the detail of json structure? (e.g. it contains is_public)
if it needs, where should I write?
There was a problem hiding this comment.
@tsutsun17 You don't have to, this is a limitation of our currently generated documentation that we are planning to address in an automated way :)
tsutsun17
changed the title
🤖 I have created a release *beep* *boop* --- ## [19.0.0](plugins-source-aws-v18.4.0...plugins-source-aws-v19.0.0) (2023-06-20) ### ⚠ BREAKING CHANGES * **aws:** Move `web_acl_arn` to its own table ([#11421](#11421)) * **aws:** Add support For fully describing the organizational hierarchy ([#11633](#11633)) * **aws:** Support all policy types in Identity Center ([#10985](#10985)) ### This Release has the Following Changes to Tables - Table `aws_cloudfront_functions` was added - Table `aws_cloudfront_origin_access_identities` was added - Table `aws_cloudfront_origin_request_policies` was added - Table `aws_cloudfront_response_headers_policies` was added - Table `aws_cloudtrail_channels` was added - Table `aws_cloudtrail_imports` was added - Table `aws_ec2_capacity_reservations` was added - Table `aws_elbv2_load_balancer_web_acls` was added - Table `aws_elbv2_load_balancers`: column `web_acl_arn` removed from table (:warning: breaking) - Table `aws_organizations_account_parents` was added - Table `aws_organizations_organizational_unit_parents` was added - Table `aws_organizations_organizational_units`: column `account_id` removed from table (:warning: breaking) - Table `aws_organizations_organizational_units`: column added with name `request_account_id (PK)` and type `utf8` (:warning: breaking) - Table `aws_s3_buckets`: column added with name `policy_status` and type `json` - Table `aws_ssoadmin_account_assignments` was removed (:warning: breaking) - Table `aws_ssoadmin_instances`: primary key constraint added to column `instance_arn` (:warning: breaking) - Table `aws_ssoadmin_instances`: primary key constraint removed from column `_cq_id` (:warning: breaking) - Table `aws_ssoadmin_permission_set_account_assignments` was added - Table `aws_ssoadmin_permission_set_customer_managed_policies` was added - Table `aws_ssoadmin_permission_set_inline_policies` was added - Table `aws_ssoadmin_permission_set_managed_policies` was added - Table `aws_ssoadmin_permission_set_permissions_boundaries` was added - Table `aws_ssoadmin_permission_sets`: column `inline_policy` removed from table (:warning: breaking) - Table `aws_ssoadmin_permission_sets`: column added with name `instance_arn (PK)` and type `utf8` (:warning: breaking) - Table `aws_ssoadmin_permission_sets`: primary key constraint added to column `permission_set_arn` (:warning: breaking) - Table `aws_ssoadmin_permission_sets`: primary key constraint removed from column `_cq_id` (:warning: breaking) ### Features * **aws-services:** Support newly added regions ([#11673](#11673)) ([8c0ab9d](8c0ab9d)) * **aws:** Add PolicyStatus to WrappedBucket ([#11657](#11657)) ([55d966a](55d966a)) * **aws:** Add support for Cloudfront Functions ([#11669](#11669)) ([102067a](102067a)) * **aws:** Add Support for Cloudtrail Channels ([#11670](#11670)) ([0dc13de](0dc13de)) * **aws:** Add Support for Cloudtrail Imports ([#11671](#11671)) ([c908289](c908289)) * **aws:** Add support for EC2 Capacity Reservations ([#11666](#11666)) ([70d6052](70d6052)) * **aws:** Add support For fully describing the organizational hierarchy ([#11633](#11633)) ([f66995b](f66995b)) * **aws:** Add Support for more Cloudfront Resources ([#11668](#11668)) ([52e6ad9](52e6ad9)) * **aws:** Support all policy types in Identity Center ([#10985](#10985)) ([a8ab255](a8ab255)) ### Bug Fixes * **aws:** Move `web_acl_arn` to its own table ([#11421](#11421)) ([cdda682](cdda682)) * **deps:** Update github.com/cloudquery/arrow/go/v13 digest to 1e68c51 ([#11637](#11637)) ([46043bc](46043bc)) * **deps:** Update github.com/cloudquery/arrow/go/v13 digest to 43638cb ([#11672](#11672)) ([3c60bbb](3c60bbb)) * **deps:** Update github.com/cloudquery/arrow/go/v13 digest to b0832be ([#11651](#11651)) ([71e8c29](71e8c29)) * **deps:** Update module github.com/aws/aws-sdk-go-v2 to v1.18.1 ([#11652](#11652)) ([4230b52](4230b52)) * **deps:** Update module github.com/aws/aws-sdk-go-v2/config to v1.18.27 ([#11653](#11653)) ([4b45408](4b45408)) * **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.1.0 ([#11665](#11665)) ([d8947c9](d8947c9)) * Use ServiceAccountRegion multiplexer for aws_availability_zones ([#11686](#11686)) ([7f4788f](7f4788f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
4 participants
Summary
Thank you for implementing this amazing project.
I added
is_public_by_policypolicy_statustoWrappedBucket.Though AWS shows the criteria (criteria), It is difficult for me to conclude whether a s3 bucket which uses bucket policy is public or not.
so I suggest the way using
GetBucketPolicyStatusAPI in this PR.make lintto ensure the proposed changes follow the coding style 🚨 (install golangci-lint here)make testto ensure the proposed changes pass the tests 🧪make gento ensure docs are up to date 📝