Support terraform 0.12 hcl syntax

[sergeylanzman]

remove "=" from multi block assign
Terraform from 0.12 not support multi block assign with "=" hashicorp/terraform#19156

[k8s-ci-robot]

Hi @sergeylanzman. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[chrisz100]

/ok-to-test
/lgtm

Thanks for the fix! Looks like lots of manual work, very much appreciated!

[rifelpet]

It'd be nice if we could use the new HCL library, but the readme makes it sound like this repo is only temporary: https://github.com/hashicorp/hcl2

I guess strings.Replace may be the best short term option until the library is stable.

[justinsb]

Thanks for investigating and for fixing @sergeylanzman.

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, sergeylanzman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[justinsb]

(I assume this output works in earlier versions of terraform as well - like 0.11 ?)

[justinsb]

Looks like more tests that need to be updated to the new form.

Do we know when terraform 0.12 is landing? This feels like such a big change I'm wondering if they'll be able to force everyone to move...

[sergeylanzman]

1. I assume this output works in earlier versions of terraform as well - like 0.11 ? - yes it's work with 0.11
2. Terraform 0.12 - now it's v0.12.0-beta1
3. Them add fix option to terraform, but kops generate tf by self.
4. I found all test, and fixed them. I think....

[justinsb]

@sergeylanzman thanks - I think you might have to rebase to pick up the new test that is failing (in pull-kops-bazel-test above). We can retest but it doesn't look like a flake...

/retest

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[lorantonodipo]

resource "aws_route" "0-0-0-0--0" {
  route_table_id         = "${aws_route_table.additionalcidr-example-com.id}"
  destination_cidr_block = "0.0.0.0/0"
  gateway_id             = "${aws_internet_gateway.additionalcidr-example-com.id}"
}

From the v0.12 upgrade guide:

The tasks it may suggest you perform could include:
Renaming any resources or provider aliases that have names that start with digits, because that is no longer valid in Terraform 0.12.

[rifelpet]

Fixing the aws_route resource to not begin with a digit may be tricky because simply updating the resource name on an existing cluster's terraform definition will cause terraform to delete the route and recreate it which would be disruptive. We'll need to figure out a way to do that more gracefully or warn users that they'll need to terraform state mv the route themselves between a kops update cluster --target terraform and a terraform apply.

[demisx]

We are on Terraform 0.12.3 and just starting out with Kops. The generated kubernetes.tf is not compliant with 0.12 HCL syntax and has to be manually fixed. Any idea how the @lorantonodipo mentioned issue can be resolved as well (route starting with 0)? This is a brand new set up, so it’s not going to affect anything.

[lorantonodipo]

@demisx I simply renamed the 0-0-0-0 aws route, run terraform 0.12upgrade. After it updated the tf files, terraform plan and terraform apply worked.

[demisx]

@lorantonodipo Oh, nice! Thank you for your prompt reply. Do you have any recommendations on what this route should be named? This is what got generated for me for an existing VPC with 3 AZS and public/private subnet in each one. Will public-0-0-0-0--0 name make sense? My understanding this is a route to the internet for each public subnet.

resource "aws_route" "0-0-0-0--0" {
  route_table_id         = "${aws_route_table.stg-dl-org.id}"
  destination_cidr_block = "0.0.0.0/0"
  gateway_id             = "igw-0ed07dc33479ff787"
}

resource "aws_route" "private-us-west-2a-0-0-0-0--0" {
  route_table_id         = "${aws_route_table.private-us-west-2a-stg-dl-org.id}"
  destination_cidr_block = "0.0.0.0/0"
  nat_gateway_id         = "${aws_nat_gateway.us-west-2a-stg-dl-org.id}"
}

resource "aws_route" "private-us-west-2b-0-0-0-0--0" {
  route_table_id         = "${aws_route_table.private-us-west-2b-stg-dl-org.id}"
  destination_cidr_block = "0.0.0.0/0"
  nat_gateway_id         = "${aws_nat_gateway.us-west-2b-stg-dl-org.id}"
}

resource "aws_route" "private-us-west-2c-0-0-0-0--0" {
  route_table_id         = "${aws_route_table.private-us-west-2c-stg-dl-org.id}"
  destination_cidr_block = "0.0.0.0/0"
  nat_gateway_id         = "${aws_nat_gateway.us-west-2c-stg-dl-org.id}"
}

[lorantonodipo]

@demisx

resource "aws_route" "0-0-0-0--0" {

I just renamed this one, however I was a bit sloppy and I just renamed it to "a-0-0-0--0", your name would make more sense :)

And you understanding is correct (I think), that is the route to use the internet gateway for to get to the internet.

[rifelpet]

Regarding the route resource name incompatibility, I have one proposed fix:

rifelpet@a32c487

This would require terraform users to terraform state mv the aws_route resource as I described above. Unfortunately it has the same impact on CloudFormation and from some googling and my limited knowledge of CF, I don't think it supports renaming resources. This means CF would delete and recreate the route which may result in a blip of outgoing network interruption which is probably not acceptable.

Alternatively we rename the resource closer to the hcl rendering code, perhaps like this:

rifelpet@f7bd6dd

specifically this:

rifelpet@f7bd6dd#diff-b83a0a8f383853739ac00a4d5481e818

Writing this fix uncovered that the additionalNetworkCIDRs field's resulting aws_vpc_ipv4_cidr_block_association resource has the same issue as the aws_route. I don't have any good ideas for what to prefix the names with, public seems less appropriate given that it is used for the cidr block association as well.

Thoughts on a better prefix or better approach to renaming these two resources?

[demisx]

Since "0.0.0.0" represents the default route, perhaps it could be named default-0-0-0-0--0 or simply default?

[arnaud-devops]

Is this PR will be merged soon, we can expect it to be inside kops 1.13.0, or 1.12.3 ?

[arnaud-devops]

@sergeylanzman @justinsb @chrisz100 This PR is stalled ?
What about @demisx, @rifelpet and @lorantonodipo proposals ?

[artburkart]

Out of curiosity, I ran terraform 12.9 against a file based on the examples from this PR's tests directory. I think I could benefit from a sanity check. 😅 Thanks!

Using the following config:

resource "aws_ebs_volume" "example" {
  availability_zone = "us-east-2a"
  size              = 40

  tags {
    "hello/world" = "HelloWorld"
  }
}

$ terraform plan
Error: Invalid argument name

  on main.tf line 10, in resource "aws_ebs_volume" "example":
  10:     "hello/world" = "HelloWorld"

Argument names must not be quoted.

Based on hashicorp/terraform#19240, I assume the correct format would be:

resource "aws_ebs_volume" "example" {
  availability_zone = "us-east-2a"
  size              = 40

-  tags {
+  tags = {
    "hello/world" = "HelloWorld"
  }
}

Would this mean this PR needs an additional call to strings.Replace, so it looks like this?

s = strings.Replace(s, " = {", " {", -1)
s = strings.Replace(s, "tags {", "tags = {", -1)
...
...
...
formatted = []byte(strings.Replace(string(formatted), " = {", " {", -1))
formatted = []byte(strings.Replace(string(formatted), "tags {", "tags = {", -1))

[austinorth]

Any updates on this? Would be super helpful to have in 1.15.

[kpucynski]

bump :) whats left to do?

[k8s-ci-robot]

@sergeylanzman: PR needs rebase.

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.

[k8s-ci-robot]

@sergeylanzman: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kops-bazel-test	0d9ec00	link	/test pull-kops-bazel-test
pull-kops-verify-gomod	0d9ec00	link	/test pull-kops-verify-gomod
pull-kops-verify-generated	0d9ec00	link	/test pull-kops-verify-generated
pull-kops-verify-staticcheck	0d9ec00	link	/test pull-kops-verify-staticcheck

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[tizzo]

Any updates here? Does this just need a rebase?

[mccare]

See #7052 (comment)

[rifelpet]

This work was superseded by #8825 so I think its safe to close this out
/close

[k8s-ci-robot]

@rifelpet: Closed this PR.

In response to this:

This work was superseded by #8825 so I think its safe to close this out
/close

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.