turbot / steampipe-plugin-aws

Use SQL to instantly query AWS resources across regions and accounts. Open source CLI. No DB required.
https://hub.steampipe.io/plugins/turbot/aws
Apache License 2.0
191 stars 103 forks source link

feat: add `tags` to `aws_backup_plan` table #2336

Closed pdecat closed 1 day ago

pdecat commented 2 weeks ago

Integration test logs

Logs ``` Add passing integration test logs here ```

Example query results

Results ``` > SELECT name, region, tags FROM aws_backup_plan WHERE name LIKE '%pdecat%' +-----------------------+-----------+-------------------+ | name | region | tags | +-----------------------+-----------+-------------------+ | test-pdecat-plan-tags | eu-west-3 | {"test":"pdecat"} | +-----------------------+-----------+-------------------+ ```
ParthaI commented 1 week ago

Hello, @pdecat,

The overall changes look great to me, and adding a common function to fetch the tag details for the aws_backup_* tables makes a lot of sense.

However, I noticed the updates to the tags column for the aws_backup_recovery_point and aws_backup_vault tables. I’m a bit concerned about the potential for mismatches in the tags or tags_src column values when no tags are associated with the resources. With the current changes, the column values will be populated as nil.

Could you please verify if there are any mismatches between the results from the latest released AWS plugin version and the changes introduced in this PR? It would be helpful to ensure there are no discrepancies, especially as our Steampipe mod for AWS Tags might be affected if the results differ. Previously, the tags column was populated as {} when no tags were associated with the resources.

Here is the query result with the AWS plugin version v1.2.0:

> select name, tags from aws_backup_vault
+--------------------------------+---------------+
| name                           | tags          |
+--------------------------------+---------------+
| Default                        | {}            |
| aws/efs/automatic-backup-vault | {}            |
| Default                        | {"foo":"bar"} |
| Default                        | {}            |
| Default                        | {}            |
| aws/efs/automatic-backup-vault | {}            |
| Default                        | {}            |
+--------------------------------+---------------+

Thank you!

pdecat commented 1 week ago

Hi @ParthaI,

I'm getting the same results for both AWS plugin v1.3.0 and from this PR's branch for aws_backup_vault and aws_backup_recovery_point when there are no tags.

aws_backup_vault returns {} for both versions of the plugin:

> select name, tags from aws_backup_vault where tags is null or tags = '{}'::jsonb
+--------------------------------+------+
| name                           | tags |
+--------------------------------+------+
| aws/efs/automatic-backup-vault | {}   |
| Default                        | {}   |
| Default                        | {}   |
| Default                        | {}   |
+--------------------------------+------+

aws_backup_recovery_point returns null for both versions of the plugin:

> select backup_vault_name, tags from aws_backup_recovery_point where tags is null or tags = '{}'::jsonb limit 10
+-------------------+--------+
| backup_vault_name | tags   |
+-------------------+--------+
| standard          | <null> |
| standard          | <null> |
| standard          | <null> |
| standard          | <null> |
| standard          | <null> |
| standard          | <null> |
| standard          | <null> |
| standard          | <null> |
| standard          | <null> |
| standard          | <null> |
+-------------------+--------+

> select backup_vault_name, tags from aws_backup_recovery_point where tags = '{}'::jsonb limit 10
+-------------------+------+
| backup_vault_name | tags |
+-------------------+------+
+-------------------+------+

For aws_backup_plan which is only available in this PR's branch, it's a mix:

> select name, tags from aws_backup_plan where tags is null or tags = '{}'::jsonb
+-------------------------------+--------+
| name                          | tags   |
+-------------------------------+--------+
| simple-plan-list              | <null> |
| myapp-dynamodb-plan           | <null> |
| aws/efs/automatic-backup-plan | {}     |
| test-database-my-team         | <null> |
| test-standard-my-team         | <null> |
+-------------------------------+--------+

Should I make it so aws_backup_recovery_point and aws_backup_plan always returns {} instead of null?

pdecat commented 1 week ago

After a bit of investigation, the issue with null values for some rows of the recovery point table comes from the check on the ARN format which is common to main and this PR's branch:

> select backup_vault_name, jsonb_typeof(tags), resource_type, recovery_point_arn, parent_recovery_point_arn from aws_backup_recovery_point where backup_vault_name = 'standard' and recovery_point_arn like 'arn:aws:ec2%' limit 10
+-------------------+--------------+---------------+--------------------------------------------------------+---------------------------+
| backup_vault_name | jsonb_typeof | resource_type | recovery_point_arn                                     | parent_recovery_point_arn |
+-------------------+--------------+---------------+--------------------------------------------------------+---------------------------+
| standard          | <null>       | EBS           | arn:aws:ec2:eu-west-3::snapshot/snap-0a0ad71026187d7ba | <null>                    |
| standard          | <null>       | EC2           | arn:aws:ec2:eu-west-3::image/ami-090515bb8325888ed     | <null>                    |
| standard          | <null>       | EBS           | arn:aws:ec2:eu-west-3::snapshot/snap-0adcb60292f0d12d9 | <null>                    |
| standard          | <null>       | EBS           | arn:aws:ec2:eu-west-3::snapshot/snap-0df994735d54709c0 | <null>                    |
| standard          | <null>       | EBS           | arn:aws:ec2:eu-west-3::snapshot/snap-077a76566a67ba491 | <null>                    |
| standard          | <null>       | EC2           | arn:aws:ec2:eu-west-3::image/ami-0ad9c3091448e9a08     | <null>                    |
| standard          | <null>       | EBS           | arn:aws:ec2:eu-west-3::snapshot/snap-0d68767726d64edc5 | <null>                    |
| standard          | <null>       | EBS           | arn:aws:ec2:eu-west-3::snapshot/snap-0fdfe125e7ac6221c | <null>                    |
| standard          | <null>       | EBS           | arn:aws:ec2:eu-west-3::snapshot/snap-01fb826bdb62d7610 | <null>                    |
| standard          | <null>       | EBS           | arn:aws:ec2:eu-west-3::snapshot/snap-09567ee4bb83be55b | <null>                    |
+-------------------+--------------+---------------+--------------------------------------------------------+---------------------------+
> select backup_vault_name, jsonb_typeof(tags), resource_type, recovery_point_arn, parent_recovery_point_arn from aws_backup_recovery_point where backup_vault_name = 'standard' and recovery_point_arn not like 'arn:aws:ec2%' limit 10
+-------------------+--------------+---------------+-------------------------------------------------------------------------------------------+---------------------------+
| backup_vault_name | jsonb_typeof | resource_type | recovery_point_arn                                                                        | parent_recovery_point_arn |
+-------------------+--------------+---------------+-------------------------------------------------------------------------------------------+---------------------------+
| standard          | object       | DynamoDB      | arn:aws:backup:eu-west-3:012345678901:recovery-point:0fce0a0b-eaf1-45af-a22f-439fd98476b3 | <null>                    |
| standard          | object       | DynamoDB      | arn:aws:backup:eu-west-3:012345678901:recovery-point:7adef0b3-5889-4463-a2e9-c4f0b076ec81 | <null>                    |
| standard          | object       | DynamoDB      | arn:aws:backup:eu-west-3:012345678901:recovery-point:19511a49-5f4d-4aa4-a84d-a0a132f4437c | <null>                    |
| standard          | object       | DynamoDB      | arn:aws:backup:eu-west-3:012345678901:recovery-point:c74f46e8-7a9c-4816-be8e-7ea6bd8e04a0 | <null>                    |
| standard          | object       | DynamoDB      | arn:aws:backup:eu-west-3:012345678901:recovery-point:b1d3d016-920f-47e9-bcdf-03777787dcb8 | <null>                    |
| standard          | object       | DynamoDB      | arn:aws:backup:eu-west-3:012345678901:recovery-point:5efbe25a-8bb7-48bc-ab3e-68dfbb559f89 | <null>                    |
| standard          | object       | DynamoDB      | arn:aws:backup:eu-west-3:012345678901:recovery-point:47c23cdc-3fab-4946-bfe7-3cec0427cefc | <null>                    |
| standard          | object       | DynamoDB      | arn:aws:backup:eu-west-3:012345678901:recovery-point:2cdb6da5-d89a-41ee-8771-5a8d1399f3f1 | <null>                    |
| standard          | object       | DynamoDB      | arn:aws:backup:eu-west-3:012345678901:recovery-point:bfdbfbe4-c4c7-4071-b6f9-79d42c235981 | <null>                    |
| standard          | object       | DynamoDB      | arn:aws:backup:eu-west-3:012345678901:recovery-point:f6128d1f-4ee9-45bc-8c00-7b7c1ee401a8 | <null>                    |
+-------------------+--------------+---------------+-------------------------------------------------------------------------------------------+---------------------------+

This seems to correlate some comments in the code of that table mentioning this SDK issue: https://github.com/aws/aws-sdk-go-v2/issues/1912

cbruno10 commented 6 days ago

@ParthaI @pdecat - I can't recall exactly what the other tables' tags columns return if there are no tags set, but in general, this table's behaviour should be consistent with those, especially because we use common queries in mods like the AWS Tags mod.

pdecat commented 6 days ago

Ok, so I'll fix the code to always returns {} instead of null if tags are missing (all tables) or cannot be fetched for some recovery points.

pdecat commented 6 days ago

Turns out removing the ARN format check allows to retrieve tags for all recovery points.

misraved commented 1 day ago

@ParthaI to add tags_src column to aws_backup_* tables.

pdecat commented 1 day ago

@ParthaI to add tags_src column to aws_backup_* tables.

Oh, I did not get that this was requested. I can add those if you want.

ParthaI commented 1 day ago

Hi @misraved,

I believe the tags_src column may not be necessary for the aws_backup_* tables.

According to our table development standards:

type TagOutput struct {
  Tags []Tag
}

type Tag struct {
  // The key of the tag.
  Key *string

  // The value of the tag.
  Value *string
}

This type does not align with the map[string]string structure required for the tags column.

Since the API already provides the tag data directly in the form of a map[string]string, the inclusion of the tags_src column seems redundant in this case. Let me know if you think otherwise or have any concerns.

Thank you!