Some of the prepared statement identifies used in these tests exceeded
the identifier character limit. This results in postgres truncating the
name, which is fine for a test, but does result in some noise in the
test output. This change shortens the identifies to eliminate noise like
the following:
psql:/test/credential/vault/credential_vault_library_ssh_private_key_mapping_override.sql:49: NOTICE: identifier "insert_credential_vault_library_ssh_private_key_mapping_override" will be truncated to "insert_credential_vault_library_ssh_private_key_mapping_overrid"
The tests should no longer create notices like these.
After updating `boundary dev` to use postgres:12 by default, it was
noticed that the responsiveness of boundary was noticeably slower. This
is due to the introduction of JIT being enabled by default in
postgres:12. JIT seems to incur noticeable overhead without providing
benefits, at least for the instance used for `boundary dev`.
It is also worth noting that for postgres:14 and newer, JIT does not get
triggered for the same queries since they seem to result in a cheaper
query plan that does not exceed the default threshold for JIT.
See:
https://www.postgresql.org/docs/current/jit.html
ICU-12283
This adds a Access-Control-Expose-Headers response header to an Options
request to allow the rate limiting related headers to be accessible via
requests that are subject to CORS.
Normally the tests are run on a host machine with a docker container for
the database. The docker container should be accessible via 127.0.0.1.
However there might be cases where the test execution is run in a docker
container, or the postgres server is configured to use a different
docker network, in which case the container would be accessible via some
other dns name or ip. In these cases, users can now set the TEST_DB_HOST
environment variable so the tests can connect to the postgres server.
This increases the default rate limits to:
- 1,500 reqs / 30 seconds for list actions in total
- 1,500 reqs / 30 seconds for list actions per IP address
- 150 reqs / 30 seconds for list actions per auth token
And for all other actions:
- 30,000 reqs / 30 seconds for list actions in total
- 30,000 reqs / 30 seconds for list actions per IP address
- 3,000 reqs / 30 seconds for list actions per auth token
This seems to be a reasonable default for a small to medium size cluster
based on some scale testing.
(cherry picked from commit 237c8b6084f42655edaba49055e511163fdf1c53)
Go 1.21.5 fixes a few vulnerabilities. Most notably for boundary is a
fix in net/http to limit chunked data overhead.
See: https://groups.google.com/g/golang-announce/c/iLGK3x6yuNo
(cherry picked from commit 575cd9d63b1ac4787d048d86b0b209c61a7f3946)
Add support for rate limiting
This introduces rate limiting for the Controller API endpoints. Limits
can be set separately for each combination of resource and action. And
each combination has a limit per auth-token, IP address, and in total.
By default each list action for all resources has a limit of 120
requests per minute. All other actions for all resources have a limit of
3,000 requests per minute. This can be overridden by including
`api_rate_limit` stanzas in the controller configuration. For example to
set a limit on target list requests per auth token:
controller {
api_rate_limit {
resources = ["target"]
actions = ["list"]
per = "auth-token"
limit = 10
period = "1s"
}
// other controller configuration
}
Or to completely override all of the defaults:
controller {
// total limit for all resources and actions
api_rate_limit {
resources = ["*"]
actions = ["*"]
per = "total"
limit = 500
period = "1s"
}
// limit for ip addresses to all resources and actions
api_rate_limit {
resources = ["*"]
actions = ["*"]
per = "ip-address"
limit = 100
period = "1s"
}
// Limit for auth tokens to all resources and actions
api_rate_limit {
resources = ["*"]
actions = ["*"]
per = "auth-token"
limit = 50
period = "1s"
}
// other controller configuration
}
All rate limiting can be disabled by setting the
`api_rate_limit_disable` configuration variable:
controller {
api_rate_limit_disable = true
// other controller configuration
}
When rate limiting is enabled, boundary will create quotas to track the
number of requests used within a time period. Boundary will reserve a
set amount of memory for tracking quotas. This ensures that boundary
does not consume too much memory if there is a sudden burst of requests
from many different clients. However this does mean if boundary is
unable to store a quota, it will limit the request with a 503 HTTP
status code. The amount of memory can be controlled with the
`api_rate_limit_max_quotas` configuration variable. This can also be
monitored via two new metrics:
- `boundary_controller_api_ratelimiter_quota_storage_capacity`
- `boundary_controller_api_ratelimiter_quota_storage_usage`
Clients making requests to the controller API can inspect HTTP response
headers to understand the configured limits and current usage. Each
response will contain the following headers:
- RateLimit: Provides the current limit, number of remaining requests,
and the time the quota will reset for the limit that is closest to
being exhausted for the requested resource and action.
- RateLimit-Policy: Describes the limits for the requested resource and
action.
If a request is limited, the client will receive a 429 HTTP status code,
and the Retry-After header will be set to the number of seconds the
client should wait before attempting the request again.
Previously this would truncate to the nearest integer, which could
result in clients retrying before the quota has reset, and getting rate
limited again. This also includes an update from go-rate that will round
up when reporting the reset time for the RateLimit header.
Note that in the case of a 503, the Retry-After header was already being
set correctly.
See: https://github.com/hashicorp/go-rate/pull/8
If the api.Client receives a 429 or 503 along with a Retry-After header,
the client will automatically wait the amount of time specified, and
then re-try the request.
A sys event will be emitted when the controller is constructed, and if
the rate limits are changed via a SIGHUP. The event contains the details
of the rate limits for each combination of resource and action. This
event can be used by operators to confirm the limits are set as they
would expect based on the controller config.
This provides two prometheus metrics to monitor the rate limiter:
boundary_controller_api_ratelimiter_quota_storage_capacity:
This is a gague that shows the total number of quotas that can be stored
at a given moment by the rate limiter.
boundary_controller_api_ratelimiter_quota_storage_usage:
This is a gague that shows the number of quotas currently being stored
by the limiter. If this becomes equal to the capacity, requests that
would result in a new quota being created will get limited and receive a
429 status code.
This registers the ActionSets for each resource when the handler
packages are initialized. This then allows other packages to retrieve
the ActionSets for a given resource from the action package.
The initial consumer of this will be the ratelimit package so that it
can programmatically enumerate all of the valid combinations of a
resource and action.
This leaves a number of TODO to refactor the handler packages to also
retrieve the action sets from the registered actions instead of using a
package variable. This is left as a TODO since there is some
inconsistencies in how these package variables are used between the
handlers. For example a number of packages use a map of subtypes to
ActionSets which is then used withing the handler implementation. Others
use a map, but don't seem to use it at all. And others just use an
ActionSet.
This switches the implementation of an ActionSet from a slice to a map
to ensure the elements in an ActionSet are unique. This allows for other
packages to perform set operations such as a union or difference without
needing to account for duplicates.
The nhooyr/websocket library now has a release with the new default
behavior that removes the message size limit. This allows us to switch
off of the fork and back to the upstream.
In the same release that includes the fix we need for the boundary
client, the wspb helper package was removed from websocket. This package
contained some functions to read and write protobuf messages. However,
it was using the deprecated github.com/golang/protobuf/proto library. To
avoid this dependancy in the library they opted to just remove wspb.
Thus this provides a replacement implementation that uses
google.golang.org/protobuf/proto. Notably the newer proto library seems
to lack an equivalent of proto.NewBuffer so writes to the websocket and
not able to leverage a sync.Pool of byte.Buffers to reduce allocations.
Refs: 88bb253554
See: https://github.com/nhooyr/websocket/commit/ac385120c6e34fa6584f3856d5db949a21bbb65e
When generating the JSON for submitting the github comment, if the
message was too large, the workflow would fail with the following error:
/usr/bin/jq: Argument list too long
This fix switches to using the `--rawfile`, which allows jq to process
the large comment.
When the boundary client establishes a connection to the websocket used
to communicate with a worker, it was configured with a max websocket
message size of 32KiB. This was the default behavior for v1.8.7 of
nhooyr/websocket. However, there are cases where the worker can send
messages to the client that are larger than 32KiB. When this happens the
client returns an error and terminates the connection.
Notably this seems to only currently impact sessions made to SSH
Targets, although it could theoretically impact any target type
depending on the details of how the worker performs the writing of the
messages. The issue can be reproduced by attempting to scp a large
enough file from an SSH Target to the client. In this case the
connection gets terminated and results in a partial transfer of the
file.
This message size limit appears to be unnecessary, and the default
behavior is changing in v1.8.8 of nhooyr/websocket to make the message
size unlimited. Since this version is not yet released, this commit
switches the dependency to a fork with a version that is v1.8.7 plus the
patch to disable the limit. Once v1.8.8 is release this dependency can
be set back.
See:
https://github.com/nhooyr/websocket/pull/254https://github.com/nhooyr/websocket/pull/256
This updates the version make target to use the version/VERSION file in
the same manner as the build system. This ensures that the VERSION file
is the single source of truth for the version number.
This moves the contents of the readme used for hashicorp/boundary on
docker.io/registry.hub.docker.com to a separate readme file from the one that
documents how to build docker images. It also makes some minor fixups
and updates the docker.io readme, must notably updating the minimum
version of postgres that boundary requires.
This provides a new storage.FS implementation that expands the on
fstest.MemFS by allowing tests to mark the filesystem as out of space.
When this happens any write/create operation will start returning an
error indicating that the filesystem is out of space.
fix(session): List performance with large number of session connections
This includes the backported migration and a migration to ensure the next
release has the correct view.
As part of the performance improvements for listing sessions for v0.9.0,
the view used to retrieve sessions for listing was updated to no longer
include a join on session_connection. However, a later migration that
was adding additional columns to this view inadvertently re-added the
join and session_connection columns. This resulted in requests to list
sessions performing this join, and returning additional rows for each
connection in a session. The application would ignore this additional
data, since it is no longer returned via the API. This would be
particularly impactful in cases where there are a large number of
connections per session, as this would greatly increase the amount of
data returned by the database.
This fix removes the session_connection join from this view, which
should restore the original performance improvements even in cases where
there is a large number of session connections per session.
See: https://github.com/hashicorp/boundary/pull/2160
Ref: 32070678dc
Fixes: 1e3c941be1
(cherry picked from commit 7aef341682e65b42c1b919e03936bdf81e26e87c)
As part of the performance improvements for listing sessions for v0.9.0,
the view used to retrieve sessions for listing was updated to no longer
include a join on session_connection. However, a later migration that
was adding additional columns to this view inadvertently re-added the
join and session_connection columns. This resulted in requests to list
sessions performing this join, and returning additional rows for each
connection in a session. The application would ignore this additional
data, since it is no longer returned via the API. This would be
particularly impactful in cases where there are a large number of
connections per session, as this would greatly increase the amount of
data returned by the database.
This fix removes the session_connection join from this view, which
should restore the original performance improvements even in cases where
there is a large number of session connections per session.
See: https://github.com/hashicorp/boundary/pull/2160
Ref: 32070678dc
Fixes: 1e3c941be1
This removes the minimum on width and height, instead it will just used
the largest width, height seen as the initial size for the asciicast.
This also provides the ability to still set a minimum via options to
convert.ToAsciicast if the caller has a known minimum size. Also note
that in cases where there is no size information, ie if there was no pty
request, this will still pick a default size of 80x24.
For requests to the target authorize session endpoint we support passing
a target name instead of an id. This means that `req.GetId()` might not
be the target's public id. Since the first thing authorize session does
includes retrieving the target as part of the authorization checks, this
subtype check can leverage the public id of the target instead of using
the request.
Fixes: 6178dd516e
When an SSH channel is used for a file transfer, the summary will report
the detected direction of the file transfer as either an upload or
download, relative to the end client.
The session metadata is provided all at once when a new bsr is opened.
While the session recording metadata records metadata about a recording
while the recording is happening, notably it records each time a new
connection occurs.
The nested structure of the session metadata does not lend itself to the
append-only syntax of the recording meta file. In addition we do not
want to mix different kinds of metadata together. Thus the session
metadata is now written to a separate file in the BSR, and is written as
JSON which better handles the nested structure and allows for simpler
encoding/decoding.
Similarly the summary files are intended to be written to as containers
close and have been switched to JSON.
This was originally introduced to ensure that the meta files only
included ascii characters. However, this makes reading the protocol from
meta file less reliable since it does not exactly match the protocol
string. Instead protocol strings will be recorded in the meta files as
is. Generally protocol strings should include readable characters.
This also updates the ssh.Protocol to be 4 readable characters.
This adds another test storage.FS implemenation. This can be used to
write/read containers and files from a local filesystem via calls to the
os pacakge.
This changes the ToAscinema function to take a storage.TempFile instead
of a storage.FS. It was noticed that when this was being used a
storage.FS was being created just prior to calling this just so that it
could then create a temporary file. It seemed to make more sense to just
pass the temporary file in, instead of needing to expand the storage.FS
interface.
This also renames this method to align with the file format that the
returned io.Reader will contain.
This provides a new type for a temporary file and a method on a
RecordingStorage to create temporary files. Temporary files will get
removed when closed.
This updates the fstest implementation of a storage.FS to support
opening sub containers and files in read-only mode.
Now when calling `SubContainer`:
- If using `storage.WithCreateFile`, an error is returned if the
container already exists.
- If not using `storage.WithCreateFile` and error is returned if the
container does *not* exist.
When calling `OpenFile`:
- on a container that was created with `storage.ReadOnly`, an error is
returned if attempting to open the file wth `storage.ReadWrite`.
- with `storage.WithCreateFile`, an existing file is replaced with a new
empty file, (ie "truncated").
- without `storage.WithCreateFile`, an error is returned if the file
does not exist.
The scanner can be used to read any BSR data file. When creating a new
scanner, it will first verify the BSR magic string. Individual Chunks
can then be read back by calling Scan. The scanner will compute a
sha256sum while it is reading chunks. If the scanner was created using
the WithSha256Sum option, it will compare the computed checksum with the
one provided via the option once the scanner encounters an end chunk
(DONE) or an io.EOF error from the reader. If the checksums do not
match, and error will be returned from Scan. The current sha256sum can
be returned from the scanner using the Sum method.
There is also a convenience function, ChunkWalk that can be used to call
a function on each Chunk returned by a scanner.
This adds a function for decoding all of the SSH Chunk types and
registers the function with the chunk types so that the bsr.ChunkDecode
can be used to decode SSH Chunks.
A ChunkDecoder can be used to BSR Chunks. Each call to Decode will read
the next chunk from the decoder's io.Reader. It handles decompressing
any compressed chunk data, and verifying the CRCs of each chunk.
This provides a Sha256Sums type to store the information from a
SHA256SUM file in memory for easy lookup of the sums of files. It can be
used when checking the checksums of individual files without needing to
go back to disk.
This makes it possible to have external packages implement the
storage.FS, storage.Container, and storage.File interfaces and correctly
inspect the various options.
These are used to provide the metadata about a session or connection
when writing a BSR and will be written to the corresponding .meta files
within a BSR.
This allows for a greater delta between the period returned by vault and
they expected value to account for some delay due to load on the test
runners.
The original motivation for the matrix was to mitigate an issue some
tests failing that used a vault docker container. When the tests ran in
a single step, these tests would almost always fail. By splitting the
tests per package, the failure rate of these tests was around 20%.
GitHub was able to identify the cause. There was an issue with localhost
resolution that causes requests to the vault containers to sometimes
fail, resulting in the kinds of failures that were seen. Note that the
same issue was not seen when accessing the test postgres database that
was running in a container, since the test code connects via 127.0.0.1,
while the vault container used localhost. This commit includes the
recommended fix so that localhost name resolution works as expected on
the GitHub hosted runners.
It also removes the test matrix since it is no longer needed as a work
around. In addition it seems advisable to remove the matrix given the
recent discovery that some tests were not running with the matrix. While
this was fixed, removing the matrix reduces the risk of any future
mistakes from the test splitting.
This also seems to result in a faster pipeline execution time. The
overhead to determine the matrix values, wait for a runner to be
available, and restore from the cache seems to add up to a longer
overall execution time.
Eliminating the matrix can also help with another type of failure seen.
Sometimes restoring from the cache fails. By eliminating the test
matrix, it will greatly reduce the number of cache restoring done by the
workflow, which will reduce the chances of an issue restoring from
cache.
The only remaining benefit from the matrix, was that if there was a
transient test failure, a re-run of only the failed jobs would be
relatively quick (depending on which package had the failure), while
without the matrix the entire test suite will need to be re-run.
See: https://github.com/hashicorp/boundary/pull/3199
Refs:
86214a85e1e04438b7f8
This was missed on the previous fix. It now references the correct
artifact name now that this is back to running as part of the build
workflow.
Blame: 181f0f1806
It seems the workflow_run trigger only calls the workflow as it is on
the default branch (ie `main`). This would make it difficult to make
changes to this workflow and set of tests.
Blame: e76b3b9511
This will run the cli ui (bats) tests in a separate workflow from the
build workflow. This takes advantage of the newer workflow_run event to
trigger the tests after the build has completed, so that the artifacts
from the build step can still be used.
Timothy Messier
