Merge pull request #888 from aaronlehmann/config-env-vars

More flexible environment variable overrides

configuration/configuration.go

@@ -11,7 +11,10 @@ import (
 )
 
 // Configuration is a versioned registry configuration, intended to be provided by a yaml file, and
-// optionally modified by environment variables
+// optionally modified by environment variables.
+//
+// Note that yaml field names should never include _ characters, since this is the separator used
+// in environment variable names.
 type Configuration struct {
 	// Version is the version which defines the format of the rest of the configuration
 	Version Version `yaml:"version"`
@@ -305,6 +308,8 @@ type Storage map[string]Parameters
 
 // Type returns the storage driver type, such as filesystem or s3
 func (storage Storage) Type() string {
+	var storageType []string
+
 	// Return only key in this map
 	for k := range storage {
 		switch k {
@@ -317,9 +322,15 @@ func (storage Storage) Type() string {
 		case "redirect":
 			// allow configuration of redirect
 		default:
-			return k
+			storageType = append(storageType, k)
 		}
 	}
+	if len(storageType) > 1 {
+		panic("multiple storage drivers specified in configuration or environment: " + strings.Join(storageType, ", "))
+	}
+	if len(storageType) == 1 {
+		return storageType[0]
+	}
 	return ""
 }
 

configuration/configuration_test.go

@@ -4,6 +4,8 @@ import (
 	"bytes"
 	"net/http"
 	"os"
+	"reflect"
+	"strings"
 	"testing"
 
 	. "gopkg.in/check.v1"
@@ -203,6 +205,8 @@ func (suite *ConfigSuite) TestParseIncomplete(c *C) {
 	suite.expectedConfig.Notifications = Notifications{}
 	suite.expectedConfig.HTTP.Headers = nil
 
+	// Note: this also tests that REGISTRY_STORAGE and
+	// REGISTRY_STORAGE_FILESYSTEM_ROOTDIRECTORY can be used together
 	os.Setenv("REGISTRY_STORAGE", "filesystem")
 	os.Setenv("REGISTRY_STORAGE_FILESYSTEM_ROOTDIRECTORY", "/tmp/testroot")
 	os.Setenv("REGISTRY_AUTH", "silly")
@@ -256,17 +260,6 @@ func (suite *ConfigSuite) TestParseWithDifferentEnvStorageType(c *C) {
 	c.Assert(config, DeepEquals, suite.expectedConfig)
 }
 
-// TestParseWithExtraneousEnvStorageParams validates that environment variables
-// that change parameters out of the scope of the specified storage type are
-// ignored.
-func (suite *ConfigSuite) TestParseWithExtraneousEnvStorageParams(c *C) {
-	os.Setenv("REGISTRY_STORAGE_FILESYSTEM_ROOTDIRECTORY", "/tmp/testroot")
-
-	config, err := Parse(bytes.NewReader([]byte(configYamlV0_1)))
-	c.Assert(err, IsNil)
-	c.Assert(config, DeepEquals, suite.expectedConfig)
-}
-
 // TestParseWithDifferentEnvStorageTypeAndParams validates that providing an environment variable
 // that changes the storage type will be reflected in the parsed Configuration struct and that
 // environment storage parameters will also be included
@@ -346,6 +339,131 @@ func (suite *ConfigSuite) TestParseInvalidVersion(c *C) {
 	c.Assert(err, NotNil)
 }
 
+// TestParseExtraneousVars validates that environment variables referring to
+// nonexistent variables don't cause side effects.
+func (suite *ConfigSuite) TestParseExtraneousVars(c *C) {
+	suite.expectedConfig.Reporting.Bugsnag.Endpoint = "localhost:8080"
+
+	// A valid environment variable
+	os.Setenv("REGISTRY_REPORTING_BUGSNAG_ENDPOINT", "localhost:8080")
+
+	// Environment variables which shouldn't set config items
+	os.Setenv("registry_REPORTING_NEWRELIC_LICENSEKEY", "NewRelicLicenseKey")
+	os.Setenv("REPORTING_NEWRELIC_NAME", "some NewRelic NAME")
+	os.Setenv("REGISTRY_DUCKS", "quack")
+	os.Setenv("REGISTRY_REPORTING_ASDF", "ghjk")
+
+	config, err := Parse(bytes.NewReader([]byte(configYamlV0_1)))
+	c.Assert(err, IsNil)
+	c.Assert(config, DeepEquals, suite.expectedConfig)
+}
+
+// TestParseEnvVarImplicitMaps validates that environment variables can set
+// values in maps that don't already exist.
+func (suite *ConfigSuite) TestParseEnvVarImplicitMaps(c *C) {
+	readonly := make(map[string]interface{})
+	readonly["enabled"] = true
+
+	maintenance := make(map[string]interface{})
+	maintenance["readonly"] = readonly
+
+	suite.expectedConfig.Storage["maintenance"] = maintenance
+
+	os.Setenv("REGISTRY_STORAGE_MAINTENANCE_READONLY_ENABLED", "true")
+
+	config, err := Parse(bytes.NewReader([]byte(configYamlV0_1)))
+	c.Assert(err, IsNil)
+	c.Assert(config, DeepEquals, suite.expectedConfig)
+}
+
+// TestParseEnvWrongTypeMap validates that incorrectly attempting to unmarshal a
+// string over existing map fails.
+func (suite *ConfigSuite) TestParseEnvWrongTypeMap(c *C) {
+	os.Setenv("REGISTRY_STORAGE_S3", "somestring")
+
+	_, err := Parse(bytes.NewReader([]byte(configYamlV0_1)))
+	c.Assert(err, NotNil)
+}
+
+// TestParseEnvWrongTypeStruct validates that incorrectly attempting to
+// unmarshal a string into a struct fails.
+func (suite *ConfigSuite) TestParseEnvWrongTypeStruct(c *C) {
+	os.Setenv("REGISTRY_STORAGE_LOG", "somestring")
+
+	_, err := Parse(bytes.NewReader([]byte(configYamlV0_1)))
+	c.Assert(err, NotNil)
+}
+
+// TestParseEnvWrongTypeSlice validates that incorrectly attempting to
+// unmarshal a string into a slice fails.
+func (suite *ConfigSuite) TestParseEnvWrongTypeSlice(c *C) {
+	os.Setenv("REGISTRY_LOG_HOOKS", "somestring")
+
+	_, err := Parse(bytes.NewReader([]byte(configYamlV0_1)))
+	c.Assert(err, NotNil)
+}
+
+// TestParseEnvMany tests several environment variable overrides.
+// The result is not checked - the goal of this test is to detect panics
+// from misuse of reflection.
+func (suite *ConfigSuite) TestParseEnvMany(c *C) {
+	os.Setenv("REGISTRY_VERSION", "0.1")
+	os.Setenv("REGISTRY_LOG_LEVEL", "debug")
+	os.Setenv("REGISTRY_LOG_FORMATTER", "json")
+	os.Setenv("REGISTRY_LOG_HOOKS", "json")
+	os.Setenv("REGISTRY_LOG_FIELDS", "abc: xyz")
+	os.Setenv("REGISTRY_LOG_HOOKS", "- type: asdf")
+	os.Setenv("REGISTRY_LOGLEVEL", "debug")
+	os.Setenv("REGISTRY_STORAGE", "s3")
+	os.Setenv("REGISTRY_AUTH_PARAMS", "param1: value1")
+	os.Setenv("REGISTRY_AUTH_PARAMS_VALUE2", "value2")
+	os.Setenv("REGISTRY_AUTH_PARAMS_VALUE2", "value2")
+
+	_, err := Parse(bytes.NewReader([]byte(configYamlV0_1)))
+	c.Assert(err, IsNil)
+}
+
+func checkStructs(c *C, t reflect.Type, structsChecked map[string]struct{}) {
+	for t.Kind() == reflect.Ptr || t.Kind() == reflect.Map || t.Kind() == reflect.Slice {
+		t = t.Elem()
+	}
+
+	if t.Kind() != reflect.Struct {
+		return
+	}
+	if _, present := structsChecked[t.String()]; present {
+		// Already checked this type
+		return
+	}
+
+	structsChecked[t.String()] = struct{}{}
+
+	byUpperCase := make(map[string]int)
+	for i := 0; i < t.NumField(); i++ {
+		sf := t.Field(i)
+
+		// Check that the yaml tag does not contain an _.
+		yamlTag := sf.Tag.Get("yaml")
+		if strings.Contains(yamlTag, "_") {
+			c.Fatalf("yaml field name includes _ character: %s", yamlTag)
+		}
+		upper := strings.ToUpper(sf.Name)
+		if _, present := byUpperCase[upper]; present {
+			c.Fatalf("field name collision in configuration object: %s", sf.Name)
+		}
+		byUpperCase[upper] = i
+
+		checkStructs(c, sf.Type, structsChecked)
+	}
+}
+
+// TestValidateConfigStruct makes sure that the config struct has no members
+// with yaml tags that would be ambiguous to the environment variable parser.
+func (suite *ConfigSuite) TestValidateConfigStruct(c *C) {
+	structsChecked := make(map[string]struct{})
+	checkStructs(c, reflect.TypeOf(Configuration{}), structsChecked)
+}
+
 func copyConfig(config Configuration) *Configuration {
 	configCopy := new(Configuration)
 

configuration/parser.go

@@ -4,10 +4,11 @@ import (
 	"fmt"
 	"os"
 	"reflect"
-	"regexp"
+	"sort"
 	"strconv"
 	"strings"
 
+	"github.com/Sirupsen/logrus"
 	"gopkg.in/yaml.v2"
 )
 
@@ -59,18 +60,29 @@ type VersionedParseInfo struct {
 	ConversionFunc func(interface{}) (interface{}, error)
 }
 
+type envVar struct {
+	name  string
+	value string
+}
+
+type envVars []envVar
+
+func (a envVars) Len() int           { return len(a) }
+func (a envVars) Swap(i, j int)      { a[i], a[j] = a[j], a[i] }
+func (a envVars) Less(i, j int) bool { return a[i].name < a[j].name }
+
 // Parser can be used to parse a configuration file and environment of a defined
 // version into a unified output structure
 type Parser struct {
 	prefix  string
 	mapping map[Version]VersionedParseInfo
-	env     map[string]string
+	env     envVars
 }
 
 // NewParser returns a *Parser with the given environment prefix which handles
 // versioned configurations which match the given parseInfos
 func NewParser(prefix string, parseInfos []VersionedParseInfo) *Parser {
-	p := Parser{prefix: prefix, mapping: make(map[Version]VersionedParseInfo), env: make(map[string]string)}
+	p := Parser{prefix: prefix, mapping: make(map[Version]VersionedParseInfo)}
 
 	for _, parseInfo := range parseInfos {
 		p.mapping[parseInfo.Version] = parseInfo
@@ -78,9 +90,17 @@ func NewParser(prefix string, parseInfos []VersionedParseInfo) *Parser {
 
 	for _, env := range os.Environ() {
 		envParts := strings.SplitN(env, "=", 2)
-		p.env[envParts[0]] = envParts[1]
+		p.env = append(p.env, envVar{envParts[0], envParts[1]})
 	}
 
+	// We must sort the environment variables lexically by name so that
+	// more specific variables are applied before less specific ones
+	// (i.e. REGISTRY_STORAGE before
+	// REGISTRY_STORAGE_FILESYSTEM_ROOTDIRECTORY). This sucks, but it's a
+	// lot simpler and easier to get right than unmarshalling map entries
+	// into temporaries and merging with the existing entry.
+	sort.Sort(p.env)
+
 	return &p
 }
 
@@ -111,9 +131,16 @@ func (p *Parser) Parse(in []byte, v interface{}) error {
 		return err
 	}
 
-	err = p.overwriteFields(parseAs, p.prefix)
+	for _, envVar := range p.env {
-	if err != nil {
+		pathStr := envVar.name
-		return err
+		if strings.HasPrefix(pathStr, strings.ToUpper(p.prefix)+"_") {
+			path := strings.Split(pathStr, "_")
+
+			err = p.overwriteFields(parseAs, pathStr, path[1:], envVar.value)
+			if err != nil {
+				return err
+			}
+		}
 	}
 
 	c, err := parseInfo.ConversionFunc(parseAs.Interface())
@@ -124,80 +151,133 @@ func (p *Parser) Parse(in []byte, v interface{}) error {
 	return nil
 }
 
-func (p *Parser) overwriteFields(v reflect.Value, prefix string) error {
+// overwriteFields replaces configuration values with alternate values specified
+// through the environment. Precondition: an empty path slice must never be
+// passed in.
+func (p *Parser) overwriteFields(v reflect.Value, fullpath string, path []string, payload string) error {
 	for v.Kind() == reflect.Ptr {
+		if v.IsNil() {
+			panic("encountered nil pointer while handling environment variable " + fullpath)
+		}
 		v = reflect.Indirect(v)
 	}
 	switch v.Kind() {
 	case reflect.Struct:
-		for i := 0; i < v.NumField(); i++ {
+		return p.overwriteStruct(v, fullpath, path, payload)
-			sf := v.Type().Field(i)
-			fieldPrefix := strings.ToUpper(prefix + "_" + sf.Name)
-			if e, ok := p.env[fieldPrefix]; ok {
-				fieldVal := reflect.New(sf.Type)
-				err := yaml.Unmarshal([]byte(e), fieldVal.Interface())
-				if err != nil {
-					return err
-				}
-				v.Field(i).Set(reflect.Indirect(fieldVal))
-			}
-			err := p.overwriteFields(v.Field(i), fieldPrefix)
-			if err != nil {
-				return err
-			}
-		}
 	case reflect.Map:
-		p.overwriteMap(v, prefix)
+		return p.overwriteMap(v, fullpath, path, payload)
+	case reflect.Interface:
+		if v.NumMethod() == 0 {
+			if !v.IsNil() {
+				return p.overwriteFields(v.Elem(), fullpath, path, payload)
+			}
+			// Interface was empty; create an implicit map
+			var template map[string]interface{}
+			wrappedV := reflect.MakeMap(reflect.TypeOf(template))
+			v.Set(wrappedV)
+			return p.overwriteMap(wrappedV, fullpath, path, payload)
+		}
 	}
 	return nil
 }
 
-func (p *Parser) overwriteMap(m reflect.Value, prefix string) error {
+func (p *Parser) overwriteStruct(v reflect.Value, fullpath string, path []string, payload string) error {
-	switch m.Type().Elem().Kind() {
+	// Generate case-insensitive map of struct fields
-	case reflect.Struct:
+	byUpperCase := make(map[string]int)
-		for _, k := range m.MapKeys() {
+	for i := 0; i < v.NumField(); i++ {
-			err := p.overwriteFields(m.MapIndex(k), strings.ToUpper(fmt.Sprintf("%s_%s", prefix, k)))
+		sf := v.Type().Field(i)
-			if err != nil {
+		upper := strings.ToUpper(sf.Name)
-				return err
+		if _, present := byUpperCase[upper]; present {
-			}
+			panic(fmt.Sprintf("field name collision in configuration object: %s", sf.Name))
-		}
-		envMapRegexp, err := regexp.Compile(fmt.Sprintf("^%s_([A-Z0-9]+)$", strings.ToUpper(prefix)))
-		if err != nil {
-			return err
-		}
-		for key, val := range p.env {
-			if submatches := envMapRegexp.FindStringSubmatch(key); submatches != nil {
-				mapValue := reflect.New(m.Type().Elem())
-				err := yaml.Unmarshal([]byte(val), mapValue.Interface())
-				if err != nil {
-					return err
-				}
-				m.SetMapIndex(reflect.ValueOf(strings.ToLower(submatches[1])), reflect.Indirect(mapValue))
-			}
-		}
-	case reflect.Map:
-		for _, k := range m.MapKeys() {
-			err := p.overwriteMap(m.MapIndex(k), strings.ToUpper(fmt.Sprintf("%s_%s", prefix, k)))
-			if err != nil {
-				return err
-			}
-		}
-	default:
-		envMapRegexp, err := regexp.Compile(fmt.Sprintf("^%s_([A-Z0-9]+)$", strings.ToUpper(prefix)))
-		if err != nil {
-			return err
 		}
+		byUpperCase[upper] = i
+	}
 
-		for key, val := range p.env {
+	fieldIndex, present := byUpperCase[path[0]]
-			if submatches := envMapRegexp.FindStringSubmatch(key); submatches != nil {
+	if !present {
-				mapValue := reflect.New(m.Type().Elem())
+		logrus.Warnf("Ignoring unrecognized environment variable %s", fullpath)
-				err := yaml.Unmarshal([]byte(val), mapValue.Interface())
+		return nil
-				if err != nil {
+	}
-					return err
+	field := v.Field(fieldIndex)
+	sf := v.Type().Field(fieldIndex)
+
+	if len(path) == 1 {
+		// Env var specifies this field directly
+		fieldVal := reflect.New(sf.Type)
+		err := yaml.Unmarshal([]byte(payload), fieldVal.Interface())
+		if err != nil {
+			return err
+		}
+		field.Set(reflect.Indirect(fieldVal))
+		return nil
+	}
+
+	// If the field is nil, must create an object
+	switch sf.Type.Kind() {
+	case reflect.Map:
+		if field.IsNil() {
+			field.Set(reflect.MakeMap(sf.Type))
+		}
+	case reflect.Ptr:
+		if field.IsNil() {
+			field.Set(reflect.New(sf.Type))
+		}
+	}
+
+	err := p.overwriteFields(field, fullpath, path[1:], payload)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (p *Parser) overwriteMap(m reflect.Value, fullpath string, path []string, payload string) error {
+	if m.Type().Key().Kind() != reflect.String {
+		// non-string keys unsupported
+		logrus.Warnf("Ignoring environment variable %s involving map with non-string keys", fullpath)
+		return nil
+	}
+
+	if len(path) > 1 {
+		// If a matching key exists, get its value and continue the
+		// overwriting process.
+		for _, k := range m.MapKeys() {
+			if strings.ToUpper(k.String()) == path[0] {
+				mapValue := m.MapIndex(k)
+				// If the existing value is nil, we want to
+				// recreate it instead of using this value.
+				if (mapValue.Kind() == reflect.Ptr ||
+					mapValue.Kind() == reflect.Interface ||
+					mapValue.Kind() == reflect.Map) &&
+					mapValue.IsNil() {
+					break
 				}
-				m.SetMapIndex(reflect.ValueOf(strings.ToLower(submatches[1])), reflect.Indirect(mapValue))
+				return p.overwriteFields(mapValue, fullpath, path[1:], payload)
 			}
 		}
 	}
+
+	// (Re)create this key
+	var mapValue reflect.Value
+	if m.Type().Elem().Kind() == reflect.Map {
+		mapValue = reflect.MakeMap(m.Type().Elem())
+	} else {
+		mapValue = reflect.New(m.Type().Elem())
+	}
+	if len(path) > 1 {
+		err := p.overwriteFields(mapValue, fullpath, path[1:], payload)
+		if err != nil {
+			return err
+		}
+	} else {
+		err := yaml.Unmarshal([]byte(payload), mapValue.Interface())
+		if err != nil {
+			return err
+		}
+	}
+
+	m.SetMapIndex(reflect.ValueOf(strings.ToLower(path[0])), reflect.Indirect(mapValue))
+
 	return nil
 }

docs/configuration.md

@@ -33,12 +33,6 @@ To override this value, set an environment variable like this:
 This variable overrides the `/var/lib/registry` value to the `/somewhere`
 directory.
 
->**Note**: If an environment variable changes a map value into a string, such
->as replacing the storage driver type with `REGISTRY_STORAGE=filesystem`, then
->all sub-fields will be erased. As such, specifying the storage type in the
->environment will remove all parameters related to the old storage
->configuration (order *matters*).
-
 ## Overriding the entire configuration file
 
 If the default configuration is not a sound basis for your usage, or if you are having issues overriding keys from the environment, you can specify an alternate YAML configuration file by mounting it as a volume in the container.