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 secrets CRUD API #70

Merged
merged 18 commits into from Jan 8, 2019

Conversation

Projects
None yet
3 participants
@stefanprodan
Copy link
Member

stefanprodan commented Jan 4, 2019

  • implement HTTP GET/POST/PUT/DEL for secret type
  • omit the secret value from GET result
  • return only error codes and log the error body
  • use label app.kubernetes.io/managed-by: openfaas to filter secrets created by the operator
  • add happy path CRUD tests
  • update Kubernetes deps to v1.12 (fix for klog)
  • update faas packages to get the secrets handler and struct

Fix: #69

Motivation and Context

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

How Has This Been Tested?

Tested on GKE with curl:

Create secret:

curl -vd '{"name":"test","value":"test"}' -X POST http://localhost:8081/system/secrets

 HTTP/1.1 202 Accepted

List secrets:

curl -s http://localhost:8081/system/secrets

[{"name":"test"},{"name":"test2"}]

Update secret:

curl -vd '{"name":"test","value":"test update"}' -X PUT http://localhost:8081/system/secrets

HTTP/1.1 202 Accepted

Delete secret:

curl -vd '{"name":"test"}' -X DELETE http://localhost:8081/system/secrets

HTTP/1.1 202 Accepted

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)

Impact to existing users

None

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.

stefanprodan added some commits Jan 4, 2019

Update faas-provider to v0.8.1
Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
Update faas packages to get the secrets handler and struct
Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
Add secrets CRUD API
- implement GET/POST/PUT/DEL for secret type
- omit the secret value from GET result
- return only error codes and log the error body
- use SecretNamespaceLister to avoid the K8s API rate limiter

Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
Add owner label to secrets
- use the label selector in secrets GET

Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
Add secrets API curl examples to readme
Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
Sync the glog and klog flags
Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
Update Kubernetes deps to v1.12
- tested on GKE 1.11

Signed-off-by: stefanprodan <stefan.prodan@gmail.com>

@stefanprodan stefanprodan changed the title WIP: Add secrets CRUD API Add secrets CRUD API Jan 5, 2019

stefanprodan added some commits Jan 5, 2019

Fix k8s.io/code-generator deps
Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
Update client set to Kubernetes v.1.12
- use Go 1.11 in TravisCI

Signed-off-by: stefanprodan <stefan.prodan@gmail.com>

@alexellis alexellis requested a review from LucasRoesler Jan 5, 2019

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Jan 5, 2019

@stefanprodan please could you provide your curl statements and results? It'll be useful for giving to the others who are doing faas-swarm etc whilst we have no CLI command available.

@stefanprodan

This comment has been minimized.

Copy link
Member

stefanprodan commented Jan 5, 2019

The curl statements are documented in the readme. I will add the results to the PR description.

stefanprodan added some commits Jan 5, 2019

Sync CRD with faas-netes chart
- remove secrets lister (filter not working on K8s 1.11)

Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
Fix secrets update
- replace lister with corev1 client

Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
Add secrets CRUD to RBAC
Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
@stefanprodan

This comment has been minimized.

Copy link
Member

stefanprodan commented Jan 5, 2019

@alexellis @LucasRoesler I've updated the PR description with the curl tests examples and results.

Make sure you apply the RBAC before testing since the operator now needs full access to secrets, before was readonly.

}
}

func getSecret(namespace string, r io.Reader) (*corev1.Secret, error) {

This comment has been minimized.

@LucasRoesler

LucasRoesler Jan 5, 2019

Member

Everything looks good and my manual tests worked. The only thing I thought was confusing in the code was the name of this method. I think parseSecret would be more accurate . Other than that 💯

Name: req.Name,
Namespace: namespace,
Labels: map[string]string{
"owner": "openfaas",

This comment has been minimized.

@LucasRoesler

LucasRoesler Jan 5, 2019

Member

I was thinking more about this label, I think k8s is recommending specificy label formats now https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels

I wonder if app.kubernetes.io/part-of or app.kubernetes.io/managed-by would be a better label name?

This comment has been minimized.

@stefanprodan

stefanprodan Jan 5, 2019

Member

Yes you're right, I totally forgot about that convention. I'll change it.

@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Jan 5, 2019

I also tested this with the latest build of gateway (openfaas/faas@a65df47) and everything works as expected

@LucasRoesler LucasRoesler referenced this pull request Jan 5, 2019

Merged

Add secret list command to faas-cli #578

3 of 11 tasks complete
Use app.kubernetes.io/managed-by for secrets labeling
Signed-off-by: stefanprodan <stefan.prodan@gmail.com>

@LucasRoesler LucasRoesler referenced this pull request Jan 6, 2019

Merged

Port secrets management from OF Operator #344

8 of 11 tasks complete

stefanprodan added some commits Jan 6, 2019

Add secret handler tests
- port from https://github.com/openfaas/faas-netes/pull/344/files

Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
Add client-go fake package
Signed-off-by: stefanprodan <stefan.prodan@gmail.com>

stefanprodan added some commits Jan 6, 2019

Rename secret API test to match the handler name
Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
Add tests for functions API create and update operations
Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
@@ -105,13 +105,13 @@ Start OpenFaaS controller (assumes you have a working kubeconfig on the machine)

```bash
$ go build \
&& ./openfaas-operator -kubeconfig=$HOME/.kube/config -logtostderr=true -v=4

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

Does that also need removing from helm, or is it a no-harm change?

This comment has been minimized.

@stefanprodan

stefanprodan Jan 7, 2019

Member

logtostderr=true is now the default so no need to specify it, there is a no-harm change

@@ -45,18 +45,3 @@ spec:
memory:
type: string
pattern: "^[0-9]+(Mi|Gi)"
secrets:

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

Why did this get removed?

This comment has been minimized.

@stefanprodan

stefanprodan Jan 7, 2019

Member

It doesn't work if there are no secrets specified, I've synced the CRD that's in the faas-netes chart and that one is working fine.

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

If this is fixing a bug can we track that separately or in the issue tracker?

This comment has been minimized.

@stefanprodan

stefanprodan Jan 7, 2019

Member

this was fixed in faas-netes 4 months ago openfaas/faas-netes@da1ad5a

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

Good to know 👍 Having multiple unrelated changes just means a bit more context is needed on reviews.

main.go Outdated
@@ -41,6 +41,18 @@ func main() {
flag.Set("logtostderr", "true")
flag.Parse()

klogFlags := flag.NewFlagSet("klog", flag.ExitOnError)

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

Please extract to a function with a name to document what's going on or a comment.

This comment has been minimized.

@stefanprodan

stefanprodan Jan 7, 2019

Member

the comment is 2 lines below, it syncs the the glog flags with klog

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

Can you extract to a function please?

defer r.Body.Close()
}

switch r.Method {

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

Please can you split each out into its own method for maintainability. I've asked @bartsmykla to do the same for faas-swarm.

selector := fmt.Sprintf("%s=%s", secretLabel, secretLabelValue)
res, err := kube.CoreV1().Secrets(namespace).List(metav1.ListOptions{LabelSelector: selector})
if err != nil {
w.WriteHeader(http.StatusInternalServerError)

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

In faas-netes we write the error back to the client. I think we should do that here too, or if you're worried about leaking data then at last write what happened without quoting the error.

This comment has been minimized.

@stefanprodan

stefanprodan Jan 7, 2019

Member

None of the API handlers are returning a body on error, just status codes. If we want this then we should define an error struct and refactor the whole API.

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

We do write the error to the client, here's an example: https://github.com/openfaas/faas-netes/blob/master/handlers/deploy.go#L79

No need to refactor to an error struct, just write the error and a sensible message to the client like above?

return
}
secret, err := kube.CoreV1().Secrets(namespace).Get(newSecret.GetName(), metav1.GetOptions{})
if errors.IsNotFound(err) {

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

I think that we should write a text error back in the body.

func parseSecret(namespace string, r io.Reader) (*corev1.Secret, error) {
body, _ := ioutil.ReadAll(r)
req := requests.Secret{}
err := json.Unmarshal(body, &req)

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

Can this be inlined whilst making the other edits?

README.md Outdated
@@ -145,6 +145,8 @@ kubectl get all

### API calls

#### Function CRUD

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

Replace CRUD with management?

Refactor secrets API
- break CRUD operations into functions
- return errors in the HTTP response body

Signed-off-by: stefanprodan <stefan.prodan@gmail.com>
@stefanprodan

This comment has been minimized.

Copy link
Member

stefanprodan commented Jan 7, 2019

@alexellis the latest commit addresses your comments.

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Jan 8, 2019

I would still like to see the handler split out into multiple functions per action / HTTP method.

I'll merge and this can be done afterwards. cc @LucasRoesler

Alex

@alexellis alexellis merged commit a315ddf into openfaas-incubator:master Jan 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment