Bundle status updates are missing errors

[kristiansvalland]

Short description

Status updates on the status update API are missing errors. Specifically, the errors are replaced by "{}" in the bundles[_].errors array.

Expected behavior

The elements in the bundles[_].errors array should be the result from calling error.Error(), as we probably want to know why a bundle update failed.

Suggested solution

The errors array is an array of errors, which being interfaces, marshals to "{}" because the json package does not know how or what to marshal. Implement json.Marshaler for the bundle.Status struct that correctly marshals the errors field.

Additional context

Related, but not exactly the same problem can be found in #2787. This suggests also implementing the json.Unmarshaler interface for the Status struct. A benefit of doing this is that implementations of control planes in golang can import and use this struct directly when unmarshaling HTTP requests. However, unmarshalling of the errors array would likely need to use the errors.New(string) method, loosing information about the original error type.

See also this issue in golang for marshaling of errors: https://github.com/golang/go/issues/5161

I do have capacity to work on a fix for this assuming the description of the wanted behaviour is okay.

[anderseknert]

Thanks for an excellent bug report, @kristiansvalland! Seems like you've identified a viable solution too, but could be there are some gotchas I'm not aware of. @srenatus WDYT?

[ashutosh-narkar]

The elements in the bundles[_].errors array should be the result from calling error.Error(), as we probably want to know why a bundle update failed.

Are you saying you weren't able to figure out from the Status if bundle activation failed ? Specifically code and message status fields should have captured the error. The errors field is currently used to capture parse or compile errors.

[kristiansvalland]

Hi! I see your point, @ashutosh-narkar. Perhaps I could have gotten the info I needed from those fields instead. The problem is that I don't know what to parse the array of errors as as they are sent to the control plane. We have our own representation of the Status struct, and we want typed fields and not have our errors array be an []interface{} or []struct{}.

Your remark also raises the question why the bundles[_].errors is at all part of the struct if I should rather use the other fields -- or is the the Status struct used for other things than eventually posting to the status update API? I am admittedly not extremely familiar with the whole context here.

Indeed we are dealing with compile errors on our side.

In any case, I think that either the .errors must somehow be turned into something else than the non-informative "{}", or the field and documentation of it should be removed, as currently the description "array Collection of detailed parse or compile errors that occurred during activation of this bundle" can be a bit misleading to me as a control plane developer that wants this information.

Thanks for your response to my issue! :)

[ashutosh-narkar]

Indeed we are dealing with compile errors on our side.

So in your case, there is a compile error which you probably saw from the message field but the error field is set to {}. If this is correct, can you please help us repro the issue ?

[kristiansvalland]

It seems that the errors are not empty after all, but contains elements such as

"errors": [{
    "code": "rego_parse_error",
    "message": "unexpected keyword, must be one of [in]",
    "location":{"file":"main.rego","row":2,"col":8},
    "details":{"line":"import future.keywords.every","idx":7}
}]

So this can then be fixed on our side by changing the definition of the errors to be a struct with these fields it seems. When I was testing this using the source code directly, I was naively using fmt.Error, which turned into the "{}" after json marshaling, but indeed the only type of error that is set to this field is the Error type defined in "ast/errors.go", which happens here, and that turns into json just fine.

Sorry about filing an erronous bug report.

Yet, I would like to improve the documentation of the errors field by specifying the type and subtypes, as currently the only way to know is to check empirically or to look into the source code. Would the page https://www.openpolicyagent.org/docs/latest/management-status/ be the right place to do that? An example error can be added to the example json there, the structure can be defined in the description below it, or potentially both. Let me know what you think.

Also feel free to remove the bug label.

[ashutosh-narkar]

Yes @kristiansvalland that would be right place to update the doc. I think improving that section would be great. Thanks!

[stale[bot]]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

[ashutosh-narkar]

Feel free to make the doc updates and re-open the issue when you get a chance.