Merge pull request #3270 from wfernandes/template-defaults-with-envsubst

✨ Template defaults with envsubst

cmd/clusterctl/client/yamlprocessor/simple_processor.go

@@ -19,15 +19,17 @@ package yamlprocessor
 import (
 	"fmt"
 	"regexp"
+	"sort"
 	"strings"
 
-	"github.com/pkg/errors"
-	"k8s.io/apimachinery/pkg/util/sets"
+	"github.com/drone/envsubst"
+	"github.com/drone/envsubst/parse"
 )
 
-// SimpleProcessor is a yaml processor that does simple variable substitution.
-// It implements the YamlProcessor interface. The variables are defined in
-// the following format ${variable_name}
+// SimpleProcessor is a yaml processor that uses envsubst to substitute values
+// for variables in the format ${var}. It also allows default values if
+// specified in the format ${var:=default}.
+// See https://github.com/drone/envsubst for more details.
 type SimpleProcessor struct{}
 
 var _ Processor = &SimpleProcessor{}
@@ -49,52 +51,113 @@ func (tp *SimpleProcessor) GetTemplateName(_, flavor string) string {
 	return name
 }
 
-// GetVariables returns a list of the variables specified in the yaml
-// manifest. The format of the variables being parsed is ${VAR}
+// GetVariables returns a list of the variables specified in the yaml.
 func (tp *SimpleProcessor) GetVariables(rawArtifact []byte) ([]string, error) {
-	return inspectVariables(rawArtifact), nil
+	strArtifact := convertLegacyVars(string(rawArtifact))
+
+	variables, err := inspectVariables(strArtifact)
+	if err != nil {
+		return nil, err
+	}
+
+	varNames := make([]string, 0, len(variables))
+	for k := range variables {
+		varNames = append(varNames, k)
+	}
+	sort.Strings(varNames)
+	return varNames, nil
 }
 
 // Process returns the final yaml with all the variables replaced with their
 // respective values. If there are variables without corresponding values, it
-// will return the yaml along with an error.
-func (tp *SimpleProcessor) Process(rawArtifact []byte, variablesGetter func(string) (string, error)) ([]byte, error) {
+// will return the raw yaml along with an error.
+func (tp *SimpleProcessor) Process(rawArtifact []byte, variablesClient func(string) (string, error)) ([]byte, error) {
+	tmp := convertLegacyVars(string(rawArtifact))
 	// Inspect the yaml read from the repository for variables.
-	variables := inspectVariables(rawArtifact)
+	variables, err := inspectVariables(tmp)
+	if err != nil {
+		return rawArtifact, err
+	}
 
-	// Replace variables with corresponding values read from the config
-	tmp := string(rawArtifact)
-	var err error
 	var missingVariables []string
-	for _, key := range variables {
-		val, err := variablesGetter(key)
-		if err != nil {
-			missingVariables = append(missingVariables, key)
+	// keep track of missing variables to return as error later
+	for name, hasDefault := range variables {
+		_, err := variablesClient(name)
+		// add to missingVariables list if the variable does not exist in the
+		// variablesClient AND it does not have a default value
+		if err != nil && !hasDefault {
+			missingVariables = append(missingVariables, name)
 			continue
 		}
-		exp := regexp.MustCompile(`\$\{\s*` + regexp.QuoteMeta(key) + `\s*\}`)
-		tmp = exp.ReplaceAllLiteralString(tmp, val)
 	}
+
 	if len(missingVariables) > 0 {
-		err = errors.Errorf("value for variables [%s] is not set. Please set the value using os environment variables or the clusterctl config file", strings.Join(missingVariables, ", "))
+		return rawArtifact, &errMissingVariables{missingVariables}
+	}
+
+	tmp, err = envsubst.Eval(tmp, func(in string) string {
+		v, _ := variablesClient(in)
+		return v
+	})
+	if err != nil {
+		return rawArtifact, err
 	}
 
 	return []byte(tmp), err
 }
 
-// variableRegEx defines the regexp used for searching variables inside a YAML
-var variableRegEx = regexp.MustCompile(`\${\s*([A-Z0-9_$]+)\s*}`)
+type errMissingVariables struct {
+	Missing []string
+}
 
-func inspectVariables(data []byte) []string {
-	variables := sets.NewString()
-	match := variableRegEx.FindAllStringSubmatch(string(data), -1)
+func (e *errMissingVariables) Error() string {
+	sort.Strings(e.Missing)
+	return fmt.Sprintf(
+		"value for variables [%s] is not set. Please set the value using os environment variables or the clusterctl config file",
+		strings.Join(e.Missing, ", "),
+	)
+}
 
-	for _, m := range match {
-		submatch := m[1]
-		if !variables.Has(submatch) {
-			variables.Insert(submatch)
+// inspectVariables parses through the yaml and returns a map of the variable
+// names and if they have default values. It returns an error if it cannot
+// parse the yaml.
+func inspectVariables(data string) (map[string]bool, error) {
+	variables := make(map[string]bool)
+	t, err := parse.Parse(data)
+	if err != nil {
+		return nil, err
+	}
+	traverse(t.Root, variables)
+	return variables, nil
+}
+
+// traverse recursively walks down the root node and tracks the variables
+// which are FuncNodes and if the variables have default values.
+func traverse(root parse.Node, variables map[string]bool) {
+	switch v := root.(type) {
+	case *parse.ListNode:
+		// iterate through the list node
+		for _, ln := range v.Nodes {
+			traverse(ln, variables)
+		}
+	case *parse.FuncNode:
+		if _, ok := variables[v.Param]; !ok {
+			// if there are args, then the variable has a default value
+			variables[v.Param] = len(v.Args) > 0
 		}
 	}
+}
 
-	return variables.List()
+// legacyVariableRegEx defines the regexp used for searching variables inside a YAML.
+// It searches for variables with the format ${ VAR}, ${ VAR }, ${VAR }
+var legacyVariableRegEx = regexp.MustCompile(`(\${(\s+([A-Za-z0-9_$]+)\s+)})|(\${(\s+([A-Za-z0-9_$]+))})|(\${(([A-Za-z0-9_$]+)\s+)})`)
+var whitespaceRegEx = regexp.MustCompile(`\s`)
+
+// convertLegacyVars parses through the yaml string and modifies it replacing
+// variables with the format ${ VAR}, ${ VAR }, ${VAR } to ${VAR}. This is
+// done to maintain backwards compatibility.
+func convertLegacyVars(data string) string {
+	return legacyVariableRegEx.ReplaceAllStringFunc(data, func(o string) string {
+		return whitespaceRegEx.ReplaceAllString(o, "")
+	})
 }

cmd/clusterctl/client/yamlprocessor/simple_processor_test.go

@@ -35,9 +35,10 @@ func TestSimpleProcessor_GetVariables(t *testing.T) {
 		data string
 	}
 	tests := []struct {
-		name string
-		args args
-		want []string
+		name    string
+		args    args
+		want    []string
+		wantErr bool
 	}{
 		{
 			name: "variable with different spacing around the name",
@@ -49,7 +50,7 @@ func TestSimpleProcessor_GetVariables(t *testing.T) {
 		{
 			name: "variables used in many places are grouped",
 			args: args{
-				data: "yaml with ${A} ${A} ${A}",
+				data: "yaml with ${A } ${A} ${A}",
 			},
 			want: []string{"A"},
 		},
@@ -68,20 +69,32 @@ func TestSimpleProcessor_GetVariables(t *testing.T) {
 			want: []string{"A", "B", "C"},
 		},
 		{
-			name: "variables with regex metacharacters",
+			name: "returns error for variables with regex metacharacters",
 			args: args{
 				data: "yaml with ${BA$R}\n${FOO}",
 			},
-			want: []string{"BA$R", "FOO"},
+			wantErr: true,
+		},
+		{
+			name: "variables with envsubst functions are properly parsed",
+			args: args{
+				data: "yaml with ${C:=default}\n${B}\n${A=foobar}",
+			},
+			want: []string{"A", "B", "C"},
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 			p := NewSimpleProcessor()
-
-			g.Expect(p.GetVariables([]byte(tt.args.data))).To(Equal(tt.want))
+			actual, err := p.GetVariables([]byte(tt.args.data))
+			if tt.wantErr {
+				g.Expect(err).To(HaveOccurred())
+				return
+			}
+			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(actual).To(Equal(tt.want))
 		})
 	}
 }
@@ -92,48 +105,72 @@ func TestSimpleProcessor_Process(t *testing.T) {
 		configVariablesClient config.VariablesClient
 	}
 	tests := []struct {
-		name    string
-		args    args
-		want    []byte
-		wantErr bool
+		name             string
+		args             args
+		want             []byte
+		wantErr          bool
+		missingVariables []string
 	}{
 		{
-			name: "pass and replaces variables",
+			name: "replaces legacy variables names (with spaces)",
 			args: args{
-				yaml: []byte("foo ${ BAR }"),
+				yaml: []byte("foo ${ BAR }, ${BAR }, ${ BAR}"),
 				configVariablesClient: test.NewFakeVariableClient().
 					WithVar("BAR", "bar"),
 			},
-			want:    []byte("foo bar"),
+			want:    []byte("foo bar, bar, bar"),
 			wantErr: false,
 		},
 		{
-			name: "pass and replaces variables when variable name contains regex metacharacters",
+			name: "replaces variables when variable value contains regex metacharacters",
 			args: args{
-				yaml: []byte("foo ${ BA$R }"),
+				yaml: []byte("foo ${BAR}"),
 				configVariablesClient: test.NewFakeVariableClient().
-					WithVar("BA$R", "bar"),
+					WithVar("BAR", "ba$r"),
 			},
-			want:    []byte("foo bar"),
+			want:    []byte("foo ba$r"),
 			wantErr: false,
 		},
 		{
-			name: "pass and replaces variables when variable value contains regex metacharacters",
+			name: "uses default values if variable doesn't exist in variables client",
 			args: args{
-				yaml: []byte("foo ${ BAR }"),
+				yaml: []byte("foo ${BAR=default_bar} ${BAZ:=default_baz} ${CAR=default_car} ${CAZ:-default_caz} ${DAR=default_dar}"),
 				configVariablesClient: test.NewFakeVariableClient().
-					WithVar("BAR", "ba$r"),
+					// CAZ,DAR is set but has no value
+					WithVar("BAR", "ba$r").WithVar("CAZ", "").WithVar("DAR", ""),
 			},
-			want:    []byte("foo ba$r"),
+			want:    []byte("foo ba$r default_baz default_car default_caz default_dar"),
+			wantErr: false,
+		},
+		{
+			name: "uses default variables if main variable is doesn't exist",
+			args: args{
+				yaml: []byte("foo ${BAR=default_bar} ${BAZ:=prefix${DEF}-suffix} ${CAZ=${DEF}}"),
+				configVariablesClient: test.NewFakeVariableClient().
+					WithVar("BAR", "ba$r").WithVar("DEF", "football"),
+			},
+			want:    []byte("foo ba$r prefixfootball-suffix football"),
 			wantErr: false,
 		},
 		{
-			name: "returns error when missing values for template variables",
+			name: "returns error with missing template variables listed (for better ux)",
+			args: args{
+				yaml: []byte("foo ${ BAR} ${BAZ} ${CAR}"),
+				configVariablesClient: test.NewFakeVariableClient().
+					WithVar("CAR", "car"),
+			},
+			want:             nil,
+			wantErr:          true,
+			missingVariables: []string{"BAR", "BAZ"},
+		},
+		{
+			name: "returns error when variable name contains regex metacharacters",
 			args: args{
-				yaml:                  []byte("foo ${ BAR } ${ BAZ }"),
-				configVariablesClient: test.NewFakeVariableClient(),
+				yaml: []byte("foo ${BA$R} ${BA_R}"),
+				configVariablesClient: test.NewFakeVariableClient().
+					WithVar("BA$R", "bar").WithVar("BA_R", "ba_r"),
 			},
-			want:    nil,
+			want:    []byte("foo bar ba_r"),
 			wantErr: true,
 		},
 	}
@@ -146,6 +183,14 @@ func TestSimpleProcessor_Process(t *testing.T) {
 			got, err := p.Process(tt.args.yaml, tt.args.configVariablesClient.Get)
 			if tt.wantErr {
 				g.Expect(err).To(HaveOccurred())
+				if len(tt.missingVariables) != 0 {
+					e, ok := err.(*errMissingVariables)
+					g.Expect(ok).To(BeTrue())
+					g.Expect(e.Missing).To(ConsistOf(tt.missingVariables))
+				}
+				// we want to ensure that we keep returning the original yaml
+				// as per the intended behavior of Process
+				g.Expect(got).To(Equal(tt.args.yaml))
 				return
 			}
 			g.Expect(err).NotTo(HaveOccurred())

docs/book/src/clusterctl/provider-contract.md

@@ -150,8 +150,32 @@ will look for objects to reconcile.
 
 #### Variables
 
-The components YAML can contain environment variables matching the regexp `\${\s*([A-Z0-9_]+)\s*}`; it is highly
-recommended to prefix the variable name with the provider name e.g. `${ AWS_CREDENTIALS }`
+The components YAML can contain environment variables matching the format ${VAR}; it is highly
+recommended to prefix the variable name with the provider name e.g. `${AWS_CREDENTIALS}`
+
+<aside class="note warning">
+
+<h1>Warning</h1>
+
+`clusterctl` currently supports variables with leading/trailing spaces such
+as: `${ VAR }`, `${ VAR}`,`${VAR }`. However, these formats will be deprecated
+in the near future. e.g. v1alpha4.
+
+Formats such as `${VAR$FOO}` is not supported.
+</aside>
+
+`clusterctl` uses the library [drone/envsubst][drone-envsubst] to perform
+variable substitution.
+
+```bash
+# If `VAR` is not set or empty, the default value is used. This is true for
+all the following formats.
+${VAR:=default}
+${VAR=default}
+${VAR:-default}
+```
+Other functions such as substring replacement are also supported by the
+library. See [drone/envsubst][drone-envsubst] for more information.
 
 Additionally, each provider should create user facing documentation with the list of required variables and with all the additional
 notes that are required to assist the user in defining the value for each variable.
@@ -215,10 +239,10 @@ Templates writers should use the common variables to ensure consistency across p
 
 | CLI flag                | Variable name     | Note                                        |
 | ---------------------- | ----------------- | ------------------------------------------- |
-|`--target-namespace`| `${ NAMESPACE }` | The namespace where the workload cluster should be deployed |
-|`--kubernetes-version`| `${ KUBERNETES_VERSION }` | The Kubernetes version to use for the workload cluster |
-|`--controlplane-machine-count`| `${ CONTROL_PLANE_MACHINE_COUNT }` | The number of control plane machines to be added to the workload cluster |
-|`--worker-machine-count`| `${ WORKER_MACHINE_COUNT }` | The number of worker machines to be added to the workload cluster |
+|`--target-namespace`| `${NAMESPACE}` | The namespace where the workload cluster should be deployed |
+|`--kubernetes-version`| `${KUBERNETES_VERSION}` | The Kubernetes version to use for the workload cluster |
+|`--controlplane-machine-count`| `${CONTROL_PLANE_MACHINE_COUNT}` | The number of control plane machines to be added to the workload cluster |
+|`--worker-machine-count`| `${WORKER_MACHINE_COUNT}` | The number of worker machines to be added to the workload cluster |
 
 Additionally, value of the command argument to `clusterctl config cluster <cluster-name>` (`<cluster-name>` in this case), will 
 be applied to every occurrence of the `${ CLUSTER_NAME }` variable.
@@ -295,5 +319,8 @@ If moving some of excluded object is required, the provider authors should creat
 the exact move sequence to be executed by the user.
 
 Additionally, provider authors should be aware that `clusterctl move` assumes all the provider's Controllers respect the
-`Cluster.Spec.Paused` field introduced in the v1alpha3 Cluster API specification. 
-
+`Cluster.Spec.Paused` field introduced in the v1alpha3 Cluster API specification.
+
+
+<!--LINKS-->
+[drone-envsubst]: https://github.com/drone/envsubst

go.mod

@@ -8,6 +8,7 @@ require (
 	github.com/coredns/corefile-migration v1.0.7
 	github.com/davecgh/go-spew v1.1.1
 	github.com/docker/distribution v2.7.1+incompatible
+	github.com/drone/envsubst v1.0.3-0.20200709223903-efdb65b94e5a
 	github.com/evanphx/json-patch v4.5.0+incompatible
 	github.com/go-logr/logr v0.1.0
 	github.com/gogo/protobuf v1.3.1