[#136353673] Add ability to create CDNs programmatically #753

paroxp · 2017-02-02T17:03:47Z

What

We would like to create a CDN for the PaaS Documentation and the Product Page and be able to access it, from the new subdomain.

This change, should allow any user with an access, create CloudFront Distributions. The CNAME record, will be created as part of the system domain name, for each distribution.

How to review

Run create-cloudfoundry pipeline from this branch
Run ACTION=plan make dev setup_cdn_instances
Experience no errors from the above
Run ACTION=apply make dev setup_cdn_instances
Experience no errors from the above
Wait... This may take a while. I suggest logging into AWS Console and monitoring CloudFront Distributions
Visiting docs.${DEPLOY_ENV}.dev.cloudpipeline.digital should result in paas-tech-docs
Visiting www.${DEPLOY_ENV}.dev.cloudpipeline.digital should result in paas-product-page

Who can review

Not @paroxp

dcarley

Drive-by review: This looks good! Nice use of a module too.

dcarley · 2017-02-02T17:15:45Z

terraform/cloudfront/cloudfront_distribution/distribution.tf

+  }
+
+  viewer_certificate {
+    cloudfront_default_certificate = true


Do we need to use the wildcard certs that are uploaded by make <env> manually_upload_certs in order for the client browser to get a valid cert when you visit www.cloud.service.gov.uk or docs.cloud.service.gov.uk?

Not sure how you'd test it in dev, but I think we should mention it in the review section of the PR.

Good point. I had a wrong assumption, it would just apply straight away.

Pushed new commit. Thanks.

dcarley · 2017-02-02T17:23:52Z

terraform/cloudfront/cloudfront_distribution/distribution.tf

+      origin_protocol_policy = "https-only"
+      http_port              = 80
+      https_port             = 443
+      origin_ssl_protocols   = ["TLSv1", "TLSv1.1", "TLSv1.2"]


Not a blocker, but out of curiosity, I wonder if we need to support TLSv1 and TLSv1.1

There was some discussion about it recently, do you know the outcome @bleach @davbo @davidillsley ?

Sidenote: our ELBs do because the default AWS policies do:

paas-cf/terraform/globals.tf

Line 125 in 3e3ba8b

default = "ELBSecurityPolicy-2016-08"

So far we are prioritising work to measure the impact of dropping TLSv1[.1] from GOV.UK. I'm not sure where that has got to yet though.

Looking more closely at this, I think this is only specifying the policies for connecting to origin, not for end-user requests. End-user SSL details are configured in the viewer_certificate section lower down in this file.

dcarley · 2017-02-02T17:29:23Z

terraform/cloudfront/cloudfront_distribution/distribution.tf

+  aliases         = "${var.aliases}"
+  comment         = "${var.comment}"
+  enabled         = true
+  is_ipv6_enabled = true


The future! 🐳

alext

I can't get this to work at present due to issues with the certificate. It may be that it's refusing to work with a self-signed certificate, but I haven't got to the bottom of this.

I'm not clear on the differences between the acm_certificate_arn and iam_certificate_id parameters. The former seems to apply to Amazon Certificate Manager, which I'm not sure that we're using.

alext · 2017-02-03T10:29:26Z

terraform/cloudfront/cloudfront_distribution/variables.tf

+
+variable "system_dns_zone_name" {}
+
+variable "system_domain_cert_arn" {}


These variables could all do with a description to clarify what they're used for etc.

Sure! I don't think we've been doing that in many of the other files, but you're right.

alext · 2017-02-03T10:30:50Z

terraform/scripts/set-up-cdn-instances.sh

+# Run the terraform action on the instances.
+terraform "${TERRAFORM_ACTION}" \
+  -var-file="${PAAS_CF_DIR}/terraform/${AWS_ACCOUNT}.tfvars" \
+  -var "env=${MAKEFILE_ENV_TARGET}" \


I think the env needs to be set to $DEPLOY_ENV, not $MAKEFILE_ENV_TARGET, as that's how it's being used. (Upon review I've noticed that this is also incorrect in the Pingdom script, but I think fixing that is out of scope for this PR).

Agree. It did confuse me a little when I've looked at it in the Pingdom script.

alext · 2017-02-03T10:31:46Z

terraform/scripts/set-up-cdn-instances.sh

+
+set -eu
+TERRAFORM_ACTION=${1}
+STATEFILE=cloudfront-distribution-${MAKEFILE_ENV_TARGET}.tfstate


This doesn't need any parameters included in it as it lived in the deployment-specific state bucket. I'd suggest just cloudfront.tfstate so that the statefile name matches the terraform directory name.

Good point. Thanks.

alext · 2017-02-03T10:33:00Z

Makefile

@@ -198,6 +198,11 @@ pingdom: check-tf-version ## Use custom Terraform provider to set up Pingdom che
 	$(eval export PINGDOM_CONTACT_IDS=11089310)
 	@terraform/scripts/set-up-pingdom.sh ${ACTION}

+.PHONY: setup_cdn_instances
+setup_cdn_instances: check-tf-version check-aws-credentials


This could do with a help comment like the other actions. That'll make it show up when you do make help.

Sorry, added.

alext · 2017-02-03T10:38:19Z

terraform/cf-certs/upload.tf

@@ -7,6 +7,7 @@ resource "aws_iam_server_certificate" "system" {
  certificate_body  = "${var.system_domain_crt}"
  private_key       = "${var.system_domain_key}"
  certificate_chain = "${var.system_domain_intermediate_crt}"
+  path              = "/cloudfront/${var.env}-system-domain"


I get an error running this in the pipeline as follows:

Error applying plan: 2 error(s) occurred: * aws_iam_server_certificate.system: [WARN] Error uploading server certificate, error: ValidationError: The specified value for path is invalid. It must begin and end with / and contain only alphanumeric characters and/or / characters. * aws_iam_server_certificate.apps: [WARN] Error uploading server certificate, error: ValidationError: The specified value for path is invalid. It must begin and end with / and contain only alphanumeric characters and/or / characters.

I think it's complaining because it wants this to have a trailing /.

The error also suggests that it also won't allow - characters, but the docs say they should be allowed. When testing, it allows - characters.

This also has the issue that it causes terraform to try to recreate the certs. It uploads the new certs, and then fails when trying to delete the old one because it's still in use.

As suggested in the IRL conversation, I have created a new commit in this PR. Please, let me know, if that still prevents the pipeline from progression.

alext · 2017-02-03T11:03:04Z

Makefile

@@ -46,7 +46,7 @@ lint_yaml:
 lint_terraform: dev
 	$(eval export TF_VAR_system_dns_zone_name=$SYSTEM_DNS_ZONE_NAME)
 	$(eval export TF_VAR_apps_dns_zone_name=$APPS_DNS_ZONE_NAME)
-	find terraform -mindepth 1 -maxdepth 1 -type d -not -path 'terraform/providers' -not -path 'terraform/scripts' -print0 | xargs -0 -n 1 -t terraform graph > /dev/null
+	find terraform -mindepth 1 -maxdepth 1 -type d -not -path 'terraform/providers' -not -path 'terraform/scripts' -print0 | xargs -0 -n 1 -t sh -c 'terraform get $$1 && terraform graph $$1' -- > /dev/null


This addition of terraform get causes the creation of a .terraform directory in the repo root when running it locally.

Also, this is getting quite long, and not particularly easy to read. I'd suggest extracting the body of this task into a script. That script could then use a temporary working dir to avoid polluting the repo (in a similar manner to some of the other scripts in terraform/scripts).

This has now been extracted into a separate file.

I would like to point out, that this command will fail locally, as it did before with two provider pingdom couldn't be found errors.

alext · 2017-02-03T11:10:37Z

terraform/cloudfront/cloudfront_distribution/distribution.tf

+  viewer_certificate {
+    acm_certificate_arn            = "${var.system_domain_cert_arn}"
+    cloudfront_default_certificate = false
+  }


I think this is missing a ssl_support_method param, which is required when specifying a cert (see docs). This need to be set to one of vip or sni-only. I think we can probably use sni-only.

At present, I get the following error:

2 error(s) occurred: * aws_cloudfront_distribution.cdn_instance: MalformedXML: 1 validation error detected: Value '' at 'distributionConfigWithTags.distributionConfig.viewerCertificate.sSLSupportMethod' failed to satisfy constraint: Member must satisfy enum value set: [sni-only, vip] status code: 400, request id: be2f4fc6-ea00-11e6-b6c6-29d0a4c8b7f3 * aws_cloudfront_distribution.cdn_instance: MalformedXML: 1 validation error detected: Value '' at 'distributionConfigWithTags.distributionConfig.viewerCertificate.sSLSupportMethod' failed to satisfy constraint: Member must satisfy enum value set: [sni-only, vip] status code: 400, request id: be2f9e4f-ea00-11e6-b089-c378731fc466

Sorry, I did have it in, however must have lost it in rebasing process.

We would like to create a CDN for the PaaS Documentation and the Product Page and be able to access it, from the new subdomain. The aim of any tool, is to make it easy to use. In this case as extensible as possible, without making too many changes. Therefore to achieve that, we are creating each distribution instance in form of a terraform module. These modules are all stored in one `instances.tf` file. The CNAME record, will be created as part of the system domain name.

We would like to serve our new subdomains under our existing ssl wildcard certificate.

This command is getting to long. If we extract it to separate file, we will be able to clearly see the functionality involved as also open up possibilities of expansion.

Adding the `path` attribute, to the 'terraform/cf-certs/upload.tf' is causing the task to fail. This is expected behaviour, as it redefines the certificate. Terraform would be trying to remove the old certificates, causing amazon to fail, as these are still in use. The idea is, that the new certificates will get created and defined to be used. Once that happens, the old ones, will be peacefully removed. This problem, will occur only in our DEV and CI environments. Staging and prod should not be affected. This commit, could be reverted after a week after merging this PR.

There are three price classes available to choose from described on the AWS CloudFront documentation located here: https://aws.amazon.com/cloudfront/pricing/ We have came to the conclusion, that we can safely set the class, to the smallest and cheapest one, covering only US, CA and EU as we don't think these will be accessed from outside of EU. Although still accessible, it will take longer, to locate the nearest CDN server, to the country outside of our scope.

alext · 2017-02-06T14:40:21Z

This looks good.

While reviewing this, I ran into a terraform bug that means that the cf-certs terraform needs to be run twice before the CDN can be deployed.

alext · 2017-02-06T14:41:41Z

I'd also note that this will need some manually deploying in staging and prod due because the cert properties need to be updated.

[#136353673] Add ability to create CDNs programmatically

paroxp changed the title ~~[WIP] [#136353673] Add ability to create CDNs programmatically~~ [#136353673] Add ability to create CDNs programmatically Feb 2, 2017

dcarley reviewed Feb 2, 2017

View reviewed changes

paroxp force-pushed the docs_available_136353673 branch from d4c6d60 to 94c595f Compare February 2, 2017 18:48

alext self-assigned this Feb 3, 2017

alext suggested changes Feb 3, 2017

View reviewed changes

paroxp force-pushed the docs_available_136353673 branch 4 times, most recently from efeb6ee to f65ff4e Compare February 3, 2017 12:56

paroxp changed the title ~~[#136353673] Add ability to create CDNs programmatically~~ [WIP][#136353673] Add ability to create CDNs programmatically Feb 3, 2017

paroxp force-pushed the docs_available_136353673 branch 5 times, most recently from abd3270 to 744ae4d Compare February 3, 2017 16:44

paroxp changed the title ~~[WIP][#136353673] Add ability to create CDNs programmatically~~ [#136353673] Add ability to create CDNs programmatically Feb 3, 2017

paroxp force-pushed the docs_available_136353673 branch 2 times, most recently from 79da9be to 0e8b868 Compare February 6, 2017 10:53

paroxp added 3 commits February 6, 2017 11:04

Allow selection of wildcard certificate

eaccc93

We would like to serve our new subdomains under our existing ssl wildcard certificate.

Extract lint_terraform command

2bc1766

This command is getting to long. If we extract it to separate file, we will be able to clearly see the functionality involved as also open up possibilities of expansion.

paroxp force-pushed the docs_available_136353673 branch from 0e8b868 to 5fa517c Compare February 6, 2017 11:05

alext approved these changes Feb 6, 2017

View reviewed changes

alext merged commit 99c451c into master Feb 6, 2017

alext added a commit that referenced this pull request Feb 6, 2017

Merge pull request #753 from alphagov/docs_available_136353673

9158d31

[#136353673] Add ability to create CDNs programmatically

alext deleted the docs_available_136353673 branch February 6, 2017 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#136353673] Add ability to create CDNs programmatically #753

[#136353673] Add ability to create CDNs programmatically #753

paroxp commented Feb 2, 2017 •

edited

Loading

dcarley left a comment

dcarley Feb 2, 2017

paroxp Feb 2, 2017

dcarley Feb 2, 2017

davbo Feb 2, 2017

alext Feb 3, 2017

dcarley Feb 2, 2017

alext left a comment

alext Feb 3, 2017

paroxp Feb 3, 2017

alext Feb 3, 2017

paroxp Feb 3, 2017

alext Feb 3, 2017

paroxp Feb 3, 2017

alext Feb 3, 2017

paroxp Feb 3, 2017

alext Feb 3, 2017

alext Feb 3, 2017

paroxp Feb 6, 2017

alext Feb 3, 2017

paroxp Feb 3, 2017

alext Feb 3, 2017

paroxp Feb 3, 2017

alext commented Feb 6, 2017

alext commented Feb 6, 2017


		variable "system_dns_zone_name" {}

		variable "system_domain_cert_arn" {}

[#136353673] Add ability to create CDNs programmatically #753

[#136353673] Add ability to create CDNs programmatically #753

Conversation

paroxp commented Feb 2, 2017 • edited Loading

What

How to review

Who can review

dcarley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alext left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alext commented Feb 6, 2017

alext commented Feb 6, 2017

paroxp commented Feb 2, 2017 •

edited

Loading