Skip to content

Commit

Permalink
internal/naming: Do not discard terraform- prefixed name prefixes (#1…
Browse files Browse the repository at this point in the history
…7030)

Reference: #17017

Changes:

```
* resource/aws_cloudwatch_event_rule: Prevent perpetual differences with `name_prefix` argument values beginning with `terraform-`
* resource/aws_security_group: Prevent perpetual differences with `name_prefix` argument values beginning with `terraform-`
```

Output from acceptance testing:

```
--- PASS: TestAccAWSCloudWatchEventRule_basic (70.77s)
--- PASS: TestAccAWSCloudWatchEventRule_description (52.06s)
--- PASS: TestAccAWSCloudWatchEventRule_EventBusName (75.08s)
--- PASS: TestAccAWSCloudWatchEventRule_IsEnabled (68.71s)
--- PASS: TestAccAWSCloudWatchEventRule_Name_Generated (33.92s)
--- PASS: TestAccAWSCloudWatchEventRule_NamePrefix (33.93s)
--- PASS: TestAccAWSCloudWatchEventRule_pattern (50.64s)
--- PASS: TestAccAWSCloudWatchEventRule_role (48.35s)
--- PASS: TestAccAWSCloudWatchEventRule_ScheduleAndPattern (33.93s)
--- PASS: TestAccAWSCloudWatchEventRule_tags (83.41s)

--- PASS: TestAccAWSSecurityGroup_allowAll (39.02s)
--- PASS: TestAccAWSSecurityGroup_basic (38.37s)
--- PASS: TestAccAWSSecurityGroup_change (82.06s)
--- PASS: TestAccAWSSecurityGroup_CIDRandGroups (55.51s)
--- PASS: TestAccAWSSecurityGroup_defaultEgressClassic (13.81s)
--- PASS: TestAccAWSSecurityGroup_defaultEgressVPC (49.44s)
--- PASS: TestAccAWSSecurityGroup_drift (19.07s)
--- PASS: TestAccAWSSecurityGroup_driftComplex (49.44s)
--- PASS: TestAccAWSSecurityGroup_egressConfigMode (74.91s)
--- PASS: TestAccAWSSecurityGroup_egressWithPrefixList (53.22s)
--- PASS: TestAccAWSSecurityGroup_failWithDiffMismatch (39.04s)
--- PASS: TestAccAWSSecurityGroup_forceRevokeRulesFalse (682.76s)
--- PASS: TestAccAWSSecurityGroup_forceRevokeRulesTrue (737.58s)
--- PASS: TestAccAWSSecurityGroup_ingressConfigMode (73.28s)
--- PASS: TestAccAWSSecurityGroup_ingressWithCidrAndSGsClassic (14.36s)
--- PASS: TestAccAWSSecurityGroup_ingressWithCidrAndSGsVPC (39.61s)
--- PASS: TestAccAWSSecurityGroup_ingressWithPrefixList (56.36s)
--- PASS: TestAccAWSSecurityGroup_invalidCIDRBlock (6.52s)
--- PASS: TestAccAWSSecurityGroup_IPRangeAndSecurityGroupWithSameRules (48.10s)
--- PASS: TestAccAWSSecurityGroup_IPRangesWithSameRules (35.83s)
--- PASS: TestAccAWSSecurityGroup_ipv4andipv6Egress (35.09s)
--- PASS: TestAccAWSSecurityGroup_ipv6 (46.26s)
--- PASS: TestAccAWSSecurityGroup_multiIngress (45.44s)
--- PASS: TestAccAWSSecurityGroup_Name_Generated (32.72s)
--- PASS: TestAccAWSSecurityGroup_Name_TerraformPrefix (32.03s)
--- PASS: TestAccAWSSecurityGroup_NamePrefix (34.45s)
--- PASS: TestAccAWSSecurityGroup_NamePrefix_TerraformPrefix (32.44s)
--- PASS: TestAccAWSSecurityGroup_ruleDescription (112.40s)
--- PASS: TestAccAWSSecurityGroup_ruleGathering (57.85s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitCidrBlockExceededAppend (71.38s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededAllNew (88.92s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededAppend (81.66s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededPrepend (72.60s)
--- PASS: TestAccAWSSecurityGroup_rulesDropOnError (72.96s)
--- PASS: TestAccAWSSecurityGroup_self (39.48s)
--- PASS: TestAccAWSSecurityGroup_sourceSecurityGroup (31.15s)
--- PASS: TestAccAWSSecurityGroup_tags (76.38s)
--- PASS: TestAccAWSSecurityGroup_vpc (39.96s)
--- PASS: TestAccAWSSecurityGroup_vpcNegOneIngress (45.88s)
--- PASS: TestAccAWSSecurityGroup_vpcProtoNumIngress (37.04s)
```
  • Loading branch information
bflad authored Feb 2, 2021
1 parent 075f130 commit 51c2305
Show file tree
Hide file tree
Showing 8 changed files with 134 additions and 103 deletions.
7 changes: 7 additions & 0 deletions .changelog/17030.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
```release-note:bug
resource/aws_cloudwatch_event_rule: Prevent perpetual differences with `name_prefix` argument values beginning with `terraform-`
```

```release-note:bug
resource/aws_security_group: Prevent perpetual differences with `name_prefix` argument values beginning with `terraform-`
```
23 changes: 5 additions & 18 deletions aws/internal/naming/naming.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package naming
import (
"fmt"
"regexp"
"strings"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
Expand All @@ -28,11 +27,6 @@ func Generate(name string, namePrefix string) string {
return resource.UniqueId()
}

// HasResourceUniqueIdPrefix returns true if the string has the built-in unique ID prefix
func HasResourceUniqueIdPrefix(s string) bool {
return strings.HasPrefix(s, resource.UniqueIdPrefix)
}

// HasResourceUniqueIdSuffix returns true if the string has the built-in unique ID suffix
func HasResourceUniqueIdSuffix(s string) bool {
return resourceUniqueIDSuffixRegexp.MatchString(s)
Expand All @@ -41,24 +35,17 @@ func HasResourceUniqueIdSuffix(s string) bool {
// NamePrefixFromName returns a name prefix if the string matches prefix criteria
//
// The input to this function must be strictly the "name" and not any
// additional information such as a full Amazon Resource Name (ARN). The output
// is suitable for custom resource Importer State functions after nil checking
// to ensure differences are not reported with ImportStateVerify testing, e.g.
// additional information such as a full Amazon Resource Name (ARN).
//
// An expected usage might be:
//
// d.Set("name_prefix", naming.NamePrefixFromName(d.Id()))
//
// if namePrefix := naming.NamePrefixFromName(d.Id()); namePrefix != nil {
// d.Set("name_prefix", namePrefix)
// }
func NamePrefixFromName(name string) *string {
if !HasResourceUniqueIdSuffix(name) {
return nil
}

// If the name begins with terraform-, then the name may have been fully
// generated (e.g. omitting both name and name_prefix arguments)
if HasResourceUniqueIdPrefix(name) {
return nil
}

namePrefixIndex := len(name) - resource.UniqueIDSuffixLength

if namePrefixIndex <= 0 {
Expand Down
61 changes: 9 additions & 52 deletions aws/internal/naming/naming_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,40 +53,6 @@ func TestGenerate(t *testing.T) {
}
}

func TestHasResourceUniqueIdPrefix(t *testing.T) {
testCases := []struct {
TestName string
Input string
Expected bool
}{
{
TestName: "empty",
Input: "",
Expected: false,
},
{
TestName: "incorrect prefix",
Input: "test-20060102150405000000000001",
Expected: false,
},
{
TestName: "correct prefix",
Input: "terraform-20060102150405000000000001",
Expected: true,
},
}

for _, testCase := range testCases {
t.Run(testCase.TestName, func(t *testing.T) {
got := HasResourceUniqueIdPrefix(testCase.Input)

if got != testCase.Expected {
t.Errorf("got %t, expected %t", got, testCase.Expected)
}
})
}
}

func TestHasResourceUniqueIdSuffix(t *testing.T) {
testCases := []struct {
TestName string
Expand All @@ -104,25 +70,15 @@ func TestHasResourceUniqueIdSuffix(t *testing.T) {
Expected: false,
},
{
TestName: "correct suffix, incorrect prefix",
TestName: "correct suffix with numbers",
Input: "test-20060102150405000000000001",
Expected: true,
},
{
TestName: "correct suffix with hex, incorrect prefix",
TestName: "correct suffix with hex",
Input: "test-200601021504050000000000a1",
Expected: true,
},
{
TestName: "correct suffix, correct prefix",
Input: "terraform-20060102150405000000000001",
Expected: true,
},
{
TestName: "correct suffix with hex, correct prefix",
Input: "terraform-2006010215040500000000000a",
Expected: true,
},
}

for _, testCase := range testCases {
Expand All @@ -148,29 +104,30 @@ func TestNamePrefixFromName(t *testing.T) {
Expected: nil,
},
{
TestName: "correct prefix, incorrect suffix",
TestName: "incorrect suffix",
Input: "test-123",
Expected: nil,
},
{
TestName: "correct prefix without hyphen, correct suffix",
TestName: "prefix without hyphen, correct suffix",
Input: "test20060102150405000000000001",
Expected: strPtr("test"),
},
{
TestName: "correct prefix with hyphen, correct suffix",
TestName: "prefix with hyphen, correct suffix",
Input: "test-20060102150405000000000001",
Expected: strPtr("test-"),
},
{
TestName: "correct prefix with hyphen, correct suffix with hex",
TestName: "prefix with hyphen, correct suffix with hex",
Input: "test-200601021504050000000000f1",
Expected: strPtr("test-"),
},
// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/17017
{
TestName: "incorrect prefix, correct suffix",
TestName: "terraform prefix, correct suffix",
Input: "terraform-20060102150405000000000001",
Expected: nil,
Expected: strPtr("terraform-"),
},
}

Expand Down
3 changes: 2 additions & 1 deletion aws/resource_aws_cloudwatch_event_rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ func resourceAwsCloudWatchEventRule() *schema.Resource {
"name_prefix": {
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
ConflictsWith: []string{"name"},
ValidateFunc: validateCloudWatchEventRuleName,
Expand Down Expand Up @@ -170,7 +171,7 @@ func resourceAwsCloudWatchEventRuleRead(d *schema.ResourceData, meta interface{}
d.Set("event_pattern", pattern)
}
d.Set("name", out.Name)
d.Set("name_prefix", aws.StringValue(naming.NamePrefixFromName(aws.StringValue(out.Name))))
d.Set("name_prefix", naming.NamePrefixFromName(aws.StringValue(out.Name)))
d.Set("role_arn", out.RoleArn)
d.Set("schedule_expression", out.ScheduleExpression)
d.Set("event_bus_name", out.EventBusName)
Expand Down
2 changes: 1 addition & 1 deletion aws/resource_aws_cloudwatch_event_rule_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ func TestAccAWSCloudWatchEventRule_Name_Generated(t *testing.T) {
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudWatchEventRuleExists(resourceName, &v),
naming.TestCheckResourceAttrNameGenerated(resourceName, "name"),
resource.TestCheckResourceAttr(resourceName, "name_prefix", ""),
resource.TestCheckResourceAttr(resourceName, "name_prefix", "terraform-"),
),
},
{
Expand Down
3 changes: 2 additions & 1 deletion aws/resource_aws_security_group.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ func resourceAwsSecurityGroup() *schema.Resource {
"name_prefix": {
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
ConflictsWith: []string{"name"},
ValidateFunc: validation.StringLenBetween(0, 100),
Expand Down Expand Up @@ -381,7 +382,7 @@ func resourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) erro
d.Set("arn", sgArn.String())
d.Set("description", sg.Description)
d.Set("name", sg.GroupName)
d.Set("name_prefix", aws.StringValue(naming.NamePrefixFromName(aws.StringValue(sg.GroupName))))
d.Set("name_prefix", naming.NamePrefixFromName(aws.StringValue(sg.GroupName)))
d.Set("owner_id", sg.OwnerId)
d.Set("vpc_id", sg.VpcId)

Expand Down
133 changes: 104 additions & 29 deletions aws/resource_aws_security_group_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1094,7 +1094,7 @@ func TestAccAWSSecurityGroup_ipv6(t *testing.T) {
})
}

func TestAccAWSSecurityGroup_namePrefix(t *testing.T) {
func TestAccAWSSecurityGroup_Name_Generated(t *testing.T) {
var group ec2.SecurityGroup
resourceName := "aws_security_group.test"

Expand All @@ -1104,10 +1104,94 @@ func TestAccAWSSecurityGroup_namePrefix(t *testing.T) {
CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSSecurityGroupPrefixNameConfig("tf-acc-test-prefix-"),
Config: testAccAWSSecurityGroupConfig_generatedName,
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSSecurityGroupExists(resourceName, &group),
naming.TestCheckResourceAttrNameGenerated(resourceName, "name"),
resource.TestCheckResourceAttr(resourceName, "name_prefix", "terraform-"),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"revoke_rules_on_delete"},
},
},
})
}

// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/17017
func TestAccAWSSecurityGroup_Name_TerraformPrefix(t *testing.T) {
var group ec2.SecurityGroup
resourceName := "aws_security_group.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSSecurityGroupConfigName("terraform-test"),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSSecurityGroupExists(resourceName, &group),
resource.TestCheckResourceAttr(resourceName, "name", "terraform-test"),
resource.TestCheckResourceAttr(resourceName, "name_prefix", ""),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"revoke_rules_on_delete"},
},
},
})
}

func TestAccAWSSecurityGroup_NamePrefix(t *testing.T) {
var group ec2.SecurityGroup
resourceName := "aws_security_group.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSSecurityGroupConfigNamePrefix("tf-acc-test-prefix-"),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSSecurityGroupExists(resourceName, &group),
naming.TestCheckResourceAttrNameFromPrefix(resourceName, "name", "tf-acc-test-prefix-"),
resource.TestCheckResourceAttr(resourceName, "name_prefix", "tf-acc-test-prefix-"),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"revoke_rules_on_delete"},
},
},
})
}

// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/17017
func TestAccAWSSecurityGroup_NamePrefix_TerraformPrefix(t *testing.T) {
var group ec2.SecurityGroup
resourceName := "aws_security_group.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSSecurityGroupConfigNamePrefix("terraform-test"),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSSecurityGroupExists(resourceName, &group),
naming.TestCheckResourceAttrNameFromPrefix(resourceName, "name", "terraform-test"),
resource.TestCheckResourceAttr(resourceName, "name_prefix", "terraform-test"),
),
},
{
Expand Down Expand Up @@ -1455,32 +1539,6 @@ func TestAccAWSSecurityGroup_ruleDescription(t *testing.T) {
})
}

func TestAccAWSSecurityGroup_generatedName(t *testing.T) {
var group ec2.SecurityGroup
resourceName := "aws_security_group.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSSecurityGroupConfig_generatedName,
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSSecurityGroupExists(resourceName, &group),
naming.TestCheckResourceAttrNameGenerated(resourceName, "name"),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"revoke_rules_on_delete"},
},
},
})
}

func TestAccAWSSecurityGroup_defaultEgressVPC(t *testing.T) {
resourceName := "aws_security_group.test"

Expand Down Expand Up @@ -3380,7 +3438,24 @@ resource "aws_security_group" "test" {
`, rName))
}

func testAccAWSSecurityGroupPrefixNameConfig(namePrefix string) string {
func testAccAWSSecurityGroupConfigName(name string) string {
return fmt.Sprintf(`
resource "aws_vpc" "test" {
cidr_block = "10.0.0.0/16"
tags = {
Name = "tf-acc-test-security-group-name"
}
}
resource "aws_security_group" "test" {
name = %[1]q
vpc_id = aws_vpc.test.id
}
`, name)
}

func testAccAWSSecurityGroupConfigNamePrefix(namePrefix string) string {
return fmt.Sprintf(`
resource "aws_vpc" "test" {
cidr_block = "10.0.0.0/16"
Expand Down
Loading

0 comments on commit 51c2305

Please sign in to comment.