-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security Solution] Changes default role for serverless from admin to platform_engineer #183608
[Security Solution] Changes default role for serverless from admin to platform_engineer #183608
Conversation
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations) |
Pinging @elastic/security-threat-hunting-explore (Team:Threat Hunting:Explore) |
Pinging @elastic/security-detection-engine (Team:Detection Engine) |
Pinging @elastic/security-entity-analytics (Team:Entity Analytics) |
.../e2e/detection_response/rule_management/maintenance_windows/maintenance_window_callout.cy.ts
Outdated
Show resolved
Hide resolved
…sponse/rule_management/maintenance_windows/maintenance_window_callout.cy.ts Co-authored-by: Georgii Gorbachev <[email protected]>
@@ -49,7 +49,7 @@ describe('risk tab', { tags: ['@ess', '@serverless'] }, () => { | |||
}); | |||
}); | |||
|
|||
describe('with new risk score', () => { |
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.
Great, thank you for enabling this one for entity analytics 👍
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.
Entity Analytics changes LGTM
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.
LGTM
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.
Thanks @MadameSheema! Rule Management changes LGTM!
I am curious about waitForTheRuleToBeExecuted()
. Why do tests where it is not removed still pass? Is it because these tests are not ran with platform_engineer
user?
@nikitaindik the The other tests where it works is because the user has access to all the indexes. Anyway, before merging I'll double check those indexes :) |
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.
Investigations area LGTM 🚀
@@ -61,13 +61,18 @@ import { getNewRule } from '../../../../objects/rule'; | |||
import { ALERTS_URL } from '../../../../urls/navigation'; | |||
import { waitForAlertsToPopulate } from '../../../../tasks/create_new_rule'; | |||
import { TOASTER } from '../../../../screens/alerts_detection_rules'; | |||
import { ELASTICSEARCH_USERNAME, IS_SERVERLESS } from '../../../../env_var_names_constants'; | |||
|
|||
// We need to use the 'soc_manager' role in order to have the 'Respond' action displayed in serverless |
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.
Thanks for making this change. Is there is list of serverless roles somewhere?
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.
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.
Detection engine changes look good to me!
I noticed that waitForAlertsToPopulate
doesn't care about which alerts are populated, so there's some small risk of acting on alerts that we didn't expect and it would be nice to e.g. allow a ruleId
to be passed to that helper. However, the waitFor Rule
helper wasn't really addressing this issue in the first place, so I'm happy to see an unnecessary step removed from these tests 👍
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
Summary
Addresses: #184140
In this PR we are addressing the short-term solution of #184135 where we are moving from using for serverless
admin
role for MKI environments andsystem_indices_superuser
for stateless environments toplatform_engineer
.To make the above change happen, some adjustments have been introduced to make the tests work:
Rename of custom indexes
The
admin
orsystem_indices_superuser
has access to any index on the system, but built-in roles in security serverless projects just have access to some or all the security default indexes, so they cannot access custom indexes.To make the tests work with the
platform_engineer
all the custom indexes that we use for testing purposes have been renamed to match ours.waitForTheRuleToBeExecuted
removed from some testswaitForTheRuleToBeExecuted
waits for the rule status to besucceeded
but theplatform_engineer
does not have access to all the indexes, so instead the final status result will bewarning
so this method will timeout.Originally that method was created in order to make sure that some alerts are going to be displayed but, after it, we are calling
waitForAlertsToPopulate
that makes sure that we have alerts.Some tests have been skipped from serverless executions