access_monitoring_rules: Implement schedules conditions #59044
Conversation
bernardjkim
force-pushed
the
bernard/condition-schedules
branch
from
e7e61ea to
9607e8e
Compare
bernardjkim
force-pushed
the
bernard/condition-schedules
branch
from
9607e8e to
ef4e98a
Compare
r0mant
left a comment
r0mant
left a comment
There was a problem hiding this comment.
@bernardjkim I've a question about the UX. Why is access_request.spec.creation_time.in_schedule(xxx) required at all as a part of the predicate condition? Would there be a downside to simplifying this and always applying the schedules if they're defined on the AMR?
I think that being explicit is better because you can combine with other rules, do OR, ... This is way more versatile than gluing a generic rule with an AND on top of the expression. This can also be extended if we add more data into the context. The design is inspired by what the OPA does. |
Consider all configured schedules by default
|
@r0mant as discussed offline, I've simplified the implementation to remove the need for schedule conditions, and consider all schedules by default. @hugoShaka we agree that the schedule conditions would provide a lot more versatility when configuring rules. However, we thought it might be best to keep it simple for now, and focus on the initial scope of the feature request. Open to discussing it more if you have strong opinions. |
bernardjkim
requested review from
r0mant
and removed request for
atburke and
ryanclark
bernardjkim
force-pushed
the
bernard/condition-schedules
branch
from
3cd53c6 to
46b4f85
Compare
| continue | ||
| } | ||
| if len(rule.GetSpec().GetSchedules()) != 0 && !isInSchedules { | ||
| continue |
There was a problem hiding this comment.
Here and below, should we log that we're skipping the request because the schedule doesn't match?
public-teleport-github-review-bot
bot
removed the request for review
from hugoShaka
|
@bernardjkim See the table below for backport results.
|
* Implement schedules conditions * Fix lint * Remove schedules predicate expression Consider all configured schedules by default * Support schedules within notification rules * Use timezone during evaluation * Add debug logs
* Implement schedules conditions * Fix lint * Remove schedules predicate expression Consider all configured schedules by default * Support schedules within notification rules * Use timezone during evaluation * Add debug logs
* Implement schedules conditions * Fix lint * Remove schedules predicate expression Consider all configured schedules by default * Support schedules within notification rules * Use timezone during evaluation * Add debug logs
Supports: #51682, #55075
RFD: #57334
Requires: #59007
Changelog: Enabled use of schedules within automatic review and notification access_monitoring_rules