Skip to content
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

Enable environment overrides and built-in configuration defaults #963

Merged
merged 7 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ bootstrap: ## install build deps
google.golang.org/protobuf/cmd/protoc-gen-go google.golang.org/grpc/cmd/protoc-gen-go-grpc \
github.com/kyleconroy/sqlc
# Create a config.yaml if it doesn't exist
# TODO: remove this when all config is handled in internal/config
cp -n config/config.yaml.example ./config.yaml || echo "config.yaml already exists, not overwriting"
# Create keys:
mkdir -p .ssh
Expand Down
6 changes: 6 additions & 0 deletions cmd/server/app/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ var serveCmd = &cobra.Command{
if err != nil {
return fmt.Errorf("unable to read config: %w", err)
}
if cmd.Flag("dump_config").Value.String() == "true" {
log.Printf("%+v\n", cfg)
os.Exit(0)
}

ctx = logger.FromFlags(cfg.LoggingConfig).WithContext(ctx)
zerolog.Ctx(ctx).Info().Msgf("Initializing logger in level: %s", cfg.LoggingConfig.Level)
Expand Down Expand Up @@ -117,4 +121,6 @@ func init() {
}

serveCmd.Flags().String("logging", "", "Log Level")

serveCmd.Flags().Bool("dump_config", false, "Dump Config and exit")
}
10 changes: 5 additions & 5 deletions deployment/helm/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,15 @@ spec:
- "--github-client-id-file=/secrets/app/client_id"
- "--github-client-secret-file=/secrets/app/client_secret"
env:
- name: "AUTH.ACCESS_TOKEN_PRIVATE_KEY"
- name: "AUTH_ACCESS_TOKEN_PRIVATE_KEY"
value: "/secrets/auth/access_token_rsa"
- name: "AUTH.ACCESS_TOKEN_PUBLIC_KEY"
- name: "AUTH_ACCESS_TOKEN_PUBLIC_KEY"
value: "/secrets/auth/access_token_rsa.pub"
- name: "AUTH.REFRESH_TOKEN_PRIVATE_KEY"
- name: "AUTH_REFRESH_TOKEN_PRIVATE_KEY"
value: "/secrets/auth/refresh_token_rsa"
- name: "AUTH.REFRESH_TOKEN_PUBLIC_KEY"
- name: "AUTH_REFRESH_TOKEN_PUBLIC_KEY"
value: "/secrets/auth/refresh_token_rsa.pub"
- name: "AUTH.TOKEN_KEY_PASSPHRASE"
- name: "AUTH_TOKEN_KEY_PASSPHRASE"
value: "/secrets/auth/token_key_passphrase"

# ko will always specify a digest, so we don't need to worry about
Expand Down
10 changes: 5 additions & 5 deletions docker-compose.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ services:
# Use viper environment variables to set specific paths to keys;
# these values are relative paths in config.yaml, but it's not clear
# what they are relative _to_...
- AUTH.ACCESS_TOKEN_PRIVATE_KEY=/app/.ssh/access_token_rsa
- AUTH.ACCESS_TOKEN_PUBLIC_KEY=/app/.ssh/access_token_rsa.pub
- AUTH.REFRESH_TOKEN_PRIVATE_KEY=/app/.ssh/refresh_token_rsa
- AUTH.REFRESH_TOKEN_PUBLIC_KEY=/app/.ssh/refresh_token_rsa.pub
- AUTH.TOKEN_KEY=/app/.ssh/token_key_passphrase
- AUTH_ACCESS_TOKEN_PRIVATE_KEY=/app/.ssh/access_token_rsa
- AUTH_ACCESS_TOKEN_PUBLIC_KEY=/app/.ssh/access_token_rsa.pub
- AUTH_REFRESH_TOKEN_PRIVATE_KEY=/app/.ssh/refresh_token_rsa
- AUTH_REFRESH_TOKEN_PUBLIC_KEY=/app/.ssh/refresh_token_rsa.pub
- AUTH_TOKEN_KEY=/app/.ssh/token_key_passphrase
networks:
- app_net
depends_on:
Expand Down
16 changes: 8 additions & 8 deletions internal/config/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,21 @@ package config
// AuthConfig is the configuration for the auth package
type AuthConfig struct {
// AccessTokenPrivateKey is the private key used to sign the access token for authn/z
AccessTokenPrivateKey string `mapstructure:"access_token_private_key"`
AccessTokenPrivateKey string `mapstructure:"access_token_private_key" default:"./.ssh/access_token_rsa"`
// AccessTokenPublicKey is the public key used to verify the access token for authn/z
AccessTokenPublicKey string `mapstructure:"access_token_public_key"`
AccessTokenPublicKey string `mapstructure:"access_token_public_key" default:"./.ssh/access_token_rsa.pub"`
// RefreshTokenPrivateKey is the private key used to sign the refresh token for authn/z
RefreshTokenPrivateKey string `mapstructure:"refresh_token_private_key"`
RefreshTokenPrivateKey string `mapstructure:"refresh_token_private_key" default:"./.ssh/refresh_token_rsa"`
// RefreshTokenPublicKey is the public key used to verify the refresh token for authn/z
RefreshTokenPublicKey string `mapstructure:"refresh_token_public_key"`
RefreshTokenPublicKey string `mapstructure:"refresh_token_public_key" default:"./.ssh/refresh_token_rsa.pub"`
// TokenExpiry is the expiry time for the access token in seconds
TokenExpiry int64 `mapstructure:"token_expiry"`
TokenExpiry int64 `mapstructure:"token_expiry" default:"3600"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think the other fields could also use defaults?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I somehow lost just that one file in my commit. I suspect I hadn't saved the buffer.

Fixed!

// RefreshExpiry is the expiry time for the refresh token in seconds
RefreshExpiry int64 `mapstructure:"refresh_expiry"`
RefreshExpiry int64 `mapstructure:"refresh_expiry" default:"86400"`
// NoncePeriod is the period in seconds for which a nonce is valid
NoncePeriod int64 `mapstructure:"nonce_period"`
NoncePeriod int64 `mapstructure:"nonce_period" default:"3600"`
// TokenKey is the key used to store the provider's token in the database
TokenKey string `mapstructure:"token_key"`
TokenKey string `mapstructure:"token_key" default:"./.ssh/token_key_passphrase"`
}

// GetAuthConfigWithDefaults returns a AuthConfig with default values
Expand Down
82 changes: 78 additions & 4 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@
package config

import (
"fmt"
"reflect"
"strconv"
"strings"
"unicode"

"github.com/spf13/viper"
)

Expand All @@ -33,6 +39,18 @@ type Config struct {
Auth AuthConfig `mapstructure:"auth"`
}

// DefaultConfigForTest returns a configuration with all the struct defaults set,
// but no other changes.
func DefaultConfigForTest() *Config {
v := viper.New()
SetViperDefaults(v)
c, err := ReadConfigFromViper(v)
if err != nil {
panic(fmt.Sprintf("Failed to read default config: %v", err))
}
return c
}

// ReadConfigFromViper reads the configuration from the given Viper instance.
// This will return the already-parsed and validated configuration, or an error.
func ReadConfigFromViper(v *viper.Viper) (*Config, error) {
Expand All @@ -46,8 +64,64 @@ func ReadConfigFromViper(v *viper.Viper) (*Config, error) {
// SetViperDefaults sets the default values for the configuration to be picked
// up by viper
func SetViperDefaults(v *viper.Viper) {
SetLoggingViperDefaults(v)
SetTracingViperDefaults(v)
SetMetricsViperDefaults(v)
SetCryptoViperDefaults(v)
viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
setViperStructDefaults(v, "", Config{})
}

// setViperStructDefaults recursively sets the viper default values for the given struct.
//
// Per https://github.com/spf13/viper/issues/188#issuecomment-255519149, and
// https://github.com/spf13/viper/issues/761, we need to call viper.SetDefault() for each
// field in the struct to be able to use env var overrides. This also lets us use the
// struct as the source of default values, so yay?
func setViperStructDefaults(v *viper.Viper, prefix string, s any) {
structType := reflect.TypeOf(s)

for i := 0; i < structType.NumField(); i++ {
field := structType.Field(i)
if unicode.IsLower([]rune(field.Name)[0]) {
// Skip private fields
continue
}
if field.Tag.Get("mapstructure") == "" {
// Error, need a tag
panic(fmt.Sprintf("Untagged config struct field %q", field.Name))
}
valueName := strings.ToLower(prefix + field.Tag.Get("mapstructure"))

if field.Type.Kind() == reflect.Struct {
setViperStructDefaults(v, valueName+".", reflect.Zero(field.Type).Interface())
continue
}

// Extract a default value the `default` struct tag
// we don't support all value types yet, but we can add them as needed
value := field.Tag.Get("default")
defaultValue := reflect.Zero(field.Type).Interface()
var err error // We handle errors at the end of the switch
fieldType := field.Type.Kind()
//nolint:golint,exhaustive
switch fieldType {
case reflect.String:
defaultValue = value
case reflect.Int64, reflect.Int32, reflect.Int16, reflect.Int8, reflect.Int,
reflect.Uint64, reflect.Uint32, reflect.Uint16, reflect.Uint8, reflect.Uint:
defaultValue, err = strconv.Atoi(value)
case reflect.Float64:
defaultValue, err = strconv.ParseFloat(value, 64)
case reflect.Bool:
defaultValue, err = strconv.ParseBool(value)
default:
err = fmt.Errorf("unhandled type %s", fieldType)
}
if err != nil {
// This is effectively a compile-time error, so exit early
panic(fmt.Sprintf("Bad value for field %q (%s): %q", valueName, fieldType, err))
}

if err := v.BindEnv(strings.ToUpper(valueName)); err != nil {
panic(fmt.Sprintf("Failed to bind %q to env var: %v", valueName, err))
}
v.SetDefault(valueName, defaultValue)
}
}
9 changes: 9 additions & 0 deletions internal/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,12 @@ grpc_server:
require.Equal(t, "bar", cfg.GRPCServer.Host)
require.Equal(t, 5678, cfg.GRPCServer.Port)
}

func TestReadDefaultConfig(t *testing.T) {
t.Parallel()

cfg := config.DefaultConfigForTest()
require.Equal(t, "debug", cfg.LoggingConfig.Level)
require.Equal(t, "mediator", cfg.Database.Name)
require.Equal(t, "./.ssh/token_key_passphrase", cfg.Auth.TokenKey)
}
42 changes: 7 additions & 35 deletions internal/config/crypto.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,45 +15,17 @@

package config

import (
"github.com/spf13/viper"
)

const (
saltMemory = 64 * 1024
saltIterations = 3
saltParameters = 2
saltLength = 16
saltKeyLength = 32
)

// CryptoConfig is the configuration for the crypto package
type CryptoConfig struct {
Memory uint32 `mapstructure:"memory"`
Iterations uint32 `mapstructure:"iterations"`
Parallelism uint `mapstructure:"parallelism"`
SaltLength uint32 `mapstructure:"salt_length"`
KeyLength uint32 `mapstructure:"key_length"`
}

// SetCryptoViperDefaults sets the default values for the crypto configuration
// to be picked up by viper
func SetCryptoViperDefaults(v *viper.Viper) {
// set default values when not set
v.SetDefault("salt.memory", saltMemory)
v.SetDefault("salt.iterations", saltIterations)
v.SetDefault("salt.parallelism", saltParameters)
v.SetDefault("salt.salt_length", saltLength)
v.SetDefault("salt.key_length", saltKeyLength)
Memory uint32 `mapstructure:"memory" default:"65536"`
Iterations uint32 `mapstructure:"iterations" default:"50"`
Parallelism uint `mapstructure:"parallelism" default:"4"`
SaltLength uint32 `mapstructure:"salt_length" default:"16"`
KeyLength uint32 `mapstructure:"key_length" default:"32"`
}

// GetCryptoConfigWithDefaults returns a CryptoConfig with default values
// TODO: extract from struct default tags
func GetCryptoConfigWithDefaults() CryptoConfig {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to rename this function in a future patch, currently it's used only in tests and returns DefaultConfigForTest, but the name sounds like non-test function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, I do extract this from struct tags now, and you're right, I should either remove this and have the tests use DefaultConfigForTest or name this ForTest.

return CryptoConfig{
Memory: saltMemory,
Iterations: saltIterations,
Parallelism: saltParameters,
SaltLength: saltLength,
KeyLength: saltKeyLength,
}
return DefaultConfigForTest().Salt
}
12 changes: 6 additions & 6 deletions internal/config/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,12 @@ const awsCredsProvider = "aws"

// DatabaseConfig is the configuration for the database
type DatabaseConfig struct {
Host string `mapstructure:"dbhost"`
Port int `mapstructure:"dbport"`
User string `mapstructure:"dbuser"`
Password string `mapstructure:"dbpass"`
Name string `mapstructure:"dbname"`
SSLMode string `mapstructure:"sslmode"`
Host string `mapstructure:"dbhost" default:"localhost"`
Port int `mapstructure:"dbport" default:"5432"`
User string `mapstructure:"dbuser" default:"postgres"`
Password string `mapstructure:"dbpass" default:"postgres"`
Name string `mapstructure:"dbname" default:"mediator"`
SSLMode string `mapstructure:"sslmode" default:"disable"`

// If set, use credentials from the specified cloud provider.
// Currently supported values are `aws`
Expand Down
16 changes: 3 additions & 13 deletions internal/config/logging.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,9 @@

package config

import "github.com/spf13/viper"

// LoggingConfig is the configuration for the logging package
type LoggingConfig struct {
Level string `mapstructure:"level"`
Format string `mapstructure:"format"`
LogFile string `mapstructure:"logFile"`
}

// SetLoggingViperDefaults sets the default values for the logging configuration
// to be picked up by viper
func SetLoggingViperDefaults(v *viper.Viper) {
v.SetDefault("logging.level", "debug")
v.SetDefault("logging.format", "json")
v.SetDefault("logging.logFile", "")
Level string `mapstructure:"level" default:"debug"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn't change this default, but do you think the default log level should be debug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷

I'm fine with changing it, but the goal here was to make it so that the code actually had all the defaults.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in this PR, but I was wondering if you had some thoughts about how verbose should the default deployment be based on your SaaS work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured we'd set the defaults to be appropriate for development work, though I could see preferring info for that as well to avoid things being too spammy.

Format string `mapstructure:"format" default:"json"`
LogFile string `mapstructure:"logFile" default:""`
}
10 changes: 1 addition & 9 deletions internal/config/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,7 @@

package config

import "github.com/spf13/viper"

// MetricsConfig is the configuration for the metrics
type MetricsConfig struct {
Enabled bool `mapstructure:"enabled"`
}

// SetMetricsViperDefaults sets the default values for the metrics configuration
// to be picked up by viper
func SetMetricsViperDefaults(v *viper.Viper) {
v.SetDefault("metrics.enabled", true)
Enabled bool `mapstructure:"enabled" default:"true"`
}
8 changes: 4 additions & 4 deletions internal/config/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ import (
// HTTPServerConfig is the configuration for the HTTP server
type HTTPServerConfig struct {
// Host is the host to bind to
Host string `mapstructure:"host"`
Host string `mapstructure:"host" default:"127.0.0.1"`
// Port is the port to bind to
Port int `mapstructure:"port"`
Port int `mapstructure:"port" default:"8080"`
}

// GetAddress returns the address to bind to
Expand All @@ -40,9 +40,9 @@ func (s *HTTPServerConfig) GetAddress() string {
// GRPCServerConfig is the configuration for the gRPC server
type GRPCServerConfig struct {
// Host is the host to bind to
Host string `mapstructure:"host"`
Host string `mapstructure:"host" default:"127.0.0.1"`
// Port is the port to bind to
Port int `mapstructure:"port"`
Port int `mapstructure:"port" default:"8090"`
}

// GetAddress returns the address to bind to
Expand Down
13 changes: 2 additions & 11 deletions internal/config/tracing.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,10 @@

package config

import "github.com/spf13/viper"

// TracingConfig is the configuration for our tracing capabilities
type TracingConfig struct {
Enabled bool `mapstructure:"enabled"`
Enabled bool `mapstructure:"enabled" default:"false"`
// for the demonstration, we use AlwaysSmaple sampler to take all spans.
// do not use this option in production.
SampleRatio float64 `mapstructure:"sample_ratio"`
}

// SetTracingViperDefaults sets the default values for the tracing configuration
// to be picked up by viper
func SetTracingViperDefaults(v *viper.Viper) {
v.SetDefault("tracing.enabled", false)
v.SetDefault("tracing.sample_ratio", 0.1)
SampleRatio float64 `mapstructure:"sample_ratio" default:"0.1"`
}