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 create command for secrets #580

Merged
merged 4 commits into from Jan 11, 2019

Conversation

Projects
None yet
5 participants
@viveksyngh
Copy link
Member

viveksyngh commented Jan 10, 2019

This commit adds command to create new secrets from the CLI

Signed-off-by: Vivek Singh vivekkmr45@yahoo.in

Description

Motivation and Context

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

How Has This Been Tested?

Tested on my local for docker for mac and Kubernetes

./faas-cli secret create test-secret --value=test
Creating secret: test-secret

WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
server returned unexpected status code: 500 - secrets "test-secret" already exists

./faas-cli secret create test-secret --from-file ~/Downloads/openfaas-cloud.2018-04-11.private-key.pem
Creating secret: test-secret

WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
server returned unexpected status code: 500 - secrets "test-secret" already exists

 ./faas-cli secret create of-cloud --from-file=/Users/viveks/Downloads/openfaas-cloud.2018-04-11.private-key.pem
Creating secret: of-cloud

WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
Created: 202 Accepted

./faas-cli secret create cli-test --value=test
Creating secret: cli-test

WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
Created: 202 Accepted

/faas-cli secret create of-cloud --from-file=/Users/viveks/Downloads/openfaas-cloud.2018-04-11.private-key.pem --value=test
Please either provide value or from-file for the secret

./faas-cli secret list
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.

NAME
cli-test
of-cloud
test-secret


./faas-cli secret create  --from-file=/Users/viveks/Downloads/openfaas-cloud.2018-04-11.private-key.pem
Please provide secret name

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.
Add create command for secrets
This commit adds command to create new secrets from the CLI

Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@alexellis
Copy link
Member

alexellis left a comment

Are we supporting input via stdin? Look at the login command as an example.

secretCreateCmd.Flags().StringVarP(&gateway, "gateway", "g", defaultGateway, "Gateway URL starting with http(s)://")
secretCmd.AddCommand(secretCreateCmd)

// Here you will define your flags and configuration settings.

This comment has been minimized.

@alexellis

alexellis Jan 10, 2019

Member

Let's remove this comment

@@ -0,0 +1,92 @@
// Copyright (c) OpenFaaS Author(s) 2018. All rights reserved.

This comment has been minimized.

@alexellis

alexellis Jan 10, 2019

Member

Can we put 2019?

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Jan 10, 2019

Good start. Just sent some comments.

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Jan 10, 2019

This is the API I would like to see if that's possible?

  1. So if you do:
faas-cli secret create api-key
cat ~/Downloads/derek.pem | faas-cli secret create api-key

It should read from stdin. See how this is done in faas-cli invoke as an example

  1. If you want to pass a value then do:
faas-cli secret create api-key --from-literal="04385e5c413c10ed68afb010ebe8c5dd706aa20a"
  1. And finally for files:
faas-cli secret create api-key --from-file=~/Downloads/derek.pem

@alexellis alexellis requested a review from ivanayov Jan 10, 2019

@viveksyngh

This comment has been minimized.

Copy link
Member

viveksyngh commented Jan 10, 2019

Got it .. what should be the preference if user provides more than one option, I know it is unlikely to option but just a thought ?

Add STDIN support for secret input
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@viveksyngh

This comment has been minimized.

Copy link
Member

viveksyngh commented Jan 10, 2019

./faas-cli secret create of-cloud-1
hello
Creating secret: of-cloud-1

WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
server returned unexpected status code: 500 - secrets "of-cloud-1" already exists
cat /Users/viveks/Downloads/openfaas-cloud.2018-04-11.private-key.pem | ./faas-cli secret create of-cloud-1
Creating secret: of-cloud-1

WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
Created: 202 Accepted
@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Jan 10, 2019

@viveksyngh when the cmd reads from stdin, i wonder if we should include a comment line that instructs the user to CTRL-D to indicate then end of the content? For example,

./faas-cli secret create of-cloud-1
Type CTRL-D to end input
hello
Creating secret: of-cloud-1
@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Jan 10, 2019

Yes we should, that's what I meant by looking at the invoke command originally. We have a sort of hybrid between the invoke and login commands.

Do we also need a --trim flag to be used with this command?

)

var (
secretName string

This comment has been minimized.

@leodido

leodido Jan 10, 2019

Contributor

Imho this is better declared within the runSecretCreate function (the only place where it is used).

This comment has been minimized.

@alexellis

alexellis Jan 11, 2019

Member

function or file?

This comment has been minimized.

@LucasRoesler

LucasRoesler Jan 11, 2019

Member

I agree, each of these variables should be local variables of the runSecretCreate method, similar to the gatewayAddress variable.

This can be done using this pattern, for example,

secretName := args[0]
secretValue, err := cmd.Flags().GetString("from-literal")
if err != nil {
     return err
}

This comment has been minimized.

@alexellis

alexellis Jan 11, 2019

Member

We haven't done this anywhere else, so I'm not considering this a blocker. Perhaps a nice refactoring exercise for someone?

This comment has been minimized.

@LucasRoesler

LucasRoesler Jan 11, 2019

Member

ok, sounds good, i will create a ticket

Add help message for STDIN input
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>

@viveksyngh viveksyngh force-pushed the viveksyngh:secret_create branch from f829151 to 4955ebb Jan 11, 2019

@ivanayov

This comment has been minimized.

Copy link
Member

ivanayov commented Jan 11, 2019

@viveksyngh please see #582 so not to duplicate efforts

@ivanayov
Copy link
Member

ivanayov left a comment

Added two comments.


}

if len(secretValue) == 0 && len(secretFile) == 0 {

This comment has been minimized.

@ivanayov

ivanayov Jan 11, 2019

Member

What will happen if someone passes both from literal and file?
This is possible with the flags, but we don’t cover the use-case.

This comment has been minimized.

@LucasRoesler

LucasRoesler Jan 11, 2019

Member

This seems like a good place for a switch (i would also suggest changing the name fo the variable for the from-literal flag to something like literalSecret)

var secretValue string
switch {
case len(literalSecret) > 0:
    // handle input
case len(secretFile) > 0:
   // handle parsing from the file
default:
   // handle parsing from stdin
}

This will set a clear order of precedence and also make it clear that reading directly from stdin is the default behavior

This comment has been minimized.

@alexellis

alexellis Jan 11, 2019

Member

Seems fair as a comment. Can it be done as a refactoring exercise?

This comment has been minimized.

@viveksyngh

viveksyngh Jan 11, 2019

Member

Yeah, that can be done. I had already left a comment what should be preference between different flags.

secretName = args[0]

if len(secretValue) == 0 && len(secretFile) > 0 {
secretValue, err = readSecretFromFile(secretFile)

This comment has been minimized.

@ivanayov

ivanayov Jan 11, 2019

Member

I’m not 100% sure but AFAIR in Kubernetes the file name was taken into account. Didn’t check the documentation, but remember that testing Derek was once failing because the secret file wasn’t named as shown in the docs.

This comment has been minimized.

@LucasRoesler

LucasRoesler Jan 11, 2019

Member

I am not sure I understand the comment. Reading the value form a file here in the CLI shouldn't impact how it is created in the provider.

This comment has been minimized.

@alexellis

alexellis Jan 11, 2019

Member

I think what Ivana means is that if you do --from-file - the name of the secret is usually the name of the file. Here we just allow that to be overridden

This comment has been minimized.

@viveksyngh

viveksyngh Jan 11, 2019

Member

Got it, that specific to kubernetes? I think for swarm we need to pass the secret name.

Refactor code and add some validation
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Jan 11, 2019

I just tested this locally and everything checked out

  • passing the value in multiple ways produced a validation error
  • each input method worked as expected

I tested against the openfaas-operator provider

@@ -66,7 +66,7 @@ func GetSecretList(gateway string, tlsInsecure bool) ([]schema.Secret, error) {
return results, nil
}

// RemoveSecret remove a secret via the OpenFaaS API by name
//RemoveSecret remove a secret via the OpenFaaS API by name

This comment has been minimized.

@alexellis

alexellis Jan 11, 2019

Member

What tool is removing the space between the comment and //? I use vscode and its built-in formatters that keep putting it back.

@alexellis
Copy link
Member

alexellis left a comment

LGTM, let's create follow-up items for the feedback from Lucas and others.

@alexellis alexellis merged commit 70a1871 into openfaas:master Jan 11, 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