data/aws/vpc: Sanitize cluster name for aws_lb and aws_lb_target_group #1089

wking · 2019-01-17T07:03:01Z

For folks who use multi-component cluster names like jliu-beta1.test20190117, this change avoids raising errors like:

ERROR Error: module.vpc.aws_lb.api_external: only alphanumeric characters and hyphens allowed in "name": "jliu-beta1.test20190117-ext"
ERROR
ERROR Error: module.vpc.aws_lb.api_internal: only alphanumeric characters and hyphens allowed in "name": "jliu-beta1.test20190117-int"
ERROR
ERROR Error: module.vpc.aws_lb_target_group.api_external: only alphanumeric characters and hyphens allowed in "name"
ERROR
ERROR Error: module.vpc.aws_lb_target_group.api_internal: only alphanumeric characters and hyphens allowed in "name"
ERROR
ERROR Error: module.vpc.aws_lb_target_group.services: only alphanumeric characters and hyphens allowed in "name"

I'm using HCL's replace function with a regular expression to replace everything except the characters the error message claims are supported. I've allowed both upper and lower case, because RFC 952 has:

A "name" (Net, Host, Gateway, or Domain name) is a text string up to 24 characters drawn from the alphabet (A-Z), digits (0-9), minus sign (-), and period (.). Note that periods are only allowed when they serve to delimit components of "domain style names". (See RFC-921, "Domain Name System Implementation Schedule", for background). No blank or space characters are permitted as part of a name. No distinction is made between upper and lower case.

But I haven't checked IsDNS1123Subdomain to see how it handles case.

staebler · 2019-01-17T19:49:17Z

data/data/aws/vpc/common.tf

Can we name this variable something else, like lb_base_name? Having a variable with the same name in var and local and using them both in places makes it difficult to know which one is the right one to use where.

Can we name this variable something else, like lb_base_name?

Done with 0615c32 -> 7c3a98b.

For folks who use multi-component cluster names like jliu-beta1.test20190117, this change avoids raising errors like [1]: ERROR Error: module.vpc.aws_lb.api_external: only alphanumeric characters and hyphens allowed in "name": "jliu-beta1.test20190117-ext" ERROR ERROR Error: module.vpc.aws_lb.api_internal: only alphanumeric characters and hyphens allowed in "name": "jliu-beta1.test20190117-int" ERROR ERROR Error: module.vpc.aws_lb_target_group.api_external: only alphanumeric characters and hyphens allowed in "name" ERROR ERROR Error: module.vpc.aws_lb_target_group.api_internal: only alphanumeric characters and hyphens allowed in "name" ERROR ERROR Error: module.vpc.aws_lb_target_group.services: only alphanumeric characters and hyphens allowed in "name" I'm using HCL's replace function [2] with a regular expression to replace everything except the characters the error message claims are supported. I've allowed both upper and lower case, because [3] has: A "name" (Net, Host, Gateway, or Domain name) is a text string up to 24 characters drawn from the alphabet (A-Z), digits (0-9), minus sign (-), and period (.). Note that periods are only allowed when they serve to delimit components of "domain style names". (See RFC-921, "Domain Name System Implementation Schedule", for background). No blank or space characters are permitted as part of a name. No distinction is made between upper and lower case. But I haven't checked IsDNS1123Subdomain to see how it handles case. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1666956#c0 [2]: https://www.terraform.io/docs/configuration/interpolation.html#replace-string-search-replace- [3]: https://tools.ietf.org/html/rfc952

openshift-ci-robot · 2019-01-17T21:46:53Z

@wking: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`7c3a98b`	link	`/test e2e-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

crawford · 2019-01-17T23:16:37Z

data/data/aws/vpc/common.tf


 // Only reference data sources which are gauranteed to exist at any time (above) in this locals{} block
 locals {
+  lb_base_name = "${replace(var.cluster_name, "/[^A-Za-z0-9-]/", "-")}"


nit: Is that last hyphen in the regex necessary?

No. As it stands, it is "replace all but {legal chars, including hyphens} with hyphens". Without the trailing -, it wold be replacing the input - with -. You'd want the trailing - if the replacement was an empty string, etc. Did you want me to change it?

Oh, duh. Ignore me.

crawford · 2019-01-17T23:16:51Z

/approve
/hold

openshift-ci-robot · 2019-01-17T23:17:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [crawford,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

abhinavdahiya · 2019-01-17T23:40:45Z

we depend on predictive nature of some AWS resources, like IAM roles, subnets. SO rather than doing this for LB in terraform, Can we do this in Go? Maybe create a sanitized cluster-name that we can use in all our resources that we think are sensitive to these kind of things..

wking · 2019-01-17T23:45:34Z

Can we do this in Go?

Can we punt on that until we need Go to reference a sanitized name?

abhinavdahiya · 2019-01-18T00:04:08Z

Can we do this in Go?

Can we punt on that until we need Go to reference a sanitized name?

IMO i don't want any new addition to terraform codebase wrt changes like sanitization... but i'm okay punting if others think we can fix it later, no strong opinions ;)

wking · 2019-01-18T00:21:49Z

IMO i don't want any new addition to terraform codebase wrt changes like sanitization...

Cheap compromise would be dropping cluster-name from those entries like we did for buckets in #489 ;). My issue with plumbing back into Go before we have a consumer is that I don't know how far back to take it. Just to pkg/tfvars? All the way back to pkg/assets/installconfig (although obviously not into InstallConfig?

Personally, I don't see a lot of maintenance space between ${var.cluster_name}-int and ${replace(var.cluster_name, "/[^A-Za-z0-9-]/", "-")}-int in Terraform. Both are "tweak the user-provided cluster name a bit in Terraform to generate a name for this resource".

openshift-ci-robot · 2019-02-23T05:13:17Z

@wking: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

wking · 2019-02-26T09:12:40Z

Addressed by a37a4eb (#1280).

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 17, 2019

openshift-ci-robot requested review from hardys and smarterclayton January 17, 2019 07:03

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2019

staebler reviewed Jan 17, 2019

View reviewed changes

wking force-pushed the cluster-names-with-dots branch from 0615c32 to 7c3a98b Compare January 17, 2019 20:34

crawford reviewed Jan 17, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 17, 2019

wking added the platform/aws label Jan 18, 2019

This was referenced Feb 19, 2019

*: use a trimmed, unique cluster name for ELB names #1277

Closed

*: use a human friendly ID to tag and name cloud resources. #1280

Merged

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2019

wking closed this Feb 26, 2019

wking deleted the cluster-names-with-dots branch February 26, 2019 09:12

data/aws/vpc: Sanitize cluster name for aws_lb and aws_lb_target_group #1089

data/aws/vpc: Sanitize cluster name for aws_lb and aws_lb_target_group #1089

Conversation

wking commented Jan 17, 2019

Uh oh!

staebler Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

wking Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jan 17, 2019

Uh oh!

crawford Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

wking Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crawford Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

crawford commented Jan 17, 2019

Uh oh!

openshift-ci-robot commented Jan 17, 2019

Uh oh!

abhinavdahiya commented Jan 17, 2019

Uh oh!

wking commented Jan 17, 2019

Uh oh!

abhinavdahiya commented Jan 18, 2019

Uh oh!

wking commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Feb 23, 2019

Uh oh!

wking commented Feb 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wking Jan 17, 2019 •

edited

Loading

wking commented Jan 18, 2019 •

edited

Loading