Validate Elasticsearch resource names (#1647)

* Validate resource names

* Avoid panics and truncate long names

* Fix sset ordinal suffix length calculation

* Account for dash in sset ordinal suffix length

* Update limit and add E2E test

* Explain length restrictions

* Fix test failure

config/crds/elasticsearch_v1alpha1_elasticsearch.yaml

@@ -117,10 +117,9 @@ spec:
                     description: Config represents Elasticsearch configuration.
                     type: object
                   name:
-                    description: 'Name is a logical name for this set of nodes. Used
-                      as a part of the managed Elasticsearch node.name setting. TODO:
-                      refactor and explain name length conventions'
-                    maxLength: 19
+                    description: Name is a logical name for this set of nodes. Used
+                      as a part of the managed Elasticsearch node.name setting.
+                    maxLength: 23
                     pattern: '[a-zA-Z0-9-]+'
                     type: string
                   nodeCount:

pkg/apis/elasticsearch/v1alpha1/elasticsearch_types.go

@@ -66,8 +66,7 @@ func (es ElasticsearchSpec) NodeCount() int32 {
 type NodeSpec struct {
 	// Name is a logical name for this set of nodes. Used as a part of the managed Elasticsearch node.name setting.
 	// +kubebuilder:validation:Pattern=[a-zA-Z0-9-]+
-	// +kubebuilder:validation:MaxLength=19
-	// TODO: refactor and explain name length conventions
+	// +kubebuilder:validation:MaxLength=23
 	Name string `json:"name"`
 
 	// Config represents Elasticsearch configuration.

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/certificates/ca_reconcile_test.go

@@ -20,11 +20,13 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/client-go/kubernetes/scheme"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
 var testNamer = name.Namer{
+	MaxNameLength:   validation.LabelValueMaxLength,
 	MaxSuffixLength: 27, // from a prefix length of 36
 	DefaultSuffixes: []string{"test"},
 }

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,66 +7,118 @@ 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 is the max length of a name that will keep all its child resources under the K8S max label
+	// length after accounting for the longest suffix that could be added by the operator. See https://git.io/fjpFl.
+	MaxResourceNameLength = 36
+	// MaxSuffixLength is the max allowed suffix length that will keep a name within K8S label length restrictions.
+	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 {
-	// MaxSuffixLength is the maximum allowable length of a suffix.
 	MaxSuffixLength int
-
-	// DefaultSuffixes are suffixes that should be added by default before the provided suffixes when Suffix is called.
+	MaxNameLength   int
 	DefaultSuffixes []string
 }
 
+// NewNamer creates a new Namer object with the default suffix length restriction.
+func NewNamer(defaultSuffixes ...string) Namer {
+	return Namer{
+		MaxSuffixLength: MaxSuffixLength,
+		MaxNameLength:   validation.DNS1123SubdomainMaxLength,
+		DefaultSuffixes: defaultSuffixes,
+	}
+}
+
 // WithDefaultSuffixes returns a new Namer with updated default suffixes.
 func (n Namer) WithDefaultSuffixes(defaultSuffixes ...string) Namer {
 	n.DefaultSuffixes = defaultSuffixes
 	return n
 }
 
-// Suffix a resource name.
-//
-// Panics if the suffix exceeds the suffix limits.
-// Trims the name if the concatenated result would exceed the limits.
+// Suffix generates a resource name by appending the specified suffixes.
 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 := n.MaxNameLength - 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

@@ -24,6 +24,7 @@ func TestNamer_WithDefaultSuffixes(t *testing.T) {
 			name: "should replace suffixes",
 			namer: Namer{
 				MaxSuffixLength: 27,
+				MaxNameLength:   36,
 				DefaultSuffixes: []string{"foo"},
 			},
 			args: args{
@@ -36,6 +37,7 @@ func TestNamer_WithDefaultSuffixes(t *testing.T) {
 		{
 			name: "should add suffixes when there is no suffix to begin with",
 			namer: Namer{
+				MaxNameLength:   36,
 				MaxSuffixLength: 27,
 			},
 			args: args{
@@ -55,13 +57,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
@@ -75,6 +70,7 @@ func TestNamer_Suffix(t *testing.T) {
 		{
 			name: "simple suffix",
 			namer: Namer{
+				MaxNameLength:   36,
 				MaxSuffixLength: 20,
 			},
 			args: args{ownerName: "foo", suffixes: []string{"bar"}},
@@ -83,6 +79,7 @@ func TestNamer_Suffix(t *testing.T) {
 		{
 			name: "multiple suffixes",
 			namer: Namer{
+				MaxNameLength:   36,
 				MaxSuffixLength: 20,
 			},
 			args: args{ownerName: "foo", suffixes: []string{"bar", "baz"}},
@@ -91,24 +88,13 @@ func TestNamer_Suffix(t *testing.T) {
 		{
 			name: "default suffix",
 			namer: Namer{
+				MaxNameLength:   36,
 				MaxSuffixLength: 20,
 				DefaultSuffixes: []string{"default"},
 			},
 			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 +104,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, MaxNameLength: 36, DefaultSuffixes: []string{"es"}},
+			ownerName: "extremely-long-and-unwieldy-name-for-owner-that-exceeds-the-limit",
+			suffixes:  []string{"bar", "baz"},
+			wantName:  "extremely-long-and-unwiel-es-bar-baz",
+		},
+		{
+			name:      "long suffixes",
+			namer:     Namer{MaxSuffixLength: 20, MaxNameLength: 36, 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)