Update Terraform resource names to be 0.12 compatible.

[rifelpet]

See #7052 for additional details.

According to the Terraform 0.12 upgrade guide resource names cannot start with digits. Currently both routes and VPC CIDR associations start with digits, so this adds prefixes to them so that they are valid resource identifiers in 0.12.

This is a significant change because on its own, terraform will destroy and recreate the route which impact the cluster networking. To avoid this, existing clusters this will require moving the resources within the terraform state prior to the next apply.

kops update cluster --target terraform --out ./
terraform state mv aws_route.0-0-0-0--0 aws_route.route-0-0-0-0--0
terraform plan
terraform apply

The exact terraform state command may vary depending on how Kops' terraform output is used.
See the command documentation for more details. Always run a terraform plan first to ensure the aws_route and aws_vpc_ipv4_cidr_block_association resources are not getting recreated.

Due to the potential impact, this notice should be very prominent in the Kops release notes.

I decided to break this out into a separate PR since it can be independent of the actual Terraform HCL syntax changes. This state mv will need to be performed regardless, and can be done independent of the actual Terraform 0.12 upgrade itself.

I confirmed that our GCE terraform output does not have any resources with this issue. I noticed that alicloud, spotinst, and digitalocean also support targeting terraform but we dont have integration tests setup for those and I'm not able to run them locally, so I'm not sure whether they have any resources that require updating.

[rifelpet]

Looking for feedback on how to best approach this...

This PR implements one option which adds the name prefix in the RenderTerraform function. This only affects the terraform output but it does affect all routes including private routes which arent actually impacted by the Terraform 0.12 limitation (as seen by the test failures). Alternatively we could add an if statement to only prefix the name if it is the default route, but that feels very hacky.

Another option is to update the name for the entire Route task:

kops/pkg/model/network.go

Lines 180 to 186 in 64f3eaa

	c.AddTask(&awstasks.Route{
	Name: s("0.0.0.0/0"),
	Lifecycle: b.Lifecycle,
	CIDR: s("0.0.0.0/0"),
	RouteTable: publicRouteTable,
	InternetGateway: igw,
	})

and have it more closely match the private route task:

kops/pkg/model/network.go

Lines 415 to 421 in 64f3eaa

	r = &awstasks.Route{
	Name: s("private-" + zone + "-0.0.0.0/0"),
	Lifecycle: b.Lifecycle,
	CIDR: s("0.0.0.0/0"),
	RouteTable: rt,
	NatGateway: ngw,
	}

but this will affect CloudFormation as well, and I don't believe CF can handle resource renaming the same way that terraform can.

Thoughts?

[rifelpet]

/test pull-kops-verify-staticcheck

[justinsb]

This LGTM.

Two questions:

• Do we need a release note? How does terraform handle the change?
• Did we decide whether we needed a separate --target=terraform-0.11 output, or something like that?

[rifelpet]

@justinsb

• Yes this will need a release note. I can create a separate PR with that once we decide how far back to cherry-pick this
• I'm thinking that we'll add official 0.12 support in multiple stages. The first stage will be supporting JSON output only, which Enabling JSON output for Terraform instead of writing the HCL syntax … #8145 via feature flag. The next step which is significantly more involved is to support outputting native HCL for 0.12 support. This requires using a new version of the hcl library and modifying every task's terraform structs. This portion can use a separate --target as we discussed in office hours a while back.

Making this change required for all terraform users even if they're still on 0.11 is fine because:

1. terraform 0.11 support is limited
2. The change will need to be made whenever the user does decide to upgrade to 0.12
3. Terraform recommends making these changes before upgrading rather than during the upgrade
4. The change is minimally invasive. The user just needs to run a few terraform state commands prior to terraform apply.

Therefor I think it's best that we roll this out first so that users have the opportunity to prepare for the upgrade. I'm thinking we could cherry-pick this back to 1.16 and have #8145 cherry-picked back to 1.17, unless you think #8145 is safe enough to be in 1.16 given that its behind a feature flag. We can adjust the release notes and terraform instructions to handle either case. Thoughts?

[justinsb]

Thanks @rifelpet - that plan (and this change) sounds great based on your explanation. Let's get this in and see how bad those state commands are (and what happens if we don't run them!) to decide on the cherry-pick. My inclination would be to go to 1.17 but not 1.16, so we can release 1.16 sooner, but let's discuss in office hours?

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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,rifelpet]

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

[justinsb]

Sorry - of course you wrote the commands in your initial message - the impact seems very reasonable - one state command, or some networking disruption as terraform destroys & creates.

[hakman]

/lgtm

[johanneswuerbach]

I'm thinking we could cherry-pick this back to 1.16 and have #8145 cherry-picked back to 1.17

Created the cherry-pick of #8145 to 1.17 here #8926 as we would love to migrate to terraform 0.12 :-)