[clusterctl] Allow providers to specify variable defaults for templates

[CecileRobertMichon]

User Story

As an infrastructure provider, I would like to be able to provide optional variables in infrastructure-components.yaml and in cluster-template.yaml that a user can choose to specify or use the default to avoid growing the number of required variables while providing flexibility to the user.

Detailed Description

For example, see kubernetes-sigs/cluster-api-provider-azure#649 (comment). CAPZ would like to support creating clusters in sovereign clouds (eg. AzureChina) but by default the environment should be "AzurePublicCloud". Currently our only options are 1) break the user by adding a new AZURE_ENVIRONMENT variable or 2) make the feature only available via clusterctl override, not user friendly.

It would be good to be able to provide provider-specific defaults as part of the github release assets (maybe even embedded in the templates) that clusterctl is able to proces as part of the default yaml processor.

[A clear and concise description of what you want to happen.]

Anything else you would like to add:

This is somewhat related to #3115, but I am asking to have built in defaulting in the default clusterctl template processing. At the moment, I am not interested in writing my own template processor just for defaulting and would like defaults to be available as part of the clusterctl quickstart experience.

[Miscellaneous information that will assist in solving the issue.]

/kind feature

[CecileRobertMichon]

/cc @nader-ziada @devigned

[nader-ziada]

@wfernandes what do you think?

[fabriziopandini]

my two cents:
TBH I always have tried to avoid extending the current variable substitution because it could quickly lead to introducing and maintaining our own very opinionated templating syntax and maintaining our own templating code.

However, considering if this can help the Cluster API users, I will be happy to reconsider the topic if there is a simple proposal and we can clearly define the scope/limitations in order to control the above risks.

[CecileRobertMichon]

How about simply using something like envsubst to sub in environment variables within clusterctl? That way providers can optionally provide defaults like ${AZURE_ENVIRONMENT-AzurePublicCloud} directly within the templates without breaking other providers that decide not to define defaults (and the vars that stay as ${var} are still required). Not looking for anything more complex in terms of templating syntax and definitely don't want it to become too opinionated.

[fabriziopandini]

If the scope is to support -, :- (or =, :=) following by a const value, I think this is feasible (TBD how to properly treat the const value e.g. for trailing spaces, that now in clusterctl are tolerated but ignored)

[vincepri]

We can consider using https://github.com/a8m/envsubst

[wfernandes]

It looks like this can be easily spiked out using the YamlProcessor interface to flush out the behavior and current expectations from clusterctl. I'm happy to assist here if needed.

If the community decides on the convention we want to follow we can swap it with the SimpleYamlProcessor or do some kind of transition. But it seems that envsubst looks minimal enough to support and doesn't need additional default values files. Great suggestion @CecileRobertMichon!

Also as a note, the proposal #3052 was only focusing on the cluster templates and not the infrastructure provider components. However, the PR #3115 also makes the processor available to the infrastructure providers.
Does CAPZ have a use case for templating the infrastructure components as well? @nader-ziada @CecileRobertMichon

As @fabriziopandini pointed out, if we want to limit the envsubst behavior to const values

cloud: ${AZURE_ENVIRONMENT:=AzurePublicCloud}

We may need some extra validation to block out something like

cloud: ${AZURE_ENVIRONMENT:=$SOME_OTHER_VAL}

[vincepri]

/milestone v0.3.x

[vincepri]

/priority important-soon
/help

@CecileRobertMichon Is there someone from your team that could pick this up? Seems Nader and Warren are on it :)

[k8s-ci-robot]

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

Please ensure the request meets the requirements listed here.

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

In response to this:

/priority important-soon
/help

@CecileRobertMichon Is there someone from your team that could pick this up?

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.

[ncdc]

/assign @nader-ziada @wfernandes

[ncdc]

/help cancel

[nader-ziada]

@wfernandes and me looked into using envsubst, and the TLDR is it's a lot bigger change than we expected.

the library doesn't really expose any detailed func(s) that would return a list of variables, its has a limited set of public func https://godoc.org/github.com/a8m/envsubst and doesn't really fit with how clusterctl works. With respect to the Azure PR mentioned here, clusterctl only does substitution for templates and not for infrastructure_components.yaml which is what would be needed for the AZURE_ENVIRONMENT variable.

[CecileRobertMichon]

clusterctl only does substitution for templates and not for infrastructure_components.yaml

How does clusterctl get the credential variables that are in infrastructure_components.yaml then?

Also most of the variables that can be defaulted for infra providers are in the templates, not in infrastructure_components.yaml (since that mostly has user specific credentials), so even being able to provide defaults just for the templates would be a good start.

Why do we need the list of variables? Can't we just process the template as a file/string/byte right when it is fetched from file/url before doing any other processing, eg. buf, err := envsubst.ReadFile("cluster-template.yaml")?

[nader-ziada]

the new change to add templating are geared towards the templates and not the infrastructure_components.yaml, the infrastructure components will still get values but its was not planned to have any templating.

Why do we need the list of variables? Can't we just process the template as a file/string/byte right when it is fetched from file/url before doing any other processing, eg. buf, err := envsubst.ReadFile("cluster-template.yaml")?

The issue with the doing that using envsubst library is that it only accepts values from environment variables so it would ignore the clusterctl config file and if it doesn't find a value for a variable, it would replace it with an empty string. So the file would be treated as a string and there would be no way for clusterctl to replace/override values from the command line

@wfernandes please correct me here if I got this wrong

[vincepri]

@nader-ziada @wfernandes I tried out a different envsubst library, which permits to give your own mapping function. It's closer to what the stdlib does, and we could use it as a starting point, the library also offers the envsubst/parse library that can be used directly as well. It'd parse the file for us, and we can have a custom env template that doesn't allow for empty (unset) values.

See https://play.golang.org/p/1-QFyL3OzPK.

[wfernandes]

The drone/envsubst library could work.
See spike below.
https://play.golang.org/p/exlOO2Z4dMN

However, a couple of questions.

1. Are there any considerations for including this library?
2. If we do decide this is something we want, should we be creating a CAEP for this work? Since it will be updating the behavior of clusterctl?

[vincepri]

Are there any considerations for including this library?

I can't think of any, the license allows us to use it as a library, @detiber thoughts?

If we do decide this is something we want, should we be creating a CAEP for this work? Since it will be updating the behavior of clusterctl?

Might not be worth a full proposal, unless I'm missing something — seems it's mostly additions and the behavior should be backward compatible. We might need to add more to the book though.

[CecileRobertMichon]

If we do decide this is something we want, should we be creating a CAEP for this work? Since it will be updating the behavior of clusterctl?

Would it be a breaking change? If so, then yes. But I would expect this to be backwards compatible (ie. existing users can still use the same templates without defaults and it would still work), in which case I don't think a CAEP is needed.

+1 on documenting the new usage

[detiber]

Are there any considerations for including this library?

I can't think of any, the license allows us to use it as a library, @detiber thoughts?

As long as it meets the CNCF whitelist policy, then it should be fine: https://docs.google.com/presentation/d/1tbH15qF7bXOp8veRKHlWSeH6w1NWkMN3qmIwB6LvvgU/edit#slide=id.p1

[wfernandes]

Looks like it fulfills the requirements. Unless there are any other community concerns I don't mind working on this.
However, I suggest we get #3115 merged in before we do any work on this. 😄
Also, we should suggest them updating those slide to be called CNCF Allowlist Policy 😉

[detiber]

Also, we should suggest them updating those slide to be called CNCF Allowlist Policy wink

Indeed, I'm not sure if there is a newer version available elsewhere, I had to dig through quite a few old k8s issues across various repos to dig that copy up 😬

[wfernandes]

/assign
Going to look into this and see if we can get a first pass for supporting certain envsubst syntax as discussed above.

[wfernandes]

/lifecycle active

[jsturtevant]

For anyone that lands here looking for the CNCF allow list policy the latest is: https://github.com/cncf/foundation/blob/master/allowed-third-party-license-policy.md

They updated wording to allow list ❤️