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

[Fleet] Allow snake cased Kibana assets #77515

Merged
merged 24 commits into from
Nov 4, 2020

Conversation

crob611
Copy link
Contributor

@crob611 crob611 commented Sep 15, 2020

Summary

Closes #71633

This fixes installation of Kibana assets like index-pattern whose asset path in a package will be snake cased index_pattern.

We will now also maintain a kibanaAssetType -> savedObjectType mapping. This is to make sure that any assets in an assets/kibana folder have the appropriate saved object type, since we can't just check against kibanaAssetType anymore.

Finally, a mapping of kibanaAssetType -> installFunction. When we getKibanaAssets now, we get a map of kibanaAssetType -> Array<archive>, so we can act on the kibanaAssetType. Currently, everything is just a saved object, but this is just setting up for the future in case there comes a time when an asset type is not just a simple saved object install, we can introduce new installer methods relatively easily.

I don't think there are any existing packages with index-pattern folders, but if there are we will need to make sure that they get updated at the appropriate time. Should we include a fallback to catch those legacy cases?

@crob611 crob611 added v7.10.0 Team:Fleet Team label for Observability Data Collection Fleet team labels Sep 15, 2020
@crob611 crob611 requested a review from a team September 15, 2020 16:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@crob611
Copy link
Contributor Author

crob611 commented Sep 15, 2020

@elasticmachine merge upstream

@crob611 crob611 added the release_note:skip Skip the PR/issue when compiling release notes label Sep 15, 2020
@skh
Copy link
Contributor

skh commented Sep 16, 2020

Thanks for tackling this!

We generate the index patterns logs-* and metrics-* from the packages' field definitions on the fly whenever a package is installed or removed.

If a package now brings an index pattern with the same name, we run into conflicts. I think we should check for these reserved names, and either silently skip them or return an error.

@neptunian @ruflin what do you think?

@ruflin
Copy link
Member

ruflin commented Sep 16, 2020

There are 2 parts here:

  • A package is not allowed to contain a Kibana index pattern as part of a dataset
  • If it contains an index pattern on the global level, ++ on refusing it if it is for a reserved pattern. I would not install the package -> error.

It might even possible that we get this one day in the package-spec to disallow it. @ycombinator

@crob611
Copy link
Contributor Author

crob611 commented Sep 16, 2020

Ok I pushed a new commit that will just not install index-patterns if they are one of the Fleet reserved index patterns.

Is there a way to completely bail in the middle of an install and roll back what has already been done? Throwing doesn't appear to work, which is good because the package will still install if there is an error, so not sure how to bail then.

@jfsiii
Copy link
Contributor

jfsiii commented Sep 17, 2020

@crob611 Thanks for the PR! I'm still reviewing but a 👍 overall. The PR description and code are both clear.

@jfsiii
Copy link
Contributor

jfsiii commented Sep 17, 2020

@elasticmachine merge upstream

Comment on lines 258 to 261
export type KibanaAssetReference = Pick<SavedObjectReference, 'id'> & {
type: KibanaAssetType;
};
export type KibanaAssetReference = Pick<SavedObjectReference, 'id' | 'type'>;
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, SavedObjectReference['type'] is a string vs a KibanaAssetType. I'd like to keep as-is if we can. Was this causing an issue?

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 KibanaAssetType is now the type that matches file paths, which do not match to saved object types (index_pattern vs index-pattern).

Maybe we bring the KibanaSavedObjectsType over into this file from the kibana/install file and use that type instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

@crob611 I'll re-read but that seems OK. We definitely want a name and type more refined than string.

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 OK just pushed a commit that adds a "KibanaSavedObjectType" and uses that type for KibanaAssetReference.


const resolvedAssets = await Promise.all(assetArrays);

const result = {} as Record<KibanaAssetType, ArchiveAsset[]>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind moving Record<KibanaAssetType, ArchiveAsset[]> to the function return type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the remove the as Record..., leaving either const result = {} or

Suggested change
const result = {} as Record<KibanaAssetType, ArchiveAsset[]>;
const result: Record<KibanaAssetType, ArchiveAsset[]> = {};

@jfsiii
Copy link
Contributor

jfsiii commented Sep 18, 2020

cc @neptunian for 👀

@crob611
Copy link
Contributor Author

crob611 commented Sep 21, 2020

@elasticmachine merge upstream

@crob611
Copy link
Contributor Author

crob611 commented Sep 22, 2020

@elasticmachine merge upstream

@crob611
Copy link
Contributor Author

crob611 commented Oct 8, 2020

@elasticmachine merge upstream

@jfsiii
Copy link
Contributor

jfsiii commented Nov 3, 2020

@neptunian @skh @ruflin any comments on this? I think we want it and I'd like to merge it soon to avoid conflicts with our pending changes. If you are 👍 on it, I'll merge.

@jfsiii
Copy link
Contributor

jfsiii commented Nov 3, 2020

@elasticmachine merge upstream

kibanamachine and others added 3 commits November 3, 2020 15:03
Use new `dataTypes` const which replaced `DataType` enum
Remove unused `indexPatternTypes` from outer scope
@jfsiii
Copy link
Contributor

jfsiii commented Nov 3, 2020

@crob611 I merged upstream master and fixed/caused some issues. I think it's good now, but I'll keep an eye on it

John Schulz added 3 commits November 3, 2020 16:28
fix (?) bad updates from before where new/correct value was used but result wasn't exported
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
ingestManager 1.2MB 1.2MB -4.0B

page load bundle size

id before after diff
ingestManager 386.1KB 386.7KB +609.0B

History

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

@crob611
Copy link
Contributor Author

crob611 commented Nov 4, 2020

@jfsiii Looks like this is good to go now. You good with me merging and backporting?

@jfsiii
Copy link
Contributor

jfsiii commented Nov 4, 2020

@crob611 I think so. It passes all tests and has been open for comment for some time.

Let's merge before the changes @skh, @neptunian and I have begun make this even more challenging to merge.

@crob611 crob611 merged commit 1cd477a into elastic:master Nov 4, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 5, 2020
* master: (127 commits)
  [ILM] Fix breadcrumbs (elastic#82594)
  [UX]Swap env filter with percentile (elastic#82246)
  Add platform's missing READMEs (elastic#82268)
  [Discover] Adding uiMetric to track Visualize link click (elastic#82344)
  [Search] Add used index pattern name to the search agg error field (elastic#82604)
  improve client-side SO client get pooling (elastic#82603)
  [Security Solution] Unskips Overview tests (elastic#82459)
  Embeddables/migrations (elastic#82296)
  [Enterprise Search] Refactor product server route registrations to their own files/folders (elastic#82663)
  Moving reinstall function outside of promise.all (elastic#82672)
  Load choropleth layer correctly (elastic#82628)
  Master  backport elastic#81233 (elastic#82642)
  [Fleet] Allow snake cased Kibana assets (elastic#77515)
  Reduce saved objects authorization checks (elastic#82204)
  [data.search] Add request handler context and asScoped pattern (elastic#80775)
  [ML] Fixes formatting of fields in index data visualizer (elastic#82593)
  Usage collector readme (elastic#82548)
  [Lens] Visualization validation and better error messages (elastic#81439)
  [ML] Add annotation markers to time series brush area to indicate annotations exist outside of selected range (elastic#81490)
  chore(NA): install microdnf in UBI docker build only (elastic#82611)
  ...
crob611 pushed a commit to crob611/kibana that referenced this pull request Nov 5, 2020
crob611 pushed a commit that referenced this pull request Nov 5, 2020
crob611 pushed a commit to crob611/kibana that referenced this pull request Nov 5, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 5, 2020
* master:
  Revert "[Fleet] Allow snake cased Kibana assets (elastic#77515)" (elastic#82706)
jfsiii pushed a commit that referenced this pull request Nov 5, 2020
* Revert "Revert "[Fleet] Allow snake cased Kibana assets (#77515)" (#82706)"

This reverts commit bc05e79.

* Rename test index pattern
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 77515 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 6, 2020
crob611 pushed a commit to crob611/kibana that referenced this pull request Nov 6, 2020
* Revert "Revert "[Fleet] Allow snake cased Kibana assets (elastic#77515)" (elastic#82706)"

This reverts commit bc05e79.

* Rename test index pattern
crob611 pushed a commit that referenced this pull request Nov 9, 2020
* Revert "Revert "[Fleet] Allow snake cased Kibana assets (#77515)" (#82706)"

This reverts commit bc05e79.

* Rename test index pattern

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 77515 or prevent reminders by adding the backport:skip label.

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 77515 or prevent reminders by adding the backport:skip label.

@crob611 crob611 added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Ingest Manager] Ensure installation of index-pattern assets from packages works
8 participants