From a4107e7f1db7e920f6b6bf80d275cd02793bf241 Mon Sep 17 00:00:00 2001 From: Kostiantyn Kalynovskyi Date: Tue, 7 Jul 2020 16:20:17 -0500 Subject: [PATCH] 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 | 11 ++ pkg/clusterctl/client/factory.go | 7 +- .../implementations/components_client.go | 16 +- pkg/clusterctl/implementations/errors.go | 9 + pkg/clusterctl/implementations/reader.go | 47 ++++- .../implementations/repository_client.go | 12 +- .../implementations/repository_client_test.go | 176 ++++++++++++++++-- .../testdata/functions/5/azure-resources.yaml | 29 +++ .../testdata/functions/5/kustomization.yaml | 3 + 9 files changed, 274 insertions(+), 36 deletions(-) create mode 100644 pkg/clusterctl/implementations/testdata/functions/5/azure-resources.yaml create mode 100644 pkg/clusterctl/implementations/testdata/functions/5/kustomization.yaml diff --git a/pkg/api/v1alpha1/clusterctl_types.go b/pkg/api/v1alpha1/clusterctl_types.go index ca5b21300..e5a283dde 100644 --- a/pkg/api/v1alpha1/clusterctl_types.go +++ b/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 diff --git a/pkg/clusterctl/client/factory.go b/pkg/clusterctl/client/factory.go index d80476222..af79cbdea 100644 --- a/pkg/clusterctl/client/factory.go +++ b/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, diff --git a/pkg/clusterctl/implementations/components_client.go b/pkg/clusterctl/implementations/components_client.go index 14126b2da..87bcb4641 100644 --- a/pkg/clusterctl/implementations/components_client.go +++ b/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) } diff --git a/pkg/clusterctl/implementations/errors.go b/pkg/clusterctl/implementations/errors.go index 98e1f2f8b..f82a6532b 100644 --- a/pkg/clusterctl/implementations/errors.go +++ b/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) +} diff --git a/pkg/clusterctl/implementations/reader.go b/pkg/clusterctl/implementations/reader.go index e48d706fe..6b82fc828 100644 --- a/pkg/clusterctl/implementations/reader.go +++ b/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 } diff --git a/pkg/clusterctl/implementations/repository_client.go b/pkg/clusterctl/implementations/repository_client.go index 8c88a5962..06474dc73 100644 --- a/pkg/clusterctl/implementations/repository_client.go +++ b/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, } } diff --git a/pkg/clusterctl/implementations/repository_client_test.go b/pkg/clusterctl/implementations/repository_client_test.go index aba3ca050..1dfbfdda9 100644 --- a/pkg/clusterctl/implementations/repository_client_test.go +++ b/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]) } diff --git a/pkg/clusterctl/implementations/testdata/functions/5/azure-resources.yaml b/pkg/clusterctl/implementations/testdata/functions/5/azure-resources.yaml new file mode 100644 index 000000000..9e6934326 --- /dev/null +++ b/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} \ No newline at end of file diff --git a/pkg/clusterctl/implementations/testdata/functions/5/kustomization.yaml b/pkg/clusterctl/implementations/testdata/functions/5/kustomization.yaml new file mode 100644 index 000000000..35440e624 --- /dev/null +++ b/pkg/clusterctl/implementations/testdata/functions/5/kustomization.yaml @@ -0,0 +1,3 @@ +# Contains variables that should be substituted +resources: + - azure-resources.yaml