fix: persist security group rule tags and report -1 ports for all-protocol rules (#1083)

* fix: persist security group rule tags and report -1 ports for all-protocol rules

A security group rule accepted a security-group-rule TagSpecification and kept
none of it, so DescribeSecurityGroupRules answered with no tags and every
subsequent plan offered to add them again. SGRule now carries a Tags map,
populated on both authorize paths and projected into the central tag store, and
DescribeSecurityGroupRules evaluates tag: and tag-key filters against it instead
of excluding every rule.

CreateTags and DeleteTags on an sgr- id were a silent no-op because
updateRecordTags routed only subnet-, vpc-, sg- and eni-. It now finds the rule
inside its parent group's record and mutates it there, and a revoked rule's
central entry is cleared with it.

An all-protocol rule reported the ports it was stored with, 0 and 0, where AWS
reports -1 and -1, which is the same never-converging plan by another route.

* fix: recognise sgr- as a security-group-rule resource id

DescribeTags reported ResourceType "unknown" for a security group rule, so a
resource-type filter naming security-group-rule matched none of them, and the
authorizer could build no ARN for a rule id and fell back to the any-resource
wildcard. Both read the same prefix table, which had no entry for sgr-.

spinifex/arn/ec2.go

@@ -21,6 +21,7 @@ const (
 	EC2VPC                       EC2ResourceType = "vpc"
 	EC2Subnet                    EC2ResourceType = "subnet"
 	EC2SecurityGroup             EC2ResourceType = "security-group"
+	EC2SecurityGroupRule         EC2ResourceType = "security-group-rule"
 	EC2RouteTable                EC2ResourceType = "route-table"
 	EC2InternetGateway           EC2ResourceType = "internet-gateway"
 	EC2EgressOnlyInternetGateway EC2ResourceType = "egress-only-internet-gateway"
@@ -52,6 +53,7 @@ var ec2IDPrefixes = []struct {
 	{"vpc-", EC2VPC},
 	{"subnet-", EC2Subnet},
 	{"sg-", EC2SecurityGroup},
+	{"sgr-", EC2SecurityGroupRule},
 	{"rtb-", EC2RouteTable},
 	{"igw-", EC2InternetGateway},
 	{"eigw-", EC2EgressOnlyInternetGateway},

spinifex/arn/ec2_test.go

@@ -20,6 +20,7 @@ func TestEC2TypeForID(t *testing.T) {
 		{"vpc-abc123", arn.EC2VPC, true},
 		{"subnet-abc123", arn.EC2Subnet, true},
 		{"sg-abc123", arn.EC2SecurityGroup, true},
+		{"sgr-abc123", arn.EC2SecurityGroupRule, true},
 		{"rtb-abc123", arn.EC2RouteTable, true},
 		{"igw-abc123", arn.EC2InternetGateway, true},
 		{"eigw-abc123", arn.EC2EgressOnlyInternetGateway, true},

spinifex/daemon/daemon_handlers_tag.go

@@ -30,7 +30,7 @@ type recordTagMirror interface {
 }
 
 // recordTagMirrors collects the initialized services owning centrally-stored
-// records: vpc covers vpc/subnet/sg/eni, the rest own their prefix.
+// records: vpc covers vpc/subnet/sg/sgr/eni, the rest own their prefix.
 func (d *Daemon) recordTagMirrors() []recordTagMirror {
 	var mirrors []recordTagMirror
 	if d.vpcService != nil {

spinifex/handlers/ec2/vpc/security_group.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"maps"
 	"net"
 	"regexp"
 	"slices"
@@ -29,6 +30,10 @@ var sgIDRegex = regexp.MustCompile(`^sg-[0-9a-f]{17}$`)
 // re-implementing the format check.
 var SGRuleIDRegex = regexp.MustCompile(`^sgr-[0-9a-f]{17}$`)
 
+// allProtocols is the IpProtocol value naming every protocol, which is also
+// the port value AWS reports for such a rule.
+const allProtocols = "-1"
+
 // canonicalIPProtocols are the only values the OVN ACL builder can express.
 // Anything else must be rejected: the builder emits no L4 predicate for an
 // unknown protocol, which widens the rule to every IP protocol.
@@ -129,6 +134,10 @@ type SGRule struct {
 	// so revoke/duplicate matching ignores it. The AWS Load Balancer Controller
 	// tags the node-SG rules it manages here and revokes them by this value.
 	Description string `json:"description,omitempty"`
+	// Tags are excluded from sgRuleKey for the same reason as Description: two
+	// rules with identical permissions are the same rule whatever they are
+	// labelled.
+	Tags map[string]string `json:"tags,omitempty"`
 }
 
 // SecurityGroupRecord represents a stored security group.
@@ -811,13 +820,11 @@ func (s *VPCServiceImpl) DescribeSecurityGroupRules(ctx context.Context, input *
 }
 
 // sgRuleMatchesFilters applies the DescribeSecurityGroupRules filter set to a
-// single rule. Rule-level tags are not yet persisted, so tag:* and tag-key
-// filters always return false.
+// single rule.
 func sgRuleMatchesFilters(record *SecurityGroupRecord, rule SGRule, filters map[string][]string) bool {
 	for name, values := range filters {
 		if strings.HasPrefix(name, "tag:") {
-			// rule.Tags is not yet populated; any tag:* filter excludes every rule.
-			return false
+			continue
 		}
 		switch name {
 		case "group-id":
@@ -829,8 +836,9 @@ func sgRuleMatchesFilters(record *SecurityGroupRecord, rule SGRule, filters map[
 				return false
 			}
 		case "tag-key":
-			// rule.Tags is not yet populated; tag-key excludes every rule.
-			return false
+			if !sgRuleMatchesTagKey(rule.Tags, values) {
+				return false
+			}
 		default:
 			// Unreachable: ParseFilters rejects names not in the valid set.
 			// Logs an error if a future map entry lacks a matching case.
@@ -838,21 +846,121 @@ func sgRuleMatchesFilters(record *SecurityGroupRecord, rule SGRule, filters map[
 			return false
 		}
 	}
-	return true
+	return filterutil.MatchesTags(filters, rule.Tags)
+}
+
+// updateSGRuleTags applies mut to the tags of the rule named by ruleID, which
+// lives inside its parent group's record rather than a record of its own. The
+// parent is found by scanning the account's groups: a rule id carries no
+// pointer back to the group holding it.
+func (s *VPCServiceImpl) updateSGRuleTags(ctx context.Context, accountID, ruleID string, mut func(map[string]string)) error {
+	prefix := accountID + "."
+	keys, err := s.sgKV.Keys(ctx)
+	if err != nil {
+		if errors.Is(err, jetstream.ErrNoKeysFound) {
+			return nil
+		}
+		slog.ErrorContext(ctx, "updateSGRuleTags: key listing failed", "ruleId", ruleID, "err", err)
+		return errors.New(awserrors.ErrorServerInternal)
+	}
+
+	for _, key := range keys {
+		if key == utils.VersionKey || !strings.HasPrefix(key, prefix) {
+			continue
+		}
+		entry, err := s.sgKV.Get(ctx, key)
+		if err != nil {
+			slog.ErrorContext(ctx, "updateSGRuleTags: SG read failed", "key", key, "err", err)
+			return errors.New(awserrors.ErrorServerInternal)
+		}
+		var record SecurityGroupRecord
+		if err := json.Unmarshal(entry.Value(), &record); err != nil {
+			slog.ErrorContext(ctx, "updateSGRuleTags: SG unmarshal failed", "key", key, "err", err)
+			return errors.New(awserrors.ErrorServerInternal)
+		}
+
+		rule := findSGRuleByID(record.IngressRules, ruleID)
+		if rule == nil {
+			rule = findSGRuleByID(record.EgressRules, ruleID)
+		}
+		if rule == nil {
+			continue
+		}
+		if rule.Tags == nil {
+			rule.Tags = map[string]string{}
+		}
+		mut(rule.Tags)
+
+		data, err := json.Marshal(record)
+		if err != nil {
+			return fmt.Errorf("failed to marshal security group record: %w", err)
+		}
+		// Compare-and-swap on the revision read above: a concurrent authorize on
+		// the same group would otherwise lose its rule to this write.
+		if _, err := s.sgKV.Update(ctx, key, data, entry.Revision()); err != nil {
+			slog.ErrorContext(ctx, "updateSGRuleTags: SG write failed", "key", key, "ruleId", ruleID, "err", err)
+			return errors.New(awserrors.ErrorServerInternal)
+		}
+		return nil
+	}
+	return nil
+}
+
+// findSGRuleByID returns a pointer into rules so a caller can mutate the stored
+// rule in place.
+func findSGRuleByID(rules []SGRule, ruleID string) *SGRule {
+	for i := range rules {
+		if rules[i].RuleId == ruleID {
+			return &rules[i]
+		}
+	}
+	return nil
+}
+
+// sgRuleMatchesTagKey reports whether the rule carries any of the named tag
+// keys, whatever their values.
+func sgRuleMatchesTagKey(tags map[string]string, keys []string) bool {
+	for key := range tags {
+		if filterutil.MatchesAny(keys, key) {
+			return true
+		}
+	}
+	return false
+}
+
+// applySGRuleTags stamps the security-group-rule TagSpecification onto every
+// rule an authorize call creates. AWS tags the rules the call creates and no
+// others, so this runs on the new rules alone.
+func applySGRuleTags(rules []SGRule, specs []*ec2.TagSpecification) []SGRule {
+	tags := utils.ExtractTags(specs, ec2.ResourceTypeSecurityGroupRule)
+	if len(tags) == 0 {
+		return rules
+	}
+	for i := range rules {
+		rules[i].Tags = maps.Clone(tags)
+	}
+	return rules
 }
 
 // sgRuleToSecurityGroupRule flattens a stored SGRule into the AWS API shape.
 // accountID supplies GroupOwnerId and ReferencedGroupInfo.UserId; VpcId is
 // derived from the parent record (same-VPC references are enforced on write).
 func sgRuleToSecurityGroupRule(record *SecurityGroupRecord, rule SGRule, isEgress bool, accountID string) *ec2.SecurityGroupRule {
+	// An all-protocol rule has no ports to report. AWS answers -1 for both;
+	// reporting the stored zeroes offers a caller two ports it never sent.
+	fromPort, toPort := rule.FromPort, rule.ToPort
+	if rule.IpProtocol == allProtocols {
+		fromPort, toPort = -1, -1
+	}
 	out := &ec2.SecurityGroupRule{
 		SecurityGroupRuleId: aws.String(rule.RuleId),
 		GroupId:             aws.String(record.GroupId),
 		GroupOwnerId:        aws.String(accountID),
 		IsEgress:            aws.Bool(isEgress),
 		IpProtocol:          aws.String(rule.IpProtocol),
-		FromPort:            aws.Int64(rule.FromPort),
-		ToPort:              aws.Int64(rule.ToPort),
+		FromPort:            aws.Int64(fromPort),
+		ToPort:              aws.Int64(toPort),
+		Tags:                utils.MapToEC2Tags(rule.Tags),
 	}
 	if rule.Description != "" {
 		out.Description = aws.String(rule.Description)
@@ -901,6 +1009,7 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupIngress(ctx context.Context, inpu
 		slog.WarnContext(ctx, "AuthorizeSecurityGroupIngress: invalid rule", "groupId", groupId, "err", err)
 		return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", err)
 	}
+	newRules = applySGRuleTags(newRules, input.TagSpecifications)
 	if err := s.validateSGRuleReferences(ctx, accountID, record.VpcId, newRules); err != nil {
 		return nil, err
 	}
@@ -926,6 +1035,10 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupIngress(ctx context.Context, inpu
 		return nil, errors.New(awserrors.ErrorServerInternal)
 	}
 
+	for _, nr := range newRules {
+		s.projectRecordTags(ctx, accountID, nr.RuleId, nr.Tags)
+	}
+
 	slog.InfoContext(ctx, "AuthorizeSecurityGroupIngress completed", "groupId", groupId, "newRules", len(newRules), "accountID", accountID)
 
 	if err := s.requestSGEvent("vpc.update-sg", SGEvent{
@@ -976,6 +1089,7 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupEgress(ctx context.Context, input
 		slog.WarnContext(ctx, "AuthorizeSecurityGroupEgress: invalid rule", "groupId", groupId, "err", err)
 		return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", err)
 	}
+	newRules = applySGRuleTags(newRules, input.TagSpecifications)
 	if err := s.validateSGRuleReferences(ctx, accountID, record.VpcId, newRules); err != nil {
 		return nil, err
 	}
@@ -1001,6 +1115,10 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupEgress(ctx context.Context, input
 		return nil, errors.New(awserrors.ErrorServerInternal)
 	}
 
+	for _, nr := range newRules {
+		s.projectRecordTags(ctx, accountID, nr.RuleId, nr.Tags)
+	}
+
 	slog.InfoContext(ctx, "AuthorizeSecurityGroupEgress completed", "groupId", groupId, "newRules", len(newRules), "accountID", accountID)
 
 	if err := s.requestSGEvent("vpc.update-sg", SGEvent{
@@ -1058,6 +1176,7 @@ func (s *VPCServiceImpl) RevokeSecurityGroupIngress(ctx context.Context, input *
 	if err := assertRulesPresent(record.IngressRules, revokeRules); err != nil {
 		return nil, err
 	}
+	revokedIDs := removedSGRuleIDs(record.IngressRules, revokeRules)
 	record.IngressRules = removeSGRules(record.IngressRules, revokeRules)
 
 	data, err := json.Marshal(record)
@@ -1068,6 +1187,10 @@ func (s *VPCServiceImpl) RevokeSecurityGroupIngress(ctx context.Context, input *
 		return nil, errors.New(awserrors.ErrorServerInternal)
 	}
 
+	for _, id := range revokedIDs {
+		s.clearRecordTags(ctx, accountID, id)
+	}
+
 	slog.InfoContext(ctx, "RevokeSecurityGroupIngress completed", "groupId", groupId, "revokedRules", len(revokeRules), "accountID", accountID)
 
 	if err := s.requestSGEvent("vpc.update-sg", SGEvent{
@@ -1120,6 +1243,7 @@ func (s *VPCServiceImpl) RevokeSecurityGroupEgress(ctx context.Context, input *e
 	if err := assertRulesPresent(record.EgressRules, revokeRules); err != nil {
 		return nil, err
 	}
+	revokedIDs := removedSGRuleIDs(record.EgressRules, revokeRules)
 	record.EgressRules = removeSGRules(record.EgressRules, revokeRules)
 
 	data, err := json.Marshal(record)
@@ -1130,6 +1254,10 @@ func (s *VPCServiceImpl) RevokeSecurityGroupEgress(ctx context.Context, input *e
 		return nil, errors.New(awserrors.ErrorServerInternal)
 	}
 
+	for _, id := range revokedIDs {
+		s.clearRecordTags(ctx, accountID, id)
+	}
+
 	slog.InfoContext(ctx, "RevokeSecurityGroupEgress completed", "groupId", groupId, "revokedRules", len(revokeRules), "accountID", accountID)
 
 	if err := s.requestSGEvent("vpc.update-sg", SGEvent{
@@ -1575,6 +1703,24 @@ func assertRulesPresent(existing, toRevoke []SGRule) error {
 	return nil
 }
 
+// removedSGRuleIDs names the stored rules a revoke will drop. A revoke request
+// carries permissions rather than IDs, so the IDs whose tag entries have to go
+// with them can only be read off the stored rules before they are removed.
+func removedSGRuleIDs(existing, toRemove []SGRule) []string {
+	removeSet := make(map[string]bool, len(toRemove))
+	for _, r := range toRemove {
+		removeSet[sgRuleKey(r)] = true
+	}
+
+	var ids []string
+	for _, r := range existing {
+		if removeSet[sgRuleKey(r)] {
+			ids = append(ids, r.RuleId)
+		}
+	}
+	return ids
+}
+
 // removeSGRules removes matching rules from the existing set.
 func removeSGRules(existing, toRemove []SGRule) []SGRule {
 	removeSet := make(map[string]bool)

spinifex/handlers/ec2/vpc/security_group_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/mulgadc/spinifex/spinifex/awserrors"
+	"github.com/mulgadc/spinifex/spinifex/filterutil"
 	"github.com/mulgadc/spinifex/spinifex/testutil"
 	"github.com/mulgadc/spinifex/spinifex/utils"
 	"github.com/nats-io/nats.go"
@@ -2362,3 +2363,141 @@ func TestSecurityGroupRule_DescriptionRoundTrips(t *testing.T) {
 	_, err = svc.DeleteSecurityGroup(context.Background(), &ec2.DeleteSecurityGroupInput{GroupId: aws.String(backendSG)}, testAccountID)
 	require.NoError(t, err, "backend SG must delete once its node-SG reference is revoked")
 }
+
+func TestAuthorizeSecurityGroupIngress_StoresRuleTags(t *testing.T) {
+	t.Parallel()
+	svc := setupTestVPCService(t)
+	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
+	sgID := createTestSG(t, svc, vpcID, "rule-tags-sg")
+
+	out, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{
+		GroupId: aws.String(sgID),
+		IpPermissions: []*ec2.IpPermission{{
+			IpProtocol: aws.String("tcp"),
+			FromPort:   aws.Int64(80),
+			ToPort:     aws.Int64(80),
+			IpRanges:   []*ec2.IpRange{{CidrIp: aws.String("192.168.1.33/32")}},
+		}},
+		TagSpecifications: []*ec2.TagSpecification{{
+			ResourceType: aws.String(ec2.ResourceTypeSecurityGroupRule),
+			Tags:         []*ec2.Tag{{Key: aws.String("terraform-aws-modules"), Value: aws.String("alb")}},
+		}},
+	}, testAccountID)
+	require.NoError(t, err)
+	require.Len(t, out.SecurityGroupRules, 1)
+	ruleID := aws.StringValue(out.SecurityGroupRules[0].SecurityGroupRuleId)
+	assert.Equal(t, map[string]string{"terraform-aws-modules": "alb"},
+		filterutil.EC2TagsToMap(out.SecurityGroupRules[0].Tags), "authorize must echo the tags it stored")
+
+	desc, err := svc.DescribeSecurityGroupRules(context.Background(), &ec2.DescribeSecurityGroupRulesInput{
+		SecurityGroupRuleIds: []*string{aws.String(ruleID)},
+	}, testAccountID)
+	require.NoError(t, err)
+	require.Len(t, desc.SecurityGroupRules, 1)
+	assert.Equal(t, map[string]string{"terraform-aws-modules": "alb"},
+		filterutil.EC2TagsToMap(desc.SecurityGroupRules[0].Tags))
+}
+
+func TestDescribeSecurityGroupRules_FiltersByTag(t *testing.T) {
+	t.Parallel()
+	svc := setupTestVPCService(t)
+	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
+	sgID := createTestSG(t, svc, vpcID, "rule-tag-filter-sg")
+
+	tagged, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{
+		GroupId: aws.String(sgID),
+		IpPermissions: []*ec2.IpPermission{{
+			IpProtocol: aws.String("tcp"), FromPort: aws.Int64(443), ToPort: aws.Int64(443),
+			IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/8")}},
+		}},
+		TagSpecifications: []*ec2.TagSpecification{{
+			ResourceType: aws.String(ec2.ResourceTypeSecurityGroupRule),
+			Tags:         []*ec2.Tag{{Key: aws.String("owner"), Value: aws.String("alb")}},
+		}},
+	}, testAccountID)
+	require.NoError(t, err)
+	taggedID := aws.StringValue(tagged.SecurityGroupRules[0].SecurityGroupRuleId)
+
+	_, err = svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{
+		GroupId: aws.String(sgID),
+		IpPermissions: []*ec2.IpPermission{{
+			IpProtocol: aws.String("tcp"), FromPort: aws.Int64(8080), ToPort: aws.Int64(8080),
+			IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/8")}},
+		}},
+	}, testAccountID)
+	require.NoError(t, err)
+
+	byValue, err := svc.DescribeSecurityGroupRules(context.Background(), &ec2.DescribeSecurityGroupRulesInput{
+		Filters: []*ec2.Filter{{Name: aws.String("tag:owner"), Values: []*string{aws.String("alb")}}},
+	}, testAccountID)
+	require.NoError(t, err)
+	require.Len(t, byValue.SecurityGroupRules, 1)
+	assert.Equal(t, taggedID, aws.StringValue(byValue.SecurityGroupRules[0].SecurityGroupRuleId))
+
+	byKey, err := svc.DescribeSecurityGroupRules(context.Background(), &ec2.DescribeSecurityGroupRulesInput{
+		Filters: []*ec2.Filter{{Name: aws.String("tag-key"), Values: []*string{aws.String("owner")}}},
+	}, testAccountID)
+	require.NoError(t, err)
+	require.Len(t, byKey.SecurityGroupRules, 1)
+	assert.Equal(t, taggedID, aws.StringValue(byKey.SecurityGroupRules[0].SecurityGroupRuleId))
+}
+
+func TestDescribeSecurityGroupRules_AllProtocolReportsMinusOnePorts(t *testing.T) {
+	t.Parallel()
+	svc := setupTestVPCService(t)
+	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
+	sgID := createTestSG(t, svc, vpcID, "all-proto-sg")
+
+	out, err := svc.AuthorizeSecurityGroupEgress(context.Background(), &ec2.AuthorizeSecurityGroupEgressInput{
+		GroupId: aws.String(sgID),
+		IpPermissions: []*ec2.IpPermission{{
+			IpProtocol: aws.String("-1"),
+			IpRanges:   []*ec2.IpRange{{CidrIp: aws.String("10.1.0.0/16")}},
+		}},
+	}, testAccountID)
+	require.NoError(t, err)
+	require.Len(t, out.SecurityGroupRules, 1)
+	assert.Equal(t, int64(-1), aws.Int64Value(out.SecurityGroupRules[0].FromPort))
+	assert.Equal(t, int64(-1), aws.Int64Value(out.SecurityGroupRules[0].ToPort))
+}
+
+func TestApplyRecordTags_ReachesASecurityGroupRule(t *testing.T) {
+	t.Parallel()
+	svc := setupTestVPCService(t)
+	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
+	sgID := createTestSG(t, svc, vpcID, "rule-createtags-sg")
+
+	out, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{
+		GroupId: aws.String(sgID),
+		IpPermissions: []*ec2.IpPermission{{
+			IpProtocol: aws.String("tcp"), FromPort: aws.Int64(22), ToPort: aws.Int64(22),
+			IpRanges: []*ec2.IpRange{{CidrIp: aws.String("192.168.1.33/32")}},
+		}},
+	}, testAccountID)
+	require.NoError(t, err)
+	ruleID := aws.StringValue(out.SecurityGroupRules[0].SecurityGroupRuleId)
+
+	require.NoError(t, svc.ApplyRecordTags(&ec2.CreateTagsInput{
+		Resources: []*string{aws.String(ruleID)},
+		Tags:      []*ec2.Tag{{Key: aws.String("Name"), Value: aws.String("ssh")}},
+	}, testAccountID))
+
+	desc, err := svc.DescribeSecurityGroupRules(context.Background(), &ec2.DescribeSecurityGroupRulesInput{
+		SecurityGroupRuleIds: []*string{aws.String(ruleID)},
+	}, testAccountID)
+	require.NoError(t, err)
+	require.Len(t, desc.SecurityGroupRules, 1)
+	assert.Equal(t, map[string]string{"Name": "ssh"},
+		filterutil.EC2TagsToMap(desc.SecurityGroupRules[0].Tags))
+
+	require.NoError(t, svc.RemoveRecordTags(&ec2.DeleteTagsInput{
+		Resources: []*string{aws.String(ruleID)},
+		Tags:      []*ec2.Tag{{Key: aws.String("Name")}},
+	}, testAccountID))
+
+	desc, err = svc.DescribeSecurityGroupRules(context.Background(), &ec2.DescribeSecurityGroupRulesInput{
+		SecurityGroupRuleIds: []*string{aws.String(ruleID)},
+	}, testAccountID)
+	require.NoError(t, err)
+	assert.Empty(t, desc.SecurityGroupRules[0].Tags)
+}

spinifex/handlers/ec2/vpc/service_impl.go

@@ -1107,10 +1107,13 @@ func (s *VPCServiceImpl) clearRecordTags(ctx context.Context, accountID, resourc
 	}
 }
 
-// updateRecordTags applies mut to the tag map of the subnet-, vpc-, sg-, or
-// eni-scoped record identified by resourceID. Other resource ids are a no-op.
+// updateRecordTags applies mut to the tag map of the subnet-, vpc-, sg-, sgr-,
+// or eni-scoped record identified by resourceID. Other resource ids are a
+// no-op.
 func (s *VPCServiceImpl) updateRecordTags(ctx context.Context, accountID, resourceID string, mut func(map[string]string)) error {
 	switch {
+	case strings.HasPrefix(resourceID, "sgr-"):
+		return s.updateSGRuleTags(ctx, accountID, resourceID, mut)
 	case strings.HasPrefix(resourceID, "subnet-"):
 		return utils.UpdateKVRecordTags(ctx, s.subnetKV, accountID, resourceID, func(r *SubnetRecord) {
 			if r.Tags == nil {