From 56f960ebdca66e3dd49f89c158897780f76281ca Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Thu, 18 Jun 2026 18:23:57 +1000 Subject: [PATCH] fix(ec2): mirror CreateTags/DeleteTags into AMI config (#386) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CreateTags/DeleteTags wrote only the standalone tag store, which DescribeImages does not read (it reads AMI metadata), so tag mutations on an AMI were invisible — DeleteTags appeared to be a silent no-op. Mirror tag add/remove into the owning AMI config (ownership-checked; absent or foreign AMIs skipped) so DescribeImages reflects them, matching the existing subnet/vpc mirror. --- spinifex/daemon/daemon_handlers_tag.go | 18 ++- spinifex/handlers/ec2/image/service_impl.go | 87 ++++++++++++ .../ec2/image/service_impl_recordtags_test.go | 124 ++++++++++++++++++ 3 files changed, 225 insertions(+), 4 deletions(-) create mode 100644 spinifex/handlers/ec2/image/service_impl_recordtags_test.go diff --git a/spinifex/daemon/daemon_handlers_tag.go b/spinifex/daemon/daemon_handlers_tag.go index 8f468e9b3..372c5e64b 100644 --- a/spinifex/daemon/daemon_handlers_tag.go +++ b/spinifex/daemon/daemon_handlers_tag.go @@ -17,9 +17,9 @@ func (d *Daemon) handleEC2DescribeTags(msg *nats.Msg) { handleNATSRequest(msg, d.tagsService.DescribeTags) } -// createTags writes the generic tag store, then mirrors subnet/vpc tags into -// their owning records so tag-filtered describes (e.g. LBC subnet -// auto-discovery via DescribeSubnets) observe them. +// createTags writes the generic tag store, then mirrors tags into owning records +// so tag-filtered describes observe them: subnet/vpc (e.g. LBC subnet +// auto-discovery via DescribeSubnets) and AMIs (DescribeImages). func (d *Daemon) createTags(input *ec2.CreateTagsInput, accountID string) (*ec2.CreateTagsOutput, error) { out, err := d.tagsService.CreateTags(input, accountID) if err != nil { @@ -30,11 +30,16 @@ func (d *Daemon) createTags(input *ec2.CreateTagsInput, accountID string) (*ec2. return nil, err } } + if d.imageService != nil { + if err := d.imageService.ApplyRecordTags(input, accountID); err != nil { + return nil, err + } + } return out, nil } // deleteTags removes from the generic tag store, then mirrors the removal into -// the owning subnet/vpc records. +// the owning subnet/vpc records and AMI configs. func (d *Daemon) deleteTags(input *ec2.DeleteTagsInput, accountID string) (*ec2.DeleteTagsOutput, error) { out, err := d.tagsService.DeleteTags(input, accountID) if err != nil { @@ -45,5 +50,10 @@ func (d *Daemon) deleteTags(input *ec2.DeleteTagsInput, accountID string) (*ec2. return nil, err } } + if d.imageService != nil { + if err := d.imageService.RemoveRecordTags(input, accountID); err != nil { + return nil, err + } + } return out, nil } diff --git a/spinifex/handlers/ec2/image/service_impl.go b/spinifex/handlers/ec2/image/service_impl.go index 41b70cd2a..c8c1847f7 100644 --- a/spinifex/handlers/ec2/image/service_impl.go +++ b/spinifex/handlers/ec2/image/service_impl.go @@ -693,6 +693,93 @@ func (s *ImageServiceImpl) putAMIConfig(imageID string, meta viperblock.AMIMetad return err } +// ApplyRecordTags mirrors CreateTags into the owning AMI config so +// DescribeImages observes tags added after registration. Non-ami ids, AMIs +// absent from this store, and AMIs the caller does not own are skipped; the +// generic tag store stays their record of truth. +func (s *ImageServiceImpl) ApplyRecordTags(input *ec2.CreateTagsInput, accountID string) error { + if input == nil { + return nil + } + merge := func(tags map[string]string) { + for _, t := range input.Tags { + if t.Key != nil && t.Value != nil { + tags[*t.Key] = *t.Value + } + } + } + for _, res := range input.Resources { + if res == nil || !strings.HasPrefix(*res, "ami-") { + continue + } + if err := s.updateAMITags(*res, accountID, merge); err != nil { + return err + } + } + return nil +} + +// RemoveRecordTags mirrors DeleteTags into the owning AMI config. Empty +// input.Tags clears all tags; a tag with a value deletes only on a value match +// (AWS-faithful), a nil value deletes unconditionally. +func (s *ImageServiceImpl) RemoveRecordTags(input *ec2.DeleteTagsInput, accountID string) error { + if input == nil { + return nil + } + remove := func(tags map[string]string) { + if len(input.Tags) == 0 { + for k := range tags { + delete(tags, k) + } + return + } + for _, t := range input.Tags { + if t.Key == nil { + continue + } + if t.Value == nil { + delete(tags, *t.Key) + } else if cur, ok := tags[*t.Key]; ok && cur == *t.Value { + delete(tags, *t.Key) + } + } + } + for _, res := range input.Resources { + if res == nil || !strings.HasPrefix(*res, "ami-") { + continue + } + if err := s.updateAMITags(*res, accountID, remove); err != nil { + return err + } + } + return nil +} + +// updateAMITags read-modify-writes the tag map of the AMI config identified by +// imageID. An AMI absent from this store or owned by another account is skipped +// (its tags are not this service's to mutate); a corrupt config propagates. +func (s *ImageServiceImpl) updateAMITags(imageID, accountID string, mut func(map[string]string)) error { + meta, err := s.GetAMIConfig(imageID) + if err != nil { + if objectstore.IsNoSuchKeyError(err) { + return nil + } + return err + } + if err := s.checkAMIOwnership(meta, accountID); err != nil { + if err.Error() == awserrors.ErrorUnauthorizedOperation { + slog.Debug("updateAMITags: skipping AMI not owned by caller", "imageId", imageID) + return nil + } + return err + } + if meta.Tags == nil { + meta.Tags = map[string]string{} + } + mut(meta.Tags) + return s.putAMIConfig(imageID, meta) +} + // checkAMIOwnership rejects cross-account and system-AMI mutations. Empty // owner is ServerInternal (corrupt config) rather than a misleading 403. func (s *ImageServiceImpl) checkAMIOwnership(meta viperblock.AMIMetadata, accountID string) error { diff --git a/spinifex/handlers/ec2/image/service_impl_recordtags_test.go b/spinifex/handlers/ec2/image/service_impl_recordtags_test.go new file mode 100644 index 000000000..f954c5c51 --- /dev/null +++ b/spinifex/handlers/ec2/image/service_impl_recordtags_test.go @@ -0,0 +1,124 @@ +package handlers_ec2_image + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func createTagsInput(imageID string, tags map[string]string) *ec2.CreateTagsInput { + in := &ec2.CreateTagsInput{Resources: []*string{aws.String(imageID)}} + for k, v := range tags { + in.Tags = append(in.Tags, &ec2.Tag{Key: aws.String(k), Value: aws.String(v)}) + } + return in +} + +func TestApplyRecordTags_OwnedAMI(t *testing.T) { + svc, store := setupTestImageService(t) + createTestAMIConfigWithName(t, store, "ami-owned", "img") + + err := svc.ApplyRecordTags(createTagsInput("ami-owned", map[string]string{"env": "prod"}), testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig("ami-owned") + require.NoError(t, err) + assert.Equal(t, "prod", meta.Tags["env"]) +} + +func TestApplyRecordTags_NotOwnedSkipped(t *testing.T) { + svc, store := setupTestImageService(t) + createTestAMIConfigWithOwner(t, store, "ami-foreign", "img", "999999999999") + + err := svc.ApplyRecordTags(createTagsInput("ami-foreign", map[string]string{"env": "prod"}), testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig("ami-foreign") + require.NoError(t, err) + assert.NotContains(t, meta.Tags, "env") +} + +func TestApplyRecordTags_AbsentAMINoop(t *testing.T) { + svc, _ := setupTestImageService(t) + err := svc.ApplyRecordTags(createTagsInput("ami-missing", map[string]string{"env": "prod"}), testAccountID) + require.NoError(t, err) +} + +func TestApplyRecordTags_NonAMIResourceSkipped(t *testing.T) { + svc, store := setupTestImageService(t) + createTestAMIConfigWithName(t, store, "ami-owned", "img") + + in := &ec2.CreateTagsInput{ + Resources: []*string{aws.String("i-123"), aws.String("ami-owned")}, + Tags: []*ec2.Tag{{Key: aws.String("k"), Value: aws.String("v")}}, + } + require.NoError(t, svc.ApplyRecordTags(in, testAccountID)) + + meta, err := svc.GetAMIConfig("ami-owned") + require.NoError(t, err) + assert.Equal(t, "v", meta.Tags["k"]) +} + +func TestRemoveRecordTags_ValueMatchAndMismatch(t *testing.T) { + svc, store := setupTestImageService(t) + createTestAMIConfigWithName(t, store, "ami-owned", "img") + require.NoError(t, svc.ApplyRecordTags(createTagsInput("ami-owned", map[string]string{"a": "1", "b": "2"}), testAccountID)) + + // value mismatch keeps; value match deletes + in := &ec2.DeleteTagsInput{ + Resources: []*string{aws.String("ami-owned")}, + Tags: []*ec2.Tag{ + {Key: aws.String("a"), Value: aws.String("wrong")}, + {Key: aws.String("b"), Value: aws.String("2")}, + }, + } + require.NoError(t, svc.RemoveRecordTags(in, testAccountID)) + + meta, err := svc.GetAMIConfig("ami-owned") + require.NoError(t, err) + assert.Equal(t, "1", meta.Tags["a"]) + assert.NotContains(t, meta.Tags, "b") +} + +func TestRemoveRecordTags_NilValueUnconditional(t *testing.T) { + svc, store := setupTestImageService(t) + createTestAMIConfigWithName(t, store, "ami-owned", "img") + require.NoError(t, svc.ApplyRecordTags(createTagsInput("ami-owned", map[string]string{"a": "1"}), testAccountID)) + + in := &ec2.DeleteTagsInput{ + Resources: []*string{aws.String("ami-owned")}, + Tags: []*ec2.Tag{{Key: aws.String("a")}}, + } + require.NoError(t, svc.RemoveRecordTags(in, testAccountID)) + + meta, err := svc.GetAMIConfig("ami-owned") + require.NoError(t, err) + assert.NotContains(t, meta.Tags, "a") +} + +func TestRemoveRecordTags_EmptyClearsAll(t *testing.T) { + svc, store := setupTestImageService(t) + createTestAMIConfigWithName(t, store, "ami-owned", "img") + require.NoError(t, svc.ApplyRecordTags(createTagsInput("ami-owned", map[string]string{"a": "1", "b": "2"}), testAccountID)) + + in := &ec2.DeleteTagsInput{Resources: []*string{aws.String("ami-owned")}} + require.NoError(t, svc.RemoveRecordTags(in, testAccountID)) + + meta, err := svc.GetAMIConfig("ami-owned") + require.NoError(t, err) + assert.Empty(t, meta.Tags) +} + +func TestRemoveRecordTags_NotOwnedSkipped(t *testing.T) { + svc, store := setupTestImageService(t) + createTestAMIConfigWithOwner(t, store, "ami-foreign", "img", "999999999999") + + in := &ec2.DeleteTagsInput{Resources: []*string{aws.String("ami-foreign")}} + require.NoError(t, svc.RemoveRecordTags(in, testAccountID)) + // foreign AMI untouched (still readable, no panic / error) + _, err := svc.GetAMIConfig("ami-foreign") + require.NoError(t, err) +}