-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[UA][Core] Surface integrations with internal APIs in upgrade assistant #199026
Changes from all commits
0e10cb4
087ca98
7663d11
e31e86b
687164e
003a5b1
8adf247
2161b63
eb48e72
d2287ac
457732b
119d689
07262b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
/* | ||
* 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", the "GNU Affero General Public License v3.0 only", 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", the "GNU Affero General Public | ||
* License v3.0 only", or the "Server Side Public License, v 1". | ||
*/ | ||
|
||
import type { IRouter } from '@kbn/core/server'; | ||
import { DEPRECATED_ROUTES } from '../../../common'; | ||
|
||
export const registerInternalDeprecatedRoute = (router: IRouter) => { | ||
router.get( | ||
{ | ||
path: DEPRECATED_ROUTES.INTERNAL_DEPRECATED_ROUTE, | ||
validate: false, | ||
options: { | ||
// Explicitly set access is to internal | ||
access: 'internal', | ||
deprecated: { | ||
documentationUrl: 'https://elastic.co/', | ||
severity: 'critical', | ||
message: 'Additonal message for internal deprecated api', | ||
reason: { type: 'deprecate' }, | ||
}, | ||
Comment on lines
+21
to
+26
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. This actually raises something of an oversight: I don't think ever setting an internal route as deprecated was intended for this API 🤔 , although being able to set My gut feel would be to remove it from the types for internal routes (versioned or non-versioned). FWIW I don't think we should block on it here. 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. Lets address this in a separate PR after we think about it for all API deprecations. Created a placeholder issue for now #199675 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.
|
||
}, | ||
}, | ||
async (ctx, req, res) => { | ||
return res.ok({ | ||
body: { | ||
result: | ||
'Called deprecated route with `access: internal`. Check UA to see the deprecation.', | ||
}, | ||
}); | ||
} | ||
); | ||
|
||
router.get( | ||
{ | ||
path: DEPRECATED_ROUTES.INTERNAL_ONLY_ROUTE, | ||
validate: false, | ||
// If no access is specified then it defaults to internal | ||
}, | ||
async (ctx, req, res) => { | ||
return res.ok({ | ||
body: { | ||
result: | ||
'Called route with `access: internal` Although this API is not marked as deprecated it will show in UA. Check UA to see the deprecation.', | ||
}, | ||
}); | ||
} | ||
); | ||
}; |
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,62 @@ | ||||||||||
/* | ||||||||||
* 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", the "GNU Affero General Public License v3.0 only", 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", the "GNU Affero General Public | ||||||||||
* License v3.0 only", or the "Server Side Public License, v 1". | ||||||||||
*/ | ||||||||||
|
||||||||||
import type { | ||||||||||
ApiDeprecationDetails, | ||||||||||
DomainDeprecationDetails, | ||||||||||
} from '@kbn/core-deprecations-common'; | ||||||||||
|
||||||||||
import type { PostValidationMetadata } from '@kbn/core-http-server'; | ||||||||||
import type { BuildApiDeprecationDetailsParams } from '../types'; | ||||||||||
import { | ||||||||||
getApiDeprecationMessage, | ||||||||||
getApiDeprecationsManualSteps, | ||||||||||
getApiDeprecationTitle, | ||||||||||
} from './i18n_texts'; | ||||||||||
|
||||||||||
export const getIsAccessApiDeprecation = ({ | ||||||||||
isInternalApiRequest, | ||||||||||
isPublicAccess, | ||||||||||
}: PostValidationMetadata): boolean => { | ||||||||||
const isNotPublicAccess = !isPublicAccess; | ||||||||||
const isNotInternalRequest = !isInternalApiRequest; | ||||||||||
Comment on lines
+27
to
+28
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. nit:
Suggested change
|
||||||||||
|
||||||||||
return !!(isNotPublicAccess && isNotInternalRequest); | ||||||||||
}; | ||||||||||
|
||||||||||
export const buildApiAccessDeprecationDetails = ({ | ||||||||||
apiUsageStats, | ||||||||||
deprecatedApiDetails, | ||||||||||
}: BuildApiDeprecationDetailsParams): DomainDeprecationDetails<ApiDeprecationDetails> => { | ||||||||||
const { apiId, apiTotalCalls, totalMarkedAsResolved } = apiUsageStats; | ||||||||||
const { routeVersion, routePath, routeDeprecationOptions, routeMethod } = deprecatedApiDetails; | ||||||||||
|
||||||||||
const deprecationLevel = routeDeprecationOptions?.severity || 'warning'; | ||||||||||
|
||||||||||
return { | ||||||||||
apiId, | ||||||||||
title: getApiDeprecationTitle(deprecatedApiDetails), | ||||||||||
level: deprecationLevel, | ||||||||||
message: getApiDeprecationMessage(deprecatedApiDetails, apiUsageStats), | ||||||||||
documentationUrl: routeDeprecationOptions?.documentationUrl, | ||||||||||
correctiveActions: { | ||||||||||
manualSteps: getApiDeprecationsManualSteps(), | ||||||||||
mark_as_resolved_api: { | ||||||||||
routePath, | ||||||||||
routeMethod, | ||||||||||
routeVersion, | ||||||||||
apiTotalCalls, | ||||||||||
totalMarkedAsResolved, | ||||||||||
timestamp: new Date(), | ||||||||||
}, | ||||||||||
}, | ||||||||||
deprecationType: 'api', | ||||||||||
domainId: 'core.http.access-deprecations', | ||||||||||
}; | ||||||||||
}; |
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.
These are great for testing within Kibana, where the internal origin header is added to a request sent from console.
However, we need to surface calls to internal APIs mainly when requests originate externally and may not necessarily include the header. Surfacing these will warn end-users about the upcoming change.
We also want telemetry, to track adoption of internal APIs that are being consumed by integrations and use this info to consider public API alternatives instead,
In other words:
desired:
unversioned|get|/XXXXXXXXXXXX/bundles/plugin/transform/1.0.0/{path*}
,unversioned|get|/XXXXXXXXXXXX/bundles/plugin/discover/1.0.0/{path*}
andunversioned|get|/XXXXXXXXXXXX/bundles/plugin/aiops/1.0.0/{path*}
))scenario 2:
desired:
****/gundle/**
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.
Good catch on the bundle tracking, it should be fixed now, the boolean i was using for non versioned routes excluded the bundles from the calculation. The
bundles
arepublic
apis so they should not be tracked,I dont fully understand the scenarios and desired behaviors though. I believe I am covering the desired behavior here but I am not sure i follow the rest of the comment.
I've updated the README and console commands to explicitly set
elasticInternalOrigin=false
to enable tracking them from the console for quick tesitng. We do have integration tests to ensure proper testing for the behavior of this and the previous api deprecations.In the dev console we need to explicitly set the query param
elasticInternalOrigin
tofalse
to track the request as non-internal origin.Can you clarify what you mean exactly here? External vs internal calls are specified via the header (or query param), and if the header/queryParam is missing then its an external call which in that case qualifies to be surfaced in the deprecations as access api deprecation (Internal access + the requesting caller is external).
Are you sure? how are you testing this? I've tested locally and we do have unit and integration tests that ensure this behavior is working as expected.
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.
Retested and the changes look great! Both scenarios I was testing for work as expected.
With the restriction explicitly disabled in
kibana.yml
, we get a deprecation entry in UA when the API is called without the internal origin header and the usage counters increment, consistent with logging added in #195696.Steps to reproduce the scenario without using the example plugin:
kibana.yml
:server.restrictInternalApis: false
curl --location 'localhost:5601/abc/internal/kibana/global_settings' --header 'Content-Type: application/json' --header 'Accept-Encoding: gzip, deflate, br' --header 'kbn-xsrf: kibana' --header 'Kbn-Version: 9.0.0' --header 'Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ=='
Response should be similar to
Response should be similar to