Augment error behaviour #920
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…uringValidationMsg
pregnor
previously approved these changes
Jan 27, 2023
panyuenlau
reviewed
Jan 27, 2023
panyuenlau
approved these changes
Jan 27, 2023
pregnor
approved these changes
Jan 31, 2023
bartam1
pushed a commit
that referenced
this pull request
Feb 17, 2023
* add Unwrap method to package errorfactory and a unit test for it * temporary: add webhooks err check functions and unit tests * rename removingStorageMsg var and add comment about the use of errorDuringValidationMsg * sanitize error unit tests and reorder public error check functions * refactor to use errors.As instead of errors.Cause which no longer fits * replace pkg alias apiErrors with apierrors in pkg webhooks * camelCase testName and testCases in errors_test.go from pkg webhooks * add comment for Unwrap method and shorten public error comparison functions in pkg webhooks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's in this PR?
This PR aims to improve the errors implementations (in the errorfactory and webhooks packages).
Along the way, I'm also renaming a couple of variables and functions to better reflect their usage.
The custom error types from the errorfactory package can be improved and brought up to date (to Go > 1.13 recommendations) by adding an
Unwrap()method for each one of these types returning the embeddederrorso that errors that could be wrapped inside can be inspected and matched on with all of their details, not just the string representation, witherrors.Is()anderrors.As().I'm also adding a unit test for the
Unwrap()behaviour.The constant error message strings from the webhooks package don't need to be exported, but, because we still have to do some string matching, we can add exported matching functions for all them much like we have right now for 2 of them (
IsAdmissionCantConnect(error) boolandIsInvalidReplicationFactor(error) bool). This will facilitate and hide the details of the string matching that we need to do.I'm also adding unit tests for these new functions.
The changes made to the
controllerspackage reflect the updated usage pattern of the errors from theerrorfactorypackage. Without these, that code no longer works correctly.The previous model that performed a
type switchon the error returned byemperror/errors.Cause(err)no longer works because the error types from theerrorfactorypackage can now Unwrap() and return the errors inside, not just stopping the unwrap immediately and returning the outer type. Because we no longer break the error chain, the "last" error in the chain (deemed as "Cause") is no longer the type from theerrorfactorypackage but whatever it wraps or further.Now, we can and should use the standard
errors.As(...)function instead which doesn't require the type we seek to be at the very end of the chain but anywhere inside the chain of wrapped errors. Yes, we currently use theAs(...)function from the emperror/errors package but that translates directly to theerrors.As(...)from the standard library.Additional context
Currently, in koperator we have 2 patterns for our own errors:
These embed the builtin "error" type and wrap external errors passed in to the errorfactory.New function alongside lists of "details". We make use of the
emperror/errorsmodule which returns error types that can be chained (have both Error() and Unwrap() methods).Right now errors like APIFailure (used like in koperator/pkg/k8sutil/resource.go) can be inspected or matched on for type but we lose the details of the error(s) wrapped inside. This can be improved by adding an
Unwrap()method for each one of these types returning the embedded error.These are unexported constant strings used during the validating webhook functions. These are used as parts of the strings of some log and error messages.
These end up used to create either an
erroror afield.Error(k8s.io/apimachinery/pkg/util/validation/field). In both situations, the ValidateCreate and ValidateUpdate sets of methods return errors that, underneath, are ofStatusErrortype (apimachinery/pkg/api/errors/errors.go) fromapimachinery. This type hides all other details from the wrapped errors except for the accumulated string. Sometimes the error chain is broken even earlier by other error types from apimachinery.The downside to the error types we end up returning from the webhook functions is that we don't have a reasonable solution to avoid doing string matching to discriminate between errors.