Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix for dynamodb autoscaling policy import #8397

Merged
merged 8 commits into from
Jul 18, 2019
Merged

fix for dynamodb autoscaling policy import #8397

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6f4e59b
fix to fix dynamodb autoscaling policy import and also keep ecs and r…
jdecarli Apr 21, 2019
8e280ed
refactor/cleanup and unit test for validateAppautoscalingPolicyImport…
jdecarli Apr 23, 2019
ca98ae3
updating to use the same table names as the documentation
jdecarli Apr 23, 2019
c4f2e1e
refactor and cleanup resourceAwsAppautoscalingPolicyImport
jdecarli May 21, 2019
b0e095c
fixing typo/error in test logic
jdecarli May 22, 2019
7d86d43
fixing integration test (TestAccAWSAppautoScalingPolicy_dynamoDb)
jdecarli May 24, 2019
7ecc6c1
resource/aws_appautoscaling_policy: Update import validation support
nywilken Jul 18, 2019
a04400e
Merge branch 'master' into fix-for-dynamodb-policy-import-issue-8306
nywilken Jul 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 32 additions & 10 deletions aws/resource_aws_appautoscaling_policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,18 +407,13 @@ func resourceAwsAppautoscalingPolicyDelete(d *schema.ResourceData, meta interfac

func resourceAwsAppautoscalingPolicyImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
idParts := strings.Split(d.Id(), "/")
var serviceNamespace, resourceId, scalableDimension, policyName string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about moving all this string splitting logic into validateAppautoscalingPolicyImportInput to help simplify the Import function?

  func resourceAwsAppautoscalingPolicyImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {                          
    idParts, err := validateAppautoscalingPolicyImportInput(d.Id())                                                                               
    if err != nil {                                                                                                                               
      return nil, fmt.Errorf("unexpected format (%q), expected <service-namespace>/<resource-id>/<scalable-dimension>/<policy-name>", d.Id())     
    }                                                                                                                                             
                                                                                                                                                  
    serviceNamespace := idParts[0]                                                                                                                
    resourceId := idParts[1]                                                                                                                      
    scalableDimension := idParts[2]                                                                                                               
    policyName := idParts[3]                                                                                                                      
                                                                                                                                                  
    d.Set("service_namespace", serviceNamespace)                                                                                                  
    d.Set("resource_id", resourceId)                                                                                                              
    d.Set("scalable_dimension", scalableDimension)                                                                                                
    d.Set("name", policyName)                                                                                                                     
    d.SetId(policyName)                                                                                                                                                                                                                                                                             
    return []*schema.ResourceData{d}, nil                                                                                                         
  }

var err error

if len(idParts) < 4 {
return nil, fmt.Errorf("unexpected format (%q), expected <service-namespace>/<resource-id>/<scalable-dimension>/<policy-name>", d.Id())
}

serviceNamespace := idParts[0]
resourceId := strings.Join(idParts[1:len(idParts)-2], "/")
scalableDimension := idParts[len(idParts)-2]
policyName := idParts[len(idParts)-1]
serviceNamespace, resourceId, scalableDimension, policyName, err = validateAppautoscalingPolicyImportInput(idParts)

if serviceNamespace == "" || resourceId == "" || scalableDimension == "" || policyName == "" {
return nil, fmt.Errorf("unexpected format (%q), expected <service-namespace>/<resource-id>/<scalable-dimension>/<policy-name>", d.Id())
if err != nil {
return nil, err
}

d.Set("service_namespace", serviceNamespace)
Expand All @@ -430,6 +425,33 @@ func resourceAwsAppautoscalingPolicyImport(d *schema.ResourceData, meta interfac
return []*schema.ResourceData{d}, nil
}

func validateAppautoscalingPolicyImportInput(idParts []string) (string, string, string, string, error) {
var serviceNamespace, resourceId, scalableDimension, policyName string

if len(idParts) < 4 || len(idParts) > 6 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just checked the doc (aws application-autoscaling describe-scaling-policies help).

  • resourceId may contain from 0 to 3 "/" characters
  • scalableDimension seems to be without "/"
  • policyName can also contain variable number of "/": 0-3

It would be almost better to

  • choose different field delimiter
  • or, import by serviceNamespace/policyName (if it is unique in namespace)
  • or, import by ARN

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @zarnovican

Regarding your suggestions...

Choose different field delimiter

If you check the Terraform import documentation you will see the format Terraform chose for the input in all 3 cases (ecs, rds and dynamodb). Acording to that format, I also made 2 unit tests in TestValidateAppautoscalingPolicyImportInput_positiveCases and TestValidateAppautoscalingPolicyImportInput_negativeCases.

I might be wrong (I'm really new to golang), but as far as I can see the only possible delimiter is / (this is the input coming from the import command) and the problem is, as you mention, that it varies depending on the serviceNamespace. This is basically the root of the issue this pr tries to resolve.

Another option I found is to leave the code as it is, truncating the last item in the slice idParts which is the tableName (which also happens to be redundant, because its coming as the third element in the slice). But this will also force us to append this tableName element to the policy, and this will look pretty ugly in the code with something like this:

// truncating last redundant element
if idParts[0] == "dynamodb" {
	idParts = idParts[:len(idParts) -1]
}

serviceNamespace = idParts[0]
resourceId = strings.Join(idParts[1:len(idParts)-2], "/")
scalableDimension = idParts[len(idParts)-2]
policyName =  idParts[len(idParts)-1]

// appending tableName to the policy
if idParts[0] == "dynamodb" {
	policyName = policyName + "/" + idParts[2]
}

It removes the need for the switch, but adds the truncating and appending.

Import by other fields (serviceNamespace/policyName or ARN)

I think this involves design changes that are out of scope for this issue. Seems like a nice suggestion, but I feel is up to the Terraform team to do some analysis on this. Maybe @nywilken can shed some light on this.

return serviceNamespace, resourceId, scalableDimension, policyName, fmt.Errorf("unexpected format (%q), expected <service-namespace>/<resource-id>/<scalable-dimension>/<policy-name>", idParts[0])
}

switch idParts[0] {
nywilken marked this conversation as resolved.
Show resolved Hide resolved
case "ecs", "rds":
serviceNamespace = idParts[0]
resourceId = strings.Join(idParts[1:len(idParts)-2], "/")
scalableDimension = idParts[len(idParts)-2]
policyName = idParts[len(idParts)-1]
case "dynamodb":
serviceNamespace = idParts[0]
resourceId = strings.Join(idParts[1:3], "/")
scalableDimension = idParts[len(idParts)-3]
policyName = strings.Join(idParts[4:], "/")
}

if serviceNamespace == "" || resourceId == "" || scalableDimension == "" || policyName == "" {
return serviceNamespace, resourceId, scalableDimension, policyName, fmt.Errorf("unexpected format (%q), expected <service-namespace>/<resource-id>/<scalable-dimension>/<policy-name>", idParts[0])
}

return serviceNamespace, resourceId, scalableDimension, policyName, nil
}

// Takes the result of flatmap.Expand for an array of step adjustments and
// returns a []*applicationautoscaling.StepAdjustment.
func expandAppautoscalingStepAdjustments(configured []interface{}) ([]*applicationautoscaling.StepAdjustment, error) {
Expand Down
87 changes: 84 additions & 3 deletions aws/resource_aws_appautoscaling_policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package aws

import (
"fmt"
"strings"
"testing"

"github.com/aws/aws-sdk-go/aws"
Expand All @@ -11,6 +12,85 @@ import (
"github.com/hashicorp/terraform/terraform"
)

func TestValidateAppautoscalingPolicyImportInput_positiveCases(t *testing.T) {
Copy link
Contributor

@nywilken nywilken Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a slightly different version of these tests. This refactor combines the test cases into one function and relies on an expectedError field to assert that the input validation should fail when expected.

func TestValidateAppautoscalingPolicyImportInput(t *testing.T) {
	testCases := []struct {
		input         string
		errorExpected bool
		expected      []string
	}{
		{
			input:         "rds/cluster:id/rds:cluster:ReadReplicaCount/cpu-auto-scaling",
			expected:      []string{"rds", "cluster:id", "rds:cluster:ReadReplicaCount", "cpu-auto-scaling"},
			errorExpected: false,
		},
		{
			input:         "ecs/service/clusterName/serviceName/ecs:service:DesiredCount/scale-down",
			expected:      []string{"ecs", "service/clusterName/serviceName", "ecs:service:DesiredCount", "scale-down"},
			errorExpected: false,
		},
		{
			input:         "dynamodb/table/tableName/dynamodb:table:ReadCapacityUnits/DynamoDBReadCapacityUtilization:table/tableName",
			expected:      []string{"dynamodb", "table/tableName", "dynamodb:table:ReadCapacityUnits", "DynamoDBReadCapacityUtilization:table/tableName"},
			errorExpected: false,
		},
		{
			input:         "dynamodb/missing/parts",
			errorExpected: true,
		},
	}

	for _, tc := range testCases {
		idParts, err := validateAppautoscalingPolicyImportInput(tc.input)
		if tc.errorExpected == false && err != nil {
			t.Errorf("validateAppautoscalingPolicyImportInput(%q): resulted in an unexpected error: %s", tc.input, err)
		}

		if tc.errorExpected == true && err == nil {
			t.Errorf("validateAppautoscalingPolicyImportInput(%q): expected an error, but returned successfully", tc.input)
		}

		if !reflect.DeepEqual(tc.expected, idParts) {
			t.Errorf("validateAppautoscalingPolicyImportInput(%q): expected %q, but got %q", tc.input, strings.Join(tc.expected, "/"), strings.Join(idParts, "/"))
		}
	}
}

testCases := []struct {
input string
expectedServiceNamespace string
expectedResourceId string
expectedScalableDimension string
expectedPolicyName string
}{
{
"rds/cluster:id/rds:cluster:ReadReplicaCount/cpu-auto-scaling",
"rds", "cluster:id", "rds:cluster:ReadReplicaCount", "cpu-auto-scaling"},
{
"ecs/service/clusterName/serviceName/ecs:service:DesiredCount/scale-down",
"ecs", "service/clusterName/serviceName", "ecs:service:DesiredCount", "scale-down"},
{
"dynamodb/table/tableName/dynamodb:table:ReadCapacityUnits/DynamoDBReadCapacityUtilization:table/tableName",
"dynamodb", "table/tableName", "dynamodb:table:ReadCapacityUnits", "DynamoDBReadCapacityUtilization:table/tableName"},
}

for _, test := range testCases {
idParts := strings.Split(test.input, "/")
serviceNamespace, resourceId, scalableDimension, policyName, err := validateAppautoscalingPolicyImportInput(idParts)

if err != nil || serviceNamespace != test.expectedServiceNamespace {
t.Errorf("serviceNamespace interpolation error -> %s != %s", test.expectedServiceNamespace, serviceNamespace)
}
if err != nil && resourceId != test.expectedResourceId {
t.Errorf("resourceId interpolation error -> %s != %s", test.expectedResourceId, resourceId)
}
if err != nil || scalableDimension != test.expectedScalableDimension {
t.Errorf("scalableDimension interpolation error -> %s != %s", test.expectedScalableDimension, scalableDimension)
}
if err != nil || policyName != test.expectedPolicyName {
t.Errorf("policyName interpolation error -> %s != %s", test.expectedPolicyName, policyName)
}
}
}

func TestValidateAppautoscalingPolicyImportInput_negativeCases(t *testing.T) {
testCases := []struct {
input string
expectedServiceNamespace string
expectedResourceId string
expectedScalableDimension string
expectedPolicyName string
}{
{
"wrongValue",
"", "", "", ""},
{
"",
"", "", "", ""},
{
"too/many/arguments/that/are/not/required",
"", "", "", ""},
}

for _, test := range testCases {
idParts := strings.Split(test.input, "/")
serviceNamespace, resourceId, scalableDimension, policyName, err := validateAppautoscalingPolicyImportInput(idParts)

if err == nil || serviceNamespace != test.expectedServiceNamespace {
t.Errorf("serviceNamespace interpolation error -> %s != %s", test.expectedServiceNamespace, serviceNamespace)
}
if err == nil || resourceId != test.expectedResourceId {
t.Errorf("resourceId interpolation error -> %s != %s", test.expectedResourceId, resourceId)
}
if err == nil || scalableDimension != test.expectedScalableDimension {
t.Errorf("scalableDimension interpolation error -> %s != %s", test.expectedScalableDimension, scalableDimension)
}
if err == nil || policyName != test.expectedPolicyName {
t.Errorf("policyName interpolation error -> %s != %s", test.expectedPolicyName, policyName)
}
if err != nil && err.Error() == "" {
t.Errorf("empty error -> error should describe outcome")
}
}
}

func TestAccAWSAppautoScalingPolicy_basic(t *testing.T) {
var policy applicationautoscaling.ScalingPolicy
appAutoscalingTargetResourceName := "aws_appautoscaling_target.test"
Expand Down Expand Up @@ -184,7 +264,8 @@ func TestAccAWSAppautoScalingPolicy_dynamoDb(t *testing.T) {
Config: testAccAWSAppautoscalingPolicyDynamoDB(randPolicyName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSAppautoscalingPolicyExists("aws_appautoscaling_policy.dynamo_test", &policy),
resource.TestCheckResourceAttr("aws_appautoscaling_policy.dynamo_test", "name", randPolicyName),
resource.TestCheckResourceAttr("aws_appautoscaling_policy.dynamo_test", "name", fmt.Sprintf("DynamoDBWriteCapacityUtilization:table/%s", randPolicyName)),
resource.TestCheckResourceAttr("aws_appautoscaling_policy.dynamo_test", "policy_type", "TargetTrackingScaling"),
resource.TestCheckResourceAttr("aws_appautoscaling_policy.dynamo_test", "service_namespace", "dynamodb"),
resource.TestCheckResourceAttr("aws_appautoscaling_policy.dynamo_test", "scalable_dimension", "dynamodb:table:WriteCapacityUnits"),
),
Expand Down Expand Up @@ -527,7 +608,7 @@ resource "aws_appautoscaling_target" "dynamo_test" {
}

resource "aws_appautoscaling_policy" "dynamo_test" {
name = "%s"
name = "DynamoDBWriteCapacityUtilization:${aws_appautoscaling_target.dynamo_test.resource_id}"
policy_type = "TargetTrackingScaling"
service_namespace = "dynamodb"
resource_id = "table/${aws_dynamodb_table.dynamodb_table_test.name}"
Expand All @@ -545,7 +626,7 @@ resource "aws_appautoscaling_policy" "dynamo_test" {

depends_on = ["aws_appautoscaling_target.dynamo_test"]
}
`, randPolicyName, randPolicyName)
`, randPolicyName)
}

func testAccAWSAppautoscalingPolicy_multiplePoliciesSameName(tableName1, tableName2, namePrefix string) string {
Expand Down