Require KOPS_TERRAFORM_0_12_RENAMED, to guard against tf breakage

permalinks/terraform_renamed.md

@@ -0,0 +1,73 @@
+# Terraform 0.12 Naming Compatibility
+
+Terraform 0.12 introduced new restrictions on naming, breaking
+compatibility with earlier terraform versions when resource names
+start with a number.  Single-zone etcd clusters (and possibly some
+other scenarios) would generate terraform names for EBS volumes that
+start with a number, which are no longer permitted.
+
+For new clusters, kOps now avoids this problem.  But for existing
+clusters, in order for terraform not to erase your data, a manual
+state migration is needed first.
+
+In order to prevent against data-loss, kOps will detect the problem
+and require you to pass an environment variable to indicate that you
+have performed the migration.
+
+NOTE: You must perform this migration with terraform 0.11.
+
+To do this state migration, first run `terraform state list`.
+
+You should see something like this, depending on how many
+control-plane nodes you have:
+
+```
+...
+aws_ebs_volume.1-etcd-events-foo-example-com
+aws_ebs_volume.1-etcd-main-foo-example-com
+aws_ebs_volume.2-etcd-events-foo-example-com
+aws_ebs_volume.2-etcd-main-foo-example-com
+aws_ebs_volume.3-etcd-events-foo-example-com
+aws_ebs_volume.3-etcd-main-foo-example-com
+...
+```
+
+We want to prefix each of those names with `ebs-`.
+
+A one liner to do so is:
+
+```
+terraform-0.11 state list | grep aws_ebs_volume | cut -d. -f2 | xargs -I {} terraform-0.11 state mv aws_ebs_volume.{} aws_ebs_volume.ebs-{}
+```
+
+This is equivalent to the manual form:
+
+```
+terraform-0.11 state mv aws_ebs_volume.1-etcd-events-foo-example-com aws_ebs_volume.ebs-1-etcd-events-foo-example-com
+terraform-0.11 state mv aws_ebs_volume.1-etcd-main-foo-example-com aws_ebs_volume.ebs-1-main-events-foo-example-com
+terraform-0.11 state mv aws_ebs_volume.2-etcd-events-foo-example-com aws_ebs_volume.ebs-2-etcd-events-foo-example-com
+terraform-0.11 state mv aws_ebs_volume.2-etcd-main-foo-example-com aws_ebs_volume.ebs-2-etcd-main-foo-example-com
+terraform-0.11 state mv aws_ebs_volume.3-etcd-events-foo-example-com aws_ebs_volume.ebs-3-etcd-events-foo-example-com
+terraform-0.11 state mv aws_ebs_volume.3-etcd-main-foo-example-com aws_ebs_volume.ebs-3-etcd-main-foo-example-com
+```
+
+Finally, you should repeat the kops update command passing
+`KOPS_TERRAFORM_0_12_RENAMED=ebs`.
+
+Note that you must then run `terraform init` / `terraform plan` /
+`terraform apply` using terraform 0.12.26, not terraform 0.13.
+
+Carefully review the output of `terraform plan` / `terraform apply` to
+ensure that the EBS volumes are not being deleted & recreated.  Note
+that `aws_security_group_rule` will be deleted and recreated, due to
+the same terraform naming restriction.
+
+If you encounter the "A duplicate Security Group rule..." error, you
+will likely have to run `terraform apply` twice, because of the
+terraform bug described in
+`https://github.com/hashicorp/terraform/pull/2376`
+
+Note that you must _always_ pass `KOPS_TERRAFORM_0_12_RENAMED=ebs` to
+`kops` for these clusters, as kOps otherwise has no way to know that
+the rename has been done.  However, kOps will "fail safe" and simply
+refuse to generate terraform in these cases.

tests/integration/update_cluster/complex/cloudformation.json

@@ -1116,7 +1116,7 @@
         ]
       }
     },
-    "AWSEC2Volume1aetcdeventscomplexexamplecom": {
+    "AWSEC2Volumeaetcdeventscomplexexamplecom": {
       "Type": "AWS::EC2::Volume",
       "Properties": {
         "AvailabilityZone": "us-test-1a",
@@ -1130,7 +1130,7 @@
           },
           {
             "Key": "Name",
-            "Value": "1a.etcd-events.complex.example.com"
+            "Value": "a.etcd-events.complex.example.com"
           },
           {
             "Key": "Owner",
@@ -1142,7 +1142,7 @@
           },
           {
             "Key": "k8s.io/etcd/events",
-            "Value": "1a/1a"
+            "Value": "a/a"
           },
           {
             "Key": "k8s.io/role/master",
@@ -1155,7 +1155,7 @@
         ]
       }
     },
-    "AWSEC2Volume1aetcdmaincomplexexamplecom": {
+    "AWSEC2Volumeaetcdmaincomplexexamplecom": {
       "Type": "AWS::EC2::Volume",
       "Properties": {
         "AvailabilityZone": "us-test-1a",
@@ -1169,7 +1169,7 @@
           },
           {
             "Key": "Name",
-            "Value": "1a.etcd-main.complex.example.com"
+            "Value": "a.etcd-main.complex.example.com"
           },
           {
             "Key": "Owner",
@@ -1181,7 +1181,7 @@
           },
           {
             "Key": "k8s.io/etcd/main",
-            "Value": "1a/1a"
+            "Value": "a/a"
           },
           {
             "Key": "k8s.io/role/master",

tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml

@@ -26,11 +26,11 @@ spec:
   etcdClusters:
   - etcdMembers:
     - instanceGroup: master-us-test-1a
-      name: 1a
+      name: a
     name: main
   - etcdMembers:
     - instanceGroup: master-us-test-1a
-      name: 1a
+      name: a
     name: events
   iam:
     permissionsBoundary: arn:aws:iam:00000000000:policy/boundaries

tests/integration/update_cluster/complex/in-v1alpha2.yaml

@@ -26,11 +26,11 @@ spec:
   etcdClusters:
   - etcdMembers:
     - instanceGroup: master-us-test-1a
-      name: 1a
+      name: a
     name: main
   - etcdMembers:
     - instanceGroup: master-us-test-1a
-      name: 1a
+      name: a
     name: events
   iam:
     permissionsBoundary: arn:aws:iam:00000000000:policy/boundaries

tests/integration/update_cluster/complex/kubernetes.tf

@@ -203,32 +203,32 @@ resource "aws_autoscaling_group" "nodes-complex-example-com" {
   vpc_zone_identifier = [aws_subnet.us-test-1a-complex-example-com.id]
 }
 
-resource "aws_ebs_volume" "ebs-1a-etcd-events-complex-example-com" {
+resource "aws_ebs_volume" "a-etcd-events-complex-example-com" {
   availability_zone = "us-test-1a"
   encrypted         = false
   size              = 20
   tags = {
     "KubernetesCluster"                         = "complex.example.com"
-    "Name"                                      = "1a.etcd-events.complex.example.com"
+    "Name"                                      = "a.etcd-events.complex.example.com"
     "Owner"                                     = "John Doe"
     "foo/bar"                                   = "fib+baz"
-    "k8s.io/etcd/events"                        = "1a/1a"
+    "k8s.io/etcd/events"                        = "a/a"
     "k8s.io/role/master"                        = "1"
     "kubernetes.io/cluster/complex.example.com" = "owned"
   }
   type = "gp2"
 }
 
-resource "aws_ebs_volume" "ebs-1a-etcd-main-complex-example-com" {
+resource "aws_ebs_volume" "a-etcd-main-complex-example-com" {
   availability_zone = "us-test-1a"
   encrypted         = false
   size              = 20
   tags = {
     "KubernetesCluster"                         = "complex.example.com"
-    "Name"                                      = "1a.etcd-main.complex.example.com"
+    "Name"                                      = "a.etcd-main.complex.example.com"
     "Owner"                                     = "John Doe"
     "foo/bar"                                   = "fib+baz"
-    "k8s.io/etcd/main"                          = "1a/1a"
+    "k8s.io/etcd/main"                          = "a/a"
     "k8s.io/role/master"                        = "1"
     "kubernetes.io/cluster/complex.example.com" = "owned"
   }

upup/pkg/fi/cloudup/awstasks/ebsvolume.go

@@ -18,6 +18,7 @@ package awstasks
 
 import (
 	"fmt"
+	"os"
 
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
@@ -244,18 +245,43 @@ func (_ *EBSVolume) RenderTerraform(t *terraform.TerraformTarget, a, e, changes
 		Tags:             e.Tags,
 	}
 
-	return t.RenderResource("aws_ebs_volume", e.TerraformName(), tf)
+	tfName, _ := e.TerraformName()
+	return t.RenderResource("aws_ebs_volume", tfName, tf)
 }
 
 func (e *EBSVolume) TerraformLink() *terraform.Literal {
-	return terraform.LiteralSelfLink("aws_ebs_volume", e.TerraformName())
+	tfName, _ := e.TerraformName()
+	return terraform.LiteralSelfLink("aws_ebs_volume", tfName)
 }
 
-func (e *EBSVolume) TerraformName() string {
-	if (*e.Name)[0] >= '0' && (*e.Name)[0] <= '9' {
-		return fmt.Sprintf("ebs-%v", *e.Name)
+// TerraformName returns the terraform-safe name, along with a boolean indicating of whether name-prefixing was needed.
+func (e *EBSVolume) TerraformName() (string, bool) {
+	usedPrefix := false
+	name := fi.StringValue(e.Name)
+	if name[0] >= '0' && name[0] <= '9' {
+		usedPrefix = true
+		return fmt.Sprintf("ebs-%v", name), usedPrefix
 	}
-	return *e.Name
+	return name, usedPrefix
+}
+
+// PreRun is run before general task execution, and checks for terraform breaking changes.
+func (e *EBSVolume) PreRun(c *fi.Context) error {
+	if _, ok := c.Target.(*terraform.TerraformTarget); ok {
+		_, usedPrefix := e.TerraformName()
+		if usedPrefix {
+			if os.Getenv("KOPS_TERRAFORM_0_12_RENAMED") == "" {
+				fmt.Fprintf(os.Stderr, "Terraform 0.12 broke compatibility and disallowed names that begin with a number.\n")
+				fmt.Fprintf(os.Stderr, "  To move an existing cluster to the new syntax, you must first move existing volumes to the new names.\n")
+				fmt.Fprintf(os.Stderr, "  To indicate that you have already performed the rename, pass KOPS_TERRAFORM_0_12_RENAMED=ebs environment variable.\n")
+				fmt.Fprintf(os.Stderr, "  Not doing so will result in data loss.\n")
+				fmt.Fprintf(os.Stderr, "For detailed instructions: https://github.com/kubernetes/kops/blob/master/permalinks/terraform_renamed.md\n")
+				return fmt.Errorf("must update terraform state for 0.12, and then pass KOPS_TERRAFORM_0_12_RENAMED=ebs")
+			}
+		}
+	}
+
+	return nil
 }
 
 type cloudformationVolume struct {

upup/pkg/fi/cloudup/new_cluster.go

@@ -736,10 +736,9 @@ func trimCommonPrefix(names []string) []string {
 		}
 	}
 
-	for i := range names {
-		_, err := strconv.Atoi(names[i])
-		if err == nil {
-			names[i] = "etcd-" + names[i]
+	for i, name := range names {
+		if len(name) > 0 && name[0] >= '0' && name[0] <= '9' {
+			names[i] = "etcd-" + name
 		}
 	}
 

upup/pkg/fi/executor.go

@@ -55,6 +55,14 @@ func (o *RunTasksOptions) InitDefaults() {
 func (e *executor) RunTasks(taskMap map[string]Task) error {
 	dependencies := FindTaskDependencies(taskMap)
 
+	for _, task := range taskMap {
+		if taskPreRun, ok := task.(TaskPreRun); ok {
+			if err := taskPreRun.PreRun(e.context); err != nil {
+				return err
+			}
+		}
+	}
+
 	taskStates := make(map[string]*taskState)
 
 	for k, task := range taskMap {

upup/pkg/fi/task.go

@@ -28,6 +28,12 @@ type Task interface {
 	Run(*Context) error
 }
 
+// TaskPreRun is implemented by tasks that perform some initial validation.
+type TaskPreRun interface {
+	// PreRun will be run for all TaskPreRuns, before any Run functions are invoked.
+	PreRun(*Context) error
+}
+
 // TaskAsString renders the task for debug output
 // TODO: Use reflection to make this cleaner: don't recurse into tasks - print their names instead
 // also print resources in a cleaner way (use the resource source information?)