Add multiple namespaces in the operator #673

martindekov · 2020-07-27T12:59:14Z

Adding support for multiple namespaces in the operator
as per:
#616

Signed-off-by: Martin Dekov [email protected]

Description

This PR fully covers Issue #616 as opposed to the draft #668 which covered only function object creation. This change is e2e. This is a working change, did not include any major refactoring of the code as proposed by Lucas in the draft, but can revisit.

Motivation and Context

I have raised an issue to propose this change (required)

Closes #616

How Has This Been Tested?

Built change in this image: martindekov/faas-netes:asd-3 and deployed operator with the local chart:

helm install openfaas . -n openfaas --set functionNamespace=openfaas-fn --set basic_auth=false --set clusterRole=true --set operator.create=true --set operator.image=martindekov/faas-netes:asd-3

Tested default behavior (no namespace specified and function is deployed in default openfaas-fn namespace) and when specifying -n flag with functions from store as this change does not involve cli specific logic
Tested in default and custom ns:

Create function
List functions
Invoke function
Remove function

Tested secrets creation across default and custom namespace:

Tested logs in default and custom namespace:

Also tested default crd in extras namespace:

mdekov@mdekov-Lenovo-Y520-15IKBN:~/go/src/github.com/martindekov/dummyfunc$ kubectl get pods -n extras
NAME                          READY   STATUS              RESTARTS   AGE
figlet-5b7cddfb48-kqzzb       1/1     Running             0          3m50s
lockbot-86bf4f7b44-x7rbs      0/1     ContainerCreating   0          6s
stronghash-65c88667bd-dsv9s   1/1     Running             0          3d4h
mdekov@mdekov-Lenovo-Y520-15IKBN:~/go/src/github.com/martindekov/dummyfunc$ cat 
.gitignore      lockbot/        lockbot.yml     something.yaml  template/       
mdekov@mdekov-Lenovo-Y520-15IKBN:~/go/src/github.com/martindekov/dummyfunc$ cat something.yaml 
---
apiVersion: openfaas.com/v1
kind: Function
metadata:
  name: lockbot
  namespace: extras
spec:
  name: lockbot
  image: martindekov/lockbot:latest

mdekov@mdekov-Lenovo-Y520-15IKBN:~/go/src/github.com/martindekov/dummyfunc$

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

Adding support for multiple namespaces in the operator as per: openfaas#616 Signed-off-by: Martin Dekov <[email protected]>

LucasRoesler · 2020-08-02T13:14:23Z

@martindekov i just went through a manual test of the operator and i think i found a bug with secrets

Install this branch

$ kind create cluster
$ kubectl create namespace extras
$ kubectl create namespace openfaas
$ kubectl create namespace openfaas-fn
$ cd chart 
$ helm upgrade --install openfaas ./openfaas --namespace openfaas --values ./openfaas/values.yaml --set operator.create=true --set openfaasImagePullPolicy=Always --set operator.image=martindekov/faas-netes:asd-3 --set clusterRole=true --set gateway.directFunctions=true --set faasnetes.imagePullPolicy=Always --set basic_auth=false

Deploy a test function to the default namespace and to a new namespace extras

$ cat << EOF > stack.yml
provider:
  name: openfaas
  gateway: http://127.0.0.1:8080

functions:
  stronghash:
    skip_build: true
    image: functions/alpine:latest
    fprocess: "sha512sum"
EOF
$ faas-cli deploy
$ faas-cli deploy --namespace=extras
$ kubectl -n extras get function
NAME         AGE
stronghash   13s
$ kubectl -n extras get deploy stronghash
NAME         READY   UP-TO-DATE   AVAILABLE   AGE
stronghash   1/1     1            1           7s
$ faas-cli invoke stronghash --namespace=extras <<< "foo"
0cf9180a764aba863a67b6d72f0918bc131c6772642cb2dce5a34f0a702f9470ddc2bf125c12198b1995c233c34b4afd346c54a2334c350a948a51b6e8b4e6b6

Reset

$ faas-cli remove
$ faas-cli remove --namespace=extras

Test secrets namespace

$ faas-cli secret create password --from-literal=secret_hash
Creating secret: password
Created: 202 Accepted

$ cat << EOF > stack.yml
provider:
  name: openfaas
  gateway: http://127.0.0.1:8080

functions:
  stronghash:
    skip_build: true
    image: functions/alpine:latest
    fprocess: "sha512sum"
    secrets:
        - password
EOF
$ faas-cli deploy
$ kubectl get function -n openfaas-fn
NAME         AGE
stronghash   13s
$ kubectl -n openfaas-fn get deploy stronghash
NAME         READY   UP-TO-DATE   AVAILABLE   AGE
stronghash   1/1     1            1           7s
$ faas-cli invoke stronghash  <<< "foo"
0cf9180a764aba863a67b6d72f0918bc131c6772642cb2dce5a34f0a702f9470ddc2bf125c12198b1995c233c34b4afd346c54a2334c350a948a51b6e8b4e6b6


$ faas-cli secret create --namespace=extras password --from-literal=secret_hash
Creating secret: password
server returned unexpected status code: 400 -

Unfortunately, the operator is not printing any logs after i send that request. I do see the request in the gateway logs though

kubectl -n=openfaas logs deploy/gateway gateway  -f
#...
2020/08/02 13:12:49 Forwarded [POST] to /system/secrets - [400] - 0.009358s seconds
2020/08/02 13:12:50 Forwarded [POST] to /system/secrets - [400] - 0.010448s seconds

kubectl -n=openfaas logs deploy/gateway operator  -f
#...

I don't see any obvious change that would cause this. I am still debugging.

martindekov · 2020-08-02T19:54:27Z

Yeah Lucas you get that if secret already exists, or you are trying to access non existing/annotated namespace. In general believe we can return better or more verbose response.

Did you remove all secrets before creating one? I couldn't replicate the error. It shows only if I am trying to deploy function in non annotated/existing namespace or if a secret already exists in the namespace.

LucasRoesler · 2020-08-03T07:47:57Z

@martindekov the issue is that the secret does not exist yet in the extras namespace that i created. So I can't create the secret with the CLI and I can't deploy the function because the required secret is missing

alexellis · 2020-08-03T12:46:03Z

So I can't create the secret with the CLI

Why?

LucasRoesler · 2020-08-03T14:13:42Z

So I can't create the secret with the CLI

Why?

@alexellis Because of the error I included in my original test. My expectation is that

faas-cli secret create --namespace=extras password --from-literal="super secret value"

in my test should work. @martindekov indicated that the error is because the secret may already exist, but I verified that the namespace was completely empty and had no existing secret.

alexellis · 2020-08-03T14:22:13Z

That seems odd. Is there a secret in openfaas-fn with the same name?

This should work, and only depends on the REST / HTTP implementation, rather than the CRD side of it.

kubectl create ns extras
faas-cli secret create --namespace=extras password --from-literal="super secret value"

LucasRoesler · 2020-08-03T14:26:05Z

Yes i had created the same secret in the default namespace already.

I was testing that I can deploy copies of a function to different namespaces, e.g. dev, stg, prod

alexellis · 2020-08-12T16:15:58Z

pkg/server/namespace.go

 	return func(w http.ResponseWriter, r *http.Request) {
+		res := handlers.ListNamespaces(defaultNamespace, clientset)


Where did defer r.Body.Close() go ?

Well this function is literally taken from handlers.MakeNamespacesLister which does not have defer r.Body.Close().

On another note I could have just reuse handlers.MakeNamespacesLister instead of copy pasting, but this followed how the other handlers are implemented for the operator logic and bigger refactoring should take place as per Lucas's comment from the previous review

Please add it at the top, for belt-and-braces

Good point, will add in both places

martindekov · 2020-08-16T12:38:23Z

@LucasRoesler you did not annotate the extras namespace with openfaas=1, hence the error. Not sure where we checked openfaas=1, is it the gateway? I believe we should not be able to deploy functions or do any operations in namespaces which are not annotated in general.

LucasRoesler · 2020-08-17T15:40:08Z

@martindekov that makes sense. Unfortunately I can't test it right now. My laptop is unavailable for most of this week ☹️

It seems like a good approach to limit via labels/annotations , but I don't recall where it is enforced. It can't be in the gateway because it is agnostic and doesn't really know about namespaces

martindekov · 2020-08-19T19:31:46Z

Yep, Lucas, thanks again for taking a look at this. Opened issue for the problem: #679 it is in both here and in faas-netes so both should be updated. This is not critical for this feature. We can mention operator in the same issue and fix both in one commit

LucasRoesler · 2020-08-20T11:03:11Z

@martindekov make sense, it otherwise seemed to work. I am going to re-test it today and give a final 👍 👎

LucasRoesler

@martindekov i re-tested (with the correct annotations) and now everything is working as expected. I left one comment about an ignored error. Once we log that error, I will give a 👍

LucasRoesler · 2020-08-20T11:30:15Z

pkg/server/namespace.go

-		defer r.Body.Close()
-
-		res, _ := json.Marshal([]string{defaultNamespace})
+		out, _ := json.Marshal(res)


please catch and at least log the error here

@martindekov ?

Agreed, even if it's the same elsewhere

I delayed the response here, I also agree

LucasRoesler · 2020-08-20T11:40:19Z

pkg/server/replicas.go

@@ -81,7 +103,7 @@ func makeReplicaHandler(namespace string, kube kubernetes.Interface) http.Handle
 		}

 		opts := metav1.GetOptions{}
-		dep, err := kube.AppsV1().Deployments(namespace).Get(context.TODO(), functionName, opts)
+		dep, err := kube.AppsV1().Deployments(lookupNamespace).Get(context.TODO(), functionName, opts)


we should probably pass the r.Context() here, but we should open a second PR for this kind of improvement

I will try work out those in #679

alexellis · 2020-08-21T14:24:43Z

@LucasRoesler is there anything else outstanding as far as your'e concerned? Have you had another chance to test since recent changes were made?

alexellis · 2020-08-21T14:25:42Z

main.go

@@ -103,10 +103,15 @@ func main() {
 	// auto-scaling is does via the HTTP API that acts on the deployment Spec.Replicas
 	defaultResync := time.Minute * 5

-	kubeInformerOpt := kubeinformers.WithNamespace(config.DefaultFunctionNamespace)
+	namespaceScope := config.DefaultFunctionNamespace
+	if operator && config.ClusterRole {


Given that we have shared code now, would it make sense to have that namespaceScope set even when we're not using the operator?

I did this as precaution for the feature, otherwise we can merge those, I would need to test the case before doing this though

alexellis · 2020-08-21T14:26:33Z

pkg/config/read_config.go

@@ -49,6 +49,7 @@ func (ReadConfig) Read(hasEnv ftypes.HasEnv) (BootstrapConfig, error) {

 	cfg.DefaultFunctionNamespace = ftypes.ParseString(hasEnv.Getenv("function_namespace"), "default")
 	cfg.ProfilesNamespace = ftypes.ParseString(hasEnv.Getenv("profiles_namespace"), cfg.DefaultFunctionNamespace)
+	cfg.ClusterRole = ftypes.ParseBoolValue(hasEnv.Getenv("cluster_role"), false)


Do we want to think about this internal as "cluster role" or something else, like multiple_namespaces, single_namespace?

Internally would make sense to rename the variable for more friendly experience for the developer, on the other hand this code revolves around kubernetes so I believe developers should be aware of what ClusterRole is

alexellis · 2020-08-21T14:26:59Z

pkg/handlers/namespaces.go

@@ -22,7 +22,7 @@ func MakeNamespacesLister(defaultNamespace string, clientset kubernetes.Interfac
 	return func(w http.ResponseWriter, r *http.Request) {
 		log.Println("Query namespaces")

-		res := list(defaultNamespace, clientset)
+		res := ListNamespaces(defaultNamespace, clientset)


Did this go to being a public function (capital letter) so that it could be used elsewhere?

Yes, I use it in the makeNamespace handler of the operator, though as per Lucas's feedback we don't really need exposing this, we can just reuse the namespace resolver from the faas-netes handlers and just delete pkg/server/namespace.go package as faas-netes and operator resolve namespaces available to the framework the same way

alexellis · 2020-08-21T14:29:06Z

pkg/server/proxy.go

@@ -1,89 +0,0 @@
-package server


Where did this file go and why?

alexellis · 2020-08-21T14:30:14Z

Does this cover both REST and the CRD?

LucasRoesler · 2020-08-21T15:00:53Z

@LucasRoesler is there anything else outstanding as far as your'e concerned? Have you had another chance to test since recent changes were made?

Per my review comment, #673 (review), I have re-teseted and just the one comment about the dropped error remains.

Everything else looked reasonable or expected to me, given previous zoom discussions.

alexellis · 2020-08-21T15:02:13Z

Great. I am restarting the build. Perhaps one of us can tidy up the review comments after merging the change from Martin?

This is part of the review from PR #673 and means that if any type fails to marshal to JSON, that we have a log line available. Signed-off-by: Alex Ellis (OpenFaaS Ltd) <[email protected]>

alexellis · 2020-08-26T16:13:05Z

@martindekov please create a PR for the additional comments.

This is part of the review from PR #673 and means that if any type fails to marshal to JSON, that we have a log line available. Signed-off-by: Alex Ellis (OpenFaaS Ltd) <[email protected]>

Refering feedback from the review openfaas#673 which adds multiple namespaces functionality which includes the following: * Extend operator readme to mention how to enable the feature * Remove mentioning that the operator does not support the feature * Replacing TODO context with the context from the request * Deferring the closing of the body of the namespaces handler * Merging the cluster role configuration so it is used in faas-netes * Reusing the logic of namespaces and secrets handling present in faas-netes in the operator. Removing the custom implementations as they are no longer used Signed-off-by: Martin Dekov <[email protected]>

Refering feedback from the review #673 which adds multiple namespaces functionality which includes the following: * Extend operator readme to mention how to enable the feature * Remove mentioning that the operator does not support the feature * Replacing TODO context with the context from the request * Deferring the closing of the body of the namespaces handler * Merging the cluster role configuration so it is used in faas-netes * Reusing the logic of namespaces and secrets handling present in faas-netes in the operator. Removing the custom implementations as they are no longer used Signed-off-by: Martin Dekov <[email protected]>

martindekov mentioned this pull request Jul 27, 2020

Add namespaces support for REST #668

Closed

11 tasks

martindekov force-pushed the mdekov/add_multiple_namespaces_support_operator branch 2 times, most recently from b2b02ac to 090ab11 Compare July 28, 2020 20:45

martindekov changed the title ~~Add namespaces support for operator~~ Add multiple namespaces in the operator Jul 28, 2020

Add multiple namespaces in the operator

b2600e2

Adding support for multiple namespaces in the operator as per: openfaas#616 Signed-off-by: Martin Dekov <[email protected]>

martindekov force-pushed the mdekov/add_multiple_namespaces_support_operator branch from 090ab11 to b2600e2 Compare July 28, 2020 20:48

alexellis requested a review from LucasRoesler August 12, 2020 16:12

alexellis reviewed Aug 12, 2020

View reviewed changes

LucasRoesler requested changes Aug 20, 2020

View reviewed changes

alexellis requested a review from LucasRoesler August 21, 2020 14:23

alexellis reviewed Aug 21, 2020

View reviewed changes

pkg/server/proxy.go

@@ -1,89 +0,0 @@

package server

Copy link

Member

alexellis Aug 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did this file go and why?

alexellis merged commit 8a9c15a into openfaas:master Aug 21, 2020

alexellis added a commit that referenced this pull request Aug 21, 2020

Log json.Marshal errors

d00b38d

This is part of the review from PR #673 and means that if any type fails to marshal to JSON, that we have a log line available. Signed-off-by: Alex Ellis (OpenFaaS Ltd) <[email protected]>

alexellis mentioned this pull request Aug 21, 2020

Log json.Marshal errors #682

Merged

alexellis added a commit that referenced this pull request Aug 26, 2020

Log json.Marshal errors

7884d1f

This is part of the review from PR #673 and means that if any type fails to marshal to JSON, that we have a log line available. Signed-off-by: Alex Ellis (OpenFaaS Ltd) <[email protected]>

martindekov mentioned this pull request Sep 7, 2020

Refer feedback from #673 and augment documentation #684

Merged

12 tasks

alexellis mentioned this pull request Sep 17, 2020

Functions are manipulated in non-annotated namespace #679

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multiple namespaces in the operator #673

Add multiple namespaces in the operator #673

martindekov commented Jul 27, 2020 •

edited

Loading

LucasRoesler commented Aug 2, 2020 •

edited

Loading

martindekov commented Aug 2, 2020 •

edited

Loading

LucasRoesler commented Aug 3, 2020

alexellis commented Aug 3, 2020

LucasRoesler commented Aug 3, 2020

alexellis commented Aug 3, 2020

LucasRoesler commented Aug 3, 2020

alexellis Aug 12, 2020

martindekov Aug 16, 2020

alexellis Aug 21, 2020

martindekov Aug 22, 2020

martindekov commented Aug 16, 2020

LucasRoesler commented Aug 17, 2020

martindekov commented Aug 19, 2020

LucasRoesler commented Aug 20, 2020

LucasRoesler left a comment

LucasRoesler Aug 20, 2020

alexellis Aug 21, 2020

alexellis Aug 21, 2020

martindekov Aug 22, 2020

LucasRoesler Aug 20, 2020

martindekov Aug 22, 2020

alexellis commented Aug 21, 2020

alexellis Aug 21, 2020

martindekov Aug 22, 2020

alexellis Aug 21, 2020

martindekov Aug 22, 2020

alexellis Aug 21, 2020

martindekov Aug 22, 2020

alexellis Aug 21, 2020

alexellis commented Aug 21, 2020

LucasRoesler commented Aug 21, 2020

alexellis commented Aug 21, 2020

alexellis commented Aug 26, 2020

		return func(w http.ResponseWriter, r *http.Request) {
		res := handlers.ListNamespaces(defaultNamespace, clientset)

Add multiple namespaces in the operator #673

Add multiple namespaces in the operator #673

Conversation

martindekov commented Jul 27, 2020 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

LucasRoesler commented Aug 2, 2020 • edited Loading

martindekov commented Aug 2, 2020 • edited Loading

LucasRoesler commented Aug 3, 2020

alexellis commented Aug 3, 2020

LucasRoesler commented Aug 3, 2020

alexellis commented Aug 3, 2020

LucasRoesler commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martindekov commented Aug 16, 2020

LucasRoesler commented Aug 17, 2020

martindekov commented Aug 19, 2020

LucasRoesler commented Aug 20, 2020

LucasRoesler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis commented Aug 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis commented Aug 21, 2020

LucasRoesler commented Aug 21, 2020

alexellis commented Aug 21, 2020

alexellis commented Aug 26, 2020

martindekov commented Jul 27, 2020 •

edited

Loading

LucasRoesler commented Aug 2, 2020 •

edited

Loading

martindekov commented Aug 2, 2020 •

edited

Loading