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

Add multiple namespaces in the operator #673

Conversation

martindekov
Copy link
Contributor

@martindekov martindekov commented Jul 27, 2020

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
    image

Tested secrets creation across default and custom namespace:
image

Tested logs in default and custom namespace:
image

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.

@martindekov martindekov mentioned this pull request Jul 27, 2020
11 tasks
@martindekov 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 martindekov changed the title Add namespaces support for operator Add multiple namespaces in the operator Jul 28, 2020
Adding support for multiple namespaces in the operator
as per:
openfaas#616

Signed-off-by: Martin Dekov <[email protected]>
@martindekov martindekov force-pushed the mdekov/add_multiple_namespaces_support_operator branch from 090ab11 to b2600e2 Compare July 28, 2020 20:48
@LucasRoesler
Copy link
Member

LucasRoesler commented Aug 2, 2020

@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
Copy link
Contributor Author

martindekov commented Aug 2, 2020

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

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

So I can't create the secret with the CLI

Why?

@LucasRoesler
Copy link
Member

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

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

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 alexellis requested a review from LucasRoesler August 12, 2020 16:12
return func(w http.ResponseWriter, r *http.Request) {
res := handlers.ListNamespaces(defaultNamespace, clientset)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will add in both places

@martindekov
Copy link
Contributor Author

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

@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
Copy link
Contributor Author

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

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

Copy link
Member

@LucasRoesler LucasRoesler left a comment

Choose a reason for hiding this comment

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

@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 👍

defer r.Body.Close()

res, _ := json.Marshal([]string{defaultNamespace})
out, _ := json.Marshal(res)
Copy link
Member

Choose a reason for hiding this comment

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

please catch and at least log the error here

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, even if it's the same elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I delayed the response here, I also agree

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try work out those in #679

@alexellis alexellis requested a review from LucasRoesler August 21, 2020 14:23
@alexellis
Copy link
Member

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

@@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -1,89 +0,0 @@
package server
Copy link
Member

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

Does this cover both REST and the CRD?

@LucasRoesler
Copy link
Member

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

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

@alexellis alexellis merged commit 8a9c15a into openfaas:master Aug 21, 2020
alexellis added a commit that referenced this pull request Aug 21, 2020
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
Copy link
Member

@martindekov please create a PR for the additional comments.

alexellis added a commit that referenced this pull request Aug 26, 2020
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 added a commit to martindekov/faas-netes that referenced this pull request Sep 7, 2020
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]>
alexellis pushed a commit that referenced this pull request Sep 17, 2020
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]>
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 this pull request may close these issues.

[Feature] Operator - add support for managing multiple-namespaces
3 participants