From bc625d24802b17093186a16df54481754893c7bc Mon Sep 17 00:00:00 2001 From: Patrick Zhan Date: Sat, 1 Aug 2026 00:07:42 -0700 Subject: [PATCH] fix(apiserver): decide the API server migration from live cluster state 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 --- .../apiserver/apiserver_controller.go | 105 +++++++++- .../apiserver/apiserver_controller_test.go | 195 ++++++++++++++++++ pkg/controller/apiserver/migration.go | 165 +++++++++++++++ pkg/controller/apiserver/migration_test.go | 192 +++++++++++++++++ 4 files changed, 648 insertions(+), 9 deletions(-) create mode 100644 pkg/controller/apiserver/migration.go create mode 100644 pkg/controller/apiserver/migration_test.go diff --git a/pkg/controller/apiserver/apiserver_controller.go b/pkg/controller/apiserver/apiserver_controller.go index f9aef63d84..9835641723 100644 --- a/pkg/controller/apiserver/apiserver_controller.go +++ b/pkg/controller/apiserver/apiserver_controller.go @@ -67,6 +67,7 @@ var log = logf.Log.WithName("controller_apiserver") func Add(mgr manager.Manager, opts options.ControllerOptions) error { r := &ReconcileAPIServer{ client: mgr.GetClient(), + apiReader: mgr.GetAPIReader(), scheme: mgr.GetScheme(), status: status.New(mgr.GetClient(), "apiserver", opts.KubernetesVersion), tierWatchReady: &utils.ReadyFlag{}, @@ -221,7 +222,13 @@ var _ reconcile.Reconciler = &ReconcileAPIServer{} type ReconcileAPIServer struct { // This client, initialized using mgr.Client() above, is a split client // that reads objects from the cache and writes to the apiserver - client client.Client + client client.Client + // apiReader reads directly from the API server, bypassing the manager's cache. + // The migration decision below must not be made from cached state: a cached read of a + // projectcalico.org/v3 object succeeds against the informer's last known contents even + // when the aggregated API server is unable to serve, which would let us act on state + // that is no longer true. + apiReader client.Reader scheme *runtime.Scheme status status.StatusManager tierWatchReady *utils.ReadyFlag @@ -473,6 +480,76 @@ func (r *ReconcileAPIServer) Reconcile(ctx context.Context, request reconcile.Re } } + // A direct upgrade from the deprecated layout (tigera-apiserver in the tigera-system + // namespace, allow-tigera policy tier) has to clear allow-tigera.default-deny before the + // calico-apiserver pod is moved into calico-system. That policy sits in the + // earlier-evaluated allow-tigera tier and selects all() endpoints, so it denies the moved + // pod at end-of-tier before the calico-system tier - whose default-deny excludes the API + // server - is ever reached. The installation controller removes it through the + // still-serving API server. Repointing the aggregated API onto a trapped pod takes that + // API down permanently, so the move waits. + // + // This is a no-op when the projectcalico.org/v3 API group is backed by CRDs natively: + // there is no aggregated API server to deadlock. + moveIsPending := false + if !r.opts.UseV3CRDs { + layout, aggregatedAPIAvailable, err := readAPIServiceState(ctx, r.apiReader) + if err != nil { + r.status.SetDegraded(operatorv1.ResourceReadError, "Error reading the projectcalico.org/v3 APIService", err, reqLogger) + return reconcile.Result{}, err + } + + denyPresent := false + if layout == layoutDeprecated && aggregatedAPIAvailable { + denyPresent, err = deprecatedDenyPresent(ctx, r.apiReader) + if err != nil { + r.status.SetDegraded(operatorv1.ResourceReadError, "Error checking for the deprecated allow-tigera.default-deny policy", err, reqLogger) + return reconcile.Result{}, err + } + } + + decision := decideMigration(layout, aggregatedAPIAvailable, denyPresent) + switch decision { + case decisionHoldAPIUnavailable: + // Do not apply anything. We cannot confirm the deprecated deny is gone, and + // nothing can clear it while the aggregated API is unable to serve, so moving + // the workload now could only make the situation harder to recover. The + // periodic reconcile re-runs this every utils.PeriodicReconcileTime regardless + // of the requeue below. + reqLogger.Info("The projectcalico.org/v3 API is unavailable and the API server has not been migrated; holding the migration until it can serve") + r.status.SetDegraded(operatorv1.ResourceNotReady, "Waiting for the projectcalico.org/v3 API to become available before migrating the API server", nil, reqLogger) + return reconcile.Result{RequeueAfter: utils.StandardRetry}, nil + case decisionWaitForDenyRemoval: + // The deny is removed by the installation controller only after it renders v3 + // NetworkPolicy into the calico-system tier (pkg/render/kubecontrollers/kube-controllers.go), + // which it only does once the tiers controller has created that tier + // (pkg/controller/installation/core_controller.go), which the tiers controller only + // does once IsProjectCalicoV3Available reports APIServer.Status.State == Ready + // (pkg/controller/tiers/tiers_controller.go). This controller is the only writer of + // that field, and it does so on a path this hold returns before reaching. On a + // supported upgrade that field was already persisted by the previous operator version + // before this reconcile ever ran, so the wait clears on its own; this is not a bug in + // the upgrade path. It would only spin forever if that status were never latched - for + // example the tigera-secure APIServer CR was deleted and recreated around the upgrade. + // Holding is still correct in that case: the alternative is repointing the aggregated + // API onto a pod the deny traps, which is a worse and equally permanent failure, while + // holding here leaves the cluster recoverable. + reqLogger.Info("Waiting for the deprecated allow-tigera.default-deny policy to be removed before migrating the API server") + r.status.SetDegraded(operatorv1.ResourceNotReady, "Waiting for the deprecated allow-tigera.default-deny policy in calico-system to be removed before migrating the API server; this clears once the installation controller deletes that policy", nil, reqLogger) + return reconcile.Result{RequeueAfter: utils.StandardRetry}, nil + case decisionProceed: + moveIsPending = layout == layoutDeprecated + default: + // Fail closed: an unrecognised decision must not fall through to applying the + // move. Repointing the aggregated API onto a pod we have not vouched for would be + // unrecoverable, so treat anything we don't explicitly know about the same as a + // hold. + reqLogger.Error(nil, "Unrecognised migration decision; holding the migration", "decision", decision) + r.status.SetDegraded(operatorv1.ResourceNotReady, "Waiting to migrate the API server: unrecognised migration decision", nil, reqLogger) + return reconcile.Result{RequeueAfter: utils.StandardRetry}, nil + } + } + err = utils.PopulateK8sServiceEndPoint(r.client) if err != nil { r.status.SetDegraded(operatorv1.ResourceReadError, "Error reading services endpoint configmap", err, reqLogger) @@ -577,6 +654,21 @@ func (r *ReconcileAPIServer) Reconcile(ctx context.Context, request reconcile.Re return reconcile.Result{}, err } + // If the projectcalico.org/v3 API group is being backed by our aggregated API server, then + // v3 NetworkPolicy will fail to reconcile until the Calico API server is healthy. Thus, we + // normally only render v3.NetworkPolicy after the aggregated API server becomes available, + // to avoid a chicken-and-egg scenario. + // + // If the projectcalico.org/v3 API group is implemented using CRDs natively, we can install + // network policies immediately, as there is no dependency on the API server deployment. + renderPolicy := r.opts.UseV3CRDs || includeV3NetworkPolicy + + if policyComponentFirst(moveIsPending, renderPolicy) { + // On the migration pass the aggregated API has already been confirmed available, so + // the policy can be applied before the workload it protects rather than after it. + components = append(components, render.APIServerPolicy(&apiServerCfg)) + } + components = append(components, component, rcertificatemanagement.CertificateManagement(&rcertificatemanagement.Config{ @@ -587,14 +679,9 @@ func (r *ReconcileAPIServer) Reconcile(ctx context.Context, request reconcile.Re }), ) - // If the projectcalico.org/v3 API group is being backed by our aggregated API server, then v3 NetworkPolicy will fail to reconcile until the Calico API server is healthy. - // Thus, we only render v3.NetworkPolicy after the aggregated API server becomes available to avoid a chicken-and-egg scenario. - // - // If the projectcalico.org/v3 API group is implemented using CRDs natively, we can install network policies immediately, as there is no - // dependency on the API server deployment. - // - // We do this last to avoid transient errors with policy preventing progression of the controller. - if r.opts.UseV3CRDs || includeV3NetworkPolicy { + if renderPolicy && !policyComponentFirst(moveIsPending, renderPolicy) { + // We do this last to avoid transient errors with policy preventing progression of the + // controller. components = append(components, render.APIServerPolicy(&apiServerCfg)) } diff --git a/pkg/controller/apiserver/apiserver_controller_test.go b/pkg/controller/apiserver/apiserver_controller_test.go index ebcd062b5b..78e8717d93 100644 --- a/pkg/controller/apiserver/apiserver_controller_test.go +++ b/pkg/controller/apiserver/apiserver_controller_test.go @@ -32,7 +32,9 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + apiregv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/reconcile" v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" @@ -49,11 +51,43 @@ import ( "github.com/tigera/operator/pkg/dns" "github.com/tigera/operator/pkg/render" rmeta "github.com/tigera/operator/pkg/render/common/meta" + "github.com/tigera/operator/pkg/render/common/networkpolicy" "github.com/tigera/operator/pkg/render/common/secret" "github.com/tigera/operator/pkg/tls" "github.com/tigera/operator/test" ) +// recordCreateOrder wraps cli with a controller-runtime interceptor client that records every +// object Create call, in the order the calls happen, without changing their behaviour (each +// call is still forwarded to cli). The returned indexOf looks up the position of the first +// Create call for a given Kind/ObjectKey, or -1 if it was never created. See the "policy/ +// workload ordering" Context below for why this is the signal used to pin component ordering. +func recordCreateOrder(cli client.WithWatch) (client.WithWatch, func(kind client.Object, key client.ObjectKey) int) { + type created struct { + kind string + key client.ObjectKey + } + var order []created + + wrapped := interceptor.NewClient(cli, interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + order = append(order, created{kind: fmt.Sprintf("%T", obj), key: client.ObjectKeyFromObject(obj)}) + return c.Create(ctx, obj, opts...) + }, + }) + + indexOf := func(kind client.Object, key client.ObjectKey) int { + want := created{kind: fmt.Sprintf("%T", kind), key: key} + for i, c := range order { + if c == want { + return i + } + } + return -1 + } + return wrapped, indexOf +} + var _ = Describe("apiserver controller tests", func() { var ( cli client.Client @@ -158,6 +192,145 @@ var _ = Describe("apiserver controller tests", func() { mockStatus.On("SetDegraded", operatorv1.ResourceNotReady, mock.Anything, mock.Anything, mock.Anything).Return().Maybe() }) + It("uses a reader that is distinct from the cached client", func() { + r := ReconcileAPIServer{client: cli, apiReader: cli, scheme: scheme, status: mockStatus} + Expect(r.apiReader).NotTo(BeNil()) + }) + + It("does not apply anything while a migration is pending and the aggregated API is unavailable", func() { + Expect(cli.Create(ctx, installation)).To(BeNil()) + Expect(cli.Create(ctx, apiService("tigera-system", false))).NotTo(HaveOccurred()) + mockStatus.On("SetDegraded", operatorv1.ResourceNotReady, mock.Anything, mock.Anything, mock.Anything).Return() + + r := ReconcileAPIServer{ + client: cli, apiReader: cli, scheme: scheme, status: mockStatus, + tierWatchReady: ready, migrationWatchReady: &utils.ReadyFlag{}, + opts: options.ControllerOptions{EnterpriseCRDExists: true, DetectedProvider: operatorv1.ProviderNone}, + } + result, err := r.Reconcile(ctx, reconcile.Request{}) + Expect(err).NotTo(HaveOccurred()) + // Distinguishes this hold from the early, non-gate-related nil-error returns + // elsewhere in Reconcile, which leave RequeueAfter at its zero value. + Expect(result.RequeueAfter).To(Equal(utils.StandardRetry)) + + // The workload must not have been applied. + d := &appsv1.Deployment{} + err = cli.Get(ctx, client.ObjectKey{Name: "calico-apiserver", Namespace: "calico-system"}, d) + Expect(kerror.IsNotFound(err)).To(BeTrue()) + + // And the APIService must not have been repointed. + as := &apiregv1.APIService{} + Expect(cli.Get(ctx, client.ObjectKey{Name: calicoAPIServiceName}, as)).NotTo(HaveOccurred()) + Expect(as.Spec.Service.Namespace).To(Equal("tigera-system")) + }) + + It("does not apply anything while the deprecated allow-tigera.default-deny policy is still in place", func() { + // This is the literal bug this gate exists to prevent: the APIService still points at + // the deprecated namespace, the aggregated API can serve, but the deny that would trap + // the moved pod is still present in calico-system. The move must not happen. + Expect(cli.Create(ctx, installation)).To(BeNil()) + Expect(cli.Create(ctx, apiService("tigera-system", true))).NotTo(HaveOccurred()) + Expect(cli.Create(ctx, networkpolicy.DeprecatedAllowTigeraNetworkPolicyObject("default-deny", render.APIServerNamespace))).NotTo(HaveOccurred()) + mockStatus.On("SetDegraded", operatorv1.ResourceNotReady, mock.Anything, mock.Anything, mock.Anything).Return() + + r := ReconcileAPIServer{ + client: cli, apiReader: cli, scheme: scheme, status: mockStatus, + tierWatchReady: ready, migrationWatchReady: &utils.ReadyFlag{}, + opts: options.ControllerOptions{EnterpriseCRDExists: true, DetectedProvider: operatorv1.ProviderNone}, + } + result, err := r.Reconcile(ctx, reconcile.Request{}) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(utils.StandardRetry)) + + // The workload must not have been applied. + d := &appsv1.Deployment{} + err = cli.Get(ctx, client.ObjectKey{Name: "calico-apiserver", Namespace: "calico-system"}, d) + Expect(kerror.IsNotFound(err)).To(BeTrue()) + + // And the APIService must not have been repointed. + as := &apiregv1.APIService{} + Expect(cli.Get(ctx, client.ObjectKey{Name: calicoAPIServiceName}, as)).NotTo(HaveOccurred()) + Expect(as.Spec.Service.Namespace).To(Equal("tigera-system")) + }) + + Context("policy/workload ordering", func() { + // These specs pin the actual behaviour change: which order Reconcile appends the + // projectcalico.org/v3 NetworkPolicy component relative to the API server workload and + // the certificate-management component. policyComponentFirst's own unit tests (in + // migration_test.go) only pin its truth table; they say nothing about whether the + // append call in Reconcile still uses it correctly. A refactor that collapsed the two + // `if`s, or spliced the policy append between the workload and certificate-management + // appends, would keep those unit tests green while breaking real ordering. These specs + // close that gap by observing Reconcile's actual effect on the client. + // + // The signal is the order in which Create calls land on the client, captured with a + // controller-runtime interceptor client (recordCreateOrder, below). Two more obvious + // signals were tried and rejected: + // - fake-client ResourceVersion: rejected empirically. The fake client's object + // tracker assigns ResourceVersion independently per object, so a freshly created + // NetworkPolicy and a freshly created Deployment both come back "1" regardless of + // which was created first - it carries no cross-object ordering information. + // - parsing the "Done reconciling component" debug log line emitted by + // handler.CreateOrUpdateOrDelete: this would work today, but the log message text + // is not a documented contract, and ReconcileAPIServer has no field to inject a + // test logger, so pinning behaviour on it would mean either regexing rendered log + // output or globally overriding the package logger for the process. The + // interceptor hooks a stable, public controller-runtime test extension point + // instead, and records the same thing the log line reports: real Create calls, in + // the order handler.CreateOrUpdateOrDelete issues them while walking the + // `components` slice built by Reconcile. + It("creates the policy before the workload and certificate-management components on the migrating pass", func() { + Expect(cli.Create(ctx, installation)).To(BeNil()) + // layoutDeprecated + available + no deny present => decisionProceed with + // moveIsPending true: this is the pass that performs the migration. + Expect(cli.Create(ctx, apiService("tigera-system", true))).NotTo(HaveOccurred()) + + wrapped, indexOf := recordCreateOrder(cli.(client.WithWatch)) + r := ReconcileAPIServer{ + client: wrapped, apiReader: wrapped, scheme: scheme, status: mockStatus, + tierWatchReady: ready, migrationWatchReady: &utils.ReadyFlag{}, + opts: options.ControllerOptions{EnterpriseCRDExists: true, DetectedProvider: operatorv1.ProviderNone}, + } + _, err := r.Reconcile(ctx, reconcile.Request{}) + Expect(err).NotTo(HaveOccurred()) + + polIdx := indexOf(&v3.NetworkPolicy{}, client.ObjectKey{Namespace: render.APIServerNamespace, Name: render.APIServerPolicyName}) + depIdx := indexOf(&appsv1.Deployment{}, client.ObjectKey{Namespace: render.APIServerNamespace, Name: render.APIServerName}) + secIdx := indexOf(&corev1.Secret{}, client.ObjectKey{Namespace: render.APIServerNamespace, Name: apiSecret.Name}) + Expect(polIdx).To(BeNumerically(">=", 0), "policy NetworkPolicy was never created") + Expect(depIdx).To(BeNumerically(">=", 0), "workload Deployment was never created") + Expect(secIdx).To(BeNumerically(">=", 0), "certificate-management Secret was never created") + + Expect(polIdx).To(BeNumerically("<", depIdx), "policy must be created before the workload on the migrating pass") + Expect(polIdx).To(BeNumerically("<", secIdx), "policy must be created before certificate-management on the migrating pass") + }) + + It("creates the policy after the workload and certificate-management components on an ordinary reconcile", func() { + Expect(cli.Create(ctx, installation)).To(BeNil()) + // No APIService at all => layoutAbsent => moveIsPending false: an ordinary pass, + // same as every ordinary install or upgrade that isn't performing the migration. + + wrapped, indexOf := recordCreateOrder(cli.(client.WithWatch)) + r := ReconcileAPIServer{ + client: wrapped, apiReader: wrapped, scheme: scheme, status: mockStatus, + tierWatchReady: ready, migrationWatchReady: &utils.ReadyFlag{}, + opts: options.ControllerOptions{EnterpriseCRDExists: true, DetectedProvider: operatorv1.ProviderNone}, + } + _, err := r.Reconcile(ctx, reconcile.Request{}) + Expect(err).NotTo(HaveOccurred()) + + polIdx := indexOf(&v3.NetworkPolicy{}, client.ObjectKey{Namespace: render.APIServerNamespace, Name: render.APIServerPolicyName}) + depIdx := indexOf(&appsv1.Deployment{}, client.ObjectKey{Namespace: render.APIServerNamespace, Name: render.APIServerName}) + secIdx := indexOf(&corev1.Secret{}, client.ObjectKey{Namespace: render.APIServerNamespace, Name: apiSecret.Name}) + Expect(polIdx).To(BeNumerically(">=", 0), "policy NetworkPolicy was never created") + Expect(depIdx).To(BeNumerically(">=", 0), "workload Deployment was never created") + Expect(secIdx).To(BeNumerically(">=", 0), "certificate-management Secret was never created") + + Expect(polIdx).To(BeNumerically(">", depIdx), "policy must be created after the workload on an ordinary reconcile") + Expect(polIdx).To(BeNumerically(">", secIdx), "policy must be created after certificate-management on an ordinary reconcile") + }) + }) + Context("verify reconciliation", func() { It("should use builtin images", func() { installation.Spec.CertificateManagement = certificateManagement @@ -165,6 +338,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -226,6 +400,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -279,6 +454,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -304,6 +480,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -326,6 +503,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -350,6 +528,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -372,6 +551,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: notReady, @@ -397,6 +577,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -424,6 +605,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -449,6 +631,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: notReady, @@ -475,6 +658,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -517,6 +701,7 @@ var _ = Describe("apiserver controller tests", func() { Expect(cli.Create(ctx, ts)).NotTo(HaveOccurred()) r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -549,6 +734,7 @@ var _ = Describe("apiserver controller tests", func() { } r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -601,6 +787,7 @@ var _ = Describe("apiserver controller tests", func() { Expect(cli.Create(ctx, ts)).NotTo(HaveOccurred()) r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -670,6 +857,7 @@ var _ = Describe("apiserver controller tests", func() { Expect(cli.Create(ctx, ts)).NotTo(HaveOccurred()) r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -774,6 +962,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -803,6 +992,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -833,6 +1023,7 @@ var _ = Describe("apiserver controller tests", func() { It("Should reconcile multi-cluster setup for a management cluster for a multiple tenant", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -884,6 +1075,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -919,6 +1111,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -963,6 +1156,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, @@ -991,6 +1185,7 @@ var _ = Describe("apiserver controller tests", func() { r := ReconcileAPIServer{ client: cli, + apiReader: cli, scheme: scheme, status: mockStatus, tierWatchReady: ready, diff --git a/pkg/controller/apiserver/migration.go b/pkg/controller/apiserver/migration.go new file mode 100644 index 0000000000..cafd1db27a --- /dev/null +++ b/pkg/controller/apiserver/migration.go @@ -0,0 +1,165 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package apiserver + +import ( + "context" + "time" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + apiregv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/tigera/operator/pkg/render" + "github.com/tigera/operator/pkg/render/common/networkpolicy" +) + +// calicoAPIServiceName is the aggregated API registration for projectcalico.org/v3. +const calicoAPIServiceName = "v3.projectcalico.org" + +// deprecatedAPIServerNamespace is where the API server ran before it was rehomed into +// calico-system. +const deprecatedAPIServerNamespace = "tigera-system" + +// apiServerLayout says which API server the aggregated projectcalico.org/v3 API is +// currently registered against. +type apiServerLayout int + +const ( + // layoutAbsent means there is no APIService yet: a fresh install. + layoutAbsent apiServerLayout = iota + // layoutDeprecated means the APIService still points into tigera-system, so the + // migration has not been performed. + layoutDeprecated + // layoutCurrent means the APIService points into calico-system. + layoutCurrent +) + +// readAPIServiceState reports which API server layout is registered and whether the +// aggregated API can currently serve requests. +// +// Both answers come from the APIService, which is an apiregistration.k8s.io object served +// by the kube-apiserver itself. Reading it therefore does not depend on the aggregated API +// being healthy, which is the whole point: it is the one place that can tell us the +// aggregated API is down. spec.service.namespace gives the layout; the Available condition +// is the kube-aggregator's own verdict on whether requests can be served. +func readAPIServiceState(ctx context.Context, reader client.Reader) (apiServerLayout, bool, error) { + as := &apiregv1.APIService{} + if err := reader.Get(ctx, client.ObjectKey{Name: calicoAPIServiceName}, as); err != nil { + if apierrors.IsNotFound(err) { + return layoutAbsent, false, nil + } + return layoutAbsent, false, err + } + + layout := layoutCurrent + if as.Spec.Service != nil && as.Spec.Service.Namespace == deprecatedAPIServerNamespace { + layout = layoutDeprecated + } + + available := false + for _, c := range as.Status.Conditions { + if c.Type == apiregv1.Available { + available = c.Status == apiregv1.ConditionTrue + break + } + } + return layout, available, nil +} + +// denyReadTimeout bounds the one read in this file that is proxied to the aggregated API +// server. The APIService read is served by the kube-apiserver itself and needs no bound, but +// this Get goes through the aggregator to the Calico API server, and the reconcile worker is +// single-threaded: an unbounded read against a backend that has just stopped answering would +// stall every other reconcile in this controller. +const denyReadTimeout = 15 * time.Second + +// deprecatedDenyPresent reports whether allow-tigera.default-deny still exists in the +// calico-system namespace. That policy sits in the earlier-evaluated allow-tigera tier and +// selects all() endpoints, so while it exists it denies the migrated calico-apiserver pod +// at end-of-tier before the calico-system tier is ever consulted. +// +// Only NotFound means "absent". A NoMatch error means the RESTMapper has no mapping for the +// kind, which is not evidence about the policy, so it is returned rather than treated as an +// absence that would license the move. +func deprecatedDenyPresent(ctx context.Context, reader client.Reader) (bool, error) { + ctx, cancel := context.WithTimeout(ctx, denyReadTimeout) + defer cancel() + + deny := networkpolicy.DeprecatedAllowTigeraNetworkPolicyObject("default-deny", render.APIServerNamespace) + if err := reader.Get(ctx, client.ObjectKeyFromObject(deny), deny); err != nil { + if apierrors.IsNotFound(err) { + return false, nil + } + return false, err + } + return true, nil +} + +// migrationDecision is what the controller should do about the API server migration on this +// pass. +type migrationDecision int + +const ( + // decisionProceed means reconcile normally. + decisionProceed migrationDecision = iota + // decisionWaitForDenyRemoval means a migration is pending and safe to make, but the + // deprecated deny that would trap the moved pod is still in place. The + // installation controller removes it through the still-serving API server. + decisionWaitForDenyRemoval + // decisionHoldAPIUnavailable means a migration is pending but the aggregated API cannot + // serve, so we can neither verify the trap is gone nor rely on anything clearing it. + // Applying the move now would repoint the aggregated API onto a pod we cannot vouch for. + decisionHoldAPIUnavailable +) + +// decideMigration decides what to do about the migration from the live state. +// +// A migration is pending only when the APIService still points into the deprecated +// namespace. Keying on that rather than on the deny's presence matters: a two-hop upgrade +// through CE 3.22 leaves the deny in place, with a selector that excludes the API server, +// after the move has already happened. Asking about the policy there would hold a migration +// that is already complete. +func decideMigration(layout apiServerLayout, aggregatedAPIAvailable, denyPresent bool) migrationDecision { + if layout != layoutDeprecated { + return decisionProceed + } + // This is a pre-filter, not a guarantee: the condition reflects a probe from up to + // ~30s ago. What actually proves the API can serve is the caller's uncached Get of the + // deprecated deny, made immediately before this function is called and only when this + // condition is true: that Get is served by the aggregated API itself, so it errors + // rather than returning a decoded answer if the API cannot serve, and the caller + // returns before decideMigration ever runs. So a stale "true" cannot make it here as a + // false denyPresent, and a stale "false" only costs an unneeded hold - it is never + // wrong to wait a bit longer. + if !aggregatedAPIAvailable { + return decisionHoldAPIUnavailable + } + if denyPresent { + return decisionWaitForDenyRemoval + } + return decisionProceed +} + +// policyComponentFirst reports whether the projectcalico.org/v3 NetworkPolicy component +// should be applied before the API server workload. +// +// It is rendered last normally, so that a fresh install is not blocked on an API server that +// cannot become available until the install has progressed. On the pass that performs the +// migration that ordering leaves the moved pod running with no policy of its own, and by +// then we have established that the aggregated API can serve, so the policy can go first. +func policyComponentFirst(moveIsPending, renderPolicy bool) bool { + return moveIsPending && renderPolicy +} diff --git a/pkg/controller/apiserver/migration_test.go b/pkg/controller/apiserver/migration_test.go new file mode 100644 index 0000000000..db4e8f67d6 --- /dev/null +++ b/pkg/controller/apiserver/migration_test.go @@ -0,0 +1,192 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package apiserver + +import ( + "context" + "errors" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + apiregv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" +) + +func apiService(ns string, available bool) *apiregv1.APIService { + st := apiregv1.ConditionFalse + if available { + st = apiregv1.ConditionTrue + } + return &apiregv1.APIService{ + ObjectMeta: metav1.ObjectMeta{Name: calicoAPIServiceName}, + Spec: apiregv1.APIServiceSpec{ + Group: "projectcalico.org", + Version: "v3", + Service: &apiregv1.ServiceReference{Name: "calico-api", Namespace: ns}, + }, + Status: apiregv1.APIServiceStatus{ + Conditions: []apiregv1.APIServiceCondition{ + {Type: apiregv1.Available, Status: st}, + }, + }, + } +} + +func newReader(objs ...client.Object) client.Client { + s := runtime.NewScheme() + Expect(apiregv1.AddToScheme(s)).NotTo(HaveOccurred()) + return fake.NewClientBuilder().WithScheme(s).WithObjects(objs...).Build() +} + +var _ = Describe("readAPIServiceState", func() { + ctx := context.Background() + + It("reports layoutAbsent when the APIService does not exist", func() { + layout, available, err := readAPIServiceState(ctx, newReader()) + Expect(err).NotTo(HaveOccurred()) + Expect(layout).To(Equal(layoutAbsent)) + Expect(available).To(BeFalse()) + }) + + It("reports the deprecated layout when the APIService points at tigera-system", func() { + layout, available, err := readAPIServiceState(ctx, newReader(apiService("tigera-system", true))) + Expect(err).NotTo(HaveOccurred()) + Expect(layout).To(Equal(layoutDeprecated)) + Expect(available).To(BeTrue()) + }) + + It("reports the current layout when the APIService points at calico-system", func() { + layout, available, err := readAPIServiceState(ctx, newReader(apiService("calico-system", true))) + Expect(err).NotTo(HaveOccurred()) + Expect(layout).To(Equal(layoutCurrent)) + Expect(available).To(BeTrue()) + }) + + It("reports unavailable when the Available condition is False", func() { + _, available, err := readAPIServiceState(ctx, newReader(apiService("tigera-system", false))) + Expect(err).NotTo(HaveOccurred()) + Expect(available).To(BeFalse()) + }) + + It("reports unavailable when there is no Available condition at all", func() { + as := apiService("tigera-system", true) + as.Status.Conditions = nil + _, available, err := readAPIServiceState(ctx, newReader(as)) + Expect(err).NotTo(HaveOccurred()) + Expect(available).To(BeFalse()) + }) +}) + +func denyPolicy() *v3.NetworkPolicy { + return &v3.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "allow-tigera.default-deny", Namespace: "calico-system"}, + Spec: v3.NetworkPolicySpec{Tier: "allow-tigera", Selector: "all()"}, + } +} + +func newV3Reader(objs ...client.Object) client.Client { + s := runtime.NewScheme() + Expect(apiregv1.AddToScheme(s)).NotTo(HaveOccurred()) + Expect(v3.AddToScheme(s)).NotTo(HaveOccurred()) + return fake.NewClientBuilder().WithScheme(s).WithObjects(objs...).Build() +} + +// errReader returns a fixed error from Get, so the no-swallowing behaviour is testable. +type errReader struct { + client.Reader + err error +} + +func (e errReader) Get(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + return e.err +} + +var _ = Describe("deprecatedDenyPresent", func() { + ctx := context.Background() + + It("reports false when the policy is absent", func() { + present, err := deprecatedDenyPresent(ctx, newV3Reader()) + Expect(err).NotTo(HaveOccurred()) + Expect(present).To(BeFalse()) + }) + + It("reports true when the policy is present", func() { + present, err := deprecatedDenyPresent(ctx, newV3Reader(denyPolicy())) + Expect(err).NotTo(HaveOccurred()) + Expect(present).To(BeTrue()) + }) + + It("returns a NoMatch error instead of treating it as absent", func() { + noMatch := &meta.NoKindMatchError{ + GroupKind: schema.GroupKind{Group: "projectcalico.org", Kind: "NetworkPolicy"}, + } + _, err := deprecatedDenyPresent(ctx, errReader{err: noMatch}) + Expect(err).To(HaveOccurred()) + Expect(meta.IsNoMatchError(err)).To(BeTrue()) + }) + + It("returns any other error", func() { + _, err := deprecatedDenyPresent(ctx, errReader{err: errors.New("boom")}) + Expect(err).To(MatchError("boom")) + }) +}) + +var _ = Describe("decideMigration", func() { + It("proceeds on a fresh install", func() { + Expect(decideMigration(layoutAbsent, false, false)).To(Equal(decisionProceed)) + }) + + It("proceeds once the APIService already points at calico-system, even if the deny lingers", func() { + // This is the state a two-hop upgrade through CE 3.22 leaves behind: the namespace + // is gone and allow-tigera.default-deny is still present, excluding the API server + // by selector. There is no move left to make, so there is nothing to wait for. + Expect(decideMigration(layoutCurrent, true, true)).To(Equal(decisionProceed)) + }) + + It("holds without acting when a migration is pending and the aggregated API cannot serve", func() { + Expect(decideMigration(layoutDeprecated, false, true)).To(Equal(decisionHoldAPIUnavailable)) + Expect(decideMigration(layoutDeprecated, false, false)).To(Equal(decisionHoldAPIUnavailable)) + }) + + It("waits for the deny to be removed when a migration is pending and the API can serve", func() { + Expect(decideMigration(layoutDeprecated, true, true)).To(Equal(decisionWaitForDenyRemoval)) + }) + + It("proceeds when a migration is pending, the API can serve and the deny is gone", func() { + Expect(decideMigration(layoutDeprecated, true, false)).To(Equal(decisionProceed)) + }) +}) + +var _ = Describe("policyComponentFirst", func() { + It("is false when the policy is not being rendered at all", func() { + Expect(policyComponentFirst(true, false)).To(BeFalse()) + }) + + It("is false on an ordinary reconcile with no migration pending", func() { + Expect(policyComponentFirst(false, true)).To(BeFalse()) + }) + + It("is true on the pass that performs the migration", func() { + Expect(policyComponentFirst(true, true)).To(BeTrue()) + }) +})