Add `dce auth`; Refactor Config file mgmt; Add integration tests #38

eschwartz · 2019-11-21T21:58:25Z

Stuff going on here

Add dce auth command
- pops you out to a browser at the /auth endpoint that @joshmarsh is working on
- see Authentication with Cognito #33
Remove dce.yml configs we aren't using
Configure a AWS creds provider that will load creds from the auth token, but fallback to env vars or the ~/.aws/credentials file
Run dce auth before any command, if we don't have any valid creds in our chain
Refactor how to load and save our config YAML, so it's easier to work with from inside multiple commands
Remove viper dependency (wasn't really being used)
Some other cleanup/refactor around init logic, to get this all wired up properly
- I'll try to point these out in the code
New setup for integration tests
- These test call our cobra CLI programmatically. For some reasons:
  - Lets us use debugger to run through tests
  - Don't need to rebuild for every test run
  - Can still use mocks at boundaries (eg. mock out web browser call)
- I'm hoping this new testing pattern makes it easier to write tests for CLI commands going forward.

joshmarsh · 2019-11-21T23:47:32Z

cmd/root.go

+		isAuthCurrentCommand := len(os.Args) == 2 && os.Args[1] == authCmd.Name()
+		if !hasValidCreds && !isAuthCurrentCommand {
+			log.Print("No valid DCE credentials found")
+			service.Authenticate()


eschwartz · 2019-11-22T22:24:15Z

.gitignore

+junit-report/
+
+dce-cli


ignore the compiled binary, from running make build

eschwartz · 2019-11-22T22:24:52Z

cmd/accounts.go

@@ -33,7 +33,7 @@ var accountsDescribeCmd = &cobra.Command{
 	Short: "describe an account",
 	Args:  cobra.ExactValidArgs(1),
 	Run: func(cmd *cobra.Command, args []string) {
-		service.GetAccount(args[0])
+		Service.GetAccount(args[0])


I've exposed this global service objects. This is the only way we inject mocks into these service containers (at least without a bigger refactor to how we do DI)

eschwartz · 2019-11-22T22:25:19Z

cmd/auth.go

 func init() {
 	RootCmd.AddCommand(authCmd)
-	authCmd.Flags().StringVarP(&authUrl, "url-override", "u", "", "Override the DCE login url")


no longer used (auth endpoint is always at <dce-api>/auth)

eschwartz · 2019-11-22T22:25:56Z

cmd/auth.go

-	Run: func(cmd *cobra.Command, args []string) {
-		service.Authenticate(authUrl)
+	RunE: func(cmd *cobra.Command, args []string) error {
+		return Service.Authenticate()


I'd like to move towards CLI commands returning errors.
This allows us to make assertions against these errors in integration tests

eschwartz · 2019-11-22T22:26:28Z

cmd/init.go

@@ -12,6 +12,6 @@ var initCmd = &cobra.Command{
 	Use:   "init",
 	Short: "First time DCE cli setup. Creates config file at ~/.dce.yaml",
 	Run: func(cmd *cobra.Command, args []string) {
-		service.InitializeDCE(cfgFile)
+		Service.InitializeDCE()


The location of the config file is now globally configured within our FileSystemer object.
This provides a common path for services to access the config.

eschwartz · 2019-11-22T22:27:09Z

cmd/root.go


 func init() {
-	cobra.OnInitialize(initObservation, initConfig, initUtil, initService)


Moved initialization to PreRun hook, just because the init functions don't return errors, which makes testing them difficult.

eschwartz · 2019-11-22T22:27:53Z

cmd/root.go

+	// If config file does not exist,
+	// run the `dce init` command
+	if !fsUtil.IsExistingFile(cfgFile) {
+		if cmd.Name() != initCmd.Name() {


I would like to actually just run dce init here, similar to how to do a hook to verify dce auth.
I was just running into some circular reasoning around initialization.

eschwartz · 2019-11-22T22:28:18Z

cmd/root.go

+	return serialized, nil
+}
+
+func onInit(cmd *cobra.Command, args []string) error {


most of this logic was just moved from the original initConfig function

eschwartz · 2019-11-22T22:28:39Z

configs/config.go

@@ -2,17 +2,17 @@ package configs

 // Root contains config
 type Root struct {
-	System struct {


system.auth.loginurl is no longer used

eschwartz · 2019-11-22T22:29:10Z

go.mod

@@ -19,15 +19,18 @@ replace github.com/ugorji/go v1.1.4 => github.com/ugorji/go v0.0.0-2018120416352

 require (
 	github.com/aws/aws-sdk-go v1.25.16
-	github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e
+	github.com/coreos/bbolt v1.3.2 // indirect


removed viper.
Added ptr (tiny utility for creating pointers)

eschwartz · 2019-11-22T22:29:59Z

internal/util/api.go

+	token       *string
+}
+
+func NewAPIClient(input *NewAPIClientInput) *operations.Client {


Refactored into a constructor method.
Some new logic being dealt with here, around how creds are passed in.

eschwartz · 2019-11-22T22:30:34Z

internal/util/filesystem.go

+// WriteConfig writes the Config objects as YAML
+// to the config file location (dce.yml)
+func (u *FileSystemUtil) WriteConfig() error {
+	return u.writeToYAMLFile(u.ConfigFile, u.Config)


these are replacing what viper was doing,
but in a way that's easily exposed to all our services

eschwartz · 2019-11-22T22:30:53Z

internal/util/filesystem.go

@@ -87,28 +103,28 @@ func (s *FileSystemUtil) MvToTempDir(prefix string) (string, string) {
 	return destinationDir, originDir
 }

-func (s *FileSystemUtil) RemoveAll(path string) {
+func (u *FileSystemUtil) RemoveAll(path string) {


just standardizing the var name for receivers (my IDE was mad at me about it)

eschwartz · 2019-11-22T22:31:25Z

internal/util/session.go

+	// First, we'll check for credentials in the
+	// dce.yaml's `api.token` config.
+	// then we'll use AWS's standard chain (env vars, ~/aws/credentials file)
+	creds := credentials.NewChainCredentials([]credentials.Provider{


This is how we're setting up the AWS credentials chain, which allows us to either use the configured API token (from cognito), or regular IAM creds

eschwartz · 2019-11-22T22:32:03Z

internal/util/util.go

-	var session = session.New(&aws.Config{
-		Region:      config.Region,
-	})
+	var awsSession *session.Session


Setting up a common session object, using our new creds chain,
so that both the DCE API and AWS services are using the same creds

eschwartz · 2019-11-22T22:34:23Z

cmd/root.go

 }

 // Execute adds all child commands to the root command and sets flags appropriately.
 func Execute() {
 	if err := RootCmd.Execute(); err != nil {
-		fmt.Println(err)


this was actually printing error messages twice (I guess cobra prints them for us?)

@joshmarsh

Stuff going on here - Add `dce auth` command - pops you out to a browser at the `/auth` endpoint that @joshmarsh is working on - see #33 - Remove dce.yml configs we aren't using - Configure a AWS creds provider that will load creds from the auth token, but fallback to env vars or the `~/.aws/credentials` file - Run `dce auth` before any command, if we don't have any valid creds in our chain - Refactor how to load and save our config YAML, so it's easier to work with from inside multiple commands - Remove viper dependency (wasn't really being used) - Some other cleanup/refactor around init logic, to get this all wired up properly - I'll try to point these out in the code - New setup for integration tests - These test call our cobra CLI programmatically. For some reasons: - Lets us use debugger to run through tests - Don't need to rebuild for every test run - Can still use mocks at boundaries (eg. mock out web browser call) - I'm hoping this new testing pattern makes it easier to write tests for CLI commands going forward.

eschwartz · 2019-11-22T22:35:06Z

cmd/root.go

 	serialized = []byte(entry.Message)
-	if err != nil {
-		return nil, fmt.Errorf("Failed to marshal fields to JSON, %v", err)
+	return serialized, nil


this was breaking tests (we're inspecting the logger in tests, to do assertions against CLI outputs)

eschwartz · 2019-11-22T22:35:54Z

internal/util/util.go

-	if config.API.Host != nil {
-		initializedApiClient = apiUtil.InitApiClient()
+	var apiClient APIer
+	if config.API.Host != nil && config.API.BasePath != nil {


Fixed some logic here, that would break if the dce.yml didn't have API configs

eschwartz · 2019-11-22T22:36:48Z

pkg/service/init.go

+	s.promptUserForConfig(s.Config)
+
+	// Write the config to dce.yml
+	err := s.Util.WriteConfig()


using our new common config file mgmt methods

eschwartz · 2019-11-22T22:37:34Z

pkg/service/init.go

-	newConfig.API.Host = s.Util.PromptBasic("Host name of the DCE API (example: abcde12345.execute-api.us-east-1.amazonaws.com)", nil)
-	newConfig.API.BasePath = s.Util.PromptBasic("Base path of the DCE API (example: /apigw-stage-name)", nil)
-
-	newConfig.GithubToken = s.Util.PromptBasic("Github token used to download releases from github (Leave blank to use GITHUB_TOKEN env variable)", nil)


I want to remove prompts for values that aren't required.
I'd rather keep my github token in an env var, then have to keep it in sync with the dce.yml file.
So I don't want dce init to force me to enter a value here.

eschwartz · 2019-11-22T22:37:51Z

pkg/service/init.go

-
-	newConfig.GithubToken = s.Util.PromptBasic("Github token used to download releases from github (Leave blank to use GITHUB_TOKEN env variable)", nil)
-	return &newConfig
+	if config.API.Host == nil {


If you run dce init a second time, and everythings already configured, it will just be a no-op

eschwartz · 2019-11-22T22:38:12Z

tests/integration/auth_test.go

+	"testing"
+)
+
+func TestAuthCommand(t *testing.T) {


Check out the new integration tests

eschwartz · 2019-11-22T22:38:30Z

tests/integration/auth_test.go

+			})
+
+			// Mock Weber.OpenURL()
+			mockWeber := &mocks.Weber{}


it's an integration test, but we can still mock at useful boundaries (like, for opening web pages)

eschwartz · 2019-11-22T22:38:58Z

tests/integration/auth_test.go

+			mockWeber.On("OpenURL", "https://dce.example.com/api/auth")
+
+			// Enter the API Token (IRL, would be provided by the web page)
+			cli.AnswerBasic("Enter API Token: ", "my-api-token")


I wrote a stub Prompter, that will allow you to make assertions against CLI prompts, and provide answers.

eschwartz · 2019-11-22T22:39:26Z

tests/integration/auth_test.go

+			cli.AssertAllPrompts()
+			mockWeber.AssertExpectations(t)
+
+			output := cli.Output()


cli.Output() grabs the logs sent to our logger

eschwartz · 2019-11-22T22:39:53Z

tests/integration/init_test.go

+
+func TestInitCommand(t *testing.T) {
+
+	t.Run("GIVEN custom config path flag is provided", func(t *testing.T) {


These were all migrated from being functional tests (as per a conversation with @joshmarsh )

eschwartz · 2019-11-22T22:41:27Z

tests/integration/util.go

+
+func NewCLITest(t *testing.T) *cliTest {
+	// Reset globals, so they don't leak between tests
+	cmd.Config = &configs.Root{}


A bunch of bootstrapping logic here for CLI integration tests.

Most of the complexity here comes from how we're using globals for dependency management,
and that these globals aren't initialized until you execute a command.
This makes it kind of tricky to inject dependencies into our command runner.

We might want to reconsider how we're setup with dependency injection to make this
kind of think easier, but that can be for another day...

eschwartz · 2019-11-22T22:42:10Z

tests/integration/util.go

+// cli.Inject(func(input *injectorInput) {
+//		input.util.Weber = &mocks.Weber{}
+// })
+func (test *cliTest) Inject(f injector) {


Note this Inject() method, which you'll need to use to inject mocks into global services
(because the global service references are nil until the command is run)

eschwartz · 2019-11-22T22:42:23Z

tests/unit/auth_test.go

@@ -1,33 +0,0 @@
-package unit


Now covered in integration tests

eschwartz · 2019-11-22T22:42:32Z

tests/unit/init_test.go

@@ -1,101 +0,0 @@
-package unit


now covered in integration tests

joshmarsh reviewed Nov 21, 2019

View reviewed changes

eschwartz force-pushed the dce-auth branch from 643aef2 to 7eb999e Compare November 22, 2019 22:17