Improve error handling across kong & ingress-nginx#152
Improve error handling across kong & ingress-nginx#152k8s-ci-robot merged 1 commit intokubernetes-sigs:mainfrom
Conversation
k8s-ci-robot
added
kind/bug
k8s-ci-robot
added
the
size/S
| httpRoute, ok := gatewayResources.HTTPRoutes[key] | ||
| if !ok { | ||
| panic("HTTPRoute not exists - this should never happen") | ||
| return field.ErrorList{field.InternalError(nil, fmt.Errorf("HTTPRoute does not exist"))} |
There was a problem hiding this comment.
On lines 44-45 we get errors from calculateBackendRefWeight. Although you replaced panic with returning an error, the errors above are still invisible and the user can not know whats wrong.
The concern is relevant for changes in Kong as well.
There was a problem hiding this comment.
That's a good catch. In those cases, instead of panicking or returning uninformative errors such as "HTTPRoute does not exist," we could continue to the next iteration. All the informative errors will be collected and returned at the end of the for-loop
WDYT @LiorLieberman @mlavacca?
There was a problem hiding this comment.
continue plus a comment on top of this to explain sounds good to me.
Also since you changed kong/converter.go to the logic proposed, I think is ok to log and return an error because in kong this indeed should never happen.
There was a problem hiding this comment.
continue plus a comment on top of this to explain sounds good to me.
Done
Also since you changed kong/converter.go to the logic #152 (comment), I think is ok to log and return an error because in kong this indeed should never happen.
I'm not exactly sure what you mean here
There was a problem hiding this comment.
for all kong featureParsers changes you have added, (returning error instead of panic) maybe we should add that this should never happen? or something more clear to the user?
Because this indeed should never happen now with the change you added to check the errors and return before calling the featureParsers in kong/converter.go
I hope this is a bit more clear
There was a problem hiding this comment.
OK got it now. I added back the "this should never happen" message. If we indeed encounter this error at some point for whatever reason we'll figure out how to address it better
levikobi
force-pushed
the
improve-error-handling
branch
from
18cd934 to
67cbfa8
Compare
k8s-ci-robot
added
size/M
levikobi
force-pushed
the
improve-error-handling
branch
from
67cbfa8 to
c25070c
Compare
|
/lgtm |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: levikobi, LiorLieberman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
levikobi
force-pushed
the
improve-error-handling
branch
from
c25070c to
6b75fe1
Compare
k8s-ci-robot
removed
the
lgtm
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
|
thanks, |
k8s-ci-robot
added
the
lgtm
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Improves error handling across Kong and ingress-nginx providers by removing panic calls and stopping conversions if an error occurs.
Also, prevents the tool from crashing in case no
pathTypeis specified.Which issue(s) this PR fixes:
Fixes #151
Does this PR introduce a user-facing change?: