Validate Elasticsearch resource names

pkg/controller/apmserver/name/name.go

@@ -9,22 +9,14 @@ import (
 )
 
 const (
-	// APM name, used as prefix, is limited to 36 characters,
-	MaxAPMNameLength = 36
-	// this leaves common_name.MaxNameLength - 36 characters for a suffix.
-	MaxSuffixLength = common_name.MaxNameLength - MaxAPMNameLength
-
 	secretTokenSuffix = "token"
 	httpServiceSuffix = "http"
 	configSuffix      = "config"
 	deploymentSuffix  = "server"
 )
 
 // APMNamer is a Namer that is configured with the defaults for resources related to an APM resource.
-var APMNamer = common_name.Namer{
-	MaxSuffixLength: MaxSuffixLength,
-	DefaultSuffixes: []string{"apm"},
-}
+var APMNamer = common_name.NewNamer("apm")
 
 func SecretToken(apmName string) string {
 	return APMNamer.Suffix(apmName, secretTokenSuffix)

pkg/controller/common/keystore/resources_test.go

@@ -76,7 +76,7 @@ func TestResources(t *testing.T) {
 		{
 			name:           "no secure settings specified: no resources",
 			client:         k8s.WrapClient(fake.NewFakeClient()),
-			kb:             v1alpha1.Kibana{},
+			kb:             testKibana,
 			wantContainers: nil,
 			wantVersion:    "",
 			wantNil:        true,

pkg/controller/common/keystore/user_secret_test.go

@@ -62,7 +62,7 @@ func Test_secureSettingsVolume(t *testing.T) {
 			name:        "no secure settings specified in Kibana spec: should return nothing",
 			c:           k8s.WrapClient(fake.NewFakeClient()),
 			w:           createWatches(""),
-			kb:          v1alpha1.Kibana{},
+			kb:          testKibana,
 			wantVolume:  nil,
 			wantVersion: "",
 			wantWatches: []string{},

pkg/controller/common/name/name.go

@@ -7,20 +7,42 @@ package name
 import (
 	"fmt"
 	"strings"
+	"unicode/utf8"
 
 	"github.com/elastic/cloud-on-k8s/pkg/utils/stringsutil"
+	"github.com/hashicorp/go-multierror"
+	"k8s.io/apimachinery/pkg/util/validation"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 )
 
-var (
-	log = logf.Log.WithName("name")
-)
-
 const (
-	// MaxNameLength is the maximum length of a resource name.
-	MaxNameLength = 63
+	MaxResourceNameLength = 36
+	MaxSuffixLength       = validation.LabelValueMaxLength - MaxResourceNameLength
 )
 
+var log = logf.Log.WithName("name")
+
+// nameLengthError is an error type for names exceeding the allowed length.
+type nameLengthError struct {
+	reason    string
+	maxLen    int
+	actualLen int
+	value     string
+}
+
+func newNameLengthError(reason string, maxLen int, value string) nameLengthError {
+	return nameLengthError{
+		reason:    reason,
+		maxLen:    maxLen,
+		actualLen: len(value),
+		value:     value,
+	}
+}
+
+func (nle nameLengthError) Error() string {
+	return fmt.Sprintf("%s: '%s' has length %d which is more than %d", nle.reason, nle.value, nle.actualLen, nle.maxLen)
+}
+
 // Namer assists with constructing names for K8s resources and avoiding collisions by ensuring that certain suffixes
 // are always used, and prevents the use of too long suffixes.
 type Namer struct {
@@ -31,6 +53,14 @@ type Namer struct {
 	DefaultSuffixes []string
 }
 
+// NewNamer creates a new Namer object with the default suffix length restriction.
+func NewNamer(defaultSuffixes ...string) Namer {
+	return Namer{
+		MaxSuffixLength: MaxSuffixLength,
+		DefaultSuffixes: defaultSuffixes,
+	}
+}
+
 // WithDefaultSuffixes returns a new Namer with updated default suffixes.
 func (n Namer) WithDefaultSuffixes(defaultSuffixes ...string) Namer {
 	n.DefaultSuffixes = defaultSuffixes
@@ -40,33 +70,55 @@ func (n Namer) WithDefaultSuffixes(defaultSuffixes ...string) Namer {
 // Suffix a resource name.
 //
 // Panics if the suffix exceeds the suffix limits.
-// Trims the name if the concatenated result would exceed the limits.
 func (n Namer) Suffix(ownerName string, suffixes ...string) string {
+	suffixedName, err := n.SafeSuffix(ownerName, suffixes...)
+	if err != nil {
+		// we should never encounter an error at this point because the names should have been validated
+		log.Error(err, "Invalid name. This could prevent the operator from functioning correctly", "name", suffixedName)
+	}
+
+	return suffixedName
+}
+
+// SafeSuffix attempts to generate a suffixed name, returning an error if the generated name is unacceptable.
+func (n Namer) SafeSuffix(ownerName string, suffixes ...string) (string, error) {
 	var suffixBuilder strings.Builder
+	var err error
+
 	for _, s := range n.DefaultSuffixes {
-		suffixBuilder.WriteString("-") // #nosec G104
-		suffixBuilder.WriteString(s)   // #nosec G104
+		suffixBuilder.WriteString("-")
+		suffixBuilder.WriteString(s)
 	}
 	for _, s := range suffixes {
-		suffixBuilder.WriteString("-") // #nosec G104
-		suffixBuilder.WriteString(s)   // #nosec G104
+		suffixBuilder.WriteString("-")
+		suffixBuilder.WriteString(s)
 	}
 
 	suffix := suffixBuilder.String()
 
 	// This should never happen because we control all the suffixes!
 	if len(suffix) > n.MaxSuffixLength {
-		panic(fmt.Errorf("suffix should not exceed %d characters: got %s", n.MaxSuffixLength, suffix))
+		err = multierror.Append(err, newNameLengthError("suffix exceeds max length", n.MaxSuffixLength, suffix))
+		suffix = truncate(suffix, n.MaxSuffixLength)
 	}
 
-	// This should never happen because the ownerName length should have been validated.
-	// Trim the ownerName and log an error as fallback.
-	maxPrefixLength := MaxNameLength - len(suffix)
+	maxPrefixLength := validation.LabelValueMaxLength - len(suffix)
 	if len(ownerName) > maxPrefixLength {
-		log.Error(fmt.Errorf("ownerName should not exceed %d characters: got %s", maxPrefixLength, ownerName),
-			"Failed to suffix resource")
-		ownerName = ownerName[:maxPrefixLength]
+		err = multierror.Append(err, newNameLengthError("owner name exceeds max length", maxPrefixLength, ownerName))
+		ownerName = truncate(ownerName, maxPrefixLength)
+	}
+
+	return stringsutil.Concat(ownerName, suffix), err
+}
+
+func truncate(s string, length int) string {
+	var b strings.Builder
+	for _, r := range s {
+		if b.Len()+utf8.RuneLen(r) > length {
+			return b.String()
+		}
+		b.WriteRune(r)
 	}
 
-	return stringsutil.Concat(ownerName, suffix)
+	return b.String()
 }

pkg/controller/common/name/name_test.go

@@ -55,13 +55,6 @@ func TestNamer_WithDefaultSuffixes(t *testing.T) {
 }
 
 func TestNamer_Suffix(t *testing.T) {
-	t.Run("too long suffix should panic", func(t *testing.T) {
-		require.Panics(t, func() {
-			namer := Namer{MaxSuffixLength: 1}
-			namer.Suffix("foo", "bar")
-		})
-	})
-
 	type args struct {
 		ownerName string
 		suffixes  []string
@@ -97,18 +90,6 @@ func TestNamer_Suffix(t *testing.T) {
 			args: args{ownerName: "foo", suffixes: []string{"bar", "baz"}},
 			want: "foo-default-bar-baz",
 		},
-		{
-			name: "too long owner name",
-			namer: Namer{
-				MaxSuffixLength: 20,
-				DefaultSuffixes: []string{"default"},
-			},
-			args: args{
-				ownerName: "this-owner-name-is-too-long-and-needs-to-be-trimmed-in-order-to-fit-the-suffix",
-				suffixes:  []string{"bar", "baz"},
-			},
-			want: "this-owner-name-is-too-long-and-needs-to-be-tri-default-bar-baz",
-		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -118,3 +99,36 @@ func TestNamer_Suffix(t *testing.T) {
 		})
 	}
 }
+
+func TestNamerSafeSuffixErrors(t *testing.T) {
+	testCases := []struct {
+		name      string
+		namer     Namer
+		ownerName string
+		suffixes  []string
+		wantName  string
+	}{
+		{
+			name:      "long owner name",
+			namer:     Namer{MaxSuffixLength: 20, DefaultSuffixes: []string{"es"}},
+			ownerName: "extremely-long-and-unwieldy-name-for-owner-that-exceeds-the-limit",
+			suffixes:  []string{"bar", "baz"},
+			wantName:  "extremely-long-and-unwieldy-name-for-owner-that-exce-es-bar-baz",
+		},
+		{
+			name:      "long suffixes",
+			namer:     Namer{MaxSuffixLength: 20, DefaultSuffixes: []string{"es"}},
+			ownerName: "test",
+			suffixes:  []string{"bar", "baz", "very-long-suffix-exceeding-the-limit"},
+			wantName:  "test-es-bar-baz-very-lon",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			haveName, err := tc.namer.SafeSuffix(tc.ownerName, tc.suffixes...)
+			require.Error(t, err)
+			require.Equal(t, tc.wantName, haveName)
+		})
+	}
+}

pkg/controller/elasticsearch/initcontainer/initcontainer_test.go

@@ -70,7 +70,7 @@ func TestNewInitContainers(t *testing.T) {
 				tt.args.elasticsearchImage,
 				tt.args.SetVMMaxMapCount,
 				volume.SecretVolume{},
-				"clusterName",
+				"clustername",
 				tt.args.keystoreResources,
 			)
 			assert.NoError(t, err)

pkg/controller/elasticsearch/name/name.go

@@ -5,17 +5,19 @@
 package name
 
 import (
-	"github.com/elastic/cloud-on-k8s/pkg/controller/common/name"
+	"fmt"
+	"strconv"
+	"strings"
+
+	"github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1alpha1"
+	common_name "github.com/elastic/cloud-on-k8s/pkg/controller/common/name"
+	"github.com/elastic/cloud-on-k8s/pkg/utils/k8s"
+	"github.com/pkg/errors"
+	apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
+	"k8s.io/apimachinery/pkg/util/validation"
 )
 
 const (
-	// Whatever the named resource, it must never exceed 63 characters to be used as a label.
-	MaxLabelLength = 63
-	// Elasticsearch name, used as prefix, is limited to 36 characters,
-	MaxElasticsearchNameLength = 36
-	// this leaves 63 - 36 = 27 characters for a suffix.
-	MaxSuffixLength = MaxLabelLength - MaxElasticsearchNameLength
-
 	configSecretSuffix                = "config"
 	secureSettingsSecretSuffix        = "secure-settings"
 	httpServiceSuffix                 = "http"
@@ -29,10 +31,62 @@ const (
 	transportCertificatesSecretSuffix = "transport-certificates"
 )
 
-// ESNamer is a Namer that is configured with the defaults for resources related to an ES cluster.
-var ESNamer = name.Namer{
-	MaxSuffixLength: MaxSuffixLength,
-	DefaultSuffixes: []string{"es"},
+var (
+	// ESNamer is a Namer that is configured with the defaults for resources related to an ES cluster.
+	ESNamer = common_name.NewNamer("es")
+
+	suffixes = []string{
+		configSecretSuffix,
+		secureSettingsSecretSuffix,
+		httpServiceSuffix,
+		elasticUserSecretSuffix,
+		xpackFileRealmSecretSuffix,
+		internalUsersSecretSuffix,
+		unicastHostsConfigMapSuffix,
+		licenseSecretSuffix,
+		defaultPodDisruptionBudget,
+		scriptsConfigMapSuffix,
+		transportCertificatesSecretSuffix,
+	}
+)
+
+// Validate checks the validity of resource names that will be generated by the given Elasticsearch object.
+func Validate(es v1alpha1.Elasticsearch) error {
+	esName := k8s.ExtractNamespacedName(&es).Name
+	if len(esName) > common_name.MaxResourceNameLength {
+		return fmt.Errorf("name exceeds maximum allowed length of %d", common_name.MaxResourceNameLength)
+	}
+
+	// validate ssets
+	for _, nodeSpec := range es.Spec.Nodes {
+		if errs := apimachineryvalidation.NameIsDNSSubdomain(nodeSpec.Name, false); len(errs) > 0 {
+			return fmt.Errorf("invalid nodeSpec name '%s': [%s]", nodeSpec.Name, strings.Join(errs, ","))
+		}
+
+		ssetName, err := ESNamer.SafeSuffix(esName, nodeSpec.Name)
+		if err != nil {
+			return errors.Wrapf(err, "error generating StatefulSet name for nodeSpec: '%s'", nodeSpec.Name)
+		}
+
+		// length of the ordinal suffix that will be added to the pods of this sset
+		podOrdinalSuffixLen := len(strconv.FormatInt(int64(nodeSpec.NodeCount), 10))
+		// there should be enough space for the ordinal suffix
+		if validation.DNS1123SubdomainMaxLength-len(ssetName) < podOrdinalSuffixLen {
+			return fmt.Errorf("generated StatefulSet name '%s' exceeds allowed length of %d",
+				ssetName,
+				validation.DNS1123SubdomainMaxLength-podOrdinalSuffixLen)
+		}
+	}
+
+	// validate other suffixes
+	// it would be better to use the actual naming functions here but they panic on error
+	for _, suffix := range suffixes {
+		if _, err := ESNamer.SafeSuffix(esName, suffix); err != nil {
+			return err
+		}
+	}
+
+	return nil
 }
 
 // StatefulSet returns the name of the StatefulSet corresponding to the given NodeSpec.