From 1a071b3d971a4d6a36b9280d2245934546696cfd Mon Sep 17 00:00:00 2001 From: aryanmehrotra Date: Sat, 26 Sep 2026 18:33:57 +0530 Subject: [PATCH 1/2] fix(aws-ec2): tag Elastic IPs and VPC endpoints after creation CreateTags and DeleteTags on an eipalloc- or vpce- id fell through the EC2 tag router to the compute tagger, which only knows instance, volume, snapshot and image ids, so both calls answered InvalidID.NotFound. Tags set through TagSpecifications at AllocateAddress / CreateVpcEndpoint worked, but nothing could change them afterwards. Route eipalloc-, vpce- and vpce-svc- ids through the networking NetworkResourceTagger onto the same records the create-time tags live on, so DescribeAddresses and DescribeVpcEndpoints (including their tag: and tag-key filters) reflect tags added and removed later. DescribeTags now also reports elastic-ip, vpc-endpoint and vpc-endpoint-service tags. The tag write holds Mock.mu, and DescribeVPCEndpoints now takes the read lock, so a concurrent tag write does not race a describe of the same record. --- providers/aws/vpc/endpoint.go | 3 + providers/aws/vpc/tags.go | 29 ++- providers/aws/vpc/tags_test.go | 68 ++++++ server/aws/ec2/tags.go | 37 ++- server/aws/ec2/tags_eip_vpce_test.go | 210 ++++++++++++++++++ .../networking/driver/aws_capabilities.go | 4 +- 6 files changed, 348 insertions(+), 3 deletions(-) create mode 100644 server/aws/ec2/tags_eip_vpce_test.go diff --git a/providers/aws/vpc/endpoint.go b/providers/aws/vpc/endpoint.go index 8ee0c5bbc..aee4d327b 100644 --- a/providers/aws/vpc/endpoint.go +++ b/providers/aws/vpc/endpoint.go @@ -110,6 +110,9 @@ func (m *Mock) DeleteVPCEndpoint( func (m *Mock) DescribeVPCEndpoints( _ context.Context, ids []string, ) ([]driver.VPCEndpoint, error) { + m.mu.RLock() + defer m.mu.RUnlock() + for _, id := range ids { if !m.endpoints.Has(id) { return nil, errors.Newf( diff --git a/providers/aws/vpc/tags.go b/providers/aws/vpc/tags.go index be9d1d12b..7b31dbbcf 100644 --- a/providers/aws/vpc/tags.go +++ b/providers/aws/vpc/tags.go @@ -11,7 +11,8 @@ import ( // UpdateResourceTags merges tags onto a VPC-family resource that has no // dedicated Update*Tags method: route tables, internet gateways, NAT // gateways, network ACLs, DHCP option sets, peering connections, managed -// prefix lists, and egress-only internet gateways. An unknown or missing id +// prefix lists, egress-only internet gateways, security-group rules, Elastic IP +// allocations, VPC endpoints and VPC endpoint services. An unknown or missing id // is NotFound, so the wire layer can map it to the InvalidID.NotFound code // real EC2 returns for CreateTags on a non-existent resource. func (m *Mock) UpdateResourceTags(_ context.Context, id string, tags map[string]string) error { @@ -63,6 +64,32 @@ func (m *Mock) mutateResourceTags(id string, transform func(map[string]string) m }) case strings.HasPrefix(id, "sgr-"): return m.mutateRuleTags(id, transform) + default: + return m.mutateAddressingTags(id, transform) + } +} + +// mutateAddressingTags is the mutateResourceTags continuation for Elastic IP +// allocations (eipalloc-), VPC endpoint services (vpce-svc-) and VPC endpoints +// (vpce-). These are the same records AllocateAddress / CreateVpcEndpoint +// TagSpecifications write, so tags added after creation show up on the +// Describe calls. vpce-svc- is checked before vpce- because it shares the prefix. +// m.mu is held because the Describe readers for these records read their fields +// under m.mu.RLock (see the Mock.mu comment). +func (m *Mock) mutateAddressingTags(id string, transform func(map[string]string) map[string]string) bool { + m.mu.Lock() + defer m.mu.Unlock() + + switch { + case strings.HasPrefix(id, "eipalloc-"): + return m.eips.Update(id, func(v *eipData) *eipData { v.Tags = transform(v.Tags); return v }) + case strings.HasPrefix(id, "vpce-svc-"): + return m.endpointServices.Update(id, func(v *driver.EndpointService) *driver.EndpointService { + v.Tags = transform(v.Tags) + return v + }) + case strings.HasPrefix(id, "vpce-"): + return m.endpoints.Update(id, func(v *driver.VPCEndpoint) *driver.VPCEndpoint { v.Tags = transform(v.Tags); return v }) default: return false } diff --git a/providers/aws/vpc/tags_test.go b/providers/aws/vpc/tags_test.go index 45cd4985e..5f7205df3 100644 --- a/providers/aws/vpc/tags_test.go +++ b/providers/aws/vpc/tags_test.go @@ -100,3 +100,71 @@ func routeTableTag(t *testing.T, m *Mock, id, key string) string { func isNotFound(err error) bool { return err != nil && errors.IsNotFound(err) } + +// TestAddressingResourceTagger covers tagging Elastic IP allocations, VPC +// endpoints and VPC endpoint services after creation: the tag merges with the +// ones set at create time on the same record the Describe calls read, delete +// removes only the named key, and an unknown id of each prefix is NotFound. +func TestAddressingResourceTagger(t *testing.T) { + ctx := context.Background() + m := newTestMock() + v := createTestVPC(m) + + eip, err := m.AllocateAddress(ctx, driver.ElasticIPConfig{Tags: map[string]string{"Name": "eip"}}) + requireNoError(t, err) + + ep, err := m.CreateVPCEndpoint(ctx, driver.VPCEndpointConfig{ + VPCID: v.ID, ServiceName: "com.amazonaws.us-east-1.s3", EndpointType: "Gateway", + Tags: map[string]string{"Name": "ep"}, + }) + requireNoError(t, err) + + svc, err := m.CreateVPCEndpointServiceConfiguration(ctx, driver.EndpointServiceConfig{ + NetworkLoadBalancerARNs: []string{"arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/net/n/1"}, + }) + requireNoError(t, err) + + for _, id := range []string{eip.AllocationID, ep.ID, svc.ID} { + requireNoError(t, m.UpdateResourceTags(ctx, id, map[string]string{"env": "prod"})) + } + + eips, err := m.DescribeAddresses(ctx, []string{eip.AllocationID}) + requireNoError(t, err) + assertEqual(t, "prod", eips[0].Tags["env"]) + assertEqual(t, "eip", eips[0].Tags["Name"]) + + eps, err := m.DescribeVPCEndpoints(ctx, []string{ep.ID}) + requireNoError(t, err) + assertEqual(t, "prod", eps[0].Tags["env"]) + assertEqual(t, "ep", eps[0].Tags["Name"]) + + svcs, err := m.DescribeVPCEndpointServiceConfigurations(ctx, []string{svc.ID}) + requireNoError(t, err) + assertEqual(t, "prod", svcs[0].Tags["env"]) + + for _, id := range []string{eip.AllocationID, ep.ID, svc.ID} { + requireNoError(t, m.RemoveResourceTags(ctx, id, []string{"env"})) + } + + eips, err = m.DescribeAddresses(ctx, []string{eip.AllocationID}) + requireNoError(t, err) + + if _, ok := eips[0].Tags["env"]; ok { + t.Fatalf("eip still carries env after RemoveResourceTags: %v", eips[0].Tags) + } + + assertEqual(t, "eip", eips[0].Tags["Name"]) + + eps, err = m.DescribeVPCEndpoints(ctx, []string{ep.ID}) + requireNoError(t, err) + + if _, ok := eps[0].Tags["env"]; ok { + t.Fatalf("endpoint still carries env after RemoveResourceTags: %v", eps[0].Tags) + } + + for _, id := range []string{"eipalloc-missing", "vpce-missing", "vpce-svc-missing"} { + if err := m.UpdateResourceTags(ctx, id, map[string]string{"k": "v"}); !isNotFound(err) { + t.Fatalf("UpdateResourceTags(%s) = %v, want NotFound", id, err) + } + } +} diff --git a/server/aws/ec2/tags.go b/server/aws/ec2/tags.go index 84127147d..fa4aa03dc 100644 --- a/server/aws/ec2/tags.go +++ b/server/aws/ec2/tags.go @@ -95,6 +95,7 @@ func (h *Handler) describeTags(w http.ResponseWriter, r *http.Request) { var recs []tagRecord recs = h.collectComputeTags(r.Context(), recs) recs = h.collectNetworkTags(r.Context(), recs) + recs = h.collectAddressingTags(r.Context(), recs) items := make([]describeTagItemXML, 0, len(recs)) @@ -185,6 +186,37 @@ func (h *Handler) collectNetworkTags(ctx context.Context, recs []tagRecord) []ta return recs } +// collectAddressingTags appends tag records for Elastic IP allocations, VPC +// endpoints and VPC endpoint services, using the resource-type names real EC2 +// DescribeTags reports for them. +func (h *Handler) collectAddressingTags(ctx context.Context, recs []tagRecord) []tagRecord { + if h.vpc == nil { + return recs + } + + if eips, err := h.vpc.DescribeAddresses(ctx, nil); err == nil { + for i := range eips { + recs = appendTagRecords(recs, eips[i].AllocationID, "elastic-ip", eips[i].Tags) + } + } + + if eps, err := h.vpc.DescribeVPCEndpoints(ctx, nil); err == nil { + for i := range eps { + recs = appendTagRecords(recs, eps[i].ID, "vpc-endpoint", eps[i].Tags) + } + } + + if svcs, ok := h.vpc.(netdriver.VPCEndpointServices); ok { + if list, err := svcs.DescribeVPCEndpointServiceConfigurations(ctx, nil); err == nil { + for i := range list { + recs = appendTagRecords(recs, list[i].ID, "vpc-endpoint-service", list[i].Tags) + } + } + } + + return recs +} + // appendSGRuleTagRecords appends tag records for each security-group rule that // carries tags, keyed by the rule's sgr- id. func appendSGRuleTagRecords(recs []tagRecord, rules []netdriver.SecurityRule) []tagRecord { @@ -332,7 +364,10 @@ func tagNotFoundCode(id string) string { // their own methods and are handled separately. // //nolint:gochecknoglobals // static id-prefix routing table -var networkResourceTagPrefixes = []string{"rtb-", "igw-", "nat-", "acl-", "dopt-", "pcx-", "pl-", "eigw-", "sgr-"} +var networkResourceTagPrefixes = []string{ + "rtb-", "igw-", "nat-", "acl-", "dopt-", "pcx-", "pl-", "eigw-", "sgr-", + "eipalloc-", "vpce-", // vpce- also covers vpce-svc- endpoint services +} // networkTaggableID reports whether id belongs to a resource tagged via the // NetworkResourceTagger optional interface. diff --git a/server/aws/ec2/tags_eip_vpce_test.go b/server/aws/ec2/tags_eip_vpce_test.go new file mode 100644 index 000000000..c9c99f19e --- /dev/null +++ b/server/aws/ec2/tags_eip_vpce_test.go @@ -0,0 +1,210 @@ +package ec2_test + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/ec2" + ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" + "github.com/aws/smithy-go" +) + +// tagOnlyFilters are the two ways a caller finds its own resource by tag. +func tagOnlyFilters(key, value string) [][]ec2types.Filter { + return [][]ec2types.Filter{ + {{Name: aws.String("tag:" + key), Values: []string{value}}}, + {{Name: aws.String("tag-key"), Values: []string{key}}}, + } +} + +func addressTagged(ctx context.Context, t *testing.T, c *ec2.Client, id string, fs []ec2types.Filter) (map[string]string, bool) { + t.Helper() + + out, err := c.DescribeAddresses(ctx, &ec2.DescribeAddressesInput{Filters: fs}) + if err != nil { + t.Fatalf("DescribeAddresses: %v", err) + } + + for i := range out.Addresses { + if aws.ToString(out.Addresses[i].AllocationId) == id { + return sdkTagMap(out.Addresses[i].Tags), true + } + } + + return nil, false +} + +func endpointTagged(ctx context.Context, t *testing.T, c *ec2.Client, id string, fs []ec2types.Filter) (map[string]string, bool) { + t.Helper() + + out, err := c.DescribeVpcEndpoints(ctx, &ec2.DescribeVpcEndpointsInput{Filters: fs}) + if err != nil { + t.Fatalf("DescribeVpcEndpoints: %v", err) + } + + for i := range out.VpcEndpoints { + if aws.ToString(out.VpcEndpoints[i].VpcEndpointId) == id { + return sdkTagMap(out.VpcEndpoints[i].Tags), true + } + } + + return nil, false +} + +func sdkTagMap(tags []ec2types.Tag) map[string]string { + m := make(map[string]string, len(tags)) + for _, tg := range tags { + m[aws.ToString(tg.Key)] = aws.ToString(tg.Value) + } + + return m +} + +func describeTagsFor(ctx context.Context, t *testing.T, c *ec2.Client, id string) map[string]string { + t.Helper() + + out, err := c.DescribeTags(ctx, &ec2.DescribeTagsInput{ + Filters: []ec2types.Filter{{Name: aws.String("resource-id"), Values: []string{id}}}, + }) + if err != nil { + t.Fatalf("DescribeTags: %v", err) + } + + m := map[string]string{} + for _, td := range out.Tags { + m[aws.ToString(td.Key)] = aws.ToString(td.Value) + "|" + string(td.ResourceType) + } + + return m +} + +// assertTagRoundTrip drives CreateTags -> Describe (by tag filter and via +// DescribeTags) -> DeleteTags -> Describe for one resource id through the real +// SDK client. describe returns the resource's tags and whether the filtered +// Describe call returned it at all. +func assertTagRoundTrip( + ctx context.Context, t *testing.T, c *ec2.Client, id, resourceType string, + describe func(fs []ec2types.Filter) (map[string]string, bool), +) { + t.Helper() + + if _, err := c.CreateTags(ctx, &ec2.CreateTagsInput{ + Resources: []string{id}, + Tags: []ec2types.Tag{{Key: aws.String("owner"), Value: aws.String("team-a")}}, + }); err != nil { + t.Fatalf("CreateTags(%s): %v", id, err) + } + + for _, fs := range tagOnlyFilters("owner", "team-a") { + tags, found := describe(fs) + if !found { + t.Fatalf("%s not returned for filter %s after CreateTags", id, aws.ToString(fs[0].Name)) + } + + if tags["owner"] != "team-a" || tags["Name"] != "created" { + t.Fatalf("%s tags = %v, want owner=team-a merged with create-time Name=created", id, tags) + } + } + + if got := describeTagsFor(ctx, t, c, id); got["owner"] != "team-a|"+resourceType { + t.Fatalf("DescribeTags(%s) = %v, want owner=team-a|%s", id, got, resourceType) + } + + if _, err := c.DeleteTags(ctx, &ec2.DeleteTagsInput{ + Resources: []string{id}, + Tags: []ec2types.Tag{{Key: aws.String("owner")}}, + }); err != nil { + t.Fatalf("DeleteTags(%s): %v", id, err) + } + + if _, found := describe(tagOnlyFilters("owner", "team-a")[0]); found { + t.Fatalf("%s still matches tag:owner after DeleteTags", id) + } + + tags, found := describe(nil) + if !found || tags["Name"] != "created" { + t.Fatalf("%s after DeleteTags: found=%v tags=%v, want Name=created kept", id, found, tags) + } + + if _, ok := tags["owner"]; ok { + t.Fatalf("%s still carries owner after DeleteTags: %v", id, tags) + } +} + +// TestCreateTagsOnElasticIP pins that an Elastic IP allocation id can be +// tagged and untagged after creation, and that the tags land on the same record +// AllocateAddress TagSpecifications populate. Before the fix, CreateTags on an +// eipalloc- id fell through to the compute tagger and answered InvalidID.NotFound. +func TestCreateTagsOnElasticIP(t *testing.T) { + ctx := context.Background() + c, _ := newTagServer(t) + + alloc, err := c.AllocateAddress(ctx, &ec2.AllocateAddressInput{ + Domain: ec2types.DomainTypeVpc, + TagSpecifications: []ec2types.TagSpecification{{ + ResourceType: ec2types.ResourceTypeElasticIp, + Tags: []ec2types.Tag{{Key: aws.String("Name"), Value: aws.String("created")}}, + }}, + }) + if err != nil { + t.Fatalf("AllocateAddress: %v", err) + } + + id := aws.ToString(alloc.AllocationId) + + assertTagRoundTrip(ctx, t, c, id, "elastic-ip", func(fs []ec2types.Filter) (map[string]string, bool) { + return addressTagged(ctx, t, c, id, fs) + }) +} + +// TestCreateTagsOnVPCEndpoint is the vpce- counterpart of +// TestCreateTagsOnElasticIP. +func TestCreateTagsOnVPCEndpoint(t *testing.T) { + ctx := context.Background() + c, _ := newTagServer(t) + + vpc, err := c.CreateVpc(ctx, &ec2.CreateVpcInput{CidrBlock: aws.String("10.0.0.0/16")}) + if err != nil { + t.Fatalf("CreateVpc: %v", err) + } + + ep, err := c.CreateVpcEndpoint(ctx, &ec2.CreateVpcEndpointInput{ + VpcId: vpc.Vpc.VpcId, + ServiceName: aws.String("com.amazonaws.us-east-1.s3"), + VpcEndpointType: ec2types.VpcEndpointTypeGateway, + TagSpecifications: []ec2types.TagSpecification{{ + ResourceType: ec2types.ResourceTypeVpcEndpoint, + Tags: []ec2types.Tag{{Key: aws.String("Name"), Value: aws.String("created")}}, + }}, + }) + if err != nil { + t.Fatalf("CreateVpcEndpoint: %v", err) + } + + id := aws.ToString(ep.VpcEndpoint.VpcEndpointId) + + assertTagRoundTrip(ctx, t, c, id, "vpc-endpoint", func(fs []ec2types.Filter) (map[string]string, bool) { + return endpointTagged(ctx, t, c, id, fs) + }) +} + +// TestCreateTagsOnMissingAddressingIDs pins that tagging an Elastic IP or VPC +// endpoint id that does not exist is a NotFound, not a silent success. +func TestCreateTagsOnMissingAddressingIDs(t *testing.T) { + ctx := context.Background() + c, _ := newTagServer(t) + + for _, id := range []string{"eipalloc-0000000000000000", "vpce-0000000000000000"} { + _, err := c.CreateTags(ctx, &ec2.CreateTagsInput{ + Resources: []string{id}, + Tags: []ec2types.Tag{{Key: aws.String("k"), Value: aws.String("v")}}, + }) + + var apiErr smithy.APIError + if !errors.As(err, &apiErr) || apiErr.ErrorCode() != "InvalidID.NotFound" { + t.Fatalf("CreateTags(%s) err = %v, want InvalidID.NotFound", id, err) + } + } +} diff --git a/services/networking/driver/aws_capabilities.go b/services/networking/driver/aws_capabilities.go index 323c338d6..f9ce4a2e5 100644 --- a/services/networking/driver/aws_capabilities.go +++ b/services/networking/driver/aws_capabilities.go @@ -670,7 +670,9 @@ type VPCBlockPublicAccess interface { // It tags VPC-family resources that have no dedicated Update*Tags method on the // portable Networking interface: route tables, internet gateways, NAT gateways, // network ACLs, DHCP option sets, peering connections, managed prefix lists, and -// egress-only internet gateways. The EC2 tag handler routes those id prefixes here. +// egress-only internet gateways, security-group rules, Elastic IP allocations, +// VPC endpoints and VPC endpoint services. The EC2 tag handler routes those id +// prefixes here. type NetworkResourceTagger interface { UpdateResourceTags(ctx context.Context, id string, tags map[string]string) error RemoveResourceTags(ctx context.Context, id string, keys []string) error From c1e835133cef83e6e9f9780a8e3421d85765bf2f Mon Sep 17 00:00:00 2001 From: aryanmehrotra Date: Sun, 27 Sep 2026 16:22:38 +0530 Subject: [PATCH 2/2] fix(aws-ec2): match EC2 CreateTags/DeleteTags semantics on the tag path Review follow-up for the Elastic IP / VPC endpoint tagging change. - An unknown eipalloc-, vpce- or vpce-svc- id answered the generic InvalidID.NotFound. The EC2 error reference defines InvalidAllocationID.NotFound, InvalidVpcEndpointId.NotFound and InvalidVpcEndpointServiceId.NotFound, and this package already returns them from the resources' own actions; tagNotFoundCode now does too. - DeleteTags with no Tag parameter succeeded and left every tag in place on eipalloc-/vpce- ids. EC2 deletes all user-defined tags and keeps the aws: ones; RemoveResourceTags now does that for an empty key list, and the handler resolves the omitted-Tag case itself for every tagger. - DeleteTags Key=k,Value=v deleted k whatever its value. EC2 deletes a key sent without a value regardless of value, and one sent with a value (including "") only on an exact match. - A CreateTags/DeleteTags batch was not atomic: the ids before an unknown one were written. Both handlers now read every resource first (new ResourceTags on the AWS VPC and compute mocks) and write only when the whole batch checks out. - The 50-tag limit counted only the request, so two calls could leave 52 tags on a resource. It now counts existing user tags plus new keys (aws: tags do not count, an overwritten key counts once). Keys over 128 and values over 256 Unicode characters are rejected with InvalidParameterValue, per the EC2 tag restrictions. - ModifyVPCEndpoint wrote the endpoint's fields without Mock.mu while DescribeVPCEndpoints reads them under RLock; it now takes the lock. --- providers/aws/ec2/tags.go | 36 +++ providers/aws/ec2/tags_test.go | 51 ++++ providers/aws/vpc/endpoint.go | 5 + providers/aws/vpc/tags.go | 67 ++++- providers/aws/vpc/tags_test.go | 126 +++++++++ server/aws/ec2/endpoint.go | 4 +- server/aws/ec2/tags.go | 222 +++++++++++++-- server/aws/ec2/tags_eip_vpce_test.go | 385 ++++++++++++++++++++++++++- server/aws/ec2/tags_internal_test.go | 42 +++ 9 files changed, 906 insertions(+), 32 deletions(-) create mode 100644 providers/aws/ec2/tags_test.go create mode 100644 server/aws/ec2/tags_internal_test.go diff --git a/providers/aws/ec2/tags.go b/providers/aws/ec2/tags.go index 4365a6272..e6be7ab41 100644 --- a/providers/aws/ec2/tags.go +++ b/providers/aws/ec2/tags.go @@ -131,3 +131,39 @@ func (m *Mock) UntagResource(_ context.Context, id string, keys []string) error return nil } + +// ResourceTags returns a copy of the current tags on an EC2 instance, volume, +// snapshot, or image, or NotFound for an unknown ID. The EC2 CreateTags and +// DeleteTags handler reads it for every resource in a batch before writing, so +// an unknown id, a tag-limit breach, or a DeleteTags value mismatch is decided +// before any resource changes. It takes the same locks as TagResource. +func (m *Mock) ResourceTags(_ context.Context, id string) (map[string]string, error) { + var out map[string]string + + snapshot := func(src map[string]string) { + out = make(map[string]string, len(src)) + for k, v := range src { + out[k] = v + } + } + + if strings.HasPrefix(id, "i-") { + if !m.mutateInstanceTags(id, snapshot) { + return nil, cerrors.Newf(cerrors.NotFound, "resource %q not found", id) + } + + return out, nil + } + + m.mu.Lock() + defer m.mu.Unlock() + + src, ok := m.tagsOf(id) + if !ok { + return nil, cerrors.Newf(cerrors.NotFound, "resource %q not found", id) + } + + snapshot(src) + + return out, nil +} diff --git a/providers/aws/ec2/tags_test.go b/providers/aws/ec2/tags_test.go new file mode 100644 index 000000000..c4c83c450 --- /dev/null +++ b/providers/aws/ec2/tags_test.go @@ -0,0 +1,51 @@ +package ec2 + +import ( + "context" + "testing" + + cerrors "github.com/stackshy/cloudemu/v2/errors" + "github.com/stackshy/cloudemu/v2/services/compute/driver" +) + +// TestResourceTagsReturnsACopy covers ResourceTags on an instance and a volume +// (the two lock paths): it reports the current tags, the returned map is a copy, +// and an unknown id is NotFound. The EC2 CreateTags/DeleteTags handler reads it +// to check a batch before writing. +func TestResourceTagsReturnsACopy(t *testing.T) { + ctx := context.Background() + m := newTestMock() + + insts, err := m.RunInstances(ctx, driver.InstanceConfig{ImageID: "ami-123", InstanceType: "t2.micro"}, 1) + if err != nil { + t.Fatalf("RunInstances: %v", err) + } + + vol, err := m.CreateVolume(ctx, driver.VolumeConfig{Size: 8, AvailabilityZone: "us-east-1a"}) + if err != nil { + t.Fatalf("CreateVolume: %v", err) + } + + for _, id := range []string{insts[0].ID, vol.ID} { + if err := m.TagResource(ctx, id, map[string]string{"env": "prod"}); err != nil { + t.Fatalf("TagResource(%s): %v", id, err) + } + + got, err := m.ResourceTags(ctx, id) + if err != nil || got["env"] != "prod" { + t.Fatalf("ResourceTags(%s) = %v, %v; want env=prod", id, got, err) + } + + got["env"] = "mutated" + + if again, _ := m.ResourceTags(ctx, id); again["env"] != "prod" { + t.Fatalf("ResourceTags(%s) returned the live map: %v", id, again) + } + } + + for _, id := range []string{"i-missing", "vol-missing", "x-unknown"} { + if _, err := m.ResourceTags(ctx, id); !cerrors.IsNotFound(err) { + t.Fatalf("ResourceTags(%s) = %v, want NotFound", id, err) + } + } +} diff --git a/providers/aws/vpc/endpoint.go b/providers/aws/vpc/endpoint.go index aee4d327b..96cdb252d 100644 --- a/providers/aws/vpc/endpoint.go +++ b/providers/aws/vpc/endpoint.go @@ -132,6 +132,11 @@ func (m *Mock) DescribeVPCEndpoints( func (m *Mock) ModifyVPCEndpoint( _ context.Context, id string, cfg driver.VPCEndpointConfig, ) (*driver.VPCEndpoint, error) { + // The field writes below go through the stored pointer, so they need m.mu: + // DescribeVPCEndpoints and the EC2 tag writer touch the same record under it. + m.mu.Lock() + defer m.mu.Unlock() + ep, ok := m.endpoints.Get(id) if !ok { return nil, errors.Newf( diff --git a/providers/aws/vpc/tags.go b/providers/aws/vpc/tags.go index 7b31dbbcf..c0287532b 100644 --- a/providers/aws/vpc/tags.go +++ b/providers/aws/vpc/tags.go @@ -13,8 +13,11 @@ import ( // gateways, network ACLs, DHCP option sets, peering connections, managed // prefix lists, egress-only internet gateways, security-group rules, Elastic IP // allocations, VPC endpoints and VPC endpoint services. An unknown or missing id -// is NotFound, so the wire layer can map it to the InvalidID.NotFound code -// real EC2 returns for CreateTags on a non-existent resource. +// is NotFound; the wire layer maps it to the resource-specific code real EC2 +// defines for it (InvalidAllocationID.NotFound for eipalloc-, +// InvalidVpcEndpointId.NotFound for vpce-, InvalidVpcEndpointServiceId.NotFound +// for vpce-svc-, InvalidSecurityGroupRuleId.NotFound for sgr-), and to the +// generic InvalidID.NotFound for the rest. func (m *Mock) UpdateResourceTags(_ context.Context, id string, tags map[string]string) error { if !m.mutateResourceTags(id, func(existing map[string]string) map[string]string { return mergeTagMap(existing, tags) @@ -25,10 +28,20 @@ func (m *Mock) UpdateResourceTags(_ context.Context, id string, tags map[string] return nil } +// reservedTagPrefix is the namespace of AWS-generated tags, which EC2 DeleteTags +// never removes. +const reservedTagPrefix = "aws:" + // RemoveResourceTags drops the given tag keys from a VPC-family resource, the -// DeleteTags counterpart to UpdateResourceTags. +// DeleteTags counterpart to UpdateResourceTags. An empty key list deletes every +// user-defined tag and keeps the AWS-generated "aws:" ones, which is what EC2 +// DeleteTags does when the Tag parameter is omitted. func (m *Mock) RemoveResourceTags(_ context.Context, id string, keys []string) error { if !m.mutateResourceTags(id, func(existing map[string]string) map[string]string { + if len(keys) == 0 { + return keepReservedTags(existing) + } + return removeTagMapKeys(existing, keys) }) { return errors.Newf(errors.NotFound, "resource %q not found", id) @@ -37,6 +50,54 @@ func (m *Mock) RemoveResourceTags(_ context.Context, id string, keys []string) e return nil } +// ResourceTags returns a copy of the current tags on any resource the EC2 tag +// API addresses through this provider: VPCs, subnets and security groups plus +// every id UpdateResourceTags accepts. It is NotFound for an unknown id, so the +// EC2 CreateTags/DeleteTags handler can check every resource in a batch +// (existence, the per-resource tag limit, DeleteTags value matching) before it +// writes to any of them. The read goes through the same store/m.mu path as the +// writers, so it never races a concurrent tag write. +func (m *Mock) ResourceTags(_ context.Context, id string) (map[string]string, error) { + var out map[string]string + + snapshot := func(existing map[string]string) map[string]string { + out = copyTags(existing) + return existing + } + + var found bool + + switch { + case strings.HasPrefix(id, "vpc-"): + found = m.vpcs.Update(id, func(v *vpcData) *vpcData { v.Tags = snapshot(v.Tags); return v }) + case strings.HasPrefix(id, "subnet-"): + found = m.subnets.Update(id, func(s *subnetData) *subnetData { s.Tags = snapshot(s.Tags); return s }) + case strings.HasPrefix(id, "sg-"): + found = m.securityGroups.Update(id, func(sg *sgData) *sgData { sg.Tags = snapshot(sg.Tags); return sg }) + default: + found = m.mutateResourceTags(id, snapshot) + } + + if !found { + return nil, errors.Newf(errors.NotFound, "resource %q not found", id) + } + + return out, nil +} + +// keepReservedTags returns a fresh map holding only the "aws:" tags of existing. +func keepReservedTags(existing map[string]string) map[string]string { + out := make(map[string]string) + + for k, v := range existing { + if strings.HasPrefix(k, reservedTagPrefix) { + out[k] = v + } + } + + return out +} + // mutateResourceTags routes an id to the store that owns it and applies // transform to that record's tag map inside memstore.Update, so the store lock // covers the read-modify-write. It reports whether the id matched a known diff --git a/providers/aws/vpc/tags_test.go b/providers/aws/vpc/tags_test.go index 5f7205df3..b902c4143 100644 --- a/providers/aws/vpc/tags_test.go +++ b/providers/aws/vpc/tags_test.go @@ -2,6 +2,7 @@ package vpc import ( "context" + "sync" "testing" "github.com/stackshy/cloudemu/v2/errors" @@ -168,3 +169,128 @@ func TestAddressingResourceTagger(t *testing.T) { } } } + +// TestRemoveResourceTagsWithoutKeysKeepsReservedTags pins EC2 DeleteTags with no +// Tag parameter: every user tag goes, the AWS-generated "aws:" tags stay. Before +// the fix an empty key list removed nothing. +func TestRemoveResourceTagsWithoutKeysKeepsReservedTags(t *testing.T) { + ctx := context.Background() + m := newTestMock() + + eip, err := m.AllocateAddress(ctx, driver.ElasticIPConfig{Tags: map[string]string{ + "Name": "eip", "env": "prod", "aws:cloudformation:stack-name": "s", + }}) + requireNoError(t, err) + + requireNoError(t, m.RemoveResourceTags(ctx, eip.AllocationID, nil)) + + got, err := m.ResourceTags(ctx, eip.AllocationID) + requireNoError(t, err) + + if len(got) != 1 || got["aws:cloudformation:stack-name"] != "s" { + t.Fatalf("tags after RemoveResourceTags(nil) = %v, want only the aws: tag", got) + } +} + +// TestResourceTagsReadsEveryTaggableID covers ResourceTags on each id family the +// EC2 tag handler routes to this provider, that it returns a copy, and that an +// unknown id is NotFound. +func TestResourceTagsReadsEveryTaggableID(t *testing.T) { + ctx := context.Background() + m := newTestMock() + + v, err := m.CreateVPC(ctx, driver.VPCConfig{CIDRBlock: "10.0.0.0/16", Tags: map[string]string{"k": "vpc"}}) + requireNoError(t, err) + + s, err := m.CreateSubnet(ctx, driver.SubnetConfig{VPCID: v.ID, CIDRBlock: "10.0.1.0/24", Tags: map[string]string{"k": "subnet"}}) + requireNoError(t, err) + + sg, err := m.CreateSecurityGroup(ctx, driver.SecurityGroupConfig{VPCID: v.ID, Name: "sg", Description: "d", Tags: map[string]string{"k": "sg"}}) + requireNoError(t, err) + + eip, err := m.AllocateAddress(ctx, driver.ElasticIPConfig{Tags: map[string]string{"k": "eip"}}) + requireNoError(t, err) + + rt, err := m.CreateRouteTable(ctx, driver.RouteTableConfig{VPCID: v.ID}) + requireNoError(t, err) + requireNoError(t, m.UpdateResourceTags(ctx, rt.ID, map[string]string{"k": "rtb"})) + + for id, want := range map[string]string{v.ID: "vpc", s.ID: "subnet", sg.ID: "sg", eip.AllocationID: "eip", rt.ID: "rtb"} { + got, err := m.ResourceTags(ctx, id) + requireNoError(t, err) + assertEqual(t, want, got["k"]) + + got["k"] = "mutated" + + again, err := m.ResourceTags(ctx, id) + requireNoError(t, err) + assertEqual(t, want, again["k"]) + } + + for _, id := range []string{"vpc-missing", "subnet-missing", "sg-missing", "eipalloc-missing", "rtb-missing", "vol-other"} { + if _, err := m.ResourceTags(ctx, id); !isNotFound(err) { + t.Fatalf("ResourceTags(%s) = %v, want NotFound", id, err) + } + } +} + +// TestAddressingTagWriteDoesNotRaceDescribe pins the m.mu the tag writer holds +// for Elastic IPs and VPC endpoints: DescribeAddresses / DescribeVPCEndpoints +// read the record's Tags field under m.mu.RLock, so without the writer's lock +// this trips -race. ModifyVPCEndpoint writes the same record and is run too. +func TestAddressingTagWriteDoesNotRaceDescribe(t *testing.T) { + ctx := context.Background() + m := newTestMock() + v := createTestVPC(m) + + eip, err := m.AllocateAddress(ctx, driver.ElasticIPConfig{}) + requireNoError(t, err) + + ep, err := m.CreateVPCEndpoint(ctx, driver.VPCEndpointConfig{ + VPCID: v.ID, ServiceName: "com.amazonaws.us-east-1.s3", EndpointType: "Gateway", + }) + requireNoError(t, err) + + const iters = 200 + + var wg sync.WaitGroup + + wg.Add(4) + + go func() { + defer wg.Done() + + for i := 0; i < iters; i++ { + _ = m.UpdateResourceTags(ctx, eip.AllocationID, map[string]string{"k": "v"}) + _ = m.UpdateResourceTags(ctx, ep.ID, map[string]string{"k": "v"}) + } + }() + + go func() { + defer wg.Done() + + for i := 0; i < iters; i++ { + _, _ = m.ModifyVPCEndpoint(ctx, ep.ID, driver.VPCEndpointConfig{ + RouteTableIDs: []string{"rtb-1"}, Tags: map[string]string{"m": "v"}, + }) + } + }() + + go func() { + defer wg.Done() + + for i := 0; i < iters; i++ { + _, _ = m.DescribeAddresses(ctx, []string{eip.AllocationID}) + } + }() + + go func() { + defer wg.Done() + + for i := 0; i < iters; i++ { + _, _ = m.DescribeVPCEndpoints(ctx, []string{ep.ID}) + } + }() + + wg.Wait() +} diff --git a/server/aws/ec2/endpoint.go b/server/aws/ec2/endpoint.go index e9f9416ca..cf983b56f 100644 --- a/server/aws/ec2/endpoint.go +++ b/server/aws/ec2/endpoint.go @@ -81,7 +81,7 @@ func (h *Handler) deleteVPCEndpoints(w http.ResponseWriter, r *http.Request) { for _, id := range awsquery.ListStrings(r.Form, "VpcEndpointId") { if err := h.vpc.DeleteVPCEndpoint(r.Context(), id); err != nil { item := unsuccessfulItemXML{ResourceID: id} - item.Error.Code = "InvalidVpcEndpointId.NotFound" + item.Error.Code = codeInvalidVpcEndpointID item.Error.Message = cerrors.Message(err) unsuccessful = append(unsuccessful, item) } @@ -231,5 +231,5 @@ func toVPCEndpointXML(ep *netdriver.VPCEndpoint) vpcEndpointXML { } func writeVPCEndpointErr(w http.ResponseWriter, err error) { - writeErrWithNotFound(w, err, "InvalidVpcEndpointId.NotFound", "DependencyViolation") + writeErrWithNotFound(w, err, codeInvalidVpcEndpointID, "DependencyViolation") } diff --git a/server/aws/ec2/tags.go b/server/aws/ec2/tags.go index fa4aa03dc..b85ea5bfb 100644 --- a/server/aws/ec2/tags.go +++ b/server/aws/ec2/tags.go @@ -3,8 +3,12 @@ package ec2 import ( "context" "encoding/xml" + "fmt" "net/http" + "net/url" + "strconv" "strings" + "unicode/utf8" "github.com/stackshy/cloudemu/v2/server/wire/awsquery" netdriver "github.com/stackshy/cloudemu/v2/services/networking/driver" @@ -272,9 +276,41 @@ func tagMatchesFilter(rec tagRecord, f awsquery.Filter) bool { return false } +// tagReader is the optional read side of the EC2 taggers: the current tags of +// one resource, NotFound when it does not exist. The AWS compute and VPC +// providers implement it; the handler uses it to check a whole CreateTags / +// DeleteTags batch before writing any of it. +type tagReader interface { + ResourceTags(ctx context.Context, id string) (map[string]string, error) +} + +// currentTags returns the tags on id from the provider that owns it. A provider +// that cannot report tags yields a nil map after an existence probe (a no-op +// CreateTags), so the batch is still checked for unknown ids. +func (h *Handler) currentTags(ctx context.Context, id string) (map[string]string, error) { + var owner any = h.compute + if networkOwnedTagID(id) { + owner = h.vpc + } + + if rd, ok := owner.(tagReader); ok { + return rd.ResourceTags(ctx, id) + } + + return nil, h.tagResource(ctx, id, nil) +} + +// networkOwnedTagID reports whether the networking provider owns id's tags. +func networkOwnedTagID(id string) bool { + return strings.HasPrefix(id, "vpc-") || strings.HasPrefix(id, "subnet-") || + strings.HasPrefix(id, "sg-") || networkTaggableID(id) +} + // createTags applies tags to one or more resources, dispatching each resource // ID by prefix to the owning provider (VPC-family IDs to the networking -// provider, compute IDs to the compute tagger). +// provider, compute IDs to the compute tagger). Real EC2 checks the whole batch +// before it writes: an unknown id or a resource that would pass the tag limit +// fails the call with nothing tagged, so every id is checked first. func (h *Handler) createTags(w http.ResponseWriter, r *http.Request) { ids := awsquery.ListStrings(r.Form, "ResourceId") tags := awsquery.FlatTags(r.Form, "Tag") @@ -284,6 +320,19 @@ func (h *Handler) createTags(w http.ResponseWriter, r *http.Request) { return } + for _, id := range ids { + existing, err := h.currentTags(r.Context(), id) + if err != nil { + writeErrWithNotFound(w, err, tagNotFoundCode(id), "IncorrectState") + return + } + + if userTagCountAfter(existing, tags) > maxUserTagsPerResource { + awsquery.WriteXMLError(w, http.StatusBadRequest, codeTagLimitExceeded, msgTagLimitExceeded) + return + } + } + for _, id := range ids { if err := h.tagResource(r.Context(), id, tags); err != nil { writeErrWithNotFound(w, err, tagNotFoundCode(id), "IncorrectState") @@ -294,65 +343,196 @@ func (h *Handler) createTags(w http.ResponseWriter, r *http.Request) { awsquery.WriteXMLResponse(w, tagsResponseXML{Return: true, RequestID: "cloudemu"}) } -// maxUserTagsPerResource is the ceiling EC2 enforces on user tags per resource; -// reservedTagPrefix is the "aws:" namespace reserved for AWS-managed tags that a -// CreateTags call may not write. +// The CreateTags limits from the EC2 tag restrictions: at most 50 user tags per +// resource, keys up to 128 and values up to 256 Unicode characters, and the +// "aws:" key namespace reserved for AWS-generated tags (which do not count +// toward the 50). const ( maxUserTagsPerResource = 50 + maxTagKeyLen = 128 + maxTagValueLen = 256 reservedTagPrefix = "aws:" + + codeTagLimitExceeded = "TagLimitExceeded" + codeTagLengthExceeded = "InvalidParameterValue" + codeInvalidVpcEndpointID = "InvalidVpcEndpointId.NotFound" + msgTagLimitExceeded = "The maximum number of tags per resource is 50" ) -// validateUserTags enforces the CreateTags restrictions real EC2 applies before -// any tag is written: at most 50 user tags per resource (TagLimitExceeded), and -// no key in the reserved "aws:" namespace (InvalidTagKey.Malformed). Only the -// key is checked; real EC2 permits a value that starts with "aws:". It returns -// the wire error code and message plus ok=false when a rule is violated. +// userTagCountAfter is how many user (non-"aws:") tags a resource holds once +// tags are merged onto existing: a key already present is overwritten, not +// added. +func userTagCountAfter(existing, tags map[string]string) int { + n := 0 + + for k := range existing { + if _, overwritten := tags[k]; !overwritten && !strings.HasPrefix(k, reservedTagPrefix) { + n++ + } + } + + return n + len(tags) +} + +// validateUserTags enforces the per-request CreateTags restrictions real EC2 +// applies before any tag is written: no more than 50 tags in the request +// (TagLimitExceeded), no key in the reserved "aws:" namespace +// (InvalidTagKey.Malformed), keys of at most 128 and values of at most 256 +// characters (InvalidParameterValue). A value that starts with "aws:" is +// permitted. The per-resource limit, which also counts the tags a resource +// already has, is checked by createTags. It returns the wire error code and +// message plus ok=false when a rule is violated. func validateUserTags(tags map[string]string) (code, msg string, ok bool) { if len(tags) > maxUserTagsPerResource { - return "TagLimitExceeded", - "The maximum number of tags per resource is 50", false + return codeTagLimitExceeded, msgTagLimitExceeded, false } - for k := range tags { + for k, v := range tags { if strings.HasPrefix(k, reservedTagPrefix) { return "InvalidTagKey.Malformed", "The specified tag key is not valid. Tag keys cannot be empty or null, and cannot start with aws:", false } + + if utf8.RuneCountInString(k) > maxTagKeyLen { + return codeTagLengthExceeded, + fmt.Sprintf("Tag key exceeds the maximum length of %d characters", maxTagKeyLen), false + } + + if utf8.RuneCountInString(v) > maxTagValueLen { + return codeTagLengthExceeded, + fmt.Sprintf("Tag value exceeds the maximum length of %d characters", maxTagValueLen), false + } } return "", "", true } -// deleteTags removes tags (by key) from one or more resources. +// deleteTagSpec is one Tag.N entry of a DeleteTags request. hasValue is false +// when the request sent no Tag.N.Value at all, which deletes the key whatever +// its value; an explicit Value (even "") deletes it only on an exact match. +type deleteTagSpec struct { + key string + value string + hasValue bool +} + +// parseDeleteTags reads the Tag.N.Key / Tag.N.Value pairs of a DeleteTags +// request, keeping whether each Value was present. Entries without a key are +// skipped, as FlatTags does. +func parseDeleteTags(form url.Values) []deleteTagSpec { + idxs := awsquery.CollectIndices(form, "Tag") + specs := make([]deleteTagSpec, 0, len(idxs)) + + for _, idx := range idxs { + base := "Tag." + strconv.Itoa(idx) + + k := form.Get(base + ".Key") + if k == "" { + continue + } + + _, hasValue := form[base+".Value"] + specs = append(specs, deleteTagSpec{key: k, value: form.Get(base + ".Value"), hasValue: hasValue}) + } + + return specs +} + +// deleteTagKeys resolves a DeleteTags request against one resource's current +// tags into the keys to remove. With no Tag entries it is every user tag (EC2 +// never deletes "aws:" tags that way); otherwise it is each named key that is +// present and, when the entry carries a value, holds exactly that value. +// write is false when nothing on the resource matches, so the caller skips a +// removal whose empty key list the provider would read as "delete all". +// +// existing is nil when the provider cannot report tags; then the named keys are +// passed through unchecked (and an empty list is the provider's delete-all). +func deleteTagKeys(existing map[string]string, specs []deleteTagSpec) (keys []string, write bool) { + if existing == nil { + keys = make([]string, 0, len(specs)) + for _, s := range specs { + keys = append(keys, s.key) + } + + return keys, true + } + + if len(specs) == 0 { + for k := range existing { + if !strings.HasPrefix(k, reservedTagPrefix) { + keys = append(keys, k) + } + } + + return keys, len(keys) > 0 + } + + for _, s := range specs { + v, ok := existing[s.key] + if !ok || (s.hasValue && v != s.value) { + continue + } + + keys = append(keys, s.key) + } + + return keys, len(keys) > 0 +} + +// deleteTags removes tags from one or more resources. Like createTags it checks +// every id (and resolves which keys match) before it removes anything, so an +// unknown id fails the batch with nothing deleted. func (h *Handler) deleteTags(w http.ResponseWriter, r *http.Request) { ids := awsquery.ListStrings(r.Form, "ResourceId") - tags := awsquery.FlatTags(r.Form, "Tag") + specs := parseDeleteTags(r.Form) - keys := make([]string, 0, len(tags)) - for k := range tags { - keys = append(keys, k) + type removal struct { + id string + keys []string } + plan := make([]removal, 0, len(ids)) + for _, id := range ids { - if err := h.untagResource(r.Context(), id, keys); err != nil { + existing, err := h.currentTags(r.Context(), id) + if err != nil { writeErrWithNotFound(w, err, tagNotFoundCode(id), "IncorrectState") return } + + if keys, write := deleteTagKeys(existing, specs); write { + plan = append(plan, removal{id: id, keys: keys}) + } + } + + for _, p := range plan { + if err := h.untagResource(r.Context(), p.id, p.keys); err != nil { + writeErrWithNotFound(w, err, tagNotFoundCode(p.id), "IncorrectState") + return + } } awsquery.WriteXMLResponse(w, deleteTagsResponseXML{Return: true, RequestID: "cloudemu"}) } // tagNotFoundCode returns the "…NotFound" error code real EC2 emits when -// CreateTags/DeleteTags names a non-existent resource. An instance id yields the -// resource-specific InvalidInstanceID.NotFound; other resource types fall back -// to the generic InvalidID.NotFound. +// CreateTags/DeleteTags names a non-existent resource: the resource-specific +// code the EC2 error reference defines (and this package already returns from +// the resource's own actions) where there is one, and the generic +// InvalidID.NotFound for the rest. vpce-svc- is matched before vpce-, whose +// prefix it shares. func tagNotFoundCode(id string) string { switch { case strings.HasPrefix(id, "i-"): return codeInvalidInstanceID case strings.HasPrefix(id, "sgr-"): return "InvalidSecurityGroupRuleId.NotFound" + case strings.HasPrefix(id, "eipalloc-"): + return "InvalidAllocationID.NotFound" + case strings.HasPrefix(id, "vpce-svc-"): + return "InvalidVpcEndpointServiceId.NotFound" + case strings.HasPrefix(id, "vpce-"): + return codeInvalidVpcEndpointID default: return "InvalidID.NotFound" } diff --git a/server/aws/ec2/tags_eip_vpce_test.go b/server/aws/ec2/tags_eip_vpce_test.go index c9c99f19e..1121d5928 100644 --- a/server/aws/ec2/tags_eip_vpce_test.go +++ b/server/aws/ec2/tags_eip_vpce_test.go @@ -3,6 +3,8 @@ package ec2_test import ( "context" "errors" + "strconv" + "strings" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -190,21 +192,392 @@ func TestCreateTagsOnVPCEndpoint(t *testing.T) { }) } -// TestCreateTagsOnMissingAddressingIDs pins that tagging an Elastic IP or VPC -// endpoint id that does not exist is a NotFound, not a silent success. +func assertTagAPIErr(t *testing.T, err error, code, what string) { + t.Helper() + + var apiErr smithy.APIError + if !errors.As(err, &apiErr) || apiErr.ErrorCode() != code { + t.Fatalf("%s err = %v, want %s", what, err, code) + } +} + +// TestCreateTagsOnMissingAddressingIDs pins that tagging or untagging an Elastic +// IP, VPC endpoint or VPC endpoint service id that does not exist fails with the +// resource-specific NotFound code the EC2 error reference defines for it (and +// that this package already returns from the resource's own actions), not a +// silent success and not the generic InvalidID.NotFound. func TestCreateTagsOnMissingAddressingIDs(t *testing.T) { ctx := context.Background() c, _ := newTagServer(t) - for _, id := range []string{"eipalloc-0000000000000000", "vpce-0000000000000000"} { + for id, code := range map[string]string{ + "eipalloc-0000000000000000": "InvalidAllocationID.NotFound", + "vpce-0000000000000000": "InvalidVpcEndpointId.NotFound", + "vpce-svc-0000000000000000": "InvalidVpcEndpointServiceId.NotFound", + } { _, err := c.CreateTags(ctx, &ec2.CreateTagsInput{ Resources: []string{id}, Tags: []ec2types.Tag{{Key: aws.String("k"), Value: aws.String("v")}}, }) + assertTagAPIErr(t, err, code, "CreateTags("+id+")") + + _, err = c.DeleteTags(ctx, &ec2.DeleteTagsInput{Resources: []string{id}}) + assertTagAPIErr(t, err, code, "DeleteTags("+id+")") + } +} + +// newTaggedEIP allocates an Elastic IP carrying tags from create time. +func newTaggedEIP(ctx context.Context, t *testing.T, c *ec2.Client, tags map[string]string) string { + t.Helper() + + spec := make([]ec2types.Tag, 0, len(tags)) + for k, v := range tags { + spec = append(spec, ec2types.Tag{Key: aws.String(k), Value: aws.String(v)}) + } + + alloc, err := c.AllocateAddress(ctx, &ec2.AllocateAddressInput{ + Domain: ec2types.DomainTypeVpc, + TagSpecifications: []ec2types.TagSpecification{{ + ResourceType: ec2types.ResourceTypeElasticIp, Tags: spec, + }}, + }) + if err != nil { + t.Fatalf("AllocateAddress: %v", err) + } + + return aws.ToString(alloc.AllocationId) +} + +// newTaggedEndpoint creates a Gateway VPC endpoint carrying tags from create time. +func newTaggedEndpoint(ctx context.Context, t *testing.T, c *ec2.Client, tags map[string]string) string { + t.Helper() + + vpc, err := c.CreateVpc(ctx, &ec2.CreateVpcInput{CidrBlock: aws.String("10.0.0.0/16")}) + if err != nil { + t.Fatalf("CreateVpc: %v", err) + } + + spec := make([]ec2types.Tag, 0, len(tags)) + for k, v := range tags { + spec = append(spec, ec2types.Tag{Key: aws.String(k), Value: aws.String(v)}) + } + + ep, err := c.CreateVpcEndpoint(ctx, &ec2.CreateVpcEndpointInput{ + VpcId: vpc.Vpc.VpcId, + ServiceName: aws.String("com.amazonaws.us-east-1.s3"), + VpcEndpointType: ec2types.VpcEndpointTypeGateway, + TagSpecifications: []ec2types.TagSpecification{{ + ResourceType: ec2types.ResourceTypeVpcEndpoint, Tags: spec, + }}, + }) + if err != nil { + t.Fatalf("CreateVpcEndpoint: %v", err) + } + + return aws.ToString(ep.VpcEndpoint.VpcEndpointId) +} + +// TestDeleteTagsWithoutTagsClearsUserTags pins EC2 DeleteTags with the Tag +// parameter omitted: "we delete all user-defined tags for the specified +// resources". Before the fix it answered success and left every tag in place on +// eipalloc- and vpce- ids, while the same call on an i- id cleared them. +func TestDeleteTagsWithoutTagsClearsUserTags(t *testing.T) { + ctx := context.Background() + c, _ := newTagServer(t) + + eip := newTaggedEIP(ctx, t, c, map[string]string{"Name": "created", "env": "prod"}) + ep := newTaggedEndpoint(ctx, t, c, map[string]string{"Name": "created", "env": "prod"}) + + if _, err := c.DeleteTags(ctx, &ec2.DeleteTagsInput{Resources: []string{eip, ep}}); err != nil { + t.Fatalf("DeleteTags: %v", err) + } + + if tags, _ := addressTagged(ctx, t, c, eip, nil); len(tags) != 0 { + t.Fatalf("eip tags after DeleteTags with no Tag = %v, want none", tags) + } + + if tags, _ := endpointTagged(ctx, t, c, ep, nil); len(tags) != 0 { + t.Fatalf("endpoint tags after DeleteTags with no Tag = %v, want none", tags) + } +} + +// TestDeleteTagsMatchesValue pins the DeleteTags Tag.N.Value rule: a key sent +// without a value deletes the tag whatever its value, a key sent with a value +// deletes it only when the value matches, and an explicit empty value matches +// only an empty-valued tag. Before the fix Key=env,Value=wrong deleted env. +func TestDeleteTagsMatchesValue(t *testing.T) { + ctx := context.Background() + c, _ := newTagServer(t) + + eip := newTaggedEIP(ctx, t, c, map[string]string{"env": "prod", "blank": "", "owner": "a"}) + + del := func(tags ...ec2types.Tag) { + t.Helper() + + if _, err := c.DeleteTags(ctx, &ec2.DeleteTagsInput{Resources: []string{eip}, Tags: tags}); err != nil { + t.Fatalf("DeleteTags: %v", err) + } + } + + del(ec2types.Tag{Key: aws.String("env"), Value: aws.String("wrong")}, + ec2types.Tag{Key: aws.String("owner"), Value: aws.String("")}) + + if tags, _ := addressTagged(ctx, t, c, eip, nil); tags["env"] != "prod" || tags["owner"] != "a" { + t.Fatalf("tags after value-mismatched DeleteTags = %v, want env=prod and owner=a kept", tags) + } + + del(ec2types.Tag{Key: aws.String("env"), Value: aws.String("prod")}, + ec2types.Tag{Key: aws.String("blank"), Value: aws.String("")}) + + tags, _ := addressTagged(ctx, t, c, eip, nil) + if _, ok := tags["env"]; ok { + t.Fatalf("env kept after DeleteTags Key=env,Value=prod: %v", tags) + } + + if _, ok := tags["blank"]; ok { + t.Fatalf("blank kept after DeleteTags Key=blank,Value=\"\": %v", tags) + } + + del(ec2types.Tag{Key: aws.String("owner")}) + + if tags, _ := addressTagged(ctx, t, c, eip, nil); len(tags) != 0 { + t.Fatalf("tags after key-only DeleteTags = %v, want none", tags) + } +} + +// TestTagBatchIsAtomic pins that CreateTags and DeleteTags check every +// ResourceId before writing: a batch naming one unknown id fails with nothing +// changed on the ids that do exist. Before the fix the EIP listed ahead of the +// bogus id was tagged (and untagged) anyway. +func TestTagBatchIsAtomic(t *testing.T) { + ctx := context.Background() + c, _ := newTagServer(t) + + eip := newTaggedEIP(ctx, t, c, map[string]string{"Name": "created"}) + ep := newTaggedEndpoint(ctx, t, c, map[string]string{"Name": "created"}) + batch := []string{eip, "vpce-0000000000000bad", ep} + + _, err := c.CreateTags(ctx, &ec2.CreateTagsInput{ + Resources: batch, + Tags: []ec2types.Tag{{Key: aws.String("owner"), Value: aws.String("a")}}, + }) + assertTagAPIErr(t, err, "InvalidVpcEndpointId.NotFound", "CreateTags(mixed batch)") + + if tags, _ := addressTagged(ctx, t, c, eip, nil); tags["owner"] != "" { + t.Fatalf("eip tagged by a failed CreateTags batch: %v", tags) + } + + _, err = c.DeleteTags(ctx, &ec2.DeleteTagsInput{ + Resources: batch, + Tags: []ec2types.Tag{{Key: aws.String("Name")}}, + }) + assertTagAPIErr(t, err, "InvalidVpcEndpointId.NotFound", "DeleteTags(mixed batch)") + + if tags, _ := addressTagged(ctx, t, c, eip, nil); tags["Name"] != "created" { + t.Fatalf("eip untagged by a failed DeleteTags batch: %v", tags) + } +} + +// numberedTags returns n tags k..k. +func numberedTags(start, n int) []ec2types.Tag { + out := make([]ec2types.Tag, 0, n) + for i := start; i < start+n; i++ { + out = append(out, ec2types.Tag{Key: aws.String("k" + strconv.Itoa(i)), Value: aws.String("v")}) + } + + return out +} + +// TestCreateTagsCountsExistingTags pins the 50-tag limit per resource: the tags +// a resource already carries count toward it (an overwritten key counts once, +// and "aws:" tags do not count). Before the fix only the request was counted, so +// two CreateTags calls could leave a resource with more than 50 tags. +func TestCreateTagsCountsExistingTags(t *testing.T) { + ctx := context.Background() + c, _ := newTagServer(t) + + eip := newTaggedEIP(ctx, t, c, map[string]string{"Name": "created"}) + + create := func(tags []ec2types.Tag) error { + _, err := c.CreateTags(ctx, &ec2.CreateTagsInput{Resources: []string{eip}, Tags: tags}) + return err + } + + if err := create(numberedTags(0, 40)); err != nil { + t.Fatalf("CreateTags(40): %v", err) + } + + // 41 on the resource + 10 new = 51. + assertTagAPIErr(t, create(numberedTags(40, 10)), "TagLimitExceeded", "CreateTags(41+10)") + + // 41 + 9 new + overwrite of k0 = 50, the limit itself. + if err := create(append(numberedTags(40, 9), numberedTags(0, 1)...)); err != nil { + t.Fatalf("CreateTags to exactly 50: %v", err) + } + + if tags, _ := addressTagged(ctx, t, c, eip, nil); len(tags) != 50 { + t.Fatalf("eip has %d tags, want 50", len(tags)) + } + + assertTagAPIErr(t, create(numberedTags(100, 1)), "TagLimitExceeded", "CreateTags(51st)") +} + +// TestCreateTagsRejectsOversizedKeyAndValue pins the EC2 tag restrictions of +// 128 characters per key and 256 per value, counted in Unicode characters, not +// bytes. Before the fix any length was accepted. +func TestCreateTagsRejectsOversizedKeyAndValue(t *testing.T) { + ctx := context.Background() + c, _ := newTagServer(t) + + eip := newTaggedEIP(ctx, t, c, nil) + + create := func(k, v string) error { + _, err := c.CreateTags(ctx, &ec2.CreateTagsInput{ + Resources: []string{eip}, + Tags: []ec2types.Tag{{Key: aws.String(k), Value: aws.String(v)}}, + }) + + return err + } + + if err := create(strings.Repeat("é", 128), strings.Repeat("é", 256)); err != nil { + t.Fatalf("CreateTags at the 128/256-character limits: %v", err) + } + + assertTagAPIErr(t, create(strings.Repeat("k", 129), "v"), "InvalidParameterValue", "CreateTags(129-char key)") + assertTagAPIErr(t, create("k", strings.Repeat("v", 257)), "InvalidParameterValue", "CreateTags(257-char value)") +} + +// TestTagsOnVPCEndpointServiceOverTheWire covers the vpce-svc- id on the wire: +// CreateTags/DeleteTags reach the endpoint service record, and DescribeTags +// reports it as resource-type vpc-endpoint-service. It also pins the +// resource-type filter: each type returns only its own resources. +func TestTagsOnVPCEndpointServiceOverTheWire(t *testing.T) { + ctx := context.Background() + c, _ := newTagServer(t) + + svcOut, err := c.CreateVpcEndpointServiceConfiguration(ctx, &ec2.CreateVpcEndpointServiceConfigurationInput{ + NetworkLoadBalancerArns: []string{"arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/net/n/1"}, + }) + if err != nil { + t.Fatalf("CreateVpcEndpointServiceConfiguration: %v", err) + } + + svc := aws.ToString(svcOut.ServiceConfiguration.ServiceId) + eip := newTaggedEIP(ctx, t, c, nil) + ep := newTaggedEndpoint(ctx, t, c, nil) + + if _, err := c.CreateTags(ctx, &ec2.CreateTagsInput{ + Resources: []string{svc, eip, ep}, + Tags: []ec2types.Tag{{Key: aws.String("owner"), Value: aws.String("a")}}, + }); err != nil { + t.Fatalf("CreateTags: %v", err) + } + + byType := func(types ...string) map[string]string { + t.Helper() + + out, err := c.DescribeTags(ctx, &ec2.DescribeTagsInput{Filters: []ec2types.Filter{ + {Name: aws.String("resource-type"), Values: types}, + {Name: aws.String("key"), Values: []string{"owner"}}, + }}) + if err != nil { + t.Fatalf("DescribeTags(%v): %v", types, err) + } + + got := map[string]string{} + for _, td := range out.Tags { + got[aws.ToString(td.ResourceId)] = string(td.ResourceType) + } + + return got + } + + for typ, id := range map[string]string{"vpc-endpoint-service": svc, "elastic-ip": eip, "vpc-endpoint": ep} { + if got := byType(typ); len(got) != 1 || got[id] != typ { + t.Fatalf("DescribeTags(resource-type=%s) = %v, want only %s", typ, got, id) + } + } + + if got := byType("vpc-endpoint-service", "elastic-ip"); len(got) != 2 || got[svc] == "" || got[eip] == "" { + t.Fatalf("DescribeTags(resource-type=vpc-endpoint-service,elastic-ip) = %v, want %s and %s", got, svc, eip) + } + + if _, err := c.DeleteTags(ctx, &ec2.DeleteTagsInput{ + Resources: []string{svc}, Tags: []ec2types.Tag{{Key: aws.String("owner")}}, + }); err != nil { + t.Fatalf("DeleteTags(%s): %v", svc, err) + } + + cfgs, err := c.DescribeVpcEndpointServiceConfigurations(ctx, &ec2.DescribeVpcEndpointServiceConfigurationsInput{ + ServiceIds: []string{svc}, + }) + if err != nil || len(cfgs.ServiceConfigurations) != 1 { + t.Fatalf("DescribeVpcEndpointServiceConfigurations: %v %v", cfgs, err) + } + + if tags := sdkTagMap(cfgs.ServiceConfigurations[0].Tags); len(tags) != 0 { + t.Fatalf("endpoint service tags after DeleteTags = %v, want none", tags) + } +} + +// TestDeleteTagsSemanticsOnEveryTagger drives the same DeleteTags rules through +// the other taggers the handler dispatches to (compute for i-, the networking +// provider's own methods for vpc- and subnet-): a value mismatch keeps the tag, +// and omitting Tag clears every user tag. +func TestDeleteTagsSemanticsOnEveryTagger(t *testing.T) { + ctx := context.Background() + c, _ := newTagServer(t) + + run, err := c.RunInstances(ctx, &ec2.RunInstancesInput{ + ImageId: aws.String("ami-123"), InstanceType: ec2types.InstanceTypeT2Micro, + MinCount: aws.Int32(1), MaxCount: aws.Int32(1), + }) + if err != nil { + t.Fatalf("RunInstances: %v", err) + } + + vpc, err := c.CreateVpc(ctx, &ec2.CreateVpcInput{CidrBlock: aws.String("10.1.0.0/16")}) + if err != nil { + t.Fatalf("CreateVpc: %v", err) + } + + subnet, err := c.CreateSubnet(ctx, &ec2.CreateSubnetInput{VpcId: vpc.Vpc.VpcId, CidrBlock: aws.String("10.1.1.0/24")}) + if err != nil { + t.Fatalf("CreateSubnet: %v", err) + } + + ids := []string{aws.ToString(run.Instances[0].InstanceId), aws.ToString(vpc.Vpc.VpcId), aws.ToString(subnet.Subnet.SubnetId)} + + if _, err := c.CreateTags(ctx, &ec2.CreateTagsInput{ + Resources: ids, + Tags: []ec2types.Tag{ + {Key: aws.String("env"), Value: aws.String("prod")}, + {Key: aws.String("owner"), Value: aws.String("a")}, + }, + }); err != nil { + t.Fatalf("CreateTags: %v", err) + } + + if _, err := c.DeleteTags(ctx, &ec2.DeleteTagsInput{ + Resources: ids, Tags: []ec2types.Tag{{Key: aws.String("env"), Value: aws.String("wrong")}}, + }); err != nil { + t.Fatalf("DeleteTags(value mismatch): %v", err) + } + + for _, id := range ids { + if got := describeTagsFor(ctx, t, c, id); len(got) != 2 { + t.Fatalf("%s tags after value-mismatched DeleteTags = %v, want env and owner kept", id, got) + } + } + + if _, err := c.DeleteTags(ctx, &ec2.DeleteTagsInput{Resources: ids}); err != nil { + t.Fatalf("DeleteTags(no Tag): %v", err) + } - var apiErr smithy.APIError - if !errors.As(err, &apiErr) || apiErr.ErrorCode() != "InvalidID.NotFound" { - t.Fatalf("CreateTags(%s) err = %v, want InvalidID.NotFound", id, err) + for _, id := range ids { + if got := describeTagsFor(ctx, t, c, id); len(got) != 0 { + t.Fatalf("%s tags after DeleteTags with no Tag = %v, want none", id, got) } } } diff --git a/server/aws/ec2/tags_internal_test.go b/server/aws/ec2/tags_internal_test.go new file mode 100644 index 000000000..8e6a7b617 --- /dev/null +++ b/server/aws/ec2/tags_internal_test.go @@ -0,0 +1,42 @@ +package ec2 + +import ( + "slices" + "testing" +) + +// TestDeleteTagKeys covers how a DeleteTags request resolves against one +// resource's current tags, including the pass-through used when the owning +// provider cannot report tags (existing == nil). +func TestDeleteTagKeys(t *testing.T) { + existing := map[string]string{"env": "prod", "blank": "", "aws:managed": "x"} + + cases := []struct { + name string + existing map[string]string + specs []deleteTagSpec + want []string + write bool + }{ + {"no Tag keeps aws: tags", existing, nil, []string{"blank", "env"}, true}, + {"key only", existing, []deleteTagSpec{{key: "env"}}, []string{"env"}, true}, + {"value match", existing, []deleteTagSpec{{key: "env", value: "prod", hasValue: true}}, []string{"env"}, true}, + {"value mismatch", existing, []deleteTagSpec{{key: "env", value: "dev", hasValue: true}}, nil, false}, + {"empty value matches only empty", existing, []deleteTagSpec{ + {key: "blank", hasValue: true}, {key: "env", hasValue: true}, + }, []string{"blank"}, true}, + {"absent key", existing, []deleteTagSpec{{key: "missing"}}, nil, false}, + {"no user tags left", map[string]string{"aws:managed": "x"}, nil, nil, false}, + {"unknown tags pass keys through", nil, []deleteTagSpec{{key: "env", value: "v", hasValue: true}}, []string{"env"}, true}, + {"unknown tags, no Tag", nil, nil, []string{}, true}, + } + + for _, tc := range cases { + keys, write := deleteTagKeys(tc.existing, tc.specs) + slices.Sort(keys) + + if write != tc.write || !slices.Equal(keys, tc.want) { + t.Errorf("%s: deleteTagKeys = %v, %v; want %v, %v", tc.name, keys, write, tc.want, tc.write) + } + } +}