clusterctl generate not accepting environment variable

[lentzi90]

One of the variables in CAPM3 infrastructure-components.yaml does not seem to recognize the environment variable, but happily takes the value from clusterctl.yaml.

What steps did you take and what happened:

# Download the infrastructure-components.yaml for CAPM3
curl -sLO https://github.com/metal3-io/cluster-api-provider-metal3/releases/download/v1.3.0/infrastructure-components.yaml
# Check that there is a variable for "enableBMHNameBasedPreallocation".
clusterctl generate yaml --from infrastructure-components.yaml --list-variables
# Check the yaml file itself. It should show a flag like this: 
# --enableBMHNameBasedPreallocation=${enableBMHNameBasedPreallocation:=false}
grep enableBMHNameBasedPreallocation infrastructure-components.yaml
# Check that the default works as expected (flag is set to false)
clusterctl generate yaml --from infrastructure-components.yaml | grep enableBMHNameBasedPreallocation
# Try setting it to true by using environment variables
export enableBMHNameBasedPreallocation="true"
clusterctl generate yaml --from infrastructure-components.yaml | grep enableBMHNameBasedPreallocation
# Result: It is still set to false!

# Try setting the variable through clusterctl.yaml instead.
unset enableBMHNameBasedPreallocation
echo "enableBMHNameBasedPreallocation: true" > clusterctl.yaml
clusterctl --config clusterctl.yaml generate yaml --from infrastructure-components.yaml | grep enableBMHNameBasedPreallocation
# Result: It is set to true!

What did you expect to happen:

The flag should take the value from the environment variable (and it should have higher priority than the value in clusterctl.yaml).

Anything else you would like to add:

The other variable in this file (CAPM3_FAST_TRACK) works as expected and I cannot tell why it is in any way different.

Environment:

• Cluster-api version: v1.3.3
• minikube/kind version: -
• Kubernetes version: (use kubectl version): -
• OS (e.g. from /etc/os-release): Ubuntu 22.04

/kind bug
/area clusterctl

[sbueringer]

Interesting. Thx for reporting!

cc @ykakarap @Jont828

[fabriziopandini]

/triage accepted
/help

[k8s-ci-robot]

@fabriziopandini:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/triage accepted
/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[aniruddha2000]

Investigating the code I found there is something wrong with this part -

https://github.com/kubernetes-sigs/cluster-api/blob/main/cmd/clusterctl/client/yamlprocessor/simple_processor.go#L122-L125

tmp, err = envsubst.Eval(tmp, func(in string) string {
-	v, _ := variablesClient(in)
+       v := os.Getenv(in)
	return v
})

replacing with os.Getenv() gets the env var successfully and if I run

$ export enableBMHNameBasedPreallocation="true"
$ ./bin/clusterctl generate yaml --from infrastructure-components.yaml | grep enableBMHNameBasedPreallocation
        - --enableBMHNameBasedPreallocation=true

There may be some issue with input.ConfigVariablesClient.Get at clusterctl/client/repository/template.go#L116.

But actually, I am not sure if we are using this clusterctl/client/config/reader_memory.go#L63-L68 Get method or not.

further investigating looks like it's a viper clusterctl/client/config/reader_viper.go#L173-L178 issue

tmp, err = envsubst.Eval(tmp, func(in string) string {
		v, err := variablesClient(in)
+		if err != nil {
+			log.Println(err)
+		}
		return v
})

$ export enableBMHNameBasedPreallocation="true"
$ ./bin/clusterctl generate yaml --from infrastructure-components.yaml | grep enableBMHNameBasedPreallocation
2023/02/28 22:53:34 Failed to get value for variable "CAPM3_FAST_TRACK". Please set the variable value using os env variables or using the .clusterctl config file
2023/02/28 22:53:34 Failed to get value for variable "enableBMHNameBasedPreallocation". Please set the variable value using os env variables or using the .clusterctl config file
        - --enableBMHNameBasedPreallocation=false

[LuBingtan]

By reading the code in viper, I think this is an issue caused by viper.

When viper gets the environment variable, it will convert the environment variable name to uppercase.

Before looking for environment variables, viper will process the env key string: https://github.com/spf13/viper/blob/master/viper.go#L1306

		if val, ok := v.getEnv(v.mergeWithEnvPrefix(envKey)); ok {
			return val
		}

strings.ToUpper will convert the key to uppercase: https://github.com/spf13/viper/blob/master/viper.go#L526

func (v *Viper) mergeWithEnvPrefix(in string) string {
	if v.envPrefix != "" {
		return strings.ToUpper(v.envPrefix + "_" + in)
	}

	return strings.ToUpper(in)
}

So that in this case, viper can not find the environment variable enableBMHNameBasedPreallocation which contains lowercase letters.

[LuBingtan]

I'm not sure if this is an expected behavior or not. If we need to fix it, I'd like to take this up 😄

[fabriziopandini]

If the problem is in viper assuming the env variables names are all upper case, IMO this should be fixed there.

Also the fix proposed above IMO is not correct, because we intentionally use the variableClient everywhere, and the default VariableClient implementation relies on viper to allow users to define clusterctl variables both as env variables or as an entry in the clusterctl config file

What we can do at this state in CAPI is to document this viper behavior somewhere in the book, e.g. https://cluster-api.sigs.k8s.io/clusterctl/configuration.html#variables

[musaprg]

Let me take this up since it looks like a tiny fix of the docs.

/assign
/lifecycle active