cluster name cannot contain dash (-)

[antoniobeyah]

When loading the latest image, I get the error:

F0911 19:30:22.099545       1 alb-controller.go:79] [ALB-INGRESS] [controller] [ERROR]: Cluster name cannot contain '-'

This issue is not present on the 0.8 image. Can some details be added on why this is a restriction and/or just remove the check?

Ref

Full Log (latest image b58d0f278b):

-> kubectl logs alb-ingress-controller-276746378-09qt2 -n cloudplatform
I0911 19:30:22.064659       1 main.go:24] [ALB-INGRESS] [controller] [INFO]: Log level read as "", defaulting to INFO. To change, set LOG_LEVEL environment variable to WARN, ERROR, or DEBUG.
I0911 19:30:22.064963       1 launch.go:108] &{ALB Ingress Controller 1.0.0 git-00000000 git://github.com/coreos/alb-ingress-controller}
I0911 19:30:22.064984       1 launch.go:111] Watching for ingress class: alb
I0911 19:30:22.065211       1 launch.go:270] Creating API server client for https://10.0.0.1:443
I0911 19:30:22.081043       1 launch.go:127] validated cloudplatform/alb-ingress-controller-default-backend as the default backend
I0911 19:30:22.099498       1 alb-controller.go:67] [ALB-INGRESS] [controller] [INFO]: Ingress class set to alb
F0911 19:30:22.099545       1 alb-controller.go:79] [ALB-INGRESS] [controller] [ERROR]: Cluster name cannot contain '-'

`https://github.com/coreos/alb-ingress-controller/blob/a8342042c7be1ca6ea1ab935924a440c0d7d4ea9/pkg/controller/alb-controller.go#L76-L78

[joshrosso]

@antoniobeyah Please note for your potential PR that the restriction on the dash is rooted in how resolution of the loadbalancer is done in AWS.

Specifically for when we're assembling an ingress list from existing AWS resources.

https://github.com/coreos/alb-ingress-controller/blob/0e1c716458fcd0fd228b1690be76e516f5699b1a/pkg/albingresses/albingresses.go#L81-L86

We use the dash to discern cluster name from other properties.

https://github.com/coreos/alb-ingress-controller/blob/53b0c222aaec9b00605b1f1bf3d57c256e4de78c/pkg/aws/elbv2/elbv2.go#L86-L108

The PR would need to take this in account. I recall ALB naming being quite restrictive, so this might be something to look into.

[alexmbird]

I've just run into this. Personally I'd be happy with any separator character to use in CLUSTER_NAME. How about an underscore?

NB: just tried an underscore and it results in ValidationError: The load balancer name 'kcl_sand_a-default-myappa-52a9' can only contain characters that are alphanumeric characters and hyphens(-)

[joshrosso]

Right, that's our biggest limitation at the moment.

Really the CLUSTER_NAME is our way of qualifying that ALB resources belong to an instance of an ALB controller. As I was working on automated subnet detection (meaning qualifying subnets to deploy the ALB to without an annotation #7) CLUSTER_NAME is even more relevant as method that provisioned the kubernetes cluster may have set these tags with a CLUSTER_NAME greater than 11 characters and containing -s.

I'd like to propose we add another environment variable CONTROLLER_ID that has the limitation of no -s and 11 characters. CLUSTER_NAME can then be unrestricted (or at least more so).

I'd like to try implementing this today so let me know your thoughts @antoniobeyah as I know you may have already started on a PR.

[antoniobeyah]

Instead of trying to control the name of the LB based on the cluster name would it make more sense to just use a tag on the load balancer? That way you can just do an exact match against the tag value?

[joshrosso]

While that's possible, I think I still like the idea of being able to login to the AWS console and see clearly at a glance what LBs belong to what controller.

I'd rather not just drop a bunch of arbitrary UID(s) or hashs of some sort.

I'm open to adding CLUSTER_NAME as a tag, but would prefer a configurable variable of some sort (e.g. CONTROLLER_ID) to be attached to the name. If CONTROLLER_ID.

This would also prevent us from having to rework a bunch of the assembly logic today.

Thoughts?

[antoniobeyah]

In that case my vote would be to use the cluster name tag for discovery and just stripping the invalid characters from the LB name for display (use CONTROLLER_ID if present). There is already some precedence for using a cluster tag for cluster specific resources, and it fits naturally into the way AWS expects you to work.

Some use cases for the cluster tag:

• Tracking costs for a given cluster (the tag can be added as a cost allocation tag and be automatically calculated in aws billing)
• IAM permissions can be assigned and determined based on tags
• Aligns with other products (i.e. Tectonic adds in a kubernetes.io/cluster/<cluster_name> tag)
• No need for a person to understand the logic if the name creation when trying to narrow resources down to a specific cluster

I'm not opposed to having a CONTROLLER_ID but I would ask that it not be a required value and we use the cluster name tag as the discovery mechanism because it aligns with how a person would discover the resources if they were drilling down to a specific cluster.

[joshrosso]

@antoniobeyah, stripping the invalid characters for just the alb naming sgtm.

[joshrosso]

Let me know if/when you start work on this so I don't duplicate effort.

[joshrosso]

This limitation has been removed in 918d034

[joshrosso]

Enhanced in aca9241 to ensure ALB name always complies with ^[a-zA-Z0-9]+$