Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix: create records using for_each instead of count #37

Merged
merged 7 commits into from
Jan 27, 2022
Merged

Fix: create records using for_each instead of count #37

merged 7 commits into from
Jan 27, 2022

Conversation

1david5
Copy link
Contributor

@1david5 1david5 commented Jan 26, 2022

what

  • Modify default and ipv6 aws_route53_record resources to use for_each instead of count.

why

  • Prevent destroying and recreating DNS records when removing elements from aliases list.

@1david5 1david5 requested review from a team as code owners January 26, 2022 17:43
bridgecrew[bot]
bridgecrew bot reviewed Jan 26, 2022
View reviewed changes
Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

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

Bridgecrew has found infrastructure configuration errors in this PR ⬇️

main.tf Outdated
@@ -6,9 +6,9 @@ data "aws_route53_zone" "default" {
}

resource "aws_route53_record" "default" {
count = module.this.enabled ? length(compact(var.aliases)) : 0
for_each = module.this.enabled ? toset(compact(var.aliases)) : []
Copy link

@bridgecrew bridgecrew bot Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

MEDIUM   Ensure Route53 A Record has an attached resource
    Resource: aws_route53_record.default | ID: BC_AWS_GENERAL_95

Description

This check ensures that Route53 A records point to resources part of your Account rather than just random IP addresses. On the platform this check additionally compares IP's against provisioned EIP. In Checkov the graph correlates the A record against know AWS resources from EIP to Global Accelerator.

main.tf Outdated
@@ -6,9 +6,9 @@ data "aws_route53_zone" "default" {
}

resource "aws_route53_record" "default" {
count = module.this.enabled ? length(compact(var.aliases)) : 0
for_each = module.this.enabled ? toset(compact(var.aliases)) : []
Copy link

@bridgecrew bridgecrew bot Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

HIGH   Ensure Route 53 entries relate to account resources only
    Resource: aws_route53_record.default | ID: BC_AWS_NETWORKING_60

Description

Checks that all A records in Route 53 point to resources created in the current AWS account.

#Rationale
A check to protect against domain hijacking, where an unrelated IP address is added to an AWS managed DNS zone.

@1david5 1david5 requested a review from a team as a code owner January 26, 2022 17:44
korenyoni
korenyoni previously approved these changes Jan 27, 2022
View reviewed changes
Copy link
Member

@korenyoni korenyoni left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@korenyoni korenyoni changed the title create records using for_each instead of count Fix: create records using for_each instead of count Jan 27, 2022
@korenyoni korenyoni added bug 🐛 An issue with the system minor New features that do not break anything and removed bug 🐛 An issue with the system labels Jan 27, 2022
@korenyoni
Copy link
Member

/test all

@mergify mergify bot dismissed korenyoni’s stale review January 27, 2022 21:01

This Pull Request has been updated, so we're dismissing all reviews.

bridgecrew[bot]
bridgecrew bot reviewed Jan 27, 2022
View reviewed changes
main.tf Outdated Show resolved Hide resolved
bridgecrew[bot]
bridgecrew bot reviewed Jan 27, 2022
View reviewed changes
main.tf Outdated Show resolved Hide resolved
bridgecrew[bot]
bridgecrew bot reviewed Jan 27, 2022
View reviewed changes
Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

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

⚠️   Due to 7c53db1 - Merge branch 'master' into use-for_each - 2 new errors were added

Change details

Error ID Change Path Resource
BC_AWS_NETWORKING_60 Added /main.tf aws_route53_record.default
BC_AWS_GENERAL_95 Added /main.tf aws_route53_record.default

@korenyoni korenyoni mentioned this pull request Jan 27, 2022
Merged
@mergify
Copy link

mergify bot commented Jan 27, 2022

This pull request is now in conflict. Could you fix it @1david5? 🙏

@korenyoni
Copy link
Member

Hope you don't mind I take this opportunity to update the contributors list in README.md (via README.yaml) 😄

@korenyoni
Copy link
Member

/test all

korenyoni
korenyoni approved these changes Jan 27, 2022
View reviewed changes
Copy link
Member

@korenyoni korenyoni left a comment

Choose a reason for hiding this comment

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

LGTM

@korenyoni korenyoni added the no-release Do not create a new release (wait for additional code changes) label Jan 27, 2022
@korenyoni
Copy link
Member

Note that this is a minor release because it will cause destruction and recreation of resources due to the address change.

@korenyoni korenyoni merged commit c88d5bc into cloudposse:master Jan 27, 2022
@1david5
Copy link
Contributor Author

1david5 commented Jan 27, 2022

Perfect. thank you @korenyoni 👍

@1david5 1david5 deleted the use-for_each branch January 27, 2022 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridgecrew bridgecrew[bot] bridgecrew[bot] left review comments

@korenyoni korenyoni korenyoni approved these changes

@nitrocode nitrocode Awaiting requested review from nitrocode nitrocode was automatically assigned from cloudposse/contributors

@Benbentwo Benbentwo Awaiting requested review from Benbentwo Benbentwo was automatically assigned from cloudposse/contributors

Assignees
No one assigned
Labels
bug 🐛 An issue with the system minor New features that do not break anything no-release Do not create a new release (wait for additional code changes)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@1david5 @korenyoni @cloudpossebot