[Fleet] Tagging of integration assets by juliaElastic · Pull Request #137184 · elastic/kibana

juliaElastic · 2022-07-26T13:38:59Z

Summary

Solves #123904

Added logic to create Managed and integration name tags and reference them with installed package assets.

To verify:

Add integration with assets e.g. System, Apache
Go to Tags view and verify that the tags are created and linked to the assets of each package. The Managed tag should appear once only.
On uninstalling and adding the integration again, the integration name tag should not be duplicated.

Open questions / issues:

Should we add a description to the tags?
The white color of tags look good on dark mode, though not the best on light mode, should we change it?

Pending:

Unit and functional tests

Checklist

Unit or functional tests were updated or added to match the most common scenarios

nchaulet · 2022-07-26T15:16:50Z

+  const managedTagName = 'Managed';
+  const tagColor = '#FFFFFF';


nit: it will be nice to move this to a constant

elasticmachine · 2022-07-26T15:16:55Z

Pinging @elastic/fleet (Team:Fleet)

nchaulet

One small questions about the robustness of the tagging client but overall looks good 🚀
also I think tagKibanaAssets will be a good candidate to unit tests and it will be a nice addition here.

kpollich · 2022-07-26T19:30:29Z

+const TAG_COLOR = '#FFFFFF';
+const MANAGED_TAG_NAME = 'Managed';
+
+export async function tagKibanaAssets({


Do you think it'd be possible to wrap this in an APM span and track how it impacts installation performance for packages?

juliaElastic · 2022-07-27T07:15:48Z

        httpCode: 200,
        expectResponse: ({ body }) => {
-          expect(body).to.eql({
+          expect(body.tags.slice(0, 1)).to.eql({


had to touch this test because with the new logic of adding tags for installed package assets, 2 new tags were added, because Synthetics package is being installed on startup

actual: '{\n' + ' "tags": [\n' + ' {\n' + ' "color": "#FF00FF"\n' + ' "description": "Tag 1 in default space"\n' + ' "id": "default-space-tag-1"\n' + ' "name": "tag-1"\n' + ' }\n' + ' {\n' + ' "color": "#77CC11"\n' + ' "description": "Tag 2 in default space"\n' + ' "id": "default-space-tag-2"\n' + ' "name": "tag-2"\n' + ' }\n' + ' {\n' + ' "color": "#FFFFFF"\n' + ' "description": ""\n' + ' "id": "7833a370-0cfe-11ed-947f-438bf3d7f352"\n' + ' "name": "Elastic Synthetics"\n' + ' }\n' + ' {\n' + ' "color": "#FFFFFF"\n' + ' "description": ""\n' + ' "id": "77aa9d50-0cfe-11ed-947f-438bf3d7f352"\n' + ' "name": "Managed"\n' + ' }\n' + ' ]\n' + '}',

juliaElastic · 2022-07-27T07:16:15Z


        const displayedTags = await tagManagementPage.getDisplayedTagNames();
-        expect(displayedTags.length).to.be(3);
+        expect(displayedTags.length).to.be.greaterThan(2);


same reason, there are now 5 tags instead of 3

Error: expected 5 to equal 3

I thought we fixed a while ago the problem of fleet assets 'polluting' the test data for all the suites by not installing any assets during FTR tests. Why is this still necessary then?

I'll dig deeper, I'm not familiar that part of the code that installs synthetics package on startup, it would be great to exclude that from SOT tests.

Yea, especially given we have other tests (SO management mostly) that take as granted that the data injected via the test datasets are the only existing objects during the tests. We already encountered failures / flakiness because of these auto-installed packages in the past, and I would really love to avoid encountering that again. cc @joshdover given he was part of the discussions at that time (IIRC)

This is the only place I know of where this package gets installed automatically:

kibana/x-pack/plugins/synthetics/server/routes/synthetics_service/install_index_templates.ts

Lines 26 to 28 in 57f7a48

const installation = await server.fleet.packageService.asInternalUser.ensureInstalledPackage({

pkgName: 'synthetics',

});

I doubt that is getting called in this test though? As an aside, that endpoint should really be a POST and not a GET...

The installation of synthetics package comes from fleet setup here: https://github.com/elastic/kibana/blob/main/x-pack/plugins/fleet/server/services/setup.ts#L111

const autoUpdateablePackages = compact( await Promise.all( AUTO_UPDATE_PACKAGES.map((pkg) => isPackageInstalled({ savedObjectsClient: soClient, pkgName: pkg.name, }).then((installed) => (installed ? pkg : undefined)) ) ) ); packages = [ ...packages, ...autoUpdateablePackages.filter((pkg) => !preconfiguredPackageNames.has(pkg.name)), ]; logger.debug('Setting up initial Fleet packages'); const { nonFatalErrors: preconfiguredPackagesNonFatalErrors } = await ensurePreconfiguredPackagesAndPolicies( soClient, esClient, policies, packages, defaultOutput, defaultDownloadSource, DEFAULT_SPACE_ID );

I found a way to fix that by loading an empty kibana es archive like we do in fleet api tests.

await esArchiver.load('test/functional/fixtures/es_archiver/empty_kibana');

This is more a workaround that a solution (I'd like to just have fleet NOT install anything by default, or at least to have the option to do so for FTR tests), but I'm in PTO starting today so I won't be able to follow this PR, so it will have to do for now. I'd love you to open an issue to discuss about this problem though, if possible.

Sure, I can open an issue. I am not sure if there is a way to disable fleet plugin in SOT tests, I'll check.

@juliaElastic Your link didn't work for me. We shouldn't be installing any packages by default I think.

I found a workaround by setting an invalid registryUrl, this prevents installing the default packages. I didn't find a better way to disable fleet plugin from SOT tests so far.

juliaElastic · 2022-07-27T12:00:14Z

I checked the local kibana performance with APM, here is what I saw:

Adding System package policy took 15s, add tags took 2s
Adding Apache package policy took 14s, add tags took 2.5s

kpollich · 2022-07-27T12:31:18Z

Adding System package policy took 15s, add tags took 2s
Adding Apache package policy took 14s, add tags took 2.5s

This seems a little steep to add several seconds for each package policy. Maybe we should dig in a bit deeper here to see why this takes as long as it does?

pgayvallet · 2022-07-27T12:36:45Z

Maybe we should dig in a bit deeper here to see why this takes as long as it does?

The default refreshsetting for SO create operations is wait_for, and that's the value used by the tag client, so this is likely what is introducing this duration, given 1.5sec + 0.9sec are spent in PUT _create es call.

for each package policy

Unless I'm reading the APM chart wrong, it should only occur for the first installation (as the tags are created only once and the GET operations are negligible)?

juliaElastic · 2022-07-27T12:41:20Z

Maybe we should dig in a bit deeper here to see why this takes as long as it does?

The default refreshsetting for SO create operations is wait_for, and that's the value used by the tag client, so this is likely what is introducing this duration, given 1.5sec + 0.9sec are spent in PUT _create es call.

Yes, the majority of the time comes from the ES operation to create the tags.

for each package policy

Unless I'm reading the APM chart wrong, it should only occur for the first installation (as the tags are created only once and the GET operations are negligible)?

You are right, the tag creation happens once per package installation, subsequent package policy creations should not take this much time.
EDIT: created another package policy for system, it took 2.9s and the tagging logic was not invoked (as it is only called when installing a package)

I'll check on cloud pr deployment as well how long does it take.

EDIT: on cloud pr deployment I can't find the APM traces yet, overall installing system and apache packages take about 7.5-8s. Compared to staging (without the tagging) it takes 6.3-6.7s. So the difference is about 1s in cloud.

joshdover · 2022-07-27T13:14:12Z

The default refreshsetting for SO create operations is wait_for, and that's the value used by the tag client, so this is likely what is introducing this duration, given 1.5sec + 0.9sec are spent in PUT _create es call.

Can we allow fleet to disable the refresh here by adding a new parameter to the tag client?

joshdover · 2022-07-27T13:15:07Z

Unless I'm reading the APM chart wrong, it should only occur for the first installation (as the tags are created only once and the GET operations are negligible)?

Yes, but this is also a key onboarding flow that we want to optimize.

juliaElastic · 2022-07-27T15:17:09Z

@joshdover @kpollich Are you okay with merging this feature and improve later when savedObjectsTagging optimizations are done?

kpollich · 2022-07-27T15:19:32Z

@joshdover @kpollich Are you okay with merging this feature and improve later when savedObjectsTagging optimizations are done?

I think I would rather this be delayed until 8.5. The performance impacts on first time package installation seem significant enough that we should sort them out before landing this.

juliaElastic · 2022-07-27T15:24:25Z

@joshdover @kpollich Are you okay with merging this feature and improve later when savedObjectsTagging optimizations are done?

I think I would rather this be delayed until 8.5. The performance impacts on first time package installation seem significant enough that we should sort them out before landing this.

Yes, it is definitely for 8.5 as it is a new feature. My question was rather around whether we should wait for the savedObjectsTagging optimizations.

joshdover · 2022-07-27T15:26:33Z

I don't have a strong preference on whether it's fixed in this PR or not. I'm hoping the change to tagging is just as small as this similar PR: #131339

@pgayvallet do you have any objections to us doing the same for the tag client?

juliaElastic · 2022-07-27T15:29:43Z

I don't have a strong preference on whether it's fixed in this PR or not. I'm hoping the change to tagging is just as small as this similar PR: #131339

@pgayvallet do you have any objections to us doing the same for the tag client?

I am happy to contribute as well if we think this is important for fleet, and kibana core team might not have the capacity.

juliaElastic · 2022-07-28T06:28:09Z

@elasticmachine merge upstream

pgayvallet

LGTM

juliaElastic · 2022-07-28T08:57:48Z

@elasticmachine merge upstream

juliaElastic · 2022-07-28T12:06:26Z

I added the refresh: false option to the tag and tested locally that the tag creation sped up significantly, to about 200ms.

kibana-ci · 2022-07-28T15:22:23Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`savedObjectsTagging`	70	72	+2
`savedObjectsTaggingOss`	45	46	+1
total			+3

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`savedObjectsTaggingOss`	0	1	+1

Unknown metric groups

API count

id	before	after	diff
`savedObjectsTagging`	76	78	+2
`savedObjectsTaggingOss`	90	91	+1
total			+3

History

💚 Build #61439 succeeded d288690
💔 Build #61421 failed 7660613
💔 Build #61366 failed df7b186
💔 Build #61332 failed a430602
💔 Build #61191 failed f74e91d
💚 Build #61122 succeeded 766ee1a

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

cc @juliaElastic

WIP saved object tagging

90e46c9

juliaElastic added release_note:feature Makes this part of the condensed release notes v8.4.0 labels Jul 26, 2022

juliaElastic self-assigned this Jul 26, 2022

juliaElastic commented Jul 26, 2022

View reviewed changes

Comment thread x-pack/plugins/fleet/server/plugin.ts Outdated

juliaElastic added 2 commits July 26, 2022 16:14

fixing plugin usage

cfe84a8

added logic to create tags before assigning

77ec521

juliaElastic added the ci:deploy-cloud label Jul 26, 2022

juliaElastic changed the title ~~[Fleet] WIP saved object tagging~~ [Fleet] Tagging of integration assets Jul 26, 2022

juliaElastic marked this pull request as ready for review July 26, 2022 15:05

juliaElastic requested a review from a team as a code owner July 26, 2022 15:05

nchaulet self-requested a review July 26, 2022 15:08

nchaulet reviewed Jul 26, 2022

View reviewed changes

botelastic Bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Jul 26, 2022

nchaulet reviewed Jul 26, 2022

View reviewed changes

Comment thread x-pack/plugins/fleet/server/services/epm/kibana/assets/install.ts Outdated

moved constants out

d7cbcf4

nchaulet approved these changes Jul 26, 2022

View reviewed changes

kpollich reviewed Jul 26, 2022

View reviewed changes

fixed tests, added span

c4373f9

juliaElastic requested a review from a team as a code owner July 27, 2022 07:14

juliaElastic commented Jul 27, 2022

View reviewed changes

juliaElastic added v8.5.0 and removed v8.4.0 labels Jul 27, 2022

juliaElastic commented Jul 27, 2022

View reviewed changes

Comment thread x-pack/plugins/fleet/server/services/epm/kibana/assets/tag_assets.ts

juliaElastic added 4 commits July 27, 2022 10:14

added unit test to tagKibanaAssets

c7512b2

fix test

7706aab

fix types

d00478b

fixed tests

a5fa279

juliaElastic added 2 commits July 27, 2022 13:34

fixed tests

3f92ebe

fix types

4382894

fix sot tests by loading empty kibana archive

766ee1a

added tag checking to api integration test

f74e91d

Merge branch 'main' into feat/saved-object-tagging

a430602

pgayvallet approved these changes Jul 28, 2022

View reviewed changes

kibanamachine and others added 2 commits July 28, 2022 04:57

Merge branch 'main' into feat/saved-object-tagging

df7b186

added refresh option to speed up tagging assets

7660613

fixed tests

d288690

juliaElastic removed the ci:deploy-cloud label Jul 28, 2022

workaround to prevent installing fleet packages in SOT functional tests

0c9f93e

juliaElastic merged commit c6d4fb5 into elastic:main Jul 29, 2022

kibanamachine added the backport:skip This PR does not require backporting label Jul 29, 2022

andrewkroh mentioned this pull request Aug 15, 2022

Add Security Solution Tag to Threat Integrations elastic/integrations#3951

Closed

		const managedTagName = 'Managed';
		const tagColor = '#FFFFFF';

	const installation = await server.fleet.packageService.asInternalUser.ensureInstalledPackage({
	pkgName: 'synthetics',
	});

Conversation

juliaElastic commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 26, 2022

Uh oh!

Uh oh!

nchaulet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliaElastic Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

juliaElastic commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kpollich commented Jul 27, 2022

Uh oh!

pgayvallet commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juliaElastic commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshdover commented Jul 27, 2022

Uh oh!

joshdover commented Jul 27, 2022

Uh oh!

juliaElastic commented Jul 27, 2022

Uh oh!

kpollich commented Jul 27, 2022

Uh oh!

juliaElastic commented Jul 27, 2022

Uh oh!

joshdover commented Jul 27, 2022

Uh oh!

juliaElastic commented Jul 27, 2022

Uh oh!

juliaElastic commented Jul 28, 2022

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

juliaElastic commented Jul 28, 2022

Uh oh!

juliaElastic commented Jul 28, 2022

Uh oh!

kibana-ci commented Jul 28, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

juliaElastic commented Jul 26, 2022 •

edited

Loading

juliaElastic Jul 27, 2022 •

edited

Loading

juliaElastic commented Jul 27, 2022 •

edited

Loading

pgayvallet commented Jul 27, 2022 •

edited

Loading

juliaElastic commented Jul 27, 2022 •

edited

Loading