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] Support browsing granular integrations #99866

Merged
merged 12 commits into from
May 26, 2021

Conversation

jen-huang
Copy link
Contributor

@jen-huang jen-huang commented May 11, 2021

Summary

Part of #93315. This PR has two main parts:

Allow users to browse "granular integrations"

Screenshots

Granular AWS integration tiles on browse page
image

Integration specific detail page
image

Installed integrations page, single AWS tile
image

  • What this means: packages can now export multiple integrations by exporting multiple policy_templates in their package definition. Previously, all packages exported 1 policy template. Now, we have changes coming to the AWS package (Adjust aws package to use input groups integrations#767) so that it exports multiple AWS integrations: AWS CloudWatch, AWS DynamoDB, etc.
  • Each one of these integrations need to show as a separate icon/tile in Integrations > All integrations. The search functionality should work normally with these new tiles. Clicking one of these tiles will bring you to the integration detail page that shows that integration's specific icon, screenshots, and README content.
  • In Integrations > Installed integrations, only the top-level package tile should show (aka only AWS tile) for now.
  • All changes should be BWC with previous packages (aka packages that only export 1 policy template).

Support package-level vars (user configurable variables)

Screenshots

Policy editor "Integrations settings" area expanded to include package-level vars
image

Package-level vars are used properly while compiling final agent YAML
image

  • Previously we only supported vars at the input- and stream- level. Now, packages can start using package-level vars. Fleet will use these package-level vars when compiling the final agent YAML.

How to test

TLDR Build in-flight AWS package and load it into your local package registry

  1. Get elastic-package https://github.com/elastic/elastic-package
  2. Pull down Adjust aws package to use input groups integrations#767
  3. From that PR, cd into <integrations>packages/aws
  4. Run elastic-package build, make note of the built aws/0.6.0 package at <integrations>/build
  5. Pull down and build package-registry https://github.com/elastic/package-registry
  6. Copy the built 0.6.0 package to <package-registry>/build/package-storage/packages/aws
  7. Run the package registry go run .
  8. Edit kibana.dev.yml to use your local package registry by adding: xpack.fleet.registryUrl: http://localhost:8080
  9. Start Kibana and observe the above changes to Integrations UI and policy editor. Play around to make sure I didn't break any other packages

What's next

You'll notice that the policy editor for the AWS package only displays "Collect billing metrics". This is expected for now. The second part of the work to fully support all the requirements in #93315 is to adjust the policy editor to support multiple policy_templates too. Originally I tried putting everything in one PR but that second part is a bit more complex and it was holding up the rest of the work from going in 🙃

@jen-huang jen-huang added release_note:enhancement v8.0.0 Team:Fleet Team label for Observability Data Collection Fleet team v7.14.0 labels May 11, 2021
@jen-huang jen-huang self-assigned this May 11, 2021
@jen-huang jen-huang requested a review from a team as a code owner May 11, 2021 23:14
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@jen-huang
Copy link
Contributor Author

@kpollich You might be interested in reviewing this PR and running through the above testing steps as it relates to what you've been working on. I apologize in advance that you might need to resolve even more conflicts with the top-level Integration UI work if this PR makes it in before that 😅

@kpollich
Copy link
Member

@kpollich You might be interested in reviewing this PR and running through the above testing steps as it relates to what you've been working on. I apologize in advance that you might need to resolve even more conflicts with the top-level Integration UI work if this PR makes it in before that 😅

Thanks, @jen-huang - definitely touches a lot of the same files that will be moved as part of #99848. I think it definitely makes the most sense to aim for landing this PR first before we move integrations to a separate app. It will be easier to fix conflicts by just "re-moving" files around instead of recreating your changes on a bunch of newly relocated/renamed files.

@jen-huang jen-huang requested a review from a team as a code owner May 19, 2021 23:46
@jen-huang jen-huang changed the title [WIP][Fleet] Support browsing granular integrations [Fleet] Support browsing granular integrations May 19, 2021
@jen-huang
Copy link
Contributor Author

@elasticmachine merge upstream

@jen-huang
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

I ran through the local testing instructions in the PR description and everything seems good on my end. This should get a second review from someone with more experience on the Fleet codebase, though.

validationResults.vars = packageVars.reduce((results, [name, varEntry]) => {
results[name] = validatePackagePolicyConfig(varEntry, packageVarsByName[name]);
return results;
}, {} as ValidationEntry);
Copy link
Member

Choose a reason for hiding this comment

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

Minor thing, but it seems like elsewhere in the fleet codebase we use the reduce's generic - e.g. packageVars.reduce<ValidationEntry>(...)- instead of a type assertion like this. Although looking more it seems inconsistent file-to-file. Is this something we should or could make consistent for all our uses of reduce?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah our usage has been inconsistent, I will address this in the next PR for granularity support in order to get this one in now. thanks!

function InstalledPackages() {
});

// Packages can export multiple integrations, aka `policy_templates`
Copy link
Member

Choose a reason for hiding this comment

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

Super helpful comment - thank you!

@jen-huang
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
fleet 455 456 +1

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
fleet 972 980 +8

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fleet 731.8KB 736.9KB +5.2KB

Page load bundle

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

id before after diff
fleet 346.3KB 347.5KB +1.2KB

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id before after diff
ingest-package-policies 31 32 +1
Unknown metric groups

API count

id before after diff
fleet 1062 1070 +8

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 22 20 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 261 149 -112
lens 67 45 -22
licensing 18 15 -3
lists 239 236 -3
maps 286 208 -78
ml 121 115 -6
monitoring 109 56 -53
securitySolution 386 342 -44
stackAlerts 101 95 -6
total -342

History

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

cc @jen-huang

@jen-huang jen-huang enabled auto-merge (squash) May 26, 2021 16:58
@jen-huang jen-huang disabled auto-merge May 26, 2021 16:59
@jen-huang jen-huang merged commit ba7c027 into elastic:master May 26, 2021
@jen-huang jen-huang deleted the granularity-integration-list branch May 26, 2021 16:59
@jen-huang jen-huang added the auto-backport Deprecated - use backport:version if exact versions are needed label May 26, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request May 26, 2021
* Manual cherry pick of work to support integration tiles and package-level vars

* Fix types

* Remove registry input group typings

* Show integration-specific readme, title, and icon in package details page

* Revert unnecessary changes

* Add package-level `vars` field to package policy SO mappings

* Fix types

* Fix test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request May 26, 2021
* Manual cherry pick of work to support integration tiles and package-level vars

* Fix types

* Remove registry input group typings

* Show integration-specific readme, title, and icon in package details page

* Revert unnecessary changes

* Add package-level `vars` field to package policy SO mappings

* Fix types

* Fix test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Jen Huang <its.jenetic@gmail.com>
ecezalp pushed a commit to ecezalp/kibana that referenced this pull request May 26, 2021
* Manual cherry pick of work to support integration tiles and package-level vars

* Fix types

* Remove registry input group typings

* Show integration-specific readme, title, and icon in package details page

* Revert unnecessary changes

* Add package-level `vars` field to package policy SO mappings

* Fix types

* Fix test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
ecezalp pushed a commit to ecezalp/kibana that referenced this pull request May 26, 2021
* Manual cherry pick of work to support integration tiles and package-level vars

* Fix types

* Remove registry input group typings

* Show integration-specific readme, title, and icon in package details page

* Revert unnecessary changes

* Add package-level `vars` field to package policy SO mappings

* Fix types

* Fix test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@mdelapenya
Copy link
Contributor

mdelapenya commented May 28, 2021

Hi, we have found a regression in the e2e tests, probably caused by this PR.

We are observing changes in how an integration is added to the policy. Before these changes, we were able to hit the /api/fleet/package_policies endpoint passing a payload for a PackageDataStream (https://github.com/elastic/e2e-testing/blob/master/internal/kibana/policies.go#L132-L142), but now we receive a HTTP 400 error code:

Error: could not add package to policy; API status code = 400; response body = {"statusCode":400,"error":"Bad Request","message":"[request body.package.id]: definition for this key is missing"}

And this code hasn’t changed in the test framework.

I've checked the git history of Kibana for the x-pack/plugins/fleet path, and the below list contains the possible culprit commits:

Screenshot 2021-05-28 at 11 45 36

I've diff'ed #99866, and the code in there seems a feasible candidate to cause the regression. I'm currently running the tests for the previous PR (#100476) using the dedicated pipeline we created to run e2e tests for Kibana: https://beats-ci.elastic.co/blue/organizations/jenkins/e2e-tests%2Fe2e-testing-mbp/detail/master/922/pipeline.

At the same time, I've run the e2e tests for #99866: https://beats-ci.elastic.co/blue/organizations/jenkins/e2e-tests%2Fe2e-testing-mbp/detail/master/924/pipeline

In both cases we:

  1. have built the docker image for each PR first, using "Deploy my Kibana" tool, so they are available in the Docker registry: docker.elastic.co/observability-ci/kibana:pr100476
  2. Once built (~50mins) run the e2e tests using pr$ID as KIBANA_VERSION.(pr99866 and pr 100476)

It's possible to reproduce this locally:

Before this changes, the tests adding integration to policies pass:

 TAGS="linux_integration" TIMEOUT_FACTOR=3 LOG_LEVEL=TRACE DEVELOPER_MODE=true KIBANA_VERSION=pr100476 make -C e2e/_suites/fleet functional-test

Passing:

2 scenarios (2 passed)
10 steps (10 passed)
3m0.812610611s

With this changes, the tests adding integration to policies fail:

 TAGS="linux_integration" TIMEOUT_FACTOR=3 LOG_LEVEL=TRACE DEVELOPER_MODE=true KIBANA_VERSION=pr99866 make -C e2e/_suites/fleet functional-test

Failing with:

--- Failed steps:

  Scenario Outline: Adding the Linux Integration to an Agent ... # features/linux_integration.feature:6
    When the "Linux" integration is "added" in the policy # features/linux_integration.feature:9
      Error: could not add package to policy; API status code = 400; response body = {"statusCode":400,"error":"Bad Request","message":"[request body.package.id]: definition for this key is missing"}

  Scenario Outline: Adding the Linux Integration to an Agent ... # features/linux_integration.feature:6
    When the "Linux" integration is "added" in the policy # features/linux_integration.feature:9
      Error: could not add package to policy; API status code = 400; response body = {"statusCode":400,"error":"Bad Request","message":"[request body.package.id]: definition for this key is missing"}


2 scenarios (2 failed)
10 steps (4 passed, 2 failed, 4 skipped)
2m44.62434826s

@ruflin
Copy link
Member

ruflin commented May 28, 2021

Thanks for the all the investigation @mdelapenya . @nchaulet @kpollich I assume we introduce a breaking change during the introduction of the granular integration implementation. Can you please have a look?

@kpollich
Copy link
Member

@mdelapenya it looks like the issue here is that the package object in the request body includes an id field, which the fleet codebase doesn't expect.

Pasting the error message from above just for convenience:

{"statusCode":400,"error":"Bad Request","message":"[request body.package.id]: definition for this key is missing"}

The PackageDataStream struct in https://github.com/elastic/e2e-testing/blob/master/internal/kibana/policies.go#L132-L142 defines the package field as another struct IntegrationPackage here: https://github.com/elastic/e2e-testing/blob/888ecfec924124d6359e2dadf999acc347c12477/internal/kibana/integrations.go#L14-L19

This struct defines the package object as such:

{
  package: { 
    id: string,
    name: string,
    title: string,
    version: string
  }
}

Fleet's expected schema for this object is defined here:

package: schema.maybe(
schema.object({
name: schema.string(),
title: schema.string(),
version: schema.string(),
})
),

I was able to reproduce this exact error locally by forcing an id field into this package object when we create a package policy in the Fleet UI:

Screenshots Screen Shot 2021-05-28 at 8 30 37 AM Screen Shot 2021-05-28 at 8 31 50 AM Screen Shot 2021-05-28 at 8 32 20 AM Screen Shot 2021-05-28 at 8 32 47 AM

This PR didn't alter anything around this schema that I can see, the only changes made to this particular request structure were to allow for the vars field at the package level to support granular integrations here:

vars: schema.maybe(ConfigRecordSchema),

I'm new to the project (and Elastic in general 🙂), so I'm not 100% certain on how to proceed towards a resolution, but maybe someone else could chime in on a few questions:

  1. Should Fleet support this id field when creating package policies?
  2. Is the Linux integration exporting this ID and causing this specific failure, or this an issue with the E2E tests?

@mdelapenya
Copy link
Contributor

As far as I know, the package.id field was already there and the tests did pass. What I understand from this analysis is that now the schema contract is more strict. Maybe in the past that new field was simple skipped and now a strict contract is enforced.

@kpollich
Copy link
Member

As far as I know, the package.id field was already there and the tests did pass. What I understand from this analysis is that now the schema contract is more strict. Maybe in the past that new field was simple skipped and now a strict contract is enforced.

That could definitely be the case. I'll surface this with the rest of the Fleet team and take a look at how we enforce this request schema.

@mdelapenya
Copy link
Contributor

We simple retrieve the integration hitting /epm/packages?experimental=true, and then filter them by packageName (i.e. "linux"), and with that JSON response we build the struct discussed above, so it very likely includes package ID. We could discard that field on our side, but IMHO it would be like a workaround

@kpollich
Copy link
Member

We simple retrieve the integration hitting /epm/packages?experimental=true, and then filter them by packageName (i.e. "linux"), and with that JSON response we build the struct discussed above, so it very likely includes package ID. We could discard that field on our side, but IMHO it would be like a workaround

Yes I agree. I just filed an issue (#100897) to permit the id field in create requests to unblock the E2E tests. I'll take this on and ping you once a PR is up.

@mdelapenya
Copy link
Contributor

Cool thank you for the fast response! If we decide to not accept the id field in the payload, I've just removed the ID in a local branch of the e2e tests, and it works, just in case.

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:enhancement Team:Fleet Team label for Observability Data Collection Fleet team v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants