-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Spaces NP Migration - Moving server to the LegacyAPI model #45382
Conversation
This comment has been minimized.
This comment has been minimized.
@@ -122,8 +118,7 @@ export const spaces = (kibana: Record<string, any>) => | |||
|
|||
async init(server: Server) { | |||
const kbnServer = (server as unknown) as KbnServer; | |||
const initializerContext = ({ | |||
legacyConfig: server.config(), |
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.
legacyConfig
moves to LegacyAPI.legacyConfig
usage: server.usage, | ||
tutorial: { | ||
addScopedTutorialContextFactory: server.addScopedTutorialContextFactory, | ||
const core: CoreSetup = ({ |
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.
Now that all legacy requirements have moved to LegacyAPI
, we can remove SpacesCoreSetup
in favor of the real CoreSetup
provided by NP
@@ -177,20 +158,31 @@ export const spaces = (kibana: Record<string, any>) => | |||
spaces: this, | |||
}; | |||
|
|||
const { spacesService, log } = await plugin(initializerContext).setup(core, plugins); | |||
const legacyRouter = server.route.bind(server); |
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.
The only component that couldn't move into LegacyAPI
is the legacy router, because the route definitions are created before the legacy API is initialized. This is very short lived though, as my next PR will move the server code to a real NP plugin, which will use the NP router.
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.
Btw, aren't you registering routes in setupLegacyComponents
that is called from registerLegacyAPI
?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💚 Build Succeeded |
Pinging @elastic/kibana-security |
ACK: will review today |
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.
Looks great! Just few minor nits/questions. Briefly tested locally and everything worked as expected, will test more thoroughly when draft turns into ready-to-review PR.
http.registerOnPreAuth(async function spacesOnPreAuthHandler( | ||
request: KibanaRequest, | ||
response: LifecycleResponseFactory, | ||
toolkit: OnPreAuthToolkit | ||
) { | ||
const serverBasePath: string = getLegacyAPI().legacyConfig.get('server.basePath'); |
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.
note: do we not use http.basePath.get
here intentionally (it seems it can work here as expected)? And if so, what is our plan/replacement in NP?
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 think it would technically work, as this interceptor hasn't rewritten the base path yet. It felt clearer to me that we were looking for the server's base path by reading this config directly, as opposed to relying on the implicit behavior of the execution order here.
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.
as opposed to relying on the implicit behavior of the execution order here.
Agree and it works for me, I'm just curious what we'll do in NP since we don't have anything except for basePath service there afaik.
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.
Yeah, good question.
@restrry what do you think? I know we have plans for exposing these values to the client, but do you know how we can get access to things like server.basePath
from within server-side plugins?
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.
For this specific property, we could make http.basePath.get
take an optional request
object, instead of a required one, but that won't solve the general problem.
The spaces plugin also needs access to the kibana.index
property too, in order to collect its telemetry metrics. Is this property exposed via a core service somewhere?
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.
Can't we just make serverBasePath
property of BasePath service as public readonly
to not confuse it with "request" base path, but at the same time still keep them together?
Having access to kibana.index
is listed as a dependency for the Security plugin as well. If it stays in Kibana plugin, then we need just Kibana NP plugin that would expose that for the time being (like we did with Timelion NP plugin and timelion.ui.enabled
config key).
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.
Can't we just make serverBasePath property of BasePath service as public readonly to not confuse it with "request" base path, but at the same time still keep them together?
Yep, that's a good idea!
Having access to kibana.index is listed as a dependency for the Security plugin as well. If it stays in Kibana plugin, then we need just Kibana NP plugin that would expose that for the time being (like we did with Timelion NP plugin and timelion.ui.enabled config key).
Yeah that'd work too. So maybe the answer here is that plugins and services will just expose the properties as needed, rather than introducing a whitelisting system for the yml
entries in general.
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've created #45991
x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.ts
Outdated
Show resolved
Hide resolved
@@ -11,10 +11,12 @@ import { | |||
HttpServiceSetup, |
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.
note: not related to this PR, but it seems these tests should be within a integration_tests
folder (these are run with jest_integration.js
and have a larger default timeout so that you don't need to increase it explicitly).
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.
Ah I didn't know this -- thanks!
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.
Build failed due to timeout when I did this and removed the explicit timeout increases :(
https://kibana-ci.elastic.co/job/elastic+kibana+pull-request/2706/
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.
Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.
Hmm, maybe they weren't picked up by jest_integration
for some reason? Are they run when you use node scripts/jest_integration.js
locally?
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.
hmm, apparently not:
elastic-mbp:x-pack larry$ node scripts/jest_integration.js ./legacy/plugins/spaces/server/lib/request_interceptors/
/Users/larry/repos/kibana/x-pack/scripts/jest_integration.js:19
throw new Error(`
^
Error:
jest_integration tests have been disabled because of a flaky failure in the
example integration test: https://github.com/elastic/kibana/issues/32795#issuecomment-471585274
when un-skipping these tests make sure to uncomment test/scripts/jenkins_xpack.sh lines 33-37
at Object.<anonymous> (/Users/larry/repos/kibana/x-pack/scripts/jest_integration.js:19:7)
at Module._compile (internal/modules/cjs/loader.js:689:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
at Module.load (internal/modules/cjs/loader.js:599:32)
at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
at Function.Module._load (internal/modules/cjs/loader.js:530:3)
at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
at startup (internal/bootstrap/node.js:283:19)
at bootstrapNodeJSCore (internal/bootstrap/node.js:743:3)
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.
Ugh, didn't know that 😭 Sorry for wasting your time on that
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.
No worries! I'll just revert it
x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts
Show resolved
Hide resolved
💔 Build Failed |
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.
👍 For platform changes
@azasypkin ready for another review whenever you get a chance |
ACK: will review today |
@azasypkin you can wait on the review -- I thought through a couple of different scenarios that I want to verify before you spend more time on this. |
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.
@azasypkin you can wait on the review -- I thought through a couple of different scenarios that I want to verify before you spend more time on this.
Ha, actually code LGTM, just few nits. Let me know when I can start testing it locally then.
x-pack/legacy/plugins/spaces/server/lib/get_spaces_usage_collector.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/spaces/server/lib/get_spaces_usage_collector.test.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts
Outdated
Show resolved
Hide resolved
@@ -177,20 +158,31 @@ export const spaces = (kibana: Record<string, any>) => | |||
spaces: this, | |||
}; | |||
|
|||
const { spacesService, log } = await plugin(initializerContext).setup(core, plugins); | |||
const legacyRouter = server.route.bind(server); |
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.
Btw, aren't you registering routes in setupLegacyComponents
that is called from registerLegacyAPI
?
x-pack/legacy/plugins/spaces/server/new_platform/spaces_service/spaces_service.test.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/spaces/server/routes/api/__fixtures__/create_test_handler.ts
Outdated
Show resolved
Hide resolved
); | ||
|
||
return { | ||
available, | ||
enabled: spacesAvailableAndEnabled, // similar behavior as _xpack API in ES | ||
enabled: available, |
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.
It's not technically necessary to have both available
and enabled
here, but I chose to keep it in so that telemetry consumers can have a consistent schema when searching for spaces-related statistics.
💚 Build Succeeded |
@azasypkin now this is ready for you :) |
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.
LGTM! Tested locally and haven't noticed anything suspicious. Just two notes:
- We return 404 when
default
is used as a space ID in URL, but redirect to space selector when space isn't known. It probably works as intended since we do this inmaster
, just got curious - When users delete the last non-default space we redirect them to space selector in this PR (with only
Default
space to choose from), but to the root (without space selector) inmaster
. It's super edge case and doesn't feel like a big deal though.
That is curious! I'm glad I didn't make it any worse, but I'll see if I can track that down.
Good catch! I agree it's an edge case, but I'd still like to solve this. I think it'll be best to tackle this once #45991 merges, since at that point I'll be able to send users back to the root path again without introducing another legacy shim. |
@elasticmachine update branch |
💚 Build Succeeded |
Summary
Inspired by @azasypkin's work on the security plugin migration, this updates the spaces server code to use the
LegacyAPI
approach to better delineate what we have left to migrate. It also allowed for additional cleanup so that I can move the server code to a real NP plugin in a followup PR.Notable changes:
The space selector screen is no longer rendered at the Kibana root, but rather at
/spaces/space_selector
. Having a dedicated route for this allowed theonPostAuth
request interceptor to move to the NP, as it was previously rendering a hidden app inside of the interceptor. Now, the interceptor redirects to a legacy route (/spaces/space_selector
) which is responsible for rendering the view.