Don't allow dot at the end of domain name #359

dak1n1 · 2018-09-27T21:22:38Z

This will prevent errors when creating the S3 bucket,
since the S3 bucket name may not end in dot.

staebler · 2018-09-27T23:34:58Z

pkg/types/config/validate.go

This is an improvement on the error message. However, if we can, we should improve the validation of the user input earlier to catch invalid bucket names. The current validation of the base domain currently allows a trailing dot. If a trailing dot results in an invalid S3 bucket name later, then either (1) we need to reject a base domain with a trailing dot or (2) we need to accommodate the trailing dot to make a valid S3 bucket name.

This is an improvement on the error message. However...

@dak1n1, let me know if you want to pick these suggestions up or punt them to futute work. Theyre good suggestions, but I'm fine getting there in baby steps.

@wking I like the idea of catching the error earlier. I'll be in the SFO office next week. This could be a good task for me to work on, since this is my very first golang commit.

dak1n1 · 2018-09-28T19:04:26Z

I made the validation error happen much sooner than before. Here's the output now:

[fedora@ip-172-31-18-42 installer (throwaway %)]$ time bin/openshift-install cluster --log-level=debug
INFO[0000] Fetching Terraform Variables...              
DEBU[0000] Generating dependencies of Terraform Variables... 
INFO[0000]   Fetching Install Config...                 
DEBU[0000]   Generating dependencies of Install Config... 
INFO[0000]     Fetching Cluster ID...                   
DEBU[0000]     Generating Cluster ID...                 
INFO[0000]     Fetching Email Address...                
DEBU[0000]     Generating Email Address...              
INFO[0000]     Fetching Password...                     
DEBU[0000]     Generating Password...                   
INFO[0000]     Fetching SSH Key...                      
DEBU[0000]     Generating SSH Key...                    
INFO[0000]     Fetching Base Domain...                  
DEBU[0000]     Generating Base Domain...                
FATA[0000] failed to generate asset: failed to generate asset "Base Domain": invalid domain name: 'dakinitest.aws.openshift.com.' 

real    0m0.057s
user    0m0.063s
sys     0m0.005s

dak1n1 · 2018-09-28T19:10:46Z

I'm working on the failing unit test now...

wking · 2018-09-28T20:16:25Z

pkg/types/config/validate.go

nit: '%s' -> %q to let Go quote for you ;).

wking · 2018-09-28T20:19:10Z

pkg/types/config/validate.go

note to self: Comments are usually a hint that the initial dev felt this was surprising/important. But checking the Git logs, this is from d9359cb (coreos/tectonic-installer#3085), and neither the commit message nor PR discussion has anything to say about why they specifically allowed trailing dots then.

dak1n1 · 2018-09-28T20:33:12Z

@wking is there a way I can run the unit tests manually before pushing? It's currently failing because I changed the error message from invalid domain name to invalid domain name '%s'. It expects the old value still.

It's located here: pkg/types/config/validate_test.go const invalidDomainMsg = "invalid domain name".

wking · 2018-09-28T21:01:42Z

... is there a way I can run the unit tests manually before pushing?

On your branch in a checkout in my GOPATH:

$ git checkout origin/pr/359
$ echo $GOPATH
/home/trking/.local/lib/go
$ pwd
/home/trking/.local/lib/go/src/github.com/openshift/installer

Run the tests on everything under pkg/:

$ go test ./pkg/...
...
--- FAIL: TestDomainName (0.01s)
	validate_test.go:680: For DomainName("."), expected "invalid domain name", got "invalid domain name: '\".\"'"
	validate_test.go:680: For DomainName("日本語"), expected "invalid domain name", got "invalid domain name: '\"日本語\"'"
	validate_test.go:680: For DomainName("日本語.com"), expected "invalid domain name", got "invalid domain name: '\"日本語.com\"'"
	validate_test.go:680: For DomainName("abc.日本語.com"), expected "invalid domain name", got "invalid domain name: '\"abc.日本語.com\"'"
	validate_test.go:680: For DomainName("a日本語a.com"), expected "invalid domain name", got "invalid domain name: '\"a日本語a.com\"'"
	validate_test.go:680: For DomainName("1.2.3.4."), expected "", got "invalid domain name: '\"1.2.3.4.\"'"
	validate_test.go:680: For DomainName("abc."), expected "", got "invalid domain name: '\"abc.\"'"
	validate_test.go:680: For DomainName("abc.com."), expected "", got "invalid domain name: '\"abc.com.\"'"
	validate_test.go:680: For DomainName(".abc"), expected "invalid domain name", got "invalid domain name: '\".abc\"'"
	validate_test.go:680: For DomainName(".abc.com"), expected "invalid domain name", got "invalid domain name: '\".abc.com\"'"
	validate_test.go:680: For DomainName(".abc.com"), expected "invalid domain name", got "invalid domain name: '\".abc.com\"'"
--- FAIL: TestEmail (0.00s)
	validate_test.go:680: For Email("a@日本語.com"), expected "invalid domain name", got "invalid domain name: '\"日本語.com\"'"
	validate_test.go:680: For Email("[email protected]"), expected "invalid domain name", got "invalid domain name: '\".com\"'"
FAIL
FAIL	github.com/openshift/installer/pkg/types/config	0.110s
...

In this case, you'll probably have to update those tests to look for "message starts with {regexp}" instead of "message exactly matches".

wking · 2018-09-28T21:04:28Z

In this case, you'll probably have to update those tests to look for "message starts with {regexp}" instead of "message exactly matches".

The already-vendored assert.Regexp may be useful for this.

dak1n1 · 2018-10-01T18:56:08Z

I've been given some other tasks to work on this sprint, so I'm going to leave the regex stuff out for now. It looks like it's beyond my current ability to implement. But I hope to improve error messaging later. For now, I'll just proceed with the default error about the invalid domain.

crawford · 2018-10-01T20:28:44Z

Technically, the trailing dot is valid (and in many cases it is preferred). We should probably just fix the creation of the S3 bucket since we no longer rely on CNAMEs (which required the S3 bucket to match the domain name).

dak1n1 · 2018-10-01T20:37:54Z

@crawford in that case, maybe I should change the way the S3 bucket name is generated? Because right now it uses the domain name to generate the S3 bucket name, which results in an invalid S3 bucket name. I could remove the trailing dot as part of constructing the S3 bucket name. Does that sound alright?

crawford · 2018-10-01T20:42:09Z

@dak1n1 I would just drop the domain name from the bucket name.

wking · 2018-10-02T04:49:35Z

I would just drop the domain name from the bucket name.

Why not just drop the bucket name entirely? Based on this example, we can use bucket IDs for a bucket-object's bucket input. By leaving off the bucket's optional bucket input, Terraform will give it a random, unique name, which we'll never use. So something like:

diff --git a/data/data/aws/bootstrap/variables.tf b/data/data/aws/bootstrap/variables.tf
index fd7b172..f775eeb 100644
--- a/data/data/aws/bootstrap/variables.tf
+++ b/data/data/aws/bootstrap/variables.tf
@@ -10,7 +10,7 @@ variable "associate_public_ip_address" {
 
 variable "bucket" {
   type        = "string"
-  description = "The S3 bucket name for bootstrap ignition file."
+  description = "The S3 bucket name or ID for bootstrap ignition file."
 }
 
 variable "cluster_name" {
diff --git a/data/data/aws/main.tf b/data/data/aws/main.tf
index d87c01a..d91715f 100644
--- a/data/data/aws/main.tf
+++ b/data/data/aws/main.tf
@@ -20,7 +20,7 @@ module "bootstrap" {
 
   ami                         = "${var.tectonic_aws_ec2_ami_override}"
   associate_public_ip_address = "${var.tectonic_aws_endpoints != "private"}"
-  bucket                      = "${aws_s3_bucket.tectonic.bucket}"
+  bucket                      = "${aws_s3_bucket.bootstrap.id}"
   cluster_name                = "${var.tectonic_cluster_name}"
   elbs                        = "${module.vpc.aws_lbs}"
   elbs_length                 = "${module.vpc.aws_lbs_length}"
@@ -131,9 +131,7 @@ resource "aws_route53_zone" "tectonic_int" {
     ), var.tectonic_aws_extra_tags)}"
 }
 
-resource "aws_s3_bucket" "tectonic" {
-  bucket = "${lower(var.tectonic_cluster_name)}.${var.tectonic_base_domain}"
-
+resource "aws_s3_bucket" "bootstrap" {
   acl = "private"
 
   tags = "${merge(map(

Then we could drop validateS3Bucket completely and leave ValidateDomainName alone. Does that sound plausible?

dak1n1 · 2018-10-02T18:42:54Z

So I already have something that is working for this, but I'm open to changing it if it's not the direction you guys want to go. What I did is just drop the trailing dot on the S3 bucket name. This keeps the S3 name consistent with how it's been in the past (clustername + domain name). It's easy for an Ops person to visually identify and search for the bucket name, without knowing the cluster ID. And it's working in my testing.

When the user passes the installer a domain name ending in dot `.`, the S3 bucket name generated by the installer is invalid. This commit automatically drops the trailing dot to create valid S3 bucket names.

crawford · 2018-10-02T18:55:01Z

For what it's worth, we will delete this bucket shortly after cluster installation, so I don't think I would worry so much about day-2 operations.

wking · 2018-10-02T19:24:17Z

I'm fine punting larger name adjustments to follow-up work.

/lgtm

openshift-ci-robot · 2018-10-02T19:24:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dak1n1, 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~~ [wking]

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

Alex gave the history behind our previous bucket name [1]: We should probably just fix the creation of the S3 bucket since we no longer rely on CNAMEs (which required the S3 bucket to match the domain name). But now we can just let AWS pick a random bucket name for us. I've also dropped the no-longer-used S3Bucket validator. [1]: openshift#359 (comment)

openshift-ci-robot requested review from abhinavdahiya and steveej September 27, 2018 21:23

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 27, 2018

staebler reviewed Sep 27, 2018

View reviewed changes

dak1n1 changed the title ~~Added S3 bucket name to error message~~ [WIP] Don't allow dot at the end of domain name Sep 28, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 28, 2018

wking reviewed Sep 28, 2018

View reviewed changes

pkg/types/config/validate.go Outdated

Copy link

Member

wking Sep 28, 2018

Choose a reason for hiding this comment

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

nit: '%s' -> %q to let Go quote for you ;).

dak1n1 reacted with thumbs up emoji

wking reviewed Sep 28, 2018

View reviewed changes

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 1, 2018

dak1n1 changed the title ~~[WIP] Don't allow dot at the end of domain name~~ Don't allow dot at the end of domain name Oct 1, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 1, 2018

openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 2, 2018

Sanitize S3 bucket name by removing trailing dot

02c1145

When the user passes the installer a domain name ending in dot `.`, the S3 bucket name generated by the installer is invalid. This commit automatically drops the trailing dot to create valid S3 bucket names.

openshift-ci-robot assigned wking Oct 2, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 2, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2018

openshift-merge-robot merged commit 748b9d7 into openshift:master Oct 2, 2018

wking mentioned this pull request Oct 18, 2018

kubernetes.io/cluster/CLUSTER_NAME tag missing from EIPs, NAT GWs, S3 buckets and R53 zones #458

Closed

wking mentioned this pull request Oct 18, 2018

data/aws: Drop the bucket name #489

Merged

wking mentioned this pull request Dec 11, 2018

Trailing periods in base domains #831

Closed

Don't allow dot at the end of domain name #359

Don't allow dot at the end of domain name #359

Uh oh!

Conversation

dak1n1 commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staebler Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

wking Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

dak1n1 Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

dak1n1 commented Sep 28, 2018

Uh oh!

dak1n1 commented Sep 28, 2018

Uh oh!

wking Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

wking Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

dak1n1 commented Sep 28, 2018

Uh oh!

wking commented Sep 28, 2018

Uh oh!

wking commented Sep 28, 2018

Uh oh!

dak1n1 commented Oct 1, 2018

Uh oh!

crawford commented Oct 1, 2018

Uh oh!

dak1n1 commented Oct 1, 2018

Uh oh!

crawford commented Oct 1, 2018

Uh oh!

wking commented Oct 2, 2018

Uh oh!

dak1n1 commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crawford commented Oct 2, 2018

Uh oh!

wking commented Oct 2, 2018

Uh oh!

openshift-ci-robot commented Oct 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dak1n1 commented Sep 27, 2018 •

edited

Loading

dak1n1 commented Oct 2, 2018 •

edited

Loading