Disable management plugins using contextRef #160671
Conversation
| enabled: schema.conditional( | ||
| schema.contextRef('serverless'), | ||
| true, | ||
| schema.boolean({ defaultValue: true }), |
There was a problem hiding this comment.
By taking this approach, we also set the setting to false in the serverless.yml file. This would give us a clear view of all of the plugins that are disabled. Alternatively, we can set the default value to false here and then remove the configuration from the serverless.yml file.
There was a problem hiding this comment.
++ I think the approach of explicitly setting false in the serverless.yml makes things easier to reason about and prevents our serverless configs from spreading out into more places, even if it seems counterintuitive to default to true here. You could also consider adding a comment to these to make that even clearer for folks in the future.
There was a problem hiding this comment.
Thanks! Good idea about adding a comment 👍
| @@ -9,7 +9,12 @@ import { schema, TypeOf } from '@kbn/config-schema'; | |||
| import { PluginConfigDescriptor } from '@kbn/core-plugins-server'; | |||
|
|
|||
| const configSchema = schema.object({ | |||
| enabled: schema.boolean({ defaultValue: true }), | |||
| enabled: schema.conditional( | |||
There was a problem hiding this comment.
Unfortunately, this breaks the common functional tests (I think related to work done via #159272). If I run the tests using the common config:
node scripts/functional_tests_server.js --config test_serverless/api_integration/test_suites/common/config.ts
Then, I get the following error:
FATAL Error: [config validation of [xpack.cloud_integrations.data_migration].enabled]: a value wasn't expected to be present
There was a problem hiding this comment.
Yeah while this approach makes things safer in terms of minimizing our API surface area, the tradeoff is that serverless & self-managed need to be tested separately & common tests won't really work anymore.
There was a problem hiding this comment.
This is actually related to the common serverless test suite. It looks like in #159272, a change was made so that the common suite runs directly with the serverless.yml file instead of specifying a project type. See: https://github.com/elastic/kibana/pull/159272/files#diff-818fc0329318f82bcee0547dfb8edb3800cce36ea3fad7c62d58363197d91366R28
There was a problem hiding this comment.
Ah, I misunderstood your comment. Looking at the code I'm wondering if what's throwing it off is that we aren't getting a --serverless cli flag at all when running the common tests. Ultimately that's what we look at in the environment to set the context ref accordingly, so schema.contextRef('serverless') is likely returning false.
Poking around in src/cli/serve.js it appears we try to read from the provided config files as a backup method to determine if we're in serverless mode (if the --serverless flag isn't explicitly passed), however I think we are checking for a top-level serverless value to be in the yml files, which I don't think we provide anywhere... so we may have a bug in that logic.
FYI @jbudz, this might be a bug, but keep me honest if I'm misreading things :)
There was a problem hiding this comment.
Sorry, should have clarified 😅
Ah, I see. @jbudz let me know if you'd like me to open up an issue or if I can help in any way.
There was a problem hiding this comment.
The tests were fixed via #160783
|
@clintandrewhall @lukeelmers I would appreciate if you both could take a look at this PR and provide some feedback on the approach. Thank you! |
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
alisonelizabeth
added
release_note:skip
|
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
There was a problem hiding this comment.
Thanks for working on this @alisonelizabeth! Code changes look good to me and I tested locally to verify that the plugins are enabled in self-managed and disabled in serverless.
Just a question: in #158186, we disabled the UIs of Users, Roles, and Role Mappings using contextRef, but we set their default values in serverless context to be false. Do we want to change them to true so that they are consistent with these config settings? (In case we change them to true, we would also need to set these config settings to false in serverless.yml)
|
Thanks for the review @ElenaStoeva!
Great catch. Yes, I think it makes sense to change the approach for the security UIs as well. I might defer this to a separate PR since I'll be out on PTO and do not have time to address it before I leave. |
kibanamachine
added
v8.10.0
backport:skip
No worries! I just opened a separate PR for these changes. |
…#162187) This is a follow-up to #160671, where the Management plugins were disabled using `contextRef`. The configs for disabling the UI of the security management plugins were added in #158186. In this PR, they are changed so that they follow the same convention for disabling the Management plugins - setting the default values of the configs to `true` and explicitly setting them to `false` in the `serverless.yml` file. This way, we have a clear view in `serverless.yml` of all plugins/functionalities that have been disabled.
…elastic#162187) This is a follow-up to elastic#160671, where the Management plugins were disabled using `contextRef`. The configs for disabling the UI of the security management plugins were added in elastic#158186. In this PR, they are changed so that they follow the same convention for disabling the Management plugins - setting the default values of the configs to `true` and explicitly setting them to `false` in the `serverless.yml` file. This way, we have a clear view in `serverless.yml` of all plugins/functionalities that have been disabled.
This PR updates work that was done to disable certain plugins in Stack Management for serverless. The configs now leverage the schema.contextRef('serverless') check to prevent the configs from leaking to self-managed.