diff --git a/providers/aws/services/opensearch/client.go b/providers/aws/services/opensearch/client.go index f72cb948f..f4c9e8ff0 100644 --- a/providers/aws/services/opensearch/client.go +++ b/providers/aws/services/opensearch/client.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "log" + "math" "sync" "time" @@ -19,8 +20,8 @@ import ( "github.com/LeanerCloud/CUDly/providers/aws/internal/purchasecfg" ) -// OpenSearchAPI defines the interface for OpenSearch operations (enables mocking) -type OpenSearchAPI interface { +// API defines the interface for OpenSearch operations (enables mocking). +type API interface { PurchaseReservedInstanceOffering(ctx context.Context, params *opensearch.PurchaseReservedInstanceOfferingInput, optFns ...func(*opensearch.Options)) (*opensearch.PurchaseReservedInstanceOfferingOutput, error) DescribeReservedInstanceOfferings(ctx context.Context, params *opensearch.DescribeReservedInstanceOfferingsInput, optFns ...func(*opensearch.Options)) (*opensearch.DescribeReservedInstanceOfferingsOutput, error) DescribeReservedInstances(ctx context.Context, params *opensearch.DescribeReservedInstancesInput, optFns ...func(*opensearch.Options)) (*opensearch.DescribeReservedInstancesOutput, error) @@ -33,15 +34,14 @@ type STSAPI interface { GetCallerIdentity(ctx context.Context, params *sts.GetCallerIdentityInput, optFns ...func(*sts.Options)) (*sts.GetCallerIdentityOutput, error) } -// Client handles AWS OpenSearch Reserved Instances +// Client handles AWS OpenSearch Reserved Instances. type Client struct { - client OpenSearchAPI - stsClient STSAPI - region string - - accountOnce sync.Once - accountID string + client API + stsClient STSAPI accountErr error + accountID string + region string + accountOnce sync.Once } // NewClient creates a new OpenSearch client with purchase-path retry/timeout @@ -55,32 +55,32 @@ func NewClient(cfg aws.Config) *Client { } } -// SetOpenSearchAPI sets a custom OpenSearch API client (for testing) -func (c *Client) SetOpenSearchAPI(api OpenSearchAPI) { +// SetOpenSearchAPI sets a custom OpenSearch API client (for testing). +func (c *Client) SetOpenSearchAPI(api API) { c.client = api } -// SetSTSAPI sets a custom STS client (for testing) +// SetSTSAPI sets a custom STS client (for testing). func (c *Client) SetSTSAPI(api STSAPI) { c.stsClient = api } -// GetServiceType returns the service type +// GetServiceType returns the service type. func (c *Client) GetServiceType() common.ServiceType { return common.ServiceSearch } -// GetRegion returns the region +// GetRegion returns the region. func (c *Client) GetRegion() string { return c.region } -// GetRecommendations returns empty as OpenSearch uses centralized Cost Explorer recommendations +// GetRecommendations returns empty as OpenSearch uses centralized Cost Explorer recommendations. func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } -// GetExistingCommitments retrieves existing OpenSearch Reserved Instances +// GetExistingCommitments retrieves existing OpenSearch Reserved Instances. func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { commitments := make([]common.Commitment, 0) var nextToken *string @@ -131,14 +131,17 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen // PurchaseCommitment purchases an OpenSearch Reserved Instance. // -// PurchaseReservedInstanceOfferingInput has no Tags field — tagging happens +// PurchaseReservedInstanceOfferingInput has no Tags field -- tagging happens // post-purchase via opensearch:AddTags with a reserved-instance ARN // (arn:aws:es:::reserved-instance/). AWS hasn't // explicitly documented reserved-instance as a supported resource type for // AddTags (only domain/data-source/application), so the call may return a -// validation error — in which case retry.ErrPermanent short-circuits and the +// validation error -- in which case retry.ErrPermanent short-circuits and the // failure is logged without blocking the purchase. If AWS ever adds support, // this will start working with no code change. +// On tagging failure a structured line is emitted (OPENSEARCH_TAG_FAILED) so +// operator dashboards can alert on untagged RIs; see +// runbooks/opensearch-untagged-ri.md for the manual remediation steps. func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, @@ -185,10 +188,15 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, nil } + instanceCount, countErr := safeInt32Count(rec.Count) + if countErr != nil { + result.Error = countErr + return result, result.Error + } input := &opensearch.PurchaseReservedInstanceOfferingInput{ ReservedInstanceOfferingId: aws.String(offeringID), ReservationName: aws.String(reservationName), - InstanceCount: aws.Int32(int32(rec.Count)), // #nosec G115 -- Count from CE recommendation; AWS RI purchase limits keep this far below math.MaxInt32 + InstanceCount: aws.Int32(instanceCount), } response, err := c.client.PurchaseReservedInstanceOffering(ctx, input) @@ -211,7 +219,9 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati } if err := c.tagReservedInstance(ctx, result.CommitmentID, rec, opts.Source); err != nil { - log.Printf("WARNING: failed to tag OpenSearch RI %s after purchase (RI is bought; tag missing, source recorded in purchase_history): %v", result.CommitmentID, err) + // Structured line for operator dashboards / alerting. The RI is + // purchased; only the tag is missing. Follow runbooks/opensearch-untagged-ri.md. + log.Printf("OPENSEARCH_TAG_FAILED commitment_id=%s error=%v", result.CommitmentID, err) } return result, nil @@ -463,7 +473,7 @@ func normalizeOpenSearchPaymentOption(option string) string { } } -// matchesPaymentOption checks if the offering payment option matches +// matchesPaymentOption checks if the offering payment option matches. func (c *Client) matchesPaymentOption(offeringOption types.ReservedInstancePaymentOption, required string) bool { switch required { case "all-upfront": @@ -499,13 +509,13 @@ func (c *Client) matchesDuration(offeringDuration int32, requiredMonths int) boo return int(offeringMonths) >= requiredMonths-1 && int(offeringMonths) <= requiredMonths+1 } -// ValidateOffering checks if an offering exists without purchasing +// ValidateOffering checks if an offering exists without purchasing. func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation) error { _, err := c.findOfferingID(ctx, rec, "") return err } -// GetOfferingDetails retrieves offering details +// GetOfferingDetails retrieves offering details. func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { offeringID, err := c.findOfferingID(ctx, rec, "") if err != nil { @@ -541,7 +551,7 @@ func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendati return details, nil } -// GetValidResourceTypes returns valid OpenSearch instance types (static list) +// GetValidResourceTypes returns valid OpenSearch instance types (static list). func (c *Client) GetValidResourceTypes(ctx context.Context) ([]string, error) { return []string{ "t2.small.search", @@ -597,7 +607,7 @@ func (c *Client) GetValidResourceTypes(ctx context.Context) ([]string, error) { }, nil } -// getTermMonthsFromDuration converts duration in seconds to months +// getTermMonthsFromDuration converts duration in seconds to months. func getTermMonthsFromDuration(duration int32) int { offeringMonths := duration / 2592000 if offeringMonths >= 30 { @@ -605,3 +615,13 @@ func getTermMonthsFromDuration(duration int32) int { } return 12 } + +// safeInt32Count validates that n is a positive value that fits in int32 and +// returns it as int32. This prevents a gosec G115 integer-overflow conversion +// on the PurchaseReservedInstanceOffering InstanceCount field. +func safeInt32Count(n int) (int32, error) { + if n < 1 || n > math.MaxInt32 { + return 0, fmt.Errorf("instance count %d is out of valid range [1, %d]", n, math.MaxInt32) + } + return int32(n), nil +} diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index e9662fd5a..a51011f45 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -1,8 +1,10 @@ package opensearch import ( + "bytes" "context" "fmt" + "log" "strings" "testing" "time" @@ -16,7 +18,7 @@ import ( "github.com/stretchr/testify/mock" ) -// MockOpenSearchClient implements OpenSearchAPI for testing +// MockOpenSearchClient implements API for testing. type MockOpenSearchClient struct { mock.Mock } @@ -97,8 +99,8 @@ func TestClient_GetRecommendations(t *testing.T) { func TestClient_GetExistingCommitments(t *testing.T) { tests := []struct { - name string setupMocks func(*MockOpenSearchClient) + name string expectedLen int expectError bool }{ @@ -1008,3 +1010,67 @@ func TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall(t *testing.T) { } mockOS.AssertNotCalled(t, "DescribeReservedInstanceOfferings", mock.Anything, mock.Anything) } + +// TestPurchaseCommitment_TagFailure_StructuredLog asserts that when AddTags +// returns an error after a successful purchase: +// - the purchase result is still Success=true (tag failure is non-fatal) +// - a line containing "OPENSEARCH_TAG_FAILED" is emitted to the log +// - the commitment ID is present in that log line (for operator lookup) +// - no AWS account ID or other PII appears in the log line +func TestPurchaseCommitment_TagFailure_StructuredLog(t *testing.T) { + mockOS := &MockOpenSearchClient{} + mockSTS := &MockOpenSearchSTSClient{} + t.Cleanup(func() { mockOS.AssertExpectations(t); mockSTS.AssertExpectations(t) }) + + client := &Client{client: mockOS, stsClient: mockSTS, region: "us-east-1"} + + rec := common.Recommendation{ + Service: common.ServiceSearch, + ResourceType: "m5.large.search", + Count: 1, + Term: "1yr", + Region: "us-east-1", + PaymentOption: "all-upfront", + } + + mockOS.On("DescribeReservedInstanceOfferings", mock.Anything, mock.Anything). + Return(&opensearch.DescribeReservedInstanceOfferingsOutput{ + ReservedInstanceOfferings: []types.ReservedInstanceOffering{{ + ReservedInstanceOfferingId: aws.String("off-tag-fail"), + InstanceType: types.OpenSearchPartitionInstanceTypeM5LargeSearch, + Duration: 31536000, + PaymentOption: types.ReservedInstancePaymentOptionAllUpfront, + }}, + }, nil) + + const riID = "ri-tag-fail-abc123" + mockOS.On("PurchaseReservedInstanceOffering", mock.Anything, mock.Anything). + Return(&opensearch.PurchaseReservedInstanceOfferingOutput{ + ReservedInstanceId: aws.String(riID), + }, nil) + + mockSTS.On("GetCallerIdentity", mock.Anything, mock.Anything). + Return(&sts.GetCallerIdentityOutput{Account: aws.String("000000000000")}, nil) + + mockOS.On("AddTags", mock.Anything, mock.Anything). + Return(nil, fmt.Errorf("ValidationException: invalid resource type")).Once() + + // Redirect log output to capture the structured line. + var buf bytes.Buffer + origWriter := log.Writer() + log.SetOutput(&buf) + t.Cleanup(func() { log.SetOutput(origWriter) }) + + result, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + + assert.NoError(t, err, "tag failure must not surface as a purchase error") + assert.True(t, result.Success, "purchase must remain successful when tagging fails") + assert.Equal(t, riID, result.CommitmentID) + + logOut := buf.String() + assert.Contains(t, logOut, "OPENSEARCH_TAG_FAILED", "structured sentinel must appear in log") + assert.Contains(t, logOut, "commitment_id="+riID, "commitment ID must be present for operator lookup") + // The account ID (000000000000) must not be logged -- it is not PII but + // the structured line should stay minimal and scoped to the commitment. + assert.NotContains(t, logOut, "000000000000", "account ID must not appear in the tag-failure log line") +} diff --git a/runbooks/opensearch-untagged-ri.md b/runbooks/opensearch-untagged-ri.md new file mode 100644 index 000000000..7c29696da --- /dev/null +++ b/runbooks/opensearch-untagged-ri.md @@ -0,0 +1,77 @@ +# Runbook: OpenSearch RI purchased but not tagged + +## Alert + +Log pattern (grep / CloudWatch Logs Insights): + +```text +OPENSEARCH_TAG_FAILED commitment_id= error= +``` + +Emitted by `providers/aws/services/opensearch/client.go` when +`opensearch:AddTags` fails after a successful RI purchase. The RI is active; +only the CUDly source tag is absent. + +## Background + +AWS's `PurchaseReservedInstanceOffering` API has no inline `Tags` field. +CUDly attempts a best-effort `AddTags` call after purchase using a +constructed ARN of the form: + +```text +arn:aws:es:::reserved-instance/ +``` + +AWS has not officially documented `reserved-instance` as a supported ARN +type for `opensearch:AddTags` (only `domain`, `data-source`, and +`application` are listed). The call is wrapped in `retry.ErrPermanent` so +the retry budget is not exhausted on calls AWS will never accept. If AWS +extends support, the tag call will start succeeding with no code change. + +## Impact + +The RI is purchased and active. Cost attribution and audit queries that +rely on the CUDly source tag (key: `cudly:purchase-source`) will not find +this reservation unless it is manually tagged. + +## Remediation + +1. Identify the untagged RI from the log line: + + ```text + OPENSEARCH_TAG_FAILED commitment_id= error=... + ``` + +2. Tag it manually via the AWS CLI: + + ```bash + REGION= + ACCOUNT= + RI_ID= + SOURCE= # e.g. cudly-cli or cudly-web + + aws opensearch add-tags \ + --arn "arn:aws:es:${REGION}:${ACCOUNT}:reserved-instance/${RI_ID}" \ + --tag-list \ + Key=Purpose,Value="Reserved Instance Purchase" \ + Key=Tool,Value=CUDly \ + "Key=cudly:purchase-source,Value=${SOURCE}" + ``` + + If the call returns a `ValidationException` the ARN type is still + unsupported by AWS; proceed to the fallback below. + +3. **Fallback (AWS still rejects reserved-instance ARN):** Tag the parent + OpenSearch domain instead, or record the RI ID in your cost-allocation + spreadsheet until AWS adds native support. + +## Follow-up + +If this alert fires repeatedly (not just on ValidationException but on +transient errors), open an issue to add a retry with backoff instead of +the current permanent-error short-circuit. Reference issue #250. + +If AWS releases documentation confirming `reserved-instance` support for +`AddTags`, remove the `retry.ErrPermanent` wrapper in +`providers/aws/services/opensearch/client.go:tagReservedInstance` and +update this runbook.