| Age | Commit message (Collapse) | Author |
|---|
|
There's a bunch of generic packages that really don't depend much on the
repository object format. Convert them to test with the SHA256 object
format.
|
|
gRPC depends on the target's scheme to determine which resolver to use.
Built-in DNS Resolver registers itself with "dns" scheme. We should use
a different scheme for this resolver. However, Ruby, and other
cares-based clients, don't support custom resolver. At GitLab, the gRPC
target configuration is shared between components. To ensure the
compatibility between clients, this custom resolver use same "dns"
scheme.
We don't want to mess up the default global registry. Clients are
expected to inject this resolver via exposed WithGitalyDNSResolver dial
option.
|
|
This commit sets the client-side load-balancing strategy to round-robin
via grpc.WithDefaultServiceConfig dial option. This strategy is only
effective if the host target can be resolved to multiple addresses. So,
it's fairly safe to set it by default.
|
|
|
|
Reformat surces with the newly upgraded gofumpt v0.4.0.
|
|
With 7e87131ed (golangci-lint: Allow `testing.T` as first parameter,
2022-08-10) we set `context-as-argument` the sole argument to
`revive:rules`. This causes golangci-lint to disable all revive lints
that were previously enabled by default.
Re-enable the default revive lints by explicitly listing them in our
config. This triggers a large number of lint errors, resolve these as
well in this commit.
|
|
The `grpc.Server.Serve()` function will return an error when the gRPC
server has either failed unexpectedly or in case it wasn't properly shut
down. We don't check this error in many places though, which both causes
the errcheck linter to complain and hides issues we have with some of
our test setups.
Fix the missing error checks via a new `testhelper.MustServe()` helper
function that does the error checking for us. This surfaces some test
errors for tests that forget to shut down the server or that manually
close the listener, which should be handled by gRPC itself. All of those
errors are fixed while at it.
Drop the corresponding linting exceptions.
|
|
We don't properly check the error when closing the `SidechannelWaiter`
in many places. Fix this and drop the corresponding linting exceptions.
|
|
Do not assume client users want the same tracing interceptors with the same configuration.
|
|
Enforce consistent naming of `testing.TB` variables, which should be
called `tb`, and adapt tests that violate this rule.
|
|
We currently exclude a revive rule that `context.Context` should be the
first parameter for our test sources. This can be handled better though
because golangci-lint allows us to exclude certain types from this rule.
Adapt the rule to allow `testing.T` et al before `context.Context` and
remove the excluded rule. Interestingly, this now surfaces a whole bunch
of `nolint: revive` annotations that aren't needed anymore, so we fix
them in the same commit.
|
|
Convert the client's dialling tests to use the `grpc_testing` test
service instead of the `grpc_proxy` one.
|
|
We're about to add the ability to test with SHA256 hashes. We assume
none of the tests work with this object format.
With this change we add the build constraint to not run any test when
the tag 'gitaly_test_sha256' is set.
|
|
Rewrite callers of `testhelper.ModifyEnviroment()`, which is about to be
removed in favor of either `t.Setenv()` or `testhelper.Unsetenv()`.
|
|
This commit changes the major version in the package name from v14 to
v15
Updating go.mod & go.sum with new module name v15
Update Makefile to bump major version to v15
Update the gitaly package name in the Makefile. Also update
gitaly-git2go-v14 -> gitaly-git2go-v15. We need to keep
gitaly-git2go-v14 for a release however, for zero downtime upgrades.
This pulls directly from a sha that is v14.
Update package name from v14->v15 for auth, client, cmd, internal packages
This commit changes the package name from v14 to v15 in go and proto
files in the internal, auth, client, cmd packages.
proto: Update major package number in package name
tools: Change major version number in package name from v14 to v15
gitaly-git2go: Change the package name from v14 to v15
update module updater for v15
Update the documentation for the module updater to reflect v15
|
|
testhelper.Context() currently return a cancellation function as a
second return value. Majority of the tests do not need to explicitly
cancel the context but they simply defer its cancellation to clean up
after the test. Given this, we can reduce the test verbosity and make
testhelper.Context easier to compose by removing the unnecessary second
return value. This adds a t.Cleanup function to automatically cancel the
context at the end of the tests and omits the returned cancellation
function.
Tests which simply `defer cancel()` have had the extra call removed. Some
tests explicitly call the cancellation, and these tests have been modified
to add context.WithCancel around the testhelper.Context call. There are a
few loctions where testing.TB was passed down to test helpers that create the
context.
|
|
This makes it easier to test sidechannel RPC's in downstream consumers
(gitlab-workhorse, gitlab-shell).
Changelog: other
|
|
Add SSHUploadPackWithSidechannel
Closes gitlab-com/gl-infra/scalability#1513
See merge request gitlab-org/gitaly!4251
|
|
This adds UploadPackWithSidechannel which will be used internally by
gitaly-ssh, and externally by gitlab-shell.
|
|
Prior to this change, ModifyEnvironment was returning a function to
revert the changes done. This was typically passed to defer or to
t.Cleanup().
This change moves calling t.Cleanup() to the ModifyEnvironment function
itself.
|
|
There's a bunch of tests which use `ContextWithTimeout()` without them
really requiring it. We do not want to benchmark how fast the system
under test is, and the timeouts may cause flaky tests on busy and/or
slow machines.
Remove usage of `ContextWithTimeout()` from tests which don't need it.
|
|
The naming of the `GrpcEqualErr()` helper function is quite irregular
compared to other helper functions we have. Most importantly, it's
significantly different from `RequireGrpcCode()` and may thus be easy to
miss.
Rename it to `RequireGrpcError()` to clarify this.
|
|
Personally, whenever I want to compare gRPC errors and/or messages, I
stand there asking myself where the helper I want is located again. This
is caused by a split we have in test helpers related to gRPC: while we
do have `testhelper.RequireGrpcError()`, checking for actual errors is
done via `testassert.GrpcEqualErr()`. I always have to look up which of
both does what and where it's located.
Fix this by absorbing the helpers from the testassert package into the
testhelper package.
|
|
The testhelper has functions to create test loggers for us. These are
awkardly named though, and furthermore we have a global variable which
contains a reference to one of the functions which creates the logger
for us, which is bad design. Furthermore, this global variable isn't
modified by anything anymore, making it essentially the same as the
function it's aliasing.
Rename functions to match Go best practices and remove the global
variable.
|
|
Most of the linting exception we have are about missing documentation of
public variables or comments. Let's inline all of these -- it's hard to
keep track of exceptions and keep the list up-to-date. By having a
`//nolint` directive at the site where the document is missing it
becomes a breeze to directly clean it up. Furthermore, it also acts as a
reminder that one might want to add a comment when one stumbles over any
of these comments.
|
|
Clean up various exceptions for the errcheck linter, either by removing
exceptions which don't apply anymore, by handling the errors or by
ignoring errors.
|
|
Setup of a test package's main function is quite repetitive: we call a
separate `testMain()` function such that we can use `defer` calls, this
function calls `MustHaveNoChildProcess()` and `Configure()`, and then we
return the result of `m.Run()`. This is almost always exactly the same,
with some exceptions where we need to have additional setup code.
Unify this code via a single `testhelper.Run()` function which does all
of this. It allows us greater flexibility in the setup code such that we
can easily perform additional validation after the tests without having
to modify all callsites.
Note that this change removes calls to the goleak package in some
places. These will resurface in a later commit in this patch series,
where we instead move this call into `testhelper.Run()` such that it is
executed by default.
|
|
This change adds publicly exported code to the gitaly/client package
that allows Gitaly clients to accept sidechannel connections coming
back from a Gitaly server they have connected to. This is done via a
new dial function DialSidechannel.
Changelog: added
|
|
Reformat sources with gofumpt.
|
|
It is not common to use snake case names for packages
and package aliases in Go.
The change renames aliases to a preferred single word name.
We also use 'gitaly' prefix for the project-defined packages
that clashes with standard or 3-rd party package names.
|
|
Changelog: changed
|
|
|
|
The new "v14" version of the Gitaly module is named to match
the next GitLab release. The module versioning is needed in
order to pull gitaly as a dependency in other projects. The
change updates all imports to include v14 version. The go.mod
file was modified as well after go mod tidy execution. And
the changes in dependency licenses are reflected in the NOTICE
file.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/3177
|
|
Convert `TempDir()` to use `t.Cleanup()` instead of returning a cleanup
function.
|
|
Praefect needs to provide a custom handshaker to use when establishing
a connection to Gitaly in order to set up the multiplexed connection.
This commit allows for injecting a handshaker into the Dial function
to perform the custom handshake.
|
|
DialWithMux should not be part of our public API as it is an
implementation detail between Praefect and Gitaly. Let's remove
DialWithMux from the public package thus and use the internal dialing
API in Praefect.
|
|
Praefect requires a more flexible API to plug in its multiplexing
implementation to when dialing to Gitaly. This implementation should
not be part of the public API interface so we don't have to maintain
backwards compatibility. This commit moves the internals of the dialing
into an internal subpackage. This way we can retain the exisiting public
APIs we offer in `client` package and provide more flexible APIs for
internal use in `ìnternal/gitaly/client`.
|
|
This commit adds DialWithMux to client package which dials to Gitaly
using a multiplexed connection. This is an experimental function that
will be removed in the future and is not considered for backwards
compatibility. This function is only going to be used for performance
testing the multiplexed connection.
|
|
There was a previous attempt at synchronizing the test by waiting
for the server to close the stream. There is another race in the test
as well though.
The client first sends the stream close to the server and then goes off to finish
the client span. If the server receives the client stream closing before
the client managed to finish its span, the server will race finishing
its 'nested span' with the client's '/mwitkow.testproto/TestService/PingStream'
span.
This commit fixes the race by synchronizing the server's stream handler to only
proceed after the client has returned from the CloseSend method.
|
|
This commit fixes TestDial_Tracing/stream which is flaking and failing
the pipeline every now and then.
The ordering of the items is indeterministic due to a race in finishing
two spans. The main test goroutine does not wait for the stream to close
but only receives a single message. Once that message is received, both
the main test goroutine and the stream handler race to finish their spans.
This commit fixes the situation by making the main test goroutine wait for
the stream to close and only then finishing its span.
|
|
Gitaly allows to set up global tracing, which may create spans for
several events. Each span denotes the start and end point, what is
happening and some arbitrary baggage which is user-defined.
We have a test which exercises this code and asserts that spans are
correctly created and registered across RPC calls, but the test is flaky
and causes lots of CI failures. The failure mode is kind of weird, in
that spans sometimes have a different order than the order we expect.
It is unclear what exactly is causing the races, and personally I have
not been able to reproduce it locally. This commit thus refactors both
testcases and makes them more explicit about what kind of data they
expect, which hopefully helps in nailing down what's happening.
One thing that _may_ be a potential source of the bug is that we now
flush the tracer earlier. Previously, we'd only close the tracer after
the testcase ran. But given that the close may also flush any pending
events, this could potentially be a source of uncertainty.
|
|
The `testhelper.GetTemporaryGitalySocketFileName()` function currently
returns an error. Given that all callers use `require.NoError()` on that
error, let's convert it to instead receive a `testing.TB` and not return
an error.
|
|
We're about to migrate `testhelper.GetTemporaryGitalySocketFileName()`
to receive a `testing.TB` as argument. Let's inject this testing context
when starting listeners in "./client/dial_test.go" to be prepared for
this change.
|
|
The default minimum TLS version in Go is currently TLS 1.0. Because
there are known weaknesses and vulnerabilities in both TLS 1.0 and
1.1, GitLab has announced the end of support for those versions on
October 2018 already [1]. But Gitaly never followed through and still
accepts the old TLS versions.
Fix this by raising the minimum required TLS version to TLS 1.2 for both
client- and server-side connections.
[1]: https://about.gitlab.com/blog/2018/10/15/gitlab-to-deprecate-older-tls/
|
|
We're currently using `NewServerTLSFromCert()`, which is inflexible and
doesn't allow us to tweak details of the resulting TLS transport
credentials. This commit converts them to use an equivalent invocation
of `NewTLS()` instead, which allows us to specify the `tls.Config`
directly.
No change in behaviour is expected from this commit.
|
|
We're currently using `NewClientTLSFromCert()`, which is inflexible and
doesn't allow us to tweak details of the resulting TLS transport
credentials. This commit converts them to use an equivalent invocation
of `NewTLS()` instead, which allows us to specify the `tls.Config`
directly.
No change in behaviour is expected from this commit.
|
|
|
|
The golangci-lint project has a set of default excludes for linters
which are applied by default. This means that we cannot choose ourselves
which linting messages we want to include, as some of them cannot be
controlled by us.
One noteworthy linting check which is excluded right now checks whether
comments for types and functions are prefixed with their respective
name. This is something which we try to enforce in Gitaly, too, but
naturally there's cases where it slips through review.
So let's disable the default excludes such that we can enable this
linting check. As it turns out, there's quite a lot of cases where we
failed to adhere to this style, which this commit also fixes. One thing
which we don't do though is to have per-package comments, so let's
actively ignore it for now.
|
|
It could be possible the same address will require a new token
to be used. This could be possible in case of the new token
rollout. The simplest approach is to make a token part of the
key in the connections pool. With that the same address will
be able to have multiple connections with different token.
Closes: https://gitlab.com/gitlab-org/gitaly/-/issues/2699
|
|
There is no way we could identify if the connection we establish
to remote is properly configured and could be used with no issues.
As a workaround on it is to issue a request to the remote when
connection is set. The simplest approach is to issue a health check
request as both gitaly and praefect expose this service outside.
As we don't want to make it after each call to Dial as it could be
missed or if we change implementation later it makes sense to define
a custom Dialer for that purpose. It should be used with the pools
we are using except of praefect connection pool to gitaly nodes,
as it would prevent praefect from start if one of gitaly nodes is not
accessible.
Closes: https://gitlab.com/gitlab-org/gitaly/-/issues/2699
|
