Skip to content

Commit

Permalink
Refactor label parsing to set OF labels last
Browse files Browse the repository at this point in the history
**What**
- Ensure that internal OF labels are set after user labels to ensure
that users do no override these internal values
- Refactor the getMinReplicaCount to work with the labels pointer, this
helps simplify the control flow in the handler methods
- Add constants for the label keys and update all references to those
keys throughout

Closes openfaas#209

Signed-off-by: Lucas Roesler <[email protected]>
  • Loading branch information
LucasRoesler committed Oct 7, 2018
1 parent 5dadd40 commit 310b5ec
Show file tree
Hide file tree
Showing 6 changed files with 155 additions and 50 deletions.
2 changes: 1 addition & 1 deletion handlers/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -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[OFFunctionNameLabel]; found {
return true
}
}
Expand Down
38 changes: 4 additions & 34 deletions handlers/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"os"
"path/filepath"
"regexp"
"strconv"
"strings"

"github.com/openfaas/faas/gateway/requests"
Expand All @@ -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])?$`)
Expand Down Expand Up @@ -165,22 +161,9 @@ func makeDeploymentSpec(request requests.CreateFunctionRequest, existingSecrets
FailureThreshold: 3,
}

initialReplicas := int32p(initialReplicasCount)
labels := map[string]string{
"faas_function": request.Service,
}

if request.Labels != nil {
if min := getMinReplicaCount(*request.Labels); min != nil {
initialReplicas = min
}
for k, v := range *request.Labels {
labels[k] = v
}
}

initialReplicas := getMinReplicaCount(request.Labels)
labels := parseLabels(request.Service, request.Labels)
nodeSelector := createSelector(request.Constraints)

resources, resourceErr := createResources(request)

if resourceErr != nil {
Expand Down Expand Up @@ -210,7 +193,7 @@ func makeDeploymentSpec(request requests.CreateFunctionRequest, existingSecrets
Spec: v1beta1.DeploymentSpec{
Selector: &metav1.LabelSelector{
MatchLabels: map[string]string{
"faas_function": request.Service,
OFFunctionNameLabel: request.Service,
},
},
Replicas: initialReplicas,
Expand Down Expand Up @@ -283,7 +266,7 @@ func makeServiceSpec(request requests.CreateFunctionRequest) *corev1.Service {
Spec: corev1.ServiceSpec{
Type: corev1.ServiceTypeClusterIP,
Selector: map[string]string{
"faas_function": request.Service,
OFFunctionNameLabel: request.Service,
},
Ports: []corev1.ServicePort{
{
Expand Down Expand Up @@ -399,19 +382,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
Expand Down
55 changes: 55 additions & 0 deletions handlers/labels.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package handlers

import (
"log"
"strconv"
)

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

// OFFunctionNameLabel 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"
OFFunctionNameLabel = "faas_function"
// OFFunctionMinReplicaCount is a label that user's can set and will be passed to Kubernetes
// as the Deployment replicas value.
OFFunctionMinReplicaCount = "com.openfaas.scale.min"
)

// parseLabels will copy the user request labels and ensure that any required internal labels
// are set appropriately.
func parseLabels(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[OFFunctionNameLabel] = functionName

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 {
if labels == nil {
return int32p(initialReplicasCount)
}

l := *labels
if value, exists := l[OFFunctionMinReplicaCount]; exists {
minReplicas, err := strconv.Atoi(value)
if err == nil && minReplicas > 0 {
return int32p(int32(minReplicas))
}

log.Println(err)
}

return int32p(initialReplicasCount)
}
91 changes: 91 additions & 0 deletions handlers/lables_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package handlers

import "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{OFFunctionMinReplicaCount: "2"},
output: 2,
},
}

for _, s := range scenarios {
t.Run(s.name, func(t *testing.T) {
output := getMinReplicaCount(s.labels)
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_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{OFFunctionNameLabel: "testFunc"},
},
{
name: "empty map returns just the function name",
labels: &map[string]string{},
functionName: "testFunc",
output: map[string]string{OFFunctionNameLabel: "testFunc"},
},
{
name: "non-empty map does not overwrite the function name label",
labels: &map[string]string{OFFunctionNameLabel: "anotherValue", "customLabel": "test"},
functionName: "testFunc",
output: map[string]string{OFFunctionNameLabel: "testFunc", "customLabel": "test"},
},
}

for _, s := range scenarios {
t.Run(s.name, func(t *testing.T) {
output := parseLabels(s.functionName, s.labels)
if output == nil {
t.Errorf("parseLabels should not return nil map")
}

outputFuncName := output[OFFunctionNameLabel]
if outputFuncName != s.functionName {
t.Errorf("parseLabels should always set the function name: expected %s, got %s", s.functionName, outputFuncName)
}

for key, value := range output {
expectedValue := s.output[key]
if value != expectedValue {
t.Errorf("Incorrect output label for %s, expected: %s, got %s", key, expectedValue, value)
}
}

})
}
}
2 changes: 1 addition & 1 deletion handlers/reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func getServiceList(functionNamespace string, clientset *kubernetes.Clientset) (
functions := []requests.Function{}

listOpts := metav1.ListOptions{
LabelSelector: "faas_function",
LabelSelector: OFFunctionNameLabel,
}

res, err := clientset.ExtensionsV1beta1().Deployments(functionNamespace).List(listOpts)
Expand Down
17 changes: 3 additions & 14 deletions handlers/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,23 +72,12 @@ 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 := parseLabels(request.Service, request.Labels)
labels["uid"] = fmt.Sprintf("%d", time.Now().Nanosecond())

deployment.Labels = labels
deployment.Spec.Template.ObjectMeta.Labels = labels
deployment.Spec.Replicas = getMinReplicaCount(request.Labels)

deployment.Annotations = annotations
deployment.Spec.Template.Annotations = annotations
Expand Down

0 comments on commit 310b5ec

Please sign in to comment.