diff --git a/cmd/clusterctl/client/yamlprocessor/simple_processor.go b/cmd/clusterctl/client/yamlprocessor/simple_processor.go index 773e121a556b..62b96367d22b 100644 --- a/cmd/clusterctl/client/yamlprocessor/simple_processor.go +++ b/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, "") + }) } diff --git a/cmd/clusterctl/client/yamlprocessor/simple_processor_test.go b/cmd/clusterctl/client/yamlprocessor/simple_processor_test.go index 6cfc724a653c..6e4a1ca05d56 100644 --- a/cmd/clusterctl/client/yamlprocessor/simple_processor_test.go +++ b/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,11 +69,18 @@ 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"}, }, } @@ -80,8 +88,13 @@ func TestSimpleProcessor_GetVariables(t *testing.T) { 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()) diff --git a/docs/book/src/clusterctl/provider-contract.md b/docs/book/src/clusterctl/provider-contract.md index abe1f9e85bc0..d4ba388fbfb8 100644 --- a/docs/book/src/clusterctl/provider-contract.md +++ b/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}` + + + +`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 ` (`` 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. + + + +[drone-envsubst]: https://github.com/drone/envsubst diff --git a/go.mod b/go.mod index 2bc253701f98..b5237c6adeb5 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 85989f5ca949..f96adeb1c4f4 100644 --- a/go.sum +++ b/go.sum @@ -94,6 +94,8 @@ github.com/docker/go-units v0.4.0/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDD github.com/docker/spdystream v0.0.0-20160310174837-449fdfce4d96 h1:cenwrSVm+Z7QLSV/BsnenAOcDXdX4cMv4wP0B/5QbPg= github.com/docker/spdystream v0.0.0-20160310174837-449fdfce4d96/go.mod h1:Qh8CwZgvJUkLughtfhJv5dyTYa91l1fOUCrgjqmcifM= github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815/go.mod h1:WwZ+bS3ebgob9U8Nd0kOddGdZWjyMGR8Wziv+TBNwSE= +github.com/drone/envsubst v1.0.3-0.20200709223903-efdb65b94e5a h1:pf3CyiWgjOLL7cjFos89AEOPCWSOoQt7tgbEk/SvBAg= +github.com/drone/envsubst v1.0.3-0.20200709223903-efdb65b94e5a/go.mod h1:N2jZmlMufstn1KEqvbHjw40h1KyTmnVzHcSc9bFiJ2g= github.com/dustin/go-humanize v0.0.0-20171111073723-bb3d318650d4/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk= github.com/dustin/go-humanize v1.0.0 h1:VSnTsYCnlFHaM2/igO1h6X3HA71jcobQuxemgkq4zYo= github.com/dustin/go-humanize v1.0.0/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk= diff --git a/test/infrastructure/docker/go.sum b/test/infrastructure/docker/go.sum index 8246a6f33834..1dcee16db1bd 100644 --- a/test/infrastructure/docker/go.sum +++ b/test/infrastructure/docker/go.sum @@ -81,6 +81,7 @@ github.com/docker/go-units v0.3.3/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDD github.com/docker/go-units v0.4.0/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk= github.com/docker/spdystream v0.0.0-20160310174837-449fdfce4d96/go.mod h1:Qh8CwZgvJUkLughtfhJv5dyTYa91l1fOUCrgjqmcifM= github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815/go.mod h1:WwZ+bS3ebgob9U8Nd0kOddGdZWjyMGR8Wziv+TBNwSE= +github.com/drone/envsubst v1.0.3-0.20200709223903-efdb65b94e5a/go.mod h1:N2jZmlMufstn1KEqvbHjw40h1KyTmnVzHcSc9bFiJ2g= github.com/dustin/go-humanize v0.0.0-20171111073723-bb3d318650d4/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk= github.com/dustin/go-humanize v1.0.0/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk= github.com/elazarl/goproxy v0.0.0-20170405201442-c4fc26588b6e/go.mod h1:/Zj4wYkgs4iZTTu3o/KG3Itv/qCCa8VVMlb3i9OVuzc=