Merge pull request #1255 from staebler/validate_cluster_name_from_yaml

types: validate cluster name in InstallConfig

pkg/asset/installconfig/basedomain.go

@@ -46,7 +46,7 @@ func (a *baseDomain) Generate(parents asset.Parents) error {
 				Help:    "The base domain of the cluster. All DNS records will be sub-domains of this base and will also include the cluster name.",
 			},
 			Validate: survey.ComposeValidators(survey.Required, func(ans interface{}) error {
-				return validate.DomainName(ans.(string))
+				return validate.DomainName(ans.(string), true)
 			}),
 		},
 	}, &a.BaseDomain)

pkg/asset/installconfig/clustername.go

@@ -4,6 +4,7 @@ import (
 	survey "gopkg.in/AlecAivazis/survey.v1"
 
 	"github.com/openshift/installer/pkg/asset"
+	"github.com/openshift/installer/pkg/types/validation"
 	"github.com/openshift/installer/pkg/validate"
 )
 
@@ -15,19 +16,24 @@ var _ asset.Asset = (*clusterName)(nil)
 
 // Dependencies returns no dependencies.
 func (a *clusterName) Dependencies() []asset.Asset {
-	return []asset.Asset{}
+	return []asset.Asset{
+		&baseDomain{},
+	}
 }
 
 // Generate queries for the cluster name from the user.
-func (a *clusterName) Generate(asset.Parents) error {
+func (a *clusterName) Generate(parents asset.Parents) error {
+	bd := &baseDomain{}
+	parents.Get(bd)
+
 	return survey.Ask([]*survey.Question{
 		{
 			Prompt: &survey.Input{
 				Message: "Cluster Name",
 				Help:    "The name of the cluster.  This will be used when generating sub-domains.\n\nFor libvirt, choose a name that is unique enough to be used as a prefix during cluster deletion.  For example, if you use 'demo' as your cluster name, `openshift-install destroy cluster` may destroy all domains, networks, pools, and volumes that begin with 'demo'.",
 			},
 			Validate: survey.ComposeValidators(survey.Required, func(ans interface{}) error {
-				return validate.DomainName(ans.(string))
+				return validate.DomainName(validation.ClusterDomain(bd.BaseDomain, ans.(string)), false)
 			}),
 		},
 	}, &a.ClusterName)

pkg/types/validation/installconfig.go

@@ -18,6 +18,12 @@ import (
 	"github.com/openshift/installer/pkg/validate"
 )
 
+// ClusterDomain returns the cluster domain for a cluster with the specified
+// base domain and cluster name.
+func ClusterDomain(baseDomain, clusterName string) string {
+	return fmt.Sprintf("%s.%s", clusterName, baseDomain)
+}
+
 // ValidateInstallConfig checks that the specified install config is valid.
 func ValidateInstallConfig(c *types.InstallConfig, openStackValidValuesFetcher openstackvalidation.ValidValuesFetcher) field.ErrorList {
 	allErrs := field.ErrorList{}
@@ -27,16 +33,24 @@ func ValidateInstallConfig(c *types.InstallConfig, openStackValidValuesFetcher o
 	if c.TypeMeta.APIVersion != types.InstallConfigVersion && c.TypeMeta.APIVersion != "v1beta1" { // FIXME: v1beta1 is a temporary hack to get CI across the transition
 		return field.ErrorList{field.Invalid(field.NewPath("apiVersion"), c.TypeMeta.APIVersion, fmt.Sprintf("install-config version must be %q", types.InstallConfigVersion))}
 	}
-	if c.ObjectMeta.Name == "" {
-		allErrs = append(allErrs, field.Required(field.NewPath("metadata", "name"), "cluster name required"))
-	}
 	if c.SSHKey != "" {
 		if err := validate.SSHPublicKey(c.SSHKey); err != nil {
 			allErrs = append(allErrs, field.Invalid(field.NewPath("sshKey"), c.SSHKey, err.Error()))
 		}
 	}
-	if err := validate.DomainName(c.BaseDomain); err != nil {
-		allErrs = append(allErrs, field.Invalid(field.NewPath("baseDomain"), c.BaseDomain, err.Error()))
+	nameErr := validate.DomainName(c.ObjectMeta.Name, false)
+	if nameErr != nil {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "name"), c.ObjectMeta.Name, nameErr.Error()))
+	}
+	baseDomainErr := validate.DomainName(c.BaseDomain, true)
+	if baseDomainErr != nil {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("baseDomain"), c.BaseDomain, baseDomainErr.Error()))
+	}
+	if nameErr == nil && baseDomainErr == nil {
+		clusterDomain := ClusterDomain(c.BaseDomain, c.ObjectMeta.Name)
+		if err := validate.DomainName(clusterDomain, true); err != nil {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("baseDomain"), clusterDomain, err.Error()))
+		}
 	}
 	if c.Networking != nil {
 		allErrs = append(allErrs, validateNetworking(c.Networking, field.NewPath("networking"))...)

pkg/types/validation/installconfig_test.go

@@ -1,6 +1,7 @@
 package validation
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/golang/mock/gomock"
@@ -77,13 +78,13 @@ func TestValidateInstallConfig(t *testing.T) {
 			installConfig: validInstallConfig(),
 		},
 		{
-			name: "missing name",
+			name: "invalid name",
 			installConfig: func() *types.InstallConfig {
 				c := validInstallConfig()
-				c.ObjectMeta.Name = ""
+				c.ObjectMeta.Name = "bad-name-"
 				return c
 			}(),
-			expectedError: `^metadata.name: Required value: cluster name required$`,
+			expectedError: `^metadata.name: Invalid value: "bad-name-": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '\.', and must start and end with an alphanumeric character \(e\.g\. 'example\.com', regex used for validation is '\[a-z0-9]\(\[-a-z0-9]\*\[a-z0-9]\)\?\(\\\.\[a-z0-9]\(\[-a-z0-9]\*\[a-z0-9]\)\?\)\*'\)$`,
 		},
 		{
 			name: "invalid ssh key",
@@ -103,6 +104,16 @@ func TestValidateInstallConfig(t *testing.T) {
 			}(),
 			expectedError: `^baseDomain: Invalid value: "\.bad-domain\.": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '\.', and must start and end with an alphanumeric character \(e\.g\. 'example\.com', regex used for validation is '\[a-z0-9]\(\[-a-z0-9]\*\[a-z0-9]\)\?\(\\\.\[a-z0-9]\(\[-a-z0-9]\*\[a-z0-9]\)\?\)\*'\)$`,
 		},
+		{
+			name: "overly long cluster domain",
+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.ObjectMeta.Name = fmt.Sprintf("test-cluster%050d", 0)
+				c.BaseDomain = fmt.Sprintf("test-domain%050d.a%060d.b%060d.c%060d", 0, 0, 0, 0)
+				return c
+			}(),
+			expectedError: `^baseDomain: Invalid value: "` + fmt.Sprintf("test-cluster%050d.test-domain%050d.a%060d.b%060d.c%060d", 0, 0, 0, 0, 0) + `": must be no more than 253 characters$`,
+		},
 		{
 			name: "missing networking",
 			installConfig: func() *types.InstallConfig {

pkg/validate/validate.go

@@ -35,9 +35,11 @@ func validateSubdomain(v string) error {
 }
 
 // DomainName checks if the given string is a valid domain name and returns an error if not.
-func DomainName(v string) error {
-	// Trailing dot is OK
-	return validateSubdomain(strings.TrimSuffix(v, "."))
+func DomainName(v string, acceptTrailingDot bool) error {
+	if acceptTrailingDot {
+		v = strings.TrimSuffix(v, ".")
+	}
+	return validateSubdomain(v)
 }
 
 type imagePullSecret struct {

pkg/validate/validate_test.go

@@ -82,7 +82,7 @@ func TestSubnetCIDR(t *testing.T) {
 	}
 }
 
-func TestDomainName(t *testing.T) {
+func TestDomainName_AcceptingTrailingDot(t *testing.T) {
 	cases := []struct {
 		domain string
 		valid  bool
@@ -113,7 +113,48 @@ func TestDomainName(t *testing.T) {
 	}
 	for _, tc := range cases {
 		t.Run(tc.domain, func(t *testing.T) {
-			err := DomainName(tc.domain)
+			err := DomainName(tc.domain, true)
+			if tc.valid {
+				assert.NoError(t, err)
+			} else {
+				assert.Error(t, err)
+			}
+		})
+	}
+}
+
+func TestDomainName_RejectingTrailingDot(t *testing.T) {
+	cases := []struct {
+		domain string
+		valid  bool
+	}{
+		{"", false},
+		{" ", false},
+		{"a", true},
+		{".", false},
+		{"日本語", false},
+		{"日本語.com", false},
+		{"abc.日本語.com", false},
+		{"a日本語a.com", false},
+		{"abc", true},
+		{"ABC", false},
+		{"ABC123", false},
+		{"ABC123.COM123", false},
+		{"1", true},
+		{"0.0", true},
+		{"1.2.3.4", true},
+		{"1.2.3.4.", false},
+		{"abc.", false},
+		{"abc.com", true},
+		{"abc.com.", false},
+		{"a.b.c.d.e.f", true},
+		{".abc", false},
+		{".abc.com", false},
+		{".abc.com", false},
+	}
+	for _, tc := range cases {
+		t.Run(tc.domain, func(t *testing.T) {
+			err := DomainName(tc.domain, false)
 			if tc.valid {
 				assert.NoError(t, err)
 			} else {