Skip to content
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

Merged
merged 18 commits into from
Jan 26, 2023

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Jan 19, 2023

Summary

Fix #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.

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Saved Objects Feature:Functional Testing release_note:skip Skip the PR/issue when compiling release notes v8.7.0 labels Jan 19, 2023
@pgayvallet pgayvallet marked this pull request as ready for review January 19, 2023 16:27
@pgayvallet pgayvallet requested a review from a team as a code owner January 19, 2023 16:27
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@pgayvallet pgayvallet requested a review from a team January 19, 2023 16:27
@pgayvallet pgayvallet changed the title Create the kbnSoApis FTR plugin Create the ftrSoApis FTR plugin Jan 19, 2023
@TinaHeiligers TinaHeiligers self-requested a review January 19, 2023 16:33
Copy link
Contributor

@spalger spalger left a 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?

@TinaHeiligers
Copy link
Contributor

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.
The other options are to add an override to each public SO HTTP API but that would "leak" dev requirements into Prod.

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Jan 19, 2023

APIs which are actually publicly available, even if they require special roles or something that are only enabled in staging environments or something

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:

In a dedicated plugin only enabled during testing
With a tag similar to 'access:migrateSavedObjects' to prevent production usage

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?

@spalger
Copy link
Contributor

spalger commented Jan 23, 2023

I'm really not too sure how we're going to prevent dev requirements from leaking into Prod

Avoiding this is the problem. We run our tests against production instances, which means that we can't have test-only APIs

@pgayvallet
Copy link
Contributor Author

pgayvallet commented Jan 23, 2023

I remembered how much testing we do against ESS Kibana Instances

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.

Is there any other option here that doesn't have these downsides?

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 /src/plugins), so that they are automatically available in any production environment. I kinda dislike it, but it would work.

It would have to upside to switch from using 'public' (/api/) endpoint in favor of internal (/internal) ones, so even if exposed on all deployments, we wouldn't have to consider those FTR-specific endpoint as 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 ftrApis.enable: true). It doesn't sounds like that much of a burden, so maybe it would be acceptable for Cloud? We probably need to confirm that with them though.

even if they require special roles or something that are only enabled in staging environments or something

@spalger I though you just said these tests were supposed to be able to run on any production environment?

I'm really concerned about the amount of coverage we're shifting away from the public saved objects APIs and to internal versions

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?

@pgayvallet
Copy link
Contributor Author

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.

@lukeelmers
Copy link
Member

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.

@pgayvallet
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@spalger spalger left a 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

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a 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.

@@ -0,0 +1,3 @@
# ftrApis plugin

Set of APIs used internally by the FTR.
Copy link
Contributor

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) => {
Copy link
Contributor

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?

Copy link
Contributor Author

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) => {
Copy link
Contributor

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.

Copy link
Contributor Author

@pgayvallet pgayvallet Jan 26, 2023

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...).

@pgayvallet pgayvallet enabled auto-merge (squash) January 26, 2023 07:36
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pgayvallet pgayvallet merged commit cd9a53f into elastic:main Jan 26, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 26, 2023
pgayvallet added a commit that referenced this pull request Jan 30, 2023
## 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>
kqualters-elastic pushed a commit to kqualters-elastic/kibana that referenced this pull request Feb 6, 2023
## 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>
kqualters-elastic pushed a commit to kqualters-elastic/kibana that referenced this pull request Feb 6, 2023
## 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Functional Testing Feature:Saved Objects release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Develop a custom plugin that exposes dedicated endpoints for the FTR SO services to use.
7 participants