[WIP] provider/aws: CloudFront

[AlexanderEkdahl]

Created a new PR as this is a rewritten implementation of my earlier attempt. The only feature not supported are multiple origins and behaviors.

Also worth noting is the fragility of the CloudFront API. Whenever a new attribute is added(DefaultTTL was added this summer) they are optional when creating the resource but not when updating an existing resource. Hopefully this does not happen too often.

previous PR: #1780

@radeksimko @justincampbell @dalehamel

[AlexanderEkdahl]

Works with both custom origins, s3 buckets and s3 website endpoints.

resource "aws_s3_bucket" "bucket" {
  bucket = "cloudfront-terraform-example"
  policy = "${file("policy.json")}"

  website {
    index_document = "index.html"
  }
}

resource "aws_cloudfront_web_distribution" "simple" {
  origin_domain_name = "${aws_s3_bucket.bucket.website_endpoint}"
}

[dalehamel]

@AlexanderEkdahl Awesome! @justincampbell what's left to get this to 🚢 💨 ?

[radeksimko]

@AlexanderEkdahl Thanks for re-submitting this! After a first quick look it looks fine, but someone will give it a thorough review before actually merging it in, probably after HashiConf, when most maintainers should be less busy :)

[apparentlymart]

Thanks for working on this! I thought the previous attempts had been abandoned so I'd started one in #3259, but I'm very happy for you to finish this up instead. :)

My attempt supported multiple origins and both S3 and custom origin types, but it annoyed me that this complicated the "easy" case of having just one origin, so I was planning to do another pass at that. If you want to borrow anything from my implementation then you're very welcome to, but getting the simple case merged first is a fine idea too, as long as there is a non-breaking-change path from here to there.

[AlexanderEkdahl]

From what I can see your implementation does not support multiple behaviors and linking them to an origin. Without that multiple origins does not do anything because every request will be routed to the the first origin anyway. This PR support both S3 and custom origins as well.

But like you said. Adding support for multiple origins and behaviors wont break old configurations 👊

[yogin]

Definitely looking forward to this, I don't need multi origins/behaviors, so would be more than happy with the current implementation 👍

[ringods]

Also make sure that this gets updated in the AWS coverage sheet. See #28.

[AlexanderEkdahl]

@radeksimko @catsby @justincampbell Have any of you had any time to review this?

[antonbabenko]

👍 for Cloudfront support. Kudos to @AlexanderEkdahl

[rgabo]

@radeksimko do you have any update on when do you expect a more thorough review? Would be great to see this merged back.

[ahromis]

Looking forward to seeing Cloudfront supported!

[AlexanderEkdahl]

I've rebased my PR on top of master for your convenience.

[radeksimko]

Hi,
sorry for the silence here, it would be nice to get this in. I'm just looking at the code.

[radeksimko]

I tried running attached acceptance tests and it has errored out unfortunately:

=== RUN   TestAccCloudFront
--- FAIL: TestAccCloudFront (658.69s)
    testing.go:137: Step 1 error: Error applying: 1 error(s) occurred:

        * aws_cloudfront_web_distribution.main: Error updating CloudFront distribution: IllegalUpdate: WebACLId is missing for the resource
            status code: 400, request id: 65e4a846-824d-11e5-9ab0-4c3390724117
    testing.go:158: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.

        Error: Error applying: 1 error(s) occurred:

        * aws_cloudfront_web_distribution.main: Error updating CloudFront distribution: IllegalUpdate: WebACLId is missing for the resource
            status code: 400, request id: 65e4a846-824d-11e5-9ab0-4c3390724117

        State: aws_cloudfront_web_distribution.main:
          ID = F13BN8IM6H1KER
          aliases.# = 0
          allowed_methods.# = 2
          allowed_methods.0 = HEAD
          allowed_methods.1 = GET
          cached_methods.# = 2
          cached_methods.0 = HEAD
          cached_methods.1 = GET
          certificate_id =
          comment =
          default_root_object =
          default_ttl = 86400
          domain_name = d2itrp7gougkek.cloudfront.net
          enabled = false
          forward_cookie = none
          forward_query_string = false
          forwarded_headers.# = 0
          geo_restriction_type = none
          geo_restrictions.# = 0
          logging_bucket =
          logging_enabled = false
          logging_include_cookies = false
          logging_prefix =
          maximum_ttl = 31536000
          minimum_ssl = SSLv3
          minimum_ttl = 0
          origin_domain_name = fileserver.example.com
          origin_http_port = 80
          origin_https_port = 443
          origin_path =
          origin_protocol_policy = http-only
          price_class = PriceClass_All
          smooth_streaming = false
          ssl_support_method = vip
          status = Deployed
          viewer_protocol_policy = allow-all
          zone_id = Z2FDTNDATAQYW2
FAIL
exit status 1
FAIL
    github.com/hashicorp/terraform/builtin/providers/aws    658.711s

[AlexanderEkdahl]

That certainly did not happen when I initially created this.

This is the second time the API introduces breaking changes and it only happens with the update method. Creating a resource is not as strict but updating a resource requires every parameter.

This could be a problem in the future... I'll look into it!

[radeksimko]

It would be probably best to remove this function here and use expandStringList instead. It does exactly the same thing and we use it in other resources already.

[ringods]

@AlexanderEkdahl I'm waiting for this CloudFront support too. I can free up a developer to complete your work. He recently contributed the postgresql provider, so he has a bit of knowledge on Terraform development. Let me know if you need a helping hand.

[ocxo]

@ringods speaking for myself, yes please 😄

[AlexanderEkdahl]

I'm currently working on this. I've updated the tests in accordance with @radeksimko feedback and I'm currently investigating some SSL changes that have been made to the API since last working on this. Gzip compression is also new which should be supported by this resource.

[ocxo]

thanks for all your work on this @AlexanderEkdahl!

[phinze]

Hi @AlexanderEkdahl, let us know when you'd like us to re-review for you!

[catsby]

Gzip compression is also new which should be supported by this resource.

We can ship without this and have it in a follow up PR, if that's easier for you.
Thanks for all the work!

[radeksimko]

I'm happy to re-review this and also agree with other folks that it's better to ship minimal implementation which works, rather than trying to implement possibly all relevant features in this PR.

If you cannot spend more time on this, then letting @ringods / @fromonesrc finish this is also an option. You'd still be very likely listed as the author in git and we'd very likely include the original commits (unless the mentioned guys would have decided to rework it completely from scratch, which I don't think is necessary).

[rgabo]

thanks for the great work @AlexanderEkdahl. I've just tested the feature as part of our infra in a throwaway environment and I thought I'd share my experience/feedback. The PR merges cleanly on top of v0.6.9 btw.

• aws_cloudfront_web_distribution makes perfect sense, the single origin set up is good for most use cases. We have other use cases with multiple origins and behaviors and we use CloudFormation for those, but for our basic needs the resource is great.
• Having to append the log bucket id with .s3.amazonaws.com felt unnecessary when setting the logging_bucket attribute. Given that it cannot be anything else but an S3 bucket, the resource could append internally. The same had to be done for origin_domain_name but given that does not necessarily have to be an S3 bucket, it is understandable.

Most important though that the resource creation takes ages and resulted in a Terraform crash. I realize that the resource needs to become "Enabled" and CloudFront takes long to create a distribution but it seemed that long creation time was causing the crash. I've uploaded the relevant part of the crash.log to Gist: https://gist.github.com/rgabo/e3b542be97fffd6416f4

Hope this helps and we will keep testing.

[AlexanderEkdahl]

Glad that it worked for you.

Regarding multiple origin support it is something that could be added without breaking previous Terraform configurations. Multiple origins would also require multiple behaviors in order to be usable which would be a lot of work.

@rgabo Regarding the bucket id, does that also apply for buckets outside the standard region? If that is the case the suffix is indeed unnecessary.

There are two problems with developing a Cloudfront provider. The first is that testing takes ages since every operation on a distribution takes approximately 15 minutes.

The other problem is that while creating a resource does not require every parameter to be set, updating a resource creates every parameter to be valid. This breaks compatibility whenever Cloudfront changes their API.

A possible workaround would be to download the current configuration and merge it before any update...

[AlexanderEkdahl]

If someone else wants to finish this I'm all for it and you can contact me directly if you have any questions.

It should only be a matter of adding gzip compression, maybe add another test case, and changing everything according to @radeksimko suggestions which are all legit.

[johnewart]

At work we would like to see support for this as well so I would be happy to take a look at finishing any outstanding functionality and concerns.

[AlexanderEkdahl]

Things that need to be done:

• Rebase on top of master(did this 9 days ago so shouldn't require anything)
• Add tests for SSL behaviours. Different fields affect other fields which makes this feature very brittle. Also new features were added just a couple of days ago.
• Add support for Gzip compression
• Update according to @radeksimko comments.

[vancluever]

We need this here at PBP (very soon, was surprised to see it wasn't in already), so I'm willing to add my increment to the attempt counter. @johnewart have you started any work on this?

[johnewart]

I have and should be able to finish up next week

[vancluever]

@johnewart thanks for the reply.

First off, I'd like to acknowledge the efforts of everyone here. After looking the SDK bits for this for the last day or so, I can really say that this is probably the most frustrating part of the AWS SDK that I've worked with so far - the configuration structure is very complicated.

That said, I personally have dove into the code the last couple of days and found some concerns:

• Issues with naming of tests and some spelling errors that should probably be corrected. I will annotate a couple of examples.
• Small naming nit-pick - in both CloudFormation and API/SDK the web CloudFront distribution is referred to as simply "distribution", example: AWS::CloudFront::Distribution (CloudFormation) and CreateDistribution (SDK).
• Most importantly, the resource data structure is laid out in a way that would make me doubt if the current structure of resource parameters would indeed scale to support things like multiple origins. This is due to the lack of sub-resources in the schema - something that @radeksimko has brought up but looks systemic to the point of making things unscalable.

It's my opinion that the structure of the resource should be changed so that it better matches the CloudFormation resource. Yes, this will take a lot of interpolation for variables that are required in SDK but are not necessarily required to set up the distribution (I'm kind of wondering why length/data pairs were needed in such abundance in the config structure), but if this is to be scalable, and Terraform really does want to stick to API as much as possible, it needs to be done.

I did start some work on this that I want to finish - I'm not the biggest fan of work duplication but I hope it will help get people considering how this should look if it's to be scalable.

PS: I don't think that anyone should have to go to CloudFormation for something because the Terraform resource can't match its features. Something about that just seems wrong to me.

PPS: For testing, and possibly even just setup in general, waiting for the distribution to come online may be unnecessary. I'll annotate what I'm talking about. Also, adding a DeletionPolicy style attribute (where the distribution is retained on deletion) may help facilitate testing as well.

[lukehoersten]

Looks like this is close. Will this be in next release?

[vancluever]

Guys,

My refactor is nearly complete.

The branch is here: https://github.com/paybyphone/terraform/tree/cloudfront_v3

It is functional, the acceptance tests work (but they have retain_on_delete so that the acceptance tests complete within a sane amount of time). I also did a bit of scrubbing to make sure there were no issues with diffs on any of the tests either.

The acceptance tests are pretty much the CloudFormation examples, and cover multiple scenarios:

• S3 origin only
• Custom origin
• Multiple origins with multiple cache behaviors

As there is a lot to cover here, I'd like to let the source speak for itself for now, any feedback is welcome and I'll try and move on it ASAP.

Some things I will be implementing shortly:

• Some validators in Create and Update to enforce singular instances of the sub-resources that can only have one instance.
• An aws_cloudfront_origin_access_identity resource to generate origin access identities for S3 buckets (incidentally, CloudFormation does not support generating these either currently).

Feedback is welcome. I'd like @AlexanderEkdahl's efforts to be recognized on this, but the scale of the refactor is pretty vast and there is not much original code left, so I'm not too sure on how to proceed.

[ocxo]

Great, @vancluever! I'd be happy with it shipping without aws_cloudfront_origin_access_identity support and adding that in later if it will help it land quicker. Thanks for the work on this.

[vancluever]

Thanks @fromonesrc!

This resource is definitely complete on its own - as long as you adhere to the rules in the documentation as to what needs to be a single element only, it should be stable.

I'm actually working on a specific MaxItems validator for lists and sets in a separate branch/PR, and yeah we could just do aws_cloudfront_origin_access_identity later (it should be a small resource and an easy merge). So I'm thinking this is good to merge on its own, and those items can be added in later merges.

Again, need feedback, I can just open a new PR if that would speed things along.

[vancluever]

Guys,

I've added #5221 just to streamline merging of my branch.

Also the new squash includes some re-factors to ensure it works with the latest TF, and the aws_cloudfront_origin_access_identity resource.

Waiting with bated breath ;)

[catsby]

Closing this in favor of #5221– I'll get it reviewed and in as soon as possible

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.