Saved Objects: Provide ability to remove SO type from global SO HTTP API without hiding from the client

[TinaHeiligers]

Plugins currently use hidden=true to prevent exposing saved objects to the SO HTTP APIs.

However, this also hides their SO types from the server-side SavedObjectsClient which requires them to explicitly enable these types by creating a new client with includedHiddenTypes. This means plugins wanting to adopt an API layer must refactor their code. At the same time, hiding the type from the client doesn’t really benefit plugins.

Can we make plugin authors' lives easier by introducing a new saved object type option that only stops exposing a type over the SO HTTP APIs?

Scope for this issue:

• Flesh out a design for how the two options (hidden and hideFromHttpApi) might interact with each other.
• How would we introduce this so that we don’t change the behavior of any existing SO types?
• Ensure the new option still works with the delete functionality of Saved Objects Management (ATM, hidden types throw an error when trying to delete them from SOM when they are exposed in SOM).

[elasticmachine]

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

[TinaHeiligers]

In sprint planning the question was raised about how deletes might work if a saved object is no longer exposed from the generic HTTP APIs.

Similar to hidden=true, which today just shows an error on the UI when you delete it, we don’t think it should be possible to delete a type from the saved object management screen at all. For now, we propose to keep this functionality for types that aren’t hidden=true or hiddenFromHttp=true, but this is our proposal for the long term.

We are tracking the request for custom delete logic in Ensuring referential integrity for saved objects deleted from saved objects management and if teams need this for hidden/hiddenFromHttp types we’ll treat this as a new feature request.

We should work with global experience to agree about the long term vision for deleting.

[TinaHeiligers]

Proposal: (Updated)

# skip to the bottom for a summary #

hiddenFromHttp option

Introduce a new SavedObjectsType option to hide a type from the HTTP API:

export interface SavedObjectsType<Attributes = any> {
  name: string;
  hidden: boolean;
  hiddenFromHttpApis?: boolean; // option to hide SO from HTTP API
  namespaceType: SavedObjectsNamespaceType;
...
}

The type registry exposes a method that indicates if a type should be exposed to the HTTP API:

export interface ISavedObjectTypeRegistry {
  ...
   hiddenFromHttpApis(type: string): boolean;
  ...
}

HTTP API behavior

HTTP API's take the additional type option into account.
For example, the get SO route is registered as:

interface RouteDependencies {
  coreUsageData: InternalCoreUsageDataSetup;
  typeRegistry: ISavedObjectTypeRegistry; // expose typeRegistry to HTTP routes
}

export const registerGetRoute = (
  router: InternalSavedObjectRouter,
  { coreUsageData }: RouteDependencies
) => {
  router.get(
    {
      path: '/{type}/{id}',
      validate: {
        params: schema.object({
          type: schema.string(),
          id: schema.string(),
        }),
      },
    },
catchAndReturnBoomErrors(async (context, req, res) => {
      const { type, id } = req.params;

      const usageStatsClient = coreUsageData.getClient();
      usageStatsClient.incrementSavedObjectsGet({ request: req }).catch(() => {});

      const { savedObjects } = await context.core;
      // Only implement blocking behavior for visible types, hidden types are taken care of in the repository
      // Assumes hiddenFromHttpApis can only be configured for visible types ('hidden:false')
      if (savedObjects.typeRegistry.isHiddenFromHttpApis(type)) {
        throw SavedObjectsErrorHelpers.createUnsupportedTypeError(type); // visible type is not exposed to the HTTP API
      }

      const object = await savedObjects.client.get(type, id);
      return res.ok({ body: object });
    })
  );
};

Interaction between hidden and hiddenFromHttpApis options

We want the new option to have a similar effect of declaring a type to be hidden BUT we don't want to hide a type from the client.

At the same time, we also need to make sure there aren't any unintentional side effects if hidden and hiddenFromHttpApis are declared for the same type:

option	global SO client	HTTP	remarks
hidden=false, hiddenFromHttpApis=true	Y	X	what we want to achieve
hidden=true, hiddenFromHttpApis=true	X	X	no effect, except for bulkDelete with force: true (kbnArchiver dependency)
hidden=false, hiddenFromHttpApis=false	Y	Y	behavior for visible types today
hidden=true, hiddenFromHttpApis=false	X	X	doesn't make sense

Declaring hiddenFromHttpApis for already hidden types either has no effect or doesn't make sense and could lead to unexpected (possibly unintended?) behavior. As @rudolf pointed out, we'd also likely break test suites since kbnArchiver depends on bulkDelete and the way force works today.

To keep things simple, I think we'd introduce the fewest side effects by only allowing hiddenFromHttpApis to be configured for non-hidden types and have the registerType API throw a validation error for types registered with hidden:true and hiddenFromHttpApis !== undefined:

const validateType = ({ name, management, hidden, hiddenFromHttpApis }: SavedObjectsType) => {
  if (management) {
	...
    if (management.visibleInManagement !== undefined && !management.importableAndExportable) {
    	...
    }
  }
    if (hidden && hiddenFromHttpApis !== undefined) {
      throw new Error(
        `Type ${name}: 'hiddenFromHttpApis' cannot be defined when specifying 'hidden: true'`
      );
    }
};

How do we handle the new parameter in the bulk APIs?

Keep it simple:

Don't allow any bulk APIs to work with non-standard SO types (hidden: false && hiddenFromHttpApis: undefined // <- defaults):
For example, bulkGet:

export const registerBulkGetRoute = (
  router: InternalSavedObjectRouter,
  { coreUsageData }: RouteDependencies
) => {
  router.post(
    {
      path: '/_bulk_get',
      ...
    },
    catchAndReturnBoomErrors(async (context, req, res) => {
      let itemsToGet = req.body;
      const usageStatsClient = coreUsageData.getClient();
      usageStatsClient.incrementSavedObjectsBulkGet({ request: req }).catch(() => {});

      const { getClient, typeRegistry } = (await context.core).savedObjects;

      const allTypesVisibleToHttpApis = typeRegistry
        .getVisibleToHttpApisTypes() // only types with `hidden:false` && `hiddenFromHttpApis:false`
        .map((fullType) => fullType.name);      
      throwOnGloballyHiddenTypes(
          allTypesVisibleToHttpApis,
          itemsToGet.map(({ type }) => type)
        );
      }
      const result = await getClient().bulkGet([...itemsToGet]);
      return res.ok({ body: result });
    })
  );
};

Limited overrides:

If we need to be a bit more lenient, we could consider adding an optional query param to override this behavior, much in the same way using force:true in bulkDelete) force-deletes all objects in multiple namespaces:

export const registerBulkGetRoute = (
  router: InternalSavedObjectRouter,
  { coreUsageData }: RouteDependencies
) => {
  router.post(
    {
      path: '/_bulk_get',
      validate: {
        query: schema.object({ ignoreHiddenFromHttpApis: schema.maybe(schema.boolean()) }),
        body: schema.arrayOf(
          schema.object({
          ...
          })
        ),
      },
    },
    catchAndReturnBoomErrors(async (context, req, res) => {
      let itemsToGet = req.body;
       ...
      if (!ignoreHiddenFromHttpApis) {
        throwOnGloballyHiddenTypes(allTypesVisibleToHttpApis, itemsToGet.map(({ type }) => type));
      }
      const result = await client.bulkGet([...itemsToGet]);
      return res.ok({ body: result });
    })
  );
};

The added benefit is that it will allow kbnArchiver to keep the same behavior for bulkDelete, thereby not breaking tests!

Other options:

Other options I explored:

• filter out the types hidden from the HTTP API and pass the rest along, adding error to the response body for those types (mutate response from client)
• config to hide specific apis per type:

enum HttpApiToHide {
  get = 'GET',
  update = 'UPDATE',
  delete = 'DELETE',
  resolve = 'RESOLVE',
  ...
  bulk_get = 'BULK_GET'
  '*' = 'ALL'
}
SavedObjectsType.hiddenFromHTTPApis: HttpApiToHide[]

• a few others that didn't go very far

It becomes really complicated really fast, leads to unpredictable behavior, and adds more inconsistency in an already inconsistent system (SOR handling of hidden types in bulk APIs). Then of course, there's the added complexity of private saved objects that are on the cards.

Our safest bet is to leave customized behavior and configuration up to saved objects' type owners, which is where we're trying to go.

Summary:

After all that, here's what I propose:

TypeRegistry:

• types with hidden:true cannot set the hiddenFromHttpApis flag. Ensured through validation on type registration.
• for hidden:false types, SavedObjectTypeRegistry.isHiddenFromHttpApis defaults to false. Note: calling the method will return false for hidden types too, as it doesn't take the hidden property into account. The method is not a setter though, so we should be ok here.

Non-bulk Routes

• Routes where we could use the simple blocking implementation: get, update, create, resolve, delete (throw when the type has hiddenFromHttpApis: true). For delete, that would override the force behavior too.
• More logic is needed for find: any types provided to the request that aren't both visible and visible to the http APIS will throw an error in the handler.
• import/export: support for hidden types was added recently. Assuming that we don't then want to remove support for visible types, we can leave these untouched.
• Uneffected routes: '/_migrate', '/_resolve_import_errors', '/deprecations/_delete_unknown_types', legacy import/exports

Bulk Routes

• throw on any saved object type in the payload with hiddenFromHttpApis:true (default)
• query param to override the blocking mechanisms and risk the consequences of defaulting to the response from the SO respository.
• Use the query param to ensure we don't break tests (kbnArchiver can override the new behavior)

[TinaHeiligers]

@elastic/kibana-core Please review the proposal and LMK WYT

[pgayvallet]

The overall proposal makes sense to me.

Minor NITs / remarks:

• I would personally prefer something slightly more explicit:hiddenFromHttpApis, but I would be fine either way

• I think I would enforce consistency by having the registerType API throws if a consumer registers a type with hidden: true, hiddenFromApi: false, as it doesn't functionally makes sense.

• We probably want some details of how bulk APIs would work with this param. E.g calling the bulkGet endpoint with a mix of types being hiddenFromHttp: true and hiddenFromHttp: false/undefined (especially given we may be fairly inconsistent on the way we're handling the hidden parameter for some of our bulk APIs in the SOR)

[afharo]

I wonder if we should make more explicit that we want most/all SOs to be hiddenFromHttpApis: true.

Should we flip the definition to exposeOnHttpApis? We could also define it as @deprecated from day one because we want everyone to migrate from using this.

It requires a noisier PR, but I think it is closer to what we want to achieve in the long run: delete the HTTP APIs (and the option that we are adding today).

WDYT?

[TinaHeiligers]

I would personally prefer something slightly more explicit:hiddenFromHttpApis.

👍

enforce consistency by having the registerType API throws if a consumer registers a type with hidden: true, hiddenFromApi: false

👍

details of how bulk APIs would work with this param...

We might want to leverage something similar to how delete and bulkDelete handle forced updates (and take the new option into account with force too). That being said, we could go with the simplest option of all-or-nothing in that we simply don't allow mutating behavior (deletion) with any types other than "normal" saved objects that aren't hidden and aren't hidden from the HTTP API's.

The trouble with that of course is forcing teams to implement a list view where SO's can be deleted from the UI. They'd have to implement a list view anyway and expose a delete method.

Should we flip the definition to exposeOnHttpApis? We could also define it as @deprecated from day one because we want everyone to migrate from using this.

It's worth thinking about.

[rudolf]

We need to double check if hiddenFromHttpApi saved object types might break test suites using kbnArchiver (because of the dependency on bulkDelete)

[TinaHeiligers]

In addition, we agreed as a team to take the approach of flipping the definition from hiddeFromHttpApis to exposeToHttpApis and define it as @deprecated from day one. The default value for non-hidden types will be exposeToHttpApis: true.

Update: Introducing a new parameter that's true by default is not backward compatible, whereas an optional parameter that defaults to false/undefined is. As such, I strongly propose we go forward with the original proposal with the modifications suggested in #147150 (comment).

[TinaHeiligers]

@rudolf The SO HTTP API routes all still implement catchAndReturnBoomErrors, even though the TODO was handled way back in 2020. Do we still plan on removing catchAndReturnBoomErrors or has that plan changed?

[TinaHeiligers]

@elastic/kibana-core I've updated the design proposal after exploring more. The short version is summarized at the end. PTAL.

[pgayvallet]

Overall the proposal looks good to me.

I think the only major concern I have would be about the ignoreHiddenFromHttpApis option that we would be putting in place to work around the fact that the FTR kbnArchiver needs to be able to load and delete any arbitrary type.

The reason I dislike this part, in addition to the fact that we would kinda be leaking 'dev/test-only' options in production, is that long term, the goal is to fully get rid of these global SO HTTP APIs, so this workaround will not work then. Which is why ihmo, we should think of a long term solution right now to avoid being blocked later.

Or at least, if we go with the work around now for simplicity's sake, we should at least have an idea of how we would address this mid/long term when we'll effectively remove those APIs.

A solution I've been thinking about was to develop an kbnArchiver custom kibana plugin that would expose dedicated endpoints for the FTR SO import service to leverage (instead of using the global SO APIs directly). These endpoints would, under the hood, use a custom SO client that would use includedHiddenTypes: ALL. We would need to adapt the FTR test runner to always include this custom plugin (to avoid adapting all our inherited FTR configs to manually specifies it).

The major concern about this proposal is to check if that could work with the way cloud is running our FTR test suites against their environment, as that means they would be forced to also install / use this custom kbnArchiver plugin.

@rudolf wdyt?

And just another nitpick:

types with hidden:true cannot set the hiddenFromHttpApis flag

I would just throw if the type explicitly specifies hidden: true, hiddenFromHttpApis: false and accepts other permutations, as they're technically all valid.

[TinaHeiligers]

I think the only major concern I have would be about the ignoreHiddenFromHttpApis option that we would be putting in place to work around the fact that the FTR kbnArchiver needs to be able to load and delete any arbitrary type.

Yea, I wasn't happy with this part either, precisely because we'd eventually need another solution for kbnArchiver but wasn't certain what sort of timeframes we were talking about here.

For now, I thought to at least get something in place so that teams can start migrating to their own APIs and follow up with a long-term solution. The workaround buys us a little time to explore options.

A solution I've been thinking about was to develop an kbnArchiver custom kibana plugin that would expose dedicated endpoints for the FTR SO import service to leverage (instead of using the global SO APIs directly).

I like where you're going with this! I've created a dedicated issue we can use to explore the idea and discuss other options if a dedicated plugin won't work on Cloud.

I would just throw if the type explicitly specifies hidden: true, hiddenFromHttpApis: false and accepts other permutations, as they're technically all valid

I ran into issues when allowing hidden types to set the flag but those can be worked around, even if it means more verbose conditional checks.

[TinaHeiligers]

@rudolf @pgayvallet I'd prefer to handle the kbnArchiver dependency issue separately and go with the ignoreHiddenFromHttpApis workaround for now with some way of restricting its us to kbnArchiver only (e.g. simple if statement checking env for example)

That way we unblock other teams from migrating off of the API's with less risk of breaking tests (and fewer CODEOWNERS reviews) in the initial work while we come up with a plan for the kbnArchiver.

[rudolf]

I had a look at how KbnClientSavedObjects gets used in our tests. It's exposed through the 'kibanaServer' FTR service and tests mostly use it to cleanup after importing fixtures. But there's also a couple of places where it's used to reach into the database from a test, e.g. to update a SO mid-test to simulate a scenario or to get a SO to assert something. So KbnClientSavedObjects allows you basic access to saved objects without having to drop all the way down to raw Elasticsearch documents. Not all saved objects are or should be importable/exportable and custom HTTP APIs might block certain update operations that don't make sense for a public API but that's still useful to be able to do from a test. Asking each team to build test-only HTTP APIs to be able to reach into the database in a way that they don't want to expose through their public HTTP API isn't effective. So it feels like long term we would want/have to keep something like KbnClientSavedObjects with at least get, create, update, bulkDelete. Ignoring technical complexity, perhaps ideally this would be a real savedObjects client talking directly with Elasticsearch instead of a slightly different API that you need to lookup for writing tests with an unecessary HTTP layer.

So I kinda see the following options:

1. Remove support for saved objects from FTR and tell teams to use Elasticsearch directly
2. Expose a real saved objects client from FTR that connects directly to Elasticsearch
3. Keep KbnClientSavedObjects that connects to an HTTP API
   1. Reuse the generic saved objects HTTP APIs with a ignoreHiddenFromHttpApis "override"
   2. Create a dedicated testing-only HTTP API
      1. In a dedicated plugin only enabled during testing
      2. With a tag similar to 'access:migrateSavedObjects' to prevent production usage

Even though I would lean towards (2) as a more ideal long term solution (3) sounds like the least amount of work while still providing what teams need. I agree with @pgayvallet that dedicated testing-only APIs would be preferable, even if these are just a copy-paste of the "real" APIs.

@rudolf The SO HTTP API routes all still implement catchAndReturnBoomErrors, even though the TODO was #65291 way back in 2020. Do we still plan on removing catchAndReturnBoomErrors or has that plan changed?

Yes, that should be dead / unecessary code and safe to remove. But if all these APIs are getting removed in the medium term then the priority of polishing this is quite low, I wouldn't bother.

[TinaHeiligers]

Reuse the generic saved objects HTTP APIs with a ignoreHiddenFromHttpApis "override"

We'll need to add find to the list of apis:
kbnServer.savedObjects.cleanStandardList uses bulk_delete with a mix of hidden and non-hidden types (using force as an override) after find.

[TinaHeiligers]

Open questions:

• How should we handle unknown types? ATM, I’m allowing find to simply pass over types not found in the registry and leave them for the repository to handle.
• API usage: Should we count failed API requests as usage? I tend to think yes, we should. Even if a request fails, there was still a call to the API, which means there’s still API consumption.
• Saved Objects Management: should we continue to allow deleting types that aren’t hidden but aren’t allowed to use the public SO HTTP API’s? ATM, forcing teams to declare their types as hidden from the APIs but not hidden will leave them hanging w.r.t. managing their SO from a ready-built UI (SOM).

[pgayvallet]

How should we handle unknown types? ATM, I’m allowing find to simply pass over types not found in the registry and leave them for the repository to handle.

Sounds reasonable to me, we're preserving the same behavior for unknown types that way.

API usage: Should we count failed API requests as usage?

I'd say yes.

Saved Objects Management: should we continue to allow deleting types that aren’t hidden but aren’t allowed to use the public SO HTTP API’s

Yes we should. We need to create a dedicated SOM HTTP API for delete operations performed from the SOM UI instead of using the client-side SO client to do so.