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

[Testing] Support Basic Auth for multiple gateways #182

Merged
merged 1 commit into from Nov 3, 2017

Conversation

Projects
None yet
9 participants
@stefanprodan
Member

stefanprodan commented Oct 22, 2017

Description

This PR adds a new command named login that allows users to register multiple gateways protected with basic auth. The credentials are being saved to $HOME/.openfaas/config.yml on Linux/macOS and %USERPROFILE%/.openfaas/config.yml on Windows. The yml extension is used to avoid overriding the openfaas-bitbar config.yaml.

CLI login command specs:

  • validates the credentials by calling list for a given gateway
  • issues a warning if the password is passed via command line and advise on using stdin
  • issues a warning if the gateway is not on HTTPS and advise on using Letsencrypt
  • creates or updates credentials for a given gateway
  • encodes the username and password with base64 for local storage
  • sets auth to basic type in the YAML auth store

Upon calling a gateway API endpoint, the config store lookups a matching gateway URL entry. If an entry is found, the username and password are being decoded from base64 and used to setup the basic auth header. All API calls that return a 401 error will point the user to the login command.

CLI login command help:

./faas-cli login -h
Log in to OpenFaaS gateway.
If no gateway is specified, the default local one will be used.

Usage:
  faas-cli login [--username USERNAME] [--password PASSWORD] [--gateway GATEWAY_URL] [flags]

Examples:
  faas-cli login -u user -p password --gateway http://localhost:8080
  cat ~/faas_pass.txt | faas-cli login -u user --password-stdin --gateway https://openfaas.mydomain.com

Flags:
      --gateway string    Gateway URI (default "http://localhost:8080")
  -h, --help              help for login
  -p, --password string   Gateway password
      --password-stdin    Reads the gateway password from stdin
  -u, --username string   Gateway username

CLI logout command help:

./faas-cli logout --help
Log out from OpenFaaS gateway.
If no gateway is specified, the default local one will be used.

Usage:
  faas-cli logout [--gateway GATEWAY_URL] [flags]

Examples:
  faas-cli logout --gateway https://openfaas.mydomain.com

Flags:
      --gateway string   Gateway URI (default "http://localhost:8080")
  -h, --help             help for logout

Motivation and Context

  • I have raised an issue to propose this change (see #178)

How Has This Been Tested?

Tested against OpenFaaS (gateway v0.6.5) and Caddy as reverse proxy with basic auth and TLS.

Gateway Docker Swarm test environments:

  • GCP Compute Engine
  • Ubuntu 16.04
  • Docker Swarm CE 17.09.0-ce and Docker Swarm EE 17.06.2-ee-3
  • Caddy 0.10.9 FOSS edition
  • Letsencrypt certificate

Gateway Kubernetes test environment:

  • Google Container Engine
  • Kubernetes 1.8.1
  • Caddy 0.10.10 FOSS edition

CLI test environments:

  • macOS Siera 10.12.6
  • Ubuntu 16.04
  • Windows 10 Pro (go 1.9.2 windows/amd64)

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.

@stefanprodan stefanprodan changed the title from Multi-tenancy basic auth support to Basic auth support for multiple gateways Oct 22, 2017

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Oct 23, 2017

Member

When we get closer to completing review please can you squash commits?

Member

alexellis commented Oct 23, 2017

When we get closer to completing review please can you squash commits?

Show outdated Hide outdated README.md Outdated
"gopkg.in/yaml.v2"
)
var (

This comment has been minimized.

@alexellis

alexellis Oct 23, 2017

Member

Out of curiosity.. is this all original work, or based upon an example?

@alexellis

alexellis Oct 23, 2017

Member

Out of curiosity.. is this all original work, or based upon an example?

This comment has been minimized.

@stefanprodan

stefanprodan Oct 23, 2017

Member

The naming convention follows docker/cli/config and the decode function is using the same escape as in docker registry store, other than that is as "original" as this little pice of code can be :)

@stefanprodan

stefanprodan Oct 23, 2017

Member

The naming convention follows docker/cli/config and the decode function is using the same escape as in docker registry store, other than that is as "original" as this little pice of code can be :)

This comment has been minimized.

@alexellis

alexellis Oct 28, 2017

Member

Can you link me to that code please? Thank you

@alexellis

alexellis Oct 28, 2017

Member

Can you link me to that code please? Thank you

This comment has been minimized.

@johnmccabe

johnmccabe Oct 30, 2017

Member

@alexellis do you still need this?

@johnmccabe

johnmccabe Oct 30, 2017

Member

@alexellis do you still need this?

This comment has been minimized.

@stefanprodan
@stefanprodan

stefanprodan Oct 30, 2017

Member
Show outdated Hide outdated proxy/auth_test.go Outdated
github.com/inconshreveable/mousetrap 76626ae9c91c4f2a10f34cad8ce83ea42c93bb75
github.com/mitchellh/go-homedir b8bc1bf767474819792c23f32d8286a45736f1c6

This comment has been minimized.

@alexellis

alexellis Oct 23, 2017

Member

Is this new dep Windows compatible, too?

@alexellis

alexellis Oct 23, 2017

Member

Is this new dep Windows compatible, too?

This comment has been minimized.

@stefanprodan

stefanprodan Oct 23, 2017

Member
Show outdated Hide outdated proxy/auth_test.go Outdated

@open-derek open-derek bot added the no-dco label Oct 23, 2017

@open-derek

This comment has been minimized.

Show comment
Hide comment
@open-derek

open-derek bot Oct 23, 2017

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.

open-derek bot commented Oct 23, 2017

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.

@open-derek open-derek bot removed the no-dco label Oct 23, 2017

@alexellis alexellis referenced this pull request Oct 23, 2017

Closed

Initial Basic Auth works #180

8 of 11 tasks complete
@stefanprodan

This comment has been minimized.

Show comment
Hide comment
@stefanprodan
Member

stefanprodan commented Oct 23, 2017

@alexellis you can squash all commits at merge, see https://github.com/blog/2141-squash-your-commits

@@ -0,0 +1,21 @@
The MIT License (MIT)

This comment has been minimized.

@nenadilic84

nenadilic84 Oct 25, 2017

Contributor

@alexellis, @stefanprodan, I don't like the fact that we checkin the code in vendor. I think I mentioned this before, but it should be that only use the vendor.conf to have the right versions, and then let the tool checkout everything before building. I would just put .gitignore in this folder.

@nenadilic84

nenadilic84 Oct 25, 2017

Contributor

@alexellis, @stefanprodan, I don't like the fact that we checkin the code in vendor. I think I mentioned this before, but it should be that only use the vendor.conf to have the right versions, and then let the tool checkout everything before building. I would just put .gitignore in this folder.

This comment has been minimized.

@alexellis

alexellis Oct 26, 2017

Member

This is how dependencies are managed on Golang projects. It may seem odd, but you can find more information by looking at Docker or Google's Go projects.

@alexellis

alexellis Oct 26, 2017

Member

This is how dependencies are managed on Golang projects. It may seem odd, but you can find more information by looking at Docker or Google's Go projects.

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Oct 26, 2017

Member

@stefanprodan I won't be altering the commits at merge time because it'll lose the sign off. If you could do that when the work is ready that would work better.

Member

alexellis commented Oct 26, 2017

@stefanprodan I won't be altering the commits at merge time because it'll lose the sign off. If you could do that when the work is ready that would work better.

@alexellis alexellis changed the title from Basic auth support for multiple gateways to [WIP] Support Basic Auth for multiple gateways Oct 26, 2017

@open-derek

This comment has been minimized.

Show comment
Hide comment
@open-derek

open-derek bot Oct 26, 2017

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.

open-derek bot commented Oct 26, 2017

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.

@open-derek open-derek bot added no-dco and removed no-dco labels Oct 26, 2017

@open-derek

This comment has been minimized.

Show comment
Hide comment
@open-derek

open-derek bot Oct 26, 2017

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.

open-derek bot commented Oct 26, 2017

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.

@open-derek open-derek bot removed the no-dco label Oct 26, 2017

@stefanprodan

This comment has been minimized.

Show comment
Hide comment
@stefanprodan

stefanprodan Oct 26, 2017

Member

Squash done and ready for merge :)

Member

stefanprodan commented Oct 26, 2017

Squash done and ready for merge :)

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Oct 28, 2017

Member

Thanks for squashing. I'll try to do a detailed code-review over the weekend. This is great timing.

I've done some testing on my local machine with basic auth on nginx and so far looks very good 👍

Member

alexellis commented Oct 28, 2017

Thanks for squashing. I'll try to do a detailed code-review over the weekend. This is great timing.

I've done some testing on my local machine with basic auth on nginx and so far looks very good 👍

@alexellis alexellis requested review from johnmccabe and alexellis Oct 28, 2017

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Oct 28, 2017

Member

@johnmccabe when you're up to it, please can you review the config changes?

Member

alexellis commented Oct 28, 2017

@johnmccabe when you're up to it, please can you review the config changes?

@johnmccabe

Looks great, just a few comments/changes.

Show outdated Hide outdated proxy/delete.go Outdated
Show outdated Hide outdated proxy/deploy.go Outdated
Show outdated Hide outdated proxy/invoke.go Outdated
Show outdated Hide outdated proxy/list.go Outdated
Show outdated Hide outdated config/file.go Outdated
Show outdated Hide outdated config/file.go Outdated
Show outdated Hide outdated proxy/invoke.go Outdated
Show outdated Hide outdated proxy/auth_test.go Outdated
}
// EnsureFile creates the root dir and config file
func EnsureFile() (string, error) {

This comment has been minimized.

@johnmccabe

johnmccabe Oct 29, 2017

Member

Would be cleaner when testing to allow the homedir to be overridden, currently you get your local ~/.openfaas/ dir polluted with test YAML.

@johnmccabe

johnmccabe Oct 29, 2017

Member

Would be cleaner when testing to allow the homedir to be overridden, currently you get your local ~/.openfaas/ dir polluted with test YAML.

This comment has been minimized.

@alexellis

alexellis Nov 1, 2017

Member

Yes - talked about this with @johnmccabe on Sunday. We should be able to override so that we can use os.Tempdir or similar for the integration tests.

As a side note about the cli tests, integration tests should perhaps be in their own area so we can run them independently.

@alexellis

alexellis Nov 1, 2017

Member

Yes - talked about this with @johnmccabe on Sunday. We should be able to override so that we can use os.Tempdir or similar for the integration tests.

As a side note about the cli tests, integration tests should perhaps be in their own area so we can run them independently.

This comment has been minimized.

@stefanprodan

stefanprodan Nov 1, 2017

Member

I've used ioutil.TempDir("", "faas-cli-file-test") take a look at the latest commit.

@stefanprodan

stefanprodan Nov 1, 2017

Member

I've used ioutil.TempDir("", "faas-cli-file-test") take a look at the latest commit.

Show outdated Hide outdated config/file_test.go Outdated
@open-derek

This comment has been minimized.

Show comment
Hide comment
@open-derek

open-derek bot Oct 29, 2017

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.

open-derek bot commented Oct 29, 2017

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.

@open-derek open-derek bot added the no-dco label Oct 29, 2017

@alexellis

Stefan thank you so much for all your work on this. It's a great addition - I can't believe how much this CLI has come on since I started it. I've added some comments for my review and I know you also need to do a small rebase too.

@stefanprodan

This comment has been minimized.

Show comment
Hide comment
@stefanprodan

stefanprodan Nov 1, 2017

Member

@alexellis I've also implemented the logout command and added tests for the store remove func. Hopefully this will get merged before running into more conflicts. I would like to keep the 2 commit separate, one for each command, if that's ok with you.

Member

stefanprodan commented Nov 1, 2017

@alexellis I've also implemented the logout command and added tests for the store remove func. Hopefully this will get merged before running into more conflicts. I would like to keep the 2 commit separate, one for each command, if that's ok with you.

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 1, 2017

Member

When we spoke on Slack we settled on logout being out of scope for this PR. I'll have to set some more time aside to review.

Member

alexellis commented Nov 1, 2017

When we spoke on Slack we settled on logout being out of scope for this PR. I'll have to set some more time aside to review.

@stefanprodan

This comment has been minimized.

Show comment
Hide comment
@stefanprodan

stefanprodan Nov 1, 2017

Member

The logout code is straight forward, 20 lines of code with 100% test coverage :)

Member

stefanprodan commented Nov 1, 2017

The logout code is straight forward, 20 lines of code with 100% test coverage :)

return err
}
password = strings.TrimSpace(string(passwordStdin))

This comment has been minimized.

@alexellis

alexellis Nov 2, 2017

Member

Do we also need to trim the --password flag data?

@alexellis

alexellis Nov 2, 2017

Member

Do we also need to trim the --password flag data?

This comment has been minimized.

@stefanprodan

stefanprodan Nov 3, 2017

Member

done in the latest commit

@stefanprodan

stefanprodan Nov 3, 2017

Member

done in the latest commit

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 2, 2017

Member

Started to do some testing on this. I found the output format of ~/.openfaas/config.yml looked a little odd:

auths:
  http://127.0.0.1:8081:
    auth: basic
    token: value
  http://other-site:8080:
    auth: basic
    token: value
  http://other-gateway:8081:
    auth: basic
    token: value

There was a comment Joe Beda made last Friday about using values for keys which seemed to make sense at the time. We have broken the rule for function names in the OpenFaaS stack YAML but it looks worse here with the URL and then the final colon on the line.

Do you have any other suggestions on improving the key for the URL?

Member

alexellis commented Nov 2, 2017

Started to do some testing on this. I found the output format of ~/.openfaas/config.yml looked a little odd:

auths:
  http://127.0.0.1:8081:
    auth: basic
    token: value
  http://other-site:8080:
    auth: basic
    token: value
  http://other-gateway:8081:
    auth: basic
    token: value

There was a comment Joe Beda made last Friday about using values for keys which seemed to make sense at the time. We have broken the rule for function names in the OpenFaaS stack YAML but it looks worse here with the URL and then the final colon on the line.

Do you have any other suggestions on improving the key for the URL?

@stefanprodan

This comment has been minimized.

Show comment
Hide comment
@stefanprodan

stefanprodan Nov 2, 2017

Member

@alexellis Regarding the config format, I am using the same format as Docker CLI uses for storing the registries auth. I don't see a problem with it, this is how a golang map is getting serialized in yaml.

Member

stefanprodan commented Nov 2, 2017

@alexellis Regarding the config format, I am using the same format as Docker CLI uses for storing the registries auth. I don't see a problem with it, this is how a golang map is getting serialized in yaml.

@@ -40,10 +41,12 @@ func DeleteFunction(gateway string, functionName string) error {
}
switch delRes.StatusCode {
case 200, 201, 202:
case http.StatusOK, http.StatusCreated, http.StatusAccepted:

This comment has been minimized.

@alexellis

alexellis Nov 2, 2017

Member

Thanks for making this better.

@alexellis

alexellis Nov 2, 2017

Member

Thanks for making this better.

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 2, 2017

Member

Have you tested thoroughly on Windows?

Member

alexellis commented Nov 2, 2017

Have you tested thoroughly on Windows?

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 2, 2017

Member

I think this format is closer to the "Kubernetes way of YAML" - https://github.com/estesp/bucketbench

name: Basic
image: alpine:latest
command: date
rootfs: /home/estesp/containers/alpine
detached: true
drivers:
  - 
   type: Docker
   threads: 5
   iterations: 15
  - 
   type: Runc
   threads: 5
   iterations: 50

I don't know how easy it is to serialize that way.

Another example (looking at the "containers")

apiVersion: apps/v1beta2 # for versions before 1.7.0 use apps/v1beta1
kind: Deployment
metadata:
  name: nginx-deployment
spec:
  replicas: 3
  template:
    metadata:
      labels:
        app: nginx
    spec:
  - name: nginx-container
    image: nginx

  - name: debian-container
    image: debian
    command: ["/bin/sh"]

Both appear to be applying an array of "map/object" in order to force this syntax.. I mean you can checkout the video from Joe. I think we should at least evaluate his advice. Around here - https://www.youtube.com/watch?v=iMzf_oWsRi0

Member

alexellis commented Nov 2, 2017

I think this format is closer to the "Kubernetes way of YAML" - https://github.com/estesp/bucketbench

name: Basic
image: alpine:latest
command: date
rootfs: /home/estesp/containers/alpine
detached: true
drivers:
  - 
   type: Docker
   threads: 5
   iterations: 15
  - 
   type: Runc
   threads: 5
   iterations: 50

I don't know how easy it is to serialize that way.

Another example (looking at the "containers")

apiVersion: apps/v1beta2 # for versions before 1.7.0 use apps/v1beta1
kind: Deployment
metadata:
  name: nginx-deployment
spec:
  replicas: 3
  template:
    metadata:
      labels:
        app: nginx
    spec:
  - name: nginx-container
    image: nginx

  - name: debian-container
    image: debian
    command: ["/bin/sh"]

Both appear to be applying an array of "map/object" in order to force this syntax.. I mean you can checkout the video from Joe. I think we should at least evaluate his advice. Around here - https://www.youtube.com/watch?v=iMzf_oWsRi0

Show outdated Hide outdated commands/login.go Outdated
@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 2, 2017

Member

Minor nit between docker login and faas-cli login:

docker login -u alex
(starts reading password from stdin)
faas-cli login -u alex
Error: password cannot be an empty string

Might be worth looking at how we do the invoke command and doing the same. Could be a follow-up PR / issue.

Member

alexellis commented Nov 2, 2017

Minor nit between docker login and faas-cli login:

docker login -u alex
(starts reading password from stdin)
faas-cli login -u alex
Error: password cannot be an empty string

Might be worth looking at how we do the invoke command and doing the same. Could be a follow-up PR / issue.

@stefanprodan

This comment has been minimized.

Show comment
Hide comment
@stefanprodan

stefanprodan Nov 2, 2017

Member

I've updated the description, the CLI unit tests and login/logout manual testing were performed on:

  • macOS Siera 10.12.6
  • Ubuntu 16.04
  • Windows 10 Pro (go 1.9.2 windows/amd64)
Member

stefanprodan commented Nov 2, 2017

I've updated the description, the CLI unit tests and login/logout manual testing were performed on:

  • macOS Siera 10.12.6
  • Ubuntu 16.04
  • Windows 10 Pro (go 1.9.2 windows/amd64)
@stefanprodan

This comment has been minimized.

Show comment
Hide comment
@stefanprodan

stefanprodan Nov 2, 2017

Member

@alexellis regarding the config, if you look at bucketbench you'll see that the drivers are an array not a map. You can't force a map to be serialized as an array, so you'll not get the -.

Member

stefanprodan commented Nov 2, 2017

@alexellis regarding the config, if you look at bucketbench you'll see that the drivers are an array not a map. You can't force a map to be serialized as an array, so you'll not get the -.

@rgee0

This comment has been minimized.

Show comment
Hide comment
@rgee0

rgee0 Nov 2, 2017

Member

My sense is that the yaml format should be formed of nested maps so that each value has a name. This enhances the readability and use ability for the user.

It’s actually not too far off, probably just needs the gateway uri to identify itself as a gateway value.

Member

rgee0 commented Nov 2, 2017

My sense is that the yaml format should be formed of nested maps so that each value has a name. This enhances the readability and use ability for the user.

It’s actually not too far off, probably just needs the gateway uri to identify itself as a gateway value.

@stefanprodan

This comment has been minimized.

Show comment
Hide comment
@stefanprodan

stefanprodan Nov 2, 2017

Member

A nested map is not an array, if @alexellis wants the config to look like the k8s one I need to change it to an array.

auths:
 - gateway: http://127.0.0.1:8081
   auth: basic
   token: value
 - gateway: http://other-site:8080
   auth: basic
   token: value

@alexellis should I rewrite the config package to have the above format?

Member

stefanprodan commented Nov 2, 2017

A nested map is not an array, if @alexellis wants the config to look like the k8s one I need to change it to an array.

auths:
 - gateway: http://127.0.0.1:8081
   auth: basic
   token: value
 - gateway: http://other-site:8080
   auth: basic
   token: value

@alexellis should I rewrite the config package to have the above format?

@johnmccabe

This comment has been minimized.

Show comment
Hide comment
@johnmccabe

johnmccabe Nov 2, 2017

Member

Regarding the file structure:

  • Is the config file intended to be human readable/editable?
    • if it is, then yes to named keys
  • if not then using a map is easier as you don’t need to iterate through the slice to find a particular gateway
Member

johnmccabe commented Nov 2, 2017

Regarding the file structure:

  • Is the config file intended to be human readable/editable?
    • if it is, then yes to named keys
  • if not then using a map is easier as you don’t need to iterate through the slice to find a particular gateway
@stefanprodan

This comment has been minimized.

Show comment
Hide comment
@stefanprodan

stefanprodan Nov 2, 2017

Member

I don't see users editing the auth data in the config file, but if the config will grow in time I think we should make it as readable as possible. Changing the code from map to array is an easy change with little overhead. @alexellis what's your take on this?

Member

stefanprodan commented Nov 2, 2017

I don't see users editing the auth data in the config file, but if the config will grow in time I think we should make it as readable as possible. Changing the code from map to array is an easy change with little overhead. @alexellis what's your take on this?

@stefanprodan

This comment has been minimized.

Show comment
Hide comment
@stefanprodan

stefanprodan Nov 3, 2017

Member

I've switched from map to array, this how it looks:

auths:
- gateway: http://35.198.149.1
  auth: basic
  token: value
- gateway: http://35.198.149.2
  auth: basic
  token: value
Member

stefanprodan commented Nov 3, 2017

I've switched from map to array, this how it looks:

auths:
- gateway: http://35.198.149.1
  auth: basic
  token: value
- gateway: http://35.198.149.2
  auth: basic
  token: value

@alexellis alexellis changed the title from [WIP] Support Basic Auth for multiple gateways to [Testing] Support Basic Auth for multiple gateways Nov 3, 2017

login/logout commands
Signed-off-by: Stefan Prodan <stefan@weave.works>
@alexellis

LGTM - thank you for bearing with us Stefan and for making this a top quality PR.

@alexellis alexellis merged commit 5a1e6cb into openfaas:master Nov 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@developius

This comment has been minimized.

Show comment
Hide comment
@developius

developius Nov 3, 2017

Contributor

This is really cool. Clearly this PR has been merged but I had a comment about using --password-stdin though - all the CLIs I've ever used use the format -p - (or --password -) to mean "grab this from stdin".
Example:

$ cat ~/faas_pass.txt | faas-cli login -u user -p - --gateway http://localhost:8080

Thoughts?

Contributor

developius commented Nov 3, 2017

This is really cool. Clearly this PR has been merged but I had a comment about using --password-stdin though - all the CLIs I've ever used use the format -p - (or --password -) to mean "grab this from stdin".
Example:

$ cat ~/faas_pass.txt | faas-cli login -u user -p - --gateway http://localhost:8080

Thoughts?

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 3, 2017

Member

What does the Docker CLI do?

Member

alexellis commented Nov 3, 2017

What does the Docker CLI do?

@developius

This comment has been minimized.

Show comment
Hide comment
@developius

developius Nov 3, 2017

Contributor

It uses --password-stdin, but that's the only occurrence I've ever seen of not using -p -

Contributor

developius commented Nov 3, 2017

It uses --password-stdin, but that's the only occurrence I've ever seen of not using -p -

@stefanprodan

This comment has been minimized.

Show comment
Hide comment
@stefanprodan

stefanprodan Nov 3, 2017

Member

@developius how will you do that with cobra?

Member

stefanprodan commented Nov 3, 2017

@developius how will you do that with cobra?

@developius

This comment has been minimized.

Show comment
Hide comment
@developius

developius Nov 3, 2017

Contributor

@stefanprodan I think you could just check to see if the password flag was set to -, and if it was do the same logic as if passwordStdin was true login.go#L61-L72

Contributor

developius commented Nov 3, 2017

@stefanprodan I think you could just check to see if the password flag was set to -, and if it was do the same logic as if passwordStdin was true login.go#L61-L72

@stealthybox

This comment has been minimized.

Show comment
Hide comment
@stealthybox

stealthybox Nov 4, 2017

- is usually a program specific alias for /dev/stdin.
( I actually quite dislike this because it overrides other shell syntax, but it's fairly standard. )

For -p the param isn't a file name.
I believe the proper flag for a - alias would be something like --password-file.

This would be useful for other things like secrets in a tmpfs.

stealthybox commented Nov 4, 2017

- is usually a program specific alias for /dev/stdin.
( I actually quite dislike this because it overrides other shell syntax, but it's fairly standard. )

For -p the param isn't a file name.
I believe the proper flag for a - alias would be something like --password-file.

This would be useful for other things like secrets in a tmpfs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment