From 93fafdc2fd6775083aaa4beb79f40f5202901eb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20B=C3=A4hler?= <26610571+oliverbaehler@users.noreply.github.com> Date: Tue, 11 Aug 2026 12:03:16 +0200 Subject: [PATCH] fix: do not mutate on update and bound pvcs (#2073) * fix: do not mutate on update and bound pvcs Signed-off-by: Oliver Baehler * fix: do not mutate on update and bound pvcs Signed-off-by: Oliver Baehler --------- Signed-off-by: Oliver Baehler --- charts/capsule/templates/configuration.yaml | 10 + internal/controllers/tenant/manager.go | 11 +- .../tenant/resourcequota_watch_test.go | 2 +- internal/controllers/tenant/resourcequotas.go | 264 ++++++++++++++---- .../tenant/resourcequotas_cleanup_test.go | 233 ++++++++++++++++ internal/webhook/pvc/handler.go | 12 + internal/webhook/pvc/handler_test.go | 132 +++++++++ internal/webhook/pvc/pvc_mutating_volume.go | 4 + internal/webhook/pvc/pvc_validating_volume.go | 4 + 9 files changed, 614 insertions(+), 58 deletions(-) create mode 100644 internal/controllers/tenant/resourcequotas_cleanup_test.go diff --git a/charts/capsule/templates/configuration.yaml b/charts/capsule/templates/configuration.yaml index 297cc28d..c8874b04 100644 --- a/charts/capsule/templates/configuration.yaml +++ b/charts/capsule/templates/configuration.yaml @@ -539,6 +539,11 @@ spec: {{- with $.Values.webhooks.matchConditions }} {{- toYaml . | nindent 10 }} {{- end }} + - name: skip-bound-pvc-updates + expression: > + request.operation != "UPDATE" || + !has(oldObject.status.phase) || + oldObject.status.phase != "Bound" - name: requires-pvc-spec-validation expression: > request.operation != "UPDATE" || @@ -1238,6 +1243,11 @@ spec: {{- with $.Values.webhooks.matchConditions }} {{- toYaml . | nindent 10 }} {{- end }} + - name: skip-bound-pvc-updates + expression: > + request.operation != "UPDATE" || + !has(oldObject.status.phase) || + oldObject.status.phase != "Bound" - name: requires-pvc-spec-validation expression: > request.operation != "UPDATE" || diff --git a/internal/controllers/tenant/manager.go b/internal/controllers/tenant/manager.go index 15191148..457210be 100644 --- a/internal/controllers/tenant/manager.go +++ b/internal/controllers/tenant/manager.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "slices" + "sync" "time" "github.com/go-logr/logr" @@ -64,6 +65,9 @@ type Manager struct { classes supportedClasses discoveryCache cache.DiscoveryNamespacedResourceCache + + resourceQuotaSyncMu sync.Mutex + resourceQuotaSyncs map[string]*tenantResourceQuotaSync } type supportedClasses struct { @@ -438,8 +442,13 @@ func (r *Manager) syncResourceQuotasForResourceQuota(ctx context.Context, quota return } + reader := r.reader + if reader == nil { + reader = r.Client + } + tenant := &capsulev1beta2.Tenant{} - if err := r.Get(ctx, client.ObjectKey{Name: owner.Name}, tenant); err != nil { + if err := reader.Get(ctx, client.ObjectKey{Name: owner.Name}, tenant); err != nil { if !apierrors.IsNotFound(err) { r.Log.Error(err, "cannot retrieve Tenant for ResourceQuota sync", "tenant", owner.Name) } diff --git a/internal/controllers/tenant/resourcequota_watch_test.go b/internal/controllers/tenant/resourcequota_watch_test.go index 0ee7bc0d..66e3cac9 100644 --- a/internal/controllers/tenant/resourcequota_watch_test.go +++ b/internal/controllers/tenant/resourcequota_watch_test.go @@ -71,7 +71,7 @@ func TestResourceQuotaWatchSyncsOnlyTheOwnerTenantQuotas(t *testing.T) { Object: map[string]any{"metadata": map[string]any{"name": "widget"}}, }}}, nil }) - manager := &Manager{Client: cl, DynamicClient: dynamicClient, Metrics: metrics.NewTenantRecorder()} + manager := &Manager{Client: cl, reader: cl, DynamicClient: dynamicClient, Metrics: metrics.NewTenantRecorder()} manager.syncResourceQuotasForResourceQuota(context.Background(), trigger) diff --git a/internal/controllers/tenant/resourcequotas.go b/internal/controllers/tenant/resourcequotas.go index 309fb08a..3b885b4d 100644 --- a/internal/controllers/tenant/resourcequotas.go +++ b/internal/controllers/tenant/resourcequotas.go @@ -10,6 +10,7 @@ import ( "maps" "strconv" "strings" + "sync" "github.com/go-logr/logr" "golang.org/x/sync/errgroup" @@ -45,18 +46,92 @@ import ( // // In case of Namespace-scoped Resource Budget, we're just replicating the resources across all registered Namespaces. -func (r *Manager) syncResourceQuotas(ctx context.Context, log logr.Logger, tenant *capsulev1beta2.Tenant) (err error) { //nolint:gocognit - if err := r.runGarbageCollection(ctx, tenant, &corev1.ResourceQuota{}); err != nil { - return err +type tenantResourceQuotaSync struct { + mutex sync.Mutex + refs int +} + +func (r *Manager) syncResourceQuotas(ctx context.Context, log logr.Logger, tenant *capsulev1beta2.Tenant) error { + return r.withTenantResourceQuotaSync(tenant.Name, func() error { + latest, err := r.latestResourceQuotaTenant(ctx, tenant) + if err != nil || latest == nil { + return err + } + + return r.syncResourceQuotasLocked(ctx, log, latest) + }) +} + +func (r *Manager) latestResourceQuotaTenant( + ctx context.Context, + tenant *capsulev1beta2.Tenant, +) (*capsulev1beta2.Tenant, error) { + reader := r.reader + if reader == nil { + reader = r.Client } - // Remove prior metrics, to avoid cleaning up for metrics of deleted ResourceQuotas - r.Metrics.DeleteTenantResourceMetrics(tenant.Name) - // Expose the namespace quota and usage as metrics for the tenant - r.Metrics.TenantResourceUsageGauge.WithLabelValues(tenant.Name, "namespaces", "").Set(float64(tenant.Status.Size)) + latest := &capsulev1beta2.Tenant{} + if err := reader.Get(ctx, client.ObjectKey{Name: tenant.Name}, latest); err != nil { + if apierrors.IsNotFound(err) { + r.Metrics.DeleteTenantResourceMetrics(tenant.Name) - if tenant.Spec.NamespaceOptions != nil && tenant.Spec.NamespaceOptions.Quota != nil { - r.Metrics.TenantResourceLimitGauge.WithLabelValues(tenant.Name, "namespaces", "").Set(float64(*tenant.Spec.NamespaceOptions.Quota)) + return nil, nil + } + + return nil, err + } + + if latest.DeletionTimestamp != nil { + r.Metrics.DeleteTenantResourceMetrics(tenant.Name) + + return nil, nil + } + + // Keep the status reconciled in the current Tenant pass, but always use the + // latest persisted quota-related spec. This prevents an older, slower pass + // from restoring quota objects or metrics after a newer spec removed them. + snapshot := tenant.DeepCopy() + snapshot.Spec.ResourceQuota = *latest.Spec.ResourceQuota.DeepCopy() + snapshot.Spec.NamespaceOptions = latest.Spec.NamespaceOptions.DeepCopy() + + return snapshot, nil +} + +func (r *Manager) withTenantResourceQuotaSync(tenant string, syncFn func() error) error { + r.resourceQuotaSyncMu.Lock() + if r.resourceQuotaSyncs == nil { + r.resourceQuotaSyncs = make(map[string]*tenantResourceQuotaSync) + } + + lock := r.resourceQuotaSyncs[tenant] + if lock == nil { + lock = &tenantResourceQuotaSync{} + r.resourceQuotaSyncs[tenant] = lock + } + + lock.refs++ + r.resourceQuotaSyncMu.Unlock() + + lock.mutex.Lock() + defer func() { + lock.mutex.Unlock() + + r.resourceQuotaSyncMu.Lock() + + lock.refs-- + if lock.refs == 0 { + delete(r.resourceQuotaSyncs, tenant) + } + r.resourceQuotaSyncMu.Unlock() + }() + + return syncFn() +} + +func (r *Manager) syncResourceQuotasLocked(ctx context.Context, log logr.Logger, tenant *capsulev1beta2.Tenant) (err error) { //nolint:gocognit + if err := r.prepareResourceQuotaSync(ctx, tenant); err != nil { + return err } //nolint:nestif @@ -102,6 +177,17 @@ func (r *Manager) syncResourceQuotas(ctx context.Context, log logr.Logger, tenan return scopeErr } + // Prune removed hard resources independently of their current usage. + // Previously this only happened in the under-quota branch, leaving + // stale resources behind when a remaining quota was at or over limit. + for item := range list.Items { + for name := range list.Items[item].Spec.Hard { + if !toKeep.Has(name) { + delete(list.Items[item].Spec.Hard, name) + } + } + } + // Iterating over all the options declared for the ResourceQuota, // summing all the used quota across different Namespaces to determinate // if we're hitting a Hard quota at Tenant level. @@ -168,12 +254,6 @@ func (r *Manager) syncResourceQuotas(ctx context.Context, log logr.Logger, tenan newHard.Add(list.Items[item].Status.Used[name]) // add back usage in current ns list.Items[item].Spec.Hard[name] = newHard - - for k := range list.Items[item].Spec.Hard { - if !toKeep.Has(k) { - delete(list.Items[item].Spec.Hard, k) - } - } } } @@ -182,6 +262,15 @@ func (r *Manager) syncResourceQuotas(ctx context.Context, log logr.Logger, tenan } } + // An empty hard map has no resource iteration above, but existing + // ResourceQuotas still need their old hard values and annotations + // removed. + if len(resourceQuota.Hard) == 0 { + if scopeErr = r.resourceQuotasPrune(ctx, toKeep, list.Items...); scopeErr != nil { + return scopeErr + } + } + return nil }) } @@ -200,29 +289,60 @@ func (r *Manager) syncResourceQuotas(ctx context.Context, log logr.Logger, tenan } } - // getting requested ResourceQuota keys - keys := make([]string, 0, len(tenant.Spec.ResourceQuota.Items)) + return runForTenantNamespaces(ctx, tenant, func(ctx context.Context, namespace string) error { + return r.syncResourceQuota(ctx, log, tenant, namespace) + }) +} +func (r *Manager) prepareResourceQuotaSync( + ctx context.Context, + tenant *capsulev1beta2.Tenant, +) error { + // Metrics are derived from the desired Tenant spec. Clear the previous label + // set before any API work so a cleanup error cannot leave removed quota + // entries exported indefinitely. + r.Metrics.DeleteTenantResourceMetrics(tenant.Name) + + if err := r.runGarbageCollection(ctx, tenant, &corev1.ResourceQuota{}); err != nil { + return err + } + + // Expose the namespace quota and usage as metrics for the tenant + r.Metrics.TenantResourceUsageGauge.WithLabelValues(tenant.Name, "namespaces", "").Set(float64(tenant.Status.Size)) + + if tenant.Spec.NamespaceOptions != nil && tenant.Spec.NamespaceOptions.Quota != nil { + r.Metrics.TenantResourceLimitGauge.WithLabelValues(tenant.Name, "namespaces", "").Set(float64(*tenant.Spec.NamespaceOptions.Quota)) + } + + // Prune removed quota items from every namespace still assigned to the + // Tenant, including namespaces whose Ready condition is currently false. + // Readiness must not prevent deletion of obsolete enforcement resources. + keys := make([]string, 0, len(tenant.Spec.ResourceQuota.Items)) for i := range tenant.Spec.ResourceQuota.Items { keys = append(keys, strconv.Itoa(i)) } - return runForTenantNamespaces(ctx, tenant, func(ctx context.Context, namespace string) error { - return r.syncResourceQuota(ctx, log, tenant, namespace, keys) - }) + namespaces := make([]string, 0, len(tenant.Status.Spaces)) + for _, namespace := range tenant.Status.Spaces { + namespaces = append(namespaces, namespace.Name) + } + + if err := runForNamespaces(ctx, namespaces, func(ctx context.Context, namespace string) error { + return r.pruningResources(ctx, namespace, keys, &corev1.ResourceQuota{}) + }); err != nil { + return err + } + + return nil } -func (r *Manager) syncResourceQuota(ctx context.Context, log logr.Logger, tenant *capsulev1beta2.Tenant, namespace string, keys []string) (err error) { +func (r *Manager) syncResourceQuota(ctx context.Context, log logr.Logger, tenant *capsulev1beta2.Tenant, namespace string) (err error) { // getting ResourceQuota labels for the mutateFn var typeLabel string if typeLabel, err = utils.GetTypeLabel(&corev1.ResourceQuota{}); err != nil { return err } - // Pruning resource of non-requested resources - if err = r.pruningResources(ctx, namespace, keys, &corev1.ResourceQuota{}); err != nil { - return err - } for index, resQuota := range tenant.Spec.ResourceQuota.Items { target := &corev1.ResourceQuota{ @@ -294,6 +414,25 @@ func (r *Manager) resourceQuotasUpdate( toKeep sets.Set[corev1.ResourceName], limit resource.Quantity, list ...corev1.ResourceQuota, +) (err error) { + return r.persistResourceQuotaState(ctx, &resourceName, actual, toKeep, limit, list...) +} + +func (r *Manager) resourceQuotasPrune( + ctx context.Context, + toKeep sets.Set[corev1.ResourceName], + list ...corev1.ResourceQuota, +) error { + return r.persistResourceQuotaState(ctx, nil, resource.Quantity{}, toKeep, resource.Quantity{}, list...) +} + +func (r *Manager) persistResourceQuotaState( + ctx context.Context, + resourceName *corev1.ResourceName, + actual resource.Quantity, + toKeep sets.Set[corev1.ResourceName], + limit resource.Quantity, + list ...corev1.ResourceQuota, ) (err error) { group := new(errgroup.Group) @@ -329,37 +468,7 @@ func (r *Manager) resourceQuotasUpdate( before := found.DeepCopy() - // Ensuring annotation map is there to avoid uninitialized map error and - // assigning the overall usage - if found.Annotations == nil { - found.Annotations = make(map[string]string) - } - - // Pruning the Capsule quota annotations: - // if the ResourceQuota is updated by removing some objects, - // we could still have left-overs which could be misleading. - for k := range found.Annotations { - if (strings.HasPrefix(k, capsulev1beta2.HardCapsuleQuotaAnnotation) || - strings.HasPrefix(k, capsulev1beta2.UsedCapsuleQuotaAnnotation)) && - (annotationsToKeep == nil || !annotationsToKeep.Has(k)) { - delete(found.Annotations, k) - } - } - - found.Labels = maps.Clone(rq.Labels) - if actualKey, keyErr := capsulev1beta2.UsedQuotaFor(resourceName); keyErr == nil { - found.Annotations[actualKey] = actual.String() - } - - if limitKey, keyErr := capsulev1beta2.HardQuotaFor(resourceName); keyErr == nil { - found.Annotations[limitKey] = limit.String() - } - - if rq.Spec.Hard != nil { - found.Spec.Hard = rq.Spec.Hard.DeepCopy() - } else { - found.Spec.Hard = nil - } + applyResourceQuotaState(found, &rq, resourceName, actual, limit, annotationsToKeep) if apiequality.Semantic.DeepEqual(before, found) { return nil @@ -376,3 +485,46 @@ func (r *Manager) resourceQuotasUpdate( return err } + +func applyResourceQuotaState( + found *corev1.ResourceQuota, + desired *corev1.ResourceQuota, + resourceName *corev1.ResourceName, + actual resource.Quantity, + limit resource.Quantity, + annotationsToKeep sets.Set[string], +) { + if found.Annotations == nil { + found.Annotations = make(map[string]string) + } + + // Remove quota annotations for resources no longer present in the Tenant + // spec before writing the current resource values. + for key := range found.Annotations { + quotaAnnotation := strings.HasPrefix(key, capsulev1beta2.HardCapsuleQuotaAnnotation) || + strings.HasPrefix(key, capsulev1beta2.UsedCapsuleQuotaAnnotation) + if quotaAnnotation && !annotationsToKeep.Has(key) { + delete(found.Annotations, key) + } + } + + found.Labels = maps.Clone(desired.Labels) + + if resourceName != nil { + if actualKey, err := capsulev1beta2.UsedQuotaFor(*resourceName); err == nil { + found.Annotations[actualKey] = actual.String() + } + + if limitKey, err := capsulev1beta2.HardQuotaFor(*resourceName); err == nil { + found.Annotations[limitKey] = limit.String() + } + } + + if desired.Spec.Hard == nil { + found.Spec.Hard = nil + + return + } + + found.Spec.Hard = desired.Spec.Hard.DeepCopy() +} diff --git a/internal/controllers/tenant/resourcequotas_cleanup_test.go b/internal/controllers/tenant/resourcequotas_cleanup_test.go new file mode 100644 index 00000000..80e16b7a --- /dev/null +++ b/internal/controllers/tenant/resourcequotas_cleanup_test.go @@ -0,0 +1,233 @@ +// Copyright 2020-2026 Project Capsule Authors +// SPDX-License-Identifier: Apache-2.0 + +package tenant + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + capsulev1beta2 "github.com/projectcapsule/capsule/api/v1beta2" + "github.com/projectcapsule/capsule/internal/metrics" + "github.com/projectcapsule/capsule/pkg/api" + capsulemeta "github.com/projectcapsule/capsule/pkg/api/meta" +) + +func TestSyncResourceQuotasPrunesRemovedItemsFromUnreadyNamespaces(t *testing.T) { + t.Parallel() + + tenant := quotaCleanupTenant([]corev1.ResourceQuotaSpec{{Hard: corev1.ResourceList{ + corev1.ResourceLimitsCPU: resource.MustParse("1"), + }}}) + tenant.Spec.ResourceQuota.Scope = api.ResourceQuotaScopeNamespace + tenant.Status.Spaces[0].Conditions = capsulemeta.ConditionList{{ + Type: capsulemeta.ReadyCondition, + Status: metav1.ConditionFalse, + }} + + keep := managedResourceQuota(tenant.Name, "team-a", "0", corev1.ResourceList{ + corev1.ResourceLimitsCPU: resource.MustParse("1"), + }) + remove := managedResourceQuota(tenant.Name, "team-a", "1", corev1.ResourceList{ + corev1.ResourceLimitsMemory: resource.MustParse("1Gi"), + }) + manager := quotaCleanupManager(t, tenant, keep, remove) + manager.Metrics.TenantResourceUsageGauge.WithLabelValues(tenant.Name, corev1.ResourceLimitsMemory.String(), "1").Set(1) + manager.Metrics.TenantResourceLimitGauge.WithLabelValues(tenant.Name, corev1.ResourceLimitsMemory.String(), "1").Set(1) + + if err := manager.syncResourceQuotas(context.Background(), logr.Discard(), tenant); err != nil { + t.Fatalf("syncResourceQuotas() unexpected error: %v", err) + } + + deleted := &corev1.ResourceQuota{} + err := manager.Get(context.Background(), client.ObjectKeyFromObject(remove), deleted) + if !apierrors.IsNotFound(err) { + t.Fatalf("removed ResourceQuota lookup error = %v, want NotFound", err) + } + assertQuotaMetricAbsent(t, manager.Metrics, tenant.Name, corev1.ResourceLimitsMemory, "1") +} + +func TestSyncResourceQuotasUsesLatestTenantSpecForCleanup(t *testing.T) { + t.Parallel() + + latest := quotaCleanupTenant([]corev1.ResourceQuotaSpec{{Hard: corev1.ResourceList{ + corev1.ResourceLimitsCPU: resource.MustParse("1"), + }}}) + latest.Spec.ResourceQuota.Scope = api.ResourceQuotaScopeNamespace + + stale := latest.DeepCopy() + stale.Spec.ResourceQuota.Items = append(stale.Spec.ResourceQuota.Items, corev1.ResourceQuotaSpec{ + Hard: corev1.ResourceList{corev1.ResourceLimitsMemory: resource.MustParse("1Gi")}, + }) + + keep := managedResourceQuota(latest.Name, "team-a", "0", corev1.ResourceList{ + corev1.ResourceLimitsCPU: resource.MustParse("1"), + }) + remove := managedResourceQuota(latest.Name, "team-a", "1", corev1.ResourceList{ + corev1.ResourceLimitsMemory: resource.MustParse("1Gi"), + }) + manager := quotaCleanupManager(t, latest, keep, remove) + manager.Metrics.TenantResourceUsageGauge.WithLabelValues(latest.Name, corev1.ResourceLimitsMemory.String(), "1").Set(1) + manager.Metrics.TenantResourceLimitGauge.WithLabelValues(latest.Name, corev1.ResourceLimitsMemory.String(), "1").Set(1) + + if err := manager.syncResourceQuotas(context.Background(), logr.Discard(), stale); err != nil { + t.Fatalf("syncResourceQuotas() unexpected error: %v", err) + } + + deleted := &corev1.ResourceQuota{} + err := manager.Get(context.Background(), client.ObjectKeyFromObject(remove), deleted) + if !apierrors.IsNotFound(err) { + t.Fatalf("ResourceQuota from stale Tenant spec lookup error = %v, want NotFound", err) + } + assertQuotaMetricAbsent(t, manager.Metrics, latest.Name, corev1.ResourceLimitsMemory, "1") +} + +func TestSyncResourceQuotasPrunesRemovedHardResources(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + desiredHard corev1.ResourceList + }{ + { + name: "remaining resource is at its limit", + desiredHard: corev1.ResourceList{ + corev1.ResourceLimitsCPU: resource.MustParse("1"), + }, + }, + { + name: "hard map is empty", + desiredHard: corev1.ResourceList{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + tenant := quotaCleanupTenant([]corev1.ResourceQuotaSpec{{Hard: tt.desiredHard}}) + quota := managedResourceQuota(tenant.Name, "team-a", "0", corev1.ResourceList{ + corev1.ResourceLimitsCPU: resource.MustParse("1"), + corev1.ResourceLimitsMemory: resource.MustParse("1Gi"), + }) + quota.Status.Used = corev1.ResourceList{ + corev1.ResourceLimitsCPU: resource.MustParse("1"), + } + usedMemory, err := capsulev1beta2.UsedQuotaFor(corev1.ResourceLimitsMemory) + if err != nil { + t.Fatalf("build used-memory annotation: %v", err) + } + hardMemory, err := capsulev1beta2.HardQuotaFor(corev1.ResourceLimitsMemory) + if err != nil { + t.Fatalf("build hard-memory annotation: %v", err) + } + quota.Annotations = map[string]string{usedMemory: "1Gi", hardMemory: "1Gi"} + + manager := quotaCleanupManager(t, tenant, quota) + manager.Metrics.TenantResourceUsageGauge.WithLabelValues(tenant.Name, corev1.ResourceLimitsMemory.String(), "0").Set(1) + manager.Metrics.TenantResourceLimitGauge.WithLabelValues(tenant.Name, corev1.ResourceLimitsMemory.String(), "0").Set(1) + + if err := manager.syncResourceQuotas(context.Background(), logr.Discard(), tenant); err != nil { + t.Fatalf("syncResourceQuotas() unexpected error: %v", err) + } + + updated := &corev1.ResourceQuota{} + if err := manager.Get(context.Background(), client.ObjectKeyFromObject(quota), updated); err != nil { + t.Fatalf("get updated ResourceQuota: %v", err) + } + if _, ok := updated.Spec.Hard[corev1.ResourceLimitsMemory]; ok { + t.Fatalf("removed memory hard quota is still present: %#v", updated.Spec.Hard) + } + if _, ok := updated.Annotations[usedMemory]; ok { + t.Fatalf("removed memory usage annotation is still present: %#v", updated.Annotations) + } + if _, ok := updated.Annotations[hardMemory]; ok { + t.Fatalf("removed memory hard annotation is still present: %#v", updated.Annotations) + } + assertQuotaMetricAbsent(t, manager.Metrics, tenant.Name, corev1.ResourceLimitsMemory, "0") + }) + } +} + +func quotaCleanupTenant(items []corev1.ResourceQuotaSpec) *capsulev1beta2.Tenant { + return &capsulev1beta2.Tenant{ + ObjectMeta: metav1.ObjectMeta{Name: "tenant-a"}, + Spec: capsulev1beta2.TenantSpec{ResourceQuota: api.ResourceQuotaSpec{ + Scope: api.ResourceQuotaScopeTenant, + Items: items, + }}, + Status: capsulev1beta2.TenantStatus{ + Size: 1, + Spaces: []*capsulev1beta2.TenantStatusNamespaceItem{{ + Name: "team-a", + }}, + }, + } +} + +func managedResourceQuota( + tenant string, + namespace string, + index string, + hard corev1.ResourceList, +) *corev1.ResourceQuota { + return &corev1.ResourceQuota{ + ObjectMeta: metav1.ObjectMeta{ + Name: "capsule-" + tenant + "-" + index, + Namespace: namespace, + Labels: map[string]string{ + capsulemeta.NewTenantLabel: tenant, + capsulemeta.NewManagedByCapsuleLabel: capsulemeta.ValueController, + capsulemeta.ResourceQuotaLabel: index, + }, + }, + Spec: corev1.ResourceQuotaSpec{Hard: hard}, + } +} + +func quotaCleanupManager(t *testing.T, objects ...client.Object) *Manager { + t.Helper() + + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("add core scheme: %v", err) + } + if err := capsulev1beta2.AddToScheme(scheme); err != nil { + t.Fatalf("add Capsule scheme: %v", err) + } + + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + + return &Manager{ + Client: cl, + reader: cl, + Metrics: metrics.NewTenantRecorder(), + Log: logr.Discard(), + } +} + +func assertQuotaMetricAbsent( + t *testing.T, + recorder *metrics.TenantRecorder, + tenant string, + resourceName corev1.ResourceName, + index string, +) { + t.Helper() + + if recorder.TenantResourceUsageGauge.DeleteLabelValues(tenant, resourceName.String(), index) { + t.Fatalf("stale usage metric still exists for %s quota %s", resourceName, index) + } + if recorder.TenantResourceLimitGauge.DeleteLabelValues(tenant, resourceName.String(), index) { + t.Fatalf("stale limit metric still exists for %s quota %s", resourceName, index) + } +} diff --git a/internal/webhook/pvc/handler.go b/internal/webhook/pvc/handler.go index dd353e04..25f3926f 100644 --- a/internal/webhook/pvc/handler.go +++ b/internal/webhook/pvc/handler.go @@ -51,6 +51,14 @@ func requiresPVCSpecValidation( pvc *corev1.PersistentVolumeClaim, oldPVC *corev1.PersistentVolumeClaim, ) bool { + // A bound PVC's volume binding fields, including its selector, are immutable. + // Reapplying the tenant selector during an update would make otherwise valid + // metadata or resize updates fail for claims created before Capsule enforced it. + if req.Operation == admissionv1.Update && + isBoundPVC(oldPVC) { + return false + } + // Finalizer cleanup must remain possible after a bound PV has disappeared. // Continue validating any update that changes the PVC spec. if req.Operation != admissionv1.Update || @@ -62,3 +70,7 @@ func requiresPVCSpecValidation( return !apiequality.Semantic.DeepEqual(pvc.Spec, oldPVC.Spec) } + +func isBoundPVC(pvc *corev1.PersistentVolumeClaim) bool { + return pvc != nil && pvc.Status.Phase == corev1.ClaimBound +} diff --git a/internal/webhook/pvc/handler_test.go b/internal/webhook/pvc/handler_test.go index a4face83..288bf077 100644 --- a/internal/webhook/pvc/handler_test.go +++ b/internal/webhook/pvc/handler_test.go @@ -10,11 +10,15 @@ import ( admissionv1 "k8s.io/api/admission/v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + capsulev1beta2 "github.com/projectcapsule/capsule/api/v1beta2" + "github.com/projectcapsule/capsule/pkg/runtime/handlers" ) func TestMutatingHandlerSkipsDynamicClaimsBeforeTenantLookup(t *testing.T) { @@ -78,6 +82,114 @@ func TestValidatingHandlerSkipsTerminatingClaimWithUnchangedSpec(t *testing.T) { } } +func TestHandlersSkipBoundClaimsBeforeTenantLookup(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + + oldPVC := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "nginx-logs"}, + Spec: corev1.PersistentVolumeClaimSpec{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{ + "velero.io/dynamic-pv-restore": "test.nginx-logs.sg75p", + }}, + VolumeName: "pvc-f9bc7e8d", + }, + Status: corev1.PersistentVolumeClaimStatus{Phase: corev1.ClaimBound}, + } + newPVC := oldPVC.DeepCopy() + newPVC.Labels = map[string]string{"test": "test"} + + tests := []struct { + name string + handler handlers.Handler + }{ + { + name: "mutating", + handler: MutatingHandler(PersistentVolumeMutatingVolume()), + }, + { + name: "validating", + handler: Handler(PersistentVolumeValidatingVolume()), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + base := fake.NewClientBuilder().WithScheme(scheme).Build() + reader := &pvcCountingReader{Reader: base} + request := pvcUpdateAdmissionRequest(t, oldPVC, newPVC) + + if response := tt.handler.OnUpdate(nil, reader, admission.NewDecoder(scheme), nil)( + context.Background(), + request, + ); response != nil { + t.Fatalf("response = %#v, want nil", response) + } + + if reader.gets != 0 { + t.Fatalf("tenant lookup gets = %d, want 0 for a bound claim", reader.gets) + } + }) + } +} + +func TestVolumeHooksSkipBoundClaimUpdates(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + + oldPVC := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "nginx-logs"}, + Spec: corev1.PersistentVolumeClaimSpec{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{ + "velero.io/dynamic-pv-restore": "test.nginx-logs.sg75p", + }}, + VolumeName: "pvc-f9bc7e8d", + }, + Status: corev1.PersistentVolumeClaimStatus{Phase: corev1.ClaimBound}, + } + newPVC := oldPVC.DeepCopy() + newPVC.Labels = map[string]string{"test": "test"} + tnt := &capsulev1beta2.Tenant{ObjectMeta: metav1.ObjectMeta{Name: "test"}} + request := pvcUpdateAdmissionRequest(t, oldPVC, newPVC) + decoder := admission.NewDecoder(scheme) + + if response := PersistentVolumeMutatingVolume().OnUpdate( + nil, + nil, + oldPVC, + newPVC, + decoder, + nil, + tnt, + )(context.Background(), request); response != nil { + t.Fatalf("mutating response = %#v, want nil", response) + } + + if len(newPVC.Spec.Selector.MatchExpressions) != 0 { + t.Fatalf("selector expressions = %#v, want unchanged", newPVC.Spec.Selector.MatchExpressions) + } + + if response := PersistentVolumeValidatingVolume().OnUpdate( + nil, + nil, + oldPVC, + newPVC, + decoder, + nil, + tnt, + )(context.Background(), request); response != nil { + t.Fatalf("validating response = %#v, want nil", response) + } +} + func TestRequiresPVCSpecValidation(t *testing.T) { t.Parallel() @@ -90,6 +202,12 @@ func TestRequiresPVCSpecValidation(t *testing.T) { changed.Spec.VolumeName = "salusa" active := terminating.DeepCopy() active.DeletionTimestamp = nil + bound := active.DeepCopy() + bound.Status.Phase = corev1.ClaimBound + resizedBound := bound.DeepCopy() + resizedBound.Spec.Resources.Requests = corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("2Gi"), + } tests := []struct { name string @@ -111,6 +229,20 @@ func TestRequiresPVCSpecValidation(t *testing.T) { pvc: active, want: true, }, + { + name: "bound metadata update", + operation: admissionv1.Update, + oldPVC: bound.DeepCopy(), + pvc: bound, + want: false, + }, + { + name: "bound resize", + operation: admissionv1.Update, + oldPVC: bound.DeepCopy(), + pvc: resizedBound, + want: false, + }, { name: "terminating finalizer update", operation: admissionv1.Update, diff --git a/internal/webhook/pvc/pvc_mutating_volume.go b/internal/webhook/pvc/pvc_mutating_volume.go index 0b203004..92e31e3b 100644 --- a/internal/webhook/pvc/pvc_mutating_volume.go +++ b/internal/webhook/pvc/pvc_mutating_volume.go @@ -67,6 +67,10 @@ func (h persistentVolumeMutatingVolume) OnUpdate( tnt *capsulev1beta2.Tenant, ) handlers.Func { return func(ctx context.Context, req admission.Request) *admission.Response { + if isBoundPVC(oldPVC) { + return nil + } + if newPVC == nil || tnt == nil { return nil } diff --git a/internal/webhook/pvc/pvc_validating_volume.go b/internal/webhook/pvc/pvc_validating_volume.go index 8c95c0df..954157f3 100644 --- a/internal/webhook/pvc/pvc_validating_volume.go +++ b/internal/webhook/pvc/pvc_validating_volume.go @@ -55,6 +55,10 @@ func (h persistentVolumeValidatingVolume) OnUpdate( tnt *capsulev1beta2.Tenant, ) handlers.Func { return func(ctx context.Context, req admission.Request) *admission.Response { + if isBoundPVC(oldPVC) { + return nil + } + if err := validatePVCSelector(newPVC, tnt); err != nil { return ad.ErroredResponse(err) }