terraform 0.12 support

[messiahUA]

terraform 0.12 is out, so it would be nice to have a valid output in kops.

Currently, "terraform 0.12checklist" reports one problem:

After analyzing this configuration and working directory, we have identified some necessary steps that we recommend you take before upgrading to Terraform v0.12:

• resource "aws_route" "0-0-0-0--0" has a name that is not a valid identifier.

In Terraform 0.12, resource names must start with a letter. To fix this, rename the resource in the configuration and then use terraform state mv to mirror that name change in the state.

[rifelpet]

I agree, it would be good to revisit #6329 now that 0.12 is stable.

It might be tricky to rename resources in Kops' terraform output since terraform would destroy and recreate the route which could lead to unacceptable downtime, unless the user uses terraform state mv as mentioned.

[ngortheone]

I have switched to Terraform 0.12 as soon as stable release came out. There are plenty of rough edges and bugs still. My recommendation would be to wait for at least a couple of bug-fix releases.

[macropin]

@ngortheone Terraform 0.12.2 is out. So it's time...

[demisx]

We've been using Terraform 0.12 (currently on 0.12.7) and so far it appears pretty solid. No issues.

[lukasbernat]

it would be nice to get this sorted.

[wizard580]

Ping!

[austinorth]

Would love to see some movement on this if anyone has time to get it sorted out!

[xxfatumxx]

Problem is getting serious https://www.hashicorp.com/blog/deprecating-terraform-0-11-support-in-terraform-providers/

[austinorth]

@rifelpet do you know of anyone working on this currently?

[rifelpet]

I do not. I did learn that the original HCL2 implementation for terraform 0.12 has been merged back into the original HCL repo so it may be easier to implement the upgrade natively rather than using strings.Replace workarounds but I haven't had a chance to experiment with that. If anyone is interested in taking that on I can attempt to provide some guidance.

[fred-vogt]

@rifelpet - I'd be interested in taking this on. Looks like office hours not this week, but instead next Friday.

Presumably this involves:

• a command line flag to set desired TF output version
• updates to upup/pkg/fi/cloudup/terraform/hcl_printer.go

Posting some guidance here would be much appreciated.

[rifelpet]

@fred-vogt Thanks for volunteering!

Yes, we'd want some way to toggle it:

I'm debating between either a new value for --target, such as terraform012 or something:

kops/cmd/kops/update_cluster.go

Line 111 in 07adf0c

cmd.Flags().StringVar(&options.Target, "target", options.Target, "Target - direct, terraform, cloudformation")

kops/upup/pkg/fi/cloudup/target.go

Lines 19 to 22 in 298f796

	const TargetDirect = "direct"
	const TargetDryRun = "dryrun"
	const TargetTerraform = "terraform"
	const TargetCloudformation = "cloudformation"

or a new feature flag which would be set by environment variable.

I would lean towards the feature flag because it would easier to migrate everyone from <0.12 to 0.12. Users would opt-in to 0.12 with an environment variable and eventually we could flip the default value and later drop support for <0.12. we dont need to provide support for <0.12 long term because Terraform 0.11 itself wont be supported long term (that's a guess but we could try to confirm with Hashicorp).

Beyond that I believe only the files in the terraform package you linked to will require updating. all of the cloudup tasks themselves reference that package, so the majority of the change should be within there.

I didnt see any examples in the HCL2 branch but the integration tests might be helpful.

[fred-vogt]

@rifelpet - Thanks for the info. I'll get started.

Will pursue enablement via KOPS_FEATURE_FLAGS / pkg/featureflag/featureflag.go.

I'm thinking maybe if this if feature flagged off by default we don't have to update the kubernetes.tf files under:

• tests/integration/update_cluster
  Until after TF 0.12 becomes the default.

[rifelpet]

I agree we wont need to update the existing integration tests until we switch the default value of the feature flag, but we should add an integration test case that does use the terraform 0.12 output.

[fred-vogt]

@rifelpet - Some findings:

• the API in HCL v2 is different than v1 but it seems that both can co-exist in the same code base go modules
  • parallel import aliases
  • parallel code paths guarded by featureflag.Terraform012.Enabled() wherever the HCL APIs are used

Some concerns:

• unsure how to properly update vendor/ directory.
• go.mod, go.sum - seems that some existing deps were modified after running go get github.com/hashicorp/hcl/v2

The current approach KOPS uses isn't compatible with HCL v2.

HCL v2 doesn't support tools that want to go from JSON -> HCL using their library.

• with HCL v1 go -> json -> ast -> hcl was possible.
• with HCL v2 its not, only go -> json -> (read-only) ast.
  • there is a package hclwrite but it doesn't understand the ast from the JSON parser nor can it parse JSON.

This is a bummer.

For now tried a hybrid of HCL v1+v2.

Explored this a bit. I was able to get TF 12 output by using HCL v1 to go from JSON -> HCL -> bytes/string to fixup bytes/string -> HCL v2.

Results: 7052/tf-12-feature-flag-01

[fred-vogt]

@rifelpet - Another approach could be to "fixup" the unquoted Tag keys with regexes instead of modifying all the AWS tasks to render the alternative tag defs:

• 7052/tf-12-feature-flag-02

terraform < 0.12

resource "aws_ebs_volume" "..." {
...
  tags = {
    KubernetesCluster                                  = "..."
    Name                                               = "....etcd-events..."
    "k8s.io/etcd/events"                               = ".../...,..."
    "k8s.io/role/master"                               = "1"
    "kubernetes.io/cluster/additionalcidr.example.com" = "owned"
  }
}

terraform 0.12

resource "aws_ebs_volume" "..." {
...
  tags {
    "KubernetesCluster"                                  = "..."
    "Name"                                               = "....etcd-events..."
    "k8s.io/etcd/events"                               = ".../...,..."
    "k8s.io/role/master"                               = "1"
    "kubernetes.io/cluster/additionalcidr.example.com" = "owned"
  }
}

[fred-vogt]

@rifelpet - As for as the resource names issue (identifiers can't start with numbers in TF 12):

• 7052/tf-12-feature-flag-02

• default route / additional cidr-blocks have this issue.

• Terraform 0.11.14 has a 0.12checklist command

• Terraform 0.12 has an 0.12upgrade command

0.12checklist

for test in $(find tests/integration/update_cluster -type d -depth 1); do
    echo "$test"
    (cd "$test"; terraform-0.11.14 0.12checklist)
done

> complains about several items: tests/integration/update_cluster/lifecycle_phases

0.12upgrade

I'll look into how this is implemented. It would be nice to use this to support TF 12 in kops.

[fred-vogt]

Located the terraform upgrade implementation:

• https://github.com/hashicorp/terraform/blob/master/command/012_config_upgrade.go
  • https://github.com/hashicorp/terraform/blob/master/configs/configupgrade/upgrade.go
    • https://github.com/hashicorp/terraform/blob/master/configs/configupgrade/upgrade_native.go

configs/configupgrade/upgrade_native.go => something like this is what we'd need to convert the parsed JSON ->; HCLv1 to HCLv2 for output.

We'd need only to tweak the invalid identifiers for default route / additional VPC-ipv4 cidr blocks.

[rifelpet]

I'm thinking we could adjust the invalid identifiers in a separate PR, perhaps first. It will need to be done regardless and technically isnt tightly coupled to the 0.12 upgrade itself. I'm working on a proposal for that now but learned there are a handful of other cloud providers in Kops that support outputting to terraform, so i'm auditing those as well.

[mccare]

@fred-vogt terraform 0.12 understands json as configuration (https://www.terraform.io/docs/configuration/syntax-json.html). I've tested this succesfully with the GCE (and AWS) provider.

https://github.com/mccare/kops/blob/e9e249b98807213b36728f9c80ccde346ea9246c/upup/pkg/fi/cloudup/terraform/target.go#L281-L284

	if featureflag.Terraform012.Enabled() {
		t.files["kubernetes.tf.json"] = jsonBytes
	} else {
		f, err := hcl_parser.Parse(jsonBytes)

[rifelpet]

Good news, the first stage of 0.12 support has landed and will be in Kops 1.17. This is the renaming of certain resources to support 0.12. I've added some release notes here, if anyone is willing to review the steps I've documented that would be much appreciated. You can also built a custom kops binary from either master or release-1.17 branches to test it out yourself.

Next step will be the 0.12 JSON support in #8145 which will hopefully land in 1.18. After that we can focus on native hcl2 support.

[termie]

I've had to use this script to fix the output of the terraform stuff (running off a build of master)

  kops update cluster $1 --out=./$1 --target=terraform
  
  sed -e "s/tag = {/tag {/" -i $1/kubernetes.tf
  sed -e "s/locals = {/locals {/" -i $1/kubernetes.tf
  sed -e "s/lifecycle = {/lifecycle {/" -i $1/kubernetes.tf
  sed -e "s/root_block_device = {/root_block_device {/" -i $1/kubernetes.tf

i also strip out the terraform and provider fields, but i assume those need to have the = removed, too

[darose]

Any idea when TF 0.12 support might get released? 0.12 has been out for several months now, and it's becoming a bit of a nuisance having to keep hand-editing the generated TF code to make it 0.12-compliant.

[xxfatumxx]

@darose Check the latest alphas of kops 1.17

[rifelpet]

Just an update on the native HCL2 syntax effort. I have a very rough draft of what it could look like:

https://github.com/rifelpet/kops/tree/hcl2-native

You can compile this with make kops-install and run a kops update cluster --target terraform and see the new syntax temporarily being printed to stdout.

I'm using the hclwrite package which uses go-cty which requires adding field tags to all the terraform structs. This was the recommended solution from hashicorp devs, writing to HCL2 is simpler but terraform uses a syntax that isnt a 1:1 translation of HCL2, so this seems to be the best way.

It is far from functional, but you can at least see the progress being made. I had to manually fix the literal references (aws_security_group.masters-foo-k8s-local.id for example) but after that it terraform init; terraform plan successfully. Remaining todo items:

• Fix literal references including file()
• Add cty field tags to remaining cloud providers (only AWS is supported)
• Write to kubernetes.tf just like the original terraform target does
• Try to run a terraform fmt on the result
• Build out a brand new terraform0.12 target so it can live along side the terraform 0.11 target
• Add a test job to the Kops repo that validates the terraform 0.12 syntax

And if anyone is willing to help with any of this, please reach out to me on the k8s slack @peterr 🙂

[rifelpet]

Another update:

I have essentially all of the HCL2 syntax building done. I'll open a PR once #8734 and #8737 land which should be soon. Back in the Dec 6th Kops Office Hours we discussed creating a new target (--target terraform0.12 or something along those lines) but now I'm starting to wonder if using a feature flag would be better.

I think migration and deprecation of the old syntax would be mostly identical, we'd perform these steps spread across as many minor releases as desired:

1. Introduce new terraform 0.12 support
2. Flip feature flag default, mention as required action in Kops release notes (only applicable to the feature flag option)
3. Announce deprecation of <0.12 support, printed in Kops output and mentioned in release notes
4. Remove terraform target or feature flag

---

I'm sure there are many people subscribed to this issue, does anyone have strong opinions on which option we go with? I plan on bringing this back up at this Friday's office hours as well.

Feel free to vote by reacting to this comment:

• 🚀 for --target terraform0.12
• 🎉 for KOPS_FEATURE_FLAG=terraform0.12

[hakman]

Maybe terraform-0.12 with a dash?

[rifelpet]

Sure, I don't have a strong opinion on the new value used with either option.

• terraform012
• terraform0.12
• terraform-0.12

In the case of the --target option, it will permanently be the new --target value for HCL2 syntax, since I don't think replacing the behavior in --target terraform would be a good idea (it'd be better to use the feature flag option if we want to keep using --target terraform long term)

Thoughts?

[hakman]

Depends on the decision about supporting older terraform format. If we will just drop it, maybe we should just not complicate at all and add it in the 1.19 alpha.

[rifelpet]

It'd be great if terraform 0.11 had an official deprecation schedule that Kops could mimic. I'm having trouble finding one, only that eventually new provider versions will only support 0.12 which means Kops may add functionality that wouldn't work in terraform 0.11. I found this discussion implying 0.11 is no longer receiving bug fixes.

It seems like terraform 0.11 will continue to work "forever" with existing functionality. We have to decide if Kops should follow that or drop <0.12 support entirely.

[den-is]

make tf 0.12 default and hide tf 0.11 under such "ugly" additional keys --target terraformZero.Eleven :)
but actually I vote for 0.11 drop from future versions.
(wrote this message because didn't find where to vote :) )

[rifelpet]

We discussed this during office hours today and I think we settled on how we'll roll this out. My guess is that users that voted for --target over KOPS_FEATURE_FLAG was due to not wanting to set an env var in order to use the new syntax. Given Terraform 0.12's widespread use, we decided that we wont support the old syntax indefinitely. Therefor what we plan to do is use a feature flag but default it to use the new syntax.

The usage will be:

# new syntax
kops update cluster --target terraform

# old syntax (feature flag name TBD)
KOPS_FEATURE_FLAG=... kops update cluster --target terraform

Typically when we use feature flags we first default them to the existing behavior, then let them sit and fix any reported bugs before flipping the default to the new behavior. For this we feel comfortable enough to default to the new behavior when it is introduced. This allows those of you wanting the new syntax (which I assume will be the vast majority of --target terraform users by the time this is released) to not need to set any env vars or adjust any cli flags.

Given the current state of Kops releases and the previous tf.json work in Kops 1.18, My goal is to target Kops 1.19 with this.

[glesperance]

Has this been released? I'm still getting old terraform files generated with kops v16.1.

I'm using this command to generate the news files:

kops update cluster \
      --name=kubernetes.my-cloud.com \
      --state=s3://my-s3-config-bucket \
      --out=. \
      --target=terraform

[hiteshsp]

Looks like this isn't fixed yet I have seen the same old terraform 0.11 files.

I have run the below to fix that @glesperance
terraform 0.12upgrade on the terraform root directory. It converted the tf files from 0.11 to 0.12.

[darose]

Really can't wait until this issue gets fixed. It's over a year old. (And we're already up to terraform v0.12.28.)