Add an option for unexpanded sources (#97)

Some sources of configuration (e.g., secrets) shouldn't have environment
variable expansion enabled. Internally, we require support for this
feature in multiple places. This PR adds support for so-called "raw"
sources, which disable variable expansion.

This is (admittedly) a huge hack. I'm open to alternatives.

Author: akshayjshah

config.go

@@ -70,11 +70,25 @@ func NewYAML(options ...YAMLOption) (*YAML, error) {
 		return nil, fmt.Errorf("error applying options: %v", cfg.err)
 	}
 
+	// Some sources shouldn't have environment variables expanded; protect those
+	// sources by escaping the contents. (Expanding before merging re-exposes a
+	// number of bugs, so we can't just selectively expand sources before
+	// merging.)
+	sourceBytes := make([][]byte, len(cfg.sources))
+	for i := range cfg.sources {
+		s := cfg.sources[i]
+		if !s.raw {
+			sourceBytes[i] = s.bytes
+			continue
+		}
+		sourceBytes[i] = escapeVariables(s.bytes)
+	}
+
 	// On construction, go through a full merge-serialize-deserialize cycle to
 	// catch any duplicated keys as early as possible (in strict mode). It also
 	// strips comments, which stops us from attempting environment variable
 	// expansion. (We'll expand environment variables next.)
-	merged, err := merge.YAML(cfg.sources, cfg.strict)
+	merged, err := merge.YAML(sourceBytes, cfg.strict)
 	if err != nil {
 		return nil, fmt.Errorf("couldn't merge YAML sources: %v", err)
 	}
@@ -89,7 +103,7 @@ func NewYAML(options ...YAMLOption) (*YAML, error) {
 
 	y := &YAML{
 		name:   cfg.name,
-		raw:    cfg.sources,
+		raw:    sourceBytes,
 		strict: cfg.strict,
 	}
 

config_ext_test.go

@@ -763,3 +763,31 @@ func TestNullSources(t *testing.T) {
 		})
 	})
 }
+
+func TestRawSources(t *testing.T) {
+	env := map[string]string{
+		"ZONE": "west1",
+	}
+	lookup := func(k string) (string, bool) {
+		v, ok := env[k]
+		return v, ok
+	}
+
+	base := `zone: $ZONE`
+	secrets := `secret: abc$ZONE`
+
+	cfg := struct {
+		Zone   string
+		Secret string
+	}{}
+
+	provider, err := NewYAML(
+		Source(strings.NewReader(base)),
+		RawSource(strings.NewReader(secrets)),
+		Expand(lookup),
+	)
+	require.NoError(t, err, "failed to create provider")
+	assert.NoError(t, provider.Get(Root).Populate(&cfg), "failed to populate config struct")
+	assert.Equal(t, "west1", cfg.Zone, "unexpected zone")
+	assert.Equal(t, "abc$ZONE", cfg.Secret, "unexpected secret")
+}

escape.go

@@ -0,0 +1,18 @@
+package config
+
+import "bytes"
+
+var (
+	_dollar       = []byte("$")
+	_doubleDollar = []byte("$$")
+)
+
+// To avoid a variety of other bugs (e.g,
+// https://github.com/uber-go/config/issues/80), we defer environment variable
+// expansion until we've already merged all sources. However, merging blends
+// data from sources that should have variables expanded and those that
+// shouldn't (e.g., secrets). To protect variable-like strings in sources that
+// shouldn't be expanded, we must escape them.
+func escapeVariables(bs []byte) []byte {
+	return bytes.Replace(bs, _dollar, _doubleDollar, -1)
+}

escape_test.go

@@ -0,0 +1,29 @@
+package config
+
+import (
+	"bytes"
+	"io/ioutil"
+	"testing"
+	"testing/quick"
+
+	"github.com/stretchr/testify/require"
+	"golang.org/x/text/transform"
+)
+
+func TestEscapeVariablesQuick(t *testing.T) {
+	transformer := newExpandTransformer(func(k string) (string, bool) {
+		return "expanded", true
+	})
+	// Round-tripping any string through escaping, then expansion should return
+	// the original unchanged.
+	round := func(s string) bool {
+		escaped := escapeVariables([]byte(s))
+		r := transform.NewReader(bytes.NewReader(escaped), transformer)
+		unescaped, err := ioutil.ReadAll(r)
+		require.NoError(t, err, "failed to read expanded config")
+		return s == string(unescaped)
+	}
+	if err := quick.Check(round, nil); err != nil {
+		t.Error(err)
+	}
+}

option.go

@@ -39,9 +39,12 @@ type optionFunc func(*config)
 
 func (f optionFunc) apply(c *config) { f(c) }
 
-// Expand enables variable expansion in all provided sources. The supplied
-// function MUST behave like os.LookupEnv: it looks up a key and returns a
-// value and whether the key was found.
+// Expand enables variable expansion in all non-raw provided sources. The
+// supplied function MUST behave like os.LookupEnv: it looks up a key and
+// returns a value and whether the key was found. Any expansion is deferred
+// until after all sources are merged, so it's not possible to reference
+// different variables in different sources and have the values automatically
+// merged.
 //
 // Expand allows variable references to take two forms: $VAR or
 // ${VAR:default}. In the first form, variable names MUST adhere to shell
@@ -90,19 +93,38 @@ func Name(name string) YAMLOption {
 
 // Source adds a source of YAML configuration. Later sources override earlier
 // ones using the merge logic described in the package-level documentation.
+//
+// Sources are subject to variable expansion (via the Expand option). To
+// provide a source that remains unexpanded, use the RawSource option.
 func Source(r io.Reader) YAMLOption {
 	all, err := ioutil.ReadAll(r)
 	if err != nil {
 		return failed(err)
 	}
 	return optionFunc(func(c *config) {
-		c.sources = append(c.sources, all)
+		c.sources = append(c.sources, source{bytes: all})
+	})
+}
+
+// RawSource adds a source of YAML configuration. Later sources override
+// earlier ones using the merge logic described in the package-level
+// documentation.
+//
+// Raw sources are not subject to variable expansion. To provide a source with
+// variable expansion enabled, use the Source option.
+func RawSource(r io.Reader) YAMLOption {
+	all, err := ioutil.ReadAll(r)
+	if err != nil {
+		return failed(err)
+	}
+	return optionFunc(func(c *config) {
+		c.sources = append(c.sources, source{bytes: all, raw: true})
 	})
 }
 
 // File opens a file, uses it as a source of YAML configuration, and closes it
-// once provider construction is complete. Priority and merge logic are
-// identical to Source.
+// once provider construction is complete. Priority, merge, and expansion
+// logic are identical to Source.
 func File(name string) YAMLOption {
 	f, err := os.Open(name)
 	if err != nil {
@@ -117,20 +139,20 @@ func File(name string) YAMLOption {
 		return failed(err)
 	}
 	return optionFunc(func(c *config) {
-		c.sources = append(c.sources, all)
+		c.sources = append(c.sources, source{bytes: all})
 	})
 }
 
 // Static serializes a Go data structure to YAML and uses the result as a
 // source. If serialization fails, provider construction will return an error.
-// Priority and merge logic are identical to Source.
+// Priority, merge, and expansion logic are identical to Source.
 func Static(val interface{}) YAMLOption {
 	bs, err := yaml.Marshal(val)
 	if err != nil {
 		return failed(err)
 	}
 	return optionFunc(func(c *config) {
-		c.sources = append(c.sources, bs)
+		c.sources = append(c.sources, source{bytes: bs})
 	})
 }
 
@@ -140,10 +162,15 @@ func failed(err error) YAMLOption {
 	})
 }
 
+type source struct {
+	bytes []byte
+	raw   bool
+}
+
 type config struct {
 	name    string
 	strict  bool
-	sources [][]byte
+	sources []source
 	lookup  LookupFunc
 	err     error
 }