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

providers/aws: CloudFront distribution #1780

Closed
wants to merge 19 commits into from
Closed

providers/aws: CloudFront distribution #1780

wants to merge 19 commits into from

Conversation

AlexanderEkdahl
Copy link
Contributor

This is my stab at a CloudFront distribution. It works well and covers most use cases. However there are features that this schema does not support and I would like to hear your opinions on the matter.

  • CloudFront distributions supports multiple origins which this PR does not. I would gladly hear example where multiple origins are a necessity. Should this resource also support multiple regions or is this enough?
  • Should it be named aws_cf_distribution or aws_cf_web_distribution in order to not be confused with RTMP distributions?
  • When changing from enabled to disabled in order to delete the resource, is that partial state as described in the docs?
  • Is it even neccessary to wait until fully deployed? The terraform outputs are still correct, the user is however not guaranteed that the endpoint will be serving data correctly. For those unfamiliar with CloudFront, deploying takes 15 minutes.
  • I have observed the acceptance test taking ~40 minutes to complete. Should the timeout limit be increased?

TODO

  • Multiple origins
  • Custom Headers
  • Whitelisting cookies
  • Trusted signers
  • Allowed Methods
  • Cache behaviors
  • Custom error responses
  • Viewer certificates(Custom SSL)
  • Origin access identity
  • Update documentation
  • Cache behavior precedence
  • Gather all values when executing resourceAwsCloudFrontWebDistributionRead
  • Smooth Streaming
  • Squash commits
  • Remove log output
  • Fail quietly when resource no longer exists

Example Usage

resource "aws_cloudfront_web_distribution" "seven" {
  enabled = false
  default_origin = "main"
  default_forward_cookie = "whitelist"
  default_whitelisted_cookies = ["session"]
  aliases = ["static.example.com"]
  comment = "Useless comment"
  price_class = "PriceClass_100"
  geo_restriction_type = "blacklist"
  geo_restrictions = ["AQ", "CV"]

  origin {
    domain_name = "web.s3.amazonaws.com"
    id = "main"
  }

  origin {
    domain_name = "images.s3.amazonaws.com"
    id = "images"
    origin_path = "/photos"
  }

  behavior {
    pattern = "images/*.jpg"
    origin = "images"
  }

  behavior {
    pattern = "images/*.png"
    origin = "images"
    minimum_ttl = 100
    whitelisted_cookies = ["test"]
  }
}

@radeksimko
Copy link
Member

Thanks for this! 👍

Would you mind adding one extra exported hard-coded attribute, zone_id?
This is to make it easier to use CloudFront w/ Route 53 ALIAS records in the DSL:
http://docs.aws.amazon.com/Route53/latest/APIReference/CreateAliasRRSAPI.html
#1775

It may sound a bit ridiculous to add an argument that will always return the same value, but:

  1. Amazon may (for any reason) change that in future
  2. It should IMO be easy to create the alias using the DSL, not having to go to the documentation to look up the zone ID for any resource.

Z2FDTNDATAQYW2 is the unique hosted zone ID for all CloudFront distributions per the linked docs above ^.

@AlexanderEkdahl
Copy link
Contributor Author

Not at all! I'm currently contemplating whether or not it should support multiple origins though...

@radeksimko
Copy link
Member

Also looking at the API docs, it may be wise to use resource name aws_cloudfront_web_distribution instead to prevent future confusions & collisions:
http://docs.aws.amazon.com/AmazonCloudFront/latest/APIReference/Actions_Dist.html

Next resources that may follow in the future can be aws_cloudfront_rtmp_distribution and aws_cloudfront_origin_access_identity

@radeksimko
Copy link
Member

I'm currently contemplating whether or not it should support multiple origins though...

How about using schema.TypeSet, like the elb uses for health_check or listener?

So that the DSL becomes something like this:

resource "aws_cloudfront" "six" {
  default_root_object = "index.html"
  origin {
    domain_name = "granuldisk.s3.amazonaws.com"
    path = "/"
    http_port = 80
    https_port = 443
    protocol_policy = "http-only"
  }

  origin {
    domain_name = "dadada.s3.amazonaws.com"
    path = "/"
    http_port = 80
    https_port = 443
    protocol_policy = "http-only"
  }
}

@AlexanderEkdahl
Copy link
Contributor Author

Yes, that is what I initially had and it worked quite well. However most of the other options also apply per origin which resulted in a mess. Implementing multiple origins would result in 100% CloudFront web distribution coverage which would certainly be desirable. I'll make another attempt!

@AlexanderEkdahl
Copy link
Contributor Author

The only downside of the CloudFront resource exporting zone_id is that it results in an otherwise unnecessary dependancy. But in most cases the Route53 record would be dependant on the distribution anyway.

@radeksimko
Copy link
Member

The only downside of the CloudFront resource exporting zone_id is that it results in an otherwise unnecessary dependancy. But in most cases the Route53 record would be dependant on the distribution anyway.

Hmm, interesting, can you explain that a bit more? e.g. explain some cases when the dependency can be harmful?

@AlexanderEkdahl
Copy link
Contributor Author

I've now renamed the resource like you said. Much better!

Regarding the dependancy, the only harmful scenario I can think of is a cycle but I do not think that is likely in this case.

@radeksimko
Copy link
Member

Is it even neccessary to wait until fully deployed? The terraform outputs are still correct, the user is however not guaranteed that the endpoint will be serving data correctly. For those unfamiliar with CloudFront, deploying takes 15 minutes.
I have observed the acceptance test taking ~40 minutes to complete. Should the timeout limit be increased?

This is very similar issue to what I observed on RDS and Route53 records, both quite annoying, but sometimes you may want to wait. See #1124 and #1097

For the beginning, I'd personally not be bothered and just wait, even 40 mins, as long as all timeouts allow this, we should be safe.

@AlexanderEkdahl
Copy link
Contributor Author

I see. I'll leave it as it is for now.

Regarding the multiple origins: they are order dependant. I'm not sure how to best represent that in HCL. Got any suggestions?

@AlexanderEkdahl
Copy link
Contributor Author

I've updated this PR with support for multiple origins. There are still challenges regarding the ordering of behaviors. It's not finished yet but I'm getting there!

Updated example

resource "aws_cloudfront_web_distribution" "seven" {
  enabled = false
  default_origin = "main"
  default_forward_cookie = "whitelist"
  default_whitelisted_cookies = ["session"]
  aliases = ["static.example.com"]

  origin {
    domain_name = "web.s3.amazonaws.com"
    id = "main"
  }

  origin {
    domain_name = "images.s3.amazonaws.com"
    id = "images"
    origin_path = "/photos"
  }

  behavior {
    pattern = "images/*.jpg"
    origin = "images"
  }

  behavior {
    pattern = "images/*.png"
    origin = "images"
    minimum_ttl = 100
    whitelisted_cookies = ["test"]
  }
}

@radeksimko
Copy link
Member

I haven't tested the functionality, but I like the DSL 👍

@AlexanderEkdahl
Copy link
Contributor Author

Current status: I've been very busy with school and work. I will try and find the time to finish this next week!

@adelamarre
Copy link

Hi and thanks for this job ! know you when this feature will be available ? I need it !! ;)

@radeksimko
Copy link
Member

@AlexanderEkdahl Yes, that is usually what we do, probably better than showing error I assume? Unless you had different solution on mind. 😉

@AlexanderEkdahl
Copy link
Contributor Author

Not at all. Only thing remaining now is documentation and general cleanup.

@catsby
Copy link
Contributor

catsby commented Jun 25, 2015

Hey @AlexanderEkdahl – are you still planning on completing this work?

@catsby catsby added the waiting-response An issue/pull request is waiting for a response from the community label Jun 25, 2015
@AlexanderEkdahl
Copy link
Contributor Author

Definitely! However, this PR can be closed seeing how far behind my fork is. When finished I'll open another PR.

I've got some ideas on how to circumvent the issues with terraform and double nested sets and lack of ordering in HCL.

@catsby
Copy link
Contributor

catsby commented Jun 25, 2015

OK, thanks for the update @AlexanderEkdahl

@catsby catsby removed the waiting-response An issue/pull request is waiting for a response from the community label Aug 20, 2015
@ringods
Copy link
Contributor

ringods commented Sep 16, 2015

This pull request, and the original issue (#2723) are both closed. @AlexanderEkdahl, is your work still pending a new pull request?

@AlexanderEkdahl
Copy link
Contributor Author

Work has stalled since HCL keys are unordered. Something which i find to be a weakness but I understand that it would be difficult to make ordered HCL keys compatible with JSON.

Also it is not possible to programmatically read attributes from other resources which makes it difficult to solve in other ways.

@radeksimko
Copy link
Member

Work has stalled since HCL keys are unordered.

schema.TypeList is unordered, if you care about the order, schema.TypeSet is the solution.

Also it is not possible to programmatically read attributes from other resources

I'm not quite sure what do you mean by this - would you mind clarifying it?

@justincampbell
Copy link
Contributor

schema.TypeList is unordered, if you care about the order, schema.TypeSet is the solution.

I believe this is backwards

@AlexanderEkdahl
Copy link
Contributor Author

The problem is that I need nested content inside TypeSet which currently does not work with default values. Also the resources get extremely unwieldy to work with. I haven't given up on a complete CloudFront implementation but I've been very busy with other projects and work 😰

@johnrengelman
Copy link
Contributor

:( didn't realize we don't have this yet in Terraform.

@AlexanderEkdahl
Copy link
Contributor Author

To clarify. My implementation works and I'm using it in one of my projects. It is just not good enough to be, in my opinion, merged into master. However if anyone wants to try and implement this by themselves I would gladly share my expertise.

@justincampbell
Copy link
Contributor

I personally would prefer something that works for 80% of use cases (single origin) be merged and iterated upon. I'd be happy to work on this in a week or two when I have more time.

@AlexanderEkdahl
Copy link
Contributor Author

My initial implementation only supported one origin(see discussions above). I might revise that solution..

@dalehamel
Copy link

Hey, I just merged this branch locally and got it building again, seems pretty legit to me.

I'd be happy to push my minor changes to get it building somewhere, if it helps get this merged.

@dalehamel
Copy link

Here's my patch @AlexanderEkdahl @justincampbell https://gist.github.com/dalehamel/090118571e0227acb11b

May it help you get this into master 🙏

FYI I just used it to set up a cloudfront distribution, and it worked pretty 👌 aside from one crash for a null pointer dereference.

@timbunce
Copy link

Please count this as another vote for getting CloudFront support into Terraform, even if only basic.

@AlexanderEkdahl
Copy link
Contributor Author

I'm unable to get this building because of updates in the aws sdk. Terraform should really start vendoring its dependencies..

@AlexanderEkdahl
Copy link
Contributor Author

The panic that you are seeing is because you haven't whitelisted any cookies and it expects there to be a list of strings. That should be an easy fix in the code. I'm currently looking into it!

@AlexanderEkdahl
Copy link
Contributor Author

New PR which removes support for multiple origins, applies @dalehamel changes and fixes the panic with whitelisted cookies can be found at #3330

@ghost
Copy link

ghost commented Apr 30, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants