Pattern for defining and loading application configuration by zwass · Pull Request #149 · kolide/fleet

zwass · 2016-09-09T15:50:18Z

This provides a new pattern for defining and loading configuration for the Kolide server. With this PR, configuration is stored in the config.KolideConfig struct, and loaded through config.Manager. Refer to the patterns in config/config.go and cli/prepare.go to see how configuration is defined and used.

A follow-up PR will work on removing further references to viper throughout the codebase, instead preferring to access configuration through the strongly typed KolideConfig.

zwass · 2016-09-09T15:51:23Z

-}
-
 func Launch() {
+	rootCmd := createRootCmd()


Notice that globals and init functions are being killed off in favor of doing more work here in Launch.

zwass · 2016-09-09T15:56:09Z

This is what the help text looks like with this approach:

$ ./build/kolide serve --help
Launch the kolide server

Use kolide serve to run the main HTTPS server. The Kolide server bundles
together all static assets and dependent libraries into a statically linked go
binary (which you're executing right now). Use the options below to customize
the way that the kolide server works.

Usage:
  kolide serve [flags]

Global Flags:
      --auth.bcrypt_cost int     Env: KOLIDE_AUTH_BCRYPT_COST (default 12)
      --auth.jwt_key string      Env: KOLIDE_AUTH_JWT_KEY (default "CHANGEME")
      --auth.salt_key_size int   Env: KOLIDE_AUTH_SALT_KEY_SIZE (default 24)
      --config string            Path to a configuration file
      --mysql.address string     Env: KOLIDE_MYSQL_ADDRESS (default "localhost:3306")
      --mysql.database string    Env: KOLIDE_MYSQL_DATABASE (default "kolide")
      --mysql.password string    Env: KOLIDE_MYSQL_PASSWORD (default "kolide")
      --mysql.username string    Env: KOLIDE_MYSQL_USERNAME (default "kolide")
      --server.address string    Env: KOLIDE_SERVER_ADDRESS (default "localhost:8080")
      --server.cert string       Env: KOLIDE_SERVER_CERT (default "./tools/osquery/kolide.crt")
      --server.key string        Env: KOLIDE_SERVER_KEY (default "./tools/osquery/kolide.key")

zwass · 2016-09-09T16:02:41Z

+}
+
+// LoadConfig will load the config variables into a fully initialized AppConfig struct
+func (man ConfigManager) LoadConfig() AppConfig {


It could also be possible to add struct tags above and then use reflection to do this and AttachConfigs without requiring manual editing of these functions. (Note though that code that needs to both access the struct field tags and set the struct field values is going to end up pretty messy because of how reflect.StructField and reflect.Value behave.)

groob · 2016-09-09T16:05:08Z

Without diving into the code, the CLI interface is exactly how I envision a perfect server CLI ui :)

groob · 2016-09-09T16:07:22Z

+)
+
+type AppConfig struct {
+	Mysql struct {


Each one of these should be their own type, and embed them in AppConfig.
It's easier to read/reason about and also work with.

@groob You mean like

type mysqlConfig struct { ... } type AppConfig struct { Mysql mysqlConfig }

yeah?

Yeah, that's what I mean.

groob · 2016-09-09T16:36:58Z

@zwass I think this is going in a good direction overall.

Before completing this PR, would you mind opening the package in GoDoc and seeing what the documentation looks like? It would be great if the package API only exported functions/methods which were absolutely necessary. I'd like to be able to learn how to add/remove commands to Kolide without reading the source, and that would be good for on-boarding new developers as well.

marpaia · 2016-09-09T16:57:48Z

+				viper.GetString("mysql.password"),
+				viper.GetString("mysql.address"),
+				viper.GetString("mysql.database"),
+			)


Shouldn't the config be parsed before this? And then this should use the config struct with strong types and sane defaults?

This is old code that I haven't killed yet. We would load the config through the ConfigManager and access it with strong types from there (like https://github.com/kolide/kolide-ose/pull/149/files#diff-a4c2ccfae6e99bf0b872bdb97ae6b0d7R33).

marpaia · 2016-09-09T17:03:01Z

I don't often see applications mixing periods and underscores so freely in CLI flags (like --auth.bcrypt_cost). Are we all cool with how that looks/feels?

marpaia · 2016-09-09T17:04:13Z

@@ -69,4 +79,9 @@ Available Configurations:
      debug              (bool)    (KOLIDE_LOGGING_DEBUG)
      disable_banner     (bool)    (KOLIDE_LOGGING_DISABLE_BANNER)


Shouldn't all of this be RM'd now that the flags are being specified in help text some other way?

Yes. There's tons of cleanup that I didn't want to do until I got the go ahead from you guys on the approach.

zwass · 2016-09-09T17:04:14Z

That would be easy to change if we want to replace . with _.

groob · 2016-09-09T17:07:05Z

I've seen this in multiple applications. Here's an example from prometheus

docker run -it --rm prom/prometheus --help
usage: prometheus [<args>]


   -version false
      Print version information.

   -experimental.storage.remote.address
      The address of the remote server to send samples to. None, if
      empty. EXPERIMENTAL.

   -config.file "prometheus.yml"
      Prometheus configuration file name.

 == ALERTMANAGER ==

   -alertmanager.notification-queue-capacity 10000
      The capacity of the queue for pending alert manager notifications.

   -alertmanager.timeout 10s
      Alert manager HTTP API timeout.

   -alertmanager.url
      Comma-separated list of Alertmanager URLs to send notifications to.

IMO it's not bad.
Another common way I've seen is to have - as a separator
ex --auth-bcrypt-cost=

Either style is fine as long as it's consistent.

marpaia · 2016-09-09T17:13:49Z

Yeah, idc if we do all dots or all underscores or all dashes, but I think we should do all of one thing.

groob · 2016-09-09T17:15:05Z

All dash would be the logical choice then, since you already need the dash to specify the flag.

zwass · 2016-09-10T00:05:09Z

@marpaia and @groob This has been cleaned up substantially to pretty much how I would imagine merging it. There is still much work to be done to pass the KolideConfig struct into the places where it is needed and remove calls to viper throughout the codebase. At this point I would prefer to finalize and merge this PR, and then make a separate PR that achieves that. I believe that everything should be working as before, but the existing use of configs is inconsistent and there is a possibility I'm breaking things here until it is updated to be consistent.

Help text:

$ ./build/kolide
osquery management and orchestration

Configurable Options:

Options may be supplied in a yaml configuration file or via environment
variables. You only need to define the configuration values for which you
wish to override the default value.

Usage:
  kolide [command]

Available Commands:
  config_dump Dump the parsed configuration in yaml format
  prepare     Subcommands for initializing kolide infrastructure
  serve       Launch the kolide server

Flags:
      --app_web_address string           Env: KOLIDE_APP_WEB_ADDRESS (default "0.0.0.0:8080")
      --auth_bcrypt_cost int             Env: KOLIDE_AUTH_BCRYPT_COST (default 12)
      --auth_jwt_key string              Env: KOLIDE_AUTH_JWT_KEY (default "CHANGEME")
      --auth_salt_key_size int           Env: KOLIDE_AUTH_SALT_KEY_SIZE (default 24)
      --config string                    Path to a configuration file
  -h, --help                             help for kolide
      --logging_debug                    Env: KOLIDE_LOGGING_DEBUG
      --logging_disable_banner           Env: KOLIDE_LOGGING_DISABLE_BANNER
      --mysql_address string             Env: KOLIDE_MYSQL_ADDRESS (default "localhost:3306")
      --mysql_database string            Env: KOLIDE_MYSQL_DATABASE (default "kolide")
      --mysql_password string            Env: KOLIDE_MYSQL_PASSWORD (default "kolide")
      --mysql_username string            Env: KOLIDE_MYSQL_USERNAME (default "kolide")
      --osquery_enroll_secret string     Env: KOLIDE_OSQUERY_ENROLL_SECRET
      --osquery_node_key_size int        Env: KOLIDE_OSQUERY_NODE_KEY_SIZE (default 24)
      --osquery_result_log_file string   Env: KOLIDE_OSQUERY_RESULT_LOG_FILE (default "/tmp/osquery_result")
      --osquery_status_log_file string   Env: KOLIDE_OSQUERY_STATUS_LOG_FILE (default "/tmp/osquery_status")
      --server_address string            Env: KOLIDE_SERVER_ADDRESS (default "0.0.0.0:8080")
      --server_cert string               Env: KOLIDE_SERVER_CERT (default "./tools/osquery/kolide.crt")
      --server_key string                Env: KOLIDE_SERVER_KEY (default "./tools/osquery/kolide.key")
      --session_cookie_name string       Env: KOLIDE_SESSION_COOKIE_NAME (default "KolideSession")
      --session_expiration_seconds int   Env: KOLIDE_SESSION_EXPIRATION_SECONDS (default 7776000)
      --session_key_size int             Env: KOLIDE_SESSION_KEY_SIZE (default 64)
      --smtp_password string             Env: KOLIDE_SMTP_PASSWORD
      --smtp_pool_connections int        Env: KOLIDE_SMTP_POOL_CONNECTIONS (default 4)
      --smtp_server string               Env: KOLIDE_SMTP_SERVER
      --smtp_token_key_size int          Env: KOLIDE_SMTP_TOKEN_KEY_SIZE (default 24)
      --smtp_username string             Env: KOLIDE_SMTP_USERNAME

Use "kolide [command] --help" for more information about a command.

Godoc:

PACKAGE DOCUMENTATION

package config
    import "github.com/kolide/kolide-ose/config"


FUNCTIONS

func InitConfig(command *cobra.Command) func()
    InitConfig handles the loading of the config file. It should only be
    used outside this package to be hooked into the cobra.OnInitialize.

TYPES

type AppConfig struct {
    WebAddress string
}
    AppConfig defines configs related to HTTP

type AuthConfig struct {
    JwtKey      string
    BcryptCost  int
    SaltKeySize int
}
    AuthConfig defines configs related to user authorization

type ConfigManager struct {
    // contains filtered or unexported fields
}
    ConfigManager manages the addition and retrieval of config values for
    Kolide configs. It's only public API method is LoadConfig, which will
    return the populated KolideConfig struct.

func NewConfigManager(command *cobra.Command) ConfigManager
    NewConfigManager initializes a ConfigManager wrapping the provided cobra
    command. All config flags will be attached to that command (and
    inherited by the subcommands). Typically this should be called just
    once, with the root command.

func (man ConfigManager) LoadConfig() KolideConfig
    LoadConfig will load the config variables into a fully initialized
    AppConfig struct

type KolideConfig struct {
    Mysql   MysqlConfig
    Server  ServerConfig
    Auth    AuthConfig
    App     AppConfig
    Smtp    SmtpConfig
    Session SessionConfig
    Osquery OsqueryConfig
    Logging LoggingConfig
}
    KolideConfig stores the application configuration. Each subcategory is
    broken up into it's own struct, defined above. When editing any of these
    structs, ConfigManager.addConfigs and ConfigManager.LoadConfig should be
    updated to set and retrieve the configurations as appropriate.

type LoggingConfig struct {
    Debug         bool
    DisableBanner bool
}
    LoggingConfig defines configs related to logging

type MysqlConfig struct {
    Address  string
    Username string
    Password string
    Database string
}
    MysqlConfig defines configs related to MySQL

type OsqueryConfig struct {
    EnrollSecret  string
    NodeKeySize   int
    StatusLogFile string
    ResultLogFile string
}
    OsqueryConfig defines configs related to osquery

type ServerConfig struct {
    Address string
    Cert    string
    Key     string
}
    ServerConfig defines configs related to the Kolide server

type SessionConfig struct {
    KeySize           int
    ExpirationSeconds int
    CookieName        string
}
    SessionConfig defines configs related to user sessions

type SmtpConfig struct {
    Server          string
    Username        string
    Password        string
    PoolConnections int
    TokenKeySize    int
}
    SmtpConfig defines configs related to SMTP email

groob · 2016-09-10T00:09:16Z

@zwass, looking at GoDoc, it's not immediately clear what the difference/precedence of InitConfig() vs LoadConfig is.

Can we get rid of InitConfig or rename it somehow?

groob · 2016-09-10T00:12:28Z

victor@kl ~/g/s/g/k/kolide-ose> kolide-ose serve
INFO[0000] Using config file: /Users/victor/go/src/github.com/kolide/kolide-ose/tools/app/kolide.yaml
ts=2016-09-10T00:11:22Z transport=http address=:8080 msg=listening```

Is there a reason the config file is used if I didn't specify one explicitly?

zwass · 2016-09-10T00:16:11Z

To your first question: Good point, let me try to clean that up. They may be able to be merged.

Next question: This is behavior that I inherited from the existing config stuff. See https://github.com/kolide/kolide-ose/pull/149/files#diff-3baf47c64847a8fb8aaa8cc2e088513bR307 for the implementation. We can remove it if it does not make sense.

groob · 2016-09-10T00:18:53Z

IMO it's more confusing to have the config file loaded when flags/env variables have precedence.
It'd make more sense to have the config file only get loaded if it's explicitly specified.

@marpaia thoughts?

zwass · 2016-09-10T00:20:31Z

I'm with @groob on that matter, but not a strong opinion.

groob · 2016-09-10T00:22:47Z

@zwass would you mind addressing these golint comments as well.
The SMTP, URL, ID and other things like that should always be capitalized.
Full list of golint initialisms here https://github.com/golang/lint/blob/master/lint.go#L730

The config.ConfigManager vs config.Manager is not a big deal, but IMO the linter suggestion does sound nicer.

zwass · 2016-09-10T00:26:13Z

Good catch on those. We should add golint to make lint.

groob · 2016-09-10T00:29:06Z

+			logrus.Fatalf("Error reading config file: %s", viper.ConfigFileUsed())
+		}
+
+		logrus.Info("Using config file: ", viper.ConfigFileUsed())


A few points here:
First, Could we switch this to the go-kit/logger? We should standardize on a logger, and IMO the go-kit logger is a bit nicer than logrus.

Second, this is still a global/implicit dependency. Just like all the other dependencies which are passed down to all the packages, the logger should be explicit. Perhaps change InitConfig to take logger as a parameter?

Agreed on both points. We should definitely be standardizing logging throughout the app.

However, what do you think of doing these particular log lines directly to std(err|out) (viafmt)? At this point we haven't even looked at the configs for logging so it seems inappropriate to actually be initializing a logger and using that.

fmt sounds ok/better here.

groob · 2016-09-10T00:31:48Z

The rest LGTM!

marpaia · 2016-09-10T13:52:03Z

-	prepareCmd.AddCommand(dbCmd)
-	rootCmd.AddCommand(prepareCmd)
-}
+func createPrepareCmd(confManager config.Manager) *cobra.Command {


SUPER nit: can we change this param to be configManager? I feel like, if we pay the price of just two extra characters, we can just use the clearer, full words. Seems like the juice is worth the proverbial squeeze.

Yeah I'm fine with that. It's not really a great savings.

marpaia · 2016-09-10T14:00:37Z

I didn't know you could get text based godoc! That's cool :)

marpaia · 2016-09-10T14:03:20Z

So the original reason why the example config was loaded was because we wanted there to be defaults which may or may not have been different than the config values required to use the development environment. For example, configuring SMTP to use MailHog and stuff.

The logic implemented that IFF we can somehow determine that you're running the binary after just building from source (ie: the relative path of the example config exists), then you're PROBABLY running the dev infra and would want to load the example config, so let's just do that automatically.

If we don't think it's important to do that anymore, we can remove it, but that's why it was there in the first place.

marpaia · 2016-09-10T14:06:43Z

There are a few places in the code where viper.Get... is called directly. Is it within the scope of this PR to update those call sites to use the new config? Either by calling the config subpackage directly or injecting something like serviceConfig? If the former, should serviceConfig be deprecated to just call config directly or take the whole config structure?

zwass · 2016-09-12T16:25:14Z

@marpaia A couple of your last comments are noticing that we are not fully taking advantage of the config struct at this point. My intention is to merge this branch as the pattern for defining/loading config, and then get to work on eliminating uses of viper anywhere outside of config.go.

This PR refactors most of the codebase to use the new config patterns implemented in #149. Now the core service keeps a copy of the KolideConfig struct, and service methods can reference the configuration in that struct when they need it. The most significant refactoring is in the sessions code, separating the business logic from the storage layer.

zwass reviewed Sep 9, 2016
View reviewed changes

groob reviewed Sep 9, 2016
View reviewed changes

marpaia reviewed Sep 9, 2016
View reviewed changes

marpaia added the In Progress label Sep 9, 2016

marpaia changed the title ~~[WIP] Initial work on improving configs~~ Initial work on improving configs Sep 9, 2016

groob reviewed Sep 10, 2016
View reviewed changes

marpaia reviewed Sep 10, 2016
View reviewed changes

zwass changed the title ~~Initial work on improving configs~~ Pattern for defining and loading application configuration Sep 12, 2016

zwass added 10 commits September 12, 2016 10:10

Initial work on improving configs

1f462ac

Add/get for string and int working

7cab3cb

Major cleanup

2fa084b

Move config_dump

5915474

Fix subconfig names

3eaec17

Stop using global viper instance

6426485

@groob comments

6918000

Remove logrus

3aca7ee

@marpaia nits

0cb830b

finish rebase

9b41b46

zwass added Core Platform and removed In Progress labels Sep 12, 2016

zwass merged commit 6cb05a5 into kolide:master Sep 12, 2016

zwass mentioned this pull request Sep 13, 2016

Refactoring for config patterns #159

Merged

groob mentioned this pull request Sep 15, 2016

Isolate configuration parsing #68

Closed

zwass deleted the config_refactor branch September 15, 2016 17:22

		@@ -69,4 +79,9 @@ Available Configurations:
		debug (bool) (KOLIDE_LOGGING_DEBUG)
		disable_banner (bool) (KOLIDE_LOGGING_DISABLE_BANNER)

Conversation

zwass commented Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zwass commented Sep 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

groob commented Sep 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

groob commented Sep 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marpaia commented Sep 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zwass commented Sep 9, 2016

Uh oh!

groob commented Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marpaia commented Sep 9, 2016

Uh oh!

groob commented Sep 9, 2016

Uh oh!

zwass commented Sep 10, 2016

Uh oh!

groob commented Sep 10, 2016

Uh oh!

groob commented Sep 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zwass commented Sep 10, 2016

Uh oh!

groob commented Sep 10, 2016

Uh oh!

zwass commented Sep 10, 2016

Uh oh!

groob commented Sep 10, 2016

Uh oh!

zwass commented Sep 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

groob commented Sep 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marpaia commented Sep 10, 2016

Uh oh!

marpaia commented Sep 10, 2016

Uh oh!

marpaia commented Sep 10, 2016

Uh oh!

zwass commented Sep 12, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

zwass commented Sep 9, 2016 •

edited

Loading

groob commented Sep 9, 2016 •

edited

Loading

groob commented Sep 10, 2016 •

edited

Loading