fix(apiserver): decide the API server migration from live cluster state - #5103
fix(apiserver): decide the API server migration from live cluster state#5103xiumozhan wants to merge 1 commit into
Conversation
pasanw
left a comment
There was a problem hiding this comment.
I prefer the wait approach over the bridge approach, but we need the implementation to be more comprehensive. The current implementation has the following issues:
Deadlock scenarios:
- API server is unhealthy > needs apiserver controller reconcile to fix it > apiserver reconcile blocks waiting for default deny to be deleted by core controller > core controller can't delete default deny because API server is unhealthy
- API server is unhealthy > needs apiserver controller reconcile to fix it > apiserver controller reconcile fails to check default deny status because API server is unhealthy
False equivalence:
- If the API server is coming online but not ready, the tier watch will not be ready. This makes us fall back to the tigera-system namespace check, and proceed if the namespace is gone. But the namespace being gone provides no guarantee that the core controller has removed the default deny in calico-system. Therefore, tier watch not ready + no tigera-system namespace is not equivalent to the default deny not being present
- We treat a NoMatch to mean the v3 API group is not installed and therefore there's no way that the default deny could be in place. I think this is false, I believe the only scenario that we would see such an error is if we are in API server mode and the API server is available. Therefore, NoMatch is not equivalent to the default deny not being present
Default denies outside of system tiers
It's possible that a customer can implement a default deny in their own tier. Right now we rely on the move of the API server into the calico-system tier as being safe without any policy being there because the default deny in calico-system excludes it. If it the traffic makes it to the default deny and does not get selected, it progresses to the remaining tiers. If those tiers block the connection, then our migration will be stuck - apiserver will be getting its traffic denied.
To make the solution comprehensive, I think we need the following adjustments:
- The controller needs to detect when the current API server deployment is healthy (able to process requests). As part of this check, it can also determine which API server is deployed (the tigera-system one, or the calico-system one)
- The controller also needs to detect when a migration is needed (either namespace, tier)
- We only perform the wait for the default deny removal if the current API server is healthy and the migration is needed
- If the migration is needed but the API server is unhealthy and is a tigera-system one, we should exit the reconcile. Our reconcile might make things worse - our reconcile is only safe if we know the calico system default deny is gone but we can't check that because the API server is unhealthy. When we exit our reconcile for this case, we need to ensure something will trigger the reconcile again in the future so we can move forward once the API server is healthy.
- If the migration is needed, and the API server is healthy (and we passed the check for the default deny deletion), we should adjust the order of the components such that the policy component is first. This ensures that we don't migrate without the new policy being in place first. It's safe to put the policy component first in this scenario because we validated the api server is healthy (no deadlock potential)
- Fix the false equivalencies: remove the tigera-system namespace fallback check, remove the NoMatch condition
On a direct upgrade from the deprecated API server layout, the apiserver controller moved the API server into calico-system and repointed the v3.projectcalico.org APIService at it before the leftover allow-tigera.default-deny policy had been removed. That policy sits in the earlier-evaluated allow-tigera tier and selects all() endpoints, so it denied the moved pod at end-of-tier and the pod never became ready. The aggregated API then stayed down permanently, because removing the policy requires the API server that had just been taken out of service. The controller now decides whether to perform the move by reading live state through an uncached reader, so the decision cannot be made from cached state that is no longer true: only v3.LicenseKey is excluded from the manager cache, so a cached read of a projectcalico.org/v3 object is served from the informer's last known contents even when the aggregated API cannot serve. The v3.projectcalico.org APIService supplies both inputs, and the kube-apiserver serves it directly, so reading it does not depend on the thing being measured. spec.service.namespace says which layout is deployed; the Available condition reports the aggregator's own most recent verdict on whether requests can be served. A migration is pending only while that APIService still points into tigera-system, so an upgrade that has already moved the API server does not wait on a policy that can no longer select the pod. When a migration is pending and the aggregated API can serve, the move waits for the deprecated policy to be removed by the installation controller. When a migration is pending and the aggregated API cannot serve, nothing is applied at all: the trap cannot be confirmed gone and nothing can clear it while the API is down, so moving would only repoint the aggregated API onto a pod that cannot be vouched for. The periodic reconcile already registered in Add() retriggers the decision, and an unrecognised decision holds rather than proceeding, since failing open on this gate is not recoverable. Two unsound tests are removed. The tigera-system namespace fallback treated the namespace's absence as proof the gate had passed; that does not hold on a two-hop upgrade, where the intermediate release deletes the namespace while leaving the policy in place, nor after an administrator deletes the namespace to clear a stuck upgrade. The NoMatch exemption treated a missing RESTMapper mapping as proof the policy was absent, which is not evidence about the policy at all. On the pass that performs the migration the projectcalico.org/v3 NetworkPolicy component is applied before the workload rather than after it, removing the window in which the moved pod runs with no policy of its own. Every other pass keeps the existing order, which exists so that a fresh install is not blocked on an API server that cannot become available until the install has progressed. Refs: EV-6821
4f991b6 to
fb411c4
Compare
Description
Bug fix for EV-6821.
On a direct upgrade from the deprecated API server layout, the apiserver-controller moved the Calico API server into
calico-systemand repointed thev3.projectcalico.orgAPIService at it before the leftoverallow-tigera.default-denypolicy had been removed. That policy sits in the earlier-evaluatedallow-tigeratier and selectsall()endpoints, so it denied the moved pod at end-of-tier, the pod never became ready, and the aggregated API stayed down permanently — removing the policy requires the API server that had just been taken out of service.This replaces the first version of this PR, which gated the move on a single cached read of that policy. The controller now decides from live state read through
mgr.GetAPIReader(), so the decision cannot be made from cached contents that are no longer true: onlyv3.LicenseKeyis excluded from the manager cache, so a cached read of aprojectcalico.org/v3object is served from the informer's last known state even when the aggregated API cannot serve.Both inputs come from the
v3.projectcalico.orgAPIService, which the kube-apiserver serves itself, so reading it does not depend on the thing being measured.spec.service.namespacesays which layout is deployed; theAvailablecondition is the aggregator's own most recent verdict on whether requests can be served. A migration is pending only while that APIService still points intotigera-system, so an upgrade that has already moved the API server does not wait on a policy that can no longer select the pod.Add()retriggers the decision.Two unsound tests are removed. The
tigera-systemnamespace fallback treated the namespace's absence as proof the gate had passed; that does not hold on a two-hop upgrade, where the intermediate release deletes the namespace while leaving the policy in place, nor after an administrator deletes the namespace to clear a stuck upgrade. TheNoMatchexemption treated a missing RESTMapper mapping as proof the policy was absent, which is not evidence about the policy at all.On the pass that performs the migration the
projectcalico.org/v3NetworkPolicy component is applied before the workload rather than after it, removing the window in which the moved pod runs with no policy of its own. Every other pass keeps the existing order, which exists so a fresh install is not blocked on an API server that cannot come up yet.Deliberately not changed.
APIServer.Status.Stateremains a latch. The tiers controller gates tier creation on it throughIsProjectCalicoV3Available, so making it live would stop thecalico-systemtier being created while the move is held, which would stop the installation controller reaching its delete of the trap — a new deadlock caused by the fix.Components affected:
pkg/controller/apiserveronly.Testing
Unit tests cover the decision table, including the two-hop state where the APIService already points at
calico-systemand the deny still exists, which must proceed rather than hold. Reconcile-level tests cover both hold paths, asserting that nocalico-apiserverDeployment is created and the APIService is not repointed. The component ordering is pinned by a test that records real clientCreateorder in both directions.Live-validated twice on fresh CE 3.21.4 clusters upgrading to 3.23.1, exercising both hold branches in sequence in each run: the API server was taken down before the upgrade was applied, the gate held without applying anything while the aggregated API was unreachable, moved to waiting on the deprecated deny once it could serve again, and completed once the installation controller removed it. In both runs
calico-system.apiserver-accesswas created before thecalico-apiserverDeployment. Post-cutover unavailability while the new pod passes its readiness probe was 21 and 33 seconds, and recovered in both cases. Both runs reached the expected end state.Release Note
For PR author
make gen-files— not applicable, no API changes.make gen-versions— not applicable, no version changes.