* refactor(errors): centralize fatal error handling
Detect wrapped PanicError values consistently and preserve their underlying causes when adding fatal context. Apply the helpers to manager, cluster, and IPVS error paths.
Assisted-by: GitHub-Copilot:unspecified
Signed-off-by: Marcel Fest <marcel.fest@telekom.de>
* fix(watchers): restart after terminal watch failures
Propagate fatal endpoint watcher failures through the owning service watcher so kube-vip releases leadership instead of remaining active with a stale watch. Treat terminal service, node, and annotation watch failures as errors while preserving clean context cancellation.
Return exhausted authorization failures to RetryWatcher, safely decode watch error objects, and replace direct go-spew diagnostics with structured logging.
Fixes#1685
Assisted-by: GitHub-Copilot:unspecified
Signed-off-by: Marcel Fest <marcel.fest@telekom.de>
* fix(services): replace state after traffic policy changes
Recreate the service context and instance as one generation when a Service change requires teardown. Ignore delayed leadership cleanup from superseded contexts so it cannot remove replacement state.
This prevents a stale Cluster-policy endpoint watcher from winning the service lease after externalTrafficPolicy changes to Local.
Assisted-by: GitHub-Copilot:unspecified
Signed-off-by: Marcel Fest <marcel.fest@telekom.de>
* fix(cli): return command errors to container runtime
Propagate manager and service command failures through Cobra so the process exits with status 1. Show usage for invocation errors while keeping runtime failures concise.
Assisted-by: GitHub-Copilot:unspecified
Signed-off-by: Marcel Fest <marcel.fest@telekom.de>
* refactor(logging): use structured errors
Replace direct stdout error output with slog records for command failures and traffic mirror qdisc lookup failures.
Assisted-by: GitHub-Copilot:unspecified
Signed-off-by: Marcel Fest <marcel.fest@telekom.de>
* fix(watchers): continue after endpoint deletion
Keep EndpointSlice watchers active when an individual endpoint object is deleted so replacement objects can be observed and service traffic can recover.
Assisted-by: GitHub-Copilot:unspecified
Signed-off-by: Marcel Fest <marcel.fest@telekom.de>
---------
Signed-off-by: Marcel Fest <marcel.fest@telekom.de>
kube-vip's DHCP client uses nclient4, whose BroadcastRawUDPConn.ReadFrom could
compute a negative DHCP length and panic on a malformed reply. That was fixed
in insomniacslk/dhcp#583; this bumps the module past that commit.
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
the connmark policy-based routing was broken by tailscale, as we collide
with their connmark range (bits 16 to 23)
Signed-off-by: Daniel Nägele <daniel@naegele.dev>
This PR adds an explicit configuration option `vip_skipdad`. This allows
for a proper behaviour in IPv6 networks with RT mode and ECMP. Without
this option only one node can ever hold the same IPv6 VIP what defeats
the purpose of ECMP.
While the ARP mode currently has a recovery mode for failed DAD, RT mode
misses it. It is also more feasible to use config option, as recovery
mode in RT mode would always introduce a ~5s blackholing (we know there
will be a conflict, so first attempt to add a VIP will fail and only the
next one will force IFA_F_NODAD).
For simplicitly we ignore IPv4/IPv6 checks, as kernel transparently
strips IFA_F_NODAD from v4 addresses without throwing errors.
For systems with IPv4 duplicate detection (called ACD and used e.g. in
RHEL10's NetworkManager) this code has no effect as it's only userspace
action and addresses we add via netlink bypass any check.
First discovered on a dual-stack cluster with two routing-table-mode
advertisers: the ingress VIP answered or timed out depending on which
ECMP path the flow hashed to; with NODAD both advertisers accept
traffic.
Signed-off-by: Mat Kowalski <mko@redhat.com>
* fix(lease): do not let a stale cleanup cancel a recreated lease
Every object that starts leader election also starts a goroutine that calls
Manager.Delete once its context is cancelled. Manager.Delete looked the lease up
by name only, so it acted on whatever lease held that name at the time it ran,
not the one the caller was given.
When a service is torn down and rebuilt straight away, the replacement lease is
already registered by the time the old cleanup goroutine runs, so the cleanup
cancels the live replacement and removes it from the manager. The service is
then never handled again: its election loop exits, the lease keeps an empty
holderIdentity, and the VIP is never re-advertised.
This is reachable from an ordinary service update. Flipping
externalTrafficPolicy makes serviceChanged cancel the service context and
rebuild it, which reproduced the stuck lease for minutes.
Pass the lease the caller owns to Manager.Delete and ignore a stale caller,
which keeps cleanup scoped to the instance it belongs to. Callers already hold
that lease. Passing nil keeps the previous behaviour of deleting whichever lease
currently holds the name, which is what the existing tests assert.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* fix(lease): retire a lease when its service is torn down
The instance guard in Delete stops a late cleanup from cancelling a replacement
lease, but it cannot help when the replacement *is* the same instance.
A service teardown cancels the service context and leaves the lease registered,
because the cleanup that removes it is deferred to a goroutine. The rebuild that
follows calls Add, which finds that lease still in the map and hands it straight
back, so the new service context is parented to a lease the pending cleanup is
about to cancel. The service then cycles: acquire, lose, re-acquire, every few
seconds, and never settles.
Add Manager.Retire, and call it from the serviceChanged teardown next to the
svcMap purge, so the lease is out of the map before the replacement context is
built. Add also refuses to hand out a lease whose context is already cancelled,
which closes the same hazard for any other path that cancels a lease directly.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* fix(lease): keep a common lease alive for the services still using it
Review feedback from Patryk on #1669: Retire cancelled the lease context
outright, so with a common lease a modification of one service would also tear
down every sibling sharing that lease.
Retire was only ever needed for its side effect of getting the lease out of the
map before the rebuild, and Delete already does exactly that once the last
object is gone. Drop Retire and have the teardown path call Delete with its own
object name, so siblings keep the lease alive and the manager API stays
Add/Delete/Get.
TestManager_LeaseLifetimeInvariant replaces the single-scenario test with the
rule for the whole surface: a lease stays usable for exactly as long as at least
one object holds it, and a rebuild afterwards gets a fresh one. It is table
driven over 1, 2 and 4 objects, and the 2 and 4 cases fail against the reviewed
behaviour with "lease was cancelled with N object(s) still holding it".
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* fix(services): start the leader-election loop once per service - #1665
startLeaderElection restarts itself until the service context is cancelled,
so it only needs to be started once per service lifetime. It was started from
startServiceHandlingIfNeeded on every AddOrModify call instead, and AddOrModify
runs on every EndpointSlice event, so endpoint churn accumulated duplicate
permanent loops for the same service, all contending on the same lease.
Guard the spawn with a sync.Once on the service context. A Once needs no
clearing: the loop is bound to the service context, which is replaced whenever
the service is recreated.
Also drop the les *atomic.Int64 parameter, which was only ever incremented.
Adds a unit test that drives AddOrModify three times and asserts the loop
starts once (fails pre-fix with 3), and an -endpointFlap service e2e test that
flaps the backend 1->0->1 five times and asserts the VIP serves traffic and the
lease has a holder again.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* test(services): fault leases and API server access in the e2e test - #1665
The endpointFlap e2e test only asserted that the service recovered after
endpoint churn, which the unfixed code also satisfies, so it did not prove
anything about the reported failure.
Inject faults that actually stress the per-service leader election and assert
convergence after each one:
- endpoint churn: backend scaled 1->0->1 five times
- lease faults: the lease is deleted, then its holderIdentity is blanked, which
is the exact state reported in the issue
- API server faults: the apiserver is blocked from the leader for longer than
the lease duration, so its election client loses its backend, then restored
The assertions are on the feature contract, a held lease and a served VIP,
rather than on election internals, so they stay meaningful if the
implementation changes. Duplicate loops are not observable from outside the
process; that part stays pinned by the unit test.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* test(services): assert election faults recover, via new election metrics - #1665
The previous e2e test only asserted the service recovered, which the unfixed
code also satisfies, so it proved nothing. The duplicate election loops are not
observable from outside the process, so there was nothing to assert on.
Export the missing state as metrics, replacing the write-only les *atomic.Int64
that used to sit in startLeaderElection with real instrumentation:
- kube_vip_service_election_loops{namespace,name}: live election loops, tracked
for the lifetime of the goroutine. More than 1 per service means loops leaked.
- kube_vip_service_election_attempts_total{namespace,name}: election attempts,
so a wedged restart loop is visible as a counter that stops advancing.
- kube_vip_service_election_errors_total{namespace,name,reason}: election
failures, with reason="no_lease" for the service context and lease manager
desync.
The e2e test now injects a fault per reported failure mode and asserts a signal
that is actually broken when that bug is present:
- endpoint churn, five debounce-separated flaps (#1665): loops stay <= 1
- endpointslice deletion (#1663 / #1664): no reason="no_lease" errors
- lease deletion and blanked holderIdentity (#1650): attempts keep advancing
- apiserver blocked from the leader past the lease duration: same loss path
Every fault is also followed by the feature contract: a held lease and a VIP
that serves traffic. The unit test asserts the loop gauge alongside the call
count so both layers agree.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* test(services): assert the election error counter settles, not that it is zero
Both the fixed and the unfixed branch recorded one reason="no_lease" increment
while the service was first set up, so asserting the counter is zero failed on
correct code too. The #1664 desync makes the counter climb for the lifetime of
the process, so assert it stops growing instead.
Also sample the election loop gauge a few times rather than once, since a loop
that is about to start may not be visible in a single scrape while a leaked one
never goes away.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* test(services): assert election progress after a real leadership loss
Deleting the lease object does not make the election client lose leadership, it
just recreates the lease on the next renew, so no new election attempt follows
and the progress assertion failed on correct code.
Move that assertion to the apiserver partition, which is the fault that actually
drives OnStoppedLeading and returns the election, and is therefore where the
#1650 WaitGroup deadlock would wedge the restart loop. The lease object faults
keep asserting convergence only.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* test(services): cover the remaining election fault scenarios - #1665
The suite only faulted the paths that the three known bugs live on. Add the
neighbouring ones so a regression in this subsystem is caught wherever it lands:
- VIP release on zero endpoints: with a local traffic policy and no endpoints
anywhere, the address has to stop answering instead of black-holing traffic.
Nothing asserted the yield half of the endpoint churn cycle before.
- externalTrafficPolicy flip: makes serviceChanged cancel the service context and
drop it from svcMap, so the next event has to build a fresh context and lease.
Same desync class as the endpointslice fault, reached by a different trigger.
- service event storm: 15 annotation patches, driving the spawn-once invariant
from the service watch instead of the endpoint watch.
- follower partition: cutting a non-leader off from the apiserver must not move
the lease, stop traffic, or leak a loop on the node that comes back.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* test(services): let the election loop count settle before asserting it
A fault that rebuilds the service context, such as the externalTrafficPolicy
flip, legitimately has the old and the new loop alive at the same moment, so a
single scrape saw 2 loops on correct code.
Poll until the count settles instead. A leaked loop only exits with its service
context, which outlives the test, so it never settles and is still caught: the
unfixed code reports 11 loops after endpoint flapping.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* test(services): fault the externalTrafficPolicy teardown path
With the lease retirement fix underneath, a service that is torn down and
rebuilt by a traffic policy change converges again, so this fault can be
asserted like the others: at most one election loop, a settled error counter, a
held lease and a served VIP.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* test(services): stop the fault suite exhausting the client rate limiter
The fault tests poll the API while waiting for convergence, and by the last
fault the shared clientset had spent client-go's default 5 QPS budget, so a
lease read failed with
failed to get lease "kubevip-kube-vip-service": client rate limiter Wait
returned an error: context deadline exceeded
which looked like a convergence failure but was the test's own client giving up.
Raise QPS for the harness and halve the lease polling rate.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* test(services): fault a common lease sibling teardown
Cluster-level cover for the case Patryk raised in review of #1669: two services
share one lease, the first is deleted, and the second has to keep serving on that
same lease. A teardown that cancels the lease instead of just releasing the
leaving service takes the sibling down with it.
Adds a commonLease field to the Service helper, which sets the
kube-vip.io/serviceLease annotation along with the cluster traffic policy that a
common lease requires. Also initialises the annotation map unconditionally: it
was only allocated on the egress path, so setting any other annotation first
would have nil-panicked.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
---------
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* fix(lease): do not let a stale cleanup cancel a recreated lease
Every object that starts leader election also starts a goroutine that calls
Manager.Delete once its context is cancelled. Manager.Delete looked the lease up
by name only, so it acted on whatever lease held that name at the time it ran,
not the one the caller was given.
When a service is torn down and rebuilt straight away, the replacement lease is
already registered by the time the old cleanup goroutine runs, so the cleanup
cancels the live replacement and removes it from the manager. The service is
then never handled again: its election loop exits, the lease keeps an empty
holderIdentity, and the VIP is never re-advertised.
This is reachable from an ordinary service update. Flipping
externalTrafficPolicy makes serviceChanged cancel the service context and
rebuild it, which reproduced the stuck lease for minutes.
Pass the lease the caller owns to Manager.Delete and ignore a stale caller,
which keeps cleanup scoped to the instance it belongs to. Callers already hold
that lease. Passing nil keeps the previous behaviour of deleting whichever lease
currently holds the name, which is what the existing tests assert.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* fix(lease): retire a lease when its service is torn down
The instance guard in Delete stops a late cleanup from cancelling a replacement
lease, but it cannot help when the replacement *is* the same instance.
A service teardown cancels the service context and leaves the lease registered,
because the cleanup that removes it is deferred to a goroutine. The rebuild that
follows calls Add, which finds that lease still in the map and hands it straight
back, so the new service context is parented to a lease the pending cleanup is
about to cancel. The service then cycles: acquire, lose, re-acquire, every few
seconds, and never settles.
Add Manager.Retire, and call it from the serviceChanged teardown next to the
svcMap purge, so the lease is out of the map before the replacement context is
built. Add also refuses to hand out a lease whose context is already cancelled,
which closes the same hazard for any other path that cancels a lease directly.
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
* fix(lease): keep a common lease alive for the services still using it
Review feedback from Patryk on #1669: Retire cancelled the lease context
outright, so with a common lease a modification of one service would also tear
down every sibling sharing that lease.
Retire was only ever needed for its side effect of getting the lease out of the
map before the rebuild, and Delete already does exactly that once the last
object is gone. Drop Retire and have the teardown path call Delete with its own
object name, so siblings keep the lease alive and the manager API stays
Add/Delete/Get.
TestManager_LeaseLifetimeInvariant replaces the single-scenario test with the
rule for the whole surface: a lease stays usable for exactly as long as at least
one object holds it, and a rebuild afterwards gets a fresh one. It is table
driven over 1, 2 and 4 objects, and the 2 and 4 cases fail against the reviewed
behaviour with "lease was cancelled with N object(s) still holding it".
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
---------
Signed-off-by: Maximilian Rink <maximilian.rink@telekom.de>
In the RT-mode with cp_enable, health check resolves the node address
via getNodeIPs and calls https://node-ip:6443.
In deployments where apiserver does not have a certificate for the node
IP this fails with "failed to verify certificate: x509: [...]"
When an explicit k8s address is configured via KubernetesAddr, we want
to use it for the backend health check instead of the node addresses.
Fixes: #1670
Signed-off-by: Mat Kowalski <mko@redhat.com>
AddOrModify only calls leaseMgr.Add inside its `if svcCtx == nil` branch, but
the in-memory lease is removed independently: the cleanup goroutine started by
StartServicesLeaderElection calls leaseMgr.Delete once svcCtx.Ctx is done, and
Manager.Delete drops the lease once its last object goes away.
Several paths cancel the service context without also removing it from svcMap -
the deferred close(stopChan) in watchEndpoint, and the utils.PanicError branch
in AddOrModify. Afterwards svcMap still holds a cancelled context for that UID,
so every later watch event reuses it, skips leaseMgr.Add, and
StartServicesLeaderElection fails on
no existing lease found for service %q with UID %q
for the lifetime of the process. Leader election never restarts and the address
is never re-advertised; only restarting the pod clears it.
This is distinct from #1650, which removed the wg.Wait() deadlock but not this
svcMap <-> leaseMgr desync. Both v1.2.1 and v1.2.2 are affected.
Drop a cancelled service context so the existing code path creates a fresh
context and a fresh lease, restoring the invariant that a service context in
svcMap always has a matching lease in the lease manager.
The alternative - having StartServicesLeaderElection call leaseMgr.Add when Get
returns nil - was rejected because it would attach the lease lifetime to the
service context rather than the watcher context, inverting the intended
ownership model.
Signed-off-by: MaxRink <github@maxrink.de>
