Implement access monitor#53769
Conversation
bernardjkim
added
the
no-changelog
bernardjkim
force-pushed
the
bernard/access-monitor
branch
from
e7365a0 to
e2e559a
Compare
bernardjkim
requested review from
marcoandredinis and
tigrato
and removed request for
espadolini and
rudream
|
Hey @tigrato @marcoandredinis, could you also take a look at this PR when you have some time. Thanks! |
| // of restarting indefinitely. Fail fast mode should only be used within a | ||
| // plugin environment. See https://github.com/gravitational/teleport/pull/30039 | ||
| // for more details. | ||
| // TODO(bernardjkim): Investigate if fail fast mode is still required. |
There was a problem hiding this comment.
Is this TODO still relevant?
There was a problem hiding this comment.
Yeah, Hugo raised this concern in #53769 (comment).
The plan is to eventually replace the access request plugin Apps with this AccessMonitor implementation. I'll investigate further at that time if we still need to support a fail fast mode.
| s.cfg.Logger.ErrorContext( | ||
| ctx, | ||
| "Encountered a fatal error, it will restart after backoff.", | ||
| "component", componentName, | ||
| "error", err, | ||
| "restart_after", waitWithJitter, | ||
| ) | ||
| if s.cfg.FailFast { | ||
| return trace.Wrap(err) | ||
| } |
There was a problem hiding this comment.
If FailFast is enabled, it will not restart as the log line says.
Should we swap the log with the if block?
| select { | ||
| case initEvent := <-watcher.Events(): | ||
| if initEvent.Type != types.OpInit { | ||
| return trace.BadParameter("watcher yielded %[1]v (%[1]d) as first event, expected Init (this is a bug)", initEvent.Type) |
There was a problem hiding this comment.
Is this string valid for string formatting?
It has two placeholders (%[1]v and %[1]d), but only one value (initEvent.Type)
There was a problem hiding this comment.
I believe so. The %[1] should resolve to the argument at index 1. Just to double check, this would return something like: watcher yielded Delete (2) as first event, expected Init (this is a bug).
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
4 participants
Supports: #51682
RFD: #51979
This PR implements an access monitor. This monitor watches
access_monitoring_ruleandaccess_requestevents, and then it executes the configured handlers for each event.This access monitor is a slightly refactored implementation of the
integrations/accessrequest.App. I'd like to unify theintegrations/accessrequestApps with this new implementation in future PRs.These changes are split from a larger PR. To see how it'll be used with the access review handler, see #52988.