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] added support for installing tag saved objects #114110

Merged
merged 5 commits into from
Oct 11, 2021

Conversation

juliaElastic
Copy link
Contributor

Summary

Solves #110660

Checklist

@juliaElastic juliaElastic requested a review from a team as a code owner October 6, 2021 14:16
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Oct 6, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

type: 'tags',
id: 'sample_tag',
});
expect(resTags.id).equal('sample_tag');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test is failing locally, investigating
debg Gettings saved object: {"type":"tags","id":"sample_tag"} └- ✖ fail: Fleet Endpoints EPM Endpoints installs and uninstalls all assets reinstalls all assets should have installed the kibana assets │ Error: Request failed with status code 404

Copy link
Contributor

Choose a reason for hiding this comment

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

This may be because of the type being tags instead of tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

might be, let me update to tag

@juliaElastic juliaElastic added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.0.0 labels Oct 6, 2021
@@ -53,6 +54,7 @@ export const AssetIcons: Record<KibanaAssetType, IconType> = {
lens: 'lensApp',
security_rule: 'securityApp',
ml_module: 'mlApp',
tag: 'tagApp',
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 followed the changes from here, though I think these constants are not yet used

https://github.com/elastic/kibana/pull/88189/files

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

This is looking great, just a couple notes:

  • Let's add the sample tag to the sample dashboard in the all_assets package so that we can test this integration. This can be done by adding { "id": "sample_tag", "type": "tag", "name": "tag-ref-sample_tag" } to the references array in the sample_dashboard.json file. We can then add an assertion to the api integration test that the dashboard has the associated tag.
  • We may also want to add some UI for linking to the associated tag on the "Assets" tab of the Integration Details view
    • image
    • This is handled by x-pack/plugins/fleet/public/applications/integrations/sections/epm/screens/detail/assets/assets_accordion.tsx
    • Unfortunately, it doesn't appear that the Tags UI supports linking to an individual tag or filtering the list by a URL query parameter.
    • @mostlyjason do you think we should:
      • Link to the overall tag page /app/management/kibana/tags
      • Include the tag in the asset list, but don't link anywhere
      • Don't add the tag to the asset list at all right now, file an issue for adding support for linking to an individual tag

@juliaElastic
Copy link
Contributor Author

@joshdover How did you bring up All Assets on UI?
I am running test server with this command
FLEET_PACKAGE_REGISTRY_PORT=12345 yarn test:ftr:server --config x-pack/test/fleet_api_integration/config.ts
And then hitting http://localhost:5620/app/integrations
However on Integrations page, I am getting this error:
Unable to initialize Fleet Default policy could not be added. system is not installed, add system to xpack.fleet.packages or remove it from system-1.

@joshdover
Copy link
Contributor

joshdover commented Oct 7, 2021

@joshdover How did you bring up All Assets on UI?

I did the following:

  • Start package registry manually:
     docker run -v $(pwd)/x-pack/test/fleet_api_integration/apis/fixtures/package_registry_config.yml:/package-registry/config.yml -v $(pwd)/x-pack/test/fleet_api_integration/apis/fixtures/test_packages/:/packages/test-packages -p 12345:8080 docker.elastic.co/package-registry/distribution@sha256:35cedaaa6adac547947321fa0c3b60a63eba153ba09524b9c1a21f1247a09bd2
    
  • Start Elasticsearch with normal configs needed for Fleet:
     yarn es snapshot --license trial -E xpack.security.authc.api_key.enabled=true -E http.host=0.0.0.0
    
  • Start Kibana with this flag:
     yarn start --xpack.fleet.registryUrl=http://localhost:12345
    
  • There was is one issue in the error_handling package that needs fixing (maybe commit it as part of this PR), the icons item needs a mimetype in x-pack/test/fleet_api_integration/apis/fixtures/test_packages/error_handling/0.2.0/manifest.yml. It should be:
     icons:
       - src: '/img/logo_overrides_64_color.svg'
         size: '16x16'
         type: 'image/svg+xml'
  • Go to localhost:5601/base-path/app/integrations/detail/all_assets-0.2.0/assets

@juliaElastic
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 103.4KB 104.7KB +1.2KB

History

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

cc @juliaElastic

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

LGTM.

For posterity's sake, Julia and I discussed deferring any changes to the Asset tab since there is not a direct page to link to and not all assets are currently represented in this page currently.

@juliaElastic juliaElastic merged commit 930fe96 into elastic:master Oct 11, 2021
@joshdover
Copy link
Contributor

Ah, I just saw the spec update for tags, and I wonder if we need to update the logic to only read files that begin with the package name as specified here: https://github.com/elastic/package-spec/blob/1e17043c2cf746e7c0168594e51824c903c00e35/versions/1/kibana/spec.yml#L94

@mtojek is that the correct behavior? To only accept tags that are named system-*.json for example?

@juliaElastic juliaElastic deleted the tags branch October 11, 2021 09:01
@criamico
Copy link
Contributor

Adding a comment here as I didn't have the time to approve before the merge, I tested the branch locally and followed @joshdover steps and it worked fine for me.

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Oct 11, 2021
* added tag saved objects to assets

* fixed review comments

* added translation to constants

* added missing icon type

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

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

@juliaElastic
Copy link
Contributor Author

@joshdover how would that look like? a modified version of installKibanaSavedObjects to filter out assets that are not adhering to the file naming in the spec?

@mtojek
Copy link
Contributor

mtojek commented Oct 11, 2021

@mtojek is that the correct behavior? To only accept tags that are named system-*.json for example?

@joshdover I can explain why there is the PACKAGE_NAME prefix. Two different packages install their assets and it may happen that they both refer to the same "magic-asset-1". In this case the first installed one will be overridden with the other one, so we want to prevent it using package name prefixes.

@juliaElastic
Copy link
Contributor Author

@joshdover do we have to do anything here or can we close the issue?
I observed that all other assets have the same PACKAGE_NAME prefix in schema (dashboard, visualisation), but they don't have any special logic when installing assets.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 13, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

3 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@joshdover
Copy link
Contributor

joshdover commented Oct 18, 2021

@joshdover do we have to do anything here or can we close the issue?
I observed that all other assets have the same PACKAGE_NAME prefix in schema (dashboard, visualisation), but they don't have any special logic when installing assets.

Can you file a new issue to update this for all assets to ensure that packages do not step on one another? For now, we can rely on the package spec to enforce this for us.

@juliaElastic
Copy link
Contributor Author

@joshdover raised this: #115386

kibanamachine added a commit that referenced this pull request Oct 18, 2021
#114446)

* [Fleet] added support for installing tag saved objects (#114110)

* added tag saved objects to assets

* fixed review comments

* added translation to constants

* added missing icon type

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

* fixed backport

Co-authored-by: juliaElastic <[email protected]>
Co-authored-by: Julia Bardi <[email protected]>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants