Alert params and action config will merge sub attributes on update if not explicitly nulled

[mikecote]

Problem

Relates to #64870
Relates to #50256

If an alert type's params schema doesn't set default values of null to all it's optional properties, it is possible for an alert to fail validation of AAD when running. All someone would have to do is to create an alert with an optional value set, call the update API without the optional value set and the alert will fail running.

The same applies for action type's config schema (and possibly secrets?).

The issue happens with the following index mappings in the Elasticsearch:

"params": {
  "enabled": false,
  "type": "object"
},

The scenario can be reproduced in Elasticsearch by doing the following:

As you can see, foo.bar didn't get cleared out after the update, it got merged.

Solutions

1. Change update to create w/ overwrite: true

We would lose OCC (optimistic concurrency control) if there's any version conflicts but would solve the problem by replacing the existing document in elasticsearch (delete then insert). To use this solution, we'll need encrypted saved objects to support this by allowing custom ids.

2. Stringify the params and config in Elasticsearch

Doesn't feel right but it would work.

3. Custom code to nullify sub attributes

Since we load the alert / action on update, we could write a function that does a diff on these sub properties and sets them as null in the update params when missing.

4. Change object structure to array

I noticed the alert's actions[x].params attribute doesn't have this problem. There is a possibility that storing alert params and action configs into an array structure would solve this problem and possibly also make them searchable.

The params value structure could be something like the following:

[
  {
    "name": "index",
    "value": "foo"
  },
  {
    "name": "timeField",
    "value": "@timestamp"
  }
]

This would allow a consistent mapping of name and value where value can be enabled: false but won't be impacted by this issue (due to being within an array). This would also require a saved object migration.

5. Have clear documentation that optional properties must have default values

Maybe there's a way to validate this programatically.

[elasticmachine]

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

[mikecote]

For solution 4, I can confirm that changing the structure to array does solve the issue.

[pmuellr]

1. Change update to create w/ overwrite: true

I didn't realize this would break OCC, I didn't think it would. I thought all we really needed was ESO to support the overwrite option, which ended up requiring support for pre-defined IDs.

2. Stringify the params and config in Elasticsearch

We are doing this somewhere else, aren't we? Or maybe we used to, but not anymore? This will make the "ability to search through alert/action params" even harder, but it's still not clear to me that we need ES search through the params. Or a precise one anyway - how well would a text search work through those? Maybe that would work good enough?

3. Custom code to nullify sub attributes

This seems unpossible, since we don't really know the shapes of the params. Currently, we only require an object with a validate() function to be passed in, which is perfect for a config-schema object, but we don't require config-schema. Even if we had config-schema, not clear that we can enumerate the shape of the object from it. I think it probably only gets problematic for 2nd level objects - top level properties we could diff vs what's in ES, but if one of those properties is itself an object, is null in ES, and not null in the update, we don't have anything to compare to from the ES version.

4. Change object structure to array

I think this would need to be surfaced all the way through our APIs, so breaks everything today :-). We'd need to come up with special per-alert-type migrations to convert existing alerts.

5. Have clear documentation that optional properties must have default values

Yeah, if we do nothing else, adding more doc with big lettering is something we should do. Probably specifying that you can never use schema.maybe() for folks using config-schema.

Maybe there's a way to validate this programatically.

Not sure how introspective config-schema is, but this might work if we can analyze a schema. Alternatively, if there isn't enough introspection available for this, I wonder if we could add some new functionality to config-schema that would either check for undefined-able properties, or maybe an option on object that would disallow undefined-able properties.

[kobelb]

There's also a 6th option, which is likely the most work, adding support for non-partial updates to saved-objects. I'm not aware of any constraints that would prevent us from adding this capability, but I'll ultimately defer to the @elastic/kibana-platform team on the specifics.

[pmuellr]

There's also a 6th option, which is likely the most work, adding support for non-partial updates to saved-objects.

Pretty sure you can already do overwrites w/saved objects - you just can't with ESO's, I believe because of the non-custom-key constraint. So I thought this was the same as option 1).

It's been a while since I looked into this though, I may be remembering wrong.

Note: I was remembering wrong, see comment below ...

[pmuellr]

Poked around a little bit - it appears that there isn't an overwrite option for update, but there is one for create. So it seems like SO's only support partial update - in fact, the docs specifically shows a partial update of just the title of an index pattern - https://www.elastic.co/guide/en/kibana/current/saved-objects-api-update.html

So, not sure what I was thinking - perhaps we were earlier looking at adding an "overwrite" capability to SO.update()? I do specifically remember there was something involving the issue around using your own id's for ESOs that was going to make this more complicated though.

I'm a little surprised we only support partial update of SO's - surprised that this hasn't bitten anyone else yet, trying to "delete" an attribute by setting it to undefined on the update request.

[kobelb]

I'm a little surprised we only support partial update of SO's - surprised that this hasn't bitten anyone else yet, trying to "delete" an attribute by setting it to undefined on the update request.

Agreed :)

[mikecote]

I'm leaning towards exploring option 3. If it works, we would just require a single drop-in function and the issue would go away.

@pmuellr

This seems unpossible, since we don't really know the shapes of the params.

The way I have it in mind is we don't need to know the shape because we can compare the keys in the two objects. Any key that is missing in the new object, we programatically add a null value.

Example:

// Old
{
  number: 1,
  object: {
      abc: 3,
  },
  otherNumber: 2,
}

// New
{
  number: 2
}

// nullifyAttributes(old, new);
{
  number: 2,
  object: {
    abc: null,
  },
  otherNumber: null,
}

I think it probably only gets problematic for 2nd level objects - top level properties we could diff vs what's in ES, but if one of those properties is itself an object, is null in ES, and not null in the update, we don't have anything to compare to from the ES version.

I'm interested at having an example where using what's described above with recursion wouldn't work. Is there an object shape that wouldn't work with such approach?

[pmuellr]

Could this be a problem for clients that mistakenly have schema.maybe() (or define a defaultValue - I can't remember if that's another problem point or not)? They wouldn't be expecting a null, but could end up getting one back.

If we can get option 3 to work, I'd be happy with it, seems like it should "fix" this, given the caveat above. I haven't given it a lot of thought, not sure if there any tricky parts.

[mikecote]

Could this be a problem for clients that mistakenly have schema.maybe() (or define a defaultValue - I can't remember if that's another problem point or not)? They wouldn't be expecting a null, but could end up getting one back.

I'm thinking if this "special" function runs after going through the validator, we should be ok. If it doesn't expect a null, it would fail at the validation stage before we assign one.

Also agree it's not guaranteed this approach would work but definitely potential.

[pmuellr]

'm thinking if this "special" function runs after going through the validator,

You mean BEFORE going through the validator? But in any case, good point, at any point we want to call this special merge fn, we can afterwards call the validation function just to make sure the resulting shape is still "valid". I guess that's just for update, in which case we'd probably call validate twice - once on the incoming payload, do the merge, then validate again. May be confusing to the callers when they hit the second validation, as the messages would presumably be complaining about nulls they didn't pass in the payload ...

Seems like a good approach to try though! Simpler than adding an overwrite capability to SO's :-)

[gmmorris]

this is a duplicate of #71995