provider/aws: Run errcheck in tests (#8579)

* provider/aws: Add errcheck to Makefile, error on unchecked errors

* more exceptions

* updates for errcheck to pass

* reformat and spilt out the ignore statements

* narrow down ignores

* fix typo, only ignore Close and Write, instead of close or write

Makefile

@@ -39,7 +39,7 @@ plugin-dev: generate
 	mv $(GOPATH)/bin/$(PLUGIN) $(GOPATH)/bin/terraform-$(PLUGIN)
 
 # test runs the unit tests
-test: fmtcheck generate
+test: fmtcheck errcheck generate
 	TF_ACC= go test $(TEST) $(TESTARGS) -timeout=30s -parallel=4
 
 # testacc runs acceptance tests
@@ -89,4 +89,7 @@ fmt:
 fmtcheck:
 	@sh -c "'$(CURDIR)/scripts/gofmtcheck.sh'"
 
+errcheck:
+	@sh -c "'$(CURDIR)/scripts/errcheck.sh'"
+
 .PHONY: bin default generate test vet fmt fmtcheck tools

builtin/providers/aws/import_aws_security_group.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/terraform/helper/schema"
 )
 
@@ -80,7 +81,9 @@ func resourceAwsSecurityGroupImportState(
 			// XXX If the rule contained more than one source security group, this
 			// will choose one of them. We actually need to create one rule for each
 			// source security group.
-			setFromIPPerm(d, sg, perm)
+			if err := setFromIPPerm(d, sg, perm); err != nil {
+				return nil, errwrap.Wrapf("Error importing AWS Security Group: {{err}}", err)
+			}
 			results = append(results, d)
 		}
 	}

builtin/providers/aws/resource_aws_autoscaling_group.go

@@ -6,6 +6,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/helper/schema"
 
@@ -479,11 +480,15 @@ func resourceAwsAutoscalingGroupUpdate(d *schema.ResourceData, meta interface{})
 	}
 
 	if shouldWaitForCapacity {
-		waitForASGCapacity(d, meta, capacitySatifiedUpdate)
+		if err := waitForASGCapacity(d, meta, capacitySatifiedUpdate); err != nil {
+			return errwrap.Wrapf("Error waiting for AutoScaling Group Capacity: {{err}}", err)
+		}
 	}
 
 	if d.HasChange("enabled_metrics") {
-		updateASGMetricsCollection(d, conn)
+		if err := updateASGMetricsCollection(d, conn); err != nil {
+			return errwrap.Wrapf("Error updating AutoScaling Group Metrics collection: {{err}}", err)
+		}
 	}
 
 	return resourceAwsAutoscalingGroupRead(d, meta)

builtin/providers/aws/resource_aws_dynamodb_table.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/helper/schema"
 
@@ -342,7 +343,9 @@ func resourceAwsDynamoDbTableUpdate(d *schema.ResourceData, meta interface{}) er
 	dynamodbconn := meta.(*AWSClient).dynamodbconn
 
 	// Ensure table is active before trying to update
-	waitForTableToBeActive(d.Id(), meta)
+	if err := waitForTableToBeActive(d.Id(), meta); err != nil {
+		return errwrap.Wrapf("Error waiting for Dynamo DB Table update: {{err}}", err)
+	}
 
 	if d.HasChange("read_capacity") || d.HasChange("write_capacity") {
 		req := &dynamodb.UpdateTableInput{
@@ -361,7 +364,9 @@ func resourceAwsDynamoDbTableUpdate(d *schema.ResourceData, meta interface{}) er
 			return err
 		}
 
-		waitForTableToBeActive(d.Id(), meta)
+		if err := waitForTableToBeActive(d.Id(), meta); err != nil {
+			return errwrap.Wrapf("Error waiting for Dynamo DB Table update: {{err}}", err)
+		}
 	}
 
 	if d.HasChange("stream_enabled") || d.HasChange("stream_view_type") {
@@ -380,7 +385,9 @@ func resourceAwsDynamoDbTableUpdate(d *schema.ResourceData, meta interface{}) er
 			return err
 		}
 
-		waitForTableToBeActive(d.Id(), meta)
+		if err := waitForTableToBeActive(d.Id(), meta); err != nil {
+			return errwrap.Wrapf("Error waiting for Dynamo DB Table update: {{err}}", err)
+		}
 	}
 
 	if d.HasChange("global_secondary_index") {
@@ -462,8 +469,13 @@ func resourceAwsDynamoDbTableUpdate(d *schema.ResourceData, meta interface{}) er
 					return err
 				}
 
-				waitForTableToBeActive(d.Id(), meta)
+				if err := waitForTableToBeActive(d.Id(), meta); err != nil {
-				waitForGSIToBeActive(d.Id(), *gsi.IndexName, meta)
+					return errwrap.Wrapf("Error waiting for Dynamo DB Table update: {{err}}", err)
+				}
+
+				if err := waitForGSIToBeActive(d.Id(), *gsi.IndexName, meta); err != nil {
+					return errwrap.Wrapf("Error waiting for Dynamo DB GSIT to be active: {{err}}", err)
+				}
 
 			}
 		}
@@ -488,7 +500,9 @@ func resourceAwsDynamoDbTableUpdate(d *schema.ResourceData, meta interface{}) er
 					return err
 				}
 
-				waitForTableToBeActive(d.Id(), meta)
+				if err := waitForTableToBeActive(d.Id(), meta); err != nil {
+					return errwrap.Wrapf("Error waiting for Dynamo DB Table update: {{err}}", err)
+				}
 			}
 		}
 	}
@@ -692,7 +706,9 @@ func resourceAwsDynamoDbTableRead(d *schema.ResourceData, meta interface{}) erro
 func resourceAwsDynamoDbTableDelete(d *schema.ResourceData, meta interface{}) error {
 	dynamodbconn := meta.(*AWSClient).dynamodbconn
 
-	waitForTableToBeActive(d.Id(), meta)
+	if err := waitForTableToBeActive(d.Id(), meta); err != nil {
+		return errwrap.Wrapf("Error waiting for Dynamo DB Table update: {{err}}", err)
+	}
 
 	log.Printf("[DEBUG] DynamoDB delete table: %s", d.Id())
 

builtin/providers/aws/resource_aws_ebs_volume.go

@@ -9,6 +9,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/service/ec2"
 
+	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/helper/schema"
 )
@@ -137,7 +138,9 @@ func resourceAwsEbsVolumeCreate(d *schema.ResourceData, meta interface{}) error
 	d.SetId(*result.VolumeId)
 
 	if _, ok := d.GetOk("tags"); ok {
-		setTags(conn, d)
+		if err := setTags(conn, d); err != nil {
+			return errwrap.Wrapf("Error setting tags for EBS Volume: {{err}}", err)
+		}
 	}
 
 	return readVolume(d, result)
@@ -146,7 +149,9 @@ func resourceAwsEbsVolumeCreate(d *schema.ResourceData, meta interface{}) error
 func resourceAWSEbsVolumeUpdate(d *schema.ResourceData, meta interface{}) error {
 	conn := meta.(*AWSClient).ec2conn
 	if _, ok := d.GetOk("tags"); ok {
-		setTags(conn, d)
+		if err := setTags(conn, d); err != nil {
+			return errwrap.Wrapf("Error updating tags for EBS Volume: {{err}}", err)
+		}
 	}
 	return resourceAwsEbsVolumeRead(d, meta)
 }

builtin/providers/aws/resource_aws_internet_gateway.go

@@ -8,6 +8,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/helper/schema"
 )
@@ -48,7 +49,7 @@ func resourceAwsInternetGatewayCreate(d *schema.ResourceData, meta interface{})
 	d.SetId(*ig.InternetGatewayId)
 	log.Printf("[INFO] InternetGateway ID: %s", d.Id())
 
-	resource.Retry(5*time.Minute, func() *resource.RetryError {
+	err = resource.Retry(5*time.Minute, func() *resource.RetryError {
 		igRaw, _, err := IGStateRefreshFunc(conn, d.Id())()
 		if igRaw != nil {
 			return nil
@@ -60,6 +61,10 @@ func resourceAwsInternetGatewayCreate(d *schema.ResourceData, meta interface{})
 		}
 	})
 
+	if err != nil {
+		return errwrap.Wrapf("{{err}}", err)
+	}
+
 	err = setTags(conn, d)
 	if err != nil {
 		return err

builtin/providers/aws/resource_aws_route53_zone.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/helper/schema"
 
@@ -265,7 +266,9 @@ func resourceAwsRoute53ZoneDelete(d *schema.ResourceData, meta interface{}) erro
 	r53 := meta.(*AWSClient).r53conn
 
 	if d.Get("force_destroy").(bool) {
-		deleteAllRecordsInHostedZoneId(d.Id(), d.Get("name").(string), r53)
+		if err := deleteAllRecordsInHostedZoneId(d.Id(), d.Get("name").(string), r53); err != nil {
+			return errwrap.Wrapf("{{err}}", err)
+		}
 	}
 
 	log.Printf("[DEBUG] Deleting Route53 hosted zone: %s (ID: %s)",

builtin/providers/aws/resource_aws_s3_bucket.go

@@ -1363,7 +1363,9 @@ func normalizeRoutingRules(w []*s3.RoutingRule) (string, error) {
 	}
 
 	var rules []map[string]interface{}
-	json.Unmarshal(withNulls, &rules)
+	if err := json.Unmarshal(withNulls, &rules); err != nil {
+		return "", err
+	}
 
 	var cleanRules []map[string]interface{}
 	for _, rule := range rules {

builtin/providers/aws/resource_aws_security_group_rule.go

@@ -11,6 +11,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/terraform/helper/hashcode"
 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/helper/schema"
@@ -250,7 +251,9 @@ func resourceAwsSecurityGroupRuleRead(d *schema.ResourceData, meta interface{})
 	log.Printf("[DEBUG] Found rule for Security Group Rule (%s): %s", d.Id(), rule)
 
 	d.Set("type", ruleType)
-	setFromIPPerm(d, sg, p)
+	if err := setFromIPPerm(d, sg, p); err != nil {
+		return errwrap.Wrapf("Error setting IP Permission for Security Group Rule: {{err}}", err)
+	}
 	return nil
 }
 

builtin/providers/aws/resource_aws_sqs_queue.go

@@ -169,7 +169,9 @@ func resourceAwsSqsQueueUpdate(d *schema.ResourceData, meta interface{}) error {
 			QueueUrl:   aws.String(d.Id()),
 			Attributes: attributes,
 		}
-		sqsconn.SetQueueAttributes(req)
+		if _, err := sqsconn.SetQueueAttributes(req); err != nil {
+			return fmt.Errorf("[ERR] Error updating SQS attributes: %s", err)
+		}
 	}
 
 	return resourceAwsSqsQueueRead(d, meta)

builtin/providers/aws/resource_aws_ssm_document.go

@@ -119,7 +119,9 @@ func resourceAwsSsmDocumentCreate(d *schema.ResourceData, meta interface{}) erro
 	d.SetId(*resp.DocumentDescription.Name)
 
 	if v, ok := d.GetOk("permissions"); ok && v != nil {
-		setDocumentPermissions(d, meta)
+		if err := setDocumentPermissions(d, meta); err != nil {
+			return err
+		}
 	} else {
 		log.Printf("[DEBUG] Not setting permissions for %q", d.Id())
 	}
@@ -189,7 +191,9 @@ func resourceAwsSsmDocumentRead(d *schema.ResourceData, meta interface{}) error
 func resourceAwsSsmDocumentUpdate(d *schema.ResourceData, meta interface{}) error {
 
 	if _, ok := d.GetOk("permissions"); ok {
-		setDocumentPermissions(d, meta)
+		if err := setDocumentPermissions(d, meta); err != nil {
+			return err
+		}
 	} else {
 		log.Printf("[DEBUG] Not setting document permissions on %q", d.Id())
 	}
@@ -200,7 +204,9 @@ func resourceAwsSsmDocumentUpdate(d *schema.ResourceData, meta interface{}) erro
 func resourceAwsSsmDocumentDelete(d *schema.ResourceData, meta interface{}) error {
 	ssmconn := meta.(*AWSClient).ssmconn
 
-	deleteDocumentPermissions(d, meta)
+	if err := deleteDocumentPermissions(d, meta); err != nil {
+		return err
+	}
 
 	log.Printf("[INFO] Deleting SSM Document: %s", d.Id())
 

builtin/providers/aws/resource_aws_vpn_connection.go

@@ -12,6 +12,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/service/ec2"
 
+	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/terraform/helper/hashcode"
 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/helper/schema"
@@ -309,11 +310,14 @@ func resourceAwsVpnConnectionRead(d *schema.ResourceData, meta interface{}) erro
 	d.Set("customer_gateway_configuration", vpnConnection.CustomerGatewayConfiguration)
 
 	if vpnConnection.CustomerGatewayConfiguration != nil {
-		tunnelInfo := xmlConfigToTunnelInfo(*vpnConnection.CustomerGatewayConfiguration)
+		if tunnelInfo, err := xmlConfigToTunnelInfo(*vpnConnection.CustomerGatewayConfiguration); err != nil {
-		d.Set("tunnel1_address", tunnelInfo.Tunnel1Address)
+			log.Printf("[ERR] Error unmarshaling XML configuration for (%s): %s", d.Id(), err)
-		d.Set("tunnel1_preshared_key", tunnelInfo.Tunnel1PreSharedKey)
+		} else {
-		d.Set("tunnel2_address", tunnelInfo.Tunnel2Address)
+			d.Set("tunnel1_address", tunnelInfo.Tunnel1Address)
-		d.Set("tunnel2_preshared_key", tunnelInfo.Tunnel2PreSharedKey)
+			d.Set("tunnel1_preshared_key", tunnelInfo.Tunnel1PreSharedKey)
+			d.Set("tunnel2_address", tunnelInfo.Tunnel2Address)
+			d.Set("tunnel2_preshared_key", tunnelInfo.Tunnel2PreSharedKey)
+		}
 	}
 
 	if err := d.Set("vgw_telemetry", telemetryToMapList(vpnConnection.VgwTelemetry)); err != nil {
@@ -416,9 +420,11 @@ func telemetryToMapList(telemetry []*ec2.VgwTelemetry) []map[string]interface{}
 	return result
 }
 
-func xmlConfigToTunnelInfo(xmlConfig string) TunnelInfo {
+func xmlConfigToTunnelInfo(xmlConfig string) (*TunnelInfo, error) {
 	var vpnConfig XmlVpnConnectionConfig
-	xml.Unmarshal([]byte(xmlConfig), &vpnConfig)
+	if err := xml.Unmarshal([]byte(xmlConfig), &vpnConfig); err != nil {
+		return nil, errwrap.Wrapf("Error Unmarshalling XML: {{err}}", err)
+	}
 
 	// don't expect consistent ordering from the XML
 	sort.Sort(vpnConfig)
@@ -431,5 +437,5 @@ func xmlConfigToTunnelInfo(xmlConfig string) TunnelInfo {
 		Tunnel2PreSharedKey: vpnConfig.Tunnels[1].PreSharedKey,
 	}
 
-	return tunnelInfo
+	return &tunnelInfo, nil
 }

builtin/providers/aws/resource_aws_vpn_connection_test.go

@@ -119,7 +119,10 @@ func testAccAwsVpnConnection(
 }
 
 func TestAWSVpnConnection_xmlconfig(t *testing.T) {
-	tunnelInfo := xmlConfigToTunnelInfo(testAccAwsVpnTunnelInfoXML)
+	tunnelInfo, err := xmlConfigToTunnelInfo(testAccAwsVpnTunnelInfoXML)
+	if err != nil {
+		t.Fatalf("Error unmarshalling XML: %s", err)
+	}
 	if tunnelInfo.Tunnel1Address != "FIRST_ADDRESS" {
 		t.Fatalf("First address from tunnel XML was incorrect.")
 	}

scripts/errcheck.sh

@@ -0,0 +1,25 @@
+#!/usr/bin/env bash
+
+# Check gofmt
+echo "==> Checking AWS provider for unchecked errors..."
+echo "==> NOTE: at this time we only look for uncheck errors in the AWS package"
+
+if ! which errcheck > /dev/null; then
+    echo "==> Installing errcheck..."
+    go get -u github.com/kisielk/errcheck
+fi
+
+err_files=$(errcheck -ignoretests -ignore \
+  'github.com/hashicorp/terraform/helper/schema:Set' \
+  -ignore 'bytes:.*' \
+  -ignore 'io:Close|Write' \
+  ./builtin/providers/aws/...)
+
+if [[ -n ${err_files} ]]; then
+    echo 'Unchecked errors found in the following places:'
+    echo "${err_files}"
+    echo "Please handle returned errors. You can check directly with \`make errcheck\`"
+    exit 1
+fi
+
+exit 0