gsadmin/spinifex
1
0
Fork 0
mirror of https://github.com/mulgadc/spinifex.git synced 2026-10-19 10:52:04 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(ec2): mirror CreateTags/DeleteTags into AMI config (#386)

Browse Source 
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.
This commit is contained in:
Josh Siviour
2026-06-18 18:23:57 +10:00
committed by GitHub
parent 2faad4b9e7
commit 56f960ebdc
3 changed files with 225 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
spinifex/daemon/daemon_handlers_tag.go
+14 -4
View File
@@ -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
}
spinifex/handlers/ec2/image/service_impl.go
+87
View File
@@ -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 {
spinifex/handlers/ec2/image/service_impl_recordtags_test.go
+124
View File
@@ -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)
}