diff --git a/configuration/configuration.go b/configuration/configuration.go index 1b3519d5..6712f3d8 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 4790f21f..58cdc1a9 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 10a0461e..8b81dd5d 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 cfaabcf2..b4080cb0 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.