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.

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: Add external_aliases Which Will Not Have CNAMEs Created for Them #199

Merged
merged 12 commits into from
Dec 14, 2021
Merged

Conversation

mburns
Copy link
Contributor

@mburns mburns commented Dec 1, 2021

what

  • Allow for aliases in CloudFront which do not get the corresponding CNAME record created in Route53.
  • Misc: add BridgeCrew suppressions.

why

  • Not all CloudFront domains are managed in Route53
  • This is similar to dns_alias_enabled, but allows for a mixture of DNS providers for a single CloudFront Distribution
  • Some new false positives were raised by BridgeCrew, hence requiring some suppressions.

@mburns mburns requested review from a team as code owners December 1, 2021 23:59
@mburns mburns requested review from max-lobur and brcnblc and removed request for a team December 1, 2021 23:59
main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
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 ba35bc0 - Auto Format - 4 new errors were added

Change details

Error ID Change Path Resource
BC_AWS_GENERAL_27 Added /main.tf aws_cloudfront_distribution.default
BC_AWS_GENERAL_56 Added /main.tf aws_s3_bucket.origin
BC_AWS_NETWORKING_52 Added /main.tf aws_s3_bucket.origin
BC_AWS_GENERAL_72 Added /main.tf aws_s3_bucket.origin

@korenyoni
Copy link
Member

@mburns are you able to please add BridgeCrew exceptions for the failing checks outlined in this PR?

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. But please see comments regarding BridgeCrew.

main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
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 a0af986 - Merge branch 'master' of github.com:mburns/terraform-aws-cloudfront-s3-cdn - 4 errors were fixed.

Change details

Error ID Change Path Resource
BC_AWS_GENERAL_27 Fixed /main.tf aws_cloudfront_distribution.default
BC_AWS_GENERAL_56 Fixed /main.tf aws_s3_bucket.origin
BC_AWS_NETWORKING_52 Fixed /main.tf aws_s3_bucket.origin
BC_AWS_GENERAL_72 Fixed /main.tf aws_s3_bucket.origin

@mergify mergify bot dismissed korenyoni’s stale review December 13, 2021 21:14

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

@korenyoni
Copy link
Member

korenyoni commented Dec 13, 2021

Hey @mburns — I see that you reverted the BridgeCrew supressions, was there a particular reason why?

@mburns
Copy link
Contributor Author

mburns commented Dec 13, 2021

HI @korenyoni, just troubleshooting, sorry. It looked like the errors were fixed just by merging the latest master. I'll re-apply the change.

main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
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 1f8eb65 - Merge branch 'master' of github.com:mburns/terraform-aws-cloudfront-s3-cdn - 4 errors were fixed.

Change details

Error ID Change Path Resource
BC_AWS_GENERAL_27 Fixed /main.tf aws_cloudfront_distribution.default
BC_AWS_NETWORKING_52 Fixed /main.tf aws_s3_bucket.origin
BC_AWS_GENERAL_56 Fixed /main.tf aws_s3_bucket.origin
BC_AWS_GENERAL_72 Fixed /main.tf aws_s3_bucket.origin

@korenyoni
Copy link
Member

/test all

main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@korenyoni
Copy link
Member

@mburns I'm honestly not sure why the BridgeCrew check is still failing — I will have to take a closer look tomorrow.

CleanShot 2021-12-14 at 00 57 21@2x

main.tf Outdated Show resolved Hide resolved
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 5cf1781 - Merge branch 'master' of github.com:mburns/terraform-aws-cloudfront-s3-cdn - 4 errors were fixed.

Change details

Error ID Change Path Resource
BC_AWS_NETWORKING_52 Fixed /main.tf aws_s3_bucket.origin
BC_AWS_GENERAL_56 Fixed /main.tf aws_s3_bucket.origin
BC_AWS_GENERAL_27 Fixed /main.tf aws_cloudfront_distribution.default
BC_AWS_GENERAL_72 Fixed /main.tf aws_s3_bucket.origin

main.tf Show resolved Hide resolved
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 3fec92b - Add descriptions to BridgeCrew suppressions and move them to top of s3 bucket resource definition. - 4 errors were fixed.

Change details

Error ID Change Path Resource
BC_AWS_GENERAL_27 Fixed /main.tf aws_cloudfront_distribution.default
BC_AWS_NETWORKING_52 Fixed /main.tf aws_s3_bucket.origin
BC_AWS_GENERAL_56 Fixed /main.tf aws_s3_bucket.origin
BC_AWS_GENERAL_72 Fixed /main.tf aws_s3_bucket.origin

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 8df1b8f - Expand comment on BC supression for AWS WAF. - 4 errors were fixed.

Change details

Error ID Change Path Resource
BC_AWS_GENERAL_56 Fixed /main.tf aws_s3_bucket.origin
BC_AWS_GENERAL_72 Fixed /main.tf aws_s3_bucket.origin
BC_AWS_NETWORKING_52 Fixed /main.tf aws_s3_bucket.origin
BC_AWS_GENERAL_27 Fixed /main.tf aws_cloudfront_distribution.default

main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
@korenyoni
Copy link
Member

/test all

@korenyoni
Copy link
Member

CleanShot 2021-12-14 at 12 35 31@2x

I'm still not sure why BC is failing in the GitHub checks API.

Originally, it was because examples/complete's module instantiation was showing up in BridgeCrew without the suppressions:
CleanShot 2021-12-14 at 12 09 38@2x

I'm not sure why it's still happening though. As you can see, it's passing in BC itself.

CleanShot 2021-12-14 at 12 35 47@2x

@mburns I've also taken over your PR and just expanded the suppression comments and moved the suppressions to the top of the resource definition blocks.

Anyways, I'm going to merge this in and hope the BC errors do not keep happening in subsequent PRs.

@korenyoni korenyoni changed the title Add external_aliases which get no cnames created for them Feat: Add external_aliases Which Will Not Have CNAMEs Created for Them Dec 14, 2021
@korenyoni korenyoni added no-release Do not create a new release (wait for additional code changes) enhancement New feature or request labels Dec 14, 2021
@korenyoni korenyoni merged commit 64bd6d9 into cloudposse:master Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request no-release Do not create a new release (wait for additional code changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants