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

[WIP] - Feature/secret management to facilitate proposal #807 #968

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@acornies
Copy link
Member

acornies commented Nov 19, 2018

Description

This is the first round of changes to facilitate a simpler way of managing secrets by adding a new route and secret handler to be implemented in each respective provider.

Motivation and Context

From proposal #807

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

How Has This Been Tested?

Not fully tested yet, only manual integration tests done in Vagrant/Docker Swarm environment. TODO: add integration tests in pre-defined location

AFAICT: This feature introduces a new handler for the providers to implement CLUD on secrets: faas-netes,swarm,nomad, faas-provider? faas-cli functionality also needs to be implemented (another PR outside of this scope)

Types of changes

  • New feature (non-breaking change which adds functionality)

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.

acornies added some commits Nov 17, 2018

Initial support for secrets in gw:
- added SecretHandler type
- added discussed system/secret endpoint with appropriate http verbs

Signed-off-by: Andrew Cornies <acornies@gmail.com>
Secrets iteration:
- added delete http verb to system/secrets
- added secrets request type
- added vagrant env provisioned by existing deploy_stack.sh

Signed-off-by: Andrew Cornies <acornies@gmail.com>

@derek derek bot added the new-contributor label Nov 19, 2018

@acornies acornies changed the title Feature/secret management to facilitate proposal #807 [WIP] - Feature/secret management to facilitate proposal #807 Nov 19, 2018

Show resolved Hide resolved Vagrantfile Outdated
Show resolved Hide resolved gateway/requests/requests.go Outdated
Updates from PR comments:
- moved Vagrantfile to contrib dir
- gave secret request type more thought

Signed-off-by: Andrew Cornies <acornies@gmail.com>

@acornies acornies force-pushed the acornies:feature/secret_management branch from 716f56e to caea8a4 Nov 21, 2018

Show resolved Hide resolved gateway/requests/requests.go Outdated
type Secret struct {
Name string `json:"name"`
Value string `json:"value"` // write-only
Annotations *map[string]string `json:"annotations"`

This comment has been minimized.

@alexellis

alexellis Nov 24, 2018

Member

I think annotations or metadata will be useful and necessary. Do both Swarm and Kubernetes support adding this to secrets?

This comment has been minimized.

@acornies

acornies Nov 26, 2018

Member

Looking at this again, perhaps we should move it to the CreateSecretRequest struct instead?

This comment has been minimized.

@LucasRoesler

LucasRoesler Nov 27, 2018

Member

Why only on the Create? Won't we want to be able to read and display those annotations?

Is there a reason to use a pointer here? maps behave like pointers already, e.g.

var m map[string]string
m = nil
fmt.Printf("%d", len(m))

is valid and I think when you json unmarshal and the value is empty or omitted, the map will be set nil as well.

This comment has been minimized.

@alexellis

alexellis Nov 27, 2018

Member

It might be due to copy/paste from other areas of code? We should add omitempty on some of these properties too.

This comment has been minimized.

@acornies

acornies Nov 27, 2018

Member

How about simply a SecretRequest type? Members could be:

Secret Secret `json:"secret"`
Annotations map[string]string `json:"annotations"`

POST, PUT are pretty straightforward. GET would then return []SecretRequest instead, omitting certain values ;).

This comment has been minimized.

@LucasRoesler

LucasRoesler Nov 28, 2018

Member

I would highly recommend separating the Request struct from the Response struct. It becomes much easier to document and to extend in the future.

Show resolved Hide resolved gateway/requests/requests.go Outdated
@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Nov 27, 2018

This should have a update to the swagger spec in the api-doc folder, right?

One thing that I thought about while reviewing this is that the List endpoint might want to support filtering. There is no way to specify this in the go code here because those values would live in the GET parameters. But if we are keeping the swagger spec up-to-date, that spec should include a definition of the filter parameters that should be supported by the provider.

@acornies

This comment has been minimized.

Copy link
Member

acornies commented Nov 28, 2018

My next pass should include stuff discussed here, plus some outstanding things on the TODO list. Standby...

Updated secret types based on PR feedback:
- SecretInfo type
- ListSecretsResponse
- Move Annotations to SecretInfo
- update swagger api docs

Signed-off-by: Andrew Cornies <acornies@gmail.com>
@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Dec 4, 2018

@acornies I saw the latest push, please let me know when you feel it is ready for another review

@acornies

This comment has been minimized.

Copy link
Member

acornies commented Dec 5, 2018

@LucasRoesler thanks - I still need to add integration tests, and do more testing with the faas-swarm provider to start.

My style is to push smaller incremental updates. I can hold off the PR until it's more complete next time.

@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Dec 6, 2018

I don't mean to encourage large commits, in fact I prefer small commits. Just wanted to remind you that I am ready to review as soon again as soon as you are, don't hesitate to ping me when it is ready.

@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Dec 19, 2018

Hi @acornies , how are things going with the secrets API? If there is anything that can help with or if you have any questions I can help answer, please let me know.

@acornies

This comment has been minimized.

Copy link
Member

acornies commented Dec 19, 2018

Thanks @lucasalexander will do. I had no time in November, early Dec. However, now that things have died down a bit, I expect to make more progress. Might need help with k8s provider, but we'll see how it goes.

@@ -8,3 +8,4 @@ certifier
.editorconfig
/contrib/staging/
**/*.sha256
.vagrant

This comment has been minimized.

@alexellis

alexellis Jan 4, 2019

Member

This should probably be in the developer's .gitignore?

@alexellis alexellis referenced this pull request Jan 4, 2019

Merged

Review/merge Secrets CRUD #1012

1 of 11 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment