Remove the check for the clone cluster name. (#270)

* Sanity checks for the cluster name, improve tests.

- check that the normal and clone cluster name complies with the valid
  service name. For clone cluster, only do it if clone timestamp is not
  set; with a clone timestamp set, the clone name points to the S3 bucket

 - add tests and improve existing ones, making sure we don't call Error()
   method for an empty error, as well as that we don't miss cases where
   expected error is not empty, but actual call to be tested does not
   return an error.

Code review by @zerg-junior and @Jan-M

pkg/spec/postgresql.go

@@ -3,6 +3,7 @@ package spec
 import (
 	"encoding/json"
 	"fmt"
+	"regexp"
 	"strings"
 	"time"
 
@@ -76,6 +77,12 @@ const (
 	ClusterStatusInvalid      PostgresStatus = "Invalid"
 )
 
+const (
+	serviceNameMaxLength   = 63
+	clusterNameMaxLength   = serviceNameMaxLength - len("-repl")
+	serviceNameRegexString = `^[a-z]([-a-z0-9]*[a-z0-9])?$`
+)
+
 // Postgresql defines PostgreSQL Custom Resource Definition Object.
 type Postgresql struct {
 	metav1.TypeMeta   `json:",inline"`
@@ -126,7 +133,10 @@ type PostgresqlList struct {
 	Items []Postgresql `json:"items"`
 }
 
-var weekdays = map[string]int{"Sun": 0, "Mon": 1, "Tue": 2, "Wed": 3, "Thu": 4, "Fri": 5, "Sat": 6}
+var (
+	weekdays         = map[string]int{"Sun": 0, "Mon": 1, "Tue": 2, "Wed": 3, "Thu": 4, "Fri": 5, "Sat": 6}
+	serviceNameRegex = regexp.MustCompile(serviceNameRegexString)
+)
 
 func parseTime(s string) (time.Time, error) {
 	parts := strings.Split(s, ":")
@@ -225,10 +235,31 @@ func extractClusterName(clusterName string, teamName string) (string, error) {
 	if strings.ToLower(clusterName[:teamNameLen+1]) != strings.ToLower(teamName)+"-" {
 		return "", fmt.Errorf("name must match {TEAM}-{NAME} format")
 	}
+	if len(clusterName) > clusterNameMaxLength {
+		return "", fmt.Errorf("name cannot be longer than %d characters", clusterNameMaxLength)
+	}
+	if !serviceNameRegex.MatchString(clusterName) {
+		return "", fmt.Errorf("name must confirm to DNS-1035, regex used for validation is %q",
+			serviceNameRegexString)
+	}
 
 	return clusterName[teamNameLen+1:], nil
 }
 
+func validateCloneClusterDescription(clone *CloneDescription) error {
+	// when cloning from the basebackup (no end timestamp) check that the cluster name is a valid service name
+	if clone.ClusterName != "" && clone.EndTimestamp == "" {
+		if !serviceNameRegex.MatchString(clone.ClusterName) {
+			return fmt.Errorf("clone cluster name must confirm to DNS-1035, regex used for validation is %q",
+				serviceNameRegexString)
+		}
+		if len(clone.ClusterName) > serviceNameMaxLength {
+			return fmt.Errorf("clone cluster name must be no longer than %d characters", serviceNameMaxLength)
+		}
+	}
+	return nil
+}
+
 type postgresqlListCopy PostgresqlList
 type postgresqlCopy Postgresql
 
@@ -252,22 +283,16 @@ func (p *Postgresql) UnmarshalJSON(data []byte) error {
 	}
 	tmp2 := Postgresql(tmp)
 
-	clusterName, err := extractClusterName(tmp2.ObjectMeta.Name, tmp2.Spec.TeamID)
+	if clusterName, err := extractClusterName(tmp2.ObjectMeta.Name, tmp2.Spec.TeamID); err != nil {
-	if err == nil {
-		tmp2.Spec.ClusterName = clusterName
-	} else {
 		tmp2.Error = err
 		tmp2.Status = ClusterStatusInvalid
-	}
+	} else if err := validateCloneClusterDescription(&tmp2.Spec.Clone); err != nil {
-	// The assumption below is that a cluster to clone, if any, belongs to the same team
+		tmp2.Error = err
-	if tmp2.Spec.Clone.ClusterName != "" {
-		_, err := extractClusterName(tmp2.Spec.Clone.ClusterName, tmp2.Spec.TeamID)
-		if err != nil {
-			tmp2.Error = fmt.Errorf("%s for the cluster to clone", err)
-			tmp2.Spec.Clone = CloneDescription{}
 		tmp2.Status = ClusterStatusInvalid
+	} else {
+		tmp2.Spec.ClusterName = clusterName
 	}
-	}
+
 	*p = tmp2
 
 	return nil

pkg/spec/postgresql_test.go

@@ -43,7 +43,10 @@ var clusterNames = []struct {
 	{"acid-test", "acid", "test", nil},
 	{"test-my-name", "test", "my-name", nil},
 	{"my-team-another-test", "my-team", "another-test", nil},
-	{"------strange-team-cluster", "-----", "strange-team-cluster", nil},
+	{"------strange-team-cluster", "-----", "strange-team-cluster",
+		errors.New(`name must confirm to DNS-1035, regex used for validation is "^[a-z]([-a-z0-9]*[a-z0-9])?$"`)},
+	{"fooobar-fooobarfooobarfooobarfooobarfooobarfooobarfooobarfooobar", "fooobar", "",
+		errors.New("name cannot be longer than 58 characters")},
 	{"acid-test", "test", "", errors.New("name must match {TEAM}-{NAME} format")},
 	{"-test", "", "", errors.New("team name is empty")},
 	{"-test", "-", "", errors.New("name must match {TEAM}-{NAME} format")},
@@ -51,6 +54,18 @@ var clusterNames = []struct {
 	{"-", "-", "", errors.New("name is too short")},
 }
 
+var cloneClusterDescriptions = []struct {
+	in  *CloneDescription
+	err error
+}{
+	{&CloneDescription{"foo+bar", "", "NotEmpty"}, nil},
+	{&CloneDescription{"foo+bar", "", ""},
+		errors.New(`clone cluster name must confirm to DNS-1035, regex used for validation is "^[a-z]([-a-z0-9]*[a-z0-9])?$"`)},
+	{&CloneDescription{"foobar123456789012345678901234567890123456789012345678901234567890", "", ""},
+		errors.New("clone cluster name must be no longer than 63 characters")},
+	{&CloneDescription{"foobar", "", ""}, nil},
+}
+
 var maintenanceWindows = []struct {
 	in  []byte
 	out MaintenanceWindow
@@ -280,13 +295,14 @@ var unmarshalCluster = []struct {
 			},
 			Spec: PostgresSpec{
 				TeamID: "acid",
-				Clone:       CloneDescription{},
+				Clone: CloneDescription{
+					ClusterName: "team-batman",
+				},
 				ClusterName: "testcluster1",
 			},
-			Status: ClusterStatusInvalid,
+			Error: nil,
-			Error:  errors.New("name must match {TEAM}-{NAME} format for the cluster to clone"),
 		},
-		marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0},"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"acid","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":{}},"status":"Invalid"}`), err: nil},
+		marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0},"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"acid","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":{"cluster":"team-batman"}}}`), err: nil},
 	{[]byte(`{"kind": "Postgresql","apiVersion": "acid.zalan.do/v1"`),
 		Postgresql{},
 		[]byte{},
@@ -350,11 +366,12 @@ func TestParseTime(t *testing.T) {
 	for _, tt := range parseTimeTests {
 		aTime, err := parseTime(tt.in)
 		if err != nil {
-			if err.Error() != tt.err.Error() {
+			if tt.err == nil || err.Error() != tt.err.Error() {
 				t.Errorf("ParseTime expected error: %v, got: %v", tt.err, err)
 			}
-
 			continue
+		} else if tt.err != nil {
+			t.Errorf("Expected error: %v", tt.err)
 		}
 
 		if aTime != tt.out {
@@ -367,11 +384,12 @@ func TestWeekdayTime(t *testing.T) {
 	for _, tt := range parseWeekdayTests {
 		aTime, err := parseWeekday(tt.in)
 		if err != nil {
-			if err.Error() != tt.err.Error() {
+			if tt.err == nil || err.Error() != tt.err.Error() {
 				t.Errorf("ParseWeekday expected error: %v, got: %v", tt.err, err)
 			}
-
 			continue
+		} else if tt.err != nil {
+			t.Errorf("Expected error: %v", tt.err)
 		}
 
 		if aTime != tt.out {
@@ -383,9 +401,13 @@ func TestWeekdayTime(t *testing.T) {
 func TestClusterName(t *testing.T) {
 	for _, tt := range clusterNames {
 		name, err := extractClusterName(tt.in, tt.inTeam)
-		if err != nil && err.Error() != tt.err.Error() {
+		if err != nil {
+			if tt.err == nil || err.Error() != tt.err.Error() {
 				t.Errorf("extractClusterName expected error: %v, got: %v", tt.err, err)
+			}
 			continue
+		} else if tt.err != nil {
+			t.Errorf("Expected error: %v", tt.err)
 		}
 		if name != tt.clusterName {
 			t.Errorf("Expected cluserName: %q, got: %q", tt.clusterName, name)
@@ -393,17 +415,29 @@ func TestClusterName(t *testing.T) {
 	}
 }
 
+func TestCloneClusterDescription(t *testing.T) {
+	for _, tt := range cloneClusterDescriptions {
+		if err := validateCloneClusterDescription(tt.in); err != nil {
+			if tt.err == nil || err.Error() != tt.err.Error() {
+				t.Errorf("testCloneClusterDescription expected error: %v, got: %v", tt.err, err)
+			}
+		} else if tt.err != nil {
+			t.Errorf("Expected error: %v", tt.err)
+		}
+	}
+}
+
 func TestUnmarshalMaintenanceWindow(t *testing.T) {
 	for _, tt := range maintenanceWindows {
 		var m MaintenanceWindow
 		err := m.UnmarshalJSON(tt.in)
-		if err != nil && err.Error() != tt.err.Error() {
+		if err != nil {
+			if tt.err == nil || err.Error() != tt.err.Error() {
 				t.Errorf("MaintenanceWindow unmarshal expected error: %v, got %v", tt.err, err)
-			continue
 			}
-		if tt.err != nil && err == nil {
-			t.Errorf("Expected error")
 			continue
+		} else if tt.err != nil {
+			t.Errorf("Expected error: %v", tt.err)
 		}
 
 		if !reflect.DeepEqual(m, tt.out) {
@@ -421,7 +455,6 @@ func TestMarshalMaintenanceWindow(t *testing.T) {
 		s, err := tt.out.MarshalJSON()
 		if err != nil {
 			t.Errorf("Marshal Error: %v", err)
-			continue
 		}
 
 		if !bytes.Equal(s, tt.in) {
@@ -435,11 +468,12 @@ func TestPostgresUnmarshal(t *testing.T) {
 		var cluster Postgresql
 		err := cluster.UnmarshalJSON(tt.in)
 		if err != nil {
-			if err.Error() != tt.err.Error() {
+			if tt.err == nil || err.Error() != tt.err.Error() {
 				t.Errorf("Unmarshal expected error: %v, got: %v", tt.err, err)
 			}
-
 			continue
+		} else if tt.err != nil {
+			t.Errorf("Expected error: %v", tt.err)
 		}
 
 		if !reflect.DeepEqual(cluster, tt.out) {
@@ -457,7 +491,6 @@ func TestMarshal(t *testing.T) {
 		m, err := json.Marshal(tt.out)
 		if err != nil {
 			t.Errorf("Marshal error: %v", err)
-			continue
 		}
 		if !bytes.Equal(m, tt.marshal) {
 			t.Errorf("Marshal Postgresql expected: %q, got: %q", string(tt.marshal), string(m))
@@ -481,10 +514,15 @@ func TestUnmarshalPostgresList(t *testing.T) {
 	for _, tt := range postgresqlList {
 		var list PostgresqlList
 		err := list.UnmarshalJSON(tt.in)
-		if err != nil && err.Error() != tt.err.Error() {
+		if err != nil {
+			if tt.err == nil || err.Error() != tt.err.Error() {
 				t.Errorf("PostgresqlList unmarshal expected error: %v, got: %v", tt.err, err)
-			return
 			}
+			continue
+		} else if tt.err != nil {
+			t.Errorf("Expected error: %v", tt.err)
+		}
+
 		if !reflect.DeepEqual(list, tt.out) {
 			t.Errorf("Postgresql list unmarshall expected: %#v, got: %#v", tt.out, list)
 		}