Add namespace flag to secrets handlers #530

jonatasbaldin · 2019-10-03T18:39:22Z

Description

Add support to Namespace in Secrets.

I created the getLookupNamespace function, which I'm not 100% proud of, since it reads the r.Body and puts it back into the r struct. BUT this way I didn't need to change all the signatures of all the other handlers, also, if there's any verification for the namespace we can do in one place.

Depends on (build will fail before merging the PRs below and bumping their version in this project):

Motivation and Context

I have raised an issue to propose this change (required) faas-netes #511.

How Has This Been Tested?

Added tests ✨ Also I deployed locally and tested with the following commands:

$ faas-cli secret create secret-cli --from-literal=value

$ curl -u admin:$PASSWORD $OPENFAAS_URL/system/secrets
[{"name":"secret-cli","namespace":"openfaas-fn"}]

# no secret in the default ns
$ curl -u admin:$PASSWORD $OPENFAAS_URL/system/secrets\?namespace\=default    
[]

# creates a secret in the default ns
$ curl -u admin:$PASSWORD $OPENFAAS_URL/system/secrets -X POST -d '{"name": "secret-api", "value": "value", "namespace": "default"}' -H 'Content-Type: application/json

# doesn't list the default secret 
$ curl -u admin:$PASSWORD $OPENFAAS_URL/system/secrets                    
[{"name":"secret-cli","namespace":"openfaas-fn"}]

# list the default secret
$ curl -u admin:$PASSWORD $OPENFAAS_URL/system/secrets\?namespace\=default
[{"name":"secret-api","namespace":"default"}

# changes the secret value from default ns
$ curl -u admin:$PASSWORD $OPENFAAS_URL/system/secrets -X PUT -d '{"name": "secret-api", "value": "value-PUT", "namespace": "default"}' -H 'Content-Type: application/j
son'

# verify secret value
$ k get secret secret-api -o jsonpath='{.data.secret-api}' | base64 -D
value-PUT

# delete secret
$ curl -u admin:$PASSWORD $OPENFAAS_URL/system/secrets -X DELETE -d '{"name": "secret-api", "namespace": "defa
ult"}' -H 'Content-Type: application/json' 

# verify deletion of `secret-api`
$ faas-cli secret list

NAME
secret-cli

Yay! ❤️

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.

viveksyngh · 2019-10-03T18:56:56Z

handlers/secrets.go

+	}
+
+	if lookupNamespace == "kube-system" {
+		return "", fmt.Errorf("unable to list within the kube-system namespace"), http.StatusUnauthorized


I think we can update this error message to something like .. unable to manage secrets within the kube-system namespace. which is related to what this endpoint is trying to do.

Agreed and changed 💃

Yes, but for every place it occurs, not just the one message.

What would the other place be?

alexellis · 2019-10-03T21:35:58Z

I created X which I'm not 100% proud of since Y

^ Hmm, is there an alternative way you could do it then? It sounds like you are not convinced that it's the right solution? cc @LucasRoesler

alexellis · 2019-10-03T21:37:58Z

 curl -u admin:$PASSWORD $OPENFAAS_URL/system/secrets\?namespace\=default
[{"name":"secret-api","namespace":"default"}

Given that the namespace is an input, and we can only have one, why would you say we need it as an output? I'm not saying it's right or wrong, I'm curious about the design.

jonatasbaldin · 2019-10-03T21:48:36Z

I created X which I'm not 100% proud of since Y

^ Hmm, is there an alternative way you could do it then? It sounds like you are not convinced that it's the right solution? cc @LucasRoesler

I'm ok with this solution. Another one would be to do the query parameter verification in the GET handler and the POST verification in the other parameter, which would involve generate the kube client inside each parameter, which would change a lot.

But I think the solution is ok enough to not justify all these changes.

Would love ur input @LucasRoesler!

jonatasbaldin · 2019-10-03T21:50:20Z

 curl -u admin:$PASSWORD $OPENFAAS_URL/system/secrets\?namespace\=default
[{"name":"secret-api","namespace":"default"}
Given that the namespace is an input, and we can only have one, why would you say we need it as an output? I'm not saying it's right or wrong, I'm curious about the design.

I added because in a client – like the CLI – is probably easier to build return data listing the secret name and namespace if the result is in the body.

LucasRoesler

Two suggestions, one addresses the comment from the PR description about parsing the request body. I would recommend that change because it should help generalize the method. The other is a bit bigger of a design suggestion and probably warrants further discussion.

LucasRoesler · 2019-10-04T09:40:18Z

handlers/secrets.go

+		}
+	} else {
+		body, _ := ioutil.ReadAll(r.Body)
+		req := types.Secret{}


you could also use an anonymous struct here, which will possibly make it more flexible

req := struct{ Namespace string }{}

This method could then be used for any POST/PUT/PATCH request that has a Namespace field in the body.

In that case, you could use a switch statement on r.Method to handle all of these cases

switch r.Method { case http.MethodPost, http.MethodPut, http.Patch: // load req body default: // load from GET param }

I like this, will implement!

handlers/secrets.go

LucasRoesler · 2019-10-04T17:54:56Z

handlers/secrets.go

+		r.Body = ioutil.NopCloser(bytes.NewBuffer(body))
+	}
+
+	if req.Namespace == "kube-system" {


It is not just kube-system but the list of namespaces should be actively limited to those with a certain annotation. See

faas-netes/handlers/namespaces.go

Lines 31 to 66 in 5bb7c6b

func list(defaultNamespace string, clientset *kubernetes.Clientset) []string {

listOptions := metav1.ListOptions{}

namespaces, err := clientset.CoreV1().Namespaces().List(listOptions)

set := []string{}

// Assume that an error means that a Role, instead of ClusterRole is being used

// the Role will not be able to list namespaces, so all functions are in the

// defaultNamespace

if err != nil {

log.Printf("Error listing namespaces: %s", err.Error())

set = append(set, defaultNamespace)

return set

}

for _, n := range namespaces.Items {

if _, ok := n.Annotations["openfaas"]; ok {

set = append(set, n.Name)

}

}

if !findNamespace(defaultNamespace, set) {

set = append(set, defaultNamespace)

}

return set

}

func findNamespace(target string, items []string) bool {

for _, n := range items {

if n == target {

return true

}

}

return false

}

This code should be reused here as well.

I think this is yet another issue. We have checks for the kube-system namespace in various place, would need a slightly bigger refactor.

Could you open one?

I am not sure I agree. This change allows the user to modify secrets in namespaces that it isn't allowed to deploy functions to. There is already code for getting and checking the list of allowed. The refactor here would be to call the list method and then, in the if block, use the findNamepsace method. In fact, you could combine those into a single statement, so it could even be a one line change to the PR.

Refactoring the other checks can definitely be done in a separate PR, but I don't think we should ignore it here especially when there is a drop in solution already implemented.

But, I am ok with merging this if @alexellis wants to get the functionality in first and refactor later

Oh yes, I meant opening a PR for the other areas, I'll quickly add the check for this function

Awesome and thanks!

LucasRoesler · 2019-10-04T18:07:23Z

handlers/secrets.go

 		default:
 			w.WriteHeader(http.StatusBadRequest)
 			return
 		}
 	}
 }

+// getLookupNamespace returns the namespace specified in any request type
+func getLookupNamespace(defaultNamespace string, r *http.Request) (string, error, int) {


In general, I would prefer the handler to decide which error code to return instead. One, I generally expect error to be the last item returned from a method

func getLookupNamespace(defaultNamespace string, r *http.Request) (string, error) { }

And second, I think the separation of concerns is cleaner when a handler catches errors and then decides what the appropriate code is.

This method could return a sentinel error (see https://dave.cheney.net/2019/06/10/constant-time for an example if you are not familiar) for each error case and then have the handler can easily use a switch to decide the correct status code. This would then be cleaner for us to reuse the code in the future in cases that are not handlers, for example, in the openfaas-operator

Thanks for the advice and the article, really good read :)

I refactored the code so the handler catches the error, based on the error string (is there a better way than checking the string?).

alexellis · 2019-10-04T18:26:39Z

@LucasRoesler thank you for the abstraction suggestion. I would like to get logs and secrets working e2e and then for someone to refactor with an abstraction like this afterwards in a follow-up PR.

alexellis · 2019-10-04T18:27:21Z

I prefer to merge PRs that do only one thing, i.e. one from below:

Add namespace flag to secrets commands
Refactor all commands to abstraction pattern

LucasRoesler · 2019-10-06T09:12:37Z

handlers/secrets.go

@@ -65,7 +111,8 @@ func getSecretsHandler(kube typedV1.SecretInterface, w http.ResponseWriter, r *h
 	secrets := []types.Secret{}
 	for _, item := range res.Items {
 		secret := types.Secret{
-			Name: item.Name,
+			Name:      item.Name,
+			Namespace: item.Namespace,


This line is incorrect and is why the tests are currently failing, the faas-provider needs to be updated, the latest version is 0.12.0. This needs to be updated in the Gopkg.toml and then you need to run dep ensure again

// Secret for underlying orchestrator type Secret struct { Name string `json:"name"` Value string `json:"value,omitempty"` }

@jonatasbaldin PTAL

alexellis · 2019-10-07T19:17:17Z

handlers/secrets.go

 		default:
 			w.WriteHeader(http.StatusBadRequest)
 			return
 		}
 	}
 }

+// getLookupNamespace returns the namespace specified in any request type
+func getLookupNamespace(defaultNamespace string, r *http.Request) (string, error) {
+	req := struct{ Namespace string }{Namespace: defaultNamespace}


I am finding this confusing. I know Lucas suggested it, but why do we need this over a named type?

This allows you to unmarshal any request that contains a Namespace field, and not just one particular request type.

I think mainly because we didn't have the Secret type from faas-provider with the new Namespace field. Now we do. Will update it.

alexellis · 2019-10-07T19:18:41Z

handlers/secrets_api_test.go

@@ -198,3 +197,71 @@ func Test_SecretsHandler_ListEmpty(t *testing.T) {
 		t.Errorf(`want empty list to be valid json i.e. "[]", but was %q`, string(body))
 	}
 }
+
+func Test_GetLookupNamespace(t *testing.T) {
+	defaultNamespace := "openfaas-fn"


Would this work as a test-case table instead? Example: https://blog.alexellis.io/golang-writing-unit-tests/

I can look into refactoring into table tests, let's just get everything right first 💃

Sure, you can do it via this PR, or you could raise an issue now to track it and do it immediately after. I don't mind which.

jonatasbaldin · 2019-10-07T19:32:57Z

This last commit has the changes needed to check the allowed namespace list.

First, I needed to change the signature from the MakeNamespacesLister and list methods from namespaces.go. They were expecting a concrete type, and I couldn't use the kube client from secrets.go. Now they are the kubernetes.Interface ✨

And in the tests, I'm testing against an allowed namespace – created with the openfaas annotation – and a forbidden one, which has no annotation.

derek · 2019-10-07T19:38:58Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

alexellis · 2019-10-07T19:41:45Z

We can't take a merge commit via the rebase command, please can you revert that last change and apply it with a rebase instead?

git pull origin master --rebase

alexellis · 2019-10-07T19:57:45Z

In other words every commit has to be signed off and merge commits cannot be signed off.

Undo the merge:

git reset HEAD~1

Rebase from master

git pull origin master --rebase

Fix the conflicts:

# Edit the files
git add .
git rebase --continue

Then push back up

git push origin --force

jonatasbaldin · 2019-10-07T21:32:41Z

Thanks @alexellis, I hope this output is the expected (looks like a mess) hehehe

alexellis · 2019-10-08T04:42:47Z

Why does it look like a mess? No, I was just trying to help you revert your unsigned merge commit.

alexellis · 2019-10-08T08:29:17Z

I think we are ready to squash these commits into one and then merge, that's something you can do at your end @jonatasbaldin

Btw I can't see why this "is a mess"? @LucasRoesler thoughts?

jonatasbaldin · 2019-10-08T10:30:41Z

No worries man. I was joking because the PR looked a mess after the rebase because all the commits were listed again, I didn't know this was a thing. Just a 😂 reaction to the output hehehe

Everything is fine 🤙

I can squash the commits a bit later, ok?

Very happy we are almost merging this 🎉✨

Signed-off-by: Jonatas Baldin <[email protected]>

jonatasbaldin · 2019-10-08T12:24:11Z

Squashed ✅

LucasRoesler · 2019-10-08T12:31:59Z

If we merge this first, I will rebase my PR #532 afterwards because there are a couple of places that they overlap

alexellis

Approved.

alexellis · 2019-10-08T12:40:45Z

Thank you for your patience @jonatasbaldin and for the awesome PR to faas-netes. Keep them coming

And when you get a moment, checkout the ask on the unit test table, does it make sense?

jonatasbaldin · 2019-10-08T12:56:22Z

Thanks for all the feedback @alexellis and @LucasRoesler ❤️

@alexellis will put in my backlog for this week :)

alexellis · 2019-10-17T18:32:19Z

Thank you @jonatasbaldin

jonatasbaldin mentioned this pull request Oct 3, 2019

[Feature] multiple namespace support #511

Closed

32 tasks

viveksyngh reviewed Oct 3, 2019

View reviewed changes

jonatasbaldin closed this Oct 3, 2019

jonatasbaldin reopened this Oct 3, 2019

LucasRoesler reviewed Oct 4, 2019

View reviewed changes

LucasRoesler reviewed Oct 6, 2019

View reviewed changes

alexellis reviewed Oct 7, 2019

View reviewed changes

derek bot added the no-dco label Oct 7, 2019

viveksyngh mentioned this pull request Oct 7, 2019

Update secrets command to make them namespace aware openfaas/faas-cli#701

Closed

derek bot removed the no-dco label Oct 7, 2019

alexellis changed the title ~~Secrets namespace~~ Add namespace flag to secrets handlers Oct 8, 2019

Add namespace flag to Secrets CRUD

b3158a1

Signed-off-by: Jonatas Baldin <[email protected]>

alexellis approved these changes Oct 8, 2019

View reviewed changes

alexellis merged commit b340fa1 into openfaas:master Oct 8, 2019

	func list(defaultNamespace string, clientset *kubernetes.Clientset) []string {
	listOptions := metav1.ListOptions{}
	namespaces, err := clientset.CoreV1().Namespaces().List(listOptions)

	set := []string{}

	// Assume that an error means that a Role, instead of ClusterRole is being used
	// the Role will not be able to list namespaces, so all functions are in the
	// defaultNamespace
	if err != nil {
	log.Printf("Error listing namespaces: %s", err.Error())
	set = append(set, defaultNamespace)
	return set
	}

	for _, n := range namespaces.Items {
	if _, ok := n.Annotations["openfaas"]; ok {
	set = append(set, n.Name)
	}
	}

	if !findNamespace(defaultNamespace, set) {
	set = append(set, defaultNamespace)
	}

	return set
	}

	func findNamespace(target string, items []string) bool {
	for _, n := range items {
	if n == target {
	return true
	}
	}
	return false
	}

Add namespace flag to secrets handlers #530

Add namespace flag to secrets handlers #530

Conversation

jonatasbaldin commented Oct 3, 2019

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

viveksyngh Oct 3, 2019 • edited Loading

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 Oct 3, 2019

alexellis commented Oct 3, 2019

jonatasbaldin commented Oct 3, 2019

jonatasbaldin commented Oct 3, 2019

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

jonatasbaldin Oct 5, 2019 • edited Loading

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 Oct 4, 2019

alexellis commented Oct 4, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatasbaldin commented Oct 7, 2019

derek bot commented Oct 7, 2019

alexellis commented Oct 7, 2019

alexellis commented Oct 7, 2019

jonatasbaldin commented Oct 7, 2019

alexellis commented Oct 8, 2019

alexellis commented Oct 8, 2019

jonatasbaldin commented Oct 8, 2019

jonatasbaldin commented Oct 8, 2019

LucasRoesler commented Oct 8, 2019

alexellis left a comment

Choose a reason for hiding this comment

alexellis commented Oct 8, 2019

jonatasbaldin commented Oct 8, 2019

alexellis commented Oct 17, 2019

viveksyngh Oct 3, 2019 •

edited

Loading

jonatasbaldin Oct 5, 2019 •

edited

Loading