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

Consider security group rules with source security groups when hashing #2318

Closed
wants to merge 1 commit into from
Closed

Consider security group rules with source security groups when hashing #2318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 37 additions & 0 deletions builtin/providers/aws/resource_aws_security_group_rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,26 @@ func findResourceSecurityGroup(conn *ec2.EC2, id string) (*ec2.SecurityGroup, er
return resp.SecurityGroups[0], nil
}

// byUserIDAndGroup implements sort.Interface for []*ec2.UserIDGroupPairs based on
// UserID and then the GroupID or GroupName field (only one should be set).
type byUserIDAndGroup []*ec2.UserIDGroupPair

func (b byUserIDAndGroup) Len() int { return len(b) }
func (b byUserIDAndGroup) Swap(i, j int) { b[i], b[j] = b[j], b[i] }
func (b byUserIDAndGroup) Less(i, j int) bool {
if *b[i].UserID != *b[i].UserID {
return *b[i].UserID < *b[i].UserID
}
if b[i].GroupID != nil && b[j].GroupID != nil {
return *b[i].GroupID < *b[j].GroupID
}
if b[i].GroupName != nil && b[j].GroupName != nil {
return *b[i].GroupName < *b[j].GroupName
}

panic("mismatched security group rules, may be a terraform bug")
}

func ipPermissionIDHash(ruleType string, ip *ec2.IPPermission) string {
var buf bytes.Buffer
// for egress rules, an TCP rule of -1 is automatically added, in which case
Expand All @@ -288,6 +308,23 @@ func ipPermissionIDHash(ruleType string, ip *ec2.IPPermission) string {
}
}

if len(ip.UserIDGroupPairs) > 0 {
sort.Sort(byUserIDAndGroup(ip.UserIDGroupPairs))
for _, pair := range ip.UserIDGroupPairs {
buf.WriteString(fmt.Sprintf("%s-", *pair.UserID))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be removed; it's causing TF to always want to re-make the rule because we don't create (or store) the UserID when we create the rule. The perm we send does not have this populated, which I think is OK since the documentation say it's for EC2 Classic only anyway.

I commented it out and this branch started to behave as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I see that now too, I must have missed it before.

if pair.GroupID != nil {
buf.WriteString(fmt.Sprintf("%s-", *pair.GroupID))
} else {
buf.WriteString("-")
}
if pair.GroupName != nil {
buf.WriteString(fmt.Sprintf("%s-", *pair.GroupName))
} else {
buf.WriteString("-")
}
}
}

return fmt.Sprintf("sg-%d", hashcode.String(buf.String()))
}

Expand Down
44 changes: 43 additions & 1 deletion builtin/providers/aws/resource_aws_security_group_rule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,46 @@ func TestIpPermissionIDHash(t *testing.T) {
},
}

vpc_security_group_source := &ec2.IPPermission{
IPProtocol: aws.String("tcp"),
FromPort: aws.Long(int64(80)),
ToPort: aws.Long(int64(8000)),
UserIDGroupPairs: []*ec2.UserIDGroupPair{
&ec2.UserIDGroupPair{
UserID: aws.String("987654321"),
GroupID: aws.String("sg-12345678"),
},
&ec2.UserIDGroupPair{
UserID: aws.String("123456789"),
GroupID: aws.String("sg-987654321"),
},
&ec2.UserIDGroupPair{
UserID: aws.String("123456789"),
GroupID: aws.String("sg-12345678"),
},
},
}

security_group_source := &ec2.IPPermission{
IPProtocol: aws.String("tcp"),
FromPort: aws.Long(int64(80)),
ToPort: aws.Long(int64(8000)),
UserIDGroupPairs: []*ec2.UserIDGroupPair{
&ec2.UserIDGroupPair{
UserID: aws.String("987654321"),
GroupName: aws.String("my-security-group"),
},
&ec2.UserIDGroupPair{
UserID: aws.String("123456789"),
GroupName: aws.String("my-security-group"),
},
&ec2.UserIDGroupPair{
UserID: aws.String("123456789"),
GroupName: aws.String("my-other-security-group"),
},
},
}

// hardcoded hashes, to detect future change
cases := []struct {
Input *ec2.IPPermission
Expand All @@ -53,12 +93,14 @@ func TestIpPermissionIDHash(t *testing.T) {
{simple, "ingress", "sg-82613597"},
{egress, "egress", "sg-363054720"},
{egress_all, "egress", "sg-857124156"},
{vpc_security_group_source, "egress", "sg-1900174468"},
{security_group_source, "egress", "sg-1409919872"},
}

for _, tc := range cases {
actual := ipPermissionIDHash(tc.Type, tc.Input)
if actual != tc.Output {
t.Fatalf("input: %s - %#v\noutput: %s", tc.Type, tc.Input, actual)
t.Errorf("input: %s - %#v\noutput: %s", tc.Type, tc.Input, actual)
}
}
}
Expand Down