This avoids having to determine what the default is in various
parts of the code. If no custom timeout is passed (nil), the
default will be used.
Also remove the named return variable from cleanupContainer(),
as it wasn't used.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
We already have this config, so might as well pass it, instead of passing
each option as a separate argument.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- daemon/delete: rename var that collided with import, remove output var
- daemon: fix inconsistent receiver name and package aliases
- daemon/stop: rename imports and variables to standard naming
This is in preparation of some changes, but keeping it in a
separate commit to make review of other changes easier.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
While looking at this code, I noticed that we were wasting quite some resources
by first constructing a string, only to split it again (with `strings.Fields()`)
into a string slice.
Some conversions were also happening multiple times (int to string, IP-address to
string, etc.)
Setting up networking is known to be costing a considerable amount of time when
starting containers, and while this may only be a small part of that, it doesn't
hurt to save some resources (and readability of the code isn't significantly
impacted).
For example, benchmarking the `redirector()` code before/after:
BenchmarkParseOld-4 137646 8398 ns/op 4192 B/op 75 allocs/op
BenchmarkParseNew-4 629395 1762 ns/op 2362 B/op 24 allocs/op
Average over 10 runs:
benchstat old.txt new.txt
name old time/op new time/op delta
Parse-4 8.43µs ± 2% 1.79µs ± 3% -78.76% (p=0.000 n=9+8)
name old alloc/op new alloc/op delta
Parse-4 4.19kB ± 0% 2.36kB ± 0% -43.65% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
Parse-4 75.0 ± 0% 24.0 ± 0% -68.00% (p=0.000 n=10+10)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Just a small clean-up (there's more endpoints to do this for, but
I was working on changes in this area on the CLI when I noticed we
were setting this query-parameter unconditionally.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
ContainerConfig is used in multiple locations (for example, both for
Image.Config and Image.ContainerConfig). Unfortunately, swagger does
not allow documenting individual uses if a type is used; for this type,
the content is _optional_ when used as Image.ContainerConfig (which is
set by the classic builder, which does a "commit" of a container, but
not used when building an image with BuildKit).
This patch attempts to address this confusion by documenting that
"it may be empty (or fields not propagated) if it's used for the
Image.ContainerConfig field".
Perhaps alternatives are possible (aliasing the type?) but we can
look at those in a follow-up.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Make the call to cleanupServiceBindings during network deletion
conditional on Windows (where it is required), thereby providing a
performance improvement to network cleanup on Linux.
Signed-off-by: Trapier Marshall <tmarshall@mirantis.com>
Removal of PolicyLists from Windows VFP must be performed prior to
removing the HNS network. Otherwise PolicyList removal fails with
HNS error "network not found".
Signed-off-by: Trapier Marshall <tmarshall@mirantis.com>
ContainerConfig is used in multiple locations (for example, both for
Image.Config and Image.ContainerConfig). Unfortunately, swagger does
not allow documenting individual uses if a type is used; for this type,
the content is _optional_ when used as Image.ContainerConfig (which is
set by the classic builder, which does a "commit" of a container, but
not used when building an image with BuildKit).
This patch attempts to address this confusion by documenting that
"it may be empty (or fields not propagated) if it's used for the
Image.ContainerConfig field".
Perhaps alternatives are possible (aliasing the type?) but we can
look at those in a follow-up.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This was added in 93c3e6c91e, at which time only
some basic handling of non-succesful status codes was present;
93c3e6c91e/api/client/utils.go (L112-L121)
Given that since 38e6d474af non-successful status-
codes are already handled, and a 204 ("no content") status should not be an error,
this special case should no longer be needed.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Having to declare a package-scope variable and separately initialize it
is repetitive and error-prone. Refactor so that each metric is defined
and initialized in the same statement.
Signed-off-by: Cory Snider <csnider@mirantis.com>
git published an advisory Yesterday, which (as a counter-measure)
requires the git repository's directory to be owned by the current
user, and otherwise produce an error:
fatal: unsafe repository ('/workspace' is owned by someone else)
To add an exception for this directory, call:
git config --global --add safe.directory /workspace
The DCO check is run within a container, which is running as `root`
(to allow packages to be installed), but because of this, the user
does not match the files that are bind-mounted from the host (as they
are checked out by Jenkins, using a different user).
To work around this issue, this patch configures git to consider the
`/workspace` directory as "safe". We configure it in the `--system`
configuration so that it takes effect for "all users" inside the
container.
More details on the advisory can be found on GitHub's blog:
https://github.blog/2022-04-12-git-security-vulnerability-announced/
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Simplify some of the logic, and add documentation about the package,
as well as warnings that this package should not be used as a general-
purpose utility.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
pkg/urlutil (despite its poorly chosen name) is not really intended as a generic
utility to handle URLs, and should only be used by the builder to handle (remote)
build contexts.
- IsURL() only does a very rudimentary check for http(s):// prefixes, without any
other validation, but due to its name may give incorrect expectations.
- IsGitURL() is written specifically with docker build remote git contexts in
mind, and has handling for backward-compatibility, where strings that are
not URLs, but start with "github.com/" are accepted.
Because of the above, this patch:
- moves the package inside builder/remotecontext, close to where it's intended
to be used (ideally this would be part of build/remotecontext itself, but this
package imports many other dependencies, which would introduce those as extra
dependencies in the CLI).
- deprecates pkg/urlutil, but adds aliases as there are some external consumers.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Implement similar logic as is used in httputils.ReadJSON(). Before
this patch, endpoints using the ContainerDecoder would incorrectly
return a 500 (internal server error) status.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Implement a ReadJSON() utility to help reduce some code-duplication,
and to make sure we handle JSON requests consistently (e.g. always
check for the content-type).
Differences compared to current handling:
- prevent possible panic if request.Body is nil ("should never happen")
- always require Content-Type to be "application/json"
- be stricter about additional content after JSON (previously ignored)
- but, allow the body to be empty (an empty body is not invalid);
update TestContainerInvalidJSON accordingly, which was testing the
wrong expectation.
- close body after reading (some code did this)
We should consider to add a "max body size" on this function, similar to
7b9275c0da/api/server/middleware/debug.go (L27-L40)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
pkg/urlutil (despite its poorly chosen name) is not really intended as a generic
utility to handle URLs, and should only be used by the builder to handle (remote)
build contexts.
This patch:
- fix some cases where the host was ignored for valid addresses.
- removes a redundant use of urlutil.IsTransportURL(); instead adding code to
check if the given scheme (protocol) is supported.
- improve port validation for out of range ports.
- fix some missing validation: the driver was silently ignoring path elements,
but expected a host (not an URL)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
fix some missing validation: the driver was silently ignoring path elements
in some cases, and expecting a host (not an URL), and for unix sockets did
not validate if a path was specified.
For the latter case, we should have a fix in the upstream driver, as it
uses an empty path as default path for the socket (`defaultSocketPath`),
and performs no validation.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
pkg/urlutil (despite its poorly chosen name) is not really intended as a generic
utility to handle URLs, and should only be used by the builder to handle (remote)
build contexts.
This patch:
- removes a redundant use of urlutil.IsTransportURL(); instead adding some code
to check if the given scheme (protocol) is supported.
- define a `defaultPort` const for the default port.
- use `net.JoinHostPort()` instead of string concatenating, to account for possible
issues with IPv6 addresses.
- renames a variable that collided with an imported package.
- improves test coverage, and moves an integration test.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
