diff --git a/pkg/asset/installconfig/basedomain.go b/pkg/asset/installconfig/basedomain.go index d6d6eec0a74..5a84a5451e4 100644 --- a/pkg/asset/installconfig/basedomain.go +++ b/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) diff --git a/pkg/asset/installconfig/clustername.go b/pkg/asset/installconfig/clustername.go index 46680e1ccdf..bb448930e68 100644 --- a/pkg/asset/installconfig/clustername.go +++ b/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,11 +16,16 @@ 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{ @@ -27,7 +33,7 @@ func (a *clusterName) Generate(asset.Parents) error { 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) diff --git a/pkg/types/validation/installconfig.go b/pkg/types/validation/installconfig.go index 7e21bf13aec..9fbaadf5970 100644 --- a/pkg/types/validation/installconfig.go +++ b/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"))...) diff --git a/pkg/types/validation/installconfig_test.go b/pkg/types/validation/installconfig_test.go index 9cb04805144..94672b1beb7 100644 --- a/pkg/types/validation/installconfig_test.go +++ b/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 { diff --git a/pkg/validate/validate.go b/pkg/validate/validate.go index 2eb4af99968..e87462e62b9 100644 --- a/pkg/validate/validate.go +++ b/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 { diff --git a/pkg/validate/validate_test.go b/pkg/validate/validate_test.go index 61022d83e13..798b62ac007 100644 --- a/pkg/validate/validate_test.go +++ b/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 {