Ensure replacement values are properly quoted when necessary

[jeromefroe]

YAML requires that strings which contain characters that conflict with YAML syntax elements must be quoted in order to be parsed properly. For example, YAML does not allow unquoted strings to begin with the @ character so any string that begins with this character must be quoted. The expandTransformer, however, does not currently check if a replacement value needs to be quoted and will produce invalid YAML if it's lookup function returns a value that must be quoted. This problem is reproduced in the following program:

package main

import (
	"fmt"
	"os"
	"strings"

	"go.uber.org/config"
)

func main() {
	environment := map[string]string{"FOO": "@foo"}
	lookup := func(key string) (string, bool) {
		s, ok := environment[key]
		return s, ok
	}

	yaml := strings.NewReader("key: ${FOO}")
	p, err := config.NewYAML(config.Source(yaml), config.Expand(lookup))
	if err != nil {
		fmt.Printf("Error: Unable to construct YAML provider: %v.\n", err)
		os.Exit(1)
	}
	fmt.Println(p.Get("key").Value())
}

The output of this program is:

Error: Unable to construct YAML provider: couldn't decode merged YAML: yaml: found character that cannot start any token.

The issue is that the YAML template

key: ${FOO}

is converted to the following YAML source

key: @foo

but in order for this source to be valid YAML it should be

key: "@foo"

[prashantv]

Can you use key: "${FOO}" instead?

The given fix will always make the value a string, but we often use it for port values which are integers.

[jeromefroe]

Unfortunately key: "${FOO}" doesn't work because when the sources are merged together in the call to merge.YAML within NewYAML the values are ultimately passed to the yaml decoder which, since ${FOO} is a valid string without quotes, will output it without the quotes. I'll play around with the problem some more to see how we can handle integers.

[jeromefroe]

This is a thorny problem, expanding the variables before merging would enable one to ensure they are quoted as suggested above, but leads to other issues such as environment variables in comments being expanded as well. To get around that specific issue the package could serialize and then deserialize all objects with the yaml package before expanding the variables, but the expansion logic would also need to be extremely careful about whether a given variable would be merged away (i.e. it's okay for a preliminary variable lookup to fail as long as that field is overwritten in the final merged config).

In our case, I was able to just change the problematic value to a new value which did not need to be quoted. I'll leave the issue open just in case others run into the same problem. It would also be helpful if the package returned custom errors which allowed users to access the merged YAML file as the line numbers in the error messages are for the merged file and therefore make it difficult to track down which specific line is causing a given issue.

[prashantv]

Can you quote the value in the environment variable itself?

One odd thing about the environment variable replacement is that you can technically add a full-blown object through the environment variable, which is possible because there's no re-escaping. It's an undocumented accidental feature, but it can come in handy, but that also means you can treat the environment variable as a serialized YAML value itself.

[jeromefroe]

I could, but it's less overhead for me to just replace the value. I control the value of the variable (it's created in Terraform and then stored as a secret in Kubernetes) so I think it will be more maintainable to just replace it and ensure it doesn't have any problematic characters moving forward than have to remember that the value has quotes.

And I agree, I like that you can pass in an entire object through an environment variable. It would just be nice if the quotes around the variables weren't lost so you could control whether the variable should be interpreted as a string or not.