Allow to use variable substitution for cluster-api components

There are 2 ways to define variables for substitution:

1) Define them as Environment variables and set EnvVars: true
in clusterctl object

2) Define them in additional-vars map in clusterctl object

Also adds possibility not to substitute variables if they are not defined in
in additional-vars or in environment for specific provider. But be aware,
if these variables are defined they will be substituted even if
variable-substitution: true

Change-Id: I0c92b3c37ac7b2e7c48c1033c074baef48f752a7
Relates-To: #284

pkg/api/v1alpha1/clusterctl_types.go

@@ -29,6 +29,12 @@ type Clusterctl struct {
 	Providers   []*Provider  `json:"providers,omitempty"`
 	InitOptions *InitOptions `json:"init-options,omitempty"`
 	MoveOptions *MoveOptions `json:"move-options,omitempty"`
+	// AdditionalComponentVariables are variables that will be available to clusterctl
+	// when reading provider components
+	AdditionalComponentVariables map[string]string `json:"additional-vars,omitempty"`
+	// EnvVars if set to true, allows to source variables for cluster-api components
+	// for environment variables.
+	EnvVars bool `json:"env-vars,omitempty"`
 }
 
 // Provider is part of clusterctl config
@@ -44,6 +50,11 @@ type Provider struct {
 	// Map of versions where each key is a version and value is path relative to target path of the manifest
 	// ignored if IsClusterctlRepository is set to true
 	Versions map[string]string `json:"versions,omitempty"`
+
+	// VariableSubstitution indicates weather you want to substitute variales in the cluster-api manifests
+	// if set to true, variables will be substituted only if they are defined either in Environment or
+	// in AdditionalComponentVariables, if not they will be left as is.
+	VariableSubstitution bool `json:"variable-substitution,omitempty"`
 }
 
 // InitOptions container with exposed clusterctl InitOptions

pkg/clusterctl/client/factory.go

@@ -86,9 +86,10 @@ func (f RepositoryFactory) repoFactory(provider config.Provider) (repository.Cli
 			return nil, err
 		}
 		return &implementations.RepositoryClient{
-			Client:       repoClient,
-			ProviderType: string(repoType),
-			ProviderName: name}, nil
+			Client:               repoClient,
+			ProviderType:         string(repoType),
+			ProviderName:         name,
+			VariableSubstitution: airProv.VariableSubstitution}, nil
 	}
 	log.Printf("Creating clusterctl repository implementation interface for provider %s of type %s\n",
 		name,

pkg/clusterctl/implementations/components_client.go

@@ -25,17 +25,17 @@ var _ repository.ComponentsClient = &ComponentsClient{}
 // ComponentsClient override Get() method to return same components,
 // but in our implementation we skip variable substitution.
 type ComponentsClient struct {
-	client       repository.ComponentsClient
-	providerType string
-	providerName string
+	client               repository.ComponentsClient
+	providerType         string
+	providerName         string
+	variableSubstitution bool
 }
 
 // Get returns the components from a repository but without variable substitution
 func (cc *ComponentsClient) Get(options repository.ComponentsOptions) (repository.Components, error) {
-	// This removes variable substitution in components.yaml
-	// TODO we may consider making this configurable
-	options.SkipVariables = true
-	log.Debugf("Getting airshipctl provider components, setting skipping variable substitution.\n"+
-		"Provider type: %s, name: %s\n", cc.providerType, cc.providerName)
+	// Invert variable substitution, so that by default clusterctl will not substitute variables
+	options.SkipVariables = !cc.variableSubstitution
+	log.Printf("Getting airshipctl provider components, skipping variable substitution: %t.\n"+
+		"Provider type: %s, name: %s\n", options.SkipVariables, cc.providerType, cc.providerName)
 	return cc.client.Get(options)
 }

pkg/clusterctl/implementations/errors.go

@@ -44,3 +44,12 @@ type ErrValueForVariableNotSet struct {
 func (e ErrValueForVariableNotSet) Error() string {
 	return fmt.Sprintf("value for variable %q is not set", e.Variable)
 }
+
+// ErrAppendNotAllowed is returned when version map is empty or not defined
+type ErrAppendNotAllowed struct {
+	Variables map[string]string
+}
+
+func (e ErrAppendNotAllowed) Error() string {
+	return fmt.Sprintf(`variables %v, are not allowed to be appended from clusterctl.AdditoinalVariables`, e.Variables)
+}

pkg/clusterctl/implementations/reader.go

@@ -15,11 +15,15 @@
 package implementations
 
 import (
+	"os"
+	"regexp"
+
 	clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/client/config"
 	"sigs.k8s.io/yaml"
 
 	airshipv1 "opendev.org/airship/airshipctl/pkg/api/v1alpha1"
+	"opendev.org/airship/airshipctl/pkg/log"
 )
 
 var _ config.Reader = &AirshipReader{}
@@ -32,7 +36,8 @@ const (
 
 // AirshipReader provides a reader implementation backed by a map
 type AirshipReader struct {
-	variables map[string]string
+	variables   map[string]string
+	varsFromEnv bool
 }
 
 // configProvider is a mirror of config.Provider, re-implemented here in order to
@@ -51,10 +56,20 @@ func (f *AirshipReader) Init(config string) error {
 
 // Get implementation of clusterctl reader interface
 func (f *AirshipReader) Get(key string) (string, error) {
-	// TODO handle empty keys
+	// if value is set in variables - return it, variables from variables map take precedence over
+	// env variables
 	if val, ok := f.variables[key]; ok {
 		return val, nil
 	}
+	// if we are allowed to check environment variables and key is allowed to be taken from env
+	// look it up and return
+	if f.varsFromEnv && allowFromEnv(key) {
+		val, ok := os.LookupEnv(key)
+		if ok {
+			return val, nil
+		}
+	}
+	// if neither env nor variables slice has the var, return error
 	return "", ErrValueForVariableNotSet{Variable: key}
 }
 
@@ -73,6 +88,23 @@ func (f *AirshipReader) UnmarshalKey(key string, rawval interface{}) error {
 	return yaml.Unmarshal([]byte(data), rawval)
 }
 
+func allowFromEnv(key string) bool {
+	variableRegEx := regexp.MustCompile(`^([A-Z0-9_$]+)$`)
+	log.Debugf("Verifying that variable %s is allowed to be taken from environment", key)
+	return variableRegEx.MatchString(key)
+}
+
+func allowAppend(key, _ string) bool {
+	// TODO Investigate if more vaildation should be done here
+	forbidenVars := map[string]string{
+		config.ProvidersConfigKey: "",
+		imagesConfigKey:           "",
+	}
+	_, forbid := forbidenVars[key]
+	log.Debugf("Verifying that variable %s is allowed to be appended", key)
+	return !forbid
+}
+
 // NewAirshipReader returns airship implementation of clusterctl reader interface
 func NewAirshipReader(options *airshipv1.Clusterctl) (*AirshipReader, error) {
 	variables := map[string]string{}
@@ -89,9 +121,18 @@ func NewAirshipReader(options *airshipv1.Clusterctl) (*AirshipReader, error) {
 	if err != nil {
 		return nil, err
 	}
+	for key, val := range options.AdditionalComponentVariables {
+		// if variable is not allowed, it will be ignored
+		if allowAppend(key, val) {
+			variables[key] = val
+		}
+	}
 	// Add providers to config
 	variables[config.ProvidersConfigKey] = string(b)
 	// Add empty image configuration to config
 	variables[imagesConfigKey] = ""
-	return &AirshipReader{variables: variables}, nil
+	return &AirshipReader{
+		variables:   variables,
+		varsFromEnv: options.EnvVars,
+	}, nil
 }

pkg/clusterctl/implementations/repository_client.go

@@ -27,8 +27,9 @@ var _ config.Provider = &RepositoryClient{}
 // but in our implementation we skip variable substitution.
 type RepositoryClient struct {
 	repository.Client
-	ProviderType string
-	ProviderName string
+	VariableSubstitution bool
+	ProviderType         string
+	ProviderName         string
 }
 
 // Components provide access to YAML file for creating provider components.
@@ -36,8 +37,9 @@ func (rc *RepositoryClient) Components() repository.ComponentsClient {
 	log.Debugf("Setting up airshipctl provider Components client\n"+
 		"Provider type: %s, name: %s\n", rc.ProviderType, rc.ProviderName)
 	return &ComponentsClient{
-		client:       rc.Client.Components(),
-		providerName: rc.ProviderName,
-		providerType: rc.ProviderType,
+		client:               rc.Client.Components(),
+		providerName:         rc.ProviderName,
+		providerType:         rc.ProviderType,
+		variableSubstitution: rc.VariableSubstitution,
 	}
 }

pkg/clusterctl/implementations/repository_client_test.go

@@ -15,10 +15,13 @@
 package implementations
 
 import (
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
 	clusterctlconfig "sigs.k8s.io/cluster-api/cmd/clusterctl/client/config"
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/client/repository"
@@ -27,14 +30,153 @@ import (
 )
 
 func TestRepositoryClient(t *testing.T) {
+	airRepoClient := testRepoClient(testRepoOpts{
+		kustRoot:       "functions/4",
+		envVars:        false,
+		additionalVars: map[string]string{},
+	}, t)
+	// get the components of the repository with empty options, all defaults should work
+	// SkipVariables is to true, to make sure that it is ignored in this implementation, and instead
+	// taken from airship clusterctl provider option, which disables var substitution by default
+	c, err := airRepoClient.Components().Get(repository.ComponentsOptions{SkipVariables: true})
+	require.NoError(t, err)
+	// No errors must be returned since there is are no variables that need to be substituted
+	assert.NotNil(t, c)
+	// Make sure that target namespace is the same as defined by repository implementation bundle
+	assert.Equal(t, "newnamespace", c.TargetNamespace())
+	// Make sure that variables for substitution are actually found
+	require.Len(t, c.Variables(), 1)
+	// make sure that variable name is correct
+	assert.Equal(t, "PROVISIONING_IP", c.Variables()[0])
+}
+
+func TestMissingVariableRepoClient(t *testing.T) {
+	airRepoClient := testRepoClient(testRepoOpts{
+		kustRoot:        "functions/5",
+		envVars:         true,
+		additionalVars:  map[string]string{},
+		varSubstitution: true,
+	}, t)
+	envVars := map[string]string{
+		"AZURE_SUBSCRIPTION_ID_B64": "c29tZS1iYXNlNjQtSUQtdGV4dAo=",
+		"AZURE_TENANT_ID_B64":       "c29tZS1iYXNlNjQtVEVOQU5ULUlELXRleHQK",
+		"AZURE_CLIENT_ID_B64":       "c29tZS1iYXNlNjQtQ0xJRU5ULUlELXRleHQK",
+	}
+	for key, val := range envVars {
+		os.Setenv(key, val)
+		defer os.Unsetenv(key)
+	}
+	c, err := airRepoClient.Components().Get(repository.ComponentsOptions{})
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), `value for variables [AZURE_CLIENT_SECRET_B64] is not set`)
+	assert.Nil(t, c)
+}
+
+func TestEnvVariableSubstiutionRepoClient(t *testing.T) {
+	airRepoClient := testRepoClient(testRepoOpts{
+		kustRoot:        "functions/5",
+		envVars:         true,
+		additionalVars:  map[string]string{},
+		varSubstitution: true,
+	}, t)
+	envVars := map[string]string{
+		"AZURE_SUBSCRIPTION_ID_B64": "c29tZS1iYXNlNjQtSUQtdGV4dAo=",
+		"AZURE_TENANT_ID_B64":       "c29tZS1iYXNlNjQtVEVOQU5ULUlELXRleHQK",
+		"AZURE_CLIENT_ID_B64":       "c29tZS1iYXNlNjQtQ0xJRU5ULUlELXRleHQK",
+		"AZURE_CLIENT_SECRET_B64":   "c29tZS1iYXNlNjQtQ0xJRU5ULVNFQ1JFVC10ZXh0Cg==",
+	}
+	for key, val := range envVars {
+		os.Setenv(key, val)
+		defer os.Unsetenv(key)
+	}
+	c, err := airRepoClient.Components().Get(repository.ComponentsOptions{})
+	require.NoError(t, err)
+	assert.NotNil(t, c)
+	assert.Len(t, c.Variables(), len(dataKeyMapping()))
+	// find secret containing env variables
+	for _, obj := range c.InstanceObjs() {
+		if obj.GetKind() == "Secret" {
+			cm := &v1.ConfigMap{}
+			err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), cm)
+			require.NoError(t, err)
+			for key, expectedVal := range envVars {
+				dataKey, exists := dataKeyMapping()[key]
+				require.True(t, exists)
+				actualVal, exists := cm.Data[dataKey]
+				require.True(t, exists)
+				assert.Equal(t, expectedVal, actualVal)
+			}
+		}
+	}
+}
+
+// This test covers a case, where we want some variables to be substituted and some
+// are not. Clusterctl behavior doesn't allow to skip variable substitution completely
+// instead if SkipVariables is set to True, it will not throw errors if these variables
+// are not set in config reader.
+func TestAdditionalVariableSubstiutionRepoClient(t *testing.T) {
+	vars := map[string]string{
+		"AZURE_SUBSCRIPTION_ID_B64": "c29tZS1iYXNlNjQtSUQtdGV4dAo=",
+		"AZURE_TENANT_ID_B64":       "c29tZS1iYXNlNjQtVEVOQU5ULUlELXRleHQK",
+		"AZURE_CLIENT_ID_B64":       "c29tZS1iYXNlNjQtQ0xJRU5ULUlELXRleHQK",
+	}
+	notSubstitutedVars := map[string]string{
+		"AZURE_CLIENT_SECRET_B64": "${AZURE_CLIENT_SECRET_B64}",
+	}
+	airRepoClient := testRepoClient(testRepoOpts{
+		kustRoot:       "functions/5",
+		envVars:        false,
+		additionalVars: vars,
+		// set to false so errors are not thrown when AZURE_CLIENT_SECRET_B64 is not found
+		varSubstitution: false,
+	}, t)
+	c, err := airRepoClient.Components().Get(repository.ComponentsOptions{})
+	require.NoError(t, err)
+	assert.NotNil(t, c)
+	assert.Len(t, c.Variables(), len(dataKeyMapping()))
+	// find secret containing env variables
+	for _, obj := range c.InstanceObjs() {
+		if obj.GetKind() == "Secret" {
+			// merge two maps
+			mergedVars := map[string]string{}
+			for k, v := range vars {
+				mergedVars[k] = v
+			}
+			for k, v := range notSubstitutedVars {
+				mergedVars[k] = v
+			}
+			cm := &v1.ConfigMap{}
+			err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), cm)
+			require.NoError(t, err)
+			for key, expectedVal := range vars {
+				dataKey, exists := dataKeyMapping()[key]
+				require.True(t, exists)
+				actualVal, exists := cm.Data[dataKey]
+				require.True(t, exists)
+				assert.Equal(t, expectedVal, actualVal)
+			}
+		}
+	}
+}
+
+type testRepoOpts struct {
+	kustRoot        string
+	envVars         bool
+	additionalVars  map[string]string
+	varSubstitution bool
+}
+
+func testRepoClient(opts testRepoOpts, t *testing.T) repository.Client {
 	providerName := "metal3"
 	providerType := "InfrastructureProvider"
 	// this version contains a variable that is suppose to be substituted by clusterctl
 	// and we will test if the variable is found and not substituted
 	versions := map[string]string{
-		"v0.2.3": "functions/4",
+		"v0.2.3": opts.kustRoot,
 	}
-	options := &airshipv1.Clusterctl{
+	cctl := &airshipv1.Clusterctl{
+		AdditionalComponentVariables: opts.additionalVars,
+		EnvVars:                      opts.envVars,
 		Providers: []*airshipv1.Provider{
 			{
 				Name:     providerName,
@@ -45,7 +187,7 @@ func TestRepositoryClient(t *testing.T) {
 		},
 	}
 	// create instance of airship reader interface implementation for clusterctl and inject it
-	reader, err := NewAirshipReader(options)
+	reader, err := NewAirshipReader(cctl)
 	require.NoError(t, err)
 	require.NotNil(t, reader)
 	optionReader := clusterctlconfig.InjectReader(reader)
@@ -66,19 +208,19 @@ func TestRepositoryClient(t *testing.T) {
 	repoClient, err := repository.New(provider, configClient, optionsRepo)
 	require.NoError(t, err)
 	require.NotNil(t, repoClient)
-	// create airship implementation of clusterctl repository client
-	airRepoClient := RepositoryClient{
-		Client: repoClient,
+	return &RepositoryClient{
+		ProviderName:         providerName,
+		ProviderType:         providerType,
+		Client:               repoClient,
+		VariableSubstitution: opts.varSubstitution,
+	}
+}
+
+func dataKeyMapping() map[string]string {
+	return map[string]string{
+		"AZURE_SUBSCRIPTION_ID_B64": "subscription-id",
+		"AZURE_TENANT_ID_B64":       "tenant-id",
+		"AZURE_CLIENT_ID_B64":       "client-id",
+		"AZURE_CLIENT_SECRET_B64":   "client-secret",
 	}
-	// get the components of the repository with empty options, all defaults should work
-	c, err := airRepoClient.Components().Get(repository.ComponentsOptions{})
-	require.NoError(t, err)
-	// No errors must be returned since there is are no variables that need to be substituted
-	assert.NotNil(t, c)
-	// Make sure that target namespace is the same as defined by repository implementation bundle
-	assert.Equal(t, "newnamespace", c.TargetNamespace())
-	// Make sure that variables for substitution are actually found
-	require.Len(t, c.Variables(), 1)
-	// make sure that variable name is correct
-	assert.Equal(t, "PROVISIONING_IP", c.Variables()[0])
 }

pkg/clusterctl/implementations/testdata/functions/5/azure-resources.yaml

@@ -0,0 +1,29 @@
+## contains a namespace that should be idenitifed by components interface,
+---
+apiVersion: clusterctl.cluster.x-k8s.io/v1alpha3
+kind: Metadata
+metadata:
+  name: repository-metadata
+releaseSeries:
+- major: 0
+  minor: 3
+  contract: v1alpha3
+- major: 0
+  minor: 2
+  contract: v1alpha2
+---
+kind: Namespace
+metadata:
+  name: newnamespace
+---
+apiVersion: v1
+kind: Secret
+metadata:
+  name: manager-bootstrap-credentials
+  namespace: system
+type: Opaque
+data:
+  subscription-id: ${AZURE_SUBSCRIPTION_ID_B64}
+  tenant-id: ${AZURE_TENANT_ID_B64}
+  client-id: ${AZURE_CLIENT_ID_B64}
+  client-secret: ${AZURE_CLIENT_SECRET_B64}

pkg/clusterctl/implementations/testdata/functions/5/kustomization.yaml

@@ -0,0 +1,3 @@
+# Contains variables that should be substituted
+resources:
+ - azure-resources.yaml