mirror of
https://github.com/opentofu/opentofu.git
synced 2025-01-09 23:54:17 -06:00
Merge pull request #2376 from hashicorp/fix-security-group-rule-hashing-with-source-sg
Consider security groups with source security groups when hashing
This commit is contained in:
commit
f045e9e8d1
@ -21,6 +21,9 @@ func resourceAwsSecurityGroupRule() *schema.Resource {
|
||||
Read: resourceAwsSecurityGroupRuleRead,
|
||||
Delete: resourceAwsSecurityGroupRuleDelete,
|
||||
|
||||
SchemaVersion: 1,
|
||||
MigrateState: resourceAwsSecurityGroupRuleMigrateState,
|
||||
|
||||
Schema: map[string]*schema.Schema{
|
||||
"type": &schema.Schema{
|
||||
Type: schema.TypeString,
|
||||
@ -61,10 +64,11 @@ func resourceAwsSecurityGroupRule() *schema.Resource {
|
||||
},
|
||||
|
||||
"source_security_group_id": &schema.Schema{
|
||||
Type: schema.TypeString,
|
||||
Optional: true,
|
||||
ForceNew: true,
|
||||
Computed: true,
|
||||
Type: schema.TypeString,
|
||||
Optional: true,
|
||||
ForceNew: true,
|
||||
Computed: true,
|
||||
ConflictsWith: []string{"cidr_blocks"},
|
||||
},
|
||||
|
||||
"self": &schema.Schema{
|
||||
@ -90,6 +94,7 @@ func resourceAwsSecurityGroupRuleCreate(d *schema.ResourceData, meta interface{}
|
||||
|
||||
ruleType := d.Get("type").(string)
|
||||
|
||||
var autherr error
|
||||
switch ruleType {
|
||||
case "ingress":
|
||||
log.Printf("[DEBUG] Authorizing security group %s %s rule: %s",
|
||||
@ -105,13 +110,7 @@ func resourceAwsSecurityGroupRuleCreate(d *schema.ResourceData, meta interface{}
|
||||
req.GroupName = sg.GroupName
|
||||
}
|
||||
|
||||
_, err := conn.AuthorizeSecurityGroupIngress(req)
|
||||
|
||||
if err != nil {
|
||||
return fmt.Errorf(
|
||||
"Error authorizing security group %s rules: %s",
|
||||
"rules", err)
|
||||
}
|
||||
_, autherr = conn.AuthorizeSecurityGroupIngress(req)
|
||||
|
||||
case "egress":
|
||||
log.Printf("[DEBUG] Authorizing security group %s %s rule: %#v",
|
||||
@ -122,18 +121,28 @@ func resourceAwsSecurityGroupRuleCreate(d *schema.ResourceData, meta interface{}
|
||||
IPPermissions: []*ec2.IPPermission{perm},
|
||||
}
|
||||
|
||||
_, err = conn.AuthorizeSecurityGroupEgress(req)
|
||||
|
||||
if err != nil {
|
||||
return fmt.Errorf(
|
||||
"Error authorizing security group %s rules: %s",
|
||||
"rules", err)
|
||||
}
|
||||
_, autherr = conn.AuthorizeSecurityGroupEgress(req)
|
||||
|
||||
default:
|
||||
return fmt.Errorf("Security Group Rule must be type 'ingress' or type 'egress'")
|
||||
}
|
||||
|
||||
if autherr != nil {
|
||||
if awsErr, ok := autherr.(awserr.Error); ok {
|
||||
if awsErr.Code() == "InvalidPermission.Duplicate" {
|
||||
return fmt.Errorf(`[WARN] A duplicate Security Group rule was found. This may be
|
||||
a side effect of a now-fixed Terraform issue causing two security groups with
|
||||
identical attributes but different source_security_group_ids to overwrite each
|
||||
other in the state. See https://github.com/hashicorp/terraform/pull/2376 for more
|
||||
information and instructions for recovery. Error message: %s`, awsErr.Message())
|
||||
}
|
||||
}
|
||||
|
||||
return fmt.Errorf(
|
||||
"Error authorizing security group rule type %s: %s",
|
||||
ruleType, autherr)
|
||||
}
|
||||
|
||||
d.SetId(ipPermissionIDHash(ruleType, perm))
|
||||
|
||||
return resourceAwsSecurityGroupRuleRead(d, meta)
|
||||
@ -263,15 +272,32 @@ func findResourceSecurityGroup(conn *ec2.EC2, id string) (*ec2.SecurityGroup, er
|
||||
return resp.SecurityGroups[0], nil
|
||||
}
|
||||
|
||||
// ByGroupPair implements sort.Interface for []*ec2.UserIDGroupPairs based on
|
||||
// GroupID or GroupName field (only one should be set).
|
||||
type ByGroupPair []*ec2.UserIDGroupPair
|
||||
|
||||
func (b ByGroupPair) Len() int { return len(b) }
|
||||
func (b ByGroupPair) Swap(i, j int) { b[i], b[j] = b[j], b[i] }
|
||||
func (b ByGroupPair) Less(i, j int) bool {
|
||||
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
|
||||
// the to and from ports will be nil. We don't record this rule locally.
|
||||
if ip.IPProtocol != nil && *ip.IPProtocol != "-1" {
|
||||
if ip.FromPort != nil && *ip.FromPort > 0 {
|
||||
buf.WriteString(fmt.Sprintf("%d-", *ip.FromPort))
|
||||
buf.WriteString(fmt.Sprintf("%d-", *ip.ToPort))
|
||||
buf.WriteString(fmt.Sprintf("%s-", *ip.IPProtocol))
|
||||
}
|
||||
if ip.ToPort != nil && *ip.ToPort > 0 {
|
||||
buf.WriteString(fmt.Sprintf("%d-", *ip.ToPort))
|
||||
}
|
||||
buf.WriteString(fmt.Sprintf("%s-", *ip.IPProtocol))
|
||||
buf.WriteString(fmt.Sprintf("%s-", ruleType))
|
||||
|
||||
// We need to make sure to sort the strings below so that we always
|
||||
@ -288,6 +314,22 @@ func ipPermissionIDHash(ruleType string, ip *ec2.IPPermission) string {
|
||||
}
|
||||
}
|
||||
|
||||
if len(ip.UserIDGroupPairs) > 0 {
|
||||
sort.Sort(ByGroupPair(ip.UserIDGroupPairs))
|
||||
for _, pair := range ip.UserIDGroupPairs {
|
||||
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()))
|
||||
}
|
||||
|
||||
|
@ -0,0 +1,101 @@
|
||||
package aws
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"log"
|
||||
"strconv"
|
||||
"strings"
|
||||
|
||||
"github.com/aws/aws-sdk-go/aws"
|
||||
"github.com/aws/aws-sdk-go/service/ec2"
|
||||
"github.com/hashicorp/terraform/terraform"
|
||||
)
|
||||
|
||||
func resourceAwsSecurityGroupRuleMigrateState(
|
||||
v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) {
|
||||
switch v {
|
||||
case 0:
|
||||
log.Println("[INFO] Found AWS Security Group State v0; migrating to v1")
|
||||
return migrateSGRuleStateV0toV1(is)
|
||||
default:
|
||||
return is, fmt.Errorf("Unexpected schema version: %d", v)
|
||||
}
|
||||
|
||||
return is, nil
|
||||
}
|
||||
|
||||
func migrateSGRuleStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) {
|
||||
if is.Empty() {
|
||||
log.Println("[DEBUG] Empty InstanceState; nothing to migrate.")
|
||||
return is, nil
|
||||
}
|
||||
|
||||
perm, err := migrateExpandIPPerm(is.Attributes)
|
||||
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("[WARN] Error making new IP Permission in Security Group migration")
|
||||
}
|
||||
|
||||
log.Printf("[DEBUG] Attributes before migration: %#v", is.Attributes)
|
||||
newID := ipPermissionIDHash(is.Attributes["type"], perm)
|
||||
is.Attributes["id"] = newID
|
||||
is.ID = newID
|
||||
log.Printf("[DEBUG] Attributes after migration: %#v, new id: %s", is.Attributes, newID)
|
||||
return is, nil
|
||||
}
|
||||
|
||||
func migrateExpandIPPerm(attrs map[string]string) (*ec2.IPPermission, error) {
|
||||
var perm ec2.IPPermission
|
||||
tp, err := strconv.Atoi(attrs["to_port"])
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("Error converting to_port in Security Group migration")
|
||||
}
|
||||
|
||||
fp, err := strconv.Atoi(attrs["from_port"])
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("Error converting from_port in Security Group migration")
|
||||
}
|
||||
|
||||
perm.ToPort = aws.Long(int64(tp))
|
||||
perm.FromPort = aws.Long(int64(fp))
|
||||
perm.IPProtocol = aws.String(attrs["protocol"])
|
||||
|
||||
groups := make(map[string]bool)
|
||||
if attrs["self"] == "true" {
|
||||
groups[attrs["security_group_id"]] = true
|
||||
}
|
||||
|
||||
if attrs["source_security_group_id"] != "" {
|
||||
groups[attrs["source_security_group_id"]] = true
|
||||
}
|
||||
|
||||
if len(groups) > 0 {
|
||||
perm.UserIDGroupPairs = make([]*ec2.UserIDGroupPair, len(groups))
|
||||
// build string list of group name/ids
|
||||
var gl []string
|
||||
for k, _ := range groups {
|
||||
gl = append(gl, k)
|
||||
}
|
||||
|
||||
for i, name := range gl {
|
||||
perm.UserIDGroupPairs[i] = &ec2.UserIDGroupPair{
|
||||
GroupID: aws.String(name),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
var cb []string
|
||||
for k, v := range attrs {
|
||||
if k != "cidr_blocks.#" && strings.HasPrefix(k, "cidr_blocks") {
|
||||
cb = append(cb, v)
|
||||
}
|
||||
}
|
||||
if len(cb) > 0 {
|
||||
perm.IPRanges = make([]*ec2.IPRange, len(cb))
|
||||
for i, v := range cb {
|
||||
perm.IPRanges[i] = &ec2.IPRange{CIDRIP: aws.String(v)}
|
||||
}
|
||||
}
|
||||
|
||||
return &perm, nil
|
||||
}
|
@ -0,0 +1,67 @@
|
||||
package aws
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"github.com/hashicorp/terraform/terraform"
|
||||
)
|
||||
|
||||
func TestAWSSecurityGroupRuleMigrateState(t *testing.T) {
|
||||
cases := map[string]struct {
|
||||
StateVersion int
|
||||
ID string
|
||||
Attributes map[string]string
|
||||
Expected string
|
||||
Meta interface{}
|
||||
}{
|
||||
"v0_1": {
|
||||
StateVersion: 0,
|
||||
ID: "sg-4235098228",
|
||||
Attributes: map[string]string{
|
||||
"self": "false",
|
||||
"to_port": "0",
|
||||
"security_group_id": "sg-13877277",
|
||||
"cidr_blocks.#": "0",
|
||||
"type": "ingress",
|
||||
"protocol": "-1",
|
||||
"from_port": "0",
|
||||
"source_security_group_id": "sg-11877275",
|
||||
},
|
||||
Expected: "sg-3766347571",
|
||||
},
|
||||
"v0_2": {
|
||||
StateVersion: 0,
|
||||
ID: "sg-1021609891",
|
||||
Attributes: map[string]string{
|
||||
"security_group_id": "sg-0981746d",
|
||||
"from_port": "0",
|
||||
"to_port": "0",
|
||||
"type": "ingress",
|
||||
"self": "false",
|
||||
"protocol": "-1",
|
||||
"cidr_blocks.0": "172.16.1.0/24",
|
||||
"cidr_blocks.1": "172.16.2.0/24",
|
||||
"cidr_blocks.2": "172.16.3.0/24",
|
||||
"cidr_blocks.3": "172.16.4.0/24",
|
||||
"cidr_blocks.#": "4"},
|
||||
Expected: "sg-4100229787",
|
||||
},
|
||||
}
|
||||
|
||||
for tn, tc := range cases {
|
||||
is := &terraform.InstanceState{
|
||||
ID: tc.ID,
|
||||
Attributes: tc.Attributes,
|
||||
}
|
||||
is, err := resourceAwsSecurityGroupRuleMigrateState(
|
||||
tc.StateVersion, is, tc.Meta)
|
||||
|
||||
if err != nil {
|
||||
t.Fatalf("bad: %s, err: %#v", tn, err)
|
||||
}
|
||||
|
||||
if is.ID != tc.Expected {
|
||||
t.Fatalf("bad sg rule id: %s\n\n expected: %s", is.ID, tc.Expected)
|
||||
}
|
||||
}
|
||||
}
|
@ -7,6 +7,7 @@ import (
|
||||
|
||||
"github.com/aws/aws-sdk-go/aws"
|
||||
"github.com/aws/aws-sdk-go/aws/awserr"
|
||||
"github.com/aws/aws-sdk-go/aws/awsutil"
|
||||
"github.com/aws/aws-sdk-go/service/ec2"
|
||||
"github.com/hashicorp/terraform/helper/resource"
|
||||
"github.com/hashicorp/terraform/terraform"
|
||||
@ -44,6 +45,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
|
||||
@ -52,13 +93,15 @@ func TestIpPermissionIDHash(t *testing.T) {
|
||||
}{
|
||||
{simple, "ingress", "sg-82613597"},
|
||||
{egress, "egress", "sg-363054720"},
|
||||
{egress_all, "egress", "sg-857124156"},
|
||||
{egress_all, "egress", "sg-2766285362"},
|
||||
{vpc_security_group_source, "egress", "sg-2661404947"},
|
||||
{security_group_source, "egress", "sg-1841245863"},
|
||||
}
|
||||
|
||||
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 - %s\noutput: %s", tc.Type, awsutil.StringValue(tc.Input), actual)
|
||||
}
|
||||
}
|
||||
}
|
||||
@ -141,9 +184,9 @@ func TestAccAWSSecurityGroupRule_MultiIngress(t *testing.T) {
|
||||
var group ec2.SecurityGroup
|
||||
|
||||
testMultiRuleCount := func(*terraform.State) error {
|
||||
if len(group.IPPermissions) != 3 {
|
||||
if len(group.IPPermissions) != 2 {
|
||||
return fmt.Errorf("Wrong Security Group rule count, expected %d, got %d",
|
||||
3, len(group.IPPermissions))
|
||||
2, len(group.IPPermissions))
|
||||
}
|
||||
|
||||
var rule *ec2.IPPermission
|
||||
@ -395,7 +438,6 @@ resource "aws_security_group_rule" "ingress_1" {
|
||||
cidr_blocks = ["10.0.0.0/8"]
|
||||
|
||||
security_group_id = "${aws_security_group.web.id}"
|
||||
source_security_group_id = "${aws_security_group.worker.id}"
|
||||
}
|
||||
|
||||
resource "aws_security_group_rule" "ingress_2" {
|
||||
|
Loading…
Reference in New Issue
Block a user