Swarm has size constraints on the size of secrets, but the client-side would
read content into memory, regardless its size. This could lead to either the
client reading too much into memory, or it sending data that's larger than
the size limit of gRPC, which resulted in the error not being handled by
SwarmKit and a generic gRPC error returned.
Reading a secret from a file was added in [moby@c6f0b7f], which used a
system.OpenSequential for reading ([FILE_FLAG_SEQUENTIAL_SCAN]). While
there could be a very marginal benefit to prevent polluting the system's
cache (Windows won’t aggressively keep it in the cache, freeing up system
memory for other tasks). These details were not documented in code, and
possibly may be too marginal, but adding a comment to outline won't hurt
so this patch also adds a comment.
This patch:
- Rewrites readSecretData to not return a nil-error if no file was
set, in stead only calling it when not using a driver.
- Implements reading the data with a limit-reader to prevent reading
large files into memory.
- The limit is based on SwarmKits limits ([MaxSecretSize]), but made
twice that size, just in case larger sizes are supported in future;
the main goal is to have some constraints, and to prevent hitting
the gRPC limit.
- Updates some error messages to include STDIN (when used), or the
filename (when used).
Before this patch:
ls -lh largefile
-rw------- 1 thajeztah staff 8.1M Mar 9 00:19 largefile
docker secret create nosuchfile ./nosuchfile
Error reading content from "./nosuchfile": open ./nosuchfile: no such file or directory
docker secret create toolarge ./largefile
Error response from daemon: rpc error: code = ResourceExhausted desc = grpc: received message larger than max (8462870 vs. 4194304)
docker secret create empty ./emptyfile
Error response from daemon: rpc error: code = InvalidArgument desc = secret data must be larger than 0 and less than 512000 bytes
cat ./largefile | docker secret create toolarge -
Error response from daemon: rpc error: code = ResourceExhausted desc = grpc: received message larger than max (8462870 vs. 4194304)
cat ./emptyfile | docker secret create empty -
Error response from daemon: rpc error: code = InvalidArgument desc = secret data must be larger than 0 and less than 512000 bytes
With this patch:
docker secret create nosuchfile ./nosuchfile
error reading from ./nosuchfile: open ./nosuchfile: no such file or directory
docker secret create empty ./emptyfile
error reading from ./emptyfile: data is empty
docker secret create toolarge ./largefile
Error response from daemon: rpc error: code = InvalidArgument desc = secret data must be larger than 0 and less than 512000 bytes
cat ./largefile | docker secret create toolarge -
Error response from daemon: rpc error: code = InvalidArgument desc = secret data must be larger than 0 and less than 512000 bytes
cat ./emptyfile | docker secret create empty -
error reading from STDIN: data is empty
[moby@c6f0b7f]: c6f0b7f448
[FILE_FLAG_SEQUENTIAL_SCAN]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN
[MaxSecretSize]: https://pkg.go.dev/github.com/moby/swarmkit/v2@v2.0.0-20250103191802-8c1959736554/api/validation#MaxSecretSize
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
While there may be reasons to keep pkg/errors in production code,
we don't need them for these tests.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This makes a quick pass through our tests;
Discard output/err
----------------------------------------------
Many tests were testing for error-conditions, but didn't discard output.
This produced a lot of noise when running the tests, and made it hard
to discover if there were actual failures, or if the output was expected.
For example:
=== RUN TestConfigCreateErrors
Error: "create" requires exactly 2 arguments.
See 'create --help'.
Usage: create [OPTIONS] CONFIG file|- [flags]
Create a config from a file or STDIN
Error: "create" requires exactly 2 arguments.
See 'create --help'.
Usage: create [OPTIONS] CONFIG file|- [flags]
Create a config from a file or STDIN
Error: error creating config
--- PASS: TestConfigCreateErrors (0.00s)
And after discarding output:
=== RUN TestConfigCreateErrors
--- PASS: TestConfigCreateErrors (0.00s)
Use sub-tests where possible
----------------------------------------------
Some tests were already set-up to use test-tables, and even had a usable
name (or in some cases "error" to check for). Change them to actual sub-
tests. Same test as above, but now with sub-tests and output discarded:
=== RUN TestConfigCreateErrors
=== RUN TestConfigCreateErrors/requires_exactly_2_arguments
=== RUN TestConfigCreateErrors/requires_exactly_2_arguments#01
=== RUN TestConfigCreateErrors/error_creating_config
--- PASS: TestConfigCreateErrors (0.00s)
--- PASS: TestConfigCreateErrors/requires_exactly_2_arguments (0.00s)
--- PASS: TestConfigCreateErrors/requires_exactly_2_arguments#01 (0.00s)
--- PASS: TestConfigCreateErrors/error_creating_config (0.00s)
PASS
It's not perfect in all cases (in the above, there's duplicate "expected"
errors, but Go conveniently adds "#01" for the duplicate). There's probably
also various tests I missed that could still use the same changes applied;
we can improve these in follow-ups.
Set cmd.Args to prevent test-failures
----------------------------------------------
When running tests from my IDE, it compiles the tests before running,
then executes the compiled binary to run the tests. Cobra doesn't like
that, because in that situation `os.Args` is taken as argument for the
command that's executed. The command that's tested now sees the test-
flags as arguments (`-test.v -test.run ..`), which causes various tests
to fail ("Command XYZ does not accept arguments").
# compile the tests:
go test -c -o foo.test
# execute the test:
./foo.test -test.v -test.run TestFoo
=== RUN TestFoo
Error: "foo" accepts no arguments.
The Cobra maintainers ran into the same situation, and for their own
use have added a special case to ignore `os.Args` in these cases;
https://github.com/spf13/cobra/blob/v1.8.1/command.go#L1078-L1083
args := c.args
// Workaround FAIL with "go test -v" or "cobra.test -test.v", see #155
if c.args == nil && filepath.Base(os.Args[0]) != "cobra.test" {
args = os.Args[1:]
}
Unfortunately, that exception is too specific (only checks for `cobra.test`),
so doesn't automatically fix the issue for other test-binaries. They did
provide a `cmd.SetArgs()` utility for this purpose
https://github.com/spf13/cobra/blob/v1.8.1/command.go#L276-L280
// SetArgs sets arguments for the command. It is set to os.Args[1:] by default, if desired, can be overridden
// particularly useful when testing.
func (c *Command) SetArgs(a []string) {
c.args = a
}
And the fix is to explicitly set the command's args to an empty slice to
prevent Cobra from falling back to using `os.Args[1:]` as arguments.
cmd := newSomeThingCommand()
cmd.SetArgs([]string{})
Some tests already take this issue into account, and I updated some tests
for this, but there's likely many other ones that can use the same treatment.
Perhaps the Cobra maintainers would accept a contribution to make their
condition less specific and to look for binaries ending with a `.test`
suffix (which is what compiled binaries usually are named as).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
I could either remove the name for these contexts, or make the fake functions
more accurately reflect the actual implementation (decided to go for the latter
one)
cli/command/secret/client_test.go:19:35: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func (c *fakeClient) SecretCreate(ctx context.Context, spec swarm.SecretSpec) (types.SecretCreateResponse, error) {
^
cli/command/secret/client_test.go:26:43: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func (c *fakeClient) SecretInspectWithRaw(ctx context.Context, id string) (swarm.Secret, []byte, error) {
^
cli/command/secret/client_test.go:33:33: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func (c *fakeClient) SecretList(ctx context.Context, options types.SecretListOptions) ([]swarm.Secret, error) {
^
cli/command/secret/client_test.go:40:35: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func (c *fakeClient) SecretRemove(ctx context.Context, name string) error {
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The validation functions to test for the number of passed arguments did not
pluralize `argument(s)`, and used `argument(s)` in all cases.
This patch adds a simple `pluralize()` helper to improve this.
Before this change, `argument(s)` was used in all cases:
$ docker container ls foobar
"docker container ls" accepts no argument(s).
$ docker network create one two
"docker network create" requires exactly 1 argument(s).
$ docker network connect
"docker network connect" requires exactly 2 argument(s).
$ docker volume create one two
"docker volume create" requires at most 1 argument(s).
After this change, `argument(s)` is properly singularized or plurarized:
$ docker container ls foobar
"docker container ls" accepts no arguments.
$ docker network create one two
"docker network create" requires exactly 1 argument.
$ docker network connect
"docker network connect" requires exactly 2 arguments.
$ docker volume create one two
"docker volume create" requires at most 1 argument.
Test cases were updated accordingly.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
