Move variable defaults to -var-file

[cartermckinnon]

Description of changes:

These variables appear in multiple templates internally, so this will centralize the values.

I also fixed a weird spacing issue with the PACKER_VARIABLES logic. The foreach function in make will join the expanded 3rd argument with a space character. When the loop evaluates for packer variables that aren't defined, it results in a ton of extra space being inserted. Example before this change:

packer validate -var-file=version-variables.json  -var instance_type='m4.large'      -var arch='x86_64'  -var ami_name='amazon-eks-node-.-v20221102'              -var aws_region='us-west-2'        -var binary_bucket_region='us-west-2'                                           eks-worker-al2.json

and after this change:

packer validate -var-file=version-variables.json -var instance_type='m4.large' -var arch='x86_64' -var ami_name='amazon-eks-node-.-v20221102' -var aws_region='us-west-2' -var binary_bucket_region='us-west-2' eks-worker-al2.json

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Testing Done

Succeeded:

make 1.22

[cartermckinnon]

After this change, possible usages include:

1. make. This will use the values defined in the variables file.
2. make PACKER_VARIABLE_FILE=my-variable-file.json. This will allow users to define their own values for docker_version or any other variable in the template. If they don't include a required variable, like docker_version, they'll get an error.
3. make docker_version=1.2.3. This will override the value we have in our variable file. This is already how users should be providing these values.

[mmerkes]

LGTM, but let's update the README to explain how this works and how customers should override defaults.

[cartermckinnon]

@mmerkes and I discussed this -- we're going to try moving as many variables into the var-file as possible, to avoid confusion about where defaults are defined. All variables that have a default value that isn't a go template will be moved to the variable file. New rev coming.

[cartermckinnon]

Packer allows multiple -var-file arguments, and will merge the contents in the order that you specify them. I moved all the defaults into the variables file. Users can specify their own variables file to override a subset of the variables, or they can specify individual key-value variable overrides to make. (Or some combination of both).

[saurav-agarwalla]

Thanks for the change. It'll simplify a bunch of things.

[saurav-agarwalla]

What's the benefit in duplicating the variables here? Can't eks-worker-al2-variables.json be the source of truth?

[cartermckinnon]

This PR dynamically discovers the variables from the template using packer inspect, so they need to be present in the template. ☹️ Because we support overriding them in the Makefile via this mechanism, we have to maintain the list of variables somewhere (either in the Makefile or in the template).

We could pull these out of the variables file with something else like jq, but I'd prefer to use the Packer mechanism if it's reasonable.

[cartermckinnon]

This is the packer inspect usage I'm referring to:

amazon-eks-ami/Makefile

Line 2 in 8da3272

AVAILABLE_PACKER_VARIABLES := $(shell $(PACKER_BINARY) inspect -machine-readable eks-worker-al2.json | grep 'template-variable' | awk -F ',' '{print $$4}')

[saurav-agarwalla]

Would be possible to write tests for these behaviors?

[saurav-agarwalla]

Nevermind, I saw your comment stating that packer is the one which does this so no point in testing that. Contents of that comment might be good for this section too. :)

[cartermckinnon]

I'd err on the side of fewer impl details in this doc, but I don't feel too strongly. Can totally add if you think it's necessary