Refactor label parsing to set OF labels last

handlers/delete.go

@@ -66,7 +66,7 @@ func MakeDeleteHandler(functionNamespace string, clientset *kubernetes.Clientset
 
 func isFunction(deployment *v1beta1.Deployment) bool {
 	if deployment != nil {
-		if _, found := deployment.Labels["faas_function"]; found {
+		if _, found := deployment.Labels[FunctionNameLabel]; found {
 			return true
 		}
 	}

handlers/deploy.go

@@ -12,7 +12,6 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
-	"strconv"
 	"strings"
 
 	"github.com/openfaas/faas/gateway/requests"
@@ -28,9 +27,6 @@ import (
 // watchdogPort for the OpenFaaS function watchdog
 const watchdogPort = 8080
 
-// initialReplicasCount how many replicas to start of creating for a function
-const initialReplicasCount = 1
-
 // Regex for RFC-1123 validation:
 // 	k8s.io/kubernetes/pkg/util/validation/validation.go
 var validDNS = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
@@ -165,22 +161,12 @@ func makeDeploymentSpec(request requests.CreateFunctionRequest, existingSecrets
 		FailureThreshold:    3,
 	}
 
-	initialReplicas := int32p(initialReplicasCount)
-	labels := map[string]string{
-		"faas_function": request.Service,
+	initialReplicas, replicaErr := getMinReplicaCount(request.Labels)
+	if replicaErr != nil {
+		return nil, replicaErr
 	}
-
-	if request.Labels != nil {
-		if min := getMinReplicaCount(*request.Labels); min != nil {
-			initialReplicas = min
-		}
-		for k, v := range *request.Labels {
-			labels[k] = v
-		}
-	}
-
+	labels := buildLabels(request.Service, request.Labels)
 	nodeSelector := createSelector(request.Constraints)
-
 	resources, resourceErr := createResources(request)
 
 	if resourceErr != nil {
@@ -210,7 +196,7 @@ func makeDeploymentSpec(request requests.CreateFunctionRequest, existingSecrets
 		Spec: v1beta1.DeploymentSpec{
 			Selector: &metav1.LabelSelector{
 				MatchLabels: map[string]string{
-					"faas_function": request.Service,
+					FunctionNameLabel: request.Service,
 				},
 			},
 			Replicas: initialReplicas,
@@ -283,7 +269,7 @@ func makeServiceSpec(request requests.CreateFunctionRequest) *corev1.Service {
 		Spec: corev1.ServiceSpec{
 			Type: corev1.ServiceTypeClusterIP,
 			Selector: map[string]string{
-				"faas_function": request.Service,
+				FunctionNameLabel: request.Service,
 			},
 			Ports: []corev1.ServicePort{
 				{
@@ -399,19 +385,6 @@ func createResources(request requests.CreateFunctionRequest) (*apiv1.ResourceReq
 	return resources, nil
 }
 
-func getMinReplicaCount(labels map[string]string) *int32 {
-	if value, exists := labels["com.openfaas.scale.min"]; exists {
-		minReplicas, err := strconv.Atoi(value)
-		if err == nil && minReplicas > 0 {
-			return int32p(int32(minReplicas))
-		}
-
-		log.Println(err)
-	}
-
-	return nil
-}
-
 // configureReadOnlyRootFilesystem will create or update the required settings and mounts to ensure
 // that the ReadOnlyRootFilesystem setting works as expected, meaning:
 // 1. when ReadOnlyRootFilesystem is true, the security context of the container will have ReadOnlyRootFilesystem also

handlers/labels.go

@@ -0,0 +1,65 @@
+package handlers
+
+import (
+	"errors"
+	"fmt"
+	"strconv"
+	"time"
+)
+
+const (
+	// initialReplicasCount how many replicas to start of creating for a function, this is
+	// also used as the default return value for getMinReplicaCount
+	initialReplicasCount = 1
+
+	// FunctionNameLabel is the label key used by OpenFaaS to store the function name
+	// on the resources managed by OpenFaaS for that function.  This key is also used to
+	// denote that a resource is a "Function"
+	FunctionNameLabel = "faas_function"
+	// FunctionMinReplicaCount is a label that user's can set and will be passed to Kubernetes
+	// as the Deployment replicas value.
+	FunctionMinReplicaCount = "com.openfaas.scale.min"
+	// FunctionVersionUID is the lable key used to store the uid value for the deploy/update of a
+	// function, this is currently a unix timestamp.
+	FunctionVersionUID = "com.openfaas.uid"
+)
+
+// buildLabels will copy the user request labels and ensure that any required internal labels
+// are set appropriately.
+func buildLabels(functionName string, requestLables *map[string]string) map[string]string {
+	labels := map[string]string{}
+	if requestLables != nil {
+		for k, v := range *requestLables {
+			labels[k] = v
+		}
+	}
+
+	labels[FunctionNameLabel] = functionName
+	labels[FunctionVersionUID] = fmt.Sprintf("%d", time.Now().Nanosecond())
+
+	return labels
+}
+
+// getMinReplicaCount extracts the functions minimum replica count from the user's
+// request labels. If the value is not found, this will return the default value, 1.
+func getMinReplicaCount(labels *map[string]string) (*int32, error) {
+	if labels == nil {
+		return int32p(initialReplicasCount), nil
+	}
+
+	l := *labels
+	if value, exists := l[FunctionMinReplicaCount]; exists {
+		minReplicas, err := strconv.Atoi(value)
+		if err != nil {
+			return nil, errors.New("could not parse the minimum replica value")
+		}
+
+		if minReplicas > 0 {
+			return int32p(int32(minReplicas)), nil
+		}
+
+		return nil, errors.New("replica count must be a positive integer")
+	}
+
+	return int32p(initialReplicasCount), nil
+}

handlers/lables_test.go

@@ -0,0 +1,143 @@
+package handlers
+
+import (
+	"strings"
+	"testing"
+)
+
+func Test_getMinReplicaCount(t *testing.T) {
+	scenarios := []struct {
+		name   string
+		labels *map[string]string
+		output int
+	}{
+		{
+			name:   "nil map returns default",
+			labels: nil,
+			output: initialReplicasCount,
+		},
+		{
+			name:   "empty map returns default",
+			labels: &map[string]string{},
+			output: initialReplicasCount,
+		},
+		{
+			name:   "empty map returns default",
+			labels: &map[string]string{FunctionMinReplicaCount: "2"},
+			output: 2,
+		},
+	}
+
+	for _, s := range scenarios {
+		t.Run(s.name, func(t *testing.T) {
+			output, err := getMinReplicaCount(s.labels)
+			if err != nil {
+				t.Errorf("getMinReplicaCount should not error on an empty or valid label")
+			}
+			if output == nil {
+				t.Errorf("getMinReplicaCount should not return nil pointer")
+			}
+
+			value := int(*output)
+			if value != s.output {
+				t.Errorf("expected: %d, got %d", s.output, value)
+			}
+		})
+	}
+}
+
+func Test_getMinReplicaCount_Error(t *testing.T) {
+	scenarios := []struct {
+		name   string
+		labels *map[string]string
+		msg    string
+	}{
+		{
+			name:   "negative values should return an error",
+			labels: &map[string]string{FunctionMinReplicaCount: "-2"},
+			msg:    "replica count must be a positive integer",
+		},
+		{
+			name:   "zero values should return an error",
+			labels: &map[string]string{FunctionMinReplicaCount: "0"},
+			msg:    "replica count must be a positive integer",
+		},
+		{
+			name:   "decimal values should return an error",
+			labels: &map[string]string{FunctionMinReplicaCount: "10.5"},
+			msg:    "could not parse the minimum replica value",
+		},
+		{
+			name:   "non-integer values should return an error",
+			labels: &map[string]string{FunctionMinReplicaCount: "test"},
+			msg:    "could not parse the minimum replica value",
+		},
+	}
+
+	for _, s := range scenarios {
+		t.Run(s.name, func(t *testing.T) {
+			output, err := getMinReplicaCount(s.labels)
+			if output != nil {
+				t.Errorf("getMinReplicaCount should return nil value on invalid input")
+			}
+			if !strings.Contains(err.Error(), s.msg) {
+				t.Errorf("unexpected error: expected '%s', got '%s'", s.msg, err.Error())
+			}
+		})
+	}
+}
+
+func Test_parseLabels(t *testing.T) {
+	scenarios := []struct {
+		name         string
+		labels       *map[string]string
+		functionName string
+		output       map[string]string
+	}{
+		{
+			name:         "nil map returns just the function name",
+			labels:       nil,
+			functionName: "testFunc",
+			output:       map[string]string{FunctionNameLabel: "testFunc"},
+		},
+		{
+			name:         "empty map returns just the function name",
+			labels:       &map[string]string{},
+			functionName: "testFunc",
+			output:       map[string]string{FunctionNameLabel: "testFunc"},
+		},
+		{
+			name:         "non-empty map does not overwrite the openfaas internal labels",
+			labels:       &map[string]string{FunctionNameLabel: "anotherValue", "customLabel": "test"},
+			functionName: "testFunc",
+			output:       map[string]string{FunctionNameLabel: "testFunc", "customLabel": "test"},
+		},
+	}
+
+	for _, s := range scenarios {
+		t.Run(s.name, func(t *testing.T) {
+			output := buildLabels(s.functionName, s.labels)
+			if output == nil {
+				t.Errorf("parseLabels should not return nil map")
+			}
+
+			outputFuncName := output[FunctionNameLabel]
+			if outputFuncName != s.functionName {
+				t.Errorf("parseLabels should always set the function name: expected %s, got %s", s.functionName, outputFuncName)
+			}
+
+			_, ok := output[FunctionVersionUID]
+			if !ok {
+				t.Errorf("parseLabels should set the FunctionVersionUID label")
+			}
+
+			for key, expectedValue := range s.output {
+				value := output[key]
+				if value != expectedValue {
+					t.Errorf("Incorrect output label for %s, expected: %s, got %s", key, expectedValue, value)
+				}
+			}
+
+		})
+	}
+}

handlers/reader.go

@@ -39,7 +39,7 @@ func getServiceList(functionNamespace string, clientset *kubernetes.Clientset) (
 	functions := []requests.Function{}
 
 	listOpts := metav1.ListOptions{
-		LabelSelector: "faas_function",
+		LabelSelector: FunctionNameLabel,
 	}
 
 	res, err := clientset.ExtensionsV1beta1().Deployments(functionNamespace).List(listOpts)

handlers/update.go

@@ -72,24 +72,18 @@ func updateDeploymentSpec(
 
 		deployment.Spec.Template.Spec.NodeSelector = createSelector(request.Constraints)
 
-		labels := map[string]string{
-			"faas_function": request.Service,
-			"uid":           fmt.Sprintf("%d", time.Now().Nanosecond()),
-		}
-
-		if request.Labels != nil {
-			if min := getMinReplicaCount(*request.Labels); min != nil {
-				deployment.Spec.Replicas = min
-			}
-
-			for k, v := range *request.Labels {
-				labels[k] = v
-			}
-		}
+		labels := buildLabels(request.Service, request.Labels)
+		labels["uid"] = fmt.Sprintf("%d", time.Now().Nanosecond())
 
 		deployment.Labels = labels
 		deployment.Spec.Template.ObjectMeta.Labels = labels
 
+		replicaCount, replicaErr := getMinReplicaCount(request.Labels)
+		if replicaErr != nil {
+			return replicaErr, http.StatusBadRequest
+		}
+		deployment.Spec.Replicas = replicaCount
+
 		deployment.Annotations = annotations
 		deployment.Spec.Template.Annotations = annotations
 		deployment.Spec.Template.ObjectMeta.Annotations = annotations