-
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
[Cloud Security] Metering integration tests #187219
Conversation
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud 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.
Need a bit more time for the full review (though don't be blocked if someone else from the team gets to it first)
One question - have you checked the discussion about the MSW usage in #184555 ? Even though these are different test suite, it would be good to have a consistent approach to the MSW usage in our suits across the board
...n/test_suites/security/cloud_security_posture/serverless_metering/cloud_security_metering.ts
Outdated
Show resolved
Hide resolved
...n/test_suites/security/cloud_security_posture/serverless_metering/cloud_security_metering.ts
Outdated
Show resolved
Hide resolved
...n/test_suites/security/cloud_security_posture/serverless_metering/cloud_security_metering.ts
Show resolved
Hide resolved
Yes, those are different use cases, in #184555 it mocks responses and does not create a real server, here, since the task manager is running on another node worker (there is one worker for the test and another for the server) it cannot intercept the requests. That's the reason I used MSW middleware. |
💔 Build FailedFailed CI StepsHistoryTo update your PR or re-run it, just comment with: |
...plugins/security_solution_serverless/server/cloud_security/defend_for_containers_metering.ts
Show resolved
Hide resolved
...n/test_suites/security/cloud_security_posture/serverless_metering/cloud_security_metering.ts
Show resolved
Hide resolved
return fetch(url, { | ||
method: 'post', | ||
body: JSON.stringify(records), | ||
headers: { 'Content-Type': 'application/json' }, | ||
agent, | ||
agent: isHttps ? agent : undefined, // Conditionally add agent if URL is HTTPS for supporting integration 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.
cc: @joeypoon
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 am curious why we used fetch instead of the core HTTP service?
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 guess the reason is that the agent
attribute is not supported in the core fetch. @joeypoon, please correct me if I'm wrong.
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 have not had the opportunity to develop any large FTRs but this looks clean to me. I'll approve knowing that @maxcold 's concerns about MKI test will be resolved.
return fetch(url, { | ||
method: 'post', | ||
body: JSON.stringify(records), | ||
headers: { 'Content-Type': 'application/json' }, | ||
agent, | ||
agent: isHttps ? agent : undefined, // Conditionally add agent if URL is HTTPS for supporting integration 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.
I am curious why we used fetch instead of the core HTTP service?
) { | ||
const version = '1.2.5'; | ||
|
||
const { body: postPackageResponse } = |
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.
can we move parts which are same to own variables ? eg. the object inside send
looks pretty much the same in both code branches, probably can be extracted
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.
Nice catch, done.
@@ -76,7 +76,8 @@ export const getUsageRecords = async ( | |||
{ | |||
range: { | |||
'event.ingested': { | |||
gt: searchFrom.toISOString(), | |||
// gt: searchFrom.toISOString(), |
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'd rather add the tech debt ticket to the comment, than leaving the old code commented out
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 solved it in #186777, so I remove the comment, and will merge the other PR first.
⏳ Build in-progress
History
|
## Summary Assigns ownership of new dev dep `@mswjs/http-middleware` to cloud security team (`kibana-cloud-security-posture`). Dependency was addded in #187219
## Summary Assigns ownership of new dev dep `@mswjs/http-middleware` to cloud security team (`kibana-cloud-security-posture`). Dependency was addded in elastic#187219 (cherry picked from commit c426b06)
# Backport This will backport the following commits from `main` to `8.x`: - [Adds @mswjs/http-middleware to renovate.json (#193257)](#193257) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Jeramy Soucy","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-18T13:44:53Z","message":"Adds @mswjs/http-middleware to renovate.json (#193257)\n\n## Summary\r\n\r\nAssigns ownership of new dev dep `@mswjs/http-middleware` to cloud\r\nsecurity team (`kibana-cloud-security-posture`).\r\n\r\nDependency was addded in https://github.com/elastic/kibana/pull/187219","sha":"c426b06aa442935d02cf171da66212ddce5e5ce9","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["chore","Team:Security","release_note:skip","v9.0.0","v8.16.0","backport:version"],"title":"Adds @mswjs/http-middleware to renovate.json","number":193257,"url":"https://github.com/elastic/kibana/pull/193257","mergeCommit":{"message":"Adds @mswjs/http-middleware to renovate.json (#193257)\n\n## Summary\r\n\r\nAssigns ownership of new dev dep `@mswjs/http-middleware` to cloud\r\nsecurity team (`kibana-cloud-security-posture`).\r\n\r\nDependency was addded in https://github.com/elastic/kibana/pull/187219","sha":"c426b06aa442935d02cf171da66212ddce5e5ce9"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193257","number":193257,"mergeCommit":{"message":"Adds @mswjs/http-middleware to renovate.json (#193257)\n\n## Summary\r\n\r\nAssigns ownership of new dev dep `@mswjs/http-middleware` to cloud\r\nsecurity team (`kibana-cloud-security-posture`).\r\n\r\nDependency was addded in https://github.com/elastic/kibana/pull/187219","sha":"c426b06aa442935d02cf171da66212ddce5e5ce9"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Jeramy Soucy <[email protected]>
solves:
Using MSW middleware to mock the usage API service and verify that metering records are collected and sent as expected to the usage service from the background metering task.