-
Notifications
You must be signed in to change notification settings - Fork 7
Minimal validator #1187
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
base: main
Are you sure you want to change the base?
Minimal validator #1187
Conversation
Frostman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fredi-raspall, we need to agree on the output format; the current one would be very difficult to consume to produce a user-friendly message and difficult to parse on top of it. I'd suggest to at least flatten it a bit into list of errors and move the error type into field type instead of it being a key and always have an object as a root, e.g.:
{
"success": true
}
or
{
"errors": [
{
"type": "DeserializeError",
"message": "invalid value: integer `-1500`, expected u32 at line 79 column 20",
"line": 79,
"column": 20,
"category": "Data",
"context": {
"74": " },",
"75": " \"protocolIP\": \"7.0.0.100/32\",",
"76": " \"vtepIP\": \"7.0.0.100/32\",",
"77": " \"vtepMAC\": \"02:aa:bb:cc:dd:ee\",",
"78": " \"vtepMTU\": -1500,",
"79": " \"workers\": 1",
"80": " },",
"81": " \"groups\": {",
"82": " \"gw-group-1\": {",
"83": " \"members\": ["
}
},
{
"type": "ConversionError",
"message": "Invalid Gateway Agent object: Could not create VPC: '0' is not a valid VNI"
},
{
"type": "ConfigurationError",
"message": "blah blah 1"
},
{
"type": "ConfigurationError",
"message": "blah blah 2"
},
{
"type": "ConfigurationError",
"message": "blah blah 3"
},
]
}
I additionally suggest renaming hint to message and making it mandatory for all errors to have a message.
validator/Cargo.toml
Outdated
| ordermap = { workspace = true, features = ["serde"] } | ||
|
|
||
| serde = { workspace = true } | ||
| serde_json = { workspace = true } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline missing
Ok, I'll see what I can do. Note that some of the errors are exclusive. I.e., if we fail to deserialize, we'll not be able to validate the config. So you'll never see "DeserializeError" along with "ConversionError".
Ok! |
5ec6a34 to
b4cdc48
Compare
|
@Frostman , I've pushed a new version with a simplified output. Samples I wonder if we should add an optional "rule" field (or "rule-violation") that explains the user the reason why a configuration may be rejected in general terms. E.g. |
b4f170d to
5fd28e6
Compare
We need a new crate to be able to validate gw configurations offline, without starting dataplane. This patch adds a vanilla validator, as a rust binary. The intent is to compile this validator as wasm/wasi and use it externally. The validator expects a GatewayAgent CRD from stdin and produces a JSON-encoded response in stdout. Signed-off-by: Fredi Raspall <[email protected]>
Simplify/flatten the reply JSON object.
With this patch, the validator will output a JSON object
with mandatory success field set to true and an empty
error list. E.g:
{
"success": true,
"errors": []
}
If there is an error, success will be false and errors
contain one or more error objects. Each of the error
objects contains a type, a message and, optionally a
subtype.
Type is one of:
"Environment"
"Deserialization"
"Metadata"
"Conversion"
"Configuration"
The optional subtype is informational. Currently it is
only set for errors of type Deserialization.
E.g:
{
"success": false,
"errors": [
{
"type": "Deserialization",
"subtype": "Syntax",
"message": "trailing comma at line 8 column 3"
}
]
}
Signed-off-by: Fredi Raspall <[email protected]>
5fd28e6 to
799b546
Compare
- supports both json and yaml inputs (stdin) - output is in yaml (stdout) - no longer use serde_json but serde_yaml_ng - subtype is removed as the output of serde_yaml_ng is okay, which significantly simplifies Deserialization errors. - print to stderr if response cannot be deserialized in stdout Signed-off-by: Fredi Raspall <[email protected]>
|
Tried the latest binary @Fredi-raspall shared in Slack, and it looks ok. I think we can merge in that state and main work would be to make all validation messages as user friendly as possible. |
Addresses #1174
I will open a separate PR to:
* refine the feedback and the information provided by some of the errors.
* move some checks from conversion to validation, as they pertain there.
* see if an exhaustive (non-early-exited) validation can be added.
Sample deserialization errors
In these errors, a list of lines (context) near the issue is returned. The line number is not too exact (depending on the error type it is offset by +/-1 or 2). We should seldom (if ever) see deserialization errors when the input JSON is machine generated. So this is mostly for testing manually crafted configs.
Sample Metadata errors
Sample Conversion errors
Sample Configuration errors
Success case