[UsageCollection] Expose `KibanaRequest` to explicitly opted-in collectors #83413

afharo · 2020-11-16T11:56:51Z

Summary

Provide the kibanaRequest property when calling the fetch method. It is only provided for those collectors that have explicitly opted-in via extendFetchContext.kibanaRequest: true.

This PR is a blocker for completing #82638

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…y opt-in

afharo

Self-review.

Sorry for the number of changes: if we had opted to skip the opt-in requirement, it would be 16 changes only (and simpler). But I understand the need to explicitly request the collector to opt-in.

afharo · 2020-11-16T11:59:03Z

...collection/server/collectors/application_usage/telemetry_application_usage_collector.test.ts


  const usageCollectionMock = createUsageCollectionSetupMock();
  usageCollectionMock.makeUsageCollector.mockImplementation((config) => {
-    collector = config;
+    collector = new Collector(logger, config);


This change is required because TS would complain about fetch not having the this context

apmmachine · 2020-11-16T16:23:32Z

💚 Build Succeeded

pipeline
Commit: 3328578

elasticmachine · 2020-11-16T16:24:45Z

Pinging @elastic/kibana-telemetry (Team:KibanaTelemetry)

src/plugins/usage_collection/server/collector/collector_set.ts

src/plugins/usage_collection/server/collector/usage_collector.ts

src/plugins/usage_collection/server/usage_collection.mock.ts

chrisronline · 2020-11-16T19:35:36Z

I'm trying to log kibanaRequest right in the fetch method here: https://github.com/elastic/kibana/pull/83413/files#diff-3e87fdcf6e019404800b16542a6c08e7074dcc4e97e9d5993ad6d3517f8fc307R103 and it's coming up undefined. Shouldn't that exist since the collector is opting in?

afharo · 2020-11-16T20:01:20Z

I'm trying to log kibanaRequest right in the fetch method here: #83413 (files) and it's coming up undefined. Shouldn't that exist since the collector is opting in?

@chrisronline yes, it's intended: Even after opting-in, it's optional: it's only provided when the request needs to be scoped. This usually happens in 3 situations:

The user is requesting an example of the telemetry payload
The API /api/telemetry/v2/clusters/_stats is called with unencrypted: true
The API /api/stats?extended=true is requested

It will be undefined when actually sending the telemetry (to ensure callAsInternalUser is used).

I tried to explain this behaviour in my update in the JSDocs and the README. Please, let me know if it's not clear enough and I need to explain it further.

x-pack/plugins/monitoring/server/kibana_monitoring/collectors/get_settings_collector.ts

TinaHeiligers · 2020-11-16T20:23:12Z

x-pack/plugins/monitoring/server/kibana_monitoring/collectors/get_usage_collector.ts

@@ -18,9 +18,9 @@ import { fetchClusters } from '../../lib/alerts/fetch_clusters';
 export function getMonitoringUsageCollector(
  usageCollection: UsageCollectionSetup,
  config: MonitoringConfig,
-  callCluster: LegacyAPICaller
+  esClient: ILegacyClusterClient


nit: This feels inconsitent in terms of nomenclature. esClient should indicate we're using the new elasticsearch client, whereas callCluster traditionally refers to the legacy elasticsearch client. When I first dug into the issue of the esClient being "missing" for the SO tags collector issue, I thought it was because the new elasticsearch client wasn't available yet. Even so, the legacy client should be available from the setup contract, so how are we solving for the client not be available yet?

lukeelmers

app arch generated docs changes LGTM

lukeelmers · 2020-11-16T21:08:25Z

src/plugins/data/server/server.api.md

 import { KibanaRequest } from 'src/core/server';
+import { KibanaRequest as KibanaRequest_2 } from 'kibana/server';


I guess importing from kibana/server vs src/core/server will create a new/duplicate import in the generated docs. From what I can see this isn't a problem, but TIL.

TinaHeiligers

I've added a few comments and suggestions but they're not blockers for pushing this work through.
The implementation does the job.
LGTM

x-pack/plugins/monitoring/server/kibana_monitoring/collectors/index.ts

src/plugins/telemetry/server/telemetry_collection/get_local_stats.ts

src/plugins/usage_collection/server/collector/collector.ts

x-pack/plugins/monitoring/server/kibana_monitoring/collectors/get_usage_collector.ts

x-pack/plugins/monitoring/server/kibana_monitoring/collectors/index.ts

Bamieh

LGTM

Bamieh · 2020-11-17T10:02:38Z

src/plugins/telemetry_collection_manager/server/plugin.ts

@@ -157,7 +157,10 @@ export class TelemetryCollectionManagerPlugin
    const soClient = config.unencrypted
      ? collectionSoService.getScopedClient(config.request)
      : collectionSoService.createInternalRepository();
-    return { callCluster, timestamp, usageCollection, esClient, soClient };
+    // Provide the kibanaRequest so opted-in plugins can scope their custom clients only if the request is not encrypted
+    const kibanaRequest = config.unencrypted ? request : void 0;


why void 0 instead of undefined?

It's essentially the same. But JS allows you do do let undefined = 1, while void 0 is univoquely undefined :)

watson

kibana-security changes LGTM 👍

flash1293

KibanaApp changes LGTM, only mock adjustments in tests

chrisronline · 2020-11-17T15:00:54Z

@afharo

From #83413 (comment), I'm a bit confused and maybe this is something that will be changed in #82638.

If a user has monitoring enabled (in addition to using legacy collection), the monitoring bulk_uploader will collect the telemetry data without any valid kibanaRequest object. When this happens, the usage field inside of the monitoring documents will contain all data, not scoped to any individual user's security configuration.

With this setup, can't a limited privilege user call the /api/telemetry/v2/clusters/_stats endpoint and access potentially sensitive data that the monitoring plugin was able to access as the internal user?

Also, for the monitoring usage collector, does this mean we have to handle two different ways of being called, one with a request and one without? I can understand how this makes sense in the context of security, but it also affects what kind of telemetry data we can return. For example, the ability to access the KibanaRequest object means we can get user-specific saved object data, such as active alerts. Is the expectation that usage collectors will have to handle some fetch calls returning this kind of data and other fetch calls not returning it? This related to how monitoring wants to use this change

afharo · 2020-11-17T16:14:07Z

From #83413 (comment), I'm a bit confused and maybe this is something that will be changed in #82638.

If a user has monitoring enabled (in addition to using legacy collection), the monitoring bulk_uploader will collect the telemetry data without any valid kibanaRequest object. When this happens, the usage field inside of the monitoring documents will contain all data, not scoped to any individual user's security configuration.

With this setup, can't a limited privilege user call the /api/telemetry/v2/clusters/_stats endpoint and access potentially sensitive data that the monitoring plugin was able to access as the internal user?

@chrisronline you are very correct! If the user has access to the .monitoring indices, we'll show the data collected by bulk_uploader. However, our plans for this release are to also remove the Kibana Usage collection in bulk_uploader (#83521), so it won't be much of a problem by FF :)

Also, to be fair, that scenario already happens nowadays: if the user has read access to the .monitoring indices, they will see the telemetry data reported by monitoring. If they don't, they'll see the local telemetry (even when the monitoring strategy could have returned some data).

Also, for the monitoring usage collector, does this mean we have to handle two different ways of being called, one with a request and one without? I can understand how this makes sense in the context of security, but it also affects what kind of telemetry data we can return. For example, the ability to access the KibanaRequest object means we can get user-specific saved object data, such as active alerts. Is the expectation that usage collectors will have to handle some fetch calls returning this kind of data and other fetch calls not returning it? This related to how monitoring wants to use this change

That is also correct: when a real user requests the telemetry payload, they should only get the data they have access to. However, when sending telemetry to the remote telemetry service, we shouldn't scope that data to the user requesting it (following your example, we'll likely want to report all the existing alerts, not a partial representation of that). This has always been automatically handled in the callCluster client provided to the collectors' fetch methods. However, we found some additional use cases where collectors are using their own clients. This is the reason we made the KibanaRequest only available when opted-in: it serves a very specific use case and shouldn't be used unless absolutely necessary (the collector needs to maintain its own client). And, even when opted-in, we can't ensure KibanaRequest to exist because any server-side triggered collection (the one that is actually reported to the remote service) won't have it (and we chose not to create a Kibana-Internal-mocked KibanaRequest). Circling back to the Alerts API specifics, I think they should consider exposing an API that follows the asScoped(req)/callAsInternalUser approach. They already have the asScoped part, but it looks to me like there's a need to expose the callAsInternalUser API as well.

chrisronline · 2020-11-17T16:55:27Z

And, even when opted-in, we can't ensure KibanaRequest to exist because any server-side triggered collection (the one that is actually reported to the remote service) won't have it (and we chose not to create a Kibana-Internal-mocked KibanaRequest)

This definitely makes sense to me, but I worry about how this will work in practice.

Now that I can opt-in and sometimes receive a KibanaRequest object, I plan to update the monitoring usage collector to also fetch data about the active alerts for the registered user. However, this means that my telemetry payload will vary from environment to environment, as users with legacy collection will never have the fetch() method called with a KibanaRequest and users with Metricbeat collection (which call the api/stats?extended=true api) will always have the fetch() method called with a KibanaRequest. As a usage collector author, I will have to adjust my schema to ensure that these fields may not be present and I just want to make sure that's the design and goal (for this phase) and it's clearly documented as a potential gotcha for usage collectors.

It seems clear that this problem falls more on the Alerts side of things, rather than Telemetry so I don't think we need to dig more into it (outside of the comments above).

afharo · 2020-11-17T17:16:25Z

As a usage collector author, I will have to adjust my schema to ensure that these fields may not be present and I just want to make sure that's the design and goal (for this phase) and it's clearly documented as a potential gotcha for usage collectors.

@chrisronline that is an interesting way of putting it. The way I see it, the collector should prioritise the behaviour with the Internal client (no KibanaRequest). If the scoped request fails to retrieve any data and the collector fails, usageCollection already takes care of dismissing any failed collectors. Please, let me know if that intention is not clear with the JSDoc and README documentation and I'll try to rework it.

Re Metricbeat's collection via /api/stats?extended, we are currently scoping all the clients (ES & SO) that are sent through to the fetch method in the collectors. It means we are risking the same scenario even without the new KibanaRequest approach.

Also, I hope we can stop Metricbeat's extended collection soon enough as well (as agreed in the RFC, the end goal is to stop collecting in the .monitoring indices the telemetry about Kibana Usage) :)

chrisronline · 2020-11-17T17:28:13Z

The way I see it, the collector should prioritise the behaviour with the Internal client (no KibanaRequest).

I think my main point is that the telemetry data will differ and I hope it's clear to the consumer of this data why this may differ With that said, it does sound like #83521 may remove the following scenario and please let me know if it will or will not:

Let's say the Lens team decides to use extendFetchContext. kibanaRequest in their usage collector, and when the KibanaRequest is available, they will grab various SOs and use that data to help fill their telemetry payload.

Now, let's say the developer or PM is looking to test this functionality in cloud staging (which currently only supports legacy monitoring collection).

Because legacy monitoring collection handles collecting the Kibana usage data without access to a KibanaRequest object, the fetch() will always be called without it (if I recall correctly).

They will fail to see any of this enhanced SO data showing up in their telemetry data and wonder why. This would also be a problem if/when they pushed these changes to production and some users sent this enhanced SO data, but most would likely not (due to most users still using legacy monitoring collection).

afharo · 2020-11-17T17:46:21Z

@chrisronline the only possible scenario where I think some reported telemetry might include it vs. some might not is comparing Metricbeat's-collected telemetry (assuming Metricbeat's user has access to the SOs you mentioned) vs. Legacy collection (to be removed in #83521) or even "local/local X-Pack" collections. The legacy and local collections will not provide KibanaRequest when actually reporting telemetry to the remote service. That is why the dev who worked-around the SOs to retrieve some telemetry only for specific users did implement the collector in a wrong way because the collectors should be implemented with the user Kibana Internal (kibana_system) access in mind.

That's also why we went for an explicitly opt-in approach (despite making types harder to follow and maintain). If the dev opts-in when developing the collector, they should fully understand the caveats and limitations.

However, thanks to this conversation, I added another TODO to #83521 to make sure we also stop collecting the ?extended data in Metricbeat. Hopefully by 7.11, .monitoring-kibana indices won't include any usage data and only monitoring stats.

chrisronline

Thanks for hoping on a call and clarifying this for me! This works LGTM! Great work!

kibanamachine · 2020-11-17T19:05:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4afc962

Metrics [docs]

✅ unchanged

History

💚 Build #88245 succeeded 4afc962
💚 Build #88125 succeeded 3328578
💚 Build #88044 succeeded 3328578
💔 Build #88000 failed 3e48359
💚 Build #87960 succeeded 07a4a6c

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

… collectors (#83413) (#83615)

…o-node-details * 'master' of github.com:elastic/kibana: (65 commits) update chromedriver dependency to 87 (elastic#83624) [TSVB] use new Search API for rollup search (elastic#83275) [TSVB] Y-axis has number formatting not considering all series formatters in the group (elastic#83438) [Logs UI] Update <LogStream /> internal state when its props change (elastic#83302) Add tag bulk action context menu (elastic#82816) [code coverage] adding plugin to flush coverage data (elastic#83447) [UsageCollection] Expose `KibanaRequest` to explicitly opted-in collectors (elastic#83413) Added eventBus to trigger and listen plotHandler event (elastic#83435) [Runtime fields] Editor phase 1 (elastic#81472) [Maps] Fix threshold alert issue resolving nested fields (elastic#83577) chore(NA): remove usage of unverified es snapshots (elastic#83589) [DOCS] Adds Elastic Contributor Program link (elastic#83561) Upgrade EUI to v30.2.0 (elastic#82730) Don't show loading screen during auto-reload (elastic#83376) Functional tests - fix esArchive mappings with runtime fields (elastic#83530) [deb/rpm] Create keystore after installation (elastic#76465) [rpm] Create default environment file at "/etc/sysconfig/kibana" (elastic#82144) [docker] removes workaround for missing crypto-policies-scripts subpackage (elastic#83455) [ML] Persisted URL state for the Data frame analytics jobs and models pages (elastic#83439) adds xpack.security.authc.selector.enabled setting (elastic#83551) ...

elasticmachine · 2021-10-01T21:28:22Z

Pinging @elastic/kibana-core (Team:Core)

[UsageCollection] Expose KibanaRequest to collectors that explicitl…

57abd1e

…y opt-in

afharo added Feature:Telemetry v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:KibanaTelemetry v7.11.0 labels Nov 16, 2020

afharo commented Nov 16, 2020

View reviewed changes

afharo mentioned this pull request Nov 16, 2020

[Telemetry] Move Monitoring collection strategy to a collector #82638

Merged

6 tasks

Accept Doc API changes 🤷

07a4a6c

afharo linked an issue Nov 16, 2020 that may be closed by this pull request

[Usage Collection] Extend the APIs provided to the fetch method #75875

Closed

afharo added 2 commits November 16, 2020 15:25

[TS] Allow Collector to be extended via options

3e48359

Fix KibanaSettingsCollector definition

3328578

afharo marked this pull request as ready for review November 16, 2020 16:24

afharo requested a review from a team as a code owner November 16, 2020 16:24

afharo requested a review from a team November 16, 2020 16:24

afharo requested review from a team as code owners November 16, 2020 16:24

afharo requested a review from a team November 16, 2020 16:24

afharo added the review label Nov 16, 2020

afharo changed the title ~~[UsageCollection] Expose KibanaRequest to collectors explicitly opted-in~~ [UsageCollection] Expose KibanaRequest to explicitly opted-in collectors Nov 16, 2020