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: Document and validate ELB ssl_cert and protocol require #3887

Merged
merged 3 commits into from
Nov 12, 2015

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Nov 12, 2015

While not explicitly documented, the ssl_certificate_id attribute of an ELB Listener is only valid if the protocols are either SSL or HTTPS. If you include ssl_certificate_id in a listener of any other protocol, AWS will not return an error, however, follow up terraform plan will reveal that the Listener returned will have had it's ssl_certificate_id omitted.

For example:

resource "aws_elb" "ourapp" {
  name = "terraform-asg-deployment-example"
  availability_zones = ["us-west-2a"]
  cross_zone_load_balancing = true

  listener {
    instance_port = 443
    instance_protocol = "tcp"
    lb_port = 443
    lb_protocol = "tcp"
    ssl_certificate_id = "<some cert arn>" # assume valid cert
  }
}

This will succeed, however, the state file will have an empty ssl_certificate_id:

listener.# = 1
  listener.610193557.instance_port = 443
  listener.610193557.instance_protocol = tcp
  listener.610193557.lb_port = 443
  listener.610193557.lb_protocol = tcp
  listener.610193557.ssl_certificate_id =

which gives us a plan loop:

~ aws_elb.ourapp
    listener.610193557.instance_port:      "443" => "0"
    listener.610193557.instance_protocol:  "tcp" => ""
    listener.610193557.lb_port:            "443" => "0"
    listener.610193557.lb_protocol:        "tcp" => ""
    listener.610193557.ssl_certificate_id: "" => ""
    listener.613970091.instance_port:      "" => "443"
    listener.613970091.instance_protocol:  "" => "tcp"
    listener.613970091.lb_port:            "" => "443"
    listener.613970091.lb_protocol:        "" => "tcp"
    listener.613970091.ssl_certificate_id: "" => "<some cert arn>"

With this PR, we get a validation error in Terraform:

1 error(s) occurred:

* aws_elb.ourapp: [ERR] Invalid ssl_certificate_id / Protocol combination. Must be either HTTPS or SSL

This PR is built off of #3863

@jen20
Copy link
Contributor

jen20 commented Nov 12, 2015

LGTM

if valid {
listeners = append(listeners, l)
} else {
return nil, fmt.Errorf("[ERR] Invalid ssl_certificate_id / Protocol combination. Must be either HTTPS or SSL")
Copy link
Contributor

Choose a reason for hiding this comment

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

This message isn't the clearest, I think. There are two properties mentioned in the first part of the message, and then a statement in the second part about only one of them, but which one isn't mentioned. Also not sure why "protocol" got an uppercase P here.

Suggest instead: "ssl_certificate_id may be set only when protocol is 'https' or 'ssl'".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I updated the wording in 5cafe74 , thanks!

catsby added a commit that referenced this pull request Nov 12, 2015
providers/aws: Document and validate ELB ssl_cert and protocol require
@catsby catsby merged commit 1488bbf into master Nov 12, 2015
@catsby catsby deleted the elb-ssl-cert-fix branch November 12, 2015 20:28
@mrwilby
Copy link

mrwilby commented Nov 28, 2015

Is this check case-sensitive? My previously valid Terraform scripts fail to deploy after upgrading to 0.6.7 because I am using "HTTPS" rather than "https" in my listener block.

Does AWS really reject "HTTPS" (NB: Given this used to work, I suspect the answer is no).

Easy for me to fix this I guess, but I wonder if the code could be a bit more tolerant of what is effectively an enum with the 'wrong' capitalization.

@ghost
Copy link

ghost commented Apr 29, 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 29, 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.

4 participants