From bd68fd95eebf084e953cd50dc13871b1175c950e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 19:41:42 +0200 Subject: [PATCH] feat(providers/aws): rich self-describing RI Name tags (closes #687) EC2 and Redshift reserved-instance purchases now stamp a Name tag with the rich {svc}-{region}-{sku}-{count}x-{term}-{paymt}-{ts}-{rand} format already used by the other AWS services for their customer- supplied reservation IDs (RDS/ElastiCache/MemoryDB/OpenSearch). EC2 PurchaseReservedInstancesOfferingInput has no customer-supplied ID field and Redshift PurchaseReservedNodeOfferingInput has none either, so the Name tag is the only way to identify these commitments in the AWS console without cross-referencing CUDly. Count/Term/PaymentOption descriptor tags are also added to EC2 to match the Redshift tag set. Also fixes three pre-existing test compile errors (findOfferingID called with 2 args instead of the required 3 in rds and redshift test files). --- providers/aws/services/ec2/client.go | 18 +++ providers/aws/services/ec2/client_test.go | 106 ++++++++++++++++++ providers/aws/services/rds/client_test.go | 6 +- providers/aws/services/redshift/client.go | 16 ++- .../aws/services/redshift/client_test.go | 69 +++++++++++- 5 files changed, 204 insertions(+), 11 deletions(-) diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 275946a81..fa1a0ebf5 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -230,10 +230,28 @@ func (c *Client) findRIByIdempotencyToken(ctx context.Context, token string) (st // since AWS sometimes needs a couple of seconds before the RI ID is visible // to CreateTags. Non-NotFound errors short-circuit immediately. func (c *Client) tagReservedInstance(ctx context.Context, riID string, rec common.Recommendation, source, idempotencyToken string) error { + // EC2 PurchaseReservedInstancesOfferingInput has no customer-supplied name or + // ID field. A Name tag (issue #687) is the only way to make the RI + // self-describing in the AWS console without cross-referencing CUDly's DB. + // BuildReservationName produces the same rich {svc}-{region}-{sku}-{count}x- + // {term}-{paymt}-{ts}-{rand} format used by the other AWS service clients. + displayName := common.BuildReservationName(common.ReservationNameFields{ + Service: "ec2", + Region: rec.Region, + ResourceType: rec.ResourceType, + Count: rec.Count, + Term: rec.Term, + Payment: rec.PaymentOption, + Now: time.Now(), + }, "ec2-reserved-") tags := []types.Tag{ + {Key: aws.String("Name"), Value: aws.String(displayName)}, {Key: aws.String("Purpose"), Value: aws.String("Reserved Instance Purchase")}, {Key: aws.String("ResourceType"), Value: aws.String(rec.ResourceType)}, {Key: aws.String("Region"), Value: aws.String(rec.Region)}, + {Key: aws.String("Count"), Value: aws.String(fmt.Sprintf("%d", rec.Count))}, + {Key: aws.String("Term"), Value: aws.String(rec.Term)}, + {Key: aws.String("PaymentOption"), Value: aws.String(rec.PaymentOption)}, {Key: aws.String("PurchaseDate"), Value: aws.String(time.Now().Format("2006-01-02"))}, {Key: aws.String("Tool"), Value: aws.String("CUDly")}, } diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index e5085a76a..7f1083423 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -669,3 +669,109 @@ func TestFindOfferingID_HappyPath(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "offering-ok", id) } + +// TestClient_tagReservedInstance_NameTagPresent asserts that a purchase on the +// no-token CLI path (issue #687) stamps a self-describing Name tag on the EC2 +// RI. EC2 PurchaseReservedInstancesOfferingInput has no customer-supplied name +// field, so the Name tag is the only way to identify the reservation in the AWS +// console without cross-referencing CUDly's purchase audit log. +func TestClient_tagReservedInstance_NameTagPresent(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + client := &Client{client: mockEC2, region: "us-west-2"} + + rec := common.Recommendation{ + Service: common.ServiceCompute, + ResourceType: "m5.xlarge", + Region: "us-west-2", + Count: 3, + PaymentOption: "all-upfront", + Term: "1yr", + Details: &common.ComputeDetails{Platform: "Linux/UNIX", Tenancy: "default", Scope: "Region"}, + } + + var capturedTags []types.Tag + mockEC2.On("CreateTags", mock.Anything, mock.MatchedBy(func(in *ec2.CreateTagsInput) bool { + capturedTags = in.Tags + return len(in.Resources) == 1 && in.Resources[0] == "ri-name-test" + })).Return(&ec2.CreateTagsOutput{}, nil) + + err := client.tagReservedInstance(context.Background(), "ri-name-test", rec, "", "") + assert.NoError(t, err) + + tagMap := make(map[string]string, len(capturedTags)) + for _, tag := range capturedTags { + tagMap[aws.ToString(tag.Key)] = aws.ToString(tag.Value) + } + + name, ok := tagMap["Name"] + assert.True(t, ok, "Name tag must be present in CreateTags call") + assert.True(t, len(name) > 0, "Name tag must be non-empty") + assert.LessOrEqual(t, len(name), 60, "Name must fit the 60-char AWS reservation-name cap") + // Key segments that make the RI self-describing without CUDly: + assert.Contains(t, name, "ec2", "Name must start with the service code") + assert.Contains(t, name, "us-west-2", "Name must embed the region") + assert.Contains(t, name, "m5-xlarge", "Name must embed the SKU (dots->hyphens)") + assert.Contains(t, name, "3x", "Name must embed the count") + assert.Contains(t, name, "1yr", "Name must embed the term") + + mockEC2.AssertExpectations(t) +} + +// TestClient_PurchaseCommitment_NameTagInCreateTagsRequest asserts that an +// end-to-end purchase on the no-token CLI path (issue #687) produces a +// CreateTags call that includes a self-describing Name tag. +func TestClient_PurchaseCommitment_NameTagInCreateTagsRequest(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + client := &Client{client: mockEC2, region: "ap-southeast-1"} + + rec := common.Recommendation{ + Service: common.ServiceCompute, + ResourceType: "r6g.large", + Region: "ap-southeast-1", + Count: 2, + PaymentOption: "no-upfront", + Term: "3yr", + Details: &common.ComputeDetails{Platform: "Linux/UNIX", Tenancy: "default", Scope: "Region"}, + } + + // No idempotency token -> skip DescribeReservedInstances guard + mockEC2.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). + Return(&ec2.DescribeReservedInstancesOfferingsOutput{ + ReservedInstancesOfferings: []types.ReservedInstancesOffering{{ + ReservedInstancesOfferingId: aws.String("off-name-e2e"), + InstanceType: types.InstanceTypeR6gLarge, + Duration: aws.Int64(94608000), + OfferingType: types.OfferingTypeValuesNoUpfront, + ProductDescription: types.RIProductDescriptionLinuxUnix, + InstanceTenancy: types.TenancyDefault, + }}, + }, nil) + + mockEC2.On("PurchaseReservedInstancesOffering", mock.Anything, mock.Anything). + Return(&ec2.PurchaseReservedInstancesOfferingOutput{ + ReservedInstancesId: aws.String("ri-name-e2e"), + }, nil) + + var capturedName string + mockEC2.On("CreateTags", mock.Anything, mock.MatchedBy(func(in *ec2.CreateTagsInput) bool { + for _, tag := range in.Tags { + if aws.ToString(tag.Key) == "Name" { + capturedName = aws.ToString(tag.Value) + return true + } + } + return false + })).Return(&ec2.CreateTagsOutput{}, nil) + + result, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + assert.NoError(t, err) + assert.True(t, result.Success) + + assert.True(t, len(capturedName) > 0, "Name tag must be set on the CreateTags call") + assert.Contains(t, capturedName, "ec2", "service code must appear in Name: %q", capturedName) + assert.Contains(t, capturedName, "ap-southeast-1", "region must appear in Name: %q", capturedName) + + mockEC2.AssertExpectations(t) +} diff --git a/providers/aws/services/rds/client_test.go b/providers/aws/services/rds/client_test.go index 7f5d884a3..7541ca874 100644 --- a/providers/aws/services/rds/client_test.go +++ b/providers/aws/services/rds/client_test.go @@ -733,7 +733,7 @@ func TestFindOfferingID_PaginationCapFires(t *testing.T) { }, nil).Once() } - _, err := client.findOfferingID(context.Background(), rec) + _, err := client.findOfferingID(context.Background(), rec, "") if assert.Error(t, err) { assert.Contains(t, err.Error(), "pagination cap reached") @@ -763,7 +763,7 @@ func TestFindOfferingID_WrongVariantRejected(t *testing.T) { }, }, nil).Once() - _, err := client.findOfferingID(context.Background(), rec) + _, err := client.findOfferingID(context.Background(), rec, "") if assert.Error(t, err) { assert.Contains(t, err.Error(), "payment option") @@ -792,7 +792,7 @@ func TestFindOfferingID_HappyPath(t *testing.T) { }, }, nil).Once() - id, err := client.findOfferingID(context.Background(), rec) + id, err := client.findOfferingID(context.Background(), rec, "") assert.NoError(t, err) assert.Equal(t, "offering-ok", id) diff --git a/providers/aws/services/redshift/client.go b/providers/aws/services/redshift/client.go index b65eab42e..9dc945439 100644 --- a/providers/aws/services/redshift/client.go +++ b/providers/aws/services/redshift/client.go @@ -347,10 +347,20 @@ func (c *Client) tagReservedNode(ctx context.Context, nodeID string, rec common. // does not accept a customer-supplied node ID, so the only way to make // the reserved node identifiable from the AWS console alone (without // cross-referencing CUDly) is to encode the same descriptors that the - // other AWS service clients embed in their reservation name. Term, - // PaymentOption, and Count join the pre-existing NodeType/Region/ - // PurchaseDate set. + // other AWS service clients embed in their reservation name. The Name tag + // uses BuildReservationName to produce the same rich format so operators + // can identify the node without cross-referencing CUDly's purchase audit log. + displayName := common.BuildReservationName(common.ReservationNameFields{ + Service: "redshift", + Region: rec.Region, + ResourceType: rec.ResourceType, + Count: rec.Count, + Term: rec.Term, + Payment: rec.PaymentOption, + Now: time.Now(), + }, "redshift-reserved-") tags := []redshifttypes.Tag{ + {Key: aws.String("Name"), Value: aws.String(displayName)}, {Key: aws.String("Purpose"), Value: aws.String("Reserved Node Purchase")}, {Key: aws.String("NodeType"), Value: aws.String(rec.ResourceType)}, {Key: aws.String("Region"), Value: aws.String(rec.Region)}, diff --git a/providers/aws/services/redshift/client_test.go b/providers/aws/services/redshift/client_test.go index 03b43dff9..09764e59c 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -430,10 +430,14 @@ func TestClient_PurchaseCommitment_TagsCarryRichDescriptors(t *testing.T) { } // Required new descriptors from #687 (the existing NodeType / Region / // PurchaseDate / Tool / Purpose tags are covered by the existing - // TagsWithResolvedARN test). + // TagsWithResolvedARN test). Name tag carries the rich self-describing + // identifier so the node is findable in the AWS console without CUDly. + name := got["Name"] return got["Count"] == "2" && got["Term"] == "3yr" && - got["PaymentOption"] == "partial-upfront" + got["PaymentOption"] == "partial-upfront" && + len(name) > 0 && + name[:8] == "redshift" })).Return(&redshift.CreateTagsOutput{}, nil) result, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) @@ -1141,7 +1145,7 @@ func TestFindOfferingID_PaginationCapFires(t *testing.T) { }, nil).Once() } - _, err := client.findOfferingID(context.Background(), rec) + _, err := client.findOfferingID(context.Background(), rec, "") if assert.Error(t, err) { assert.Contains(t, err.Error(), "pagination cap reached") @@ -1173,7 +1177,7 @@ func TestFindOfferingID_WrongVariantRejected(t *testing.T) { }, }, nil).Once() - id, err := client.findOfferingID(context.Background(), rec) + id, err := client.findOfferingID(context.Background(), rec, "") assert.Error(t, err, "unknown offering type must not return success") assert.Empty(t, id) @@ -1200,8 +1204,63 @@ func TestFindOfferingID_HappyPath(t *testing.T) { }, }, nil).Once() - id, err := client.findOfferingID(context.Background(), rec) + id, err := client.findOfferingID(context.Background(), rec, "") assert.NoError(t, err) assert.Equal(t, "offering-ok", id) } + +// TestClient_tagReservedNode_NameTagPresent asserts that tagReservedNode (issue +// #687) stamps a self-describing Name tag on the Redshift reserved node. The +// Redshift purchase API has no customer-supplied node ID, so the Name tag is +// the only way to identify the node in the AWS console without CUDly. +func TestClient_tagReservedNode_NameTagPresent(t *testing.T) { + t.Parallel() + mockRS := &MockRedshiftClient{} + mockSTS := &MockRedshiftSTSClient{} + client := &Client{ + client: mockRS, + stsClient: mockSTS, + region: "eu-central-1", + } + + rec := common.Recommendation{ + Service: common.ServiceDataWarehouse, + ResourceType: "ra3.4xlarge", + Count: 1, + PaymentOption: "no-upfront", + Term: "1yr", + Region: "eu-central-1", + } + + mockSTS.On("GetCallerIdentity", mock.Anything, mock.Anything). + Return(&sts.GetCallerIdentityOutput{Account: aws.String("111222333444")}, nil) + + var capturedTags []types.Tag + mockRS.On("CreateTags", mock.Anything, mock.MatchedBy(func(in *redshift.CreateTagsInput) bool { + capturedTags = in.Tags + return true + })).Return(&redshift.CreateTagsOutput{}, nil) + + err := client.tagReservedNode(context.Background(), "rn-tag-test", rec, "test-source", "") + assert.NoError(t, err) + + tagMap := make(map[string]string, len(capturedTags)) + for _, tag := range capturedTags { + tagMap[aws.ToString(tag.Key)] = aws.ToString(tag.Value) + } + + name, ok := tagMap["Name"] + assert.True(t, ok, "Name tag must be present in CreateTags call") + assert.True(t, len(name) > 0, "Name tag must be non-empty") + assert.LessOrEqual(t, len(name), 60, "Name must fit the 60-char cap") + // Key segments that make the node self-describing without CUDly: + assert.Contains(t, name, "redshift", "Name must carry the service code") + assert.Contains(t, name, "eu-central-1", "Name must embed the region") + assert.Contains(t, name, "ra3-4xlarge", "Name must embed the SKU (dots->hyphens)") + assert.Contains(t, name, "1x", "Name must embed the count") + assert.Contains(t, name, "1yr", "Name must embed the term") + + mockRS.AssertExpectations(t) + mockSTS.AssertExpectations(t) +}