Skip to content

Conversation

dav3r
Copy link
Member

@dav3r dav3r commented Apr 11, 2020

🗣 Description

This PR adds a check in terraform-to-secrets to verify that a resource's tags are not equal to None. If the tags are None, it's value is changed to an empty dictionary so that the rest of the script runs correctly..

💭 Motivation and Context

I encountered a situation recently where terraform-to-secrets was failing:

❱ terraform-to-secrets -l debug -d -t <REDACTED>
2020-04-11 09:48:07,985 DEBUG Trying to determine GitHub repository name using git.
2020-04-11 09:48:08,005 INFO Using GitHub repository name: cisagov/nessus-packer
2020-04-11 09:48:08,006 INFO Reading state from Terraform command.
2020-04-11 09:48:10,603 INFO Searching Terraform state for IAM credentials.
2020-04-11 09:48:10,603 INFO Found credentials for user: test-nessus-packer
2020-04-11 09:48:10,603 INFO Searching Terraform state for tagged resources.
2020-04-11 09:48:10,603 DEBUG: resource[address]: aws_iam_policy.s3_read tags: {}
2020-04-11 09:48:10,603 DEBUG: resource[address]: aws_iam_role.s3_read tags: None
Traceback (most recent call last):
  File "development-guide/bin/terraform-to-secrets", line 7, in <module>
    exec(compile(f.read(), __file__, 'exec'))
  File "development-guide/project_setup/scripts/terraform-to-secrets", line 399, in <module>
    sys.exit(main())
  File "development-guide/project_setup/scripts/terraform-to-secrets", line 380, in main
    resource_secrets: Dict[str, str] = get_resource_secrets(terraform_state)
  File "development-guide/project_setup/scripts/terraform-to-secrets", line 246, in get_resource_secrets
    for secret_name, secret_value in parse_tagged_resources(terraform_state):
  File "development-guide/project_setup/scripts/terraform-to-secrets", line 136, in parse_tagged_resources
    secret_name: Optional[str] = tags.get(GITHUB_SECRET_NAME_TAG)
AttributeError: 'NoneType' object has no attribute 'get'

The output above shows the two additional lines of debug output that I added to illustrate the problem:

2020-04-11 09:48:10,603 DEBUG: resource[address]: aws_iam_policy.s3_read tags: {}
2020-04-11 09:48:10,603 DEBUG: resource[address]: aws_iam_role.s3_read tags: None

Note that the aws_iam_role.s3_read resource had a tags element, but its value was set to None, which caused the AttributeError seen above.

I am not sure if something in AWS or Terraform changed recently to cause this behavior or if we have just never created a role without any tags before.

The code in this PR simply does a check for this situation and if it occurs, it replaces None with an empty dictionary so that processing can continue.

🧪 Testing

I tested the fix locally and verified that the script ran successfully.

📷 Screenshots (if appropriate)

🚥 Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (causes existing functionality to change)

✅ Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@dav3r dav3r added the bugfix label Apr 11, 2020
@dav3r dav3r requested review from a team, felddy, jsf9k and mcdonnnj as code owners April 11, 2020 14:28
@dav3r dav3r self-assigned this Apr 11, 2020
Copy link
Contributor

@felddy felddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix. I wish that wasn't happening. See my comment about a few tweaks.
🐰 🥚 🌷

) -> Generator[Tuple[str, str], None, None]:
"""Search all resources for tags requesting the creation of a secret."""
for resource in find_resources(terraform_state, None):
tags: Dict[str, str] = resource["values"].get("tags", dict())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need a comment here to our future selves. I know that if I ever look at this I'll assume this is redundant code. Get get("tags", dict()) is guarding against unset keys in resource["values"] while the new if clause is guarding against tags being explicitly set to None.

It might be clearer to remove the default argument to get and just let the if block handle everything. (Still with a comment, cause I know I'll later say, "Gee, they should have used the default arg")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent suggestion @felddy. Please take a look at 9954518 and let me know if that works for you.

Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please see @felddy's comment.

@dav3r dav3r merged commit a141f44 into develop Apr 12, 2020
@dav3r dav3r deleted the bugfix/handle_tags_none branch April 12, 2020 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants