Sanitize value for 'app.openshift.io/runtime' label (#6284)

* Add test cases highlighting the issue

* Sanitize value for 'app.openshift.io/runtime' label

* Lowercase value of 'app.openshift.io/runtime' label after sanitization

As suggested in review, this is to follow what's done by OpenShift console;
it is always lowercase when added by OpenShift Console.

Co-authored-by: Tomas Kral <tkral@redhat.com>

Co-authored-by: Tomas Kral <tkral@redhat.com>

pkg/labels/labels.go

@@ -2,12 +2,29 @@ package labels
 
 import (
 	"errors"
+	"regexp"
+	"strings"
+	"unicode"
 
-	"k8s.io/apimachinery/pkg/labels"
+	dfutil "github.com/devfile/library/pkg/util"
+	k8slabels "k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/util/validation"
+	"k8s.io/klog"
 
 	"github.com/redhat-developer/odo/pkg/version"
 )
 
+var _replacementMap = map[string]string{
+	".": "dot",
+	"#": "sharp",
+}
+
+var (
+	_regexpInvalidCharacters        = regexp.MustCompile(`[^a-zA-Z0-9._-]`)
+	_regexpStartingWithAlphanumeric = regexp.MustCompile(`^[a-z0-9A-Z]`)
+	_regexpEndingWithAlphanumeric   = regexp.MustCompile(`[a-z0-9A-Z]$`)
+)
+
 // GetLabels return labels that should be applied to every object for given component in active application
 // if you need labels to filter component then use GetSelector instead
 // Note: isPartOfComponent denotes if the label is required for a core resource(deployment, svc, pvc, pv) of a given component deployed with `odo dev`;
@@ -15,11 +32,110 @@ import (
 func GetLabels(componentName string, applicationName string, runtime string, mode string, isPartOfComponent bool) map[string]string {
 	labels := getLabels(componentName, applicationName, mode, true, isPartOfComponent)
 	if runtime != "" {
-		labels[openshiftRunTimeLabel] = runtime
+		// 'app.openshift.io/runtime' label added by OpenShift console is always lowercase
+		labels[openshiftRunTimeLabel] = strings.ToLower(sanitizeLabelValue(openshiftRunTimeLabel, runtime))
 	}
 	return labels
 }
 
+// sanitizeLabelValue makes sure that the value specified is a valid value for a Kubernetes label, which means:
+// i) must be 63 characters or fewer (can be empty), ii) unless empty, must begin and end with an alphanumeric character ([a-z0-9A-Z]),
+// iii) could contain dashes (-), underscores (_), dots (.), and alphanumerics between.
+//
+// As such, sanitizeLabelValue might perform the following operations (taking care of repeating the process if the result is not a valid label value):
+//
+// - replace leading or trailing characters (. with "dot" or "DOT", and # with "sharp" or "SHARP", depending on the value case)
+//
+// - replace all characters that are not dashes, underscores, dots or alphanumerics between with a dash (-)
+//
+// - truncate the overall result so that it is less than 63 characters.
+func sanitizeLabelValue(key, value string) string {
+	errs := validation.IsValidLabelValue(value)
+	if len(errs) == 0 {
+		return value
+	}
+
+	klog.V(4).Infof("invalid value for label %q: %q => sanitizing it: %v", key, value, strings.Join(errs, "; "))
+
+	// Return the corresponding value if is replaceable immediately
+	if v, ok := _replacementMap[value]; ok {
+		return v
+	}
+
+	// Replacements if it starts or ends with a non-alphanumeric character
+	value = replaceAllLeadingOrTrailingInvalidValues(value)
+
+	// Now replace any characters that are not dashes, dots, underscores or alphanumerics between
+	value = _regexpInvalidCharacters.ReplaceAllString(value, "-")
+
+	// Truncate if length > 63
+	if len(value) > validation.LabelValueMaxLength {
+		value = dfutil.TruncateString(value, validation.LabelValueMaxLength)
+	}
+
+	if errs = validation.IsValidLabelValue(value); len(errs) == 0 {
+		return value
+	}
+	return sanitizeLabelValue(key, value)
+}
+
+func replaceAllLeadingOrTrailingInvalidValues(value string) string {
+	if value == "" {
+		return ""
+	}
+
+	isAllCaseMatchingPredicate := func(p func(rune) bool, s string) bool {
+		for _, r := range s {
+			if !p(r) && unicode.IsLetter(r) {
+				return false
+			}
+		}
+		return true
+	}
+	getLabelValueReplacement := func(v, replacement string) string {
+		if isAllCaseMatchingPredicate(unicode.IsLower, v) {
+			return strings.ToLower(replacement)
+		}
+		if isAllCaseMatchingPredicate(unicode.IsUpper, v) {
+			return strings.ToUpper(replacement)
+		}
+		return replacement
+	}
+
+	if !_regexpStartingWithAlphanumeric.MatchString(value) {
+		vAfterFirstChar := value[1:]
+		var isPrefixReplaced bool
+		for k, val := range _replacementMap {
+			if strings.HasPrefix(value, k) {
+				value = getLabelValueReplacement(vAfterFirstChar, val) + vAfterFirstChar
+				isPrefixReplaced = true
+				break
+			}
+		}
+		if !isPrefixReplaced {
+			value = vAfterFirstChar
+		}
+		if value == "" {
+			return value
+		}
+	}
+	if !_regexpEndingWithAlphanumeric.MatchString(value) {
+		vBeforeLastChar := value[:len(value)-1]
+		var isSuffixReplaced bool
+		for k, val := range _replacementMap {
+			if strings.HasSuffix(value, k) {
+				value = vBeforeLastChar + getLabelValueReplacement(vBeforeLastChar, val)
+				isSuffixReplaced = true
+				break
+			}
+		}
+		if !isSuffixReplaced {
+			value = vBeforeLastChar
+		}
+	}
+	return value
+}
+
 // AddStorageInfo adds labels for storage resources
 func AddStorageInfo(labels map[string]string, storageName string, isSourceVolume bool) {
 	labels[kubernetesStorageNameLabel] = storageName
@@ -92,7 +208,7 @@ func GetSelector(componentName string, applicationName string, mode string, isPa
 // if you need labels to filter component then use additional=false
 // isPartOfComponent denotes if the label is required for a core resource(deployment, svc, pvc, pv) of a given component deployed with `odo dev`
 // it is the only thing that sets it apart from the resources created via other ways (`odo deploy`, deploying resource with apply command during `odo dev`)
-func getLabels(componentName string, applicationName string, mode string, additional bool, isPartOfComponent bool) labels.Set {
+func getLabels(componentName string, applicationName string, mode string, additional bool, isPartOfComponent bool) k8slabels.Set {
 	labels := getApplicationLabels(applicationName, additional)
 	labels[kubernetesInstanceLabel] = componentName
 	if mode != ComponentAnyMode {
@@ -108,8 +224,8 @@ func getLabels(componentName string, applicationName string, mode string, additi
 // additional labels are used only when creating object
 // if you are creating something use additional=true
 // if you need labels to filter component then use additional=false
-func getApplicationLabels(application string, additional bool) labels.Set {
-	labels := labels.Set{
+func getApplicationLabels(application string, additional bool) k8slabels.Set {
+	labels := k8slabels.Set{
 		kubernetesPartOfLabel:    application,
 		kubernetesManagedByLabel: odoManager,
 	}

pkg/labels/labels_test.go

@@ -2,9 +2,11 @@ package labels
 
 import (
 	"reflect"
+	"strings"
 	"testing"
 
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/util/validation"
 
 	"github.com/redhat-developer/odo/pkg/version"
 )
@@ -74,3 +76,191 @@ func Test_getLabels(t *testing.T) {
 		})
 	}
 }
+
+func Test_sanitizeLabelValue(t *testing.T) {
+	type args struct {
+		value string
+	}
+	for _, tt := range []struct {
+		name string
+		args args
+		want string
+	}{
+		{
+			name: "valid label",
+			args: args{value: "a-valid-value"},
+			want: "a-valid-value",
+		},
+		{
+			name: "empty value",
+			args: args{value: ""},
+			want: "",
+		},
+		{
+			name: "blank value",
+			args: args{value: "  \t"},
+			want: "",
+		},
+		{
+			name: "dot",
+			args: args{value: "."},
+			want: "dot",
+		},
+		{
+			name: "leading dot - upper",
+			args: args{value: ".NET"},
+			want: "DOTNET",
+		},
+		{
+			name: "leading dot - lower",
+			args: args{value: ".net"},
+			want: "dotnet",
+		},
+		{
+			name: "leading dot - mixed",
+			args: args{value: ".NeT"},
+			want: "dotNeT",
+		},
+		{
+			name: "trailing dot - upper",
+			args: args{value: "NET."},
+			want: "NETDOT",
+		},
+		{
+			name: "trailing dot - lower",
+			args: args{value: "net."},
+			want: "netdot",
+		},
+		{
+			name: "trailing dot - mixed",
+			args: args{value: "NeT."},
+			want: "NeTdot",
+		},
+		{
+			name: "leading and trailing dots",
+			args: args{value: ".."},
+			want: "dotdot",
+		},
+		{
+			name: "leading and trailing dots",
+			args: args{value: ".NET."},
+			want: "DOTNETDOT",
+		},
+		{
+			name: "sharp",
+			args: args{value: "#"},
+			want: "sharp",
+		},
+		{
+			name: "sharpdot",
+			args: args{value: "#."},
+			want: "sharpdot",
+		},
+		{
+			name: "dotsharp",
+			args: args{value: ".#"},
+			want: "dotsharp",
+		},
+		{
+			name: "leading sharp - upper",
+			args: args{value: "#NET"},
+			want: "SHARPNET",
+		},
+		{
+			name: "leading sharp - lower",
+			args: args{value: "#net"},
+			want: "sharpnet",
+		},
+		{
+			name: "leading sharp - mixed",
+			args: args{value: "#NeT"},
+			want: "sharpNeT",
+		},
+		{
+			name: "trailing sharp - upper",
+			args: args{value: "C#"},
+			want: "CSHARP",
+		},
+		{
+			name: "trailing sharp - lower",
+			args: args{value: "c#"},
+			want: "csharp",
+		},
+		{
+			name: "trailing sharp - mixed",
+			args: args{value: "NeT#"},
+			want: "NeTsharp",
+		},
+		{
+			name: "leading and trailing sharps",
+			args: args{value: "##"},
+			want: "sharpsharp",
+		},
+		{
+			name: "single invalid character",
+			args: args{value: "?"},
+			want: "",
+		},
+		{
+			name: "leading and trailing sharps with valid character in between",
+			args: args{value: "#C#"},
+			want: "SHARPCSHARP",
+		},
+
+		{
+			name: "leading non alpha-numeric",
+			args: args{value: "-something"},
+			want: "something",
+		},
+		{
+			name: "trailing non alpha-numeric",
+			args: args{value: "some thing-"},
+			want: "some-thing",
+		},
+		{
+			name: "more than 63 characters",
+			args: args{value: "Express.js (a.k.a Express), the de facto standard server framework for Node.js"},
+			want: "Express.js--a.k.a-Express---the-de-facto-standard-server-framew",
+		},
+		{
+			name: "more than 63 characters starting with space",
+			args: args{value: " Express.js (a.k.a Express), the de facto standard server framework for Node.js"},
+			want: "Express.js--a.k.a-Express---the-de-facto-standard-server-framew",
+		},
+		{
+			name: "more than 63 characters where truncating it leads to string ending with non-alphanumeric",
+			args: args{value: " Express.js (a.k.a Express), the de facto standard server frame-work"},
+			want: "Express.js--a.k.a-Express---the-de-facto-standard-server-frame",
+		},
+		{
+			name: "more than 63 characters starting and ending with replacable char should be truncated after replacement",
+			args: args{
+				value: ".NET (Lorem Ipsum Dolor Sit Amet, consectetur adipiscing elit, " +
+					"sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.) C#",
+			},
+			want: "dotNET--Lorem-Ipsum-Dolor-Sit-Amet--consectetur-adipiscing-elit",
+		},
+		{
+			name: "more than 63 characters ending with a lot of invalid characters",
+			args: args{value: ".NET" + strings.Repeat(" ", 60)},
+			want: "DOTNET",
+		},
+		{
+			name: "more than 63 invalid-only characters",
+			args: args{value: strings.Repeat("/[@\\", 90)},
+			want: "",
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			got := sanitizeLabelValue(openshiftRunTimeLabel, tt.args.value)
+			if got != tt.want {
+				t.Errorf("unexpected value for label. Expected %q, got %q", tt.want, got)
+			}
+			validationErrs := validation.IsValidLabelValue(got)
+			if len(validationErrs) != 0 {
+				t.Errorf("expected result %q to be a valid label value, but got the following errors: %s",
+					got, strings.Join(validationErrs, "; "))
+			}
+		})
+	}
+}

tests/examples/source/devfiles/nodejs/devfile-with-metadata-language-as-invalid-label.yaml

@@ -0,0 +1,95 @@
+commands:
+- exec:
+    commandLine: npm install
+    component: runtime
+    group:
+      isDefault: true
+      kind: build
+    workingDir: /project
+  id: install
+- exec:
+    commandLine: npm start
+    component: runtime
+    group:
+      isDefault: true
+      kind: run
+    workingDir: /project
+  id: run
+- exec:
+    commandLine: npm run debug
+    component: runtime
+    group:
+      isDefault: true
+      kind: debug
+    workingDir: /project
+  id: debug
+- exec:
+    commandLine: npm test
+    component: runtime
+    group:
+      isDefault: true
+      kind: test
+    workingDir: /project
+  id: test
+- id: deploy
+  composite:
+    commands:
+      - deploy-nginx
+    group:
+      kind: deploy
+      isDefault: true
+- id: deploy-nginx
+  apply:
+    component: nginx
+components:
+- container:
+    endpoints:
+    - name: http-3000
+      targetPort: 3000
+    image: registry.access.redhat.com/ubi8/nodejs-14:latest
+    memoryLimit: 1024Mi
+    mountSources: true
+    sourceMapping: /project
+  name: runtime
+- name: nginx
+  kubernetes:
+    inlined: |
+      apiVersion: apps/v1
+      kind: Deployment
+      metadata:
+        labels:
+          app: nginx
+        name: nginx
+      spec:
+        replicas: 1
+        selector:
+          matchLabels:
+            app: nginx
+        template:
+          metadata:
+            creationTimestamp: null
+            labels:
+              app: nginx
+          spec:
+            containers:
+            - image: nginx
+              name: nginx
+metadata:
+  description: Stack with Node.js 14
+  displayName: Node.js Runtime
+  icon: https://nodejs.org/static/images/logos/nodejs-new-pantone-black.svg
+  # Language is a free-form string in the Devfile spec, and might need to be sanitized before it can be used as label
+  # See https://github.com/redhat-developer/odo/issues/6234
+  language: a custom language
+  name: nodejs-prj
+  tags:
+  - NodeJS
+  - Express
+  - ubi8
+  version: 1.0.1
+schemaVersion: 2.2.0
+starterProjects:
+- git:
+    remotes:
+      origin: https://github.com/odo-devfiles/nodejs-ex.git
+  name: nodejs-starter

tests/examples/source/devfiles/nodejs/devfile-with-metadata-project-type-as-invalid-label.yaml

@@ -0,0 +1,96 @@
+commands:
+- exec:
+    commandLine: npm install
+    component: runtime
+    group:
+      isDefault: true
+      kind: build
+    workingDir: /project
+  id: install
+- exec:
+    commandLine: npm start
+    component: runtime
+    group:
+      isDefault: true
+      kind: run
+    workingDir: /project
+  id: run
+- exec:
+    commandLine: npm run debug
+    component: runtime
+    group:
+      isDefault: true
+      kind: debug
+    workingDir: /project
+  id: debug
+- exec:
+    commandLine: npm test
+    component: runtime
+    group:
+      isDefault: true
+      kind: test
+    workingDir: /project
+  id: test
+- id: deploy
+  composite:
+    commands:
+      - deploy-nginx
+    group:
+      kind: deploy
+      isDefault: true
+- id: deploy-nginx
+  apply:
+    component: nginx
+components:
+- container:
+    endpoints:
+    - name: http-3000
+      targetPort: 3000
+    image: registry.access.redhat.com/ubi8/nodejs-14:latest
+    memoryLimit: 1024Mi
+    mountSources: true
+    sourceMapping: /project
+  name: runtime
+- name: nginx
+  kubernetes:
+    inlined: |
+      apiVersion: apps/v1
+      kind: Deployment
+      metadata:
+        labels:
+          app: nginx
+        name: nginx
+      spec:
+        replicas: 1
+        selector:
+          matchLabels:
+            app: nginx
+        template:
+          metadata:
+            creationTimestamp: null
+            labels:
+              app: nginx
+          spec:
+            containers:
+            - image: nginx
+              name: nginx
+metadata:
+  description: Stack with Node.js 14
+  displayName: Node.js Runtime
+  icon: https://nodejs.org/static/images/logos/nodejs-new-pantone-black.svg
+  name: nodejs-prj
+  # Project type is a free-form string in the Devfile spec, and might need to be sanitized before it can be used as label
+  # See https://github.com/redhat-developer/odo/issues/6234
+  projectType: .NODE
+  language: javascript
+  tags:
+  - NodeJS
+  - Express
+  - ubi8
+  version: 1.0.1
+schemaVersion: 2.2.0
+starterProjects:
+- git:
+    remotes:
+      origin: https://github.com/odo-devfiles/nodejs-ex.git
+  name: nodejs-starter

tests/integration/cmd_dev_test.go

@@ -2561,6 +2561,42 @@ CMD ["npm", "start"]
 			},
 		},
 
+		{
+			whenTitle: "Devfile contains metadata.language invalid as a label value",
+			devfile:   "devfile-with-metadata-language-as-invalid-label.yaml",
+			checkDev: func(cmpName string) {
+				commonVar.CliRunner.AssertContainsLabel(
+					"deployment",
+					commonVar.Project,
+					cmpName,
+					"app",
+					labels.ComponentDevMode,
+					"app.openshift.io/runtime",
+					"a-custom-language",
+				)
+				commonVar.CliRunner.AssertContainsLabel(
+					"service",
+					commonVar.Project,
+					cmpName,
+					"app",
+					labels.ComponentDevMode,
+					"app.openshift.io/runtime",
+					"a-custom-language",
+				)
+			},
+			checkDeploy: func(cmpName string) {
+				commonVar.CliRunner.AssertContainsLabel(
+					"deployment",
+					commonVar.Project,
+					cmpName,
+					"app",
+					labels.ComponentDeployMode,
+					"app.openshift.io/runtime",
+					"a-custom-language",
+				)
+			},
+		},
+
 		{
 			whenTitle: "Devfile contains metadata.projectType",
 			devfile:   "devfile-with-metadata-project-type.yaml",
@@ -2597,6 +2633,42 @@ CMD ["npm", "start"]
 			},
 		},
 
+		{
+			whenTitle: "Devfile contains metadata.projectType invalid as a label value",
+			devfile:   "devfile-with-metadata-project-type-as-invalid-label.yaml",
+			checkDev: func(cmpName string) {
+				commonVar.CliRunner.AssertContainsLabel(
+					"deployment",
+					commonVar.Project,
+					cmpName,
+					"app",
+					labels.ComponentDevMode,
+					"app.openshift.io/runtime",
+					"dotnode",
+				)
+				commonVar.CliRunner.AssertContainsLabel(
+					"service",
+					commonVar.Project,
+					cmpName,
+					"app",
+					labels.ComponentDevMode,
+					"app.openshift.io/runtime",
+					"dotnode",
+				)
+			},
+			checkDeploy: func(cmpName string) {
+				commonVar.CliRunner.AssertContainsLabel(
+					"deployment",
+					commonVar.Project,
+					cmpName,
+					"app",
+					labels.ComponentDeployMode,
+					"app.openshift.io/runtime",
+					"dotnode",
+				)
+			},
+		},
+
 		{
 			whenTitle: "Devfile contains neither metadata.language nor metadata.projectType",
 			devfile:   "devfile-with-metadata-no-language-project-type.yaml",