-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pagerduty AMR integration #44754
Conversation
5222085
to
fdeb10d
Compare
@tigrato can you take a look at this when you get a chance? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add test coverage.
integrations/access/pagerduty/app.go
Outdated
func (a *App) getMessageRecipient(ctx context.Context, req types.AccessRequest) (string, error) { | ||
recipientSetService := a.accessMonitoringRules.RecipientsFromAccessMonitoringRules(ctx, req) | ||
if recipientSetService.Len() > 1 { | ||
return "", trace.BadParameter("more than one service provided as PagerDuty plugin recipient") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not support this? Can we just take the first one instead of hard-failing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these could be coming from separate rules with an undefined evaluation order taking the first would be pretty much just taking a random one which I doubt would be any users intent. I think failing to post anything would make more sense until we have the ability to set evaluation order for rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EdwardDowling Test coverage is still missing on this PR.
@EdwardDowling See the table below for backport results.
|
Initial integration of Pagerduty plugin and Access Monitoring Rules routing
Depends on 43298 being merged first and will take this out of draft and update the description when it is.
changelog: Allow PagerDuty service used by plugin to be dynamically configured by creating Access Monitoring Rules resources with the required Pagerduty notify services.