diff --git a/handlers/delete.go b/handlers/delete.go index 6ab9e7010..7b69fa120 100644 --- a/handlers/delete.go +++ b/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 } } diff --git a/handlers/deploy.go b/handlers/deploy.go index 7d5c4862a..0c2a63c7d 100644 --- a/handlers/deploy.go +++ b/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 diff --git a/handlers/labels.go b/handlers/labels.go new file mode 100644 index 000000000..736cd3bcf --- /dev/null +++ b/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 +} diff --git a/handlers/lables_test.go b/handlers/lables_test.go new file mode 100644 index 000000000..1f1371d8d --- /dev/null +++ b/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) + } + } + + }) + } +} diff --git a/handlers/reader.go b/handlers/reader.go index 89db7f107..12b63f2ff 100644 --- a/handlers/reader.go +++ b/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) diff --git a/handlers/update.go b/handlers/update.go index b70c7a5b1..66c8df18f 100644 --- a/handlers/update.go +++ b/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