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

[index patterns] Add pattern validation method to index patterns fetcher #90170

Merged
merged 8 commits into from
Feb 5, 2021

Conversation

stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Feb 3, 2021

Summary

The IndexPatternsFetcher.getFieldsForWildcard method cannot handle an index pattern that contains patterns that do not match indices. The Security Solutions team needs to define a pattern list that will work whether or not the user has the matching indices for each pattern on the list.

For example, given the pattern auditbeat-*,fakebeat-*:

Screen Shot 2021-02-03 at 7 21 22 AM

Adding a validation check in IndexPatternsFetcher.getFieldsForWildcard for each pattern and only querying patterns that match indices fixes the issue:
Screen Shot 2021-02-03 at 7 21 34 AM

Checklist

For maintainers

@stephmilovic stephmilovic added Feature:Data Views Data Views code and UI - index patterns before 8.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Feb 3, 2021
@stephmilovic stephmilovic requested a review from mattkime February 3, 2021 14:25
@stephmilovic stephmilovic self-assigned this Feb 3, 2021
@stephmilovic stephmilovic requested a review from a team as a code owner February 3, 2021 14:25
)
);
return result.reduce(
(acc: string[], { body: indexLookup }, patternListIndex) =>
Copy link
Member

Choose a reason for hiding this comment

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

are we sure this is exactly what elasticsearch would do if we would request fieldcaps for a comma seperated list of patterns ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It's actually typed in a couple x-pack plugins. Would you like me to add a type here?

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

const fieldCapsResponse = await getFieldCapabilities(
this.elasticsearchClient,
pattern,
patternListActive.length > 0 ? patternListActive : patternList,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if none of the patterns are active, pass the original list to get an error:
Screen Shot 2021-02-04 at 7 32 20 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add this as a comment to the code. It definitely makes sense but its not obvious why this is being done.

async validatePatternListActive(patternList: string[]) {
const result = await Promise.all(
patternList.map((pattern) =>
this.elasticsearchClient.count({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed from the _resolve/indexapi to the count api as read only users got 403'd on the resolve. count works well: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-count.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Its annoying that the resolve endpoint works this way but this is a good choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is annoying. i would not have caught it if it weren't for that ML test!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also tried ElasticsearchClient.indices.exists and it appears read-only users get 403'd on those indices methods as well. I'm not sure if this has been taken into account for testing anywhere that uses the getFieldsForTimePattern which eventually calls ElasticsearchClient.indices.getAlias: https://github.com/elastic/kibana/blob/master/src/plugins/data/server/index_patterns/fetcher/lib/es_api.ts#L42

Copy link
Contributor

Choose a reason for hiding this comment

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

That code needs to be removed, time pattern index patterns were dropped in 7.x.

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

Just one request otherwise looks great!

const fieldCapsResponse = await getFieldCapabilities(
this.elasticsearchClient,
pattern,
patternListActive.length > 0 ? patternListActive : patternList,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add this as a comment to the code. It definitely makes sense but its not obvious why this is being done.

async validatePatternListActive(patternList: string[]) {
const result = await Promise.all(
patternList.map((pattern) =>
this.elasticsearchClient.count({
Copy link
Contributor

Choose a reason for hiding this comment

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

Its annoying that the resolve endpoint works this way but this is a good choice.

})
)
.map((p) =>
p.catch((e) => {
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 count api will return count: 0 for a pattern like nothingbeat-* but will throw the index_not_found_exception on non-wildcard, like nothingbeat-exact

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@stephmilovic stephmilovic merged commit fc516ba into elastic:master Feb 5, 2021
stephmilovic added a commit to stephmilovic/kibana that referenced this pull request Feb 5, 2021
stephmilovic added a commit that referenced this pull request Feb 8, 2021
…her (#90170) (#90512)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants