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

Fix S3 permissions to allow CloudFront logging #410

Merged
merged 2 commits into from Sep 16, 2020

Conversation

hectcastro
Copy link
Contributor

@hectcastro hectcastro commented Sep 11, 2020

Overview

It appears as though something changed with Terraform's ability to detect out-of-band grants, or perhaps a CloudFront service level change led to different behaviors than before (they recently added real-time logs). That's left us with a handful of CloudFront logs in S3 from May, but nothing between May and now.

When logging to S3 from CloudFront is configured via the AWS console, a specific set of grants that allow CloudFront to interact with an S3 bucket are added. These grants appear to get wiped away by Terraform unless they are explicitly defined in the Terraform configuration.

The changes in this PR aim to persist the two grants (one is the AWS canonical user ID, and the other is the CloudFront canonical user ID) in the Terraform project.

Checklist

  • Description of PR is in an appropriate section of CHANGELOG.md and grouped with similar changes, if possible

Testing Instructions

  • Disable CloudFront logging for the staging distribution via the AWS console
  • Execute a Terraform plan/apply cycle using develop and see that 2 grants are being removed from the aws_s3_bucket.logs resource
  • Execute a Terraform plan/apply cycle using the changes in this branch and ensure that no grants are removed the the aws_s3_bucket.logs resource

@hectcastro hectcastro force-pushed the feature/hmc/fix-cloudfront-logs branch from 30a1949 to f237dcb Compare September 11, 2020 21:27
@@ -1,6 +1,6 @@
provider "aws" {
region = var.aws_region
version = "~> 3.1.0"
version = "~> 3.6.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this change first to see if maybe it was a bug in the provider, but that didn't yield any change in behavior. I kept the change in the PR because no other resources seemed to be impacted by the upgrade.

@@ -295,3 +295,8 @@ variable "aws_ecs_task_execution_role_policy_arn" {
default = "arn:aws:iam::aws:policy/service-role/AmazonECSTaskExecutionRolePolicy"
type = string
}

variable "aws_cloudfront_canonical_user_id" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, there isn't a data resource for this like there is for the aws_canonical_user_id. hashicorp/terraform-provider-aws#12512 exists to add it to the Terraform AWS provider.

Copy link
Contributor

Choose a reason for hiding this comment

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

That'd be nice, hope that change gets worked in soon!

@hectcastro hectcastro marked this pull request as ready for review September 11, 2020 21:37
@hectcastro
Copy link
Contributor Author

A heads up @colekettler because this PR doesn't show up on our board and may have slipped through the cracks.

Copy link
Contributor

@colekettler colekettler left a comment

Choose a reason for hiding this comment

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

Wow, that's a subtle one. Looks like this fix works!

I feel like this should become part of our standard CloudFront config. Is it possible that this could be happening in other projects and we haven't noticed yet?

@hectcastro
Copy link
Contributor Author

Wow, that's a subtle one. Looks like this fix works!

I think it does, but was hopeful there might be something a bit clearer out there. Thus far, I haven't found it.

I feel like this should become part of our standard CloudFront config. Is it possible that this could be happening in other projects and we haven't noticed yet?

I think it very well may be. I opened a few issues in other projects that have similar configurations. It would be better to fix sooner rather than later so that information is available in the event that we need to troubleshoot things.

@hectcastro hectcastro force-pushed the feature/hmc/fix-cloudfront-logs branch from f237dcb to 9fcca72 Compare September 16, 2020 21:07
It appears as though something changed with Terraform's ability to
detect out-of-band grants, or perhaps a CloudFront service level change
led to different behaviors than before (they recently added real-time
logs).

When logging to S3 from CloudFront is configured via the AWS console, a
specific set of grants that allow CloudFront to interact with an S3
bucket are added. These grants appear to get wiped away by Terraform
unless they are explicitly defined in the Terraform configuration.

The changes in this PR aim to persist the two grants (one is the AWS
canonical user ID, and the other is the CloudFront canonical user ID) in
the Terraform project.
@hectcastro hectcastro force-pushed the feature/hmc/fix-cloudfront-logs branch from 9fcca72 to d68719e Compare September 16, 2020 21:08
@hectcastro hectcastro merged commit 0c43b01 into develop Sep 16, 2020
@hectcastro hectcastro deleted the feature/hmc/fix-cloudfront-logs branch September 16, 2020 21:43
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.

None yet

2 participants