Add initial condition parser for access monitoring rules #40659

EdwardDowling · 2024-04-18T16:11:25Z

Part of 3132
Adds parser for AMR conditions and enables additive recipients via the AMRs (Access monitoring rules)

Changelog: Add access monitoring rule routing for slack access plugin

integrations/access/accessrequest/app.go

integrations/access/accessrequest/request_mapping.go

integrations/access/accessrequest/app.go

github-actions · 2024-04-22T16:16:18Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

integrations/access/accessrequest/app.go

tigrato · 2024-04-23T08:55:27Z

integrations/access/accessrequest/app.go

+		return trace.Errorf("unexpected kind %s", kind)
+	}
+
+	req, ok := types.LegacyToResource153(event.Resource).(*accessmonitoringrulesv1.AccessMonitoringRule)


Do these resource kinds support expiration? If yes, this won't work because in those cases, the watch request is a pure metadata object

No plans to have expiration on the access monitoring rules.

If we have no plans, we need to forbid setting it.

By default, the backend service will use the resource.Metadata.Expires field as expiration which will cause troubles when dealing with resources deleted by the backend server.

Or better, let's just support here the correct resource from the watcher event stream for those cases and support anything the users want to do

I think forbidding it sounds like it makes more sense to me, but I am unsure what you mean by supporting the correct resource from watcher event stream. Could you elaborate a bit on what that would mean?

Generally each resource can have TTL set and I don't see why monitoring rules should be an exception.

integrations/access/accessrequest/app.go

lib/expression/parser.go

github-actions · 2024-04-23T15:09:28Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

github-actions · 2024-04-23T15:24:51Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

github-actions · 2024-04-26T15:11:15Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

github-actions · 2024-04-26T16:20:08Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

github-actions · 2024-04-26T16:25:14Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

r0mant

@EdwardDowling This PR is also missing test coverage.

integrations/access/accessrequest/app.go

r0mant · 2024-05-01T16:05:58Z

integrations/access/accessrequest/app.go

+		return trace.Errorf("unexpected kind %s", kind)
+	}
+
+	req, ok := types.LegacyToResource153(event.Resource).(*accessmonitoringrulesv1.AccessMonitoringRule)


Generally each resource can have TTL set and I don't see why monitoring rules should be an exception.

integrations/access/accessrequest/app.go

integrations/access/accessrequest/request_mapping.go

lib/services/local/access_monitoring_rules.go

integrations/access/accessrequest/app.go

integrations/access/accessrequest/request_mapping.go

lib/expression/parser.go

r0mant

@EdwardDowling The implementation overall lgtm (as long as you and @kimlisa made sure everything works end to end) but I don't really see any test coverage here, only a couple modified tests. Can you add proper tests here that create and evaluate monitoring rules?

Also, we talked about making sure that monitoring rules should override plugin's default recipient. Is that a part of this PR?

EdwardDowling · 2024-05-09T16:25:32Z

@EdwardDowling The implementation overall lgtm (as long as you and @kimlisa made sure everything works end to end) but I don't really see any test coverage here, only a couple modified tests. Can you add proper tests here that create and evaluate monitoring rules?

Also, we talked about making sure that monitoring rules should override plugin's default recipient. Is that a part of this PR?

The overwriting is handled in this PR, adding in more test coverage now.

r0mant

@EdwardDowling I still don't see a proper integration test here but I don't want to block the delivery of this feature on it any longer. Can you make sure to add a full end-to-end test as a follow up?

integrations/access/accessrequest/app.go

api/proto/teleport/accessmonitoringrules/v1/access_monitoring_rules.proto

api/proto/teleport/accessmonitoringrules/v1/access_monitoring_rules_service.proto

integrations/access/accessrequest/app.go

lib/services/local/access_monitoring_rules.go

integrations/access/accessrequest/app.go

integrations/access/accessrequest/request_mapping.go

lib/services/local/access_monitoring_rules.go

r0mant

@EdwardDowling We have about a week until we need to release this, can you add an integration test here? You guys tested it manually but lack of automated test coverage means it will regress very quickly.

lib/services/local/generic/generic.go

…tion

* Add initial condition parser for access monitoring rules * Update integrations/access/accessrequest/app.go Co-authored-by: Zac Bergquist <[email protected]> * Check previously unchecked error and minor refactor of AMR * Simplify check for applicable access monitoring rules * Refactor access monitoring rules plugin integration * Fix formating and move lock aquisition * Add methods for listing access monitoring rules with a filter * Add contains_any predicate expression func * Add in is_empty func to predicate expression * Lock AMR cache in plugins while getting initial rules * Add in check for access monitoring rule version * Update integrations/access/accessrequest/app.go Co-authored-by: Roman Tkachenko <[email protected]> * Update integrations/access/accessrequest/app.go Co-authored-by: Roman Tkachenko <[email protected]> * Move lock so it doesnt persist over api calls * Remove unused constant and add more context to logs * Appease linter * Update access monitoring rules tests to pass rule validation * Add in missing access monitoring rules list with filter code * Appease linter * Add back validation code for AMRs * Fix test plugin role and rename listaccessmonitoringrulewithfilter * Fix local test for AMR crud operations * Fix end range for listing rules * Fix unwrapping of resource153 event for monitoring rules * Refactor AMR cache init into helper function in plugin app * Add seperate response type for listAccessMonitoringRulesWithfilter * Add context to log for plugins failing to fetch recipients * Grab access monitoring rules cache under lock all at once * Add clarification for which fields are optional in listAMRfilter req * Update integrations/access/accessrequest/app.go Co-authored-by: Zac Bergquist <[email protected]> * Update integrations/access/accessrequest/app.go Co-authored-by: Zac Bergquist <[email protected]> * Add forEach to common recipient set * Move type check to after AMR event op switch * Move turn some default parser spec methods to funcs * Make some predicate func usable as methods as well * Add len func to common recipient sets * Add integration test for access monitoring rule and slack plugin * Fix error types and messages when handling AMRs * Use generic list resource with filter for AMR * Add test for generic listResourceWithFilter * Update listResourceWithFilter to use revision instead of id * Update generic tests to use revision instead of id * Fix linting --------- Co-authored-by: Zac Bergquist <[email protected]> Co-authored-by: Roman Tkachenko <[email protected]>

…2087) * Add initial condition parser for access monitoring rules * Update integrations/access/accessrequest/app.go * Check previously unchecked error and minor refactor of AMR * Simplify check for applicable access monitoring rules * Refactor access monitoring rules plugin integration * Fix formating and move lock aquisition * Add methods for listing access monitoring rules with a filter * Add contains_any predicate expression func * Add in is_empty func to predicate expression * Lock AMR cache in plugins while getting initial rules * Add in check for access monitoring rule version * Update integrations/access/accessrequest/app.go * Update integrations/access/accessrequest/app.go * Move lock so it doesnt persist over api calls * Remove unused constant and add more context to logs * Appease linter * Update access monitoring rules tests to pass rule validation * Add in missing access monitoring rules list with filter code * Appease linter * Add back validation code for AMRs * Fix test plugin role and rename listaccessmonitoringrulewithfilter * Fix local test for AMR crud operations * Fix end range for listing rules * Fix unwrapping of resource153 event for monitoring rules * Refactor AMR cache init into helper function in plugin app * Add seperate response type for listAccessMonitoringRulesWithfilter * Add context to log for plugins failing to fetch recipients * Grab access monitoring rules cache under lock all at once * Add clarification for which fields are optional in listAMRfilter req * Update integrations/access/accessrequest/app.go * Update integrations/access/accessrequest/app.go * Add forEach to common recipient set * Move type check to after AMR event op switch * Move turn some default parser spec methods to funcs * Make some predicate func usable as methods as well * Add len func to common recipient sets * Add integration test for access monitoring rule and slack plugin * Fix error types and messages when handling AMRs * Use generic list resource with filter for AMR * Add test for generic listResourceWithFilter * Update listResourceWithFilter to use revision instead of id * Update generic tests to use revision instead of id * Fix linting --------- Co-authored-by: Zac Bergquist <[email protected]> Co-authored-by: Roman Tkachenko <[email protected]>

zmb3 reviewed Apr 19, 2024

View reviewed changes

EdwardDowling marked this pull request as ready for review April 22, 2024 16:15

github-actions bot requested review from marcoandredinis and tigrato April 22, 2024 16:16

github-actions bot added the size/md label Apr 22, 2024

tigrato reviewed Apr 23, 2024

View reviewed changes

EdwardDowling requested a review from tigrato April 30, 2024 14:26

r0mant reviewed May 1, 2024

View reviewed changes

zmb3 reviewed May 1, 2024

View reviewed changes

integrations/access/accessrequest/request_mapping.go Outdated Show resolved Hide resolved

zmb3 reviewed May 1, 2024

View reviewed changes

lib/expression/parser.go Show resolved Hide resolved

marcoandredinis removed their request for review May 2, 2024 10:22

r0mant reviewed May 8, 2024

View reviewed changes

EdwardDowling requested review from kimlisa and zmb3 May 13, 2024 16:55

r0mant approved these changes May 13, 2024

View reviewed changes

integrations/access/accessrequest/app.go Outdated Show resolved Hide resolved

tigrato reviewed May 15, 2024

View reviewed changes

zmb3 reviewed May 15, 2024

View reviewed changes

EdwardDowling requested review from tigrato, r0mant and zmb3 May 20, 2024 14:54

r0mant reviewed May 20, 2024

View reviewed changes

r0mant approved these changes May 23, 2024

View reviewed changes

tigrato approved these changes May 23, 2024

View reviewed changes

lib/services/local/generic/generic.go Show resolved Hide resolved

EdwardDowling added 6 commits May 24, 2024 15:48

Add len func to common recipient sets

951c422

Add integration test for access monitoring rule and slack plugin

38213f4

Fix error types and messages when handling AMRs

07e96a9

Use generic list resource with filter for AMR

351e582

Add test for generic listResourceWithFilter

0f60325

Update listResourceWithFilter to use revision instead of id

a048e9c

EdwardDowling force-pushed the edwarddowling/access-monitoring-rule-condition branch from 2a87f4b to a048e9c Compare May 24, 2024 15:36

EdwardDowling and others added 4 commits May 24, 2024 16:36

Merge branch 'master' into edwarddowling/access-monitoring-rule-condi…

42deae4

…tion

Merge branch 'master' into edwarddowling/access-monitoring-rule-condi…

e6929f8

…tion

Update generic tests to use revision instead of id

2a41f47

Merge branch 'master' into edwarddowling/access-monitoring-rule-condi…

eba59f8

…tion

EdwardDowling enabled auto-merge May 24, 2024 16:51

Fix linting

28b99eb

EdwardDowling added this pull request to the merge queue May 24, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 24, 2024

EdwardDowling added this pull request to the merge queue May 24, 2024

EdwardDowling removed this pull request from the merge queue due to a manual request May 24, 2024

Merge branch 'master' into edwarddowling/access-monitoring-rule-condi…

91fc3c6

…tion

EdwardDowling enabled auto-merge May 24, 2024 18:18

EdwardDowling added this pull request to the merge queue May 24, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 24, 2024

EdwardDowling added this pull request to the merge queue May 24, 2024

Merged via the queue into master with commit 94332f3 May 24, 2024
40 checks passed

EdwardDowling deleted the edwarddowling/access-monitoring-rule-condition branch May 24, 2024 19:03

EdwardDowling mentioned this pull request May 28, 2024

[v15] Add initial condition parser for access monitoring rules #42087

Merged

EdwardDowling mentioned this pull request Jun 12, 2024

Enable notification routing with Access Monitoring Rules for Access Plugins #42856

Open

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial condition parser for access monitoring rules #40659

Add initial condition parser for access monitoring rules #40659

EdwardDowling commented Apr 18, 2024 •

edited

Loading

github-actions bot commented Apr 22, 2024

tigrato Apr 23, 2024

EdwardDowling Apr 23, 2024

tigrato Apr 23, 2024 •

edited

Loading

EdwardDowling Apr 29, 2024

r0mant May 1, 2024

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 26, 2024

github-actions bot commented Apr 26, 2024

github-actions bot commented Apr 26, 2024

r0mant left a comment

r0mant May 1, 2024

r0mant left a comment

EdwardDowling commented May 9, 2024

r0mant left a comment

r0mant left a comment

Add initial condition parser for access monitoring rules #40659

Add initial condition parser for access monitoring rules #40659

Conversation

EdwardDowling commented Apr 18, 2024 • edited Loading

github-actions bot commented Apr 22, 2024

tigrato Apr 23, 2024

Choose a reason for hiding this comment

EdwardDowling Apr 23, 2024

Choose a reason for hiding this comment

tigrato Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

EdwardDowling Apr 29, 2024

Choose a reason for hiding this comment

r0mant May 1, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 26, 2024

github-actions bot commented Apr 26, 2024

github-actions bot commented Apr 26, 2024

r0mant left a comment

Choose a reason for hiding this comment

r0mant May 1, 2024

Choose a reason for hiding this comment

r0mant left a comment

Choose a reason for hiding this comment

EdwardDowling commented May 9, 2024

r0mant left a comment

Choose a reason for hiding this comment

r0mant left a comment

Choose a reason for hiding this comment

EdwardDowling commented Apr 18, 2024 •

edited

Loading

tigrato Apr 23, 2024 •

edited

Loading