Auth list providers #1187

br-lewis · 2018-03-27T14:46:35Z

Adds the list-providers command to the auth subcommand. It also makes some changes to how the CLI finds the config file in the new clusters/... directory structure.

that returns a struct because this will very likely be needed for other CLI functions like `auth login`

the auth name rather than the type described in the provider object (in line with the Python CLI)

bamarni · 2018-03-27T15:08:37Z

pkg/cmd/dcos.go

@@ -9,16 +9,20 @@ import (

 	"github.com/dcos/dcos-cli/pkg/config"
 	"github.com/spf13/cobra"
+	"io"


this should go in the previous import section (https://github.com/golang/go/wiki/CodeReviewComments#imports).

bamarni · 2018-03-27T15:12:23Z

pkg/cmd/dcos.go

 )

 // rootCmd represents the base command when called without any subcommands.
 var rootCmd = &cobra.Command{
 	Use: "dcos",
 }

+var dcosConfig *Cluster


we should avoid global variables in general, I know there are already some but this will be refactored. How about calling attachedCluster() at the beginning of the list-provider command into a local variable?

bamarni · 2018-03-27T15:13:30Z

pkg/cmd/dcos_auth_listproviders.go

+	providers, err := getProviders()
+	if err != nil {
+		fmt.Println(err)
+		os.Exit(1)


Instead of printing and calling exit like here or below, other commands define the RunE func field and it can return an error.

bamarni · 2018-03-27T15:17:21Z

pkg/cmd/dcos_auth_listproviders.go

+	AuthTypeOIDCImplicitFlow = "oidc-implicit-flow"
+)
+
+var jsonOutput bool


This would require having constructor-based commands rather than global variables like authListProviders, but once this is done that'd be nice to have structs for options. Similar to the pattern here : https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/auth/cani.go#L39-L53

(I'm leaving this comment for reference not necessarily to tackle in this PR).

bamarni · 2018-03-27T15:21:10Z

pkg/cmd/dcos_auth_listproviders.go

+	AuthTypeDCOSUidPasswordLDAP = "dcos-uid-password-ldap"
+	AuthTypeSAMLSpInitiated = "saml-sp-initiated"
+	AuthTypeOIDCAuthCodeFlow = "oidc-authorization-code-flow"
+	AuthTypeOIDCImplicitFlow = "oidc-implicit-flow"


JP suggests using the "login provider" semantic here, as authentication is only about sending the ACS token to DC/OS. We still have quite some docs using "auth provider" wording, but I'd rather use the wording recommended by the security team.

bamarni · 2018-03-27T15:32:21Z

pkg/cmd/dcos_auth_listproviders.go

+	"github.com/dcos/dcos-cli/pkg/httpclient"
+	"github.com/olekukonko/tablewriter"
+	"github.com/spf13/cobra"
+	"os"


imports should be sorted as mentioned before.

bamarni · 2018-03-27T15:35:18Z

pkg/cmd/dcos_auth_listproviders.go

+func init() {
+	authCmd.AddCommand(authListProvidersCmd)
+	authListProvidersCmd.Flags().BoolVar(&jsonOutput, "json", false,
+		"returns providers in json format")


There is no line-length limit in Go, we use ~110 characters as a rule of thumb, a single line is fine here.

bamarni · 2018-03-27T15:37:13Z

pkg/cmd/dcos_auth_listproviders.go

+	}
+
+	if jsonOutput {
+		// re-marshal it into json with indents added in for pretty printing


We use sentences starting with uppercase and ending with a dot. The cmd package was a quick proof of concept but we started doing this in other packages.

bamarni · 2018-03-27T15:38:16Z

pkg/cmd/dcos_auth_listproviders.go

+
+func getProviders() (*map[string]authProvider, error) {
+	var config = dcosConfig.Config
+	var client = httpclient.New(config)


httpclient has been changed recently: 3560d63

bamarni · 2018-03-27T15:46:36Z

pkg/cmd/dcos_auth_listproviders.go

+		return nil, err
+	}
+
+	return &resp, nil


nit, this is enough as callers must check for the error and not for the pointer to be nil :

err = json.NewDecoder(response.Body).Decode(&resp) return &resp, err

accurate here (oops)

to reduce usage of globals

making the config easier to swap out for testing or for the dcos_url argument that this will need to support

needing a config and update the list-providers command to allow it to take a custom cluster baseurl as an argument

br-lewis · 2018-03-28T15:20:43Z

Build failed due to a linter error (used Url instead of URL in a variable name) but why did only 1 fail?

bamarni · 2018-03-28T15:52:20Z

Build failed due to a linter error (used Url instead of URL in a variable name) but why did only 1 fail?

I added Go tests on the linux builder Jenkinsfile, I will work on creating its own status check for the new CLI.

bamarni

Left a few minor comments, in general it looks good to me.

Some things will most likely get refactored later on (extracting some parts of the command to a package, moving away from global vars).

bamarni · 2018-03-28T15:56:56Z

pkg/cmd/dcos_auth_listproviders.go

+}
+
+func listProviders(cmd *cobra.Command, args []string) error {
+	conf := attachedCluster().Config


This is unnecessary when an argument is passed, could you execute this only when no arg is passed?

bamarni · 2018-03-28T15:58:21Z

pkg/cmd/dcos_auth_listproviders.go

+	var desc string
+	switch loginType {
+	case LoginTypeDCOSUidPassword:
+		desc = "Log in using a standard DC/OS user account (username and password)"


nit: returning directly from switch cases removes the need for the lines surrounding the switch block.

bamarni · 2018-03-28T16:02:09Z

pkg/cmd/dcos_auth_listproviders.go

+	"github.com/olekukonko/tablewriter"
+	"github.com/spf13/cobra"
+
+	"github.com/dcos/dcos-cli/pkg/httpclient"


any reason for separating this import from the other github imports? they should be grouped AFAIK

specified

unnecessarily

able to find a matching description

fmt.Errorf(...)

bamarni

LGTM, thanks!

br-lewis added 4 commits March 21, 2018 10:44

first pass at implementing auth list-providers

393a290

pull the functionality to make the request out into its own function

c8f7ada

that returns a struct because this will very likely be needed for other CLI functions like `auth login`

Merge branch 'master' into auth-list-providers

db3c89f

Add verbose auth type descriptions and change the table output to use

6263db2

the auth name rather than the type described in the provider object (in line with the Python CLI)

br-lewis requested a review from bamarni March 27, 2018 14:46

bamarni reviewed Mar 27, 2018

View reviewed changes

br-lewis added 12 commits March 28, 2018 12:30

Change authtype check to a switch case block

a4097d8

ran it through gofmt and goimport to sort the imports

e7c945d

Change auth to login because it's more accurate

8efad16

swap to using shorter variable declaration syntax

3bdeaad

Sort imports for dcos.go

c3b0ea6

Change the command to use RunE and return an error instead

f61e196

Make comments proper sentences and update some copypasted comment to be

aa74c1a

accurate here (oops)

Remove dcosConfig and instead grab it when needed in authListProviders

fa4be9d

to reduce usage of globals

Make the cluster config an argument of getProviders in preparation for

e5fd923

making the config easier to swap out for testing or for the dcos_url argument that this will need to support

Merge branch 'master' into auth-list-providers

69b41ad

Merge in master to allow the httpclient to take a baseurl instead of

25c162a

needing a config and update the list-providers command to allow it to take a custom cluster baseurl as an argument

Remove unnecessary error check

5e35527

Fixed variable with Url in name to URL

21c8290

bamarni reviewed Mar 28, 2018

View reviewed changes

br-lewis added 5 commits March 29, 2018 12:02

change url selection so that the config isn't parsed unless no url was

cb2df28

specified

Put import back with other github.com imports

bd9084b

Make each switch branch return instead of assigning to a string

889bed6

unnecessarily

Change loginTypeDescription to output the given loginType if it wasn't

56fd31a

able to find a matching description

Follow linter's advice and swap errors.New(fmt.Sprintf(...)) with

4224085

fmt.Errorf(...)

bamarni approved these changes Mar 29, 2018

View reviewed changes

bamarni merged commit fe1fd6f into dcos:master Mar 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auth list providers #1187

Auth list providers #1187

br-lewis commented Mar 27, 2018

bamarni Mar 27, 2018

bamarni Mar 27, 2018

bamarni Mar 27, 2018

bamarni Mar 27, 2018

bamarni Mar 27, 2018

bamarni Mar 27, 2018

bamarni Mar 27, 2018

bamarni Mar 27, 2018

bamarni Mar 27, 2018

bamarni Mar 27, 2018

br-lewis commented Mar 28, 2018

bamarni commented Mar 28, 2018

bamarni left a comment

bamarni Mar 28, 2018

bamarni Mar 28, 2018

bamarni Mar 28, 2018

bamarni left a comment

Auth list providers #1187

Auth list providers #1187

Conversation

br-lewis commented Mar 27, 2018

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

Choose a reason for hiding this comment

br-lewis commented Mar 28, 2018

bamarni commented Mar 28, 2018

bamarni 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

bamarni left a comment

Choose a reason for hiding this comment