fix(volume): Generate volume name compliant with DNS Label names (kna…

…tive#975)

* fix(volume): Volume names to not contain dots

 Replace non alphanumberic characters with hyphen as the
 volumen name must be a DNS_LABEL (https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names)
 ref: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#volume-v1-core

* Generate volume name compliant with DNS Label names

 Volume names to follow the DNS label standard as defined in RFC 1123. This means the name must:
 - contain at most 63 characters
 - contain only lowercase alphanumeric characters or '-'
 - start with an alphanumeric character
 - end with an alphanumeric character

CHANGELOG.adoc

@@ -30,6 +30,13 @@
 | Fix exit code for `kn service delete` and `kn revision delete` failures
 | https://github.com/knative/client/pull/971[#971]
 
+| 🎁
+| Add mock test client for dynamic client
+| https://github.com/knative/client/pull/972[#972]
+
+| 🐛
+| Fix client side volume name generation
+| https://github.com/knative/client/pull/975[#975]
 |===
 
 ## v0.16.0 (2020-07-14)

pkg/serving/config_changes.go

@@ -501,26 +501,44 @@ func UpdateImagePullSecrets(template *servingv1.RevisionTemplateSpec, pullsecret
 }
 
 // GenerateVolumeName generates a volume name with respect to a given path string.
-// Current implementation basically sanitizes the path string by changing "/" into "."
+// Current implementation basically sanitizes the path string by replacing "/" with "-"
 // To reduce any chance of duplication, a checksum part generated from the path string is appended to the sanitized string.
+// The volume name must follow the DNS label standard as defined in RFC 1123. This means the name must:
+// - contain at most 63 characters
+// - contain only lowercase alphanumeric characters or '-'
+// - start with an alphanumeric character
+// - end with an alphanumeric character
 func GenerateVolumeName(path string) string {
 	builder := &strings.Builder{}
 	for idx, r := range path {
 		switch {
-		case unicode.IsLower(r) || unicode.IsDigit(r) || r == '-' || r == '.':
+		case unicode.IsLower(r) || unicode.IsDigit(r) || r == '-':
 			builder.WriteRune(r)
 		case unicode.IsUpper(r):
 			builder.WriteRune(unicode.ToLower(r))
 		case r == '/':
 			if idx != 0 {
-				builder.WriteRune('.')
+				builder.WriteRune('-')
 			}
 		default:
 			builder.WriteRune('-')
 		}
 	}
 
-	return appendCheckSum(builder.String(), path)
+	vname := appendCheckSum(builder.String(), path)
+
+	// the name must start with an alphanumeric character
+	if !unicode.IsLetter(rune(vname[0])) && !unicode.IsNumber(rune(vname[0])) {
+		vname = fmt.Sprintf("k-%s", vname)
+	}
+
+	// contain at most 63 characters
+	if len(vname) > 63 {
+		// must end with an alphanumeric character
+		vname = fmt.Sprintf("%s-n", vname[0:61])
+	}
+
+	return vname
 }
 
 // =======================================================================================
@@ -802,8 +820,8 @@ func existsVolumeNameInVolumeMounts(volumeName string, volumeMounts []corev1.Vol
 	return false
 }
 
-func appendCheckSum(sanitiedString string, path string) string {
+func appendCheckSum(sanitizedString, path string) string {
 	checkSum := sha1.Sum([]byte(path))
 	shortCheckSum := checkSum[0:4]
-	return fmt.Sprintf("%s-%x", sanitiedString, shortCheckSum)
+	return fmt.Sprintf("%s-%x", sanitizedString, shortCheckSum)
 }

pkg/serving/config_changes_test.go

@@ -670,20 +670,29 @@ func TestGenerateVolumeName(t *testing.T) {
 		"/Ab12~`!@#$%^&*()-=_+[]{}|/\\<>,./?:;\"'xZ/",
 		"",
 		"/",
+		"/path.mypath/",
+		"/.path.mypath",
 	}
 
 	expected := []string{
-		"ab12---------------------.----..-----xz",
-		"ab12---------------------.----..-----xz.",
-		"",
-		"",
+		"ab12---------------------------------xz",
+		"ab12---------------------------------xz-",
+		"k-",
+		"k-",
+		"path-mypath-",
+		"k--path-mypath",
 	}
 
 	for i := range actual {
 		actualName := GenerateVolumeName(actual[i])
 		expectedName := appendCheckSum(expected[i], actual[i])
 		assert.Equal(t, actualName, expectedName)
 	}
+
+	// 63 char limit case, no need to append the checksum in expected string
+	expName_63 := "k---ab12---------------------------------xz-ab12--------------n"
+	assert.Equal(t, len(expName_63), 63)
+	assert.Equal(t, GenerateVolumeName("/./Ab12~`!@#$%^&*()-=_+[]{}|/\\<>,./?:;\"'xZ/Ab12~`!@#$%^&*()-=_+[]{}|/\\<>,./?:;\"'xZ/"), expName_63)
 }
 
 func TestUpdateUser(t *testing.T) {