Pagerduty plugin migration to new API client and support of new Teleport features #220
Conversation
|
@marshall-lee does it close it with the new auto behavior as well? |
marshall-lee
force-pushed
the
marshall-lee/pagerduty/migrate-6.2
branch
from
ee663d3
to
28679a4
Compare
marshall-lee
requested review from
klizhentas,
r0mant and
russjones
as code owners
marshall-lee
force-pushed
the
marshall-lee/pagerduty/migrate-6.2
branch
3 times, most recently
from
432cd46
to
4f4d9d6
Compare
marshall-lee
force-pushed
the
marshall-lee/pagerduty/migrate-6.2
branch
2 times, most recently
from
2e4eb65
to
57c34f6
Compare
marshall-lee
force-pushed
the
marshall-lee/pagerduty/migrate-6.2
branch
4 times, most recently
from
739b5f6
to
86b1c09
Compare
marshall-lee
changed the title
marshall-lee
force-pushed
the
marshall-lee/pagerduty/migrate-6.2
branch
14 times, most recently
from
cb898be
to
49c25bd
Compare
marshall-lee
force-pushed
the
marshall-lee/pagerduty/migrate-6.2
branch
6 times, most recently
from
c4eb4c8
to
79e195d
Compare
marshall-lee
changed the title
|
@r0mant @Joerger @fspmarshall can you team please review? |
|
@marshall-lee can you please refresh PR to resolve conflicts? |
marshall-lee
force-pushed
the
marshall-lee/pagerduty/migrate-6.2
branch
2 times, most recently
from
faf2ad7
to
ba7d3cd
Compare
|
@r0mant @fspmarshall @Joerger please review folks |
access/pagerduty/app.go
Outdated
| return trace.Wrap(err) | ||
| } else { | ||
| logger.Get(ctx).Debugf("Failed to determine a notification service info: %s", err.Error()) | ||
| shouldTryApprove = true |
There was a problem hiding this comment.
Curious why do we fallback to trying to auto-approve if we failed to get the service info? Wouldn't it be safer to default to false?
There was a problem hiding this comment.
Thanks for pointing this out, I rewrote that part and added some comments that clarify my idea.
access/pagerduty/app.go
Outdated
| RequestID: req.GetName(), | ||
| Review: types.AccessReview{ | ||
| ProposedState: types.RequestState_APPROVED, | ||
| Reason: fmt.Sprintf("Access requested by on-call user %s (%s)", user.Name, user.Email), |
There was a problem hiding this comment.
Log the active incident in the reason as well for tracking?
There was a problem hiding this comment.
There could be more than one active incident and theoretically there could be many of them.
But I added a logging of service names in which there're some active incidents found.
| // modifyPluginData performs a compare-and-swap update of access request's plugin data. | ||
| func (a *App) modifyPluginData(ctx context.Context, reqID string, fn func(data *PluginData) (PluginData, bool)) (bool, error) { | ||
| var lastErr error | ||
| for i := 0; i < maxModifyPluginDataTries; i++ { |
There was a problem hiding this comment.
I would convert this to a select statement on a ticker inside the for loop to provide some backoff and be nice to the PD API.
There was a problem hiding this comment.
I don't see how the backoff is useful here. It's a compare-and-swap trial loop, we can just retry immediately.
And in 99.99% cases there will be at most one retry.
marshall-lee
force-pushed
the
marshall-lee/pagerduty/migrate-6.2
branch
2 times, most recently
from
efce3e4
to
8730ae9
Compare
marshall-lee
force-pushed
the
marshall-lee/pagerduty/migrate-6.2
branch
2 times, most recently
from
c7b2738
to
ce1f795
Compare
marshall-lee
force-pushed
the
marshall-lee/pagerduty/migrate-6.2
branch
from
ce1f795
to
a0e4953
Compare
- Render resolve reason in the notification incident. - Render access reviews as a comments to notification incident. - Configure target services using request annotations. - Manage plugin data state safely using compare-and-swap. - Migrate tests to testify.
marshall-lee
force-pushed
the
marshall-lee/pagerduty/migrate-6.2
branch
from
a0e4953
to
38335bf
Compare
|
closed in favor of #266 |
Closes #219.
Part of #209.