More flexible environment variable overrides

Overriding configuration parameters with environment variables used to
work by walking the configuration structure and checking for a
corresponding environment variable for each item. This was very limiting
because only variables corresponding to items that already existed in
the configuration structure would be checked. For example, an
environment variable corresponding to nested maps would only be noticed
if the outer map's key already existed.

This commit changes environment variable overriding to iterate over the
environment instead. For environment variables beginning with the
REGISTRY_ prefix, it splits the rest of their names on "_", and
interprets that as a path to the variable to unmarshal into. Map keys
are created as necessary. If we encounter an empty interface partway
through following the path, it becomes an implicit
map[string]interface{}.

With the new unit tests added here, parser.go now has 89.2% test
coverage.

TestParseWithExtraneousEnvStorageParams was removed, because the limit
of one storage driver is no longer enforced while parsing environment
variables. Now, Storage.Type will panic if multiple drivers are
specified.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

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,10 +131,17 @@ func (p *Parser) Parse(in []byte, v interface{}) error {
 		return err
 	}
 
-	err = p.overwriteFields(parseAs, p.prefix)
+	for _, envVar := range p.env {
+		pathStr := envVar.name
+		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())
 	if err != nil {
@@ -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:
+		return p.overwriteStruct(v, fullpath, path, payload)
+	case reflect.Map:
+		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) overwriteStruct(v reflect.Value, fullpath string, path []string, payload string) error {
+	// Generate case-insensitive map of struct fields
+	byUpperCase := make(map[string]int)
 	for i := 0; i < v.NumField(); i++ {
 		sf := v.Type().Field(i)
-			fieldPrefix := strings.ToUpper(prefix + "_" + sf.Name)
+		upper := strings.ToUpper(sf.Name)
-			if e, ok := p.env[fieldPrefix]; ok {
+		if _, present := byUpperCase[upper]; present {
+			panic(fmt.Sprintf("field name collision in configuration object: %s", sf.Name))
+		}
+		byUpperCase[upper] = i
+	}
+
+	fieldIndex, present := byUpperCase[path[0]]
+	if !present {
+		logrus.Warnf("Ignoring unrecognized environment variable %s", fullpath)
+		return nil
+	}
+	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(e), fieldVal.Interface())
+		err := yaml.Unmarshal([]byte(payload), fieldVal.Interface())
 		if err != nil {
 			return err
 		}
-				v.Field(i).Set(reflect.Indirect(fieldVal))
+		field.Set(reflect.Indirect(fieldVal))
-			}
-			err := p.overwriteFields(v.Field(i), fieldPrefix)
-			if err != nil {
-				return err
-			}
-		}
-	case reflect.Map:
-		p.overwriteMap(v, prefix)
-	}
 		return nil
 	}
 
-func (p *Parser) overwriteMap(m reflect.Value, prefix string) error {
+	// If the field is nil, must create an object
-	switch m.Type().Elem().Kind() {
+	switch sf.Type.Kind() {
-	case reflect.Struct:
-		for _, k := range m.MapKeys() {
-			err := p.overwriteFields(m.MapIndex(k), strings.ToUpper(fmt.Sprintf("%s_%s", prefix, k)))
-			if err != nil {
-				return err
-			}
-		}
-		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() {
+		if field.IsNil() {
-			err := p.overwriteMap(m.MapIndex(k), strings.ToUpper(fmt.Sprintf("%s_%s", prefix, k)))
+			field.Set(reflect.MakeMap(sf.Type))
-			if err != nil {
+		}
-				return err
+	case reflect.Ptr:
+		if field.IsNil() {
+			field.Set(reflect.New(sf.Type))
 		}
 	}
-	default:
+
-		envMapRegexp, err := regexp.Compile(fmt.Sprintf("^%s_([A-Z0-9]+)$", strings.ToUpper(prefix)))
+	err := p.overwriteFields(field, fullpath, path[1:], payload)
 	if err != nil {
 		return err
 	}
 
-		for key, val := range p.env {
+	return nil
-			if submatches := envMapRegexp.FindStringSubmatch(key); submatches != nil {
+}
-				mapValue := reflect.New(m.Type().Elem())
+
-				err := yaml.Unmarshal([]byte(val), mapValue.Interface())
+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
+				}
+				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
 		}
-				m.SetMapIndex(reflect.ValueOf(strings.ToLower(submatches[1])), reflect.Indirect(mapValue))
+	} 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.