This was introduced in 1980deffae, which
changed the implementation, but forgot to update imports in these.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
There is no meaningful distinction between driverapi.Registerer and
drvregistry.DriverNotifyFunc. They are both used to register a network
driver with an interested party. They have the same function signature.
The only difference is that the latter could be satisfied by an
anonymous closure. However, in practice the only implementation of
drvregistry.DriverNotifyFunc is the
(*libnetwork.Controller).RegisterDriver method. This same method also
makes the libnetwork.Controller type satisfy the Registerer interface,
therefore the DriverNotifyFunc type is redundant. Change
drvregistry.Networks to notify a Registerer and drop the
DriverNotifyFunc type.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Add a fast-patch to some functions, to prevent locking/unlocking,
or other operations that would not be needed;
- Network.addDriverInfoToCluster
- Network.deleteDriverInfoFromCluster
- Network.addServiceInfoToCluster
- Network.deleteServiceInfoFromCluster
- Network.addDriverWatches
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- return early when failing to fetch the driver
- store network-ID and controller in a variable to prevent repeatedly
locking/unlocking. We don't expect the network's ID to change
during this operation.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This method is not part of any interface, and identical to Endpoint.Iface,
but one returns an Interface-type (driverapi.InterfaceInfo) and the other
returns a concrete type (EndpointInterface).
Interface-matching should generally happen on the receiver side, and this
function was only used in a single location, and passed as argument to
Driver.CreateEndpoint, which already matches the interface by accepting
a driverapi.InterfaceInfo.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Interface-matching should generally happen on the receiver side, and this
function was only used in a single location, and passed as argument to
Driver.CreateEndpoint, which already matches the interface by accepting
a driverapi.InterfaceInfo.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Interface-matching should generally happen on the receiver side, and this
function was only used in a single location, and passed as argument to
Driver.CreateEndpoint, which already matches the interface by accepting
a driverapi.InterfaceInfo.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
commit ab35df454d removed most of the pre-go1.17
build-tags, but for some reason, "go fix" doesn't remove these, so removing
the remaining ones manually
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Copying relevant documentation from the EndpointInfo interface. We should
remove this interface, and the related Info() function, but it's currently
acting as a "gate" to prevent accessing the Endpoint's accessors without
making sure it's fully hydrated.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
There's only one implementation; let's use that.
Also fixing a linting issue;
libnetwork/osl/interface_linux.go:91:2: S1001: should use copy(to, from) instead of a loop (gosimple)
for i, iface := range n.iFaces {
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
InterfaceOptions() returned an IfaceOptionSetter interface, which contained
"methods" that returned functional options. Such a construct could have made
sense if the functional options returned would (e.g.) be pre-propagated with
information from the Sandbox (network namespace), but none of that was the case.
There was only one implementation of IfaceOptionSetter (networkNamespace),
which happened to be the same as the only implementation of Sandbox, so remove
the interface as well, to help networkNamespace with its multi-personality
disorder.
This patch:
- removes Sandbox.Bridge() and makes it a regular function (WithIsBridge)
- removes Sandbox.Master() and makes it a regular function (WithMaster)
- removes Sandbox.MacAddress() and makes it a regular function (WithMACAddress)
- removes Sandbox.Address() and makes it a regular function (WithIPv4Address)
- removes Sandbox.AddressIPv6() and makes it a regular function (WithIPv6Address)
- removes Sandbox.LinkLocalAddresses() and makes it a regular function (WithLinkLocalAddresses)
- removes Sandbox.Routes() and makes it a regular function (WithRoutes)
- removes Sandbox.InterfaceOptions().
- removes the IfaceOptionSetter interface.
Note that the IfaceOption signature was changes as well to allow returning
an error. This is not currently used, but will be used for some options
in the near future, so adding that in preparation.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
NeighborOptions() returned an NeighborOptionSetter interface, which
contained "methods" that returned functional options. Such a construct
could have made sense if the functional options returned would (e.g.)
be pre-propagated with information from the Sandbox (network namespace),
but none of that was the case.
There was only one implementation of NeighborOptionSetter (networkNamespace),
which happened to be the same as the only implementation of Sandbox, so
remove the interface as well, to help networkNamespace with its multi-personality
disorder.
This patch:
- removes Sandbox.LinkName() and makes it a regular function (WithLinkName)
- removes Sandbox.Family() and makes it a regular function (WithFamily)
- removes Sandbox.NeighborOptions().
- removes the NeighborOptionSetter interface
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
osl.NewSandbox() always returns a nil interface on Windows (and other non-Linux
platforms). This means that any code that these fields are always nil, and
any code using these fields must be considered Linux-only.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
osl.NewSandbox() always returns a nil interface on Windows (and other non-Linux
platforms). This means that any code that these fields are always nil, and
any code using these fields must be considered Linux-only;
- libnetwork/Controller.defOsSbox
- libnetwork/Sandbox.osSbox
Ideally, these fields would live in Linux-only files, but they're referenced
in various platform-neutral parts of the code, so let's start with moving
the initialization code to Linux-only files.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Copying the descriptions from the Sandbox, Info, NeighborOptionSetter,
and IfaceOptionSetter interfaces that it implements.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Check if firewalld is running before running the function, so that consumers
of the function don't have to check for the status.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Windows uses the container-iD as ID for sandboxes, so it's not needed to
generate an ID when running on Windows.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Prior to moby/moby#44968, libnetwork would happily accept a ChildSubnet
with a bigger mask than its parent subnet. In such case, it was
producing IP addresses based on the parent subnet, and the child subnet
was not allocated from the address pool.
This commit automatically fixes invalid ChildSubnet for networks stored
in libnetwork's datastore.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
The Controller.Sandboxes method was used by some SandboxWalkers. Now
that those have been removed, there are no longer any consumers of this
method, so let's remove it for now.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This functionality has been replaced with Controller.GetSandbox, and is
no longer used anywhere.
This patch removes:
- the Controller.WalkSandboxes method
- the SandboxContainerWalker SandboxWalker
- the SandboxWalker type
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Various parts of the code were using "walkers" to iterate over the
controller's sandboxes, and the only condition for all of them was
to find the sandbox for a given container-ID. Iterating over all
sandboxes was also sub-optimal, because on Windows, the ContainerID
is used as Sandbox-ID, which can be used to lookup the sandbox from
the "sandboxes" map on the controller.
This patch implements a GetSandbox method on the controller that
looks up the sandbox for a given container-ID, using the most optimal
approach (depending on the platform).
The new method can return errors for invalid (empty) container-IDs, and
a "not found" error to allow consumers to detect non-existing sandboxes,
or potentially invalid IDs.
This new method replaces the (non-exported) Daemon.getNetworkSandbox(),
which was only used internally, in favor of directly accessing the
controller's method.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It was not exported so let's remove the abstraction to not make it look
like something more than it is.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
"Pay no attention to the implementation behind the curtain!"
There's only one implementation of the Sandbox interface, and only one implementation
of the Info interface, and they both happens to be implemented by the same type:
networkNamespace. Let's merge these interfaces.
And now that we know that there's one, and only one Info, we can drop the charade,
and relieve the Sandbox from its dual personality.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Slightly refactor Resolver.dialExtDNS:
- use net.JoinHostPort to properly format IPv6 addresses
- define a const for the default port, and avoid int -> string
conversion if no custom port is defined
- slightly simplify logic if the HostLoopback is used (at the cost of
duplicating one line); in that case we don't need to define the closure
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function was added in 36fd9d02be
(libnetwork: ce6c6e8c35),
because there were multiple places where a DNS response was created,
which had to use the same options. However, new "common" options were
added since, and having it in a function separate from the other (also
common) options was just hiding logic, so let's remove it.
What the above probably _should_ have done was to create a common utility
to create a DNS response (as all other options are shared as well). This
was actually done in 0c22e1bd07 (libnetwork:
be3531759b),
which added a `createRespMsg` utility, but missed that it could be used
for both cases.
This patch:
- removes the setCommonFlags function
- uses createRespMsg instead to share common options
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Removes the deprecated consts, which moved to a separate "scope" package
in commit 6ec03d6745, and are no longer used;
- datastore.LocalScope
- datastore.GlobalScope
- datastore.SwarmScope
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
UnavailableError is now compatible with errdefs.UnavailableError. These
errors will now return a 503 instead of a 500.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
InvalidParameter is now compatible with errdefs.InvalidParameter. Thus,
these errors will now return a 400 status code instead of a 500.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
- un-export ZoneSettings, because it's only used internally
- make conversion to a "interface" slice a method on the struct
- remove the getDockerZoneSettings() function, and move the type-definition
close to where it's used, as it was only used in a single location
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This test didn't make a lot of sense, because `checkRunning()` depends on
the `connection` package-var being set, which is done by `firewalldInit()`,
so would never be true on its own.
Add a small utility that opens its own D-Bus connection to verify if
firewalld is running, and otherwise skips the tests (preserving any
error in the process).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
DelInterfaceFirewalld returns an error if the interface to delete was
not found. Let's ignore cases where we were successfully able to get
the list of interfaces in the zone, but the interface was not part of
the zone.
This patch changes the error for these cases to an errdefs.ErrNotFound,
and updates IPTable.ProgramChain to ignore those errors.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It's used in various defers, but was using `err` as name, which can be
confusing, and increases the risk of accidentally shadowing the error.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It's used in various defers, but was using `err` as name, which can be
confusing, and increases the risk of accidentally shadowing the error.
This patch:
- introduces a `retErr` output variable, to be used in defer statements.
- explicitly changes some `err` uses to locally-scoped variables.
- moves some variable definitions closer to where they're used (where possible).
While working on this change, there was one point in the code where
error handling was ambiguous. I added a note for that, in case this
was not a bug:
> This code was previously assigning the error to the global "err"
> variable (before it was renamed to "retErr"), but in case of a
> "MaskableError" did not *return* the error:
> b325dcbff6/libnetwork/controller.go (L566-L573)
>
> Depending on code paths further down, that meant that this error
> was either overwritten by other errors (and thus not handled in
> defer statements) or handled (if no other code was overwriting it.
>
> I suspect this was a bug (but possible without effect), but it could
> have been intentional. This logic is confusing at least, and even
> more so combined with the handling in defer statements that check for
> both the "err" return AND "skipCfgEpCount":
> b325dcbff6/libnetwork/controller.go (L586-L602)
>
> To save future visitors some time to dig up history:
>
> - config-only networks were added in 25082206df
> - the special error-handling and "skipCfgEpcoung" was added in ddd22a8198
> - and updated in 87b082f365 to don't use string-matching
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
There were quite some places where the type collided with variables
named `agent`. Let's rename the type.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function has _four_ output variables of the same type, and several
defer statements that checked the error returned (but using the `err`
variable).
This patch names the return variables to make it clearer what's being
returned, and renames the error-return to `retErr` to make it clearer
where we're dealing with the returned error (and not any local err), to
prevent accidentally shadowing.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
There's nothing handling these results, and they're logged as debug-logs,
so we may as well remove the returned variables.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Both functions were generating debug logs if there was nothing to log.
The function already produces logs if things failed while deleting entries,
so these logs would only be printed if there was nothing to delete, so can
safely be discarded.
Before this change:
DEBU[2023-08-14T12:33:23.082052638Z] Revoking external connectivity on endpoint sweet_swirles (1519f9376a3abe7a1c981600c25e8df6bbd0a3bc3a074f1c2b3bcbad0438443b)
DEBU[2023-08-14T12:33:23.085782847Z] DeleteConntrackEntries purged ipv4:0, ipv6:0
DEBU[2023-08-14T12:33:23.085793847Z] DeleteConntrackEntriesByPort for udp ports purged ipv4:0, ipv6:0
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It's only called as part of the "libnetwork-setkey" re-exec, so un-exporting
it to make clear it's not for external use.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The basepath is only used on Linux, so no need to call it on other
platforms. SetBasePath was already stubbed out on other platforms,
but "osl" was still imported in various places where it was not actually
used, so trying to reduce imports to get a better picture of what parts
are used (and not used).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Some tests were implicitly skipped through the `getTestEnv()` utility,
which made it hard to discover they were not ran on Windows.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This makes it easier to spot if code is only used on Linux. Note that "all of"
the bridge driver is Linux-only.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The mutex is only used on reads, but there's nothing protecting writes,
and it looks like nothing is mutating fields after creation, so let's
remove this altogether.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
No context in the commit that added it, but PR discussion shows that
the API was mostly exploratory, and it was 8 Years go, so let's not
head in that direction :) b646784859
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Now that we removed the interface, there's no need to cast the Network
to a NetworkInfo interface, so we can remove uses of the `Info()` method.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These errors aren't used in our repo and seem unused by the OSS
community (this was checked with Sourcegraph).
- ErrIpamInternalError has never been used
- ErrInvalidRequest is unused since moby/libnetwork@c85356efa
- ErrPoolNotFound has never been used
- ErrOverlapPool has never been used
- ErrNoAvailablePool has never been used
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Check the preferredPool first, as other checks could be doing more
(such as locking, or validating / parsing). Also adding a note, as
it's unclear why we're ignoring invalid pools here.
The "invalid" conditions was added in [libnetwork#1095][1], which
moved code to reduce os-specific dependencies in the ipam package,
but also introduced a types.IsIPNetValid() function, which considers
"0.0.0.0/0" invalid, and added it to the condition to return early.
Unfortunately review does not mention this change, so there's no
context why. Possibly this was done to prevent errors further down
the line (when checking for overlaps), but returning an error here
instead would likely have avoided that as well, so we can only guess.
To make this code slightly more transparent, this patch also inlines
the "types.IsIPNetValid" function, as it's not used anywhere else,
and inlining it makes it more visible.
[1]: 5ca79d6b87 (diff-bdcd879439d041827d334846f9aba01de6e3683ed8fdd01e63917dae6df23846)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This code was only run if no preferred pool was specified, however,
since [libnetwork#1162][2], the function would already return early
if a preferred pools was set (and the overlap check to be skipped),
so this was now just dead code.
[2]: 9cc3385f44
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function intentionally holds a lock / lease on address-pools to
prevent trying the same pool repeatedly.
Let's try to make this logic slightly more transparent, and prevent
defining defers in a loop. Releasing all the pools in a singe defer
also allows us to get the network-name once, which prevents locking
and unlocking the network for each iteration.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Both functions have multiple output vars with generic types, which made
it hard to grasp what's what.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This makes it easier to consume, without first having to create an empty
PoolID.
Performance is the same:
BenchmarkPoolIDFromString-10 6100345 196.5 ns/op 112 B/op 3 allocs/op
BenchmarkPoolIDFromString-10 6252750 192.0 ns/op 112 B/op 3 allocs/op
Note that I opted not to change the return-type to a pointer, as that seems
to perform less;
BenchmarkPoolIDFromString-10 6252750 192.0 ns/op 112 B/op 3 allocs/op
BenchmarkPoolIDFromString-10 5288682 226.6 ns/op 192 B/op 4 allocs/op
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
As this function may be called repeatedly to convert to/from a string,
it may be worth optimizing it a bit. Adding a minimal Benchmark for
it as well.
Before/after:
BenchmarkPoolIDToString-10 2842830 424.3 ns/op 232 B/op 12 allocs/op
BenchmarkPoolIDToString-10 7176738 166.8 ns/op 112 B/op 7 allocs/op
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
network.requestPoolHelper and Allocator.RequestPool have many args and
output vars with generic types. Add names for them to make it easier to
grasp what's what.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The options are unused, other than for debug-logging, which made it look
as if they were actually consumed anywhere, but they aren't.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This makes it slightly more readable to see what's returned in each of
the code-paths. Also move validation of pool/subpool earlier in the
function.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Collect a list of all the links we successfully enabled (if any), and
use a single defer to disable them.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The iptables package has types defined for these actions; use them directly
instead of creating a string only to convert it to a known value.
As the linkContainers() function is only used internally, and with fixed
values, we can also remove the validation, and InvalidIPTablesCfgError
error, which is now unused.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Partially revert commit 94b880f.
The CheckDuplicate field has been introduced in commit 2ab94e1. At that
time, this check was done in the network router. It was then moved to
the daemon package in commit 3ca2982. However, commit 94b880f duplicated
the logic into the network router for no apparent reason. Finally,
commit ab18718 made sure a 409 would be returned instead of a 500.
As this logic is first done by the daemon, the error -> warning
conversion can't happen because CheckDuplicate has to be true for the
daemon package to return an error. If it's false, the daemon proceed
with the network creation, set the Warning field of its return value and
return no error.
Thus, the CheckDuplicate logic in the api is removed and
libnetwork.NetworkNameError now implements the ErrConflict interface.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
The MediaType was changed twice in;
- b3b7eb2723 ("application/vnd.docker.plugins.v1+json" -> "application/vnd.docker.plugins.v1.1+json")
- 54587d861d ("application/vnd.docker.plugins.v1.1+json" -> "application/vnd.docker.plugins.v1.2+json")
But the (integration) tests were still using the old version, so let's
use the VersionMimeType const that's defined, and use the updated version.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Outside of some tests, these options are the only code setting these fields,
so we can update them to set the value, instead of appending.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
IPv4AddrNoMatchError and IPv6AddrNoMatchError are currently implementing
BadRequestError. They are returned in two cases, and none are due to a
bad user request:
- When calling daemon's CreateNetwork route, if the bridge's IPv4
address or none of the bridge's IPv6 addresses match what's requested.
If that happens, there's a big issue somewhere in libnetwork or the
kernel.
- When restoring a network, for the same reason. In that case, the
on-disk state drifted from the interface state.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
This error can only be reached because of an error in our code, so it's
not a "bad user request". As it's never type asserted, no need to keep
it around.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
This error is only used in defensive checks whereas the precondition is
already checked by caller. If we reach it, we messed something else. So
it's definitely not a BadRequest. Also, it's not type asserted anywhere,
so just inline it.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
It was not used as a sentinel error, and didn't carry a specific type,
which made it a rather complex way to create an error.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This type was added moved to the types package as part of a refactor
in 778e2a72b3
but the introduction of the sandbox API changed the existing API to
weak types (not using a plain string);
9a47be244a
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- InvalidIPTablesCfgError: implement InternalError instead of
BadRequestError. This error is returned when an invalid iptables
action is passed as argument (ie. none of -A, -I, or -D).
- ErrInvalidDriverConfig: don't implement BadRequestError. This is
returned when libnetwork controller initialization pass bad driver
config -- there's no call from an HTTP route.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Follow-up to fca38bcd0a, which made the
Discover API optional for drivers to implement, but forgot to remove the
stubs from the Windows drivers, which didn't implement this API.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The "Capability" type defines DataScope and ConnectivityScope fields,
but their value was set from consts in the datastore package, which
required importing that package and its dependencies for the consts
only.
This patch:
- Moves the consts to a separate "scope" package
- Adds aliases for the consts in the datastore package.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Most drivers do not implement this, so detect if a driver implements
the discoverAPI, and remove the implementation from drivers that do
not support it.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
IPv6 ipt rules are exactly the same as IPv4 rules, although both
protocol don't use the same networking model. This has bad consequences,
for instance: 1. the current v6 rules disallow Neighbor
Solication/Advertisement ; 2. multicast addresses can't be used ; 3.
link-local addresses are blocked too.
To solve this, this commit changes the following rules:
```
-A DOCKER-ISOLATION-STAGE-1 ! -s fdf1:a844:380c:b247::/64 -o br-21502e5b2c6c -j DROP
-A DOCKER-ISOLATION-STAGE-1 ! -d fdf1:a844:380c:b247::/64 -i br-21502e5b2c6c -j DROP
```
into:
```
-A DOCKER-ISOLATION-STAGE-1 ! -s fdf1:a844:380c:b247::/64 ! -i br-21502e5b2c6c -o br-21502e5b2c6c -j DROP
-A DOCKER-ISOLATION-STAGE-1 ! -d fdf1:a844:380c:b247::/64 -i br-21502e5b2c6c ! -o br-21502e5b2c6c -j DROP
```
These rules only limit the traffic ingressing/egressing the bridge, but
not traffic between veth on the same bridge.
Note that, the Kernel takes care of dropping invalid IPv6 packets, eg.
loopback spoofing, thus these rules don't need to be more specific.
Solve #45460.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
- use gotest.tools assertions
- use consts and struct-literals where possible
- use assert.Check instead of t.Fatal() where possible
- fix some unhandled errors
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The rootChain variable that the Key function references is a
package-global slice. As the append() built-in may append to the slice's
backing array in place, it is theoretically possible for the temporary
slices in concurrent Key() calls to share the same backing array, which
would be a data race. Thankfully in my tests (on Go 1.20.6)
cap(rootChain) == len(rootChain)
held true, so in practice a new slice is always allocated and there is
no race. But that is a very brittle assumption to depend upon, which
could blow up in our faces at any time without warning. Rewrite the
implementation in a way which cannot lead to data races.
Signed-off-by: Cory Snider <csnider@mirantis.com>
It only had a single implementation, so let's remove the interface.
While changing, also renaming;
- datastore.DataStore -> datastore.Store
- datastore.NewDataStore -> datastore.New
- datastore.NewDataStoreFromConfig -> datastore.FromConfig
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These were only used internally, and ErrConntrackNotConfigurable was not used
as a sentinel error anywhere. Remove ErrConntrackNotConfigurable, and change
IsConntrackProgrammable to return an error instead.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
arrangeUserFilterRule uses the package-level [`ctrl` variable][1], which
holds a reference to a controller instance. This variable is set by
[`setupArrangeUserFilterRule()`][2], which is called when initialization
a controller ([`libnetwork.New`][3]).
In normal circumstances, there would only be one controller, created during
daemon startup, and the instance of the controller would be the same as
the controller that `NewNetwork` is called from, but there's no protection
for the `ctrl` variable, and various integration tests create their own
controller instance.
The global `ctrl` var was introduced in [54e7900fb89b1aeeb188d935f29cf05514fd419b][4],
with the assumption that [only one controller could ever exist][5].
This patch tries to reduce uses of the `ctrl` variable, and as we're calling
this code from inside a method on a specific controller, we inline the code
and use that specific controller instead.
[1]: 37b908aa62/libnetwork/firewall_linux.go (L12)
[2]: 37b908aa62/libnetwork/firewall_linux.go (L14-L17)
[3]: 37b908aa62/libnetwork/controller.go (L163)
[4]: 54e7900fb8
[5]: https://github.com/moby/libnetwork/pull/2471#discussion_r343457183
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function was added in libnetwork through 50964c9948
and, based on the name of the function and its signature, I think it
was meant to be a test. This patch refactors it to be one.
Changing it into a test made it slightly broken:
go test -v -run TestErrorInterfaces
=== RUN TestErrorInterfaces
errors_test.go:15: Failed to detect err network not found is of type BadRequestError. Got type: libnetwork.ErrNoSuchNetwork
errors_test.go:15: Failed to detect err endpoint not found is of type BadRequestError. Got type: libnetwork.ErrNoSuchEndpoint
errors_test.go:42: Failed to detect err unknown driver "" is of type ForbiddenError. Got type: libnetwork.NetworkTypeError
errors_test.go:42: Failed to detect err unknown network id is of type ForbiddenError. Got type: *libnetwork.UnknownNetworkError
errors_test.go:42: Failed to detect err unknown endpoint id is of type ForbiddenError. Got type: *libnetwork.UnknownEndpointError
--- FAIL: TestErrorInterfaces (0.00s)
FAIL
This was because some errors were tested twice, but for the wrong type
(`NetworkTypeError`, `UnknownNetworkError`, `UnknownEndpointError`).
Moving them to the right test left no test-cases for `types.ForbiddenError`,
so I added `ActiveContainerError` to not make that part of the code feel lonely.
Other failures were because some errors were changed from `types.BadRequestError`
to a `types.NotFoundError` error in commit ba012a703a,
so I moved those to the right part.
Before this patch:
go test -v -run TestErrorInterfaces
=== RUN TestErrorInterfaces
--- PASS: TestErrorInterfaces (0.00s)
PASS
ok github.com/docker/docker/libnetwork 0.013s
After this patch:
go test -v -run TestErrorInterfaces
=== RUN TestErrorInterfaces
--- PASS: TestErrorInterfaces (0.00s)
PASS
ok github.com/docker/docker/libnetwork 0.013s
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
commit ffd75c2e0c updated this function to
set up the DOCKER-USER chain for both iptables and ip6tables, however the
function would return early if a failure happened (instead of continuing
with the next iptables version).
This patch extracts setting up the chain to a separate function, and updates
arrangeUserFilterRule to log the failure as a warning, but continue with
the next iptables version.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These functions were mostly identical, except for iptables being enabled
by default (unless explicitly disabled by config).
Rewrite the function to a enabledIptablesVersions, which returns the list
of iptables-versions that are enabled for the controller. This prevents
having to acquire a lock twice, and simplifies arrangeUserFilterRule, which
can now just iterate over the enabled versions.
Also moving this function to a linux-only file, as other platforms don't have
the iptables types defined.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The driver-configurations are only set when creating a new controller,
using the `config.OptionDriverConfig()` option that can be passed to
`New()`, and used as "read-only" after that.
Taking away any other paths that set these options, the only type used
for per-driver options are a `map[string]interface{}`, so we can change
the type from `map[string]interface{}` to a `map[string]map[string]interface{}`,
(or its "modern" variant: `map[string]map[string]any`), so that it's
no longer needed to cast the type before use.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Don't fail early if we can still test more, and be slightly more strict
in what error we're looking for.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The test already creates instances for each ip-version, so let's
re-use them. While changing, also use assert.Check to not fail early.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
New() allows for driver-options to be passed using the config.OptionDriverConfig.
Update the test to not manually mutate the controller's configuration after
creating.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Not critical, but when used from ChainInfo, we had to construct an IPTable
based on the version of the ChainInfo, which then only used the version
we passed to get the right loopback.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Make some variables local to the if-branches to be slightly more iodiomatic,
and to make clear it's only used in that branch.
Move the bestEffortLock locking later in IPtable.raw(), because that function'
could return before the lock was even needed.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Now that all consumers of these functions are passing non-empty values,
let's validate that no empty strings for either chain or table are passed.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It's only used internally, and it was last used in commit:
0220b06cd6
But moved into the iptables package in this commit:
998f3ce22c
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This utility was not used for "Config", but for Networks and Endpoints.
Having this utility made it look like more than it was, and the related
test was effectively testing stdlib.
Abstracting the validation also was hiding that, while validation does
not allow "empty" names, it happily allows leading/trailing whitespace,
and does not remove that before creating networks or endpoints;
docker network create "bridge "
docker network create "bridge "
docker network create "bridge "
docker network create " bridge "
docker network create " bridge "
docker network create " bridge"
docker network ls --filter driver=bridge
NETWORK ID NAME DRIVER SCOPE
d4d53210f185 bridge bridge local
e9afba0d99de bridge bridge local
69fb7a7ba67c bridge bridge local
a452bf065403 bridge bridge local
49d96c59061d bridge bridge local
8eae1c4be12c bridge bridge local
86dd65b881b9 bridge bridge local
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Remove the NewGeneric utility as it was not used anywhere, except for
in tests.
Also "modernize" the type, and use `any` instead of `interface{}`.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- Use a more modern approach to check error-types
- Touch-up grammar of the error-message
- Remove redundant "nil" check for errors, as it's never nil at that point.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
...that Swarmkit no longer needs now that it has been migrated to use
the new-style driver registration APIs.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The only remaining user is Swarmkit, which now has its own private copy
of the package tailored to its needs.
Signed-off-by: Cory Snider <csnider@mirantis.com>
...which ignore the config argument. Notably, none of the network
drivers referenced by Swarmkit use config, which is good as Swarmkit
unconditionally passes nil for the config when registering drivers.
Signed-off-by: Cory Snider <csnider@mirantis.com>
setupBridgeNetFiltering:
- Indicate that the bridgeInterface argument is unused (but it's needed
to satisfy the signature).
- Return instead of nullifying the err. Still not great, but I thought it
was very slightly more logical thing to do.
checkBridgeNetFiltering:
- Remove unused argument, and scope ipVerName to the branch where it's
used.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
initConnection was effectively just part of the constructor; ot was not
used elsewhere. Merge the two functions to simplify things a bit.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This const was added in 8301dcc6d7, before
being moved to libnetwork, and moved back, but it was never used.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- remove local bridgeName variable that shadowed the const, but
used the same value
- remove some redundant `var` declarations, and changed fixed
values to a const
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
None of these errors were string-matched anywhere, so let's change them
to be non-capitalized, as they should.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
looks like this error was added in 1cbdaebaa1,
and later moved to libnetwork in 44c96449c2
which also updated the description to something that doesn't match what
it means.
In either case, this error was never used as a special / sentinel error,
so we can just use a regular error return.
While at it, I also lower-cased the error-message; it's not string-matched
anywhere, so we can update it to make linters more happy.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- validate input variables before constructing the ChainInfo
- only construct the ChainInfo if things were successful
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Clarify that the argument to New is an exclusive upper bound.
Correct the documentation for SetAnyInRange: the end argument is
inclusive rather than exclusive.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The idm package wraps bitseq.Handle to provide an offset and
synchronization. bitseq.Handle wraps bitmap.Bitmap to provide
persistence in a datastore. As no datastore is passed and the offset is
zero, the idm.Idm instance is nothing more than a concurrency-safe
wrapper around a bitmap.Bitmap with differently-named methods. Switch
over to using bitmap.Bitmap directly, using the ovmanager driver's mutex
for concurrency control.
Hold the driver mutex for the entire duration that VXLANs are being
assigned to the new network. This makes allocating VXLANs for a network
an atomic operation.
Signed-off-by: Cory Snider <csnider@mirantis.com>
In the network.obtainVxlanID() method, the mutex only guards a local
variable and a function argument. Locking is therefore unnecessary.
The network.releaseVxlanID() method is only called in two contexts:
driver.NetworkAllocate(), where the network struct is a local variable
and network.releaseVxlanID() is only called in failure code-paths in
which the network does not escape; and driver.NetworkFree(), while the
driver mutex is held. Locking is therefore unnecessary.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The OptionLocalKVProvider, OptionLocalKVProviderURL, and OptionLocalKVProviderConfig
options were only used in tests, so un-export them, and move them to the
test-files.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function was implemented in dd4950f36d
which added a "key" field, but that field was never used anywhere, and
still appears unused.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The `store.Watch()` was only used in `Controller.processEndpointCreate()`,
and skipped if the store was not "watchable" (`store.Watchable()`).
Whether a store is watchable depends on the store's datastore.scope;
local stores are not watchable;
func (ds *datastore) Watchable() bool {
return ds.scope != LocalScope
}
datastore is only initialized in two locations, and both locations set the
scope field to LocalScope:
datastore.newClient() (also called by datastore.NewDataStore()):
3e4c9d90cf/libnetwork/datastore/datastore.go (L213)
datastore.NewTestDataStore() (used in tests);
3e4c9d90cf/libnetwork/datastore/datastore_test.go (L14-L17)
Furthermore, the backing BoltDB kvstore does not implement the Watch()
method;
3e4c9d90cf/libnetwork/internal/kvstore/boltdb/boltdb.go (L464-L467)
Based on the above;
- our datastore is never Watchable()
- so datastore.Watch() is never used
This patch removes the Watchable(), Watch(), and RestartWatch() functions,
as well as the code handling watching.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The sequential field determined whether a lock was needed when storing
and retrieving data. This field was always set to true, with the exception
of NewTestDataStore() in the tests.
This field was added in a18e2f9965
to make locking optional for non-local scoped stores. Such stores are no
longer used, so we can remove this field.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Make the code slightly more idiomatic; remove some "var" declarations,
remove some intermediate variables and redundant error-checks, and remove
the filePerm const.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This boolean was not used anywhere, so we can remove it. Also cleaning up
the implementation a bit.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
