Before this PR, when graceful restart was configured for a neighbor
and when the restart flag was set by the restarting speaker, if
the neighbor was not advertising the GR capability, the initial
paths list was never sent by the restarting speaker to its neighbor
This is a problem when the server is configured with graceful
restart for all its peers without knowing if the peer supports it.
If some of the peers don't support it, they may never receive the
routes from the restarting speaker, leading to an inconsistent
routing state.
The gRPC code paths uses different functions than the BGP code path.
Thus is did not receive the fix for the mac mobility handling.
Fixes: c393f43 ("evpn: fix quadratic evpn mac-mobility handling")
Func capabilitiesFromConfig was always taken under the read lock.
However, when graceful restart is enabled for some families, this
function writes to the neighbor config which creates a data race.
Send Cease/Hard Reset notification for certain scenario when graceful
restart + notification support (RFC8538) are enabled. In this
implementation, we follow the suggestion of RFC8538 and map following
notification subcodes to Hard Reset subcode.
1. BGP_ERROR_SUB_MAXIMUM_NUMBER_OF_PREFIXES_REACHED
In this case, GoBGP is in the resource shortage and not working
properly. Thus, the peer should stop forwarding packet immediately.
2. BGP_ERROR_SUB_ADMINISTRATIVE_SHUTDOWN
This happens when the user uses DisablePeer API. This clearly indicates
user's intention of shutting down the session. Thus, we should send Hard
Reset.
3. BGP_ERROR_SUB_PEER_DECONFIGURED
This happens when the user uses DeletePeer API or StopBgp API or there's
an ASN mismatch found in the Open phase. The former two cases, the user
shows the intention to shutdown the session, so we should Hard Reset.
The latter case is not so obvious, but I think it's ok to do Hard Reset
because it is an unrecoverable error that cannot be solved without
user's involvement.
4. BGP_ERROR_SUB_HARD_RESET
This case currently doesn't exist, but obviously we should send Hard
Reset when someone explicitly specifies it.
The behavior for the remaining subcodes are unchanged. We may want to
expose a knob to adjust the behavior of BGP_ERROR_SUB_ADMINISTRATIVE_RESET
as suggested by RFC8538, but for this initial implementation, we kept it
as is.
Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
Currently, even though `ApplyPolicy` is called for determining the
accepted routes after apply policy, the new route with attribute
modifications is not returned. This is problematic for gRPC API users.
Tests are added for all four cases that were described in
https://github.com/osrg/gobgp/issues/2765. This PR makes the behaviour
correct for "Case/Attempt 2" described in the issue.
- Compares IPs using net.IP.Equal instead of using a string comparison
in order to avoid unnecessary allocations.
- Adds peer.routerID to access IP, called by peer.RouterID.
Moves a number of heavily called debug lines behind if checks to avoid
needless allocations of Fields objects and stringification of fields.
For cases where the server is not set to "debug" log level, these fields
were allocated on the heap and then immediately discarded - as well a
number of these were stringifying state / NLRIs regardless of log level.
In servers with significant amounts of routes and BGP peers, this lead
to a large amount of wasted allocations - in our case looking at Go's
memory profiler, 25% of all allocations were from these lines alone.
Fixes a deadlock condition that can happen if StopBgp is called when the
pending connection queue is full. During teardown, StopBgp calls a
mgmtOp on the server goroutine which attempts to stop the goroutine
accepting inbound connections, and waits for it to finish before
continuing.
This connection goroutine can block if the connection queue is full,
which is read by the same goroutine that processes all mgmtOps. This
means that if the queue is full and the goroutine is currently blocked,
then calling StopBgp will lead to a complete deadlock, as the connection
goroutine will never close as it is trying to send to the queue, but the
queue will not be read as the server goroutine is currently waiting for
the connection goroutine to exit.
To correct this, a context has been added that gets passed to the
connection goroutine. This is then checked in a new select statement on
the connection queue which gets cancelled by tcpListener.Close() ensuring
the goroutine exits correctly even if the queue is full.
Additionally change the `Key` to be the listener address, which is hopefully
more useful than a difficult-to-decipher dump of the listener struct.
The previous behavior would result in log lines like the following, even if
nothing went wrong:
```
time="2024-01-31T17:30:25Z" level=warning msg="accept failed" Error="<nil>" Key="&{0x140002e4000 {<nil> 0 0}}" Topic=grpc
```
With this change, the message is only logged if there was an error, and it will look like this:
```
time="2024-01-31T17:40:25Z" level=warning msg="accept failed" Error="lolol just testing" Key="127.0.0.1:59289" Topic=grpc
```
Currently, graceful restart waits for the EoR message for all address
families "enabled" for the peer, but it should only wait for "received"
address families (the address families the peer is capable of handling).
Fixes: #2524
Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
It could be that a peer gets deleted and added back during the
transition from active state to open confirm. In that case, the TCP
connection from the old version of the peer could still be up. This
is a problem if the server is a listener only as the remote peer
would consider the old TCP connection as being valid and it won't
be able to connect until the TCP connection is eventually cleaned
by the Golang GC.
Adds usage of the "prefix-based" TCP MD5 for dynamic
neighbors. Non-dynamic neighbors will continue to use
non-prefix based, which makes it more compatible with
running on older kernels, as only 4.14+ includes the
necessary support.
This change also includes tests of dynamic peers in general.
This method was attempting to read from peer.fsm before acquiring a read
lock, leading to a data race as this struct is written by a different
goroutine in parallel. Commit moves the call to RLock before the first
read from the struct.
Currently GoBGP does not accept UPDATE messages with nexthops pointing
to a loopback address. This disallows multiple GoBGP instances from
running at the same time on 127.0.0.0/8.
This PR proposes removing this constraint when the RouterID of the
current GoBGP instance itself resides within the testing subnet of
127.0.0.0/8.
With a lot of paths (hundreds of thousands) gobgp may oom or
stuck in swapping.
Allow to specify max batch size via grpc and keep unlimited batch
size by default since 21093fbc87
without preallocation on the first run, so it still should not
affect perfomance/allocations with small ammount of paths.
Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru>
When ListPathRequest is done by a gRPC client including a malformed prefix,
the server would crash an invalid memory address reference.
This commit fixes the crash by checking whether the parseCIDR method returned
an error.
Filtering by peer address worked only for initial state.
Using watch event's filter allows to use it for each event and could be extended by other conditions, e.g. peer group.
peer_address and peer_group were added to API (backward compatible).
Signed-off-by: Rinat Baygildin <bayrinat@yandex-team.ru>
During SRv6 prefix SID unmarshalling, wrong Path Attribute
length was applied (4B instead of 3B). This was causing
a 1 byte difference in PA length after marshalling & unmarshalling.
Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
restore the API breakage due to c801e18495.
However, this is a hack. Should be removed with the major version updated.
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
