-
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][Endpoint] Add serverless Security project users/roles to cypress e2e test setup #167446
[Security Solution][Endpoint] Add serverless Security project users/roles to cypress e2e test setup #167446
Conversation
…e-serverless-tests # Conflicts: # x-pack/plugins/security_solution/scripts/endpoint/common/roles_users/serverless/es_serverless_resources/roles.yml
…e-serverless-tests
…e-serverless-tests
…e-serverless-tests
…e-serverless-tests
@@ -0,0 +1,673 @@ | |||
/* |
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.
FYI: I will have a follow up PR to refactor the existing endpint_agent_runner
utility to use this module (will also look for import
's for private modules of endpoint_agent_runner
). For now, most of the code here is a duplicate.
Pinging @elastic/security-defend-workflows (Team:Defend Workflows) |
…e-serverless-tests
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.
Hey, I've gone through the code, left a few suggestions and questions (just to help me understand, thanks in advance 👍 ).
All in all, great job as usual, thanks for doing this 👍
export const cli = async () => { | ||
return run( | ||
async (cliContext: RunContext) => { | ||
const username = cliContext.flags.username as string; |
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.
Why do we need to cast this like that?
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.
username is not set as a string
in .flags
ref:
[key: string]: undefined | boolean | string | string[]; |
const isServerlessEs = (await tmpEsClient.info()).version.build_flavor === 'serverless'; | ||
await waitForKibana(tmpKbnClient); | ||
|
||
// const isServerlessEs = (await tmpEsClient.info()).version.build_flavor === '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.
redundant?
if (isServerless) { | ||
log.info(`Waiting for server to register with Elasticsearch`); | ||
|
||
await waitForFleetServerToRegisterWithElasticsearch(kbnClient, hostname, 120000); |
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.
would it be worth to add a log.verbose after this? as with hostToEnroll below?
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 is nothing here to log. The one below logs the Agent record in log.verbose()
kbnClient: KbnClient; | ||
logger: ToolingLog; | ||
policyId: string; | ||
serviceToken: string; |
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.
maybe worth to specify this as string | undefined , passing an empty string may be confusing. what do you think?
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.
same with policyId I guess ?
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.
These can't be undefined
for managed fleet server. I'll make them optional and will add runtime validation.
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.
Yes, what I meant is it's either a string for manager fs or really undefined right?
But what you just did is also great, so thanks!
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.
correct
export const startFleetServer = async ({ | ||
kbnClient, | ||
logger, | ||
policy, |
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.
this policy, coming from cliConfig.flags.policy - is this supposed to be supported only if used as a CLI and not CI (eg. cypress?). Which means that if provided, we do not call getOrCreateFleetServerAgentPolicyId
?
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.
correct - if policy
(Fleet agent policy UUID) is provided, then we don't create one automatically if one does not yet exist. It could be used from CI or CLI - it does not matter. It just provides the consumer the ability to define the specific agent policy ID to use in starting the server
- version adjusted to [latest] from [${agentVersion}]`); | ||
|
||
agentVersion = 'latest'; | ||
await maybeCreateDockerNetwork(log); |
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.
Is there any case where we need to additionally create the network (in serverless mode)? I understand that network is being already created with dockerized ES.
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.
Not sure 🤷 . I think there are some additional discussions in @patrykkopycinski 's PR about creating additional networks for Agent (hosts).
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.
I just wanted to make it consistent to make sure all docker containers are running on the same docker network
const fleetServerUrl = `https://${localhostRealIp}:${port}`; | ||
const esURL = new URL(await getFleetElasticsearchOutputHost(kbnClient)); | ||
const containerName = `dev-fleet-server.${port}`; | ||
const hostname = `dev-fleet-server.${port}.${Math.random().toString(32).substring(2, 6)}`; |
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.
Is this necessary?
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.
Yes. If we don't, the host name in fleet is duplicated when we run it multiple times locally. This ensure that the host name is unique. It was also causing an issue when calling waitForHostToEnroll()
|
||
// Only fetch/create a fleet-server policy | ||
if (!policyId && !isServerless) { | ||
policyId = await getOrCreateFleetServerAgentPolicyId(kbnClient, logger); |
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.
I think we can set policyId
the same way as serviceToken
below, it might get more readable, what do you think?
await kbnClient | ||
.request<GetOneOutputResponse>({ | ||
method: 'PUT', | ||
headers: { 'elastic-api-version': '2023-10-31' }, |
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.
I see you use API_VERSIONS.public.v1
is it possible to use it also here?
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.
That const
never made sense to me and I think will lead to issues. I was never really sure why a const was used. Version number will diverge at the api level (is my understanding) not the release level across all APIs
}, | ||
query: { | ||
perPage: 100, | ||
'elastic-api-version': '2023-10-31', |
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.
and here
…e-serverless-tests
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.
packages/kbn-test
…e-serverless-tests # Conflicts: # packages/kbn-test/src/es/test_es_cluster.ts # packages/kbn-test/src/functional_test_runner/lib/config/schema.ts # packages/kbn-test/src/functional_tests/lib/run_elasticsearch.ts # x-pack/plugins/security_solution/scripts/endpoint/common/stack_services.ts # x-pack/test/defend_workflows_cypress/serverless_config.ts
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! Thanks!
method: 'GET', | ||
path: PACKAGE_POLICY_API_ROUTES.LIST_PATTERN, | ||
headers: { | ||
'elastic-api-version': '2023-10-31', |
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.
Should we use the API_VERSIONS.public.v1
from fleet plugin?
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.
See my prior comment to @tomsonpl 's (same comment). I never really understood why we are using const
's for API version
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 the changes. 🔥 I've not tested this locally but will take it for a spin later.
'.lists*', | ||
'.items*', |
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.
I believe these should be .lists-*
and .items-*
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.
I'm going to leave it as is, since it matches whats defined in Project Controller (cc/ @dhurley14 )
{ | ||
names: [ | ||
'names:', | ||
'.alerts-security*', |
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.
This should be .alerts-security.alerts-*
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.
same comment as above
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
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.
Thank you @paul-tavares 🙇
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.
kbn-test
changes LGTM
Summary
Goal of this PR is to re-enable the serverless tests that require the login credentials for users that have the pre-defined roles from serverless assigned to them.
@kbn/test
changesesServerlessOptions
to FTR config. Currently allows forresources
to be definedresources
overrides were introduced in this PRSecurity Solution Plugin
esServerlessOptions
to the Defend Workflows cypress configurationslogin()
task to beendpoint_operations_analyst
endpoint_operations_analyst
role was also updated to match the definition used for serverless.fleet_server_services
cli module with reusable methods for working with fleet server, including genericstartFleetServer()
methodnode x-pack/plugins/security_solution/scripts/endpoint/start_fleet_server.js
Serverless specific tests now passing
Options available with new
start_fleet_server.js
cli script:Output of the
start_fleet_server.js
script