[Connectors][API] Update connectors with enabledAfterImport flag

[ymao1]

In order to support import/export of connectors, we need to mark a connector as enabled/disabled after import. This issue addresses the API portion of this. A separate issue has been created for addressing the UI portion.

1. Add enabledAfterImport flag (or something similar) to RawAction type
2. Update connectors APIs to handle the new flag - create and update should just set enabledAfterImport to true
3. Add migration to add enabledAfterImport: true to all existing connectors

[ymao1]

Should we be able to disable preconfigured connectors? My feeling is no. If it's preconfigured and users want to move to a different cluster, they can copy the kibana.yml configuration instead of importing/exporting.

[elasticmachine]

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

[pmuellr]

Should we be able to disable preconfigured connectors? My feeling is no. If it's preconfigured and users want to move to a different cluster, they can copy the kibana.yml configuration instead of importing/exporting.

Yeah, that seems correct. The secrets for preconfigured connectors are either in the config as well as the other params, or can be in a a keystore. So they already have a mechanism for "import/export".

[pmuellr]

A couple of sharp edges to watch out for when adding new fields the connector saved objects:

• we need to figure out if we want the new fields to be excluded from AAD; by default they are included in AAD, but can be explicitly marked to be excluded. When excluded, this allows us to do partial updates on just those fields, without having have all the values and re-encrypt everything

• generally, "empty" values of saved object fields should be null and not allowed to be undefined, since allowing undefined values doesn't allows us to "delete" the values later (considered a partial update, which then causes subsequent AAD issues)

[pmuellr]

Update built in connectors secrets schemas so fields are nullable

That's going to be complicated. We already have a mix of secrets which are nullable, and some are not. How would we know then if a particular secret is "really" nullable or not?

Could we just set the secrets field itself to be null? I think elasticsearch allows us to do that at runtime. We could then change the places where we call the config/secrets validation code to check the enabled flag to see if we should bother running the secrets validator or not.

... sorry, starting to ramble below, but given the number of APIs we're adding, now wondering if there is some "simpler" story ...

I'm kinda wondering if we even need to support specific enable/disable APIs. During export, we would presumably not export the secrets, but then we can't do an import of those using existing APIs as the validation would fail because there are no secrets (except for server log and the index connector since they don't have secrets :-)).

We'd need a special import API that would allow connectors to be created but not validate their secrets. Those connectors would then fail if you ever tried to run them (we validate before running), and require that the customer update the connector to add the secrets back.

What we'd be missing is a clear signal that there are connectors which need their secrets added back, since we don't really have a way of showing that now. But maybe that's just some hidden field, which we set on the special import API, and can show an indication in the alerts list (and make it sortable/etc) - I guess this is the justImported flag I referenced in the meta issue - not neccessarily "hidden", but read-only from an API point of view (and of course it would need a better name)

It does make me wonder in general how we will ensure that server log and the index connector will get exported and then imported without requiring any user interaction! Interesting edge case!

Also I'm wondering if we have use cases where we would want to enable/disable connectors independent of import/export - we've talked about it before, but I'm not sure we have any real requirements for it. Having to deal with justImported AND enabled - is that good or bad? If it's all clumped in a single enabled flag, is it important for us to be able to tell the difference - so that we can tell the customer the difference? Perhaps it depends on whether a disable API specifically nulls out the secrets or not - if it doesn't - which would be nice so that you don't have to re-enter them when re-enabling the connector, then having them as separate things seems like it makes sense. But maybe secrets == null is enough of an indicator of justImported? Maybe we don't need a specific field?

[ymao1]

Also I'm wondering if we have use cases where we would want to enable/disable connectors independent of import/export - we've talked about it before, but I'm not sure we have any real requirements for it.

My thinking behind this was since the workflow user imports connectors that are disabled and are directed to the connectors list to re-enable and re-enter secrets, each connector in the list would have some sort of enable switch (like the rules list does currently). If there is an enable, then there probably should be a disable :)

However, I see your point that this is a lot of API changes. Maybe if we consider the re-enable workflow more of prompting the user to edit their connector instead of re-enabling it, we can get away with just updating the update API instead of introducing the concept of explicit enable/disable.

I do think it would be useful to have a disable by ID API (maybe a connector is erroring; user could disable it while troubleshooting to avoid littering the logs with errors and then reenable later without having to go through all the alerts to figure out which ones used the connector, which they would need to do if they deleted the connector). But maybe this isn't something that needs to happen now?

[mikecote]

During export, we would presumably not export the secrets, but then we can't do an import of those using existing APIs as the validation would fail because there are no secrets (except for server log and the index connector since they don't have secrets :-)).

The import process would happen at the core SO layer, which bypasses our logic (including validation). This would allow objects to be imported as-is even if validation fails. We would then have to handle such a scenario.

[pmuellr]

I do think it would be useful to have a disable by ID API ... But maybe this isn't something that needs to happen now?

It doesn't need to happen now, but would be nice to kill two birds with one stone. OTOH, it adds more moving parts that we don't explicitly need for export/import.

I'm thinking that if we add enable/disable in this PR, and we end up putting newly imported connectors in a disabled state, we will probably want a "disabled reason" field, so we can distinguish "a user explicitly disabled this" vs "this was disabled because it was just imported", so we can distinguish the two.

[ymao1]

@mikecote cleared up some confusion I was having about using the saved objects import/export vs using our own API and I think we can simplify this a little to (1) remove changing the secrets to nullable and (2) remove the suggested createDisabled API and (3) de-couple the idea of enable/disable from import/export.

We can avoid changing the secrets to nullable since the import/export process bypasses our validation altogether. If we use a variable like enabledAfterImport instead of enabled (similar to enabledInConfig and enabledInLicense for the connector type, we can have a UX more similar to the license error UX on the rules table, where we show a way to "Fix" the connector. I have a POC for this here

If we do it this way, then enable/disable is kind of a completely separate issue.

If we're good with this, I'll update the issue description.

[pmuellr]

If we use a variable like enabledAfterImport instead of enabled (similar to enabledInConfig and enabledInLicense for the connector type, we can have a UX more similar to the license error UX on the rules table, where we show a way to "Fix" the connector.

That sounds good to me.

[mikecote]

I'm +1 on that idea as well 👍

[gmmorris]

I might be missing something, but I don't feel the name enabledAfterImport is quite descriptive enough.
Reading through the context above it seems to me (and I might be confusing things) that what this flag is meant to do is communicate to the framework that the Connector can't be used until it is reconfigured with the missing fields.

I'm not sure the name enabledAfterImport quite expresses that... I know it sounds like semantics, but I feel like naming it something more expressive (such as requiresUserInput or disabledDueToMissingConfiguration) would make it easier for newcomers to our code base and long term maintainance.

Any thoughts?

[ymao1]

@gmmorris Naming is hard :) When I see disabledDueToMissingConfiguration, it sounds a little alarmist and doesn't provide any context as to why the configuration might be missing...probably the same feeling you get when you see enabledAfterImport. What about disabledAfterImport? That would give users/devs a reason for why a connector might be disabled and devs can get the clue of looking at the import/export to figure out what's happening

[gmmorris]

Naming is hard :)

It's the worst.

That would give users/devs a reason for why a connector might be disabled and devs can get the clue of looking at the import/export to figure out what's happening

Yeah, that would be better for sure... I'm still not sure I'd understand what the call to action is 😬
But yeah, it's hard for sure :)