| Age | Commit message (Collapse) | Author |
|---|
|
The `log.LogrusLogger` type directly wraps the `logrus.Entry`, thus
exposing all of its functions. We don't want anybody to call them in the
ideal case so that we can ensure that we use our own logging interface
almost exclusively throughout the codebase.
Let's hide the `logrus.Entry` by making it a private member of the
`log.LogrusLogger`. While this requires us to explicitly implement some
of the expected interface-level functions, by now it's really only four
of them and thus manageable.
There are still some very limited callsites in our tests that require
access to the `logrus.Entry`, which will be addressed in another
iteration. Meanwhile, we expose an explicit `LogrusEntry()` getter that
makes the entry accessible to those usecases.
|
|
The `log.Logger` type still converts to a `logrus.Entry` when calling
functions like `WithField()` or `WithError()`. This was hard to change
though because a lot of callsites still relied on this conversion. By
now we have have fixed all blockers though, which allows us to finally
do the conversion and have the `log.Logger` type roundtrip to itself.
To do so, we inline the `logrus.FieldLogger` interface and adapt the
specific functions to return a `Logger` instead of a `logrus.Entry`.
This change also demonstrates that all internal interfaces now use the
interface exclusively and that we are free to adapt the interface as
required from now on.
Note that we also have to add two additional functions to the interface
to create the logging interceptors for the gRPC server. This is because
we have no way to create these interceptors manually from a `log.Logger`
anymore and thus need its help to do so for us.
|
|
Refactor out testhelper functions that create a logger to return our own
internal `log.LogrusLogger` instead of a `logrus.FieldLogger`. Add an
abstraction layer around adding hooks into the logger such that tests
don't need to reach into the logrus logger in general.
|
|
Split out Gitaly-specific client logic for SSHReceivePack, SSHUploadPack
and SSHUploadArchive. This is the last remaining part to ensure that
none of our own internal packages use the public `client` package.
|
|
With our migration to use the internal pool functionality we have also
converted all callsites to use `testhelper.MustClose()` to close the
pool with proper error checking in place. Because of that, we only have
a single callsite left where we don't check the error code when closing
the pools, which thus makes our linter rule superfluous where we don't
require error checks for that function.
Remove the rule and add error checking to the last remaining callsite
that doesn't.
|
|
Move the `FailOnNonTempDialError` logic into our internal client
package. Like this we can reuse the functionality internally without
having to rely on our public interface, which allows us to iterate on
its internal implementation without breaking the public API.
|
|
Move the `HealthCheckDialer` logic into our internal client package.
Like this we can reuse the functionality internally without having to
rely on our public interface, which allows us to iterate on its internal
implementation without breaking the public API.
|
|
Move the `Pool` logic into our internal client package. Like this we can
reuse the functionality internally without having to rely on our public
interface, which allows us to iterate on its internal implementation
without breaking the public API.
|
|
Move the `DialSidechannel()` logic into our internal sidechannel
package. Like this we can reuse the functionality internally without
having to rely on our public interface, which allows us to iterate on
its internal implementation without breaking the public API.
|
|
This commit wires the tests to call SharedLogger instead of NewLogger
so they'll use the test case's shared logger. This way the log output
is printed to the same logger and is ordered.
|
|
Gitaly now has a helper for recording log output and outputting it
only in tests. Replace the usage of the discarding logger with the
recording logger. These changes are search and replace so these call
sites didn't even need a log entry to begin with but rather use a
FieldLogger.
NewDiscardingLogEntry is removed as it is no longer used.
|
|
Gitaly now has a new logger for tests which outputs the logs on test
failure only. Replace all usage of discarding logger with it to capture
logs instead of discarding them.
NewDiscardingLogger is unexported as it is no longer used except
for newServerLogger.
|
|
Our style guide states that the `TestMain` function should be placed in
a file named `testhelper_test.go` for consistency's sake. A number of
packages were placing the test in a standard test file.
In cases where `TestMain` was the only function in that file rename it
to `testhelper_test.go`. If there were other functions, relocate
`TestMain` into a new `testhelper_test.go` file.
|
|
The `DefaultDNSResolverBuilderConfig()` constructs a default config that
is to be used by dependents on the Gitaly configuration. Part of this
default config is the logger, which we set to `log.Default()`. This is
arguably unexpected though, because the default logger will cause us to
print message to standard output.
Let's refactor the code to instead use a discarding logger by default.
If clients want to have logging for this code, they should instead
supply their own logger.
|
|
Convert the dnsresolver, backchannel and sidechannel packages to accept
logging interfaces instead of an explicit `logrus.Entry`. Later down the
road we will provide our own interfaces to abstract the `logrus` package
away from our own internal interfaces, so this change is an early step
into that direction.
Note that this requires a change in the backchannel package to convert
the field logger to an `logrus.Entry` by calling `WithField()` first.
We do this by adding a "component" field that points to backchannel,
which has the added benefit that we can now more clearly identify when
log messages come from inside that subsystem.
Furthermore note that we need to change the logging middleware tests to
not import our public "client" package anymore and instead use our
internal one. This change is required due to a new cyclic dependency
between the "internal/log" tests and the "client" package.
|
|
Gitaly's homegrown DNS resolver triggers DNS queries via UDP. Those
queries run independently in dedicated goroutines. They are controlled
by a timeout context. Recently, the lookup timeout is hard-coded to
15 seconds. This commit adds LookupTimeout configuration to the DNS
resolver.
Apart from providing a new config to clients, the new config is used to
control and prevent goroutines from being leaked after the test suite is
done. In the grpc-go versions before v1.56, the resolver is closed when
the bound connection is closed. After v1.56, the resolver runs more
independently. The connection doesn't wait for the resolver to finish
anymore. After the test suite finishes, there might be some in-flight
DNS queries are managed by the DNS resolver. When the connection is
closed, those queries don't make sense anymore. They can stay until they
finish. Unfortunately, the goroutine detector in the test suite
complains about leaked goroutines. The resolver of a connection is not
accessible from the outside.
Hence, the test sets a low lookup timeout and sleeps for a while until
the pending DNS queries go away.
|
|
|
|
|
|
The `client.Dial()` function already accepts a set of optional
parameters. Furthermore, we're about to add another optional parameter,
which will make the function's signature quite unwieldy.
Refactor the function to instead accept options.
|
|
Our internal client package is hosted in `internal/gitaly/client` even
though it isn't Gitaly-specific at all. Move it into our new gRPC
specific package to avoid confusion.
|
|
We've grown quite a bunch of packages that provide various different
functionality in the context of gRPC. But it is not easy to identify
them as there is no common hierarchy for gRPC-related functionality.
We're thus introducing a new `internal/grpc` package that will host
various bits and pieces related to gRPC.
Move the `internal/dnsresolver` package into this new package.
|
|
We've grown quite a bunch of packages that provide various different
functionality in the context of gRPC. But it is not easy to identify
them as there is no common hierarchy for gRPC-related functionality.
We're thus introducing a new `internal/grpc` package that will host
various bits and pieces related to gRPC.
Move the `internal/listenmux` package into this new package.
|
|
We've grown quite a bunch of packages that provide various different
functionality in the context of gRPC. But it is not easy to identify
them as there is no common hierarchy for gRPC-related functionality.
We're thus introducing a new `internal/grpc` package that will host
various bits and pieces related to gRPC.
Move the `internal/sidechannel` package into this new package.
|
|
We've grown quite a bunch of packages that provide various different
functionality in the context of gRPC. But it is not easy to identify
them as there is no common hierarchy for gRPC-related functionality.
We're thus introducing a new `internal/grpc` package that will host
various bits and pieces related to gRPC.
Move the `internal/backchannel` package into this new package.
|
|
We're about to release Gitaly v16.0. As we've landed a bunch of
previously announced removals it's thus time to bump our Go module
version from v15 to v16.
|
|
google.golang.org/grpc/test/grpc_testing was deleted in gRPC v1.55, see https://github.com/grpc/grpc-go/pull/6164.
|
|
go-grpc-middleware have deprecated their middleware chaining methods in
favor of the grpc libraries builtin chaining.
|
|
We encourage wrapping error with %w when constructing a new error. The
new error contains the original error so that it is able to be unwrapped
later. This commit converts all error wrapping to %w.
|
|
In the prior commit, some new utility methods were implemented to wrap
around tracing libraries. This commit replaces the existing calls to
opentracing library by new methods. Some some advanced usage still holds
references to opentracing, though.
|
|
Previously, we implemented a custom DNS resolver. This resolver handles
all Gitaly URL having dns scheme. The exposed dial functions in client
package should work well with this scheme. In fact, we added some tests
to verify. However, it turns out the tests use `grpc.Dial`, instead of
exposed aforementioned dial functions. Hence, after rolling the
resolver, the client could not use URLs with DNS scheme.
This commit fixes this problem. In addition, the test is strengthen to
cover such cases.
Changelog: fixed
|
|
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.
|
