Update envconfig defaults processing (#88)

This updates to the latest version of envconfig which correctly handles not overwriting default values on input structs. This simplifies a lot of the SetDefault() logic that existed to get around this limitation.

As part of this change, I noticed there was inconsistent enforcement of the "Version" field in structs, and that the field was limited to integer values (which would prefer us from having a "v1" or "1.0-beta" version in the future). I created a small type that lets us define and manage allowed versions, and standardized on the error messages across packages. I also updated many of the error messages to be more specific about the failure.

Finally, I removed the envconfig annotations from CLIConfig, since they were actually unused; Viper handles the environment variables and initialization for this structure. For the other configs, I added dedicated tests for defaults.

client-lib/go/client/jvs_client_test.go

@@ -91,7 +91,7 @@ func TestValidateJWT(t *testing.T) {
 	})
 
 	client, err := NewJVSClient(ctx, &JVSConfig{
-		Version:      1,
+		Version:      "1",
 		JVSEndpoint:  svr.URL + path,
 		CacheTimeout: 5 * time.Minute,
 	})

client-lib/go/client/jvs_config.go

@@ -19,56 +19,42 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/abcxyz/jvs/pkg/config"
 	"github.com/hashicorp/go-multierror"
 	"github.com/sethvargo/go-envconfig"
 	"gopkg.in/yaml.v2"
 )
 
-const (
-	// Version default for config.
-	Version             = 1
-	CacheTimeoutDefault = 5 * time.Minute
-)
+var versions = config.NewVersionList("1")
 
 // JVSConfig is the jvs client configuration.
 type JVSConfig struct {
 	// Version is the version of the config.
-	Version uint8 `yaml:"version,omitempty" env:"VERSION,overwrite"`
+	Version string `yaml:"version,omitempty" env:"VERSION,overwrite,default=1"`
 
 	// JVS Endpoint. Expected to be fully qualified, including port. ex. http://127.0.0.1:8080
 	JVSEndpoint string `yaml:"endpoint,omitempty" env:"ENDPOINT,overwrite"`
 
 	// CacheTimeout is the duration that keys stay in cache before being revoked.
-	CacheTimeout time.Duration `yaml:"cache_timeout" env:"CACHE_TIMEOUT,overwrite"`
+	CacheTimeout time.Duration `yaml:"cache_timeout" env:"CACHE_TIMEOUT,overwrite,default=5m"`
 }
 
 // Validate checks if the config is valid.
 func (cfg *JVSConfig) Validate() error {
-	cfg.SetDefault()
 	var err *multierror.Error
-	if cfg.Version != Version {
-		err = multierror.Append(err, fmt.Errorf("unexpected Version %d want %d", cfg.Version, Version))
+	if !versions.Contains(cfg.Version) {
+		err = multierror.Append(err, fmt.Errorf("version %q is invalid, valid versions are: %q",
+			cfg.Version, versions.List()))
 	}
 	if cfg.JVSEndpoint == "" {
 		err = multierror.Append(err, fmt.Errorf("endpoint must be set"))
 	}
 	if cfg.CacheTimeout <= 0 {
-		err = multierror.Append(err, fmt.Errorf("cache timeout invalid: %d", cfg.CacheTimeout))
+		err = multierror.Append(err, fmt.Errorf("cache timeout must be a positive duration, got %q", cfg.CacheTimeout))
 	}
 	return err.ErrorOrNil()
 }
 
-// SetDefault sets defaults for the config.
-func (cfg *JVSConfig) SetDefault() {
-	if cfg.Version == 0 {
-		cfg.Version = Version
-	}
-	if cfg.CacheTimeout == 0 {
-		// env config lib doesn't gracefully handle env overrides with defaults, have to set manually.
-		cfg.CacheTimeout = CacheTimeoutDefault
-	}
-}
-
 // LoadJVSConfig calls the necessary methods to load in config using the OsLookuper which finds env variables specified on the host.
 func LoadJVSConfig(ctx context.Context, b []byte) (*JVSConfig, error) {
 	return loadJVSConfigFromLookuper(ctx, b, envconfig.OsLookuper())

client-lib/go/client/jvs_config_test.go

@@ -44,7 +44,7 @@ endpoint: example.com:8080
 cache_timeout: 1m
 `,
 			wantConfig: &JVSConfig{
-				Version:      1,
+				Version:      "1",
 				JVSEndpoint:  "example.com:8080",
 				CacheTimeout: time.Minute,
 			},
@@ -55,7 +55,7 @@ cache_timeout: 1m
 endpoint: example.com:8080
 `,
 			wantConfig: &JVSConfig{
-				Version:      1,
+				Version:      "1",
 				JVSEndpoint:  "example.com:8080",
 				CacheTimeout: 5 * time.Minute,
 			},
@@ -68,7 +68,7 @@ endpoint: example.com:8080
 cache_timeout: 1m
 `,
 			wantConfig: nil,
-			wantErr:    "failed validating config: 1 error occurred:\n\t* unexpected Version 255 want 1\n\n",
+			wantErr:    `version "255" is invalid, valid versions are:`,
 		},
 		{
 			name: "test_invalid_timeout",
@@ -78,7 +78,7 @@ endpoint: example.com:8080
 cache_timeout: -1m
 `,
 			wantConfig: nil,
-			wantErr:    "failed validating config: 1 error occurred:\n\t* cache timeout invalid: -60000000000\n\n",
+			wantErr:    `cache timeout must be a positive duration, got "-1m0s"`,
 		},
 		{
 			name: "all_values_specified_env_override",
@@ -93,7 +93,7 @@ cache_timeout: 1m
 				"JVS_CACHE_TIMEOUT": "2m",
 			},
 			wantConfig: &JVSConfig{
-				Version:      1,
+				Version:      "1",
 				JVSEndpoint:  "other.net:443",
 				CacheTimeout: 2 * time.Minute,
 			},

client-lib/go/go.mod

@@ -3,12 +3,12 @@ module github.com/abcxyz/jvs/client-lib/go
 go 1.18
 
 require (
-	github.com/abcxyz/jvs v0.0.0-20220520175656-c98ac822412a
+	github.com/abcxyz/jvs v0.0.0-00010101000000-000000000000
 	github.com/abcxyz/pkg v0.0.0-20220627210229-493bf5fec862
 	github.com/google/go-cmp v0.5.8
 	github.com/hashicorp/go-multierror v1.1.1
 	github.com/lestrrat-go/jwx/v2 v2.0.3
-	github.com/sethvargo/go-envconfig v0.6.2
+	github.com/sethvargo/go-envconfig v0.8.0
 	gopkg.in/yaml.v2 v2.4.0
 )
 

client-lib/go/go.sum

@@ -226,8 +226,8 @@ github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZN
 github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA=
 github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ=
 github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4=
-github.com/sethvargo/go-envconfig v0.6.2 h1:982sW5Fc4zj7GWCGbt+tfjvl5fOUm/zMZXLT/hBBfr0=
-github.com/sethvargo/go-envconfig v0.6.2/go.mod h1:00S1FAhRUuTNJazWBWcJGvEHOM+NO6DhoRMAOX7FY5o=
+github.com/sethvargo/go-envconfig v0.8.0 h1:AcmdAewSFAc7pQ1Ghz+vhZkilUtxX559QlDuLLiSkdI=
+github.com/sethvargo/go-envconfig v0.8.0/go.mod h1:Iz1Gy1Sf3T64TQlJSvee81qDhf7YIlt8GMUX6yyNFs0=
 github.com/sethvargo/go-gcpkms v0.1.0 h1:pyjDLqLwpk9pMjDSTilPpaUjgP1AfSjX9WGzitZwGUY=
 github.com/spaolacci/murmur3 v0.0.0-20180118202830-f09979ecbc72/go.mod h1:JwIasOWyU6f++ZhiEuf87xNszmSA2myDM2Kzu9HwQUA=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=

go.mod

@@ -10,7 +10,7 @@ require (
 	github.com/google/uuid v1.3.0
 	github.com/hashicorp/go-multierror v1.1.1
 	github.com/lestrrat-go/jwx/v2 v2.0.3
-	github.com/sethvargo/go-envconfig v0.6.2
+	github.com/sethvargo/go-envconfig v0.8.0
 	github.com/sethvargo/go-gcpkms v0.1.0
 	github.com/sethvargo/go-retry v0.2.3
 	github.com/spf13/cobra v1.4.0

go.sum

@@ -260,8 +260,8 @@ github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6L
 github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4=
 github.com/rogpeppe/go-internal v1.6.1 h1:/FiVV8dS/e+YqF2JvO3yXRFbBLTIuSDkuC7aBOAvL+k=
 github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
-github.com/sethvargo/go-envconfig v0.6.2 h1:982sW5Fc4zj7GWCGbt+tfjvl5fOUm/zMZXLT/hBBfr0=
-github.com/sethvargo/go-envconfig v0.6.2/go.mod h1:00S1FAhRUuTNJazWBWcJGvEHOM+NO6DhoRMAOX7FY5o=
+github.com/sethvargo/go-envconfig v0.8.0 h1:AcmdAewSFAc7pQ1Ghz+vhZkilUtxX559QlDuLLiSkdI=
+github.com/sethvargo/go-envconfig v0.8.0/go.mod h1:Iz1Gy1Sf3T64TQlJSvee81qDhf7YIlt8GMUX6yyNFs0=
 github.com/sethvargo/go-gcpkms v0.1.0 h1:pyjDLqLwpk9pMjDSTilPpaUjgP1AfSjX9WGzitZwGUY=
 github.com/sethvargo/go-gcpkms v0.1.0/go.mod h1:33BuvqUjsYk0bpMgn+WCclCYtMLOyaqtn5j0fCo4vvk=
 github.com/sethvargo/go-retry v0.2.3 h1:oYlgvIvsju3jNbottWABtbnoLC+GDtLdBHxKWxQm/iU=

pkg/cli/root_test.go

@@ -40,7 +40,7 @@ func TestInitCfg(t *testing.T) {
 	initCfg()
 
 	wantCfg := &config.CLIConfig{
-		Version:        1,
+		Version:        "1",
 		Server:         "https://example.com",
 		Authentication: &config.CLIAuthentication{},
 	}

pkg/config/cli_config.go

@@ -20,17 +20,15 @@ import (
 	"github.com/hashicorp/go-multierror"
 )
 
-const (
-	// DefaultCLIConfigVersion is the default CLI config version.
-	DefaultCLIConfigVersion = 1
-)
+// CLIConfigVersions is the list of allowed versions for the CLIConfig.
+var CLIConfigVersions = NewVersionList("1")
 
 type CLIConfig struct {
 	// Version is the version of the config.
-	Version uint8 `yaml:"version,omitempty" env:"VERSION,overwrite"`
+	Version string `yaml:"version,omitempty"`
 
 	// Server is the JVS server address.
-	Server string `yaml:"server,omitempty" env:"SERVER,overwrite"`
+	Server string `yaml:"server,omitempty"`
 
 	// Authentication is the authentication config.
 	Authentication *CLIAuthentication `yaml:"authentication,omitempty"`
@@ -47,6 +45,12 @@ func (cfg *CLIConfig) Validate() error {
 	cfg.SetDefault()
 
 	var err *multierror.Error
+
+	if !CLIConfigVersions.Contains(cfg.Version) {
+		err = multierror.Append(err, fmt.Errorf("version %q is invalid, valid versions are: %q",
+			cfg.Version, CLIConfigVersions.List()))
+	}
+
 	if cfg.Server == "" {
 		err = multierror.Append(err, fmt.Errorf("missing JVS server address"))
 	}
@@ -56,8 +60,8 @@ func (cfg *CLIConfig) Validate() error {
 
 // SetDefault sets default for the config.
 func (cfg *CLIConfig) SetDefault() {
-	if cfg.Version == 0 {
-		cfg.Version = DefaultCLIConfigVersion
+	if cfg.Version == "" {
+		cfg.Version = "1"
 	}
 	if cfg.Authentication == nil {
 		cfg.Authentication = &CLIAuthentication{}

pkg/config/cli_config_test.go

@@ -21,21 +21,31 @@ import (
 	"github.com/google/go-cmp/cmp"
 )
 
-func TestValidateCLIConfig(t *testing.T) {
+func TestCLIConfig_Validate(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
 		name    string
 		cfg     *CLIConfig
 		wantErr string
-	}{{
-		name: "no_error",
-		cfg:  &CLIConfig{Server: "example.com"},
-	}, {
-		name:    "missing_server_error",
-		cfg:     &CLIConfig{},
-		wantErr: "missing JVS server address",
-	}}
+	}{
+		{
+			name: "no_error",
+			cfg:  &CLIConfig{Server: "example.com"},
+		},
+		{
+			name: "bad_version",
+			cfg: &CLIConfig{
+				Version: "255",
+			},
+			wantErr: "missing JVS server address",
+		},
+		{
+			name:    "missing_server_error",
+			cfg:     &CLIConfig{},
+			wantErr: "missing JVS server address",
+		},
+	}
 
 	for _, tc := range tests {
 		tc := tc
@@ -49,7 +59,7 @@ func TestValidateCLIConfig(t *testing.T) {
 	}
 }
 
-func TestSetDefault(t *testing.T) {
+func TestCLIConfig_SetDefault(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
@@ -60,15 +70,17 @@ func TestSetDefault(t *testing.T) {
 		name: "default_empty_authentication",
 		cfg:  &CLIConfig{},
 		wantCfg: &CLIConfig{
-			Version:        1,
+			Version:        "1",
 			Authentication: &CLIAuthentication{},
 		},
 	}}
 
 	for _, tc := range tests {
 		tc := tc
+
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
+
 			tc.cfg.SetDefault()
 			if diff := cmp.Diff(tc.wantCfg, tc.cfg); diff != "" {
 				t.Errorf("config with defaults (-want,+got):\n%s", diff)

pkg/config/crypto_config.go

@@ -25,15 +25,13 @@ import (
 	"gopkg.in/yaml.v2"
 )
 
-const (
-	// Version default for config.
-	Version = 1
-)
+// CryptoConfigVersions is the list of allowed versions for the CryptoConfig.
+var CryptoConfigVersions = NewVersionList("1")
 
 // CryptoConfig is the full jvs config.
 type CryptoConfig struct {
 	// Version is the version of the config.
-	Version uint8 `yaml:"version,omitempty" env:"VERSION,overwrite"`
+	Version string `yaml:"version,omitempty" env:"VERSION,overwrite,default=1"`
 
 	// -- Crypto variables --
 	// KeyTTL is the length of time that we expect a key to be valid for.
@@ -54,41 +52,37 @@ type CryptoConfig struct {
 
 // Validate checks if the config is valid.
 func (cfg *CryptoConfig) Validate() error {
-	cfg.SetDefault()
-
 	var err *multierror.Error
-	if cfg.Version != Version {
-		err = multierror.Append(err, fmt.Errorf("unexpected Version %d want %d", cfg.Version, Version))
+
+	if !CryptoConfigVersions.Contains(cfg.Version) {
+		err = multierror.Append(err, fmt.Errorf("version %q is invalid, valid versions are: %q",
+			cfg.Version, CryptoConfigVersions.List()))
 	}
 
 	if cfg.KeyTTL <= 0 {
-		err = multierror.Append(err, fmt.Errorf("key ttl is invalid: %v", cfg.KeyTTL))
+		err = multierror.Append(err, fmt.Errorf("key ttl must be a positive duration, got %q", cfg.KeyTTL))
 	}
 
 	if cfg.GracePeriod <= 0 {
-		err = multierror.Append(err, fmt.Errorf("grace period is invalid: %v", cfg.GracePeriod))
+		err = multierror.Append(err, fmt.Errorf("grace period must be a positive duration, got %q", cfg.GracePeriod))
 	}
 
 	if cfg.DisabledPeriod <= 0 {
-		err = multierror.Append(err, fmt.Errorf("disabled period is invalid: %v", cfg.DisabledPeriod))
+		err = multierror.Append(err, fmt.Errorf("disabled period must be a positive duration, got %q", cfg.DisabledPeriod))
 	}
 
-	// Propagation delay must be lower than grace period.
-	if cfg.PropagationDelay <= 0 || cfg.PropagationDelay > cfg.GracePeriod {
-		err = multierror.Append(err, fmt.Errorf("propagation delay is invalid: %v", cfg.PropagationDelay))
+	// Propagation delay must be positive but less than than grace period.
+	if cfg.PropagationDelay <= 0 {
+		err = multierror.Append(err, fmt.Errorf("propagation delay must be a positive duration, got %q", cfg.PropagationDelay))
+	}
+	if cfg.PropagationDelay > cfg.GracePeriod {
+		err = multierror.Append(err, fmt.Errorf("propagation delay %q must be less than grace period %q",
+			cfg.PropagationDelay, cfg.GracePeriod))
 	}
 
 	return err.ErrorOrNil()
 }
 
-// SetDefault sets default for the config.
-func (cfg *CryptoConfig) SetDefault() {
-	// TODO: set defaults for other fields if necessary.
-	if cfg.Version == 0 {
-		cfg.Version = Version
-	}
-}
-
 // GetRotationAge gets the duration after a key has been created that a new key should be created.
 func (cfg *CryptoConfig) RotationAge() time.Duration {
 	return cfg.KeyTTL - cfg.GracePeriod