[RFC] Support for event schema

[yagnik]

Description of Issue/Question

Our setup has gotten rather large with many people contributing to salt internally. As we add more beacons and reactors specifically and move to a more event driven architecture, we are noticing more number of bugs with developers making mistakes around what values are always present in the event and what are optional. This leads to edge cases throughout the codebase and a domino effect on the infrastructure.

I'd like to suggest bringing in event schema and versioning for all events that go on the event bus. Any consumer will validate the schema and only then process the event and in case of invalid schema we could possibly raise another event with invalid data.

Moreover we have started pouring our messages into our dataplatform to look at the health of the infrastructure and event processing similar to how we would do it for any front end system.

More reading on why schema are useful:
https://www.confluent.io/blog/schema-registry-kafka-stream-processing-yes-virginia-you-really-need-one/

I haven't yet hammered out the details on how this should be implemented but I'm thinking we could use jsonschema for all events and then inside each module, you can set a variable that signifies which schema you use, do validation and what to do if we get an event which doesn't match the schema.

What does the salt team think about this ? My only argument against it is that it adds complexity in the system especially for smaller teams. We could get rid of this problem by making the schema optional.

[yagnik]

cc @vikky999

[Ch3LL]

Can I get @saltstack/team-core 's opinions here? Any thoughts ideas?

[cachedout]

This is something I have thought a lot about. The lack of a schema for events is one of Salt's biggest liabilities going forward, I think. In short, I'm very much in favor of this and would welcome a discussion about implementation details. If we can get some community support behind this, I would love to introduce this in the upcoming Oxygen release of Salt.

[thatch45]

I completely agree, we should have a schema and we should have guidelines on how to build events in a consistent way

[yagnik]

I spent some time reading up the code and how schemas et al are used in salt.
Here is a rough plan of what I'm thinking, please let me know if I'm going down the right path ( I apologize in advance for numerous questions ). I'd like to split this feature under multiple pull requests:

• Add a new class for event schema placed in https://github.com/saltstack/salt/tree/develop/salt. I don't believe that event schema should be placed in utils.
• Along with the class, we can have schema files for each "event type" (tag). The schema will be of type https://github.com/saltstack/salt/blob/develop/salt/utils/schema.py
• A global dict of tag <-> schema class mapping
• Update fire_event and get_event methods here https://github.com/saltstack/salt/blob/develop/salt/utils/event.py to support validating schema before sending and after receiving the message
• The above will be split into two parts, part 1 will validate but will let invalid schema go through
• Part 2 will validate and error out (this should be a flag in config for backward compatibility)
• We should remove this flag in a version after so we enforce schema everywhere

Some questions:

• Do we have an easy way to get a list of all tags and events that salt sends internally ?

Pending:

• I haven't hatched out the detail of how to extend events similar to modules but I believe we can do the same as module to extend events and event dictionary.

@cachedout @thatch45 @Ch3LL your views on the above ?

[cachedout]

I would prefer that we don't do any schema checking if a config flag is set because not everybody would want it and there's a chance there will be a performance hit. Other than that this looks good. Nice job @yagnik !

[cachedout]

And to your second question, there is no way to list all tags/events that salt sends right now, sadly. We'll have to grep through the codebase and find them by hand. :-/

[yagnik]

@cachedout is it a safe assume that all events have unit tests ? Can I just set the config to raise and then fix tests to make sure all events are covered ?

[yagnik]

Apologies for the ridiculous amount of delay in updating this. I have hammered out some work on this here: #43583. Would love to hear your thoughts on the same.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.