Use sg rule ids and tags where possible

cloudmock/aws/mockec2/api.go

@@ -42,6 +42,7 @@ type MockEC2 struct {
 
 	securityGroupNumber int
 	SecurityGroups      map[string]*ec2.SecurityGroup
+	SecurityGroupRules  map[string]*ec2.SecurityGroupRule
 
 	subnets map[string]*subnetInfo
 

cloudmock/aws/mockec2/securitygroups.go

@@ -213,7 +213,7 @@ func (m *MockEC2) RevokeSecurityGroupIngress(request *ec2.RevokeSecurityGroupIng
 		return nil, fmt.Errorf("SecurityGroup not found")
 	}
 
-	klog.Warningf("RevokeSecurityGroupIngress not implemented - does not actually revoke permissions")
+	klog.Warningf("RevokeSecurityGroupIngress mock not implemented - does not actually revoke permissions")
 
 	response := &ec2.RevokeSecurityGroupIngressOutput{}
 	return response, nil
@@ -269,6 +269,61 @@ func (m *MockEC2) AuthorizeSecurityGroupEgress(request *ec2.AuthorizeSecurityGro
 
 	// TODO: We need to fold permissions
 
+	if m.SecurityGroupRules == nil {
+		m.SecurityGroupRules = make(map[string]*ec2.SecurityGroupRule)
+	}
+
+	for _, permission := range request.IpPermissions {
+
+		for _, iprange := range permission.IpRanges {
+
+			n := len(m.SecurityGroupRules) + 1
+			id := fmt.Sprintf("sgr-%d", n)
+			rule := &ec2.SecurityGroupRule{
+				SecurityGroupRuleId: &id,
+				GroupId:             sg.GroupId,
+				FromPort:            permission.FromPort,
+				ToPort:              permission.ToPort,
+				IsEgress:            aws.Bool(true),
+				CidrIpv4:            iprange.CidrIp,
+				IpProtocol:          permission.IpProtocol,
+				Tags:                tagSpecificationsToTags(request.TagSpecifications, ec2.ResourceTypeSecurityGroupRule),
+			}
+			if permission.FromPort == nil {
+				rule.FromPort = aws.Int64(int64(-1))
+			}
+			if permission.ToPort == nil {
+				rule.ToPort = aws.Int64(int64(-1))
+			}
+
+			m.SecurityGroupRules[id] = rule
+		}
+
+		for _, iprange := range permission.Ipv6Ranges {
+
+			n := len(m.SecurityGroupRules) + 1
+			id := fmt.Sprintf("sgr-%d", n)
+			rule := &ec2.SecurityGroupRule{
+				SecurityGroupRuleId: &id,
+				GroupId:             sg.GroupId,
+				FromPort:            permission.FromPort,
+				ToPort:              permission.ToPort,
+				IsEgress:            aws.Bool(true),
+				CidrIpv6:            iprange.CidrIpv6,
+				IpProtocol:          permission.IpProtocol,
+				Tags:                tagSpecificationsToTags(request.TagSpecifications, ec2.ResourceTypeSecurityGroupRule),
+			}
+			if permission.FromPort == nil {
+				rule.FromPort = aws.Int64(int64(-1))
+			}
+			if permission.ToPort == nil {
+				rule.ToPort = aws.Int64(int64(-1))
+			}
+
+			m.SecurityGroupRules[id] = rule
+		}
+	}
+
 	response := &ec2.AuthorizeSecurityGroupEgressOutput{}
 	return response, nil
 }
@@ -325,6 +380,111 @@ func (m *MockEC2) AuthorizeSecurityGroupIngress(request *ec2.AuthorizeSecurityGr
 
 	// TODO: We need to fold permissions
 
+	if m.SecurityGroupRules == nil {
+		m.SecurityGroupRules = make(map[string]*ec2.SecurityGroupRule)
+	}
+
+	for _, permission := range request.IpPermissions {
+
+		for _, iprange := range permission.IpRanges {
+
+			n := len(m.SecurityGroupRules) + 1
+			id := fmt.Sprintf("sgr-%d", n)
+			rule := &ec2.SecurityGroupRule{
+				SecurityGroupRuleId: &id,
+				GroupId:             sg.GroupId,
+				FromPort:            permission.FromPort,
+				ToPort:              permission.ToPort,
+				IsEgress:            aws.Bool(false),
+				CidrIpv4:            iprange.CidrIp,
+				IpProtocol:          permission.IpProtocol,
+				Tags:                tagSpecificationsToTags(request.TagSpecifications, ec2.ResourceTypeSecurityGroupRule),
+			}
+			if permission.FromPort == nil {
+				rule.FromPort = aws.Int64(int64(-1))
+			}
+			if permission.ToPort == nil {
+				rule.ToPort = aws.Int64(int64(-1))
+			}
+
+			m.SecurityGroupRules[id] = rule
+		}
+
+		for _, iprange := range permission.Ipv6Ranges {
+
+			n := len(m.SecurityGroupRules) + 1
+			id := fmt.Sprintf("sgr-%d", n)
+			rule := &ec2.SecurityGroupRule{
+				SecurityGroupRuleId: &id,
+				GroupId:             sg.GroupId,
+				FromPort:            permission.FromPort,
+				ToPort:              permission.ToPort,
+				IsEgress:            aws.Bool(false),
+				CidrIpv6:            iprange.CidrIpv6,
+				IpProtocol:          permission.IpProtocol,
+				Tags:                tagSpecificationsToTags(request.TagSpecifications, ec2.ResourceTypeSecurityGroupRule),
+			}
+			if permission.FromPort == nil {
+				rule.FromPort = aws.Int64(int64(-1))
+			}
+			if permission.ToPort == nil {
+				rule.ToPort = aws.Int64(int64(-1))
+			}
+
+			m.SecurityGroupRules[id] = rule
+		}
+
+		for _, group := range permission.UserIdGroupPairs {
+
+			n := len(m.SecurityGroupRules) + 1
+			id := fmt.Sprintf("sgr-%d", n)
+			rule := &ec2.SecurityGroupRule{
+				SecurityGroupRuleId: &id,
+				GroupId:             sg.GroupId,
+				FromPort:            permission.FromPort,
+				ToPort:              permission.ToPort,
+				IsEgress:            aws.Bool(false),
+				ReferencedGroupInfo: &ec2.ReferencedSecurityGroup{
+					GroupId: group.GroupId,
+				},
+				IpProtocol: permission.IpProtocol,
+				Tags:       tagSpecificationsToTags(request.TagSpecifications, ec2.ResourceTypeSecurityGroupRule),
+			}
+			if permission.FromPort == nil {
+				rule.FromPort = aws.Int64(int64(-1))
+			}
+			if permission.ToPort == nil {
+				rule.ToPort = aws.Int64(int64(-1))
+			}
+
+			m.SecurityGroupRules[id] = rule
+		}
+	}
+
 	response := &ec2.AuthorizeSecurityGroupIngressOutput{}
 	return response, nil
 }
+
+func (m *MockEC2) DescribeSecurityGroupRules(request *ec2.DescribeSecurityGroupRulesInput) (*ec2.DescribeSecurityGroupRulesOutput, error) {
+	m.mutex.Lock()
+	defer m.mutex.Unlock()
+
+	rules := []*ec2.SecurityGroupRule{}
+
+	sgid := ""
+	for _, filter := range request.Filters {
+		if aws.StringValue(filter.Name) == "group-id" {
+			sgid = aws.StringValue(filter.Values[0])
+		}
+	}
+
+	for _, rule := range m.SecurityGroupRules {
+		if aws.StringValue(rule.GroupId) == sgid {
+			rules = append(rules, rule)
+		}
+	}
+
+	return &ec2.DescribeSecurityGroupRulesOutput{
+		SecurityGroupRules: rules,
+	}, nil
+}

pkg/model/awsmodel/firewall.go

@@ -429,6 +429,12 @@ func AddDirectionalGroupRule(c *fi.ModelBuilderContext, t *awstasks.SecurityGrou
 
 	name := generateName(t)
 	t.Name = fi.String(name)
+	tags := make(map[string]string)
+	for key, value := range t.SecurityGroup.Tags {
+		tags[key] = value
+	}
+	tags["Name"] = *t.Name
+	t.Tags = tags
 
 	klog.V(8).Infof("Adding rule %v", name)
 	c.AddTask(t)

upup/pkg/fi/cloudup/awstasks/securitygroup.go

@@ -261,26 +261,24 @@ func (e *SecurityGroup) CloudformationLink() *cloudformation.Literal {
 }
 
 type deleteSecurityGroupRule struct {
-	groupID    *string
+	rule *ec2.SecurityGroupRule
-	permission *ec2.IpPermission
-	egress     bool
 }
 
 var _ fi.Deletion = &deleteSecurityGroupRule{}
 
 func (d *deleteSecurityGroupRule) Delete(t fi.Target) error {
-	klog.V(2).Infof("deleting security group permission: %v", fi.DebugAsJsonString(d.permission))
+	klog.V(2).Infof("deleting security group permission: %v", fi.DebugAsJsonString(d.rule))
 
 	awsTarget, ok := t.(*awsup.AWSAPITarget)
 	if !ok {
 		return fmt.Errorf("unexpected target type for deletion: %T", t)
 	}
 
-	if d.egress {
+	if fi.BoolValue(d.rule.IsEgress) {
 		request := &ec2.RevokeSecurityGroupEgressInput{
-			GroupId: d.groupID,
+			GroupId:              d.rule.GroupId,
+			SecurityGroupRuleIds: []*string{d.rule.SecurityGroupRuleId},
 		}
-		request.IpPermissions = []*ec2.IpPermission{d.permission}
 
 		klog.V(2).Infof("Calling EC2 RevokeSecurityGroupEgress")
 		_, err := awsTarget.Cloud.EC2().RevokeSecurityGroupEgress(request)
@@ -289,9 +287,9 @@ func (d *deleteSecurityGroupRule) Delete(t fi.Target) error {
 		}
 	} else {
 		request := &ec2.RevokeSecurityGroupIngressInput{
-			GroupId: d.groupID,
+			GroupId:              d.rule.GroupId,
+			SecurityGroupRuleIds: []*string{d.rule.SecurityGroupRuleId},
 		}
-		request.IpPermissions = []*ec2.IpPermission{d.permission}
 
 		klog.V(2).Infof("Calling EC2 RevokeSecurityGroupIngress")
 		_, err := awsTarget.Cloud.EC2().RevokeSecurityGroupIngress(request)
@@ -308,8 +306,8 @@ func (d *deleteSecurityGroupRule) TaskName() string {
 }
 
 func (d *deleteSecurityGroupRule) Item() string {
-	s := fi.StringValue(d.groupID) + ":"
+	s := fi.StringValue(d.rule.GroupId) + ":"
-	p := d.permission
+	p := d.rule
 	if aws.Int64Value(p.FromPort) != 0 {
 		s += fmt.Sprintf(" port=%d", aws.Int64Value(p.FromPort))
 		if aws.Int64Value(p.ToPort) != aws.Int64Value(p.FromPort) {
@@ -319,59 +317,17 @@ func (d *deleteSecurityGroupRule) Item() string {
 	if aws.StringValue(p.IpProtocol) != "-1" {
 		s += fmt.Sprintf(" protocol=%s", aws.StringValue(p.IpProtocol))
 	}
-	for _, ug := range p.UserIdGroupPairs {
+	if p.ReferencedGroupInfo != nil {
-		s += fmt.Sprintf(" group=%s", aws.StringValue(ug.GroupId))
+		s += fmt.Sprintf(" group=%s", aws.StringValue(p.ReferencedGroupInfo.GroupId))
-	}
-	for _, r := range p.IpRanges {
-		s += fmt.Sprintf(" ip=%s", aws.StringValue(r.CidrIp))
-	}
-	for _, r := range p.Ipv6Ranges {
-		s += fmt.Sprintf(" ipv6=%s", aws.StringValue(r.CidrIpv6))
 	}
+	s += fmt.Sprintf(" ip=%s", aws.StringValue(p.CidrIpv4))
+	s += fmt.Sprintf(" ipv6=%s", aws.StringValue(p.CidrIpv6))
 	//permissionString := fi.DebugAsJsonString(d.permission)
 	//s += permissionString
 
 	return s
 }
 
-func expandPermissions(sgID *string, permission *ec2.IpPermission, egress bool) []*ec2.IpPermission {
-	var rules []*ec2.IpPermission
-
-	master := &ec2.IpPermission{
-		FromPort:   permission.FromPort,
-		ToPort:     permission.ToPort,
-		IpProtocol: permission.IpProtocol,
-	}
-
-	for _, ipRange := range permission.IpRanges {
-		a := &ec2.IpPermission{}
-		*a = *master
-		a.IpRanges = []*ec2.IpRange{ipRange}
-		rules = append(rules, a)
-	}
-
-	for _, ipv6Range := range permission.Ipv6Ranges {
-		a := &ec2.IpPermission{}
-		*a = *master
-		a.Ipv6Ranges = []*ec2.Ipv6Range{ipv6Range}
-		rules = append(rules, a)
-	}
-
-	for _, ug := range permission.UserIdGroupPairs {
-		a := &ec2.IpPermission{}
-		*a = *master
-		a.UserIdGroupPairs = []*ec2.UserIdGroupPair{ug}
-		rules = append(rules, a)
-	}
-
-	if len(rules) == 0 {
-		// If there are no group or cidr restrictions, it is just a generic rule
-		rules = append(rules, master)
-	}
-
-	return rules
-}
-
 func (e *SecurityGroup) FindDeletions(c *fi.Context) ([]fi.Deletion, error) {
 	var removals []fi.Deletion
 
@@ -396,13 +352,20 @@ func (e *SecurityGroup) FindDeletions(c *fi.Context) ([]fi.Deletion, error) {
 		return nil, nil
 	}
 
-	var ingress []*ec2.IpPermission
+	cloud := c.Cloud.(awsup.AWSCloud)
-	for _, permission := range sg.IpPermissions {
+
-		rules := expandPermissions(sg.GroupId, permission, false)
+	request := &ec2.DescribeSecurityGroupRulesInput{
-		ingress = append(ingress, rules...)
+		Filters: []*ec2.Filter{
+			awsup.NewEC2Filter("group-id", *e.ID),
+		},
 	}
 
-	for _, permission := range ingress {
+	response, err := cloud.EC2().DescribeSecurityGroupRules(request)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, permission := range response.SecurityGroupRules {
 		// Because of #478, we can't remove all non-matching security groups
 		// Instead we consider only certain rules to be 'in-scope'
 		// (in the model, we typically consider only rules on port 22 and 443)
@@ -436,34 +399,7 @@ func (e *SecurityGroup) FindDeletions(c *fi.Context) ([]fi.Deletion, error) {
 		}
 		if !found {
 			removals = append(removals, &deleteSecurityGroupRule{
-				groupID:    sg.GroupId,
+				rule: permission,
-				permission: permission,
-				egress:     false,
-			})
-		}
-	}
-
-	var egress []*ec2.IpPermission
-	for _, permission := range sg.IpPermissionsEgress {
-		rules := expandPermissions(sg.GroupId, permission, true)
-		egress = append(egress, rules...)
-	}
-	for _, permission := range egress {
-		found := false
-		for _, t := range c.AllTasks() {
-			er, ok := t.(*SecurityGroupRule)
-			if !ok {
-				continue
-			}
-			if er.matches(permission) {
-				found = true
-			}
-		}
-		if !found {
-			removals = append(removals, &deleteSecurityGroupRule{
-				groupID:    sg.GroupId,
-				permission: permission,
-				egress:     true,
 			})
 		}
 	}
@@ -473,7 +409,7 @@ func (e *SecurityGroup) FindDeletions(c *fi.Context) ([]fi.Deletion, error) {
 
 // RemovalRule is a rule that filters the permissions we should remove
 type RemovalRule interface {
-	Matches(permission *ec2.IpPermission) bool
+	Matches(permission *ec2.SecurityGroupRule) bool
 }
 
 // ParseRemovalRule parses our removal rule DSL into a RemovalRule
@@ -511,7 +447,7 @@ func (r *PortRemovalRule) String() string {
 	return fi.DebugAsJsonString(r)
 }
 
-func (r *PortRemovalRule) Matches(permission *ec2.IpPermission) bool {
+func (r *PortRemovalRule) Matches(permission *ec2.SecurityGroupRule) bool {
 	// Check if port matches
 	if permission.FromPort == nil || *permission.FromPort != int64(r.Port) {
 		return false

upup/pkg/fi/cloudup/awstasks/securitygroup_test.go

@@ -60,34 +60,34 @@ func testParsesAsPort(t *testing.T, rule string, port int) {
 
 func TestPortRemovalRule(t *testing.T) {
 	r := &PortRemovalRule{Port: 22}
-	testMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(22), ToPort: aws.Int64(22)})
+	testMatches(t, r, &ec2.SecurityGroupRule{FromPort: aws.Int64(22), ToPort: aws.Int64(22)})
 
-	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(0), ToPort: aws.Int64(0)})
+	testNotMatches(t, r, &ec2.SecurityGroupRule{FromPort: aws.Int64(0), ToPort: aws.Int64(0)})
-	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(23), ToPort: aws.Int64(23)})
+	testNotMatches(t, r, &ec2.SecurityGroupRule{FromPort: aws.Int64(23), ToPort: aws.Int64(23)})
-	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(20), ToPort: aws.Int64(22)})
+	testNotMatches(t, r, &ec2.SecurityGroupRule{FromPort: aws.Int64(20), ToPort: aws.Int64(22)})
-	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(22), ToPort: aws.Int64(23)})
+	testNotMatches(t, r, &ec2.SecurityGroupRule{FromPort: aws.Int64(22), ToPort: aws.Int64(23)})
-	testNotMatches(t, r, &ec2.IpPermission{ToPort: aws.Int64(22)})
+	testNotMatches(t, r, &ec2.SecurityGroupRule{ToPort: aws.Int64(22)})
-	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(22)})
+	testNotMatches(t, r, &ec2.SecurityGroupRule{FromPort: aws.Int64(22)})
-	testNotMatches(t, r, &ec2.IpPermission{})
+	testNotMatches(t, r, &ec2.SecurityGroupRule{})
 }
 
 func TestPortRemovalRule_Zero(t *testing.T) {
 	r := &PortRemovalRule{Port: 0}
-	testMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(0), ToPort: aws.Int64(0)})
+	testMatches(t, r, &ec2.SecurityGroupRule{FromPort: aws.Int64(0), ToPort: aws.Int64(0)})
 
-	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(0), ToPort: aws.Int64(20)})
+	testNotMatches(t, r, &ec2.SecurityGroupRule{FromPort: aws.Int64(0), ToPort: aws.Int64(20)})
-	testNotMatches(t, r, &ec2.IpPermission{ToPort: aws.Int64(0)})
+	testNotMatches(t, r, &ec2.SecurityGroupRule{ToPort: aws.Int64(0)})
-	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(0)})
+	testNotMatches(t, r, &ec2.SecurityGroupRule{FromPort: aws.Int64(0)})
-	testNotMatches(t, r, &ec2.IpPermission{})
+	testNotMatches(t, r, &ec2.SecurityGroupRule{})
 }
 
-func testMatches(t *testing.T, rule *PortRemovalRule, permission *ec2.IpPermission) {
+func testMatches(t *testing.T, rule *PortRemovalRule, permission *ec2.SecurityGroupRule) {
 	if !rule.Matches(permission) {
 		t.Fatalf("rule %q failed to match permission %q", rule, permission)
 	}
 }
 
-func testNotMatches(t *testing.T, rule *PortRemovalRule, permission *ec2.IpPermission) {
+func testNotMatches(t *testing.T, rule *PortRemovalRule, permission *ec2.SecurityGroupRule) {
 	if rule.Matches(permission) {
 		t.Fatalf("rule %q unexpectedly matched permission %q", rule, permission)
 	}

upup/pkg/fi/cloudup/awstasks/securitygrouprule.go

@@ -34,6 +34,7 @@ import (
 
 // +kops:fitask
 type SecurityGroupRule struct {
+	ID        *string
 	Name      *string
 	Lifecycle fi.Lifecycle
 
@@ -49,6 +50,8 @@ type SecurityGroupRule struct {
 	SourceGroup *SecurityGroup
 
 	Egress *bool
+
+	Tags map[string]string
 }
 
 func (e *SecurityGroupRule) Find(c *fi.Context) (*SecurityGroupRule, error) {
@@ -63,35 +66,24 @@ func (e *SecurityGroupRule) Find(c *fi.Context) (*SecurityGroupRule, error) {
 		return nil, nil
 	}
 
-	request := &ec2.DescribeSecurityGroupsInput{
+	request := &ec2.DescribeSecurityGroupRulesInput{
 		Filters: []*ec2.Filter{
 			awsup.NewEC2Filter("group-id", *e.SecurityGroup.ID),
 		},
 	}
 
-	response, err := cloud.EC2().DescribeSecurityGroups(request)
+	response, err := cloud.EC2().DescribeSecurityGroupRules(request)
 	if err != nil {
 		return nil, fmt.Errorf("error listing SecurityGroup: %v", err)
 	}
 
-	if response == nil || len(response.SecurityGroups) == 0 {
+	if response == nil || len(response.SecurityGroupRules) == 0 {
 		return nil, nil
 	}
 
-	if len(response.SecurityGroups) != 1 {
+	var foundRule *ec2.SecurityGroupRule
-		klog.Fatalf("found multiple security groups for id=%s", *e.SecurityGroup.ID)
-	}
-	sg := response.SecurityGroups[0]
-	//klog.V(2).Info("found existing security group")
 
-	var foundRule *ec2.IpPermission
+	for _, rule := range response.SecurityGroupRules {
-
-	ipPermissions := sg.IpPermissions
-	if fi.BoolValue(e.Egress) {
-		ipPermissions = sg.IpPermissionsEgress
-	}
-
-	for _, rule := range ipPermissions {
 		if e.matches(rule) {
 			foundRule = rule
 			break
@@ -100,17 +92,30 @@ func (e *SecurityGroupRule) Find(c *fi.Context) (*SecurityGroupRule, error) {
 
 	if foundRule != nil {
 		actual := &SecurityGroupRule{
+			ID:            foundRule.SecurityGroupRuleId,
 			Name:          e.Name,
 			SecurityGroup: &SecurityGroup{ID: e.SecurityGroup.ID},
 			FromPort:      foundRule.FromPort,
 			ToPort:        foundRule.ToPort,
 			Protocol:      foundRule.IpProtocol,
 			Egress:        e.Egress,
+
+			Tags: intersectTags(foundRule.Tags, e.Tags),
 		}
 
 		if aws.StringValue(actual.Protocol) == "-1" {
 			actual.Protocol = nil
 		}
+
+		if fi.StringValue(actual.Protocol) != "icmpv6" {
+			if fi.Int64Value(actual.FromPort) == int64(-1) {
+				actual.FromPort = nil
+			}
+			if fi.Int64Value(actual.ToPort) == int64(-1) {
+				actual.ToPort = nil
+			}
+		}
+
 		if e.CIDR != nil {
 			actual.CIDR = e.CIDR
 		}
@@ -124,17 +129,27 @@ func (e *SecurityGroupRule) Find(c *fi.Context) (*SecurityGroupRule, error) {
 		// Avoid spurious changes
 		actual.Lifecycle = e.Lifecycle
 
+		e.ID = actual.ID
+
 		return actual, nil
 	}
-
 	return nil, nil
 }
 
-func (e *SecurityGroupRule) matches(rule *ec2.IpPermission) bool {
+func (e *SecurityGroupRule) matches(rule *ec2.SecurityGroupRule) bool {
-	if aws.Int64Value(rule.FromPort) != aws.Int64Value(e.FromPort) {
+	matchFromPort := int64(-1)
+	if e.FromPort != nil {
+		matchFromPort = *e.FromPort
+	}
+	if aws.Int64Value(rule.FromPort) != matchFromPort {
 		return false
 	}
-	if aws.Int64Value(rule.ToPort) != aws.Int64Value(e.ToPort) {
+
+	matchToPort := int64(-1)
+	if e.ToPort != nil {
+		matchToPort = *e.ToPort
+	}
+	if aws.Int64Value(rule.ToPort) != matchToPort {
 		return false
 	}
 
@@ -146,50 +161,19 @@ func (e *SecurityGroupRule) matches(rule *ec2.IpPermission) bool {
 		return false
 	}
 
-	if e.CIDR != nil {
+	if fi.StringValue(e.CIDR) != fi.StringValue(rule.CidrIpv4) {
-		match := false
+		return false
-		for _, ipRange := range rule.IpRanges {
-			if aws.StringValue(ipRange.CidrIp) == *e.CIDR {
-				match = true
-				break
-			}
-		}
-		if !match {
-			return false
-		}
 	}
 
-	if e.IPv6CIDR != nil {
+	if fi.StringValue(e.IPv6CIDR) != fi.StringValue(rule.CidrIpv6) {
-		match := false
+		return false
-		for _, ipv6Range := range rule.Ipv6Ranges {
-			if aws.StringValue(ipv6Range.CidrIpv6) == *e.IPv6CIDR {
-				match = true
-				break
-			}
-		}
-		if !match {
-			return false
-		}
 	}
 
-	if e.SourceGroup != nil {
+	if e.SourceGroup != nil || rule.ReferencedGroupInfo != nil {
-		match := false
+		if e.SourceGroup == nil || rule.ReferencedGroupInfo == nil {
-		for _, spec := range rule.UserIdGroupPairs {
+			return false
-			if e.SourceGroup == nil {
-				continue
-			}
-
-			if e.SourceGroup.ID == nil {
-				klog.Warningf("SourceGroup had nil ID: %v", e.SourceGroup)
-				continue
-			}
-
-			if aws.StringValue(spec.GroupId) == *e.SourceGroup.ID {
-				match = true
-				break
-			}
 		}
-		if !match {
+		if fi.StringValue(e.SourceGroup.ID) != fi.StringValue(rule.ReferencedGroupInfo.GroupId) {
 			return false
 		}
 	}
@@ -295,6 +279,7 @@ func (_ *SecurityGroupRule) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Secu
 				GroupId: e.SecurityGroup.ID,
 			}
 			request.IpPermissions = []*ec2.IpPermission{ipPermission}
+			request.TagSpecifications = awsup.EC2TagSpecification(ec2.ResourceTypeSecurityGroupRule, e.Tags)
 
 			klog.V(2).Infof("%s: Calling EC2 AuthorizeSecurityGroupEgress (%s)", name, description)
 			_, err := t.Cloud.EC2().AuthorizeSecurityGroupEgress(request)
@@ -306,6 +291,7 @@ func (_ *SecurityGroupRule) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Secu
 				GroupId: e.SecurityGroup.ID,
 			}
 			request.IpPermissions = []*ec2.IpPermission{ipPermission}
+			request.TagSpecifications = awsup.EC2TagSpecification(ec2.ResourceTypeSecurityGroupRule, e.Tags)
 
 			klog.V(2).Infof("%s: Calling EC2 AuthorizeSecurityGroupIngress (%s)", name, description)
 			_, err := t.Cloud.EC2().AuthorizeSecurityGroupIngress(request)
@@ -314,6 +300,8 @@ func (_ *SecurityGroupRule) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Secu
 			}
 		}
 
+	} else if changes.Tags != nil {
+		return t.AddAWSTags(*a.ID, e.Tags)
 	}
 
 	// No tags on security group rules (there are tags on the group though)