-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
secret create, config create: refactor, use limit reader, and touch up errors #5912
Open
thaJeztah
wants to merge
2
commits into
docker:master
Choose a base branch
from
thaJeztah:refactor_secret_config_create
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+105
−40
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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]: moby/moby@c6f0b7f [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/[email protected]/api/validation#MaxSecretSize Signed-off-by: Sebastiaan van Stijn <[email protected]>
Swarm has size constraints on the size of configs, 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 config from a file 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: - Factors out the reading code to a readConfigData, analogous to the equivalent in secret create. - Implements reading the data with a limit-reader to prevent reading large files into memory. - The limit is based on SwarmKits limits ([MaxConfigSize]), 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 config create nosuchfile ./nosuchfile Error reading content from "./nosuchfile": open ./nosuchfile: no such file or directory docker config create toolarge ./largefile Error response from daemon: rpc error: code = ResourceExhausted desc = grpc: received message larger than max (8462870 vs. 4194304) docker config create empty ./emptyfile Error response from daemon: rpc error: code = InvalidArgument desc = config data must be larger than 0 and less than 1024000 bytes cat ./largefile | docker config create toolarge - Error response from daemon: rpc error: code = ResourceExhausted desc = grpc: received message larger than max (8462870 vs. 4194304) cat ./emptyfile | docker config create empty - Error response from daemon: rpc error: code = InvalidArgument desc = config data must be larger than 0 and less than 1024000 bytes With this patch: docker config create nosuchfile ./nosuchfile error reading from ./nosuchfile: open ./nosuchfile: no such file or directory docker config create empty ./emptyfile error reading from ./emptyfile: data is empty docker config create toolarge ./largefile Error response from daemon: rpc error: code = InvalidArgument desc = config data must be larger than 0 and less than 1024000 bytes cat ./largefile | docker config create toolarge - Error response from daemon: rpc error: code = InvalidArgument desc = secret data must be larger than 0 and less than 1024000 bytes cat ./emptyfile | docker config create empty - error reading from STDIN: data is empty [FILE_FLAG_SEQUENTIAL_SCAN]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN [MaxConfigSize]: https://pkg.go.dev/github.com/moby/swarmkit/[email protected]/manager/controlapi#MaxConfigSize Signed-off-by: Sebastiaan van Stijn <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5912 +/- ##
==========================================
+ Coverage 59.26% 59.28% +0.01%
==========================================
Files 357 357
Lines 29771 29806 +35
==========================================
+ Hits 17645 17671 +26
- Misses 11153 11158 +5
- Partials 973 977 +4 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
secret create: refactor, use limit reader, and touch up errors
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:
set, in stead only calling it when not using a driver.
large files into memory.
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.
filename (when used).
Before this patch:
With this patch:
config create: refactor, use limit reader, and touch up errors
Swarm has size constraints on the size of configs, 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 config from a file 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:
equivalent in secret create.
large files into memory.
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.
filename (when used).
Before this patch:
With this patch:
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)