Merge pull request #23362 from hashicorp/b-ec2-instance-user-data-enc…

…oding r/instance: prevent double encoding when updating `user_data` or `user_data_base64`
hashicorp · Feb 24, 2022 · 620e0ef · 620e0ef
2 parents b2290be + cb97cdc
commit 620e0ef
Show file tree

Hide file tree

Showing 5 changed files with 158 additions and 3 deletions.
diff --git a/.changelog/23362.txt b/.changelog/23362.txt
@@ -0,0 +1,3 @@
+```release-note:enhancement
+resource/aws_instance: Prevent double base64 encoding of `user_data` and `user_data_base64` on update
+```
diff --git a/internal/service/ec2/instance.go b/internal/service/ec2/instance.go
@@ -1441,12 +1441,25 @@ func resourceInstanceUpdate(d *schema.ResourceData, meta interface{}) error {
 			}
 		}
 
+		// From the API reference:
+		// "If you are using an AWS SDK or command line tool,
+		// base64-encoding is performed for you, and you can load the text from a file.
+		// Otherwise, you must provide base64-encoded text".
+
 		if d.HasChange("user_data") {
 			log.Printf("[INFO] Modifying user data %s", d.Id())
+
+			// Decode so the AWS SDK doesn't double encode
+			userData, err := base64.StdEncoding.DecodeString(d.Get("user_data").(string))
+			if err != nil {
+				log.Printf("[DEBUG] Instance (%s) user_data not base64 decoded", d.Id())
+				userData = []byte(d.Get("user_data").(string))
+			}
+
 			input := &ec2.ModifyInstanceAttributeInput{
 				InstanceId: aws.String(d.Id()),
 				UserData: &ec2.BlobAttributeValue{
-					Value: []byte(d.Get("user_data").(string)),
+					Value: userData,
 				},
 			}
 
@@ -1457,9 +1470,13 @@ func resourceInstanceUpdate(d *schema.ResourceData, meta interface{}) error {
 
 		if d.HasChange("user_data_base64") {
 			log.Printf("[INFO] Modifying user data base64 %s", d.Id())
-			userData, err := base64.URLEncoding.DecodeString(d.Get("user_data_base64").(string))
+
+			// Schema validation technically ensures the data is Base64 encoded.
+			// Decode so the AWS SDK doesn't double encode
+			userData, err := base64.StdEncoding.DecodeString(d.Get("user_data_base64").(string))
 			if err != nil {
-				return fmt.Errorf("error updating instance (%s) user data base64: %w", d.Id(), err)
+				log.Printf("[DEBUG] Instance (%s) user_data_base64 not base64 decoded", d.Id())
+				userData = []byte(d.Get("user_data_base64").(string))
 			}
 
 			input := &ec2.ModifyInstanceAttributeInput{

diff --git a/internal/service/ec2/instance_test.go b/internal/service/ec2/instance_test.go
@@ -392,6 +392,74 @@ func TestAccEC2Instance_userDataBase64(t *testing.T) {
 	})
 }
 
+func TestAccEC2Instance_userDataBase64_updateWithBashFile(t *testing.T) {
+	var v ec2.Instance
+	resourceName := "aws_instance.test"
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:     func() { acctest.PreCheck(t) },
+		ErrorCheck:   acctest.ErrorCheck(t, ec2.EndpointsID),
+		Providers:    acctest.Providers,
+		CheckDestroy: testAccCheckInstanceDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccInstanceConfigWithUserDataBase64(rName, "hello world"),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckInstanceExists(resourceName, &v),
+					resource.TestCheckResourceAttr(resourceName, "user_data_base64", "aGVsbG8gd29ybGQ="),
+				),
+			},
+			{
+				Config: testAccInstanceConfigWithUserDataBase64_Base64EncodedFile(rName, "test-fixtures/userdata-test.sh"),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckInstanceExists(resourceName, &v),
+				),
+			},
+			{
+				ResourceName:            resourceName,
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"user_data"},
+			},
+		},
+	})
+}
+
+func TestAccEC2Instance_userDataBase64_updateWithZipFile(t *testing.T) {
+	var v ec2.Instance
+	resourceName := "aws_instance.test"
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:     func() { acctest.PreCheck(t) },
+		ErrorCheck:   acctest.ErrorCheck(t, ec2.EndpointsID),
+		Providers:    acctest.Providers,
+		CheckDestroy: testAccCheckInstanceDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccInstanceConfigWithUserDataBase64(rName, "hello world"),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckInstanceExists(resourceName, &v),
+					resource.TestCheckResourceAttr(resourceName, "user_data_base64", "aGVsbG8gd29ybGQ="),
+				),
+			},
+			{
+				Config: testAccInstanceConfigWithUserDataBase64_Base64EncodedFile(rName, "test-fixtures/userdata-test.zip"),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckInstanceExists(resourceName, &v),
+				),
+			},
+			{
+				ResourceName:            resourceName,
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"user_data"},
+			},
+		},
+	})
+}
+
 func TestAccEC2Instance_userDataBase64_update(t *testing.T) {
 	var v ec2.Instance
 	resourceName := "aws_instance.test"
@@ -3583,6 +3651,39 @@ func TestAccEC2Instance_UserData_update(t *testing.T) {
 	})
 }
 
+func TestAccEC2Instance_UserData_stringToEncodedString(t *testing.T) {
+	var v ec2.Instance
+	resourceName := "aws_instance.test"
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:     func() { acctest.PreCheck(t) },
+		ErrorCheck:   acctest.ErrorCheck(t, ec2.EndpointsID),
+		Providers:    acctest.Providers,
+		CheckDestroy: testAccCheckInstanceDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccInstanceConfigWithUserData(rName, "hello world"),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckInstanceExists(resourceName, &v),
+				),
+			},
+			{
+				Config: testAccInstanceConfigWithUserDataBase64Encoded(rName, "new world"),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckInstanceExists(resourceName, &v),
+				),
+			},
+			{
+				ResourceName:            resourceName,
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"user_data"},
+			},
+		},
+	})
+}
+
 func TestAccEC2Instance_UserData_emptyStringToUnspecified(t *testing.T) {
 	var v ec2.Instance
 	resourceName := "aws_instance.test"
@@ -4364,6 +4465,21 @@ resource "aws_instance" "test" {
 `, userData))
 }
 
+func testAccInstanceConfigWithUserDataBase64Encoded(rName, userData string) string {
+	return acctest.ConfigCompose(
+		acctest.ConfigLatestAmazonLinuxHvmEbsAmi(),
+		testAccInstanceVPCConfig(rName, false),
+		fmt.Sprintf(`
+resource "aws_instance" "test" {
+  ami       = data.aws_ami.amzn-ami-minimal-hvm-ebs.id
+  subnet_id = aws_subnet.test.id
+
+  instance_type = "t2.small"
+  user_data     = base64encode(%[1]q)
+}
+`, userData))
+}
+
 func testAccInstanceConfigWithUserDataBase64(rName, userData string) string {
 	return acctest.ConfigCompose(
 		acctest.ConfigLatestAmazonLinuxHvmEbsAmi(),
@@ -4379,6 +4495,21 @@ resource "aws_instance" "test" {
 `, userData))
 }
 
+func testAccInstanceConfigWithUserDataBase64_Base64EncodedFile(rName, filename string) string {
+	return acctest.ConfigCompose(
+		acctest.ConfigLatestAmazonLinuxHvmEbsAmi(),
+		testAccInstanceVPCConfig(rName, false),
+		fmt.Sprintf(`
+resource "aws_instance" "test" {
+  ami       = data.aws_ami.amzn-ami-minimal-hvm-ebs.id
+  subnet_id = aws_subnet.test.id
+
+  instance_type    = "t2.small"
+  user_data_base64 = filebase64(%[1]q)
+}
+`, filename))
+}
+
 func testAccInstanceConfigWithSmallInstanceType(rName string) string {
 	return acctest.ConfigCompose(
 		acctest.ConfigLatestAmazonLinuxHvmEbsAmi(),

diff --git a/internal/service/ec2/test-fixtures/userdata-test.sh b/internal/service/ec2/test-fixtures/userdata-test.sh
@@ -0,0 +1,4 @@
+#!/bin/bash -v
+apt-get update -y
+apt-get install -y nginx > /tmp/nginx.log
+
diff --git a/internal/service/ec2/test-fixtures/userdata-test.zip b/internal/service/ec2/test-fixtures/userdata-test.zip