Our test helper packages were not possible to use
with benchmarks. This refactor switches to testing.TB
to allow both tests and benchmarks to use the helpers.
* Reorder session authz checks
This implements a mechanism for service handlers to pass a repo
satisfying an interface into the scope IDs listing function. The purpose
of this interface is to allow the same function to perform authorization
checking against resources rather than requiring service handlers to
fetch all resources in the determined set of scopes and then perform
this checking. While this could be done in each service handler, this
approach centralizes the logic, which has the nice benefit of removing
some boilerplate from service handler list functions that have adapted
to it.
Tests haven't changed, which is expected -- the function should return
exactly what was being returned before, simply faster.
Previously we were matching what boundary considers an id, but that is determined after requests are routed by grpc-gateway.
This changes the regexp to match any path segment to try to behave the same way that grpc-gateway would when assigning values
to the {id} variable declared in the protobuf.
This updates the target resources to use a oneof for attributes. This
allows requests to the targets service to leverage the new
subtypes.AttributeTransformerInterceptor so that attributes are strongly
typed prior to the service handler.
The new `subtype.AttributeTransformerInterceptor` can return an error if
it fails to transform the attributes. This is mainly if invalid
attribute fields were included for a particular subtype. This error case
used to be checked in the grpc service handler functions. However, now
that it is happening in the interceptor, the `errorInterceptor` needs to
be moved up in the stack so that it will process the response of the new
transformation interceptor.
The `errorInterceptor` used to be after the `auditRequestInterceptor`
and prior to the `auditResponseInterceptor`, meaning that the
`auditResponseInterceptor` would have access to the result of the
`errorInterceptor`. But it seems the `auditRequestInterceptor` does not
use or need any of the headers set by the `errorInterceptor` so it
should be safe to move the `errorInterceptor`. This is important since
the `subtype.AttributeTransformerInterceptor` needs to be before the
audit interceptors in the stack to properly transform requests/responses
so they can be audited with the complete classification information.
This updates the credential store resources to use a oneof for
attributes. This allows requests to the credential store service to
leverage the new subtypes.AttributeTransformerInterceptor so that
attributes are strongly typed prior to the service handler.
This updates the credential library resources to use a oneof for
attributes. This allows requests to the credential library service to
leverage the new subtypes.AttributeTransformerInterceptor so that
attributes are strongly typed prior to the service handler.
This updates the credential store resources to use a oneof for
attributes. This allows requests to the credential store service to
leverage the new subtypes.AttributeTransformerInterceptor so that
attributes are strongly typed prior to the service handler.
It is not unreasonable for downstream users of the request and response
messages to mutate them, indeed we mutate the response
on the way out to get the desired external API. To avoid
inconsistent messages being audited, make a copy of the
entire message before returning from the interceptor.
This was discovered while testing the new subtype transformation
interceptor.
This interceptor is responsible for maintaining compatability with the
JSON API with the change to the protobuf messages. The messages are
changing from a single attribue field to a oneof to explicitly associate
subtype attribute messages with the resource message. The interceptor
transforms the request prior to the auditReqeust interceptor, and
transforms the response after the auditResponse interceptor. This allows
the audit interceptors to use all the necessary classification
information when creating the audit events.
Other parts of the system make use of a "type" field, while the
AuthenticateResponse sets a "token_type" on the response attributes
directly. Rename this field to "type", and move it up to the response
level, to be more consistent with other RPCs.
Also adds a new field to the AuthenticateRequest, "type" which will
replace the "token_type" field over time.
This reduces the chance that we'll hit a failure due to a mismatched
worker address, on the certificate, especially if a worker has multiple
addresses. We aren't relying on IP/DNS for security anyways, so let's
not have it be a hindrance.
The Go docs now say to use request.Clone (introduced in Go
1.13) instead of request.WithContext (introduced in Go 1.7) as it deep
copies instead of shallow copies.
* feat(worker): Instrumentation for Controller Cluster Communication
Adds request latency observations to the worker side (client) of
worker <-> controller cluster communications by integrating a new
grpc client interceptor.
* style(controller): make-gen-deltas changes
gRPC status.Handler gives us visibility into gRPC returned errors. In contrast, there are some situations where no metric would be counted at all using the interceptor approach (for example, in out of memory cases where gRPC would return a resource exhausted error).
When requesting a token, any error is fatal to the test.
The previous behavior would return a nil token, which
could cause subsequent uses to panic. This will instead
interrupt any test immediately on error.
The Scheduler is dependant on the server being inserted into the
database. Adding an upsert server call before starting scheduler
and tickers removes the race that caused log spam due to a missing
foreign key relationship.
It is documented as illegal to use t.FailNow in a goroutine.
Uses of require assertions call t.FailNow. This may have led to
inconsistent test results as some assertions were subject to
undefined behavior. Note that it is still allowed to call
assert assertions in goroutines.
See: https://pkg.go.dev/testing#T.FailNow
* Install buf with go install
This reduces the impact of using buf on our dependency
closure, which was significant. It also avoids potential
issues with building buf given our own internal dependency
constraints. This has caused issues (including crashes!)
in the past.
See https://github.com/cockroachdb/cockroach/issues/67216#issuecomment-874176186
for more discussion on using tool dependencies generally, and
https://github.com/bufbuild/buf/issues/52
for an example of where this sort of thing caused an issue for a user.
* Upgrade buf version
We should migrate to v1 sooner rather than later, as it has some
breaking changes from v0.
* Change buf lint/breaking commands
These commands were moved up one level for v1
* Migrate to buf.yaml to v1
* Remove unused imports
These were uncovered by using "buf lint".
* Use buf v1.3.1
* Depend on googleapis and grpc-gateway via BSR
* Switch to buf for proto generation
* Remove third party dependency
* Remove unused tools and scripts
* Move local protos up one level
* Temporarily don't fail on breaking changes
* Generate CI config
* Fix generation of controller.json
* Replace use of deprecated flag
* Remove unused import, regenerate files
* Remove temporary skipping over breaking check
This allows us to use SNI for actual host (e.g. routing) information. We
don't have the client cert yet so we can't look at that, so this
provides a way for us to convey the information needed to look up that
session's TLS stack.
Using SNI for hosts means we also run into the fact that we don't have
automatic agreement in terms of SANs. So when generating the certs we
now also pass worker address information to the function to be encoded
in the cert.
Finally, there is a change in how the websocket dialing happens, because
http.RoundTripper tries to be too clever for its own good and overwrites
NextProtos on a whim.
We never ended up using this and it makes it much harder to reason about the listeners.
An explanation: for the moment I separated out all of the listeners in the struct, even though one should suffice, because when I did it that way first I was hitting some really weird and hard to find intermittent behavior where it seemed like the listeners were getting crossed. So I re-did it this way with each listener very explicitly referenced. I'm happy to make a follow-up PR that attempts to combine them all into a single listener without hitting the issues I did, if desired, but at least doing it via a separate PR means that it won't be something with removing the ALPN muxer that is at fault.
This change introduces a `nonceFn` field that can be changed
by a test but at runtime is transparent to the code that calls it.
The current nonce-generating logic defaults to a test-only field which
opens us up to this value inadvertently being set.
With this change, the default behavior becomes using `base62` to
generate a random nonce, and any tests have to explicitly change that.
With this change, the Worker is now aware of its own listeners in a
more codified and structured way.
These changes have already been made in the controller on
8d48a8515c. They're now made on the
worker for similar reasons.
* feat(controller): New health service and server
This change also implements a new listener purpose, `ops`.
`ops` (short for operations) is a new listener purpose that is
responsible for serving Boundary's operations endpoints, like health
and possibly metrics.
At Startup: `ops` listeners get configured as late as possible to catch
any errors with the startup process;
At Shutdown: If the health endpoint has been set-up, we introduce a
configurable delay in the process. This is to enable the health endpoint
to report unhealthy for that amount of time. `ops` listeners are also
the last listeners to be shutdown.
In general, these changes were introduced in the same vein of the
changes to controller `startListeners`.
Specifically, these changes also decouple the Controller and the Worker
shutdown processes. Currently, a call to `stopListeners` on an instance
that is running as both a Controller and a Worker would stop the
listeners for both.
Lastly, we also add unit/integration tests to the shutdown code.
Previously, we initialized the listeners on the order they were provided
to us on the config file. In the future, we'll want to add new listener
purposes that depend on existing ones, so the init order needs to be
deterministic.
With this change, we'll initialize components in this order:
1. gRPC Server for Controller API.
2. All `api` HTTP Servers & gRPC Gateway Muxes.
3. `cluster` gRPC Server.
This commit introduces some renames for the sake of clarity as well as
improved test coverage:
- Removes `gatewayMux` reference from Controller object. This object was
misleading and is now uneeded.
- Removed uneeded `context.Context` arguments.
- Separates gRPC Server and gRPC Gateway concerns, both in functionality
and in naming. These two were conflated before.
- Added unit tests on `startListeners`.
With this change, the Controller is now aware of its own listeners in a
more codified and structured way.
The reasoning behind these changes are as follows:
1. Currently, we're iterating over these listeners in multiple spots
along this initialization flow to check each listeners' purpose. By
doing this once when we create a Controller and essentially storing the
result, we can clean up the downstream code to use those results.
2. Initial effort to centralize Controller validation. At the moment, we
are doing a lot of validation all over the `cmd` and `config` packages.
From a conceptual perspective, Controller-related validation should
be performed in the function that is responsible for constructing the
Controller object itself. This enables any developer to look in a
single place for all the constraints that we place on creating a
Controller.
Johan Brandhorst-Satzkorn
Jeff Mitchell
Todd
Timothy Messier
irenarindos
Jim
Hugo
Louis Ruch
Haotian