Repository navigation
audit(aws): surface OpenSearch RI tag-failure for ops monitoring (closes #250) - #831
Merged
Merged
Conversation
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
📝 WalkthroughWalkthroughThe OpenSearch client now uses a renamed API interface, validates reserved-instance counts, emits structured logs when tagging fails, and adds tests and an operational runbook for untagged reservations. ChangesOpenSearch commitment flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
cristim
added a commit
that referenced
this pull request
Jun 1, 2026
…lure (#831) The prior run failed because GitHub's CDN returned a 404 for the pinned actions/setup-go SHA at download time (transient infrastructure issue). No code changes are needed; this empty commit re-triggers CI.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
Rate Limit Exceeded
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
cristim
added a commit
that referenced
this pull request
Jun 19, 2026
…lure (#831) The prior run failed because GitHub's CDN returned a 404 for the pinned actions/setup-go SHA at download time (transient infrastructure issue). No code changes are needed; this empty commit re-triggers CI.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 10, 2026
…lure (#831) The prior run failed because GitHub's CDN returned a 404 for the pinned actions/setup-go SHA at download time (transient infrastructure issue). No code changes are needed; this empty commit re-triggers CI.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/aws/services/opensearch/client_test.go`:
- Around line 954-1017: The test TestPurchaseCommitment_TagFailure_StructuredLog
must also verify that the structured log includes the tag failure error details,
not just the sentinel and commitment ID. Add an assertion against logOut for the
expected error field/value, such as the ValidationException message, while
retaining the existing PII exclusion assertion.
In `@providers/aws/services/opensearch/client.go`:
- Around line 191-199: Move the safeInt32Count validation in the reservation
purchase flow to immediately after result is initialized, before findOfferingID
and idempotencyGuard execute; preserve the existing error assignment and early
return so invalid counts cannot bypass validation. Use the surrounding purchase
method and safeInt32Count as anchors.
In `@runbooks/opensearch-untagged-ri.md`:
- Around line 64-66: Revise the fallback in the runbook so parent OpenSearch
domain tagging is explicitly described as an attribution workaround, not
reserved-instance remediation. Explain that RI-level queries must map the RI ID
to the tagged parent domain (or provide equivalent query changes) to preserve
cost and audit attribution; otherwise retain the spreadsheet record-only option.
- Around line 11-13: Update the runbook to state that a failed
opensearch:AddTags request may leave all six tags absent: Purpose, ResourceType,
Region, PurchaseDate, Tool, and the CUDly source tag. Revise the remediation
command to restore the complete tag set, or explicitly instruct operators to
verify and restore every tag before closing the incident.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e1249d6-614f-43d5-a33f-6a5a9306dd3b
📒 Files selected for processing (3)
providers/aws/services/opensearch/client.goproviders/aws/services/opensearch/client_test.gorunbooks/opensearch-untagged-ri.md
Comment on lines
+954
to
+1017
|
|
||
| // 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") | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the error field as well as the sentinel.
Lines 1011-1016 do not verify the error value, so a log containing only OPENSEARCH_TAG_FAILED commitment_id=... would pass. Assert the expected field too:
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")
+ assert.Contains(t, logOut, "error=ValidationException: invalid resource type")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 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") | |
| } | |
| 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") | |
| assert.Contains(t, logOut, "error=ValidationException: invalid resource type") | |
| // 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") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@providers/aws/services/opensearch/client_test.go` around lines 954 - 1017,
The test TestPurchaseCommitment_TagFailure_StructuredLog must also verify that
the structured log includes the tag failure error details, not just the sentinel
and commitment ID. Add an assertion against logOut for the expected error
field/value, such as the ValidationException message, while retaining the
existing PII exclusion assertion.
Comment on lines
+191
to
+199
| 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)), | ||
| InstanceCount: aws.Int32(instanceCount), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the count before lookups and idempotency handling.
At Lines 191-195, validation happens after findOfferingID and idempotencyGuard. The guard at Lines 182-188 can therefore return success for an existing reservation before safeInt32Count runs, allowing an invalid count to bypass the new validation. Move this check immediately after result is initialized.
Suggested placement
func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) {
result := common.PurchaseResult{
// ...
}
+
+ instanceCount, countErr := safeInt32Count(rec.Count)
+ if countErr != nil {
+ result.Error = countErr
+ return result, result.Error
+ }
offeringID, err := c.findOfferingID(ctx, rec, opts.ExecutionID)
// ...
- instanceCount, countErr := safeInt32Count(rec.Count)
- if countErr != nil {
- result.Error = countErr
- return result, result.Error
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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)), | |
| InstanceCount: aws.Int32(instanceCount), | |
| func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { | |
| result := common.PurchaseResult{ | |
| // ... | |
| } | |
| instanceCount, countErr := safeInt32Count(rec.Count) | |
| if countErr != nil { | |
| result.Error = countErr | |
| return result, result.Error | |
| } | |
| offeringID, err := c.findOfferingID(ctx, rec, opts.ExecutionID) | |
| // ... | |
| input := &opensearch.PurchaseReservedInstanceOfferingInput{ | |
| ReservedInstanceOfferingId: aws.String(offeringID), | |
| ReservationName: aws.String(reservationName), | |
| InstanceCount: aws.Int32(instanceCount), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@providers/aws/services/opensearch/client.go` around lines 191 - 199, Move the
safeInt32Count validation in the reservation purchase flow to immediately after
result is initialized, before findOfferingID and idempotencyGuard execute;
preserve the existing error assignment and early return so invalid counts cannot
bypass validation. Use the surrounding purchase method and safeInt32Count as
anchors.
Comment on lines
+11
to
+13
| 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. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Describe all tags affected by the failed request.
Lines 11-13 claim only the CUDly source tag is absent, but client.go:342-349 sends six tags in one AddTags request. A failed request may leave Purpose, ResourceType, Region, PurchaseDate, Tool, and the source tag absent. The manual command at Lines 53-58 also restores only a subset; reword this and either restore or verify the complete tag set.
Also applies to: 45-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@runbooks/opensearch-untagged-ri.md` around lines 11 - 13, Update the runbook
to state that a failed opensearch:AddTags request may leave all six tags absent:
Purpose, ResourceType, Region, PurchaseDate, Tool, and the CUDly source tag.
Revise the remediation command to restore the complete tag set, or explicitly
instruct operators to verify and restore every tag before closing the incident.
Comment on lines
+64
to
+66
| 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. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not present parent-domain tagging as RI remediation.
Tagging the parent domain does not attach cudly:purchase-source to the reserved instance, so the RI-level cost and audit queries described at Lines 33-35 can still miss it. Document this as a separate attribution workaround and provide the required mapping/query changes, or retain record-only remediation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@runbooks/opensearch-untagged-ri.md` around lines 64 - 66, Revise the fallback
in the runbook so parent OpenSearch domain tagging is explicitly described as an
attribution workaround, not reserved-instance remediation. Explain that RI-level
queries must map the RI ID to the tagged parent domain (or provide equivalent
query changes) to preserve cost and audit attribution; otherwise retain the
spreadsheet record-only option.
#250) Emit a structured OPENSEARCH_TAG_FAILED log line when the best-effort AddTags call after RI purchase fails, so operator dashboards can alert on untagged reservations. Add runbooks/opensearch-untagged-ri.md with manual remediation steps and background on the unsupported ARN type. Update the PurchaseCommitment comment to reference the runbook.
…lure (#831) The prior run failed because GitHub's CDN returned a 404 for the pinned actions/setup-go SHA at download time (transient infrastructure issue). No code changes are needed; this empty commit re-triggers CI.
- Add periods to 14 comments missing godot terminator - Rename OpenSearchAPI interface to API (revive: stutters as opensearch.OpenSearchAPI); update SetOpenSearchAPI and test comment - Reorder Client struct fields for optimal GC pointer bitmap (govet fieldalignment: 80->72 pointer bytes) - Reorder two test-case structs to remove padding (govet fieldalignment) - Add rec.Count bounds check before int32 conversion; suppress gosec G115 with nolint comment on the now-safe cast - Add .Once() to AddTags mock to assert exactly one call (ErrPermanent guarantees no retry; strengthens the new tag-failure test)
Replace the opaque uint-subtraction trick with explicit < 1 || > MaxInt32 guards so gosec can recognize the pattern as a safe bounded conversion and no longer flags G115 on int32(n).
The pre-commit markdownlint hook (MD040) failed on runbooks/opensearch-untagged-ri.md because three fenced code blocks lacked a language specifier. Tag the plain-text log-pattern and ARN blocks as `text` so the pre-commit hook passes.
cristim
added a commit
that referenced
this pull request
Jul 19, 2026
…it (follow-up to #831) (#1460) PurchaseCommitment ran safeInt32Count after the idempotency guard, so a re-drive carrying an invalid instance count (negative / int32-overflow / zero) against an already-existing reservation would short-circuit to Success and never surface the bad input. Move the count validation to the boundary -- immediately after result init, before findOfferingID and the idempotency guard -- so invalid counts fail loud regardless of idempotency state (addresses the unresolved CodeRabbit Major thread on #831). Also harden the tag-failure structured-log test to assert the error= field carries the underlying cause, not just the OPENSEARCH_TAG_FAILED sentinel and commitment_id (unresolved CodeRabbit Minor thread on #831). Tests: - New TestClient_PurchaseCommitment_InvalidCount_FailsBeforeIdempotencyGuard: verified it FAILS on the pre-fix ordering (returns Success via short-circuit) and PASSES after the reorder; asserts the offering lookup and reservation describe are never reached. - TestClient_PurchaseCommitment_OfferingNotFound now sets a valid Count=1 (it previously relied on Count=0 being tolerated up to the offering lookup).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WARNING:log line inPurchaseCommitmentwith astructured
OPENSEARCH_TAG_FAILED commitment_id=<id> error=<msg>line sooperator dashboards and CloudWatch Logs Insights queries can alert on
untagged RIs
runbooks/opensearch-untagged-ri.mdcovering background (unsupportedreserved-instance ARN type), manual re-tagging steps, and the fallback for
when AWS still rejects the call
PurchaseCommitmentcomment to reference the runbookTest plan
TestPurchaseCommitment_TagFailure_StructuredLog(new): verifiesOPENSEARCH_TAG_FAILEDsentinel andcommitment_id=field appear in thelog, purchase result is still
Success=true, and account ID is absent fromthe line
TestClient_PurchaseCommitment_AddTagsFailureDoesNotFailPurchaseremains green (non-fatal contract unchanged)
go vet ./providers/aws/services/opensearch/...cleanSummary by CodeRabbit
Bug Fixes
Documentation