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

Impossible to create LBs of type network #1838

Closed
elsbrock opened this issue Oct 9, 2017 · 11 comments
Closed

Impossible to create LBs of type network #1838

elsbrock opened this issue Oct 9, 2017 · 11 comments
Labels
bug Addresses a defect in current functionality.

Comments

@elsbrock
Copy link

elsbrock commented Oct 9, 2017

I just gave #1806 a spin (on 5a7d0cc). With the current state it looks like I am unable to create LBs of type network:

  • Health check path and matcher only exist for LBs of type http[s], resulting in a crash
  • The target group protocol must support protocols of type tcp
  • Setting subnets is not supported for LBs of type network, instead availability zones have to be provided

First two things are easy to fix, however I don't know how to properly address the last point, because AZs cannot be passed as part of the CreateLoadBalancerInput and instead have to be set via lb.SetAvailabilityZones.

@elsbrock
Copy link
Author

elsbrock commented Oct 9, 2017

Thanks, according to #1618 I was under the impression that this should already work in latest master.

@jwinter
Copy link

jwinter commented Oct 10, 2017

I was under the same impression, i.e. that after #1806 was merged Network Load Balancers would work. @bubunyo do you know of any additional work in progress for NLB support? I don't see any other Issues or PRs.

@bubunyo
Copy link

bubunyo commented Oct 10, 2017

I don't know of any other WIP for NLB support. I don't exactly know what exactly the hold up is for the release for version 1.0.1 . I also don't know what the eta for that is. Perhaps @radeksimko can help with that.

@radeksimko
Copy link
Member

radeksimko commented Oct 10, 2017

I will take a look this week, hopefully, as I wanted to cut a release.

Lacking features can certainly be added in future releases but if there's a known crash or significantly buggy behaviour introduced by #1806 then that would make me reconsider and fix the bug before cutting the release.

Do you mind providing an example config that causes a crash @else ?

Thanks.

@radeksimko radeksimko added bug Addresses a defect in current functionality. waiting-response Maintainers are waiting on response from community or contributor. labels Oct 10, 2017
@elsbrock
Copy link
Author

elsbrock commented Oct 13, 2017

Sorry for being unclear, the crash only occured during my experiments when I allowed target group protocol tcp which is necessary for NLBs.

This is the diff that I had to apply in order to get terraform actually do something and not complain about erroneously configured provisioners:

diff --git a/aws/resource_aws_lb_target_group.go b/aws/resource_aws_lb_target_group.go
index 9cab9601..132fab88 100644
--- a/aws/resource_aws_lb_target_group.go
+++ b/aws/resource_aws_lb_target_group.go
@@ -420,11 +420,11 @@ func validateAwsLbTargetGroupPort(v interface{}, k string) (ws []string, errors
 
 func validateAwsLbTargetGroupProtocol(v interface{}, k string) (ws []string, errors []error) {
 	protocol := strings.ToLower(v.(string))
-	if protocol == "http" || protocol == "https" {
+	if protocol == "http" || protocol == "https" || protocol == "tcp" {
 		return
 	}
 
-	errors = append(errors, fmt.Errorf("%q must be either %q or %q", k, "HTTP", "HTTPS"))
+	errors = append(errors, fmt.Errorf("%q must be either %q or %q", k, "HTTP", "HTTPS", "TCP"))
 	return
 }
 
@@ -479,13 +479,15 @@ func flattenAwsLbTargetGroupResource(d *schema.ResourceData, meta interface{}, t
 
 	healthCheck := make(map[string]interface{})
 	healthCheck["interval"] = *targetGroup.HealthCheckIntervalSeconds
-	healthCheck["path"] = *targetGroup.HealthCheckPath
+	if *targetGroup.Protocol != "TCP" {
+		healthCheck["path"] = *targetGroup.HealthCheckPath
+		healthCheck["matcher"] = *targetGroup.Matcher.HttpCode
+	}
 	healthCheck["port"] = *targetGroup.HealthCheckPort
 	healthCheck["protocol"] = *targetGroup.HealthCheckProtocol
 	healthCheck["timeout"] = *targetGroup.HealthCheckTimeoutSeconds
 	healthCheck["healthy_threshold"] = *targetGroup.HealthyThresholdCount
 	healthCheck["unhealthy_threshold"] = *targetGroup.UnhealthyThresholdCount
-	healthCheck["matcher"] = *targetGroup.Matcher.HttpCode
 	d.Set("health_check", []interface{}{healthCheck})
 
 	attrResp, err := elbconn.DescribeTargetGroupAttributes(&elbv2.DescribeTargetGroupAttributesInput{
diff --git a/website/docs/r/lb.html.markdown b/website/docs/r/lb.html.markdown
index c155a15e..ee193c14 100644
--- a/website/docs/r/lb.html.markdown
+++ b/website/docs/r/lb.html.markdown
@@ -50,7 +50,7 @@ Terraform will autogenerate a name beginning with `tf-lb`.
 * `name_prefix` - (Optional) Creates a unique name beginning with the specified prefix. Conflicts with `name`.
 * `internal` - (Optional) If true, the LB will be internal.
 * `load_balancer_type` - (Optional) The type of load balancer to create. Possible values are `application` or `network`. The default value is `application`.
-* `security_groups` - (Optional) A list of security group IDs to assign to the LB.
+* `security_groups` - (Optional) A list of security group IDs to assign to the LB. Only allowed for type `application`.
 * `access_logs` - (Optional) An Access Logs block. Access Logs documented below.
 * `subnets` - (Optional) A list of subnet IDs to attach to the LB.
 * `subnet_mapping` - (Optional) A subnet mapping block as documented below.
diff --git a/website/docs/r/lb_listener.html.markdown b/website/docs/r/lb_listener.html.markdown
index f9398e05..7b7f82ab 100644
--- a/website/docs/r/lb_listener.html.markdown
+++ b/website/docs/r/lb_listener.html.markdown
@@ -44,7 +44,7 @@ The following arguments are supported:
 
 * `load_balancer_arn` - (Required, Forces New Resource) The ARN of the load balancer.
 * `port` - (Required) The port on which the load balancer is listening.
-* `protocol` - (Optional) The protocol for connections from clients to the load balancer. Valid values are `HTTP` and `HTTPS`. Defaults to `HTTP`.
+* `protocol` - (Optional) The protocol for connections from clients to the load balancer. Valid values are `HTTP`, `HTTPS` and `TCP`. Defaults to `HTTP`. Note that the set of available protocols depends on the target groups attached to the load balancer.
 * `ssl_policy` - (Optional) The name of the SSL Policy for the listener. Required if `protocol` is `HTTPS`.
 * `certificate_arn` - (Optional) The ARN of the SSL server certificate. Exactly one certificate is required if the protocol is HTTPS.
 * `default_action` - (Required) An Action block. Action blocks are documented below.
diff --git a/website/docs/r/lb_target_group.html.markdown b/website/docs/r/lb_target_group.html.markdown
index 15590ce5..257d9d7d 100644
--- a/website/docs/r/lb_target_group.html.markdown
+++ b/website/docs/r/lb_target_group.html.markdown
@@ -34,7 +34,7 @@ The following arguments are supported:
 * `name` - (Optional, Forces new resource) The name of the target group. If omitted, Terraform will assign a random, unique name.
 * `name_prefix` - (Optional, Forces new resource) Creates a unique name beginning with the specified prefix. Conflicts with `name`.
 * `port` - (Required) The port on which targets receive traffic, unless overridden when registering a specific target.
-* `protocol` - (Required) The protocol to use for routing traffic to the targets.
+* `protocol` - (Required) The protocol to use for routing traffic to the targets. Valid protocols are `HTTP`, `HTTPS` and `TCP`.
 * `vpc_id` - (Required) The identifier of the VPC in which to create the target group.
 * `deregistration_delay` - (Optional) The amount time for Elastic Load Balancing to wait before changing the state of a deregistering target from draining to unused. The range is 0-3600 seconds. The default value is 300 seconds.
 * `stickiness` - (Optional) A Stickiness block. Stickiness blocks are documented below.

What's missing here is the possibility to set the subnet. Apparently this is only possible by setting availability zones?

@dblooman
Copy link
Contributor

@else could you try my branch #1884

@bploetz
Copy link

bploetz commented Oct 19, 2017

I created a Network Load Balancer manually via the AWS console and tried using an aws_lb Data Source to work around this issue, but that doesn't work either. Details here:

#1912 (comment)

@dblooman
Copy link
Contributor

@bploetz i will have a look at the data source later if i get a chance

@Ehekatl
Copy link

Ehekatl commented Oct 25, 2017

There are a few other cases didn't handle well:

  • even with the patch from @DaveBlooman , I still need apply twice to finish nlb creation(wait more than 2 mins, time out and trying to do a update, which result Unable to update subnets error)
  • can't change anything on an existing nlb, so any change on nlb should result a force recreate
  • health check for nlb target group is tricky:
    • it requires healthy_threshold the same as unhealthy_threshold
    • for HTTP/HTTPS healthcheck protocol, matcher is required parameter when sending the api request, but it has to be "200-399"
    • timeout value can't be set or change when healthcheck protocol is TCP

@catsby
Copy link
Contributor

catsby commented Nov 14, 2017

Hello all! #2251 is a continuation of #1884 which addressed these issues, so Network Load balancers should be fully supported. There's another PR #1941 to throw an error if a user tries to update subnets on aws_lb of type network, which is not currently supported by the AWS API.

If there is anything else you see missing please let me know!

@ghost
Copy link

ghost commented Apr 10, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 10, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality.
Projects
None yet
Development

No branches or pull requests

9 participants