audit(aws): surface OpenSearch RI tag-failure for ops monitoring (closes #250)

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:<region>:<account>:reserved-instance/<uuid>). 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,11 +607,21 @@ 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 {
 		return 36
 	}
 	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
+}

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")
+}

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=<id> error=<msg>
+```
+
+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:<region>:<account>:reserved-instance/<uuid>
+```
+
+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=<ri-uuid> error=...
+   ```
+
+2. Tag it manually via the AWS CLI:
+
+   ```bash
+   REGION=<region>
+   ACCOUNT=<account-id>
+   RI_ID=<ri-uuid>
+   SOURCE=<purchase-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.