openfaas / faas-cli

Official CLI for OpenFaaS
https://www.openfaas.com/
Other
794 stars 226 forks source link

namespace flag is ignored when stack.yml exists #742

Closed doowb closed 4 years ago

doowb commented 4 years ago

Expected Behaviour

When I run:

faas deploy --name my-fn-name --image doowb/my-fn-name --namespace my-namespace

I expect the namespace flag to be used and the output to look like:

Deploying: my-fn-name.

Deployed. 202 Accepted.
URL: https://gateway.example.com/function/my-fn-name.my-namespace

Current Behaviour

The above works when there is not a stack.yml file in the directory. The following is the result I get when there is a stack.yml file, but there is not a namespace specified for the function in the file:

Deploying: my-fn-name.

Deployed. 202 Accepted.
URL: https://gateway.example.com/function/my-fn-name

Possible Solution

If it is not the desired behavior to override the namespace with the flag, then I think the documentation should be updated to specify that, otherwise, let the flag override the stack specification. Right now the documentation doesn't say either way, rather there are only examples on how to deploy with the namespace in the file, or using the flag when deploying from the store.

Context

This affected me because I was automating part of my deployment and I wanted to deploy a function to multiple namespaces, but it only deployed to the one specified in the stack.yml.

Your Environment

CLI:
 commit:  73004c23e5a4d3fdb7352f953247473477477a64
 version: 0.11.3

Gateway
 uri:     https://gateway.example.com
 version: 0.18.6
 sha:     04fead5eabc4e68fcd746b8c400ded5eeb41266b
 commit:  Hide the namespace selector when on swarm or <2 K8s ns

Provider
 name:          faas-netes
 orchestration: kubernetes
 version:       0.9.15
 sha:           41c33f9f7c29e8276bd01387f78d6f0cff847890
Waterdrips commented 4 years ago

Hey @doowb , did you want to have a look at fixing this? Otherwise I can take a look

alexellis commented 4 years ago

I think this behaviour is working as expected, kubectl works the same way as far as I remember.

alexellis commented 4 years ago

You can provide a namespace override inside the YAML file and if you like you can use an envsubst to provide overrides on that field

Waterdrips commented 4 years ago

You can provide a namespace override inside the YAML file and if you like you can use an envsubst to provide overrides on that field

Oh yeah, env var substitution that's way better

doowb commented 4 years ago

I think this behaviour is working as expected, kubectl works the same way as far as I remember.

Thank you for mentioning that... I didn't know this and I'm used to how the --gateway flag can override the value in the stack.yml so I made an assumption that the --namespace flag would also override the value in stack.yml.

I couldn't find specific documentation for the kubectl behavior (what I found through issues really only refers to the value specified in the kubeconfig or current context settings), so I experimented and discovered that an error is thrown if the --namespace flag value does not match the namespace specified in a yaml file (for instance a deployment manifest).

You can provide a namespace override inside the YAML file and if you like you can use an envsubst to provide overrides on that field

Is it okay to leave this issue open and I can submit a PR to add a suggestion to use the envsubst in the documentation?

LucasRoesler commented 4 years ago

I want to follow up on this, because I ran into this today with a team member and it was not their default expected behavior. I showed him the envsubt pattern because I have ran into this before and while it worked, it definitely slowed the process of deployment down.

This actually almost would have caused down time but we were lucky and the default value was set to a dev namespace instead of production.

At the minimum, there should be some warning that the flag is being ignored because I could easily see this causing confusion as a broken function is deployed into production. I would actually feel better with throwing an error and exiting, the flag and the stack file are incompatible and continuing the deployment is going to have unexpected behavior. Similar to what @doowb mentions above.

I understand the argument that kubectl has the same behavior but both myself and this team member are experienced and familiar with kubectl and we were still surprised the first time we experienced this in faas-cli

alexellis commented 4 years ago

@LucasRoesler as discussed in the members call can you outline a few brief command line / stack YAML examples to show the problem?

Is this similar to the "priority" approach we take for the gateway URL?

LucasRoesler commented 4 years ago

This came up over in https://github.com/openfaas/faas-cli/issues/693#issuecomment-622927949

I will copy the comment here since this is the open issue:

The use case my data science team has is that they want to deploy their models to a DEV, STG, and default namespace as part of the development flow.

The desired workflow is that

  1. all merges to master would run faas-cli up --namespace DEV --env db=...
  2. all tags on master would run faas-cli up --namespace STG --env db=...
  3. finally, there is a custom Jenkins task that they can run (via slack slash command) after verifying the deployment is stable on stg, this task would faas-cli up --namespace PROD --env db=...

We worked around this by using env substitution, but it caused and still causes a lot of confusion to the ops and data science team. Every other flag seems to override the stack file except this one flag. It honestly wasted a day of effort because the team thought they were messing something up and spent a day trying to debug before asking me about it.

Our workaround looks like this

  1. all merges to master run namespace=DEV faas-cli up --env db=...
  2. all tags on master run namespace=STG faas-cli up --env db=...
  3. the custom Jenkins task runs namespace=PROD faas-cli up --env db=...

Where we added namespace: ${namespace:-DEV} into the stack files. But this was only implemented because I told them that it was possible.

alexellis commented 4 years ago

That's great context to have, thanks. When I mentioned the gateway URL priority logic, that's what I was looking for from the community.

Something like this for all the permutations.

stack.yml CLI flag desired namespace
"" "" ""
LucasRoesler commented 4 years ago

My expectation is that the flags should all behave relatively the same, in this case a since it is a single string value like the gateway flag, it should be exactly like the gateway flag. This allows users to develop an intuation and consistent expectations about how the flags behave

LucasRoesler commented 4 years ago
stack.yml CLI flag desired namespace
"" "foo" "foo"
"bar" "foo" "foo"
"bar" "" "bar"
"" "" "openfaas-fn"
alexellis commented 4 years ago

Feel free to implement the above grid in a PR.

Since you mention consistency, and working the same way for everything, do you want to consider environment variables for this behaviour OPENFAAS_NS as per the gateway behaviour?

LucasRoesler commented 4 years ago

@alexellis env, annotations, labels, and secrets already behave this way. Basically every reference of mergeSlice and mergeMap

https://github.com/openfaas/faas-cli/blob/f7c29ea19b5df9d7aa87e9c70aacf4d9315da2cd/commands/deploy.go#L228 https://github.com/openfaas/faas-cli/blob/f7c29ea19b5df9d7aa87e9c70aacf4d9315da2cd/commands/deploy.go#L263 https://github.com/openfaas/faas-cli/blob/f7c29ea19b5df9d7aa87e9c70aacf4d9315da2cd/commands/deploy.go#L230 https://github.com/openfaas/faas-cli/blob/f7c29ea19b5df9d7aa87e9c70aacf4d9315da2cd/commands/deploy.go#L198

Function image partially behaves this way, in that we have flags that allow you to override part of the value in the image name https://github.com/openfaas/faas-cli/blob/f7c29ea19b5df9d7aa87e9c70aacf4d9315da2cd/commands/deploy.go#L270

I think is a big part of why it is currently confusing/inconsistent.

The construction of the values is a little inconsistent, because the flag for read only fs will overwrite the value form the stack file https://github.com/openfaas/faas-cli/blob/f7c29ea19b5df9d7aa87e9c70aacf4d9315da2cd/commands/deploy.go#L273 so it isn't immediately obvious in the final request struct that it comes from the yml or flag

Constraint flag also overrides the yaml, not a merge (this behavior makes sense to me) https://github.com/openfaas/faas-cli/blob/f7c29ea19b5df9d7aa87e9c70aacf4d9315da2cd/commands/deploy.go#L190-L195

Because of this https://github.com/openfaas/faas-cli/blob/f7c29ea19b5df9d7aa87e9c70aacf4d9315da2cd/commands/deploy.go#L162, the network flag also overrides any other setting

utsavanand2 commented 4 years ago

@LucasRoesler @viveksyngh @Waterdrips The fix is way simple than what I hoped for, at least when it comes to fixing the namespace issue. Do you think I have the same behavior as the grid defined by @LucasRoesler

➜  function cat func.yml
version: 1.0
provider:
  name: openfaas
  gateway: http://127.0.0.1:31112
functions:
  func:
    lang: go
    handler: ./func
    labels:
      hello: world
    image: utsavanand2/func:0.1

➜  function faas deploy -f func.yml
Deploying: func.
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.

Deployed. 202 Accepted.
URL: http://127.0.0.1:31112/function/func

➜  function kubectl get pods -n openfaas-fn
NAME                    READY   STATUS    RESTARTS   AGE
func-679944d79b-wkktv   1/1     Running   0          23s
➜  function # After editing func.yaml to add namespace: default
➜  function cat func.yml
version: 1.0
provider:
  name: openfaas
  gateway: http://127.0.0.1:31112
functions:
  func:
    lang: go
    handler: ./func
    namespace: default
    labels:
      hello: world
    image: utsavanand2/func:0.1

➜  function faas deploy -f func.yml
Deploying: func.
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.

Deployed. 202 Accepted.
URL: http://127.0.0.1:31112/function/func.default

➜  function kubectl get pods
NAME                    READY   STATUS    RESTARTS   AGE
func-679944d79b-hk546   1/1     Running   0          7s
➜  function # Now override the namespace defined in func.yaml to use "test" namespace
➜  function faas deploy -f func.yml -n test
Deploying: func.
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.

Deployed. 202 Accepted.
URL: http://127.0.0.1:31112/function/func.test

➜  function kubectl get pods -n test
NAME                    READY   STATUS    RESTARTS   AGE
func-679944d79b-95w5g   1/1     Running   0          12s
LucasRoesler commented 4 years ago

@utsavanand2 that seems to cover it correctly

LucasRoesler commented 4 years ago

@utsavanand2 are you going to open the corresponding PR?

utsavanand2 commented 4 years ago

@LucasRoesler Yes I'm implementing some tests to make sure we verify our deployment as per the grid defined above

LucasRoesler commented 4 years ago

Music to my ears 🎶