operations: Return proper error codes from UserMergeBranch
- Aug 30, 2021
-
-
Patrick Steinhardt authored
When `UserMergeBranch` fails during hook execution, then we will not cause the RPC to fail but instead embed the error in the response's `PreReceiveError` field. This has led to quite some problems: Praefect cannot determine failing RPC calls, we need to do weird games where we need to make sure to pass through Rails' error messages verbosely, and most importantly the upstream caller cannot easily deduce what really has failed without comparing returned error messages. As a result, this pattern which we use across many of our user-facing RPCs is quite lacking. To fix this, we can use gRPC's rich error model. This error model allows us to embed arbitrary proto messages into the returned error, which grants us the ability to return structured errors to the caller. Like this, we can embed various information into errors in a structured way which can then be extracted by upstream callers simply by unmarshalling those error details. Convert UserMergeBranch to use this rich error model as a proof concept. For now, rich errors only include details in case the access checks help to keep this change as focussed as possible. If this new model proves viable, we may extend this to also include additional information in different error cases, like for example the set of conflicting files in case a merge fails. Changelog: changed
-
Patrick Steinhardt authored
We don't have any tests which properly verify that UserMergeBranch works in the context of access checks. Add one which tests for allowed, disallowed and failing access checks to assert correct behaviour.
-
Patrick Steinhardt authored
Our merge-related tests still use old-style test names which don't follow our style guide. Rename them to fix this.
-
Patrick Steinhardt authored
The gRPC error model allows for two different usages: first the standard error model where one can return a simple error message associated with a code. And then the richer error model, where structured data can be added to errors such that details about the failure mode can passed to the caller [1]. In Gitaly, we always use the simple error model. This simple error model is quite limiting though. For example in the operations service, we have resorted to pass error information to the caller by embedding error details into the normal response. While not elegant, it has served us alright in the past. But this design is awkward, has issues with Praefect if it inspects proxied errors, and is just bad design in general. To fix this, this commit implements a new helper function which will enrich a gRPC status error with additional details. These details can be an arbitrary protobuf message. Like this, we can pass up arbitrary structured data to the caller, which will allow us to fix these shortcomings. [1]: https://www.grpc.io/docs/guides/error/
-
Patrick Steinhardt authored
Our helper functions to create gRPC errors is lacking wrappers for `PermissionDenied` error codes. Introduce them to prepare for a subsequent patch, where we start to return this code from `UserMergeBranch`.
-
Patrick Steinhardt authored
The GitLab mock client will currently always return success for all of its stubbed functions. In a subsequent patch, we'll want to write tests though which return various failures instead. Prepare for this by generalizing the mock client such that callers can pass arbitrary implementations of its mocked functions.
-
Patrick Steinhardt authored
In a subsequent commit, we'll need to stub out the hook manager such that we can use a different implementation for access checks. Extend `setupOperationsService()` to accept a variable number of test server options to prepare for this.
-
Patrick Steinhardt authored
When any of the hooks fails to execute in our `UpdaterWithHooks`, then we return a `PreReceiveError` to the caller. This is quite misleading though: we not only execute the pre-receive hook, but also others, and they all return the same error. Refactor the error to instead be a `HookError`. While at it, we also embed stdout and stderr in the error such that it can handle generation of the error message itself instead of using `hookErrorMessage()`, and implement `Unwrap()` such that we can use `errors.As()` to retrieve the embedded error. This will be used at a later point to distinguish `HookError`s and `NotAllowedError`s.
-
Patrick Steinhardt authored
The pre-receive hook implementation is responsible for calling GitLab's `/internal/allowed` API to do access checks. In case this API endpoint tells us that a change is not allowed, we need to make the error message available to the original caller, which then knows to return the error message via the response's `PreReceiveError` field. Our current implementation of this is wrong though: while we correctly pass up information via a `NotAllowedError` in case the access check indicates that the change is not allowed, we also pass up information when invoking the endpoint itself failed. But this really is an internal error, and we don't ever want to return such information to the user. Fix this issue by returning a normal error in such cases. Changelog: fixed
-
