Skip to content
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] Adding tests for endpoint package pipelines #73703

Merged
merged 7 commits into from
Jul 30, 2020

Conversation

jonathan-buttner
Copy link
Contributor

This PR adds tests for the endpoint package's pipelines.

image

The docker image corresponds to the endpoint package v0.12.0 release in the staging environment here: elastic/package-storage@80e93ad

@jonathan-buttner jonathan-buttner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Data Visibility Team managing the endpoint resolver Feature:Endpoint Elastic Endpoint feature v7.10.0 labels Jul 29, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-data-visibility-team (Team:Endpoint Data Visibility)

@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

@@ -1,2 +1,4 @@
package_paths:
- /packages/production
- /packages/staging
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The package is currently in the staging environment which is why we need this. I added the snapshot just in case we ever need to grab one from there.

@@ -38,26 +36,10 @@ export default function resolverAPIIntegrationTests({ getService }: FtrProviderC
});

it('excludes events that have an empty entity_id field', async () => {
// first lets get the _id of the document using the parent.process.entity_id
// then we'll use the API to search for that specific document
const res = await es.search<SearchResponse<Event>>({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean up based on Rob's previous comments. The _id is now saved from the bulk response.

@@ -47,7 +47,6 @@ export default function ({ getService }: FtrProviderContext) {
'/api/ingest_manager/epm/packages/filetest/0.1.0/kibana/visualization/sample_visualization.json'
)
.set('kbn-xsrf', 'xxx')
.expect('Content-Type', 'text/plain; charset=utf-8')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfsiii @skh @ruflin I'm removing these checks because it seems that they change based on the base docker container we use or between releases of the package-registry.

If I hit these apis chrome returns application/json:
https://epr.elastic.co/package/aws/0.1.1/kibana/dashboard/0eb5a6a0-694f-11ea-b0ac-95d4ecb1fecd.json

Same for:
https://epr-staging.ea-web.elastic.dev/package/aws/0.1.1/kibana/dashboard/0eb5a6a0-694f-11ea-b0ac-95d4ecb1fecd.json

And via the ingest manager api it's application/json too:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping the tests based on Sonja's guidance.

@@ -40,7 +40,8 @@ export default function ({ getService }: FtrProviderContext) {
}
});

it('fetches a .json kibana visualization file', async function () {
// skipping because the content-type returned by the registry is application/json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure to understand but it looks to me that the issue is fixed and we should probably update the test instead no? elastic/package-registry#590

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but we can also do it in a follow-up PR and get @jonathan-buttner unblocked on this one. Either way is fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @skh and @nchaulet I'll switch them over to application/json 👍

Copy link
Contributor

@skh skh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for Ingest Manager changes.

@jonathan-buttner
Copy link
Contributor Author

@elasticmachine merge upstream

const es = getService('es');
const generator = new EndpointDocGenerator('data');

const searchForID = async <T>(id: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ Are you marking this async just so you know it returns a Promise? I don't see it awaiting anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think it's discouraged to do return await ...
https://eslint.org/docs/rules/no-return-await

es.search() is an async function and I want to force the caller of searchForID to await on the call.

networkIndexData.eventsInfo[0]._id
);
expect(eventWithBothIPs.body.hits.hits[0]._source.source.geo?.country_name).to.be(
'United States'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ These always going to be in English, regardless of what locale it's running under?

Copy link
Contributor Author

@jonathan-buttner jonathan-buttner Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good point. I don't think they'd change much but for some reason the 8.8.8.8 ip address got switched to a different country this would fail. I'll switch it to check that the name isn't undefined 👍

Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jonathan-buttner jonathan-buttner merged commit 70d4eac into elastic:master Jul 30, 2020
@jonathan-buttner jonathan-buttner deleted the pipeline-tests branch July 30, 2020 18:43
jonathan-buttner added a commit to jonathan-buttner/kibana that referenced this pull request Jul 30, 2020
…tic#73703)

* Adding tests for endpoint package pipelines

* Removing content type check on types that can change based on docker image version

* Skipping ingest tests instead of remove expect

* Switching ingest tests over to use application/json

* Removing country names

Co-authored-by: Elastic Machine <[email protected]>
jonathan-buttner added a commit that referenced this pull request Jul 30, 2020
…) (#73871)

* Adding tests for endpoint package pipelines

* Removing content type check on types that can change based on docker image version

* Skipping ingest tests instead of remove expect

* Switching ingest tests over to use application/json

* Removing country names

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Data Visibility Team managing the endpoint resolver v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants