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()) + }) +})