openfaas / faas-netes

Serverless Functions For Kubernetes
https://www.openfaas.com
MIT License
2.12k stars 473 forks source link

Possible override of internal labels by the request labels #209

Open LucasRoesler opened 6 years ago

LucasRoesler commented 6 years ago

Currently we use the faas_function label as a selector in the Deployment of a function. But while debugging an issue with the selector, I noticed that we currently write all user supplied labels into the map that we supply to Kubernetes.

Specifically, it looks like this

    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
        }
    }

With this commit afdf2fc we explicitly require that the faas_function label equal the service name. If the user submits a different value for faas_function, it will break the deployment selector.

Expected Behaviour

The faas_function label should be strictly controlled by the faas system.

Current Behaviour

The value can be overridden by the user.

Possible Solution

We either need to skip select labels while processing the user input or we need to ensure that we set the important internal labels after we have process the user labels.

Steps to Reproduce (for bugs)

  1. Create a stack.yaml with function bar and add the label faas_function: foo
  2. Deploy the function
  3. It should either generate an error about mismatching labels, for example
    
    $ faas deploy
    Deploying: testsecretapi.

Unexpected status: 500, message: Deployment.apps "testsecretapi" is invalid: spec.template.metadata.labels: Invalid value: map[string]string{"faas_function":"foo"}: selector does not match template labels


## Your Environment
<!--- Include as many relevant details about the environment you experienced the bug in -->
* faas-netes version:  functions/faas-netesd:0.5.3
* Docker version `docker version` (e.g. Docker 17.0.05 ):  18.05.0-ce

* What version and distriubtion of Kubernetes are you using? `kubectl version`

Client Version: version.Info{Major:"1", Minor:"9", GitVersion:"v1.9.6", GitCommit:"9f8ebd171479bec0ada837d7ee641dec2f8c6dd1", GitTreeState:"clean", BuildDate:"2018-03-21T15:21:50Z", GoVersion:"go1.9.3", Compiler:"gc", Platform:"darwin/amd64"} Server Version: version.Info{Major:"1", Minor:"9", GitVersion:"v1.9.6", GitCommit:"9f8ebd171479bec0ada837d7ee641dec2f8c6dd1", GitTreeState:"clean", BuildDate:"2018-03-21T15:13:31Z", GoVersion:"go1.9.3", Compiler:"gc", Platform:"linux/amd64"}

alexellis commented 6 years ago

I think this is fixed now. Please can you take a look at the code and verify?

LucasRoesler commented 6 years ago

I think it is still an issue, this is the relevant block on the current master https://github.com/openfaas/faas-netes/blob/d2a9878e8bf31f12290389f6a7bdc913f8ca0718/handlers/deploy.go#L139-L150

Any internal labels should probably be defined as constants and should be set after we process the labels in the request, L140 in particular needs to occur after L150.

LucasRoesler commented 6 years ago

@alexellis i have added a PR with what I think the required change is