-
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
Add Mock IDP login page and role switcher #172257
Add Mock IDP login page and role switcher #172257
Conversation
…heymann/kibana into serverless-role-selector-ui
…heymann/kibana into serverless-role-selector-ui
…heymann/kibana into serverless-role-selector-ui
…ess-role-selector-ui
/** | ||
* To respond with HTML page bootstrapping Kibana application without retrieving user-specific information. | ||
* **Note:** | ||
* - Your client-side JavaScript bundle will only be loaded on an anonymous page if `plugin.enabledOnAnonymousPages` is enabled in your plugin's `kibana.jsonc` manifest file. | ||
* - You will also need to register the route serving your anonymous app with the `coreSetup.http.anonymousPaths` service in your plugin's client-side `setup` method. | ||
* */ |
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 can feel the pain and suffering in this comment addition 😄
@@ -97,6 +97,7 @@ pageLoadAssetSize: | |||
mapsEms: 26072 | |||
metricsDataAccess: 73287 | |||
ml: 82187 | |||
mockIdpPlugin: 30000 |
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.
Mind if we add a ci:cloud-deploy
label? This seems like a gap in the packaging system with a development plugin included in production.
I can see we're skipping the server load but I'm not sure how the client is going to behave.
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.
Oh..it's not a development plugin but it's pulling in mock-idp-utils which is devOnly. Can you help me understand the workflow in production?
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.
@jbudz Both plugins are development-only and are not used in production. The only reason they're broken up into two separate plugins is to avoid cyclic TS projects.
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.
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.
Both plugins already have devOnly
enabled:
Is something not 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.
Is opts.dev enabled for these QA cloud deployments?
It's not. We're using the same build script we use for releases, i.e. the node scripts/build --release
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.
@pgayvallet Is there any other mechanism in place where Kibana would load in this plugin despite the dev
CLI arg not being set?
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.
@thomheymann I apologize, I missed the ping. We have one CI script (the SO schema check) that loads Kibana while forcing all plugins to be enabled. but I don't think it would be the issue here.
However, fwiw, I'm not sure the optimizer / bundler ever supported evicting "devOnly" plugins @jbudz?
Having the plugin disabled in production for sure. Not bundling specific plugins into the distributable? AFAIK this was never implemented. I think the bundler will consider all plugins as production code?
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.
Looking at your PR and at the @kbn/mock-idp-plugin
, I'm not sure how we got to the conclusion that this plugin is disabled by default and/or not bundled for production?
The plugin discovery system is now plugged on the package list, so it's not because a plugin in under package
instead of /src/plugins
or /x-pack/plugins
that it will be disabled by default. All packages with a kibana.jsonc
file will be considered as a plugin and included in the plugin list automatically (and then loaded).
Also IIRC, the bundler will consider all plugins as production code, meaning that they WILL be included (ignoring, I assume, the devOnly
flag that is supposed to work on packages, not plugins)
@jbudz will have to confirm those assumptions of course, bundling/bazel is Operations' territory, not Core's. But if that was the case, the only approach I would think of is to add a configuration to your plugin with a schema forcing it to be disabled for production, like
enabled: schema.conditional(
schema.contextRef('dist'),
true,
schema.boolean({ defaultValue: false, validate: (raw) => {
if(raw !== false) { throw "mock idp plugin cannot be enabled in production"}
} }),
schema.boolean({ defaultValue: true })
),
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.
buildkite test this |
@elastic/kibana-operations @elastic/kibana-qa @elastic/kibana-security PR is ready for your review! |
## Summary Both cloud and serverless deployments of elastic#172257 are failing. These are small additions to help investigation, or fix potential errors.
@@ -255,7 +255,7 @@ export const BuildPackages: Task = { | |||
Path.resolve(pkgDistPath, 'package-map.json'), | |||
JSON.stringify( | |||
packages | |||
.filter((p) => p.isPlugin()) | |||
.filter((p) => p.isPlugin() && !p.isDevOnly()) |
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: without this change, @kbn/mock-idp-plugin
(dev-only) is included into package-map.json
even though it's not actually bundled into the build. Let me know if you have any better ideas how to solve 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.
Is this not going to interfere with other builds?
Is it related to this thread: #172257 (comment)?
@mistic , do you have any opinions 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 I suggest we change that line https://github.com/elastic/kibana/blob/main/src/dev/build/lib/config.ts#L249 with (!p.isPlugin() || (this.pluginFilter(p) && !p.isDevOnly()))
instead of adding the check on this line
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.
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.
Code LGTM. Tested locally for all the projects, works perfectly.
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.
Ops related changes 👍
@@ -255,7 +255,7 @@ export const BuildPackages: Task = { | |||
Path.resolve(pkgDistPath, 'package-map.json'), | |||
JSON.stringify( | |||
packages | |||
.filter((p) => p.isPlugin()) | |||
.filter((p) => p.isPlugin() && !p.isDevOnly()) |
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.
.filter((p) => p.isPlugin() && !p.isDevOnly()) | |
.filter((p) => p.isPlugin()) |
💔 Build Failed
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Canvas Sharable Runtime
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Resolves #166340 |
Follow up to #170852
Summary
Adds login page and role switcher for development-only identity provider.
Screenshots
Login page
Role switcher
Testing
SAML is only supported by ES when running in SSL mode.