quintilesims / layer0

Build, Manage, and Deploy Your Applications
Apache License 2.0
44 stars 20 forks source link

524: Apply Terraform SSL Cert ARN feature to api-refactor #526

Closed jparsons04 closed 6 years ago

jparsons04 commented 6 years ago

What does this pull request do?

With the release of v0.10.5, the feature that allows load balancer resources to have the ARN of an IAM or an ACM certificate needs to be folded into api-refactor

How should this be tested? Through the CLI and terraform. Unit tests should pass.

Checklist

closes #524

zpatrick commented 6 years ago

This looks good - but am curious your thoughts on this:

The reason I added the new CertificateARN field into develop was to keep the v.0.10 compatibility. I was thinking with api-refactor, we have a singular Certificate string field in the Port model

jparsons04 commented 6 years ago

I think it would certainly make it cleaner rather than having a separate CertificateName and CertificateARN in the Port definition. Seems reasonable to me.

diemonster commented 6 years ago

Does this https://github.com/quintilesims/layer0/pull/534 need to be added in as well?

jparsons04 commented 6 years ago

Yes, it is related and probably needed here.

jparsons04 commented 6 years ago

I checked the load balancer read code where this would have been pertinent, and I think this is already implemented. See here: https://github.com/quintilesims/layer0/blob/524-jparsons-tf-cert-arn/api/provider/aws/load_balancer_read.go#L32

jparsons04 commented 6 years ago

@sesh-kebab There was a logic reversal I had in the AWS Provider for load balancer create. I created a new instance using my branch's code, then used the following terraform template:

resource "layer0_environment" "env_a" {
  name = "env_a"
}

resource "layer0_load_balancer" "lb_iam_name" {
  name        = "lb_iam_name"
  environment = "${layer0_environment.env_a.id}"
  private     = false

  port {
    host_port      = 443
    container_port = 80
    protocol       = "https"
    certificate    = "l0-jparsonsdev-api"
  }
}

resource "layer0_load_balancer" "lb_iam_arn" {
  name        = "lb_iam_arn"
  environment = "${layer0_environment.env_a.id}"
  private     = false

  port {
    host_port      = 443
    container_port = 80
    protocol       = "https"
    certificate    = "arn:aws:iam::856306994068:server-certificate/l0/l0-jparsonsdev/l0-jparsonsdev-api"
  }
}

resource "layer0_load_balancer" "lb_acm_arn" {
  name        = "lb_acm_arn"
  environment = "${layer0_environment.env_a.id}"
  private     = false

  port {
    host_port      = 443
    container_port = 80
    protocol       = "https"
    certificate    = "arn:aws:acm:us-west-2:856306994068:certificate/bfcb2ccd-8779-47bc-b62f-ba99e5c8a73e"
  }
}

I created three load balancers in one environment, specifying the certificate in three different ways. Can you retest to see if you see different results with my code change?

Also, the unit tests should pass now.

jparsons04 commented 6 years ago

The terraform file I have used to test is now this:

resource "layer0_environment" "env_a" {
  name = "env_a"
}

resource "layer0_load_balancer" "lb_iam_arn" {
  name        = "lb_iam_arn"
  environment = "${layer0_environment.env_a.id}"
  private     = false

  port {
    host_port       = 443
    container_port  = 80
    protocol        = "https"
    certificate_arn = "arn:aws:iam::856306994068:server-certificate/l0/l0-jparsonsdev/l0-jparsonsdev-api"
  }
}

resource "layer0_load_balancer" "lb_acm_arn" {
  name        = "lb_acm_arn"
  environment = "${layer0_environment.env_a.id}"
  private     = false

  port {
    host_port       = 443
    container_port  = 80
    protocol        = "https"
    certificate_arn = "arn:aws:acm:us-west-2:856306994068:certificate/bfcb2ccd-8779-47bc-b62f-ba99e5c8a73e"
  }
}

Now with an explicit requirement to provide the ARN of a certificate rather than its friendly name, we can test adding either an IAM or ACM cert using its ARN.