Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible override of internal labels by the request labels #209

Open
LucasRoesler opened this issue May 25, 2018 · 3 comments
Open

Possible override of internal labels by the request labels #209

LucasRoesler opened this issue May 25, 2018 · 3 comments

Comments

@LucasRoesler
Copy link
Member

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

  • 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
Copy link
Member

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

@LucasRoesler
Copy link
Member Author

I think it is still an issue, this is the relevant block on the current master

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

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 added a commit to LucasRoesler/faas-netes that referenced this issue Jun 23, 2018
**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 <roesler.lucas@gmail.com>
LucasRoesler added a commit to LucasRoesler/faas-netes that referenced this issue Jun 23, 2018
**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 <roesler.lucas@gmail.com>
@LucasRoesler
Copy link
Member Author

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

LucasRoesler added a commit to LucasRoesler/faas-netes that referenced this issue Aug 3, 2018
**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 <roesler.lucas@gmail.com>
LucasRoesler added a commit to LucasRoesler/faas-netes that referenced this issue Oct 7, 2018
**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 <roesler.lucas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants