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

[Kibana Core] Allow configuration to specify an array of "disabled routes" for server AND UI #157266

Closed
jasonrhodes opened this issue May 10, 2023 · 35 comments
Labels
discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@jasonrhodes
Copy link
Member

Kibana plugins can specify routes on the server and on the client/UI side. The plugin author may not want all of that plugin's routes to be available in every scenario, but adding forking code within the route handler can be cumbersome. It might be very useful to introduce a configuration value available in kibana.yml, e.g. disabledRoutes, that would allow plugin authors to "404" a subset of routes in a certain situation without needing to make code changes inside of the plugin's app structure.

Something like this?

disabledRoutes:
  - /api/infra/metrics_api
  - /api/ml/*
  - /app/metrics/*

or

disabledRoutes:
  - path: /api/infra/metrics_api
    methods:
      - POST
      - PUT
  - path: /app/metrics/*

My thought was that the router could check this config only on start-up, and compare it to each route as it's being registered. If the disabled parameters match a registered route, that route handler would be replaced with a default 404 handler instead of the handler being registered (along with possible auto-telemetry instrumentation to log the number of requests that have been 404-ed?)

This is just one possible solution to the issue, but the primary use case for this right now is that we want to disable certain UI and API paths in the serverless world, and we'd love it if that didn't require the registration logic to check config inside of each app that needs to do this disabling.

@jasonrhodes jasonrhodes added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label May 10, 2023
@elasticmachine
Copy link
Contributor

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

@pgayvallet
Copy link
Contributor

pgayvallet commented May 10, 2023

Could you to elaborate a bit on why you think it would be beneficial to have this configuration driven?

Ihmo, it would introduce a split between where the routes are registered (plugin code) and where routes can be disabled (core's http service / configuration).

that would allow plugin authors to "404" a subset of routes in a certain situation without needing to make code changes inside of the plugin's app structure.

Can you describe a normal development cycle where this would have any value? Do you foresee scenarios where it would make sense to be able to 'disable' such routes without touching the code.

we'd love it if that didn't require the registration logic to check config inside of each app that needs to do this disabling.

Could you elaborate how this would be an issue? Configuration-driven behavior only makes sense if we're foreseeing the need to be able to change said behavior without delivering a new version. Are you planning / do you need to be able to disable some routes outside of a serverless delivery cycle / version?

If the issue is about the verbosity of wrapping each route definition with some kind of if !serverless then register..., could a tag / option based configuration work instead?

E.g

  router.post(
    {
      path: '/someApi',
      options: {
        hiddenOnServerless: true, // <-- this
      },
      validate: false
    },
   myHandler,
  );

I'm fine having a way to automate this more easily, but I still strongly think that this the information should be held by the route definition, and not by some arbitrary, decoupled, cross-plugin configuration property.

@pgayvallet
Copy link
Contributor

cc @rudolf @jloleysens curious to see your thoughts on that too

@jasonrhodes
Copy link
Member Author

jasonrhodes commented May 10, 2023

For serverless, we have apps inside the infra plugin: some of the routes should continue to work, but others should not (mostly UI side). We'll remove those from the navigation but a customer could type the URL in and the page would load, unless we write code to read from config to disable those discrete parts of the plugin. I expect writing that kind of code inside the plugin will be fragile, especially as the scope of what needs to be hidden in that environment will be changing over time.

Similarly in serverless, for ML we want to keep a few of the APIs and UI pages available, but certain API endpoints should not be available. Here, it's more important, because a user who interacts with those APIs could start expensive jobs that could be problematic in that environment.

I do understand your concern about how loosely related the two things would be. I think the idea of tagging routes, and then allowing some config-driven disabling by tag could also be an option? My understanding is that we want to avoid coupling the idea of "serverless" to the code itself, so hiddenOnServerless: true would likely be antithetical to that goal, but maybe we can narrow in on a good middle ground between these two?

@pgayvallet
Copy link
Contributor

Note that it could also be performed via http hooks

E.g

/**
* To define custom logic after Auth interceptor did make sure a user has access to the requested resource.
*
* @remarks
* The auth state is available at stage via http.auth.get(..)
* Can register any number of registerOnPostAuth, which are called in sequence
* (from the first registered to the last). See {@link OnPostAuthHandler}.
*
* @param handler {@link OnPostAuthHandler} - function to call.
*/
registerOnPostAuth: (handler: OnPostAuthHandler) => void;

it could be a good middleground: definition of the list of exclusions is not based on the route definitions, but still managed by plugin code.

@rudolf
Copy link
Contributor

rudolf commented May 10, 2023

I wonder if we could use feature capabilities. By tagging an API endpoint the security plugin would automatically restrict access to it. I think this would require explicitly giving users access to these API endpoints in non-serverless.

https://github.com/elastic/kibana/blob/main/docs/developer/architecture/security/feature-registration.asciidoc#example-2-dev-tools

@jgowdyelastic
Copy link
Member

I was hoping we'd be able to use capabilities for this, in ML we have the access of every route (146 of them) controlled by capability tags.
We have quite granular capabilities which we use for restricting route access and enabling/disabling whole areas of the UI and various buttons and controls.
Being able to just switch off groups of capabilities based on serverless restrictions would mean we have a lot of this framework already in place.

@pgayvallet
Copy link
Contributor

I agree that using capabilities for this could be a a good way to reuse the mechanisms already in place.

I think this would require explicitly giving users access to these API endpoints in non-serverless.

This is the part that bothers me. Ideally we would have a mechanism to 'add' those capabilities to everyone when not on serverless, or at least a system that doesn't imply adding specific permissions to our users on non-serverless (or more like requiring our customers to add specific permissions to their users)

@pgayvallet
Copy link
Contributor

cc @elastic/kibana-security given capabilities are kinda also owned by then. Would be happy to have your opinion on that one.

@legrego
Copy link
Member

legrego commented May 16, 2023

I still struggle to see how this is an authorization problem. We're not saying that certain users should/shouldn't call these APIs. Rather, we're saying that nobody, regardless of their privileges, should be able to call them. That's not an authorization problem, IMO. We're shipping code that we don't want anyone to invoke. This is a byproduct of our current architecture, and ideally not something we'd do given the option.

By tagging an API endpoint the security plugin would automatically restrict access to it. I think this would require explicitly giving users access to these API endpoints in non-serverless.

Superusers, by definition, can do anything. Even if we tag an API endpoint to restrict access, this would not forbid a superuser from calling these endpoints. I suspect that's not desired behavior.


I'm not opposed to leveraging capabilities to solve this problem, but I don't believe that the security plugin, or our authorization model in general, should be responsible for gatekeeping access to these APIs based on whether or not we are running in serverless.

@thomheymann
Copy link
Contributor

thomheymann commented May 16, 2023

Fully agree with @legrego here - disabling certain routes for serverless is a product decision and not an authorisation concern.

My understanding is that we want to avoid coupling the idea of "serverless" to the code itself

@jasonrhodes Can you elaborate a bit on this point? What's the concern with making code changes for serverless?

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented May 18, 2023

Ideally, plugins shouldn't be aware of the environment Kibana's running on. However, core introduced a contextRef as a way to lookup configuration and the short-term solution would be that plugins can 'lookup' the env. Alternatively, we can add a new route option as a flag to mark certain routes as unavailable (or disabled).

Core doesn't know anything about route handling in plugins and plugins will need to handle their own logic for disabling routes depending on the env.

Both config and a route decorator are ways to solve this. However, disabling/enabling routes has a higher chance of changing, hence we lean towards a route decorator as a way plugins can handle conditional logic.

@lukeelmers
Copy link
Member

Larry's right, this isn't an authorization issue. If we have a subset of routes that we know we don't want to be available anywhere in serverless, it feels wrong to keep them in place and then rely on privileges prevent access to them (plus, there's the superuser concern already mentioned).

I don't feel strongly about which approach we go with here -- a route option, tags, or configuration -- all have their pros and cons. But I don't think we should let our guiding principle of "avoid introducing isServerless flags" push us to abusing authz. Even a hiddenOnServerless route option as Pierre mentioned would still delegate the isServerless check to core, so we wouldn't be leaking it to plugins.

Concretely, there are three specific use cases we need to think about:

  1. Routes which need to be exposed in self-managed and but not in serverless
  2. Routes which need to be exposed in serverless but not in self-managed
  3. Routes which need to be configured differently in self-managed vs serverless (e.g. public in self-managed but internal in serverless)

Use case (2) could be solved by registering such routes from a Serverless-only plugin. Use cases (1) and (3) are the scenarios I think we should be trying to solve for here. I've had multiple teams bring this up already, and I expect we'll continue to be asked these questions as folks begin prepping & versioning their APIs.

@afharo
Copy link
Member

afharo commented May 18, 2023

++ to Luke's and Larry's comments.

My main concern is blocking UI routes: Core owns the navigation to the app. However, navigating to sub-paths inside the app is out of core's control. I wonder how we're going to block those UI navigations.

I'd like to focus on that use case for a minute because if we find that the only option is for the plugin to check either a feature.enabled flag or an isServerless flag to stop registering the specific UI path, I don't see why we wouldn't do the same for HTTP API routes.

Following that same thought: we may find situations where we link those paths inside our UI. So wrapping our code in if (shouldShowFeatureX) will be necessary, leading to plugin configs being the preferred solution.

TL;DR, I think plugins having config flags to enable/disable specific features is more future-proof. We can set the defaults for serverless inside the config/serverless.yml files.

@rudolf
Copy link
Contributor

rudolf commented May 23, 2023

I think it would be helpful to have more examples of APIs plugins want to disable.

@jasonrhodes
Copy link
Member Author

We're not saying that certain users should/shouldn't call these APIs. Rather, we're saying that nobody, regardless of their privileges, should be able to call them. That's not an authorization problem, IMO.

This is a matter of perspective, I think. We're saying "some users should not have access to these APIs" and what we mean by "some users" are "users who are created in serverless context", for example. We've been instructed (rightly, I think) to avoid tight coupling between the idea of serverless/not-serverless as the actual driver of product changes, and rather to define semantic differences that describe real changes in the product and then specify those semantic changes in the serverless config. I don't have strong opinions on whether this is auth or not, so I think it's just a matter of whether a parallel set of configurable access tags makes more sense?

I think it would be helpful to have more examples of APIs plugins want to disable.

Some examples:

  • We're only shipping some of the "infra" plugin routes for the initial offering of serverless. Let's say there are 10 UI routes and 6 API routes that are typically available in this plugin. To start, we only want 2 UI routes and 1 single API route available. As we test more of the other routes, we enable more of the UI routes and their associated API routes. The code doesn't really need to care about whether it's in serverless or not, or even what serverless is, it just needs a way for us to specify which routes are enabled and which are disabled for a given context.
  • Some of the Machine Learning plugin must be enabled to provide some features for other serverless plugins. However, we don't want users to be able to start all kinds of jobs that exist in that plugin, in serverless. For that env, it'd be good to block access to certain APIs while allowing access to the ones that have been allow-listed.

There are a lot of ways we can do this kind of change, I think?

  1. "Serverless users" have different roles with different sets of permissions for that environment
  2. We have a totally separate set of route tags that allow for a similar, but separated access control system so that environment-based configs can turn routes on/off without code changes
  3. We can write code in the plugin to check for independent config flags that turn routes on or off
  4. We can write code in the plugin that checks if it's in a specific env, by name, and make on/off decisions based on hard-coded logic

I think [4] is the one we should avoid, based on my own understanding of how coupling these kinds of things can lead to bad situations as well as what I've heard others say about the serverless initiative in general. [3] works, but involves some amount of code thrashing for what's essentially per-environment configuration changes. That's why I was hoping for something more like [1] or [2], but if those have significant challenges, [3] will likely work okay.

@afharo
Copy link
Member

afharo commented May 24, 2023

++ to defining them as semantic options vs. per-environmentoffering ones: I also prefer option [3] over [4].

To be pedantically correct, environment configs tend to differentiate dev, qa, staging, prod. Serverless is a new offering/product/architecture.

If we look at this as a new offering/product, options [1] and [2] may externally change the product we build, test, and ship. These alternatives may lead to unexpected errors that we didn't cover in our CI builds like API GET /api/attr/b being blocked via access/roles tags but necessary inside an allowed UI.

You may argue that option [3] could cause a similar outcome if the feature.enabled configs are set outside the build process. That's why we introduced the files config/serverless.yml and config/serverless.{projectType}.yml: there are some settings that must be owned by Kibana as a product. While it's safe to change some settings externally, enabled/disabled settings typically tend to affect dependencies and require deeper in-product testing.

[3] works, but involves some amount of code thrashing

I'm not entirely sure [1] and [2] would avoid that code trashing: while it's true it saves a bunch of "ifs" when registering the REST APIs, and we could look at ways of defining a common UI router to block access to app's internal routes (AFAIK, it doesn't exist today) to avoid those IFs in the UI, you'd still need to check the users' role/access to show/hide the buttons that link to the not-accessible UI.

And there are other components that would still be registered unless the app checks if the user has enough permissions (like flyouts, top-bar items, and modals).

To be clear, I'm not opposing to either, just giving my 2 cents on each one of those 😇

@jasonrhodes
Copy link
Member Author

All fair points, @afharo. The broken buttons and links would just fail with 404s in the most basic case, which is at least better than arriving at a UI screen that has been "removed from the navigation" without being otherwise blocked in any way, and getting who knows what kind of experience.

Serverless is a new offering/product/architecture.

If we look at this as a new offering/product, options [1] and [2] may externally change the product we build, test, and ship. These alternatives may lead to unexpected errors that we didn't cover in our CI builds like API GET /api/attr/b being blocked via access/roles tags but necessary inside an allowed UI.

Ultimately, if I'm being very honest, I think we're seeing the cracks here in trying to use a single product (Kibana and its on-prem plugins) to also serve an entirely new offering/product. I imagine every solution to these problems is going to feel "a bit off" because of this.

@jeramysoucy
Copy link
Contributor

jeramysoucy commented May 24, 2023

I think it would be helpful to have more examples of APIs plugins want to disable.

@rudolf We have several API's that we'd like to disable in serverless. Here's the list as of today (spaces to be re-evaluated):

  • All public spaces API's
  • Any deprecated API's (e.g. GET /api/security/v1/logout, POST /api/security/v1/saml, etc.)
  • All OIDC API's
  • Get/Delete roles (GET /api/security/role/{name}, DELETE /api/spaces/space/{id})
  • All role mapping API's
  • All user API's
  • Invalidate session (POST /api/security/session/_invalidate)
  • Anonymous access API's (GET /internal/security/anonymous_access/capabilities, GET /internal/security/anonymous_access/state)
  • Access agreement API's (Get /security/access_agreement, GET /internal/security/access_agreement/state)
  • Encrypted SO rotate key (POST /api/encrypted_saved_objects/_rotate_key)

We also have some API's that are currently designated as public but we'd like to designate as internal for serverless:

  • Logout (GET /api/security/logout)
  • SAML callback (POST /api/security/saml/callback)
  • Many of our "view" API's (GET /login, GET /security/account, GET /security/logged_out, GET /logout, GET /security/overwritten_session, etc.)

@afharo
Copy link
Member

afharo commented May 25, 2023

Fully agree with @legrego here - disabling certain routes for serverless is a product decision and not an authorisation concern.

Just dropping another thought here (trying to look at this from a different POV): if we think of Serverless' Project Types as product offerings with different features and pricing, you could argue that we are limiting access to some features based on their level of access (even when it's applied to all the users in that specific Project Type) 😇

WDYT?

@legrego
Copy link
Member

legrego commented May 25, 2023

if we think of Serverless' Project Types as product offerings with different features and pricing, you could argue that we are limiting access to some features based on their level of access (even when it's applied to all the users in that specific Project Type)

This, to me, sounds most similar to the licensing plugin & related services, as opposed to security.

@jasonrhodes
Copy link
Member Author

Thanks for all of the conversation, everyone. Unfortunately, it doesn't seem like we are any closer to a solution, here. Which likely means each plugin will develop their own, bespoke way for controlling route access in a serverless (and other, similar context) world. I'm feeling the urge to go "wrap the router" and make it respond to custom config to turn off routes, and that always feels like the wrong urge.

@jeramysoucy do you have plans for how you are going to control the access changes you mention in your latest comment?

@jeramysoucy
Copy link
Contributor

do you have plans for how you are going to control the access changes you mention in your latest comment?

@jasonrhodes We don't have a definitive plan or design yet. We were hoping for either a unified mechanism or some guidance from core. We have a sync with them tomorrow and this will be part of the discussion.

@pgayvallet
Copy link
Contributor

pgayvallet commented Jun 6, 2023

  1. "Serverless users" have different roles with different sets of permissions for that environment
  2. We have a totally separate set of route tags that allow for a similar, but separated access control system so that environment-based configs can turn routes on/off without code changes
  3. We can write code in the plugin to check for independent config flags that turn routes on or off
  4. We can write code in the plugin that checks if it's in a specific env, by name, and make on/off decisions based on hard-coded logic

My 2 cps:

Security strongly pushed against 1., with what I think are valid arguments.

I agree that 4. seems unnecessarily. Direct coupling to low level env, which is bad, logic won't be reuse between plugins, and so on.

Remain 2. and 3..

I personally would prefer 2., as I think it overall is the cleanest design:

  • A unified way of defining in which """env""" routes should be enabled on (strongest argument ihmo: consistency)
  • Core knows about all routes, even ones that will be disabled in a given env (could be useful for openAPI spec gen for instance, or other things)
  • No need to leak env-related info to the plugins (even if I suspect we would need to expose such info for other use cases, but anyway)

Now, to implement 2., we need the 'conditions' around route enablement/disablement to be strictly defined: There is less flexibility than just exposing the environment info to the plugins and letting them implement their conditions themselves around route registration.

So, if we want to go with 2., we need to make sure first that such enablement/disablement conditions are simple and generic enough for all plugins to use the feature.

E.g, would serverless (true/false) and project (enabling/disabling depending on serverless project types) be sufficient informations for this?

Because in that case, we could imagine something that would looks like the example from my initial contribution to this issue:

  router.post(
    {
      path: '/someApi',
      options: {
        condition: { // only enabled on serverless for solution-1 or solution-2 type of projects.
          env: "serverless",
          projects: ["project-1", "project-2"]
        }
      },
      validate: false
    },
   myHandler,
  );

  router.post(
    {
      path: '/someOtherApi',
      options: {
        condition: { // not enabled on serverless (on any solution type)
          env: "not-serverless", // idk, we need to name this consistently at some point
        }
      },
      validate: false
    },
   myHandler,
  );

(naive API example, it's just to illustrate)

Otherwise, I think we would unfortunately have to go with 3.

@rudolf
Copy link
Contributor

rudolf commented Jun 6, 2023

This is sounding very much to me like route-level feature flags. Sticking with (2) I can see two variations:

  1. on/off is dependant on the environment (Pierre's example)
  2. on/off is dependant on the feature. In any given environment we compose the features we'd like to enable/disable. We should be careful to not expose feature flags as a public API in kibana.yml that users can tweak the way they see fit.
  router.post(
    {
      path: '/getLogs',
      options: {
        featureFlags: [
          {name: "observability.infra.getLogs", default: "enabled"}
        ]
      },
      validate: false
    },
   myHandler,
  );

serverless.yml

featureFlags
  observability.*: disabled // disable all obs features by default

serverless.oblt.yml

featureFlags
  observability.infra.getLogs: enabled
featureFlags
  observability.infra.deprecated: disabled

@pgayvallet
Copy link
Contributor

I just want to get back to @afharo's comment in #157266 (comment):

My main concern is blocking UI routes: Core owns the navigation to the app. However, navigating to sub-paths inside the app is out of core's control. I wonder how we're going to block those UI navigations.

This is absolutely right. Core has no control on application sub paths ("UI routes") in any way (we're not even the same react app, technically). Meaning that option 2. can't be used for the UI. Blocking / not registering those UI routes will have to be done by each individual apps. Only option 3 or 4 can be used here, as the best we can do is to expose those "high-level" (3.) or "low-level" (4.) flags to plugins.

So depending on how much consistency we want between the two sides of the problem ( blocking UI routes and disabling server endpoints), we may prefer to just use 3. for both sides instead of 2. for the server and 3. for the UI.

Note though, I personally believe these two sides of the problem are actually two distinct problems that should be addressed differently. Blocking subparts of an application on the UI seems to be just like one of the dozen of kinds of "UI customizations" we want to be able to do to differentiate the two "products", so I would be fine with less consistency, and more specialization in the way we'll address them, and think that doing with 2. (either of the options described by @rudolf in his previous comment) would still be the best in term of DX and capabilities.

@jeramysoucy
Copy link
Contributor

  • on/off is dependant on the environment (Pierre's example)
  • on/off is dependant on the feature. In any given environment we compose the features we'd like to enable/disable. We should be careful to not expose feature flags as a public API in kibana.yml that users can tweak the way they see fit.

@rudolf @pgayvallet There are also cases where API access would differ between current offerings and serverless (public -> internal). On/off would not account for environment dependant access differences. Would it make sense to specify access as 'public' | 'internal' | 'disabled' per environment, or consider access separately from enabled/disabled?

Not sure if other teams are also considering access differences in their API's for serverless, but we have a few mentioned here.

@pgayvallet
Copy link
Contributor

pgayvallet commented Jun 8, 2023

Would it make sense to specify access as 'public' | 'internal' | 'disabled' per environment

Define internal? Are we talking about those routes that should only be accessed internally by other parts of our system (and not by end users). Because AFAIK we don't have a defined technical solution for this atm (please correct me if I'm wrong here @lukeelmers), and without knowing how we will address this other problem, it's hard to express an opinion on the feasability of including it in this feature.

But see, this confirms my concerns regarding whether we can design something generic here. From my understanding, your team only need to configure routes depending on server/not-serverless. Some IIRC other teams already expressed the need to configure depending on the project type (but without the need to switch the APIs to internal, again @lukeelmers please correct if I'm wrong). Can we really combine the requirements from everyone into something generic-ish and developer friendly here?

@lukeelmers
Copy link
Member

lukeelmers commented Jun 8, 2023

Define internal? Are we talking about those routes that should only be accessed internally by other parts of our system (and not by end users). Because AFAIK we don't have a defined technical solution for this atm

By internal I think @jeramysoucy is referring to #151940. Some teams are going to want to make APIs that are currently public in self-managed internal on serverless, since making this change in self-managed would be breaking so we aren't able to do it now.

I don't know how cleanly this logic would apply to the feature flags approach Rudolf suggested, but I do think it would work with Pierre's suggestion (#157266 (comment)) of having env-based conditions in the route options. In that case two routes would be registered: one internal and one public, both sharing the same route handler. The only difference would be that the internal one would be configured for serverless env only (and use an internal path prefix), and the public one configured for non-serverless only.

I suppose there's also the :whynotboth: approach if we eventually identify distinct cases for both env-based and feature-based toggling:

  router.post(
    {
      path: '/someApi',
      options: {
        // all conditions must be true for route to be registered
        condition: {
          env: "serverless",
          projects: ["project-1", "project-2"],
          featureFlags: ["foo"]
        }
      },
      validate: false
    },
   myHandler,
  );
featureFlags.foo.enabled: true # default for the feature flag can be set wherever we register them

If we want to go down the feature flags route, my one suggestion would be to give the design some thought -- feature flags have been brought up frequently in the past, so if we decide to provide an official platform-level mechanism for doing this, we should be certain we are aligned on functional requirements, etc.

IMO the easier short-term path here is env-based configuration since it seems it would address the main concerns as a starting point, and we could still layer in feature flags later if it turned out to be necessary.

I personally believe these two sides of the problem are actually two distinct problems that should be addressed differently. Blocking subparts of an application on the UI seems to be just like one of the dozen of kinds of "UI customizations" we want to be able to do to differentiate the two "products", so I would be fine with less consistency, and more specialization in the way we'll address them

Agree with this, I see the UI use case as distinct here and something that could be solved with lower level configs.

For example - say I have plugin foo that registers application bar with features a, b, and c, I could see having configuration options like:

# foo is enabled, but skips registering bar app, so none of the UI routes are enabled
foo.bar.ui.enabled: false
# foo registers bar app, but disables feature A and any associated UI routes
foo.bar.ui.a.enabled: false

Because it's not just about disabling the routes in the browser, ideally we are excluding any unnecessary JS from our initial and async browser bundles so we aren't loading code for features that will never be available anyway.

@afharo
Copy link
Member

afharo commented Jun 12, 2023

I love how this conversation is evolving. To make sure I understand where we are getting at, I'll try to summarize the current suggestion:

  1. We are treating server HTTP APIs separately to UI routes.
  2. UI routes are up to the plugin whether to register them or not based on plugin-owned config flags.
  3. HTTP APIs can be blocked at the Core level based on the options provided when registering the route.
  4. The blocking rules are defined either as Core-level feature flags or env offering-based conditionals.

Please, feel free to correct me if I misunderstood or missed anything.

Related side conversations:

  • internal vs. public HTTP APIs:
    • Can we expect some HTTP APIs to be public for some Serverless Project Types and internal for others?
    • Wouldn't they need to change the pathname from /internal/ to /api/ as well?
    • Potential solution: register both public and internal routes with inverse conditionals

@jeramysoucy
Copy link
Contributor

Thanks @lukeelmers, that's correct (see also #156935).

Potential solution: register both public and internal routes with inverse conditionals

@afharo This was Luke's suggestion as well, and I think this would work. Internal callers would need to know which path to call as well.

@afharo
Copy link
Member

afharo commented Jun 12, 2023

Internal callers would need to know which path to call as well.

TBH, that's my main concern with treating both (HTTP APIs and UI) separately: the UI needs to know if the route is available. Otherwise, it will degrade the UX by showing hundreds of expected errors.

We'll need the plugin-owned settings used to register the UI fixtures to be perfectly in sync with the feature flags/conditions that enable/disable HTTP APIs. IMO, that's a ticking bomb waiting to explode.

IMO, the easiest mental model for the developers is: Feature A is enabled, hence, related HTTP APIs and UI components are enabled. IMO, this translates to one option, myPlugin.featureA.enabled that is handled on both, the server and UI ends.

We can def modify the HTTP registrar to avoid devs using if:

  router.post(
    {
      path: '/getLogs',
      options: {
        featureFlags: [
          {name: "myPlugin.featureA.enabled", default: "enabled"}
        ]
      },
      validate: false
    },
   myHandler,
  );

But that'd involve the HTTP Core service having access to all plugins' config, which goes against our design patterns, so the registrar will require the plugin to provide the config:

  router.post(
    {
      path: '/getLogs',
      options: {
        featureFlags: {
			pluginConfig: config,
			rules: [
	          {name: "featureA.enabled", default: "enabled"}
    	    ]
      },
      validate: false
    },
   myHandler,
  );

At that point, is there such a big difference to the code below?

if (config.featureA.enabled) {
  router.post(
    {
      path: '/getLogs',
      validate: false
    },
   myHandler,
  );
}

Another option could be to create a new featureFlags core service that can be accessed from the UI to access the same setting used for the HTTP API registration. In that case, I'd highlight @lukeelmers's comment:

If we want to go down the feature flags route, my one suggestion would be to give the design some thought -- feature flags have been brought up frequently in the past, so if we decide to provide an official platform-level mechanism for doing this, we should be certain we are aligned on functional requirements, etc.

Also: are we sure that feature flags are true|false only? If they aren't (i.e.: theme: v7/v8 or the mentioned myPlugin.routeA.access: 'internal' | 'public'), are we registering config.schema for those? Where do we draw the line between featureFlags, config, and uiSettings?

@jasonrhodes
Copy link
Member Author

I just want to get back to @afharo's comment in #157266 (comment):

My main concern is blocking UI routes: Core owns the navigation to the app. However, navigating to sub-paths inside the app is out of core's control. I wonder how we're going to block those UI navigations.

This is absolutely right. Core has no control on application sub paths ("UI routes") in any way (we're not even the same react app, technically). Meaning that option 2. can't be used for the UI. Blocking / not registering those UI routes will have to be done by each individual apps. Only option 3 or 4 can be used here, as the best we can do is to expose those "high-level" (3.) or "low-level" (4.) flags to plugins.

@pgayvallet and @afharo I'm not sure I follow this. If the router knows how to interpret some kind of tag/flag on the route definition in the UI, aren't all paths and sub-paths in a Kibana plugin's UI defined via this router's route definitions? Is there a way plugins are defining ad-hoc sub-path routes that aren't defined by router.* methods?

Or do you just mean links to other parts of a plugin sometimes failing because that path happens to be blocked?

@jasonrhodes
Copy link
Member Author

Looks like APM is already moving forward with a custom way of handling this, so I'll back myself out of this conversation. If you all end on a recommended approach, please ping me, but for now we will likely just handle it all inside the plug-in code.

@jgowdyelastic
Copy link
Member

jgowdyelastic commented Jul 1, 2023

An update on what ML has done for this.
In this PR I've added an API for enabling/disabling ML features in kibana (setFeaturesEnabled). This is called by each serverless plugin on setup.
We need to be able to disabled whole features based on project, e.g. Observability has Anomaly Detection enabled, but Data Frame Analytics and Natural Language Processing turned off.

pluginsSetup.ml.setFeaturesEnabled({ 
  ad: true, 
  dfa: false, 
  nlp: false
});

I've added three new ML capabilities (isADEnabled, isDFAEnabled, isNLPEnabled). I appreciate that this is a bit of a stretch of kibana's capabilities system as they aren't really user capabilities, but the capabilities system is so convenient, it seemed to be best place to put them for now.

I've updated our ML capabilities switcher to look at which ML features are disabled and to switch off all related capabilities. ML has always had very granular capabilities which are checked for both server and client side routes, so being able to switch off, for example, all anomaly detection endpoints and client side pages for all users with a single flag is again very convenient.

I've updated our deeplink registration based on enabled features, so the serverless nav menus and app search will only contain links to pages which are enabled in the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests