Commit ec65f2d21b has a typo: it switches
indexes[byTable] with indexes[byNetwork]. The indexes are not
equivalent. Switch it back.
Signed-off-by: Cory Snider <csnider@mirantis.com>
When a node leaves a network or the cluster, or memberlist considers the
node as failed, NetworkDB atomically deletes all table entries (for the
left network) owned by the node. This maintains the invariant that table
entries owned by a node are present in the local database indices iff
that node is an active cluster member which is participating in the
network the entries pertain to.
(*NetworkDB).handleTableEvent() is written in a way which attempts to
minimize the amount of time it is in a critical section with the mutex
locked for writing. It first checks under a read-lock whether both the
local node and the node where the event originated are participating in
the network which the event pertains to. If the check passes, the mutex
is unlocked for reading and locked for writing so the local database
state is mutated in a critical section. That leaves a window of time
between the participation check the write-lock being acquired for a
network or node event to arrive and be processed. If a table event for a
node+network races a node or network event which triggers the purge of
all table entries for the same node+network, the invariant could be
violated. The table entry described by the table event may be reinserted
into the local database state after being purged by the node's leaving,
resulting in an orphaned table entry which the local node will bulk-sync
to other nodes indefinitely.
It's not completely wrong to perform a pre-flight check outside of the
critical section. It allows for an early return in the no-op case
without having to bear the cost of synchronization. But such optimistic
concurrency control is only sound if the condition is double-checked
inside the critical section. It is tricky to get right, and this
instance of optimistic concurrency control smells like a case of
premature optimization. Move the pre-flight check into the critical
section to ensure that the invariant is maintained.
Signed-off-by: Cory Snider <csnider@mirantis.com>
(*NetworkDB).SetPrimaryKey() acquires a read lock on the NetworkDB
instance. That seems sound on the surface as it is only reading from the
NetworkDB struct, not mutating it. However, concurrent calls to
(*memberlist.Keyring).UseKey() would get flagged by Go's race detector
due to some questionable locking in its implementation. Acquire an
exclusive lock in SetPrimaryKey so concurrent calls don't race each
other.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The bridge driver should figure out whether it's running in
a mirrored WSL2 setup, and tell the firewaller.
So, move the WSL2-deciding code back into the bridge driver
and unit test it there. Use TestIptabler to check the rules
are constructed properly.
Signed-off-by: Rob Murray <rob.murray@docker.com>
integration/internal/network/ops.go:137:1: ST1020: comment on exported function WithIPAMRange should be of the form "WithIPAMRange ..." (staticcheck)
// WithIPAM adds an IPAM with the specified Subnet, IPRange and Gateway to the network
^
libnetwork/bitmap/sequence.go:50:1: ST1020: comment on exported function New should be of the form "New ..." (staticcheck)
// NewHandle returns a new Bitmap of ordinals in the interval [0, n).
^
libnetwork/diagnostic/server.go:61:1: ST1020: comment on exported method HandleFunc should be of the form "HandleFunc ..." (staticcheck)
// Handle registers the handler function for the given pattern,
^
libnetwork/endpoint.go:388:1: ST1020: comment on exported method Key should be of the form "Key ..." (staticcheck)
// endpoint Key structure : endpoint/network-id/endpoint-id
^
libnetwork/network.go:574:1: ST1020: comment on exported method MarshalJSON should be of the form "MarshalJSON ..." (staticcheck)
// TODO : Can be made much more generic with the help of reflection (but has some golang limitations)
^
libnetwork/network.go:633:1: ST1020: comment on exported method UnmarshalJSON should be of the form "UnmarshalJSON ..." (staticcheck)
// TODO : Can be made much more generic with the help of reflection (but has some golang limitations)
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
NetworkDB's Watch() facility is problematic to use in practice. The
stream of events begins when the watch is started, so the watch cannot
be used to process table entries that existed beforehand. Either option
to process existing table entries is racy: walking the table before
starting the watch leaves a race window where events could be missed,
and walking the table after starting the watch leaves a race window
where created/updated entries could be processed twice.
Modify Watch() to initialize the channel with synthetic CREATE events
for all existing entries owned by remote nodes before hooking it up to
the live event stream. This way watchers observe an equivalent sequence
of events irrespective of whether the watch was started before or after
entries from remote nodes are added to the database. Remove the bespoke
and racy synthetic event replay logic for driver watches from the
libnetwork agent.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The gossip protocol which powers NetworkDB does not guarantee in-order
reception of events. This poses a problem with deleting entries: without
some mechanism to discard stale CREATE or UPDATE events received after a
DELETE, out-of-order reception of events could result in a deleted entry
being spuriously resurrected in the local NetworkDB state! NetworkDB
handles this situation by storing "tombstone" entries for a period of
time with the Lamport timestamps of the entries' respective DELETE
events. Out-of-order CREATE or UPDATE events will be ignored by virtue
of having older timestmaps than the tombstone entry, just like how it
works for entries that have not yet been deleted.
NetworkDB was only storing a tombstone if the entry was already present
in the local database at the time of the DELETE event. If the first
event received for an entry is a DELETE, no tombstone is stored. If a
stale CREATE/UPDATE event for the entry (with an older timestamp than
the DELETE) is subsequently received, NetworkDB erroneously creates a
live entry in the local state with stale data. Modify NetworkDB to store
tombstones for DELETE events irrespective of whether the entry was known
to NetworkDB beforehand so that it correctly discards out-of-order
CREATEs and UPDATEs in all cases.
Signed-off-by: Cory Snider <csnider@mirantis.com>
NetworkDB gossips changes to table entries to other nodes using distinct
CREATE, UPDATE and DELETE events. It is unfortunate that the wire
protocol distinguishes CREATEs from UPDATEs as nothing useful can be
done with this information. Newer events for an entry invalidate older
ones, so there is no guarantee that a CREATE event is broadcast to any
node before an UPDATE is broadcast. And due to the nature of gossip
protocols, even if the CREATE event is broadcast from the originating
node, there is no guarantee that any particular node will receive the
CREATE before an UPDATE. Any code which handles an UPDATE event
differently from a CREATE event is therefore going to behave in
unexpected ways in less than perfect conditions.
NetworkDB table watchers also receive CREATE, UPDATE and DELETE events.
Since the watched tables are local to the node, the events could all
have well-defined meanings that are actually useful. Unfortunately
NetworkDB is just bubbling up the wire-protocol event types to the
watchers. Redefine the table-watch events such that a CREATE event is
broadcast when an entry pops into existence in the local NetworkDB, an
UPDATE event is broadcast when an entry which was already present in the
NetworkDB state is modified, and a DELETE event is broadcast when an
entry which was already present in the NetworkDB state is marked for
deletion. DELETE events are broadcast with the same value as the most
recent CREATE or UPDATE event for the entry.
The handler for endpoint table events in the libnetwork agent assumed
incorrectly that CREATE events always correspond to adding a new active
endpoint and that UPDATE events always correspond to disabling an
endpoint. Fix up the handler to handle CREATE and UPDATE events using
the same code path, checking the table entry's ServiceDisabled flag to
determine which action to take.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The function used a `cmdErr` error, but it was discarded when returning an
error. Also removing the named "err" return variable.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
NetworkDB uses a hierarchy of queues to prioritize messages for
broadcast. Unfortunately the logic to pull from multiple queues is
flawed. The length of the messages pulled from the first queue is not
taken into account when pulling messages from the second queue. A list
of messages up to tiwce as long as the limit could be returned! Messages
beyond the limit will be truncated unceremoniously by memberlist.
Memberlist broadcast queues assume that all messages returned from a
GetBroadcasts call will be broadcasted to other nodes in the cluster.
Messages are popped from the queue once they have hit their retransmit
limit. On a busy system messages may be broadcast fewer times than
intended, possibly even being dropped without ever being broadcast!
Subtract the length of messages pulled from the first queue from the
broadcast size limit so the limit is not exceeded when pulling from the
second queue.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The loopback-test fixes seem to be sufficient to resolve the flakiness
of all the tests aside from TestFlakyNetworkDBIslands.
Signed-off-by: Cory Snider <csnider@mirantis.com>
NetworkDB defaults to binding to the unspecified address for gossip
communications, with no advertise address set. In this configuration,
the memberlist instance listens on all network interfaces and picks one
of the host's public IP addresses as the advertise address.
The NetworkDB unit tests don't override this default, leaving them
vulnerable to flaking out as a result of rogue network traffic
perturbing the test, or the inferred advertise address not being useable
for loopback testing. And macOS prompts for permission to allow the test
executable to listen on public interfaces every time it is rebuilt.
Modify the NetworkDB tests to explicitly bind to, advertise, and join
ports on 127.0.0.1 to make the tests more robust to flakes in CI and
more convenient to run locally.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The NetworkDB unit tests instantiate clusters which communicate over
loopback where every "node" listens on a distinct localhost port. The
tests make use of a NetworkDB configuration knob to set the port. When
the NetworkDB configuration's BindPort field is set to a nonzero value,
its memberlist instance is configured to bind to the specified port
number. However, the advertise port is left at the
memberlist.DefaultLANConfig() default value of 7946. Because of this,
nodes would be unable to contact any of the other nodes in the cluster
learned by gossip as the gossiped addresseses specify the wrong ports!
The flaky tests passed as often as they did thanks to the robustness of
the memberlist module: NetworkDB gossip and and memberlist node
liveness-probe pings to unreachable nodes can all be relayed through
the reachable nodes, the nodes on the bootstrap join list.
Make the NetworkDB unit tests less flaky by setting each node's
advertise port to the bind port.
The daemon is unaffected by this oversight as it unconditionally uses
the default listen port of 7946, which aligns with the advertise port.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The function was accessing the index map without holding the mutex, so
it would race any mutation to the database indexes. Fetch the reference
to the tree's root while holding a read lock. Since the radix tree is
immutable, taking a reference to the root is equivalent to starting a
read-only database transaction, providing a consistent view of the data
at a snapshot in time, even as the live state is mutated concurrently.
Also optimize the WalkTable function by leveraging the immutability of
the radix tree.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Per-network option com.docker.network.bridge.trusted-host-interfaces
accepts a list of interfaces that are allowed to route
directly to a container's published ports in a bridge
network with nat enabled.
This daemon level option disables direct access filtering,
enabling direct access to published ports on container
addresses in all bridge networks, via all host interfaces.
It overlaps with short-term env-var workaround:
DOCKER_INSECURE_NO_IPTABLES_RAW=1
- it does not allow packets sent from outside the host to reach
ports published only to 127.0.0.1
- it will outlive iptables (the workaround was initially intended
for hosts that do not have kernel support for the "raw" iptables
table).
Signed-off-by: Rob Murray <rob.murray@docker.com>
trusted_host_interface have access to published ports on container
addresses - enabling direct routing to the container via those
interfaces.
Signed-off-by: Rob Murray <rob.murray@docker.com>
On Sandbox restore if an Endpoint (or Network) can't be loaded from
the store and the container is to be preserved (live-restore), stub
Network and Endpoint objects are created and added to the cache. But
then the Endpoint is just dropped - leaving the stub objects in
cache.
If not-live-restore, the Sandbox is reconstructed do that it can be
deleted by sb.delete(force=true). But, the only thing sb.delete does
with the Endpoint in this case is call ep.Delete. And, ep.Delete
doesn't do anything if the Network and Endpoint can't both be loaded
from the store. So, again, the stub objects are left in the in-memory
cache and nothing that might have needed tidying up got tidied up.
So, whether live-restoring or not, just log an error and ignore an
Endpoint if it can't be loaded from disk.
Signed-off-by: Rob Murray <rob.murray@docker.com>
- Use structured logging.
- Which means ids are logged consistently.
- Use variable 'isRestore' instead of extra map lookups.
Signed-off-by: Rob Murray <rob.murray@docker.com>
libnetwork/network.go:637:6: shadow: declaration of "v" shadows declaration at line 634 (govet)
if v, ok := n.generic[netlabel.GenericData]; ok {
^
libnetwork/service_linux.go:125:7: shadow: declaration of "ep" shadows declaration at line 86 (govet)
if ep := sb.getGatewayEndpoint(); ep != nil {
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
