From a49bf24abe3c5b944c06daa167d7487e0aef3370 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Mon, 17 Aug 2015 13:41:50 -0700 Subject: [PATCH] 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 --- configuration/configuration.go | 15 +- configuration/configuration_test.go | 140 ++++++++++++++++-- configuration/parser.go | 212 +++++++++++++++++++--------- docs/configuration.md | 6 - 4 files changed, 288 insertions(+), 85 deletions(-) diff --git a/configuration/configuration.go b/configuration/configuration.go index 1b3519d52..6712f3d82 100644 --- a/configuration/configuration.go +++ b/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 "" } diff --git a/configuration/configuration_test.go b/configuration/configuration_test.go index 4790f21fd..58cdc1a9f 100644 --- a/configuration/configuration_test.go +++ b/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) diff --git a/configuration/parser.go b/configuration/parser.go index 10a0461e2..8b81dd5d5 100644 --- a/configuration/parser.go +++ b/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) - if err != nil { - return err + 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()) @@ -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++ { - 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 - } - } + return p.overwriteStruct(v, fullpath, path, payload) 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 { - switch m.Type().Elem().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() { - 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 +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) + upper := strings.ToUpper(sf.Name) + if _, present := byUpperCase[upper]; present { + panic(fmt.Sprintf("field name collision in configuration object: %s", sf.Name)) } + byUpperCase[upper] = i + } - 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 + 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(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 } diff --git a/docs/configuration.md b/docs/configuration.md index 0e79f1808..2f3fa11c3 100644 --- a/docs/configuration.md +++ b/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.