feat: Add AWS external_id support in sigv4 configuration#5157
Open
dongjiang1989 wants to merge 2 commits intoprometheus:mainfrom
Open
feat: Add AWS external_id support in sigv4 configuration#5157dongjiang1989 wants to merge 2 commits intoprometheus:mainfrom
dongjiang1989 wants to merge 2 commits intoprometheus:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
notify/sns/sns.go (1)
143-158:⚠️ Potential issue | 🟡 MinorEnforce
external_id+role_arncoupling at runtime.
external_idis only read inside therole_arnbranch (Line 143), so a config withexternal_idbut norole_arnis silently ignored. Since docs declare this constraint, add an explicit error to fail fast on invalid config.Suggested guard
func (n *Notifier) createSNSClient(ctx context.Context, tmpl func(string) string, tmplErr *error) (*sns.Client, error) { + if n.conf.Sigv4.ExternalID != "" && n.conf.Sigv4.RoleARN == "" { + return nil, fmt.Errorf("sns.sigv4.external_id requires sns.sigv4.role_arn") + } + // Base configuration options that apply to both STS (if used) and the final SNS client. baseCfgOpts := []func(*awsconfig.LoadOptions) error{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notify/sns/sns.go` around lines 143 - 158, The code currently ignores n.conf.Sigv4.ExternalID when n.conf.Sigv4.RoleARN is empty; add a runtime guard before the STS branch (the block that creates stsClient/stsProvider) that validates the coupling: if n.conf.Sigv4.ExternalID is set but n.conf.Sigv4.RoleARN is empty, return an error (from the function that initializes the SNS client) indicating invalid config. Reference the config fields n.conf.Sigv4.ExternalID and n.conf.Sigv4.RoleARN and perform this check prior to calling awsconfig.LoadDefaultConfig / sts.NewFromConfig / stscreds.NewAssumeRoleProvider so the function fails fast on the invalid configuration.
🧹 Nitpick comments (1)
notify/sns/sns_test.go (1)
121-123: Current test data includesExternalIDbut does not verify the new behavior.This case still only asserts template failures. Please add a focused test that verifies
ExternalIDis passed to STS AssumeRole whenrole_arnis set (and omitted when it is not), so the new feature is actually regression-protected.Also applies to: 137-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notify/sns/sns_test.go` around lines 121 - 123, Add a focused unit test that asserts ExternalID is forwarded to STS.AssumeRole when a RoleARN is provided and omitted when no RoleARN is set: in the sns_test.go tests that set Region/RoleARN/ExternalID (symbols: RoleARN and ExternalID) create a mock STS client and verify its AssumeRole invocation includes the ExternalId parameter when RoleARN is set, and that AssumeRole is called without ExternalId (or not called at all if implementation skips assume) when RoleARN is empty; update or add two assertions (one for the positive case and one for the negative case) to guard the new behavior so ExternalID regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@notify/sns/sns.go`:
- Around line 143-158: The code currently ignores n.conf.Sigv4.ExternalID when
n.conf.Sigv4.RoleARN is empty; add a runtime guard before the STS branch (the
block that creates stsClient/stsProvider) that validates the coupling: if
n.conf.Sigv4.ExternalID is set but n.conf.Sigv4.RoleARN is empty, return an
error (from the function that initializes the SNS client) indicating invalid
config. Reference the config fields n.conf.Sigv4.ExternalID and
n.conf.Sigv4.RoleARN and perform this check prior to calling
awsconfig.LoadDefaultConfig / sts.NewFromConfig / stscreds.NewAssumeRoleProvider
so the function fails fast on the invalid configuration.
---
Nitpick comments:
In `@notify/sns/sns_test.go`:
- Around line 121-123: Add a focused unit test that asserts ExternalID is
forwarded to STS.AssumeRole when a RoleARN is provided and omitted when no
RoleARN is set: in the sns_test.go tests that set Region/RoleARN/ExternalID
(symbols: RoleARN and ExternalID) create a mock STS client and verify its
AssumeRole invocation includes the ExternalId parameter when RoleARN is set, and
that AssumeRole is called without ExternalId (or not called at all if
implementation skips assume) when RoleARN is empty; update or add two assertions
(one for the positive case and one for the negative case) to guard the new
behavior so ExternalID regressions are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f430d2ab-b5e7-4da1-9c8b-ada1ea7d1d05
📒 Files selected for processing (3)
docs/configuration.mdnotify/sns/sns.gonotify/sns/sns_test.go
dongjiang1989
changed the title
Signed-off-by: dongjiang <dongjiang1989@126.com>
dongjiang1989
force-pushed
the
update-sigv4
branch
from
88dbf82 to
4d242dc
Compare
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.
Pull Request Checklist
Which user-facing changes does this PR introduce?
Summary by CodeRabbit