Merge pull request #4645 from mboersma/clusterctl-required-vars

✨ Show required and defaults in "clusterctl generate cluster <name> --list-variables"

cmd/clusterctl/client/repository/template.go

@@ -30,10 +30,15 @@ import (
 // 1. Checks for all the variables in the cluster template YAML file and replace with corresponding config values
 // 2. Ensure all the cluster objects are deployed in the target namespace.
 type Template interface {
-	// Variables required by the template.
-	// This value is derived by the template YAML.
+	// Variables used by the template.
+	// This value is derived from the template YAML.
 	Variables() []string
 
+	// Variables used by the template with their default values. If the value is `nil`, there is no
+	// default and the variable is required.
+	// This value is derived from the template YAML.
+	VariableMap() map[string]*string
+
 	// TargetNamespace where the template objects will be installed.
 	TargetNamespace() string
 
@@ -47,6 +52,7 @@ type Template interface {
 // template implements Template.
 type template struct {
 	variables       []string
+	variableMap     map[string]*string
 	targetNamespace string
 	objs            []unstructured.Unstructured
 }
@@ -58,6 +64,10 @@ func (t *template) Variables() []string {
 	return t.variables
 }
 
+func (t *template) VariableMap() map[string]*string {
+	return t.variableMap
+}
+
 func (t *template) TargetNamespace() string {
 	return t.targetNamespace
 }
@@ -86,9 +96,15 @@ func NewTemplate(input TemplateInput) (Template, error) {
 		return nil, err
 	}
 
+	variableMap, err := input.Processor.GetVariableMap(input.RawArtifact)
+	if err != nil {
+		return nil, err
+	}
+
 	if input.ListVariablesOnly {
 		return &template{
 			variables:       variables,
+			variableMap:     variableMap,
 			targetNamespace: input.TargetNamespace,
 		}, nil
 	}
@@ -111,6 +127,7 @@ func NewTemplate(input TemplateInput) (Template, error) {
 
 	return &template{
 		variables:       variables,
+		variableMap:     variableMap,
 		targetNamespace: input.TargetNamespace,
 		objs:            objs,
 	}, nil

cmd/clusterctl/client/yamlprocessor/processor.go

@@ -25,9 +25,13 @@ type Processor interface {
 	GetTemplateName(version, flavor string) string
 
 	// GetVariables parses the template blob of bytes and provides a
-	// list of variables that the template requires.
+	// list of variables that the template uses.
 	GetVariables([]byte) ([]string, error)
 
+	// GetVariables parses the template blob of bytes and provides a
+	// map of variables that the template uses with their default values.
+	GetVariableMap([]byte) (map[string]*string, error)
+
 	// Process processes the template blob of bytes and will return the final
 	// yaml with values retrieved from the values getter
 	Process([]byte, func(string) (string, error)) ([]byte, error)

cmd/clusterctl/client/yamlprocessor/simple_processor.go

@@ -54,13 +54,10 @@ func (tp *SimpleProcessor) GetTemplateName(_, flavor string) string {
 
 // GetVariables returns a list of the variables specified in the yaml.
 func (tp *SimpleProcessor) GetVariables(rawArtifact []byte) ([]string, error) {
-	strArtifact := convertLegacyVars(string(rawArtifact))
-
-	variables, err := inspectVariables(strArtifact)
+	variables, err := tp.GetVariableMap(rawArtifact)
 	if err != nil {
 		return nil, err
 	}
-
 	varNames := make([]string, 0, len(variables))
 	for k := range variables {
 		varNames = append(varNames, k)
@@ -69,6 +66,25 @@ func (tp *SimpleProcessor) GetVariables(rawArtifact []byte) ([]string, error) {
 	return varNames, nil
 }
 
+// GetVariableMap returns a map of the variables specified in the yaml.
+func (tp *SimpleProcessor) GetVariableMap(rawArtifact []byte) (map[string]*string, error) {
+	strArtifact := convertLegacyVars(string(rawArtifact))
+	variables, err := inspectVariables(strArtifact)
+	if err != nil {
+		return nil, err
+	}
+	varMap := make(map[string]*string, len(variables))
+	for k, v := range variables {
+		if v == "" {
+			varMap[k] = nil
+		} else {
+			v := v
+			varMap[k] = &v
+		}
+	}
+	return varMap, 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 raw yaml along with an error.
@@ -82,11 +98,11 @@ func (tp *SimpleProcessor) Process(rawArtifact []byte, variablesClient func(stri
 
 	var missingVariables []string
 	// keep track of missing variables to return as error later
-	for name, hasDefault := range variables {
+	for name, defaultValue := 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 {
+		if err != nil && len(defaultValue) == 0 {
 			missingVariables = append(missingVariables, name)
 			continue
 		}
@@ -122,8 +138,8 @@ func (e *errMissingVariables) Error() string {
 // 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)
+func inspectVariables(data string) (map[string]string, error) {
+	variables := make(map[string]string)
 	t, err := parse.Parse(data)
 	if err != nil {
 		return nil, err
@@ -134,7 +150,7 @@ func inspectVariables(data string) (map[string]bool, error) {
 
 // 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) {
+func traverse(root parse.Node, variables map[string]string) {
 	switch v := root.(type) {
 	case *parse.ListNode:
 		// iterate through the list node
@@ -143,8 +159,19 @@ func traverse(root parse.Node, variables map[string]bool) {
 		}
 	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
+			// Build up a default value string
+			b := strings.Builder{}
+			for _, a := range v.Args {
+				switch w := a.(type) {
+				case *parse.FuncNode:
+					b.WriteString(fmt.Sprintf("${%s}", w.Param))
+				case *parse.TextNode:
+					b.WriteString(w.Value)
+				}
+			}
+			// Key the variable name to its default string from the template,
+			// or to an empty string if it's required (no default).
+			variables[v.Param] = b.String()
 		}
 	}
 }

cmd/clusterctl/client/yamlprocessor/simple_processor_test.go

@@ -99,6 +99,72 @@ func TestSimpleProcessor_GetVariables(t *testing.T) {
 	}
 }
 
+func TestSimpleProcessor_GetVariablesMap(t *testing.T) {
+	type args struct {
+		data string
+	}
+	def := "default"
+	aVar := "${A}"
+	foobar := "foobar"
+	quotes := `""`
+	tests := []struct {
+		name    string
+		args    args
+		want    map[string]*string
+		wantErr bool
+	}{
+		{
+			name: "variable with different spacing around the name",
+			args: args{
+				data: "yaml with ${A} ${ B} ${ C} ${ D }",
+			},
+			want: map[string]*string{"A": nil, "B": nil, "C": nil, "D": nil},
+		},
+		{
+			name: "variables used in many places are grouped",
+			args: args{
+				data: "yaml with ${A } ${A} ${A}",
+			},
+			want: map[string]*string{"A": nil},
+		},
+		{
+			name: "variables in multiline texts are processed",
+			args: args{
+				data: "yaml with ${A}\n${B}\n${C}",
+			},
+			want: map[string]*string{"A": nil, "B": nil, "C": nil},
+		},
+		{
+			name: "returns error for variables with regex metacharacters",
+			args: args{
+				data: "yaml with ${BA$R}\n${FOO}",
+			},
+			wantErr: true,
+		},
+		{
+			name: "variables with envsubst functions are properly parsed",
+			args: args{
+				data: `yaml with ${C:=default}\n${B}\n${A=foobar}\n${E=""}\n${D:=${A}}`,
+			},
+			want: map[string]*string{"A": &foobar, "B": nil, "C": &def, "D": &aVar, "E": &quotes},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+			p := NewSimpleProcessor()
+			actual, err := p.GetVariableMap([]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))
+		})
+	}
+}
+
 func TestSimpleProcessor_Process(t *testing.T) {
 	type args struct {
 		yaml                  []byte

cmd/clusterctl/cmd/generate_cluster.go

@@ -182,7 +182,7 @@ func runGenerateClusterTemplate(cmd *cobra.Command, name string) error {
 	}
 
 	if gc.listVariables {
-		return printVariablesOutput(template)
+		return printVariablesOutput(cmd, template)
 	}
 
 	return printYamlOutput(template)

cmd/clusterctl/cmd/util.go

@@ -20,9 +20,13 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"sort"
+	"strconv"
 	"strings"
+	"text/tabwriter"
 
 	"github.com/pkg/errors"
+	"github.com/spf13/cobra"
 
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/client"
 )
@@ -41,14 +45,67 @@ func printYamlOutput(printer client.YamlPrinter) error {
 	return nil
 }
 
+func stringPtr(s string) *string {
+	return &s
+}
+
 // printVariablesOutput prints the expected variables in the template to stdout.
-func printVariablesOutput(printer client.YamlPrinter) error {
-	if len(printer.Variables()) > 0 {
-		fmt.Println("Variables:")
-		for _, v := range printer.Variables() {
+func printVariablesOutput(cmd *cobra.Command, template client.Template) error {
+	// Decorate the variable map for printing
+	variableMap := template.VariableMap()
+	for name, defaultValue := range variableMap {
+		if defaultValue != nil {
+			v := *defaultValue
+			// Add quotes around any unquoted strings
+			if len(v) > 0 && !strings.HasPrefix(v, "\"") {
+				v = fmt.Sprintf("\"%s\"", v)
+				variableMap[name] = &v
+			}
+		}
+	}
+	// Add variables that are defaulted from clusterctl
+	controlPlaneMachineCount, err := cmd.Flags().GetInt64("control-plane-machine-count")
+	if err != nil {
+		return err
+	}
+	variableMap["CONTROL_PLANE_MACHINE_COUNT"] = stringPtr(strconv.FormatInt(controlPlaneMachineCount, 10))
+	workerMachineCount, err := cmd.Flags().GetInt64("worker-machine-count")
+	if err != nil {
+		return err
+	}
+	variableMap["WORKER_MACHINE_COUNT"] = stringPtr(strconv.FormatInt(workerMachineCount, 10))
+	variableMap["KUBERNETES_VERSION"] = stringPtr("the value of --kubernetes-version")
+	variableMap["CLUSTER_NAME"] = stringPtr("<name> from \"clusterctl config cluster <name>\"")
+
+	// transform variable map into required and optional lists
+	var requiredVariables []string
+	var optionalVariables []string
+	for name, defaultValue := range variableMap {
+		if defaultValue != nil {
+			optionalVariables = append(optionalVariables, name)
+		} else {
+			requiredVariables = append(requiredVariables, name)
+		}
+	}
+	sort.Strings(requiredVariables)
+	sort.Strings(optionalVariables)
+
+	if len(requiredVariables) > 0 {
+		fmt.Println("Required Variables:")
+		for _, v := range requiredVariables {
 			fmt.Printf("  - %s\n", v)
 		}
 	}
+
+	if len(optionalVariables) > 0 {
+		fmt.Println("\nOptional Variables:")
+		w := tabwriter.NewWriter(os.Stdout, 0, 4, 2, ' ', tabwriter.FilterHTML)
+		for _, v := range optionalVariables {
+			fmt.Fprintf(w, "  - %s\t(defaults to %s)\n", v, *variableMap[v])
+		}
+		w.Flush()
+	}
+
 	fmt.Println()
 	return nil
 }

cmd/clusterctl/internal/test/fake_processor.go

@@ -17,9 +17,10 @@ limitations under the License.
 package test
 
 type FakeProcessor struct {
-	errGetVariables error
-	errProcess      error
-	artifactName    string
+	errGetVariables   error
+	errGetVariableMap error
+	errProcess        error
+	artifactName      string
 }
 
 func NewFakeProcessor() *FakeProcessor {
@@ -49,6 +50,10 @@ func (fp *FakeProcessor) GetVariables(raw []byte) ([]string, error) {
 	return nil, fp.errGetVariables
 }
 
+func (fp *FakeProcessor) GetVariableMap(raw []byte) (map[string]*string, error) {
+	return nil, fp.errGetVariableMap
+}
+
 func (fp *FakeProcessor) Process(raw []byte, variablesGetter func(string) (string, error)) ([]byte, error) {
 	return nil, fp.errProcess
 }