ref(crdconvert): respond with errors from all requested resources #4601
Conversation
Currently, CRD conversion webhooks only respond with the error returned by the first failed resource converted, but there may be several. This change aggregates errors from all resources sent in a request. Signed-off-by: Jon Huhn <[email protected]>
nojnhuh
requested review from
shashankram,
snehachhabria,
draychev and
jaellio
as code owners
Could you comment on why the aggregation of errors is necessary here? I am guessing it may be important in some circumstances to know about all the errors that might occur to be able to diagnose an issue pertaining to the conversion. I am curious to know why this is necessary in this particular case. |
Codecov Report
@@ Coverage Diff @@
## main #4601 +/- ##
==========================================
+ Coverage 68.87% 69.26% +0.39%
==========================================
Files 217 217
Lines 15019 15021 +2
==========================================
+ Hits 10344 10405 +61
+ Misses 4623 4563 -60
- Partials 52 53 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
It's not strictly necessary for the conversion to work, but I see it as generally useful to return as much information as we have available for users to be able to fix errors faster than one at a time. My primary motivation for this change is to accurately count metrics which I tried to describe above. Currently, if a request comes in to convert two resources and converting the first resource fails, the response would indicate converting both failed but the metrics would only count one failed. |
Please also document this in the code, so someone doesn't revert this back to return the first error as a form of simplification for the existing code. |
Signed-off-by: Jon Huhn <[email protected]>
pkg/crdconversion/framework.go
Outdated
| // doConversion converts the requested object given the conversion function and returns a conversion response. | ||
| // failures will be reported as Reason in the conversion response. | ||
| func doConversion(convertRequest *v1beta1.ConversionRequest, convert convertFunc) *v1beta1.ConversionResponse { | ||
| var convertedObjects []runtime.RawExtension | ||
| // aggregate errors from all objects in the request vs. only returning the first |
There was a problem hiding this comment.
nit: This comment is missing why aggregating errors helps in this case.
Signed-off-by: Jon Huhn <[email protected]>
|
GitHub Actions seems to be acting up at the moment per https://githubstatus.com, so the regular CI tests aren't running for the newest commit here. Since the previous commit passed aside from the known image scan issue for alpine that has a fix upstream in progress and I've only changed a code comment since then, I'm going to merge this. |
Description:
Currently, CRD conversion webhooks only respond with the error returned
by the first failed resource converted, but there may be several. This
change aggregates errors from all resources sent in a request.
In addition, for #4568 I'm planning to add logic to record metrics labeled with the requested resources'
apiVersionandkindfrom objects parsed out of the conversion request. Those changes (WIP) are shown here: e5587d6#diff-c4fc6ee6327a461684ba57d2d0cc26326c4cb0a980119d85cf93c730c1a7ec65. Without this change, resources in the request beyond the first one that fails will not have metrics recorded showing they were also failed to be converted.Testing done:
Affected area:
Please answer the following questions with yes/no.
Does this change contain code from or inspired by another project? No
Is this a breaking change? No
Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)? N/A