-
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
Create the ftrSoApis FTR plugin #149188
Create the ftrSoApis FTR plugin #149188
Conversation
Pinging @elastic/kibana-core (Team:Core) |
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.
At first I thought this sounded like a great idea, but then I remembered how much testing we do against ESS Kibana Instances, which won't and can't have the ftrSoApis
plugin. If we make this change in this way, all tests which interact with saved objects will not be runnable against production instances. Additionally, I'm really concerned about the amount of coverage we're shifting away from the public saved objects APIs and to internal versions.
IMO the ideal way for functional tests to run is with APIs which are actually publicly available, even if they require special roles or something that are only enabled in staging environments or something. Is there any other option here that doesn't have these downsides?
We discussed our options in #147150 (comment) and the pros and cons of each, keeping timing in mind too. Of these, this PR takes the less ideal approach but we were hoping it would work, at least, for now. |
ATM, Core doesn't know about the user making a request, and restricting access to such 'public but not normally accessible' APIs could be tricky to implement directly in Core. Are you thinking about some public plugin that's restricted to a special role and hidden behind a tag? This would map to something along the lines that @rudolf suggested:
I'm really not too sure how we're going to prevent dev requirements from leaking into Prod. @lukeelmers can you think of a novel approach? |
Avoiding this is the problem. We run our tests against production instances, which means that we can't have test-only APIs |
Yeah... This is absolutely correct. I thought about it this week-end tbh. We talked about it a while ago but somehow forgot about it when we opened the dedicated #148412 issue.
I'm going to start by saying that the saved object APIs will eventually be removed in a not-so-far futur. That is a mere fact, and we need to find a solution. Just declining all our options isn't doing to do anything. Now, We could effectively expose these test only APIs in a 'standard'/'normal' plugin (just move the plugin introduced in this PR to It would have to upside to switch from using 'public' ( In term of security it would be fine too, standard SO security would apply, so a user would only be able to manipulate the types they have access to. We could also eventually disable that plugin by default, so that all Cloud would need to do it to enable it during testing (via a simple configuration property, e.g
@spalger I though you just said these tests were supposed to be able to run on any production environment?
The 'special' APIs are using the SOR under the hood. We're not shifting that much, so I'm not really sure to see how this is an issue. We're only adapting the underlying implementation of one of FTR's service. Mind to elaborate what your concerns are about? cc @rudolf @lukeelmers : do you know who could help on Cloud side to decide if a solution is acceptable for them or not? Do you think it would be acceptable to just have these FTR apis be exposed from a normal plugin and always enabled (which is basically our easiest option while still being secured)? Do we need to do more? |
So we discussed with @spalger sync, and he's fine having these dedicated FTR APIs as long as they can easily (understand: with a config setting) be disabled or enabled. Note that in his opinion, he thinks it would make more sense to have those APIs exposed from Core rather than from a plugin, but he would be fine either way. The only remaining question now is regarding if this 'FTR APIs' plugin should be enabled by default, or if it would be acceptable to have it disabled by default and have Cloud enable it for testing. Note that we could start with this plugin being just enabled to avoid breaking anything, so that this PR get merged faster, and then reach out to cloud to figure out if we can switch to have it disabled by default. |
I think we all agree none of our options here are great, but the point about testing on ESS deployments is definitely something we overlooked. The approach Pierre outlined seems reasonable (have a separate plugin exposing APIs that are specifically marked as internal). I definitely prefer the idea of having something disable-able -- even if it is only done as a follow up step -- so that those routes aren't registered on every deployment. I know there is already a thread discussing how much of a lift this would be on the QA side to configure our cloud FTR runs to enable a special plugin. To prevent confusion in the case of someone trying to run ad hoc tests on a miscellaneous ESS deployment (which tbh I don't know how often we do this today), it might be helpful if we found a way to write the tests so they can fail with a specific error if they detect the plugin isn't available. "You need to set foo.enabled on this deployment before running tests against it" It'd make me nervous to do any of this as a long-term/permanent solution, but as this is only intended to exist as an aid to help folks transition to the new SO architecture in the mid-term, I think the tradeoffs are okay. |
@elasticmachine merge upstream |
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 I've made my points clear, I'll leave the decision making up to Core and just say that the operations specific code changes LGTM
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 looks great, thanks for taking care of this!
I've left a few comments/questions but nothing that halts merging.
src/plugins/ftr_apis/README.md
Outdated
@@ -0,0 +1,3 @@ | |||
# ftrApis plugin | |||
|
|||
Set of APIs used internally by the FTR. |
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.
nit: We should be more specific about what APIs these are and that they are only for ftr testing. e.g.
Set of APIs for the kbn server's saved object's client used internally by the functional test runner
or, more concisely:
Set of kbnServer saved objects' client APIs used internally by the functional test runner
), | ||
}, | ||
}, | ||
catchAndReturnBoomErrors(async (ctx, req, res) => { |
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.
FYI: I asked about catchAndReturnBoomErrors
and apparently it should be dead / unecessary code and safe to remove. We're probably less likely to replace these APIs in the short to medium term so should we remove it?
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, I wanted to, but it's actually used by the FTR KbnClient. Removing it causes some errors to not be properly wrapped, causing a lot of test suites to fail.
import { registerGetRoute } from './get'; | ||
import { registerUpdateRoute } from './update'; | ||
|
||
export const registerKbnClientSoRoutes = (router: IRouter) => { |
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.
If this plugin sticks around for a while, should we consider versioning it? Even if we keep the plugin HTTP API's fixed at some particular Kibana version, it's better than some arbitrary API that might be interpreted to "work" for all time.
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.
In theory, internal APIs won't need versioning, at least for now (plus, we're still waiting for a spec for API versioning...).
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
## Summary Follow-up of #149188 - Use the bulkDelete API for `KbnClientSavedObjects.bulkDelete` - Create a dedicated `/_clean` endpoint for `KbnClientSavedObjects.clean` and `KbnClientSavedObjects.cleanStandardList` --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
## Summary Fix elastic#148412 More and more SO types will not be accessible from the HTTP APIs (either `hidden:true` or `hiddenFromHTTPApis: true`). However, the FTR SO client (`KbnClientSavedObjects`) still needs to be able to access and manipulate all SO types. This PR introduces a `ftrSoApis` plugin that is loaded for all FTR suites. This plugin exposes SO APIs that are used by the FTR client instead of the public SO HTTP APIs. These APIs are configured to know about all types, even hidden ones. Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
## Summary Follow-up of elastic#149188 - Use the bulkDelete API for `KbnClientSavedObjects.bulkDelete` - Create a dedicated `/_clean` endpoint for `KbnClientSavedObjects.clean` and `KbnClientSavedObjects.cleanStandardList` --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Summary
Fix #148412
More and more SO types will not be accessible from the HTTP APIs (either
hidden:true
orhiddenFromHTTPApis: true
).However, the FTR SO client (
KbnClientSavedObjects
) still needs to be able to access and manipulate all SO types.This PR introduces a
ftrSoApis
plugin that is loaded for all FTR suites. This plugin exposes SO APIs that are used by the FTR client instead of the public SO HTTP APIs. These APIs are configured to know about all types, even hidden ones.