-
Notifications
You must be signed in to change notification settings - Fork 8.5k
refresh cache on unencrypted telemetry request #124253
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
4944a9b
refresh cache on unencrypted telemetry request
Bamieh a756fc2
Merge branch 'main' of https://github.com/elastic/kibana into telemet…
Bamieh 5df4b77
add a lot of tests and mocks :d
Bamieh 1ea03db
add functional test
Bamieh 64f132e
remove unused variable
Bamieh f945cae
add logic into getStats and add test cases
Bamieh e367a34
Merge branch 'main' of https://github.com/elastic/kibana into telemet…
Bamieh eeb31a6
lint
Bamieh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
103 changes: 103 additions & 0 deletions
103
src/plugins/telemetry/server/routes/telemetry_usage_stats.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| import { registerTelemetryUsageStatsRoutes } from './telemetry_usage_stats'; | ||
| import { coreMock, httpServerMock } from 'src/core/server/mocks'; | ||
| import type { RequestHandlerContext, IRouter } from 'kibana/server'; | ||
| import { telemetryCollectionManagerPluginMock } from '../../../telemetry_collection_manager/server/mocks'; | ||
|
|
||
| async function runRequest( | ||
| mockRouter: IRouter<RequestHandlerContext>, | ||
| body?: { unencrypted?: boolean; refreshCache?: boolean } | ||
| ) { | ||
| expect(mockRouter.post).toBeCalled(); | ||
| const [, handler] = (mockRouter.post as jest.Mock).mock.calls[0]; | ||
| const mockResponse = httpServerMock.createResponseFactory(); | ||
| const mockRequest = httpServerMock.createKibanaRequest({ body }); | ||
| await handler(null, mockRequest, mockResponse); | ||
|
|
||
| return { mockResponse, mockRequest }; | ||
| } | ||
|
|
||
| describe('registerTelemetryUsageStatsRoutes', () => { | ||
| const router = { | ||
| handler: undefined, | ||
| config: undefined, | ||
| post: jest.fn().mockImplementation((config, handler) => { | ||
| router.config = config; | ||
| router.handler = handler; | ||
| }), | ||
| }; | ||
| const telemetryCollectionManager = telemetryCollectionManagerPluginMock.createSetupContract(); | ||
| const mockCoreSetup = coreMock.createSetup(); | ||
| const mockRouter = mockCoreSetup.http.createRouter(); | ||
| const mockStats = [{ clusterUuid: 'text', stats: 'enc_str' }]; | ||
| telemetryCollectionManager.getStats.mockResolvedValue(mockStats); | ||
|
|
||
| describe('clusters/_stats POST route', () => { | ||
| it('registers _stats POST route and accepts body configs', () => { | ||
| registerTelemetryUsageStatsRoutes(mockRouter, telemetryCollectionManager, true); | ||
| expect(mockRouter.post).toBeCalledTimes(1); | ||
| const [routeConfig, handler] = (mockRouter.post as jest.Mock).mock.calls[0]; | ||
| expect(routeConfig.path).toMatchInlineSnapshot(`"/api/telemetry/v2/clusters/_stats"`); | ||
| expect(Object.keys(routeConfig.validate.body.props)).toEqual(['unencrypted', 'refreshCache']); | ||
| expect(handler).toBeInstanceOf(Function); | ||
| }); | ||
|
|
||
| it('responds with encrypted stats with no cache refresh by default', async () => { | ||
| registerTelemetryUsageStatsRoutes(mockRouter, telemetryCollectionManager, true); | ||
|
|
||
| const { mockRequest, mockResponse } = await runRequest(mockRouter); | ||
| expect(telemetryCollectionManager.getStats).toBeCalledWith({ | ||
| request: mockRequest, | ||
| unencrypted: undefined, | ||
| refreshCache: undefined, | ||
| }); | ||
| expect(mockResponse.ok).toBeCalled(); | ||
| expect(mockResponse.ok.mock.calls[0][0]).toEqual({ body: mockStats }); | ||
| }); | ||
|
|
||
| it('when unencrypted is set getStats is called with unencrypted and refreshCache', async () => { | ||
| registerTelemetryUsageStatsRoutes(mockRouter, telemetryCollectionManager, true); | ||
|
|
||
| const { mockRequest } = await runRequest(mockRouter, { unencrypted: true }); | ||
| expect(telemetryCollectionManager.getStats).toBeCalledWith({ | ||
| request: mockRequest, | ||
| unencrypted: true, | ||
| refreshCache: true, | ||
| }); | ||
| }); | ||
|
|
||
| it('calls getStats with refreshCache when set in body', async () => { | ||
| registerTelemetryUsageStatsRoutes(mockRouter, telemetryCollectionManager, true); | ||
| const { mockRequest } = await runRequest(mockRouter, { refreshCache: true }); | ||
| expect(telemetryCollectionManager.getStats).toBeCalledWith({ | ||
| request: mockRequest, | ||
| unencrypted: undefined, | ||
| refreshCache: true, | ||
| }); | ||
| }); | ||
|
|
||
| it('calls getStats with refreshCache:true even if set to false in body when unencrypted is set to true', async () => { | ||
| registerTelemetryUsageStatsRoutes(mockRouter, telemetryCollectionManager, true); | ||
| const { mockRequest } = await runRequest(mockRouter, { | ||
| refreshCache: false, | ||
| unencrypted: true, | ||
| }); | ||
| expect(telemetryCollectionManager.getStats).toBeCalledWith({ | ||
| request: mockRequest, | ||
| unencrypted: true, | ||
| refreshCache: true, | ||
| }); | ||
| }); | ||
|
|
||
| it.todo('always returns an empty array on errors on encrypted payload'); | ||
| it.todo('returns the actual request error object when in development mode'); | ||
| it.todo('returns forbidden on unencrypted and ES returns 403 in getStats'); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| /* | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧡 |
||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| import type { | ||
| TelemetryCollectionManagerPluginSetup, | ||
| TelemetryCollectionManagerPluginStart, | ||
| } from './types'; | ||
|
|
||
| export type Setup = jest.Mocked<TelemetryCollectionManagerPluginSetup>; | ||
| export type Start = jest.Mocked<TelemetryCollectionManagerPluginStart>; | ||
|
|
||
| export const telemetryCollectionManagerPluginMock = { | ||
| createSetupContract, | ||
| createStartContract, | ||
| }; | ||
|
|
||
| function createSetupContract(): Setup { | ||
| const setupContract: Setup = { | ||
| getStats: jest.fn(), | ||
| getOptInStats: jest.fn(), | ||
| setCollectionStrategy: jest.fn(), | ||
| }; | ||
|
|
||
| return setupContract; | ||
| } | ||
|
|
||
| function createStartContract(): Start { | ||
| const startContract: Start = { | ||
| getOptInStats: jest.fn(), | ||
| getStats: jest.fn(), | ||
| }; | ||
|
|
||
| return startContract; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Should this be handled down inside the
telemetryCollectionManager.getStats? It may cause different behaviours if called from somewhere else. What do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep the logic at the route level since we might want to set up things differently in different places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see where you're coming from. However, the cache is implemented in
telemetryCollectionManager.getStats, I don't see why others may want to use it in a different way. Especially bearing in mind that,getStatsis the one deciding how to scope the requests (so any other place usingunencrypted: true, refreshCache: falsemight expose again values retrieved with different permissions to the caller.IMO, the scoping and the refresh algos should be together.