From c7d6ee1263a20135dd7e1abb34f10cc9f6fc7fa2 Mon Sep 17 00:00:00 2001 From: Blaine Gardner Date: Wed, 29 Oct 2025 09:50:10 -0600 Subject: [PATCH] add initial BucketAccess controller reconciliation Add initial implementation for BucketAccess reconciliation based on v1alpha2 KEP. This initial implementation covers only the first section of Controller reconciliation for a new BucketAccess. Coverage ends at the point where reconciliation is handed off to the Sidecar. Signed-off-by: Blaine Gardner --- .../v1alpha2/bucketaccess_types.go | 8 +- .../objectstorage/v1alpha2/definitions.go | 24 + .../objectstorage.k8s.io_bucketaccesses.yaml | 18 +- .../internal/reconciler/bucketaccess.go | 430 ++++++++- .../internal/reconciler/bucketaccess_test.go | 883 ++++++++++++++++++ controller/internal/reconciler/bucketclaim.go | 2 +- docs/src/api/out.md | 5 +- internal/handoff/handoff.go | 68 ++ internal/handoff/handoff_test.go | 154 +++ internal/predicate/predicate.go | 91 ++ internal/predicate/predicate_test.go | 95 ++ sidecar/internal/reconciler/bucket.go | 2 +- .../v1alpha2/bucketaccess_types.go | 8 +- .../objectstorage/v1alpha2/definitions.go | 24 + 14 files changed, 1778 insertions(+), 34 deletions(-) create mode 100644 controller/internal/reconciler/bucketaccess_test.go create mode 100644 internal/handoff/handoff.go create mode 100644 internal/handoff/handoff_test.go create mode 100644 internal/predicate/predicate_test.go diff --git a/client/apis/objectstorage/v1alpha2/bucketaccess_types.go b/client/apis/objectstorage/v1alpha2/bucketaccess_types.go index b8ba2670..9a38c277 100644 --- a/client/apis/objectstorage/v1alpha2/bucketaccess_types.go +++ b/client/apis/objectstorage/v1alpha2/bucketaccess_types.go @@ -167,7 +167,7 @@ type BucketClaimAccess struct { AccessSecretName string `json:"accessSecretName"` } -// AccessedBucket identifies a Bucket and corresponding access parameters. +// AccessedBucket identifies a Bucket and correlates it to a BucketClaimAccess from the spec. type AccessedBucket struct { // bucketName is the name of a Bucket the access should have permissions for. // +required @@ -175,9 +175,11 @@ type AccessedBucket struct { // +kubebuilder:validation:MaxLength=253 BucketName string `json:"bucketName"` - // accessMode is the Read/Write access mode that the access should have for the bucket. + // bucketClaimName must match a BucketClaimAccess's BucketClaimName from the spec. // +required - AccessMode BucketAccessMode `json:"accessMode"` + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=253 + BucketClaimName string `json:"bucketClaimName"` } // +kubebuilder:object:root=true diff --git a/client/apis/objectstorage/v1alpha2/definitions.go b/client/apis/objectstorage/v1alpha2/definitions.go index 1db1edf5..162b76fc 100644 --- a/client/apis/objectstorage/v1alpha2/definitions.go +++ b/client/apis/objectstorage/v1alpha2/definitions.go @@ -16,12 +16,36 @@ limitations under the License. package v1alpha2 +// Finalizers const ( // ProtectionFinalizer is applied to a COSI resource object to protect it from deletion while // COSI processes deletion of the object's intermediate and backend resources. ProtectionFinalizer = `objectstorage.k8s.io/protection` ) +// Annotations +const ( + // HasBucketAccessReferencesAnnotation : This annotation is applied by the COSI Controller to a + // BucketClaim when a BucketAccess that references the BucketClaim is created. The annotation + // remains for as long as any BucketAccess references the BucketClaim. Once all BucketAccesses + // that reference the BucketClaim are deleted, the annotation is removed. + HasBucketAccessReferencesAnnotation = `objectstorage.k8s.io/has-bucketaccess-references` + + // SidecarCleanupFinishedAnnotation : This annotation is applied by a COSI Sidecar to a managed + // BucketAccess when the resources is being deleted. The Sidecar calls the Driver to perform + // backend deletion actions and then hands off final deletion cleanup to the COSI Controller + // by setting this annotation on the resource. + SidecarCleanupFinishedAnnotation = `objectstorage.k8s.io/sidecar-cleanup-finished` + + // ControllerManagementOverrideAnnotation : This annotation can be applied to a resource by the + // COSI Controller in order to reclaim management of the resource temporarily when it would + // otherwise be managed by a COSI Sidecar. This is intended for scenarios where a bug in + // provisioning needs to be rectified by a newer version of the COSI Controller. Once the bug is + // resolved, the annotation should be removed to allow normal Sidecar handoff to occur. + ControllerManagementOverrideAnnotation = `objectstorage.k8s.io/controller-management-override` +) + +// Sidecar RPC definitions const ( // RpcEndpointDefault is the default RPC endpoint unix socket location. RpcEndpointDefault = "unix:///var/lib/cosi/cosi.sock" diff --git a/client/config/crd/objectstorage.k8s.io_bucketaccesses.yaml b/client/config/crd/objectstorage.k8s.io_bucketaccesses.yaml index f1542913..4d28dc3c 100644 --- a/client/config/crd/objectstorage.k8s.io_bucketaccesses.yaml +++ b/client/config/crd/objectstorage.k8s.io_bucketaccesses.yaml @@ -142,16 +142,14 @@ spec: with per-Bucket access options. This field is populated by the COSI Controller based on the referenced BucketClaims in the spec. items: - description: AccessedBucket identifies a Bucket and corresponding - access parameters. + description: AccessedBucket identifies a Bucket and correlates it + to a BucketClaimAccess from the spec. properties: - accessMode: - description: accessMode is the Read/Write access mode that the - access should have for the bucket. - enum: - - ReadWrite - - ReadOnly - - WriteOnly + bucketClaimName: + description: bucketClaimName must match a BucketClaimAccess's + BucketClaimName from the spec. + maxLength: 253 + minLength: 1 type: string bucketName: description: bucketName is the name of a Bucket the access should @@ -160,7 +158,7 @@ spec: minLength: 1 type: string required: - - accessMode + - bucketClaimName - bucketName type: object type: array diff --git a/controller/internal/reconciler/bucketaccess.go b/controller/internal/reconciler/bucketaccess.go index bf8b90f6..28e5152e 100644 --- a/controller/internal/reconciler/bucketaccess.go +++ b/controller/internal/reconciler/bucketaccess.go @@ -18,13 +18,24 @@ package reconciler import ( "context" + "fmt" + "slices" + "time" + "github.com/go-logr/logr" + kerrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - logf "sigs.k8s.io/controller-runtime/pkg/log" + ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + ctrlpredicate "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + cosiapi "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2" objectstoragev1alpha2 "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2" + "sigs.k8s.io/container-object-storage-interface/internal/handoff" + cosipredicate "sigs.k8s.io/container-object-storage-interface/internal/predicate" ) // BucketAccessReconciler reconciles a BucketAccess object @@ -33,25 +44,57 @@ type BucketAccessReconciler struct { Scheme *runtime.Scheme } -// +kubebuilder:rbac:groups=objectstorage.k8s.io,resources=bucketaccesses,verbs=get;list;watch;create;update;patch;delete -// +kubebuilder:rbac:groups=objectstorage.k8s.io,resources=bucketaccesses/status,verbs=get;update;patch +// +kubebuilder:rbac:groups=objectstorage.k8s.io,resources=bucketaccesses,verbs=get;list;watch;create;update +// +kubebuilder:rbac:groups=objectstorage.k8s.io,resources=bucketaccesses/status,verbs=get;update // +kubebuilder:rbac:groups=objectstorage.k8s.io,resources=bucketaccesses/finalizers,verbs=update // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. -// TODO(user): Modify the Reconcile function to compare the state specified by -// the BucketAccess object against the actual cluster state, and then -// perform operations to make the cluster state reflect the state specified by -// the user. -// -// For more details, check Reconcile and its Result here: -// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.21.0/pkg/reconcile func (r *BucketAccessReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - _ = logf.FromContext(ctx) + logger := ctrl.LoggerFrom(ctx) - // TODO(user): your logic here + access := &cosiapi.BucketAccess{} + if err := r.Get(ctx, req.NamespacedName, access); err != nil { + if kerrors.IsNotFound(err) { + logger.V(1).Info("not reconciling nonexistent BucketAccess") + return ctrl.Result{}, nil + } + // no resource to add status to or report an event for + logger.Error(err, "failed to get BucketAccess") + return ctrl.Result{}, err + } - return ctrl.Result{}, nil + if handoff.BucketAccessManagedBySidecar(access) { + logger.V(1).Info("not reconciling BucketAccess that should be managed by sidecar") + return ctrl.Result{}, nil + } + + retryError, err := r.reconcile(ctx, logger, access) + if err != nil { + // Because the BucketAccess status is could be managed by either Sidecar or Controller, + // indicate that this error is coming from the Controller. + err = fmt.Errorf("COSI Controller error: %w", err) + + // Record any error as a timestamped error in the status. + access.Status.Error = cosiapi.NewTimestampedError(time.Now(), err.Error()) + if updErr := r.Status().Update(ctx, access); updErr != nil { + logger.Error(err, "failed to update BucketAccess status after reconcile error", "updateError", updErr) + // If status update fails, we must retry the error regardless of the reconcile return. + // The reconcile needs to run again to make sure the status is eventually be updated. + return reconcile.Result{}, err + } + + if !retryError { + return reconcile.Result{}, reconcile.TerminalError(err) + } + return reconcile.Result{}, err + } + + // NOTE: Do not clear the error in the status on success. Success indicates 1 of 2 things: + // 1. BucketAccess was initialized successfully, and it's now owned by the Sidecar + // 2. BucketAccess deletion cleanup was just finished, and no status update is needed + + return reconcile.Result{}, err } // SetupWithManager sets up the controller with the Manager. @@ -59,5 +102,366 @@ func (r *BucketAccessReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&objectstoragev1alpha2.BucketAccess{}). Named("bucketaccess"). + WithEventFilter( + ctrlpredicate.And( + cosipredicate.BucketAccessManagedByController(r.Scheme), // only opt in to reconciles managed by controller + ctrlpredicate.Or( + // when managed by controller, we should reconcile ALL Create/Delete/Generic events + cosipredicate.AnyCreate(), + cosipredicate.AnyDelete(), + cosipredicate.AnyGeneric(), + // opt in to desired update events + cosipredicate.BucketAccessHandoffOccurred(r.Scheme), // reconcile any handoff change + cosipredicate.ProtectionFinalizerRemoved(r.Scheme), // re-add protection finalizer if removed + ), + ), + ). Complete(r) } + +func (r *BucketAccessReconciler) reconcile( + ctx context.Context, logger logr.Logger, access *cosiapi.BucketAccess, +) (retryErrorType, error) { + if !access.GetDeletionTimestamp().IsZero() { + logger.V(1).Info("beginning BucketAccess deletion cleanup") + + // TODO: deletion logic + + ctrlutil.RemoveFinalizer(access, cosiapi.ProtectionFinalizer) + if err := r.Update(ctx, access); err != nil { + logger.Error(err, "failed to remove finalizer") + return RetryError, fmt.Errorf("failed to remove finalizer: %w", err) + } + + return DoNotRetryError, fmt.Errorf("deletion is not yet implemented") // TODO + } + + needInit, err := needsControllerInitialization(&access.Status) + if err != nil { + logger.Error(err, "processed a degraded BucketAccess") + return DoNotRetryError, fmt.Errorf("processed a degraded BucketAccess: %w", err) + } + if !needInit { + // BucketAccessClass info should only be copied to the BucketAccess status once, upon + // initial provisioning. After the info is copied, make no attempt to fill in any missing or + // lost info because we don't know whether the current Class is compatible with the info + // from the existing (old) Class info. If we reach this condition, something is systemically + // wrong. Sidecar should have ownership, but we determined otherwise, and the Sidecar will + // likely also determine us to be the owner. + logger.Error(nil, "processed a BucketAccess that should be managed by COSI Sidecar") + return DoNotRetryError, fmt.Errorf("processed a BucketAccess that should be managed by COSI Sidecar") + } + + logger = logger.WithValues("bucketAccessClassName", access.Spec.BucketAccessClassName) + + logger.V(1).Info("initializing BucketAccess") + + didAdd := ctrlutil.AddFinalizer(access, cosiapi.ProtectionFinalizer) + if didAdd { + if err := r.Update(ctx, access); err != nil { + logger.Error(err, "failed to add protection finalizer") + return RetryError, fmt.Errorf("failed to add protection finalizer: %w", err) + } + } + + claimsByName, retryErr, err := getAllBucketClaims(ctx, r.Client, access.Namespace, access.Spec.BucketClaims) + if err != nil { + logger.Error(err, "failed to get all referenced BucketClaims") + return retryErr, err + } + + // Mark as many referenced BucketClaims as possible as soon as possible in the reconcile. + // This ensures that BucketClaims are marked to protect their data from deletion quickly. + if err := markAllBucketClaimsAsAccessed(ctx, r.Client, claimsByName); err != nil { + logger.Error(err, "failed to mark all referenced BucketClaims") + return RetryError, err + } + + class := &cosiapi.BucketAccessClass{} + classNsName := types.NamespacedName{ + Name: access.Spec.BucketAccessClassName, + Namespace: "", // global resource + } + if err := r.Get(ctx, classNsName, class); err != nil { + if kerrors.IsNotFound(err) { + // TODO: for now, return an error and allow the controller to exponential backoff + // until the access class exists. in the future, optimize this by adding a + // access class reconciler that enqueues requests for BucketAccesses that reference the + // class and aren't yet passed to the sidecar. + logger.Error(err, "BucketAccessClass not found") + return RetryError, err + } + logger.Error(err, "failed to get BucketAccessClass") + return RetryError, err + } + + if err := validateAccessAgainstClass(&class.Spec, &access.Spec); err != nil { + logger.Error(err, "BucketAccess failed featureOptions validation") + return DoNotRetryError, err + } + + blockers := cannotAccessBucketClaims(claimsByName, access.Spec) + if len(blockers) > 0 { + logger.Error(nil, "access cannot be provisioned for one or more BucketClaims", "blockers", blockers) + return DoNotRetryError, fmt.Errorf("access cannot be provisioned for one or more BucketClaims: %v", blockers) + } + + waitlist := waitingOnBucketClaims(claimsByName) + if len(waitlist) > 0 { + logger.Error(nil, "waiting for prerequisites before provisioning access", "waitlist", waitlist) + // TODO: for now, return an error and allow the controller to exponential backoff until we + // are done waiting on the resources. in the future, optimize this by adding a bucketclaim + // reconciler that enqueues requests for BucketClaims when they finish provisioning. + return RetryError, fmt.Errorf("waiting for prerequisites before provisioning access: %v", waitlist) + } + + accessedBuckets, err := generateAccessedBuckets(access.Spec.BucketClaims, claimsByName) + if err != nil { + logger.Error(err, "waiting for BucketClaims to finish provisioning") + return RetryError, fmt.Errorf("waiting for BucketClaims to finish provisioning: %w", err) + } + + // After this status update, resource management should be handed off to the Sidecar + access.Status.AccessedBuckets = accessedBuckets + access.Status.DriverName = class.Spec.DriverName + access.Status.AuthenticationType = class.Spec.AuthenticationType + access.Status.Parameters = class.Spec.Parameters + access.Status.Error = nil + if err := r.Status().Update(ctx, access); err != nil { + logger.Error(err, "failed to update BucketClaim status after successful initialization") + return RetryError, err + } + + return NoError, nil +} + +// Return true if the Controller needs to initialize the BucketAccess with BucketClaim and +// BucketAccessClass info. Return false if required info is set. +// Return an error if any required info is only partially set. This indicates some sort of +// degradation or bug. +func needsControllerInitialization(s *cosiapi.BucketAccessStatus) (bool, error) { + requiredFields := map[string]bool{} + requiredFieldIsSet := func(fieldName string, isSet bool) { + requiredFields[fieldName] = isSet + } + + requiredFieldIsSet("status.accessedBuckets", len(s.AccessedBuckets) > 0) + requiredFieldIsSet("status.driverName", s.DriverName != "") + requiredFieldIsSet("status.authenticationType", string(s.AuthenticationType) != "") + + set := []string{} + for field, isSet := range requiredFields { + if isSet { + set = append(set, field) + } + } + + if len(set) == 0 { + return true, nil + } + + if len(set) == len(requiredFields) { + return false, nil + } + + return false, fmt.Errorf("required Controller-managed fields are only partially set: %v", requiredFields) +} + +// Get all BucketClaims that this BucketAccess references. +// If any claims don't exist, assume they don't exist YET; mark them nil in the resulting map +// without treating nonexistence as an error. +// When no error is returned, the output map MUST have an entry for every given BucketClaimAccess. +func getAllBucketClaims( + ctx context.Context, client client.Client, namespace string, claimAccesses []cosiapi.BucketClaimAccess, +) (map[string]*cosiapi.BucketClaim, retryErrorType, error) { + claims := make(map[string]*cosiapi.BucketClaim, len(claimAccesses)) + errs := []error{} + retryErr := RetryError + + for _, ref := range claimAccesses { + if _, ok := claims[ref.BucketClaimName]; ok { + // In testing, the CEL validation rules prevent this case, but no duplicates is critical + // to the access initialization, so double check it. + errs = append(errs, fmt.Errorf("BucketClaim %q is referenced more than once", ref.BucketClaimName)) + retryErr = DoNotRetryError + continue + } + + c := cosiapi.BucketClaim{} + nsName := types.NamespacedName{ + Namespace: namespace, + Name: ref.BucketClaimName, + } + err := client.Get(ctx, nsName, &c) + if kerrors.IsNotFound(err) { + // BucketClaim doesn't exist (yet) + claims[ref.BucketClaimName] = nil + } else if err != nil { + // Unspecified API server error that probably resolves after exponential backoff + errs = append(errs, err) + } else { + // No error + claims[ref.BucketClaimName] = &c + } + } + + if len(errs) > 0 { + return nil, retryErr, fmt.Errorf("could not get one or more BucketClaims: %v", errs) + } + + if len(claims) != len(claimAccesses) { + // Should never happen, but double check because the 1:1 requirement is critical. + return nil, retryErr, fmt.Errorf("did not get one or more BucketClaims, but no errors observed") + } + + return claims, retryErr, nil +} + +// Mark all (non-nil) BucketClaims as having a BucketAccess reference. +func markAllBucketClaimsAsAccessed( + ctx context.Context, + client client.Client, + claimsByName map[string]*cosiapi.BucketClaim, +) error { + errs := []error{} + for _, claim := range claimsByName { + if claim == nil { + continue + } + + if claim.Annotations == nil { + claim.Annotations = map[string]string{} + } + if _, ok := claim.Annotations[cosiapi.HasBucketAccessReferencesAnnotation]; ok { + continue // already present + } + // Race condition: this will still attempt to apply the annotation even when the deletion + // timestamp is set. This may interrupt an in-progress BucketClaim deletion before the point + // of no return, preserving data, or it may be too late. The BucketClaim deletion logic must + // handle the unexpected appearance of this annotation at any point. + claim.Annotations[cosiapi.HasBucketAccessReferencesAnnotation] = "" + if err := client.Update(ctx, claim); err != nil { + errs = append(errs, err) + } + } + if len(errs) > 0 { + return fmt.Errorf("failed to mark one or more BucketClaims as having a BucketAccess reference: %v", errs) + } + + return nil +} + +// Return an error if the BucketAccess doesn't meet BucketAccessClass requirements. +func validateAccessAgainstClass( + class *cosiapi.BucketAccessClassSpec, + access *cosiapi.BucketAccessSpec, +) error { + errs := []string{} + + needServiceAccount := class.AuthenticationType == cosiapi.BucketAccessAuthenticationTypeServiceAccount + if needServiceAccount && access.ServiceAccountName == "" { + errs = append(errs, "serviceAccountName must be specified") + } + + if class.FeatureOptions.DisallowMultiBucketAccess && len(access.BucketClaims) > 1 { + errs = append(errs, "multi-bucket access is disallowed") + } + + for _, claimRef := range access.BucketClaims { + if slices.Contains(class.FeatureOptions.DisallowedBucketAccessModes, claimRef.AccessMode) { + errs = append(errs, + fmt.Sprintf("accessMode %q requested for BucketClaim %q is disallowed", + claimRef.AccessMode, claimRef.BucketClaimName), + ) + } + } + + if len(errs) > 0 { + return fmt.Errorf("one or more features are disallowed by the BucketAccessClass: %v", errs) + } + return nil +} + +// Ensure that all BucketClaims can request the access to be provisioned without known errors. +// Return a list of messages that explain what is blocking provisioning. +func cannotAccessBucketClaims( + claimsByName map[string]*cosiapi.BucketClaim, + spec cosiapi.BucketAccessSpec, +) []string { + blockers := []string{} + for name, claim := range claimsByName { + if claim == nil { + continue + } + if !claim.DeletionTimestamp.IsZero() { + // The BucketClaim might not delete while this BucketAccess exists. + // The BucketAccess can't proceed for the in-deletion BucketClaim. + // Because this is a data safety race, rely on the user to resolve it as they desire. + // This race is probably rare in the real world, so going to excessive lengths to + // resolve it in COSI seems like premature optimization. + blockers = append(blockers, + fmt.Sprintf("stuck: data integrity for deleting BucketClaim %q is not guaranteed", name)) + } + if len(claim.Status.Protocols) > 0 && !slices.Contains(claim.Status.Protocols, spec.Protocol) { + blockers = append(blockers, + fmt.Sprintf("BucketClaim %q does not support protocol %q", name, spec.Protocol)) + } + } + return blockers +} + +// Ensure that all BucketClaims are provisioned enough to continue with access initialization. +// Return a list of messages that explain what needs to be waited on. +func waitingOnBucketClaims(claimsByName map[string]*cosiapi.BucketClaim) []string { + waitMsgs := []string{} + for name, claim := range claimsByName { + if claim == nil { + waitMsgs = append(waitMsgs, fmt.Sprintf("BucketClaim %q does not (yet?) exist", name)) + continue + } + if claim.Status.BoundBucketName == "" || len(claim.Status.Protocols) == 0 { + waitMsgs = append(waitMsgs, fmt.Sprintf("BucketClaim %q is still provisioning", name)) + continue + } + } + return waitMsgs +} + +// Generate the accessedBuckets status list for the BucketAccess. +func generateAccessedBuckets( + claimAccesses []cosiapi.BucketClaimAccess, + claimsByName map[string]*cosiapi.BucketClaim, +) ( + []cosiapi.AccessedBucket, + error, +) { + accessedBuckets := make([]cosiapi.AccessedBucket, len(claimAccesses)) + unbound := []string{} + + // It will be helpful for human readability if the ordering of AccessedBuckets in the status + // matches the ordering of BucketClaims in the spec. + for i, ref := range claimAccesses { + claim, ok := claimsByName[ref.BucketClaimName] + if !ok || claim == nil { + // Unexpected during runtime because getAllBucketClaims() requires that all input + // BucketAccessClaims must be represented in the claimsByName map. + return nil, fmt.Errorf("missing expected BucketClaim internally %q", ref.BucketClaimName) + } + + if claim.Status.BoundBucketName == "" { + unbound = append(unbound, ref.BucketClaimName) + continue + } + + accessedBuckets[i] = cosiapi.AccessedBucket{ + BucketName: claim.Status.BoundBucketName, + BucketClaimName: claim.GetName(), + } + } + + if len(unbound) > 0 { + return nil, fmt.Errorf("one or more BucketClaims are still unbound to a Bucket: %v", unbound) + } + + return accessedBuckets, nil +} diff --git a/controller/internal/reconciler/bucketaccess_test.go b/controller/internal/reconciler/bucketaccess_test.go new file mode 100644 index 00000000..ab18d561 --- /dev/null +++ b/controller/internal/reconciler/bucketaccess_test.go @@ -0,0 +1,883 @@ +/* +Copyright 2025 The Kubernetes Authors. + +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 reconciler + +import ( + "context" + "testing" + "time" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + cosiapi "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2" + "sigs.k8s.io/container-object-storage-interface/internal/handoff" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +func TestBucketAccessReconcile(t *testing.T) { + // valid base claim used for subtests + baseAccess := cosiapi.BucketAccess{ + ObjectMeta: meta.ObjectMeta{ + Name: "my-access", + Namespace: "my-ns", + }, + Spec: cosiapi.BucketAccessSpec{ + BucketClaims: []cosiapi.BucketClaimAccess{ + { + BucketClaimName: "readwrite-bucket", + AccessMode: cosiapi.BucketAccessModeReadWrite, + AccessSecretName: "readwrite-bucket-creds", + }, + { + BucketClaimName: "readonly-bucket", + AccessMode: cosiapi.BucketAccessModeReadOnly, + AccessSecretName: "readonly-bucket-creds", + }, + }, + BucketAccessClassName: "s3-class", + Protocol: cosiapi.ObjectProtocolS3, + ServiceAccountName: "my-app-sa", + }, + } + + accessNsName := types.NamespacedName{ + Namespace: baseAccess.Namespace, + Name: baseAccess.Name, + } + + // valid base class used by subests + baseClass := cosiapi.BucketAccessClass{ + ObjectMeta: meta.ObjectMeta{ + Name: "s3-class", + }, + Spec: cosiapi.BucketAccessClassSpec{ + DriverName: "cosi.s3.internal", + AuthenticationType: cosiapi.BucketAccessAuthenticationTypeKey, + Parameters: map[string]string{ + "maxSize": "100Gi", + "maxIops": "10", + }, + FeatureOptions: cosiapi.BucketAccessFeatureOptions{}, // base: no options + }, + } + + // first valid bucketclaim referenced by above valid access + baseReadWriteClaim := cosiapi.BucketClaim{ + ObjectMeta: meta.ObjectMeta{ + Name: "readwrite-bucket", + Namespace: "my-ns", + UID: "qwerty", + Finalizers: []string{cosiapi.ProtectionFinalizer}, + }, + Spec: cosiapi.BucketClaimSpec{ + BucketClassName: "s3-class", + Protocols: []cosiapi.ObjectProtocol{ + cosiapi.ObjectProtocolS3, + }, + }, + Status: cosiapi.BucketClaimStatus{ + BoundBucketName: "bc-qwerty", + Protocols: []cosiapi.ObjectProtocol{cosiapi.ObjectProtocolS3}, + }, + } + + readWriteClaimNsName := types.NamespacedName{ + Namespace: baseReadWriteClaim.Namespace, + Name: baseReadWriteClaim.Name, + } + + // second valid bucketclaim referenced by above valid access + baseReadOnlyClaim := cosiapi.BucketClaim{ + ObjectMeta: meta.ObjectMeta{ + Name: "readonly-bucket", + Namespace: "my-ns", + UID: "asdfgh", + Finalizers: []string{cosiapi.ProtectionFinalizer}, + }, + Spec: cosiapi.BucketClaimSpec{ + BucketClassName: "s3-class", + Protocols: []cosiapi.ObjectProtocol{ + cosiapi.ObjectProtocolS3, + }, + }, + Status: cosiapi.BucketClaimStatus{ + BoundBucketName: "bc-asdfgh", + Protocols: []cosiapi.ObjectProtocol{cosiapi.ObjectProtocolS3, cosiapi.ObjectProtocolAzure}, + }, + } + + readOnlyClaimNsName := types.NamespacedName{ + Namespace: baseReadOnlyClaim.Namespace, + Name: baseReadOnlyClaim.Name, + } + + ctx := context.Background() + nolog := logr.Discard() + scheme := runtime.NewScheme() + err := cosiapi.AddToScheme(scheme) + require.NoError(t, err) + + newClient := func(withObj ...client.Object) client.Client { + return fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(withObj...). + WithStatusSubresource(withObj...). // assume all starting objects have status + Build() + } + + t.Run("dynamic provisioning, happy path", func(t *testing.T) { + c := newClient( + baseAccess.DeepCopy(), + baseClass.DeepCopy(), + baseReadWriteClaim.DeepCopy(), + baseReadOnlyClaim.DeepCopy(), + ) + r := BucketAccessReconciler{ + Client: c, + Scheme: scheme, + } + nctx := logr.NewContext(ctx, nolog) + + res, err := r.Reconcile(nctx, ctrl.Request{NamespacedName: accessNsName}) + assert.NoError(t, err) + assert.Empty(t, res) + + access := &cosiapi.BucketAccess{} + err = c.Get(ctx, accessNsName, access) + require.NoError(t, err) + assert.Contains(t, access.GetFinalizers(), cosiapi.ProtectionFinalizer) + status := access.Status + assert.False(t, status.ReadyToUse) + assert.Nil(t, status.Error) + assert.Equal(t, "", status.AccountID) + assert.Equal(t, + []cosiapi.AccessedBucket{ + { + BucketName: "bc-qwerty", + BucketClaimName: "readwrite-bucket", + }, + { + BucketName: "bc-asdfgh", + BucketClaimName: "readonly-bucket", + }, + }, + status.AccessedBuckets, + ) + assert.Equal(t, "cosi.s3.internal", status.DriverName) + assert.Equal(t, "Key", string(status.AuthenticationType)) + assert.Equal(t, + map[string]string{ + "maxSize": "100Gi", + "maxIops": "10", + }, + status.Parameters, + ) + + assert.True(t, handoff.BucketAccessManagedBySidecar(access)) // MUST hand off to sidecar + needInit, err := needsControllerInitialization(&access.Status) // MUST be fully initialized + assert.NoError(t, err) + assert.False(t, needInit) + + crw := &cosiapi.BucketClaim{} + err = c.Get(ctx, readWriteClaimNsName, crw) + require.NoError(t, err) + assert.Contains(t, crw.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + + cro := &cosiapi.BucketClaim{} + err = c.Get(ctx, readOnlyClaimNsName, cro) + require.NoError(t, err) + assert.Contains(t, cro.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + + t.Log("run Reconcile() a second time to ensure nothing is modified") + + // using the same client and stuff from before + res, err = r.Reconcile(nctx, ctrl.Request{NamespacedName: accessNsName}) + assert.NoError(t, err) + assert.Empty(t, res) + + secondAccess := &cosiapi.BucketAccess{} + err = c.Get(ctx, accessNsName, secondAccess) + require.NoError(t, err) + assert.Equal(t, access, secondAccess) + + crw2 := &cosiapi.BucketClaim{} + err = c.Get(ctx, readWriteClaimNsName, crw2) + require.NoError(t, err) + assert.Equal(t, crw, crw2) + + cro2 := &cosiapi.BucketClaim{} + err = c.Get(ctx, readOnlyClaimNsName, cro2) + require.NoError(t, err) + assert.Equal(t, cro, cro2) + }) + + t.Run("dynamic provisioning, a bucketclaim doesn't exist", func(t *testing.T) { + c := newClient( + baseAccess.DeepCopy(), + baseClass.DeepCopy(), + baseReadWriteClaim.DeepCopy(), + // readonly-bucket claim doesn't exist + ) + r := BucketAccessReconciler{ + Client: c, + Scheme: scheme, + } + nctx := logr.NewContext(ctx, nolog) + + res, err := r.Reconcile(nctx, ctrl.Request{NamespacedName: accessNsName}) + assert.Error(t, err) + assert.NotErrorIs(t, err, reconcile.TerminalError(nil)) + assert.Empty(t, res) + + access := &cosiapi.BucketAccess{} + err = c.Get(ctx, accessNsName, access) + require.NoError(t, err) + assert.Contains(t, access.GetFinalizers(), cosiapi.ProtectionFinalizer) + status := access.Status + assert.False(t, status.ReadyToUse) + require.NotNil(t, status.Error) + assert.NotNil(t, status.Error.Time) + assert.NotContains(t, *status.Error.Message, "readwrite-bucket") + assert.Contains(t, *status.Error.Message, "readonly-bucket") + assert.Equal(t, "", status.AccountID) + assert.Empty(t, status.AccessedBuckets) + assert.Empty(t, status.DriverName) + assert.Empty(t, status.AuthenticationType) + assert.Empty(t, status.Parameters) + + assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar + needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized + assert.NoError(t, err) + assert.True(t, needInit) + + crw := &cosiapi.BucketClaim{} + err = c.Get(ctx, readWriteClaimNsName, crw) + require.NoError(t, err) + assert.Contains(t, crw.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + }) + + t.Run("dynamic provisioning, 1 claim ready, 1 claim provisioning", func(t *testing.T) { + rwc := baseReadWriteClaim.DeepCopy() + rwc.Status = cosiapi.BucketClaimStatus{} + + c := newClient( + baseAccess.DeepCopy(), + baseClass.DeepCopy(), + rwc, + baseReadOnlyClaim.DeepCopy(), + ) + r := BucketAccessReconciler{ + Client: c, + Scheme: scheme, + } + nctx := logr.NewContext(ctx, nolog) + + res, err := r.Reconcile(nctx, ctrl.Request{NamespacedName: accessNsName}) + assert.Error(t, err) + assert.NotErrorIs(t, err, reconcile.TerminalError(nil)) + assert.Empty(t, res) + + access := &cosiapi.BucketAccess{} + err = c.Get(ctx, accessNsName, access) + require.NoError(t, err) + assert.Contains(t, access.GetFinalizers(), cosiapi.ProtectionFinalizer) + status := access.Status + assert.False(t, status.ReadyToUse) + require.NotNil(t, status.Error) + assert.NotNil(t, status.Error.Time) + assert.Contains(t, *status.Error.Message, "readwrite-bucket") + assert.NotContains(t, *status.Error.Message, "readonly-bucket") + assert.Equal(t, "", status.AccountID) + assert.Empty(t, status.AccessedBuckets) + assert.Empty(t, status.DriverName) + assert.Empty(t, status.AuthenticationType) + assert.Empty(t, status.Parameters) + + assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar + needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized + assert.NoError(t, err) + assert.True(t, needInit) + + crw := &cosiapi.BucketClaim{} + err = c.Get(ctx, readWriteClaimNsName, crw) + require.NoError(t, err) + assert.Contains(t, crw.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + + cro := &cosiapi.BucketClaim{} + err = c.Get(ctx, readOnlyClaimNsName, cro) + require.NoError(t, err) + assert.Contains(t, cro.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + }) + + t.Run("dynamic provisioning, 1 claim provisioning, 1 claim deleting", func(t *testing.T) { + rwc := baseReadWriteClaim.DeepCopy() + rwc.Status = cosiapi.BucketClaimStatus{} + + roc := baseReadOnlyClaim.DeepCopy() + roc.DeletionTimestamp = &meta.Time{Time: time.Now()} + + c := newClient( + baseAccess.DeepCopy(), + baseClass.DeepCopy(), + rwc, + roc, + ) + r := BucketAccessReconciler{ + Client: c, + Scheme: scheme, + } + nctx := logr.NewContext(ctx, nolog) + + res, err := r.Reconcile(nctx, ctrl.Request{NamespacedName: accessNsName}) + assert.Error(t, err) + assert.ErrorIs(t, err, reconcile.TerminalError(nil)) + assert.Empty(t, res) + + access := &cosiapi.BucketAccess{} + err = c.Get(ctx, accessNsName, access) + require.NoError(t, err) + assert.Contains(t, access.GetFinalizers(), cosiapi.ProtectionFinalizer) + status := access.Status + assert.False(t, status.ReadyToUse) + require.NotNil(t, status.Error) + assert.NotNil(t, status.Error.Time) + assert.Contains(t, *status.Error.Message, + "data integrity for deleting BucketClaim \"readonly-bucket\" is not guaranteed") + assert.Equal(t, "", status.AccountID) + assert.Empty(t, status.AccessedBuckets) + assert.Empty(t, status.DriverName) + assert.Empty(t, status.AuthenticationType) + assert.Empty(t, status.Parameters) + + assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar + needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized + assert.NoError(t, err) + assert.True(t, needInit) + + crw := &cosiapi.BucketClaim{} + err = c.Get(ctx, readWriteClaimNsName, crw) + require.NoError(t, err) + assert.Contains(t, crw.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + + // being deleted, but still needs to be marked + cro := &cosiapi.BucketClaim{} + err = c.Get(ctx, readOnlyClaimNsName, cro) + require.NoError(t, err) + assert.Contains(t, cro.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + }) + + t.Run("dynamic provisioning, 1 claim ready, 1 claim protocol unsupported", func(t *testing.T) { + roc := baseReadOnlyClaim.DeepCopy() + roc.Status.Protocols = []cosiapi.ObjectProtocol{cosiapi.ObjectProtocolGcs} + + c := newClient( + baseAccess.DeepCopy(), + baseClass.DeepCopy(), + baseReadWriteClaim.DeepCopy(), + roc, + ) + r := BucketAccessReconciler{ + Client: c, + Scheme: scheme, + } + nctx := logr.NewContext(ctx, nolog) + + res, err := r.Reconcile(nctx, ctrl.Request{NamespacedName: accessNsName}) + assert.Error(t, err) + assert.ErrorIs(t, err, reconcile.TerminalError(nil)) + assert.Empty(t, res) + + access := &cosiapi.BucketAccess{} + err = c.Get(ctx, accessNsName, access) + require.NoError(t, err) + assert.Contains(t, access.GetFinalizers(), cosiapi.ProtectionFinalizer) + status := access.Status + assert.False(t, status.ReadyToUse) + require.NotNil(t, status.Error) + assert.NotNil(t, status.Error.Time) + assert.Contains(t, *status.Error.Message, "readonly-bucket") + assert.Equal(t, "", status.AccountID) + assert.Empty(t, status.AccessedBuckets) + assert.Empty(t, status.DriverName) + assert.Empty(t, status.AuthenticationType) + assert.Empty(t, status.Parameters) + + assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar + needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized + assert.NoError(t, err) + assert.True(t, needInit) + + crw := &cosiapi.BucketClaim{} + err = c.Get(ctx, readWriteClaimNsName, crw) + require.NoError(t, err) + assert.Contains(t, crw.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + + // being deleted, but still needs to be marked + cro := &cosiapi.BucketClaim{} + err = c.Get(ctx, readOnlyClaimNsName, cro) + require.NoError(t, err) + assert.Contains(t, cro.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + }) + + t.Run("dynamic provisioning, bucketaccessclass doesn't exist", func(t *testing.T) { + c := newClient( + baseAccess.DeepCopy(), + // class doesn't exist + baseReadWriteClaim.DeepCopy(), + baseReadOnlyClaim.DeepCopy(), + ) + r := BucketAccessReconciler{ + Client: c, + Scheme: scheme, + } + nctx := logr.NewContext(ctx, nolog) + + res, err := r.Reconcile(nctx, ctrl.Request{NamespacedName: accessNsName}) + assert.Error(t, err) + assert.NotErrorIs(t, err, reconcile.TerminalError(nil)) + assert.Empty(t, res) + + access := &cosiapi.BucketAccess{} + err = c.Get(ctx, accessNsName, access) + require.NoError(t, err) + assert.Contains(t, access.GetFinalizers(), cosiapi.ProtectionFinalizer) + status := access.Status + assert.False(t, status.ReadyToUse) + require.NotNil(t, status.Error) + assert.NotNil(t, status.Error.Time) + assert.Contains(t, *status.Error.Message, "s3-class") + assert.Equal(t, "", status.AccountID) + assert.Empty(t, status.AccessedBuckets) + assert.Empty(t, status.DriverName) + assert.Empty(t, status.AuthenticationType) + assert.Empty(t, status.Parameters) + + assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar + needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized + assert.NoError(t, err) + assert.True(t, needInit) + + crw := &cosiapi.BucketClaim{} + err = c.Get(ctx, readWriteClaimNsName, crw) + require.NoError(t, err) + assert.Contains(t, crw.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + + cro := &cosiapi.BucketClaim{} + err = c.Get(ctx, readOnlyClaimNsName, cro) + require.NoError(t, err) + assert.Contains(t, cro.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + }) + + t.Run("dynamic provisioning, bucketaccessclass disallows multi-bucket access", func(t *testing.T) { + class := baseClass.DeepCopy() + class.Spec.FeatureOptions.DisallowMultiBucketAccess = true + + c := newClient( + baseAccess.DeepCopy(), + class, + baseReadWriteClaim.DeepCopy(), + baseReadOnlyClaim.DeepCopy(), + ) + r := BucketAccessReconciler{ + Client: c, + Scheme: scheme, + } + nctx := logr.NewContext(ctx, nolog) + + res, err := r.Reconcile(nctx, ctrl.Request{NamespacedName: accessNsName}) + assert.Error(t, err) + assert.ErrorIs(t, err, reconcile.TerminalError(nil)) + assert.Empty(t, res) + + access := &cosiapi.BucketAccess{} + err = c.Get(ctx, accessNsName, access) + require.NoError(t, err) + assert.Contains(t, access.GetFinalizers(), cosiapi.ProtectionFinalizer) + status := access.Status + assert.False(t, status.ReadyToUse) + require.NotNil(t, status.Error) + assert.NotNil(t, status.Error.Time) + assert.Contains(t, *status.Error.Message, "multi-bucket access") + assert.Equal(t, "", status.AccountID) + assert.Empty(t, status.AccessedBuckets) + assert.Empty(t, status.DriverName) + assert.Empty(t, status.AuthenticationType) + assert.Empty(t, status.Parameters) + + assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar + needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized + assert.NoError(t, err) + assert.True(t, needInit) + + crw := &cosiapi.BucketClaim{} + err = c.Get(ctx, readWriteClaimNsName, crw) + require.NoError(t, err) + assert.Contains(t, crw.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + + cro := &cosiapi.BucketClaim{} + err = c.Get(ctx, readOnlyClaimNsName, cro) + require.NoError(t, err) + assert.Contains(t, cro.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + }) + + t.Run("dynamic provisioning, single-bucket passes when multi-bucket access is disallowed", func(t *testing.T) { + access := baseAccess.DeepCopy() + access.Spec.BucketClaims = []cosiapi.BucketClaimAccess{ + baseAccess.DeepCopy().Spec.BucketClaims[0], + } + + class := baseClass.DeepCopy() + class.Spec.FeatureOptions.DisallowMultiBucketAccess = true + + c := newClient( + access, + class, + baseReadWriteClaim.DeepCopy(), + baseReadOnlyClaim.DeepCopy(), + ) + r := BucketAccessReconciler{ + Client: c, + Scheme: scheme, + } + nctx := logr.NewContext(ctx, nolog) + + res, err := r.Reconcile(nctx, ctrl.Request{NamespacedName: accessNsName}) + assert.NoError(t, err) + assert.Empty(t, res) + + access = &cosiapi.BucketAccess{} + err = c.Get(ctx, accessNsName, access) + require.NoError(t, err) + assert.Contains(t, access.GetFinalizers(), cosiapi.ProtectionFinalizer) + status := access.Status + assert.False(t, status.ReadyToUse) + assert.Nil(t, status.Error) + assert.Equal(t, "", status.AccountID) + assert.Equal(t, + []cosiapi.AccessedBucket{ + { + BucketName: "bc-qwerty", + BucketClaimName: "readwrite-bucket", + }, + }, + status.AccessedBuckets, + ) + assert.Equal(t, "cosi.s3.internal", status.DriverName) + assert.Equal(t, "Key", string(status.AuthenticationType)) + assert.Equal(t, + map[string]string{ + "maxSize": "100Gi", + "maxIops": "10", + }, + status.Parameters, + ) + + assert.True(t, handoff.BucketAccessManagedBySidecar(access)) // MUST hand off to sidecar + needInit, err := needsControllerInitialization(&access.Status) // MUST be fully initialized + assert.NoError(t, err) + assert.False(t, needInit) + + crw := &cosiapi.BucketClaim{} + err = c.Get(ctx, readWriteClaimNsName, crw) + require.NoError(t, err) + assert.Contains(t, crw.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + + cro := &cosiapi.BucketClaim{} + err = c.Get(ctx, readOnlyClaimNsName, cro) + require.NoError(t, err) + assert.NotContains(t, cro.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) // not referenced + }) + + t.Run("dynamic provisioning, bucketaccessclass disallows write modes", func(t *testing.T) { + class := baseClass.DeepCopy() + class.Spec.FeatureOptions.DisallowedBucketAccessModes = []cosiapi.BucketAccessMode{ + cosiapi.BucketAccessModeReadWrite, + cosiapi.BucketAccessModeWriteOnly, + } + + c := newClient( + baseAccess.DeepCopy(), + class, + baseReadWriteClaim.DeepCopy(), + baseReadOnlyClaim.DeepCopy(), + ) + r := BucketAccessReconciler{ + Client: c, + Scheme: scheme, + } + nctx := logr.NewContext(ctx, nolog) + + res, err := r.Reconcile(nctx, ctrl.Request{NamespacedName: accessNsName}) + assert.Error(t, err) + assert.ErrorIs(t, err, reconcile.TerminalError(nil)) + assert.Empty(t, res) + + access := &cosiapi.BucketAccess{} + err = c.Get(ctx, accessNsName, access) + require.NoError(t, err) + assert.Contains(t, access.GetFinalizers(), cosiapi.ProtectionFinalizer) + status := access.Status + assert.False(t, status.ReadyToUse) + require.NotNil(t, status.Error) + assert.NotNil(t, status.Error.Time) + assert.Contains(t, *status.Error.Message, "ReadWrite") + assert.Contains(t, *status.Error.Message, "readwrite-bucket") + assert.Equal(t, "", status.AccountID) + assert.Empty(t, status.AccessedBuckets) + assert.Empty(t, status.DriverName) + assert.Empty(t, status.AuthenticationType) + assert.Empty(t, status.Parameters) + + assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar + needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized + assert.NoError(t, err) + assert.True(t, needInit) + + crw := &cosiapi.BucketClaim{} + err = c.Get(ctx, readWriteClaimNsName, crw) + require.NoError(t, err) + assert.Contains(t, crw.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + + cro := &cosiapi.BucketClaim{} + err = c.Get(ctx, readOnlyClaimNsName, cro) + require.NoError(t, err) + assert.Contains(t, cro.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + }) + + t.Run("duplicate BucketClaim reference", func(t *testing.T) { + // In testing, CEL validation rules catch this, but test it here to be careful + access := baseAccess.DeepCopy() + access.Spec.BucketClaims = []cosiapi.BucketClaimAccess{ + baseAccess.DeepCopy().Spec.BucketClaims[0], + baseAccess.DeepCopy().Spec.BucketClaims[0], + } + + c := newClient( + access, + baseClass.DeepCopy(), + baseReadWriteClaim.DeepCopy(), + baseReadOnlyClaim.DeepCopy(), + ) + r := BucketAccessReconciler{ + Client: c, + Scheme: scheme, + } + nctx := logr.NewContext(ctx, nolog) + + res, err := r.Reconcile(nctx, ctrl.Request{NamespacedName: accessNsName}) + assert.Error(t, err) + assert.ErrorIs(t, err, reconcile.TerminalError(nil)) + assert.Empty(t, res) + + access = &cosiapi.BucketAccess{} + err = c.Get(ctx, accessNsName, access) + require.NoError(t, err) + assert.Contains(t, access.GetFinalizers(), cosiapi.ProtectionFinalizer) + status := access.Status + assert.False(t, status.ReadyToUse) + require.NotNil(t, status.Error) + assert.NotNil(t, status.Error.Time) + assert.Contains(t, *status.Error.Message, "readwrite-bucket") + assert.Equal(t, "", status.AccountID) + assert.Empty(t, status.AccessedBuckets) + assert.Empty(t, status.DriverName) + assert.Empty(t, status.AuthenticationType) + assert.Empty(t, status.Parameters) + + assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar + needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized + assert.NoError(t, err) + assert.True(t, needInit) + + crw := &cosiapi.BucketClaim{} + err = c.Get(ctx, readWriteClaimNsName, crw) + require.NoError(t, err) + assert.NotContains(t, crw.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + + cro := &cosiapi.BucketClaim{} + err = c.Get(ctx, readOnlyClaimNsName, cro) + require.NoError(t, err) + assert.NotContains(t, cro.Annotations, cosiapi.HasBucketAccessReferencesAnnotation) + }) +} + +func Test_validateAccessAgainstClass(t *testing.T) { + tests := []struct { + name string // description of this test case + // Named input parameters for target function. + class *cosiapi.BucketAccessClassSpec + access *cosiapi.BucketAccessSpec + wantErr bool + }{ + {"key auth, disallow nothing", + &cosiapi.BucketAccessClassSpec{ + AuthenticationType: cosiapi.BucketAccessAuthenticationTypeKey, + FeatureOptions: cosiapi.BucketAccessFeatureOptions{}, + }, + &cosiapi.BucketAccessSpec{ + BucketClaims: []cosiapi.BucketClaimAccess{ + { + BucketClaimName: "rw", + AccessMode: cosiapi.BucketAccessModeReadWrite, + AccessSecretName: "rw", + }, + { + BucketClaimName: "ro", + AccessMode: cosiapi.BucketAccessModeReadOnly, + AccessSecretName: "ro", + }, + }, + ServiceAccountName: "", + }, + false, + }, + {"key auth, disallow multi-bucket", + &cosiapi.BucketAccessClassSpec{ + AuthenticationType: cosiapi.BucketAccessAuthenticationTypeKey, + FeatureOptions: cosiapi.BucketAccessFeatureOptions{ + DisallowMultiBucketAccess: true, + }, + }, + &cosiapi.BucketAccessSpec{ + BucketClaims: []cosiapi.BucketClaimAccess{ + { + BucketClaimName: "rw", + AccessMode: cosiapi.BucketAccessModeReadWrite, + AccessSecretName: "rw", + }, + { + BucketClaimName: "ro", + AccessMode: cosiapi.BucketAccessModeReadOnly, + AccessSecretName: "ro", + }, + }, + ServiceAccountName: "", + }, + true, + }, + {"key auth, disallow write modes", + &cosiapi.BucketAccessClassSpec{ + AuthenticationType: cosiapi.BucketAccessAuthenticationTypeKey, + FeatureOptions: cosiapi.BucketAccessFeatureOptions{ + DisallowedBucketAccessModes: []cosiapi.BucketAccessMode{ + cosiapi.BucketAccessModeReadWrite, + cosiapi.BucketAccessModeWriteOnly, + }, + }, + }, + &cosiapi.BucketAccessSpec{ + BucketClaims: []cosiapi.BucketClaimAccess{ + { + BucketClaimName: "rw", + AccessMode: cosiapi.BucketAccessModeReadWrite, + AccessSecretName: "rw", + }, + { + BucketClaimName: "ro", + AccessMode: cosiapi.BucketAccessModeReadOnly, + AccessSecretName: "ro", + }, + }, + ServiceAccountName: "", + }, + true, + }, + {"serviceaccount auth, sa given", + &cosiapi.BucketAccessClassSpec{ + AuthenticationType: cosiapi.BucketAccessAuthenticationTypeServiceAccount, + }, + &cosiapi.BucketAccessSpec{ + BucketClaims: []cosiapi.BucketClaimAccess{ + { + BucketClaimName: "rw", + AccessMode: cosiapi.BucketAccessModeReadWrite, + AccessSecretName: "rw", + }, + { + BucketClaimName: "ro", + AccessMode: cosiapi.BucketAccessModeReadOnly, + AccessSecretName: "ro", + }, + }, + ServiceAccountName: "my-sa", + }, + false, + }, + {"serviceaccount auth, no sa", + &cosiapi.BucketAccessClassSpec{ + AuthenticationType: cosiapi.BucketAccessAuthenticationTypeServiceAccount, + }, + &cosiapi.BucketAccessSpec{ + BucketClaims: []cosiapi.BucketClaimAccess{ + { + BucketClaimName: "rw", + AccessMode: cosiapi.BucketAccessModeReadWrite, + AccessSecretName: "rw", + }, + { + BucketClaimName: "ro", + AccessMode: cosiapi.BucketAccessModeReadOnly, + AccessSecretName: "ro", + }, + }, + ServiceAccountName: "", + }, + true, + }, + {"serviceaccount auth, disallow multi-bucket", + &cosiapi.BucketAccessClassSpec{ + AuthenticationType: cosiapi.BucketAccessAuthenticationTypeServiceAccount, + FeatureOptions: cosiapi.BucketAccessFeatureOptions{ + DisallowMultiBucketAccess: true, + }, + }, + &cosiapi.BucketAccessSpec{ + BucketClaims: []cosiapi.BucketClaimAccess{ + { + BucketClaimName: "rw", + AccessMode: cosiapi.BucketAccessModeReadWrite, + AccessSecretName: "rw", + }, + { + BucketClaimName: "ro", + AccessMode: cosiapi.BucketAccessModeReadOnly, + AccessSecretName: "ro", + }, + }, + ServiceAccountName: "my-sa", + }, + true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotErr := validateAccessAgainstClass(tt.class, tt.access) + if tt.wantErr { + assert.Error(t, gotErr) + } else { + assert.NoError(t, gotErr) + } + }) + } +} diff --git a/controller/internal/reconciler/bucketclaim.go b/controller/internal/reconciler/bucketclaim.go index f4396cf8..7e58b31e 100644 --- a/controller/internal/reconciler/bucketclaim.go +++ b/controller/internal/reconciler/bucketclaim.go @@ -81,7 +81,7 @@ func (r *BucketClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) } // On success, clear any errors in the status. - if claim.Status.Error != nil { + if claim.Status.Error != nil && !claim.DeletionTimestamp.IsZero() { claim.Status.Error = nil if err := r.Status().Update(ctx, claim); err != nil { logger.Error(err, "failed to update BucketClaim status after reconcile success") diff --git a/docs/src/api/out.md b/docs/src/api/out.md index cc3f120d..d20ba371 100644 --- a/docs/src/api/out.md +++ b/docs/src/api/out.md @@ -26,7 +26,7 @@ Package v1alpha2 contains API Schema definitions for the objectstorage v1alpha2 -AccessedBucket identifies a Bucket and corresponding access parameters. +AccessedBucket identifies a Bucket and correlates it to a BucketClaimAccess from the spec. @@ -36,7 +36,7 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | | `bucketName` _string_ | bucketName is the name of a Bucket the access should have permissions for. | | MaxLength: 253
MinLength: 1
| -| `accessMode` _[BucketAccessMode](#bucketaccessmode)_ | accessMode is the Read/Write access mode that the access should have for the bucket. | | Enum: [ReadWrite ReadOnly WriteOnly]
| +| `bucketClaimName` _string_ | bucketClaimName must match a BucketClaimAccess's BucketClaimName from the spec. | | MaxLength: 253
MinLength: 1
| #### Bucket @@ -206,7 +206,6 @@ _Validation:_ - Enum: [ReadWrite ReadOnly WriteOnly] _Appears in:_ -- [AccessedBucket](#accessedbucket) - [BucketAccessFeatureOptions](#bucketaccessfeatureoptions) - [BucketClaimAccess](#bucketclaimaccess) diff --git a/internal/handoff/handoff.go b/internal/handoff/handoff.go new file mode 100644 index 00000000..492301cf --- /dev/null +++ b/internal/handoff/handoff.go @@ -0,0 +1,68 @@ +/* +Copyright 2025 The Kubernetes Authors. + +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 handoff defines logic needed for handing off control of resources between Controller and +// Sidecar. +package handoff + +import ( + cosiapi "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2" +) + +// BucketAccessManagedBySidecar returns true if a BucketAccess should be managed by the Sidecar. +// A false return value indicates that it should be managed by the Controller instead. +// +// In order for COSI Controller and any given Sidecar to work well together, they should avoid +// managing the same BucketAccess resource at the same time. This will help prevent the Controller +// and Sidecar from racing with each other and causing update conflicts. +// Instances where a resource has no manager MUST be avoided without exception. +// +// Version skew between Controller and Sidecar should be assumed. In order for version skew issues +// to be minimized, avoid updating this logic unless it is absolutely critical. If updates are made, +// be sure to carefully consider all version skew cases below. Minimize dual-ownership scenarios, +// and avoid no-owner scenarios. +// +// 1. Sidecar version low, Controller version low +// 2. Sidecar version low, Controller version high +// 3. Sidecar version high, Controller version low +// 4. Sidecar version high, Controller version high +func BucketAccessManagedBySidecar(ba *cosiapi.BucketAccess) bool { + // Allow a future-compatible mechanism by which the Controller can override the normal + // BucketAccess management handoff logic in order to resolve a bug. + // Instances where this is utilized should be infrequent -- ideally, never used. + if _, ok := ba.Annotations[cosiapi.ControllerManagementOverrideAnnotation]; ok { + return false + } + + // During provisioning, there are several status fields that the Controller needs to set before + // the Sidecar can provision an access. However, tying this function's logic to ALL of the + // status items could make long-term Controller-Sidecar handoff logic fragile. More logic means + // more risk of unmanaged resources and more difficulty reasoning about how changes will impact + // ownership during version skew. Minimize risk by relying on a single determining status field. + if ba.Status.DriverName == "" { + return false + } + + // During deletion, as long as the access was handed off to the Sidecar at some point, the + // Sidecar must first clean up the backend bucket, then hand back final deletion to the + // Controller by setting an annotation. + if !ba.DeletionTimestamp.IsZero() { + _, ok := ba.Annotations[cosiapi.SidecarCleanupFinishedAnnotation] + return !ok // ok means sidecar is done cleaning up + } + + return true +} diff --git a/internal/handoff/handoff_test.go b/internal/handoff/handoff_test.go new file mode 100644 index 00000000..006f6296 --- /dev/null +++ b/internal/handoff/handoff_test.go @@ -0,0 +1,154 @@ +/* +Copyright 2025 The Kubernetes Authors. + +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 handoff + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + cosiapi "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2" +) + +func TestBucketAccessManagedBySidecar(t *testing.T) { + tests := []struct { + name string // description of this test case + // input parameters for target function. + isHandedOffToSidecar bool + hasDeletionTimestamp bool + hasSidecarCleanupFinishedAnnotation bool + // desired result + want bool + }{ + // expected real-world scenarios + {name: "new BA", + isHandedOffToSidecar: false, + hasDeletionTimestamp: false, + hasSidecarCleanupFinishedAnnotation: false, + want: false, + }, + {name: "BA handoff to sidecar", + isHandedOffToSidecar: true, + hasDeletionTimestamp: false, + hasSidecarCleanupFinishedAnnotation: false, + want: true, + }, + {name: "sidecar-managed BA begins deleting", + isHandedOffToSidecar: true, + hasDeletionTimestamp: true, + hasSidecarCleanupFinishedAnnotation: false, + want: true, + }, + {name: "controller hand-back after sidecar deletion cleanup", + isHandedOffToSidecar: true, + hasDeletionTimestamp: true, + hasSidecarCleanupFinishedAnnotation: true, + want: false, + }, + {name: "BA deleted before sidecar handoff", + isHandedOffToSidecar: false, + hasDeletionTimestamp: true, + hasSidecarCleanupFinishedAnnotation: false, + want: false, + }, + // degraded scenarios + {name: "new BA, erroneous sidecar cleanup annotation", + isHandedOffToSidecar: false, + hasDeletionTimestamp: false, + hasSidecarCleanupFinishedAnnotation: true, // erroneous + want: false, + }, + {name: "sidecar-managed BA, erroneous sidecar cleanup annotation", + isHandedOffToSidecar: true, + hasDeletionTimestamp: false, + hasSidecarCleanupFinishedAnnotation: true, // erroneous + want: true, + }, + {name: "BA deleted before sidecar handoff, erroneous sidecar cleanup annotation", + isHandedOffToSidecar: false, + hasDeletionTimestamp: true, + hasSidecarCleanupFinishedAnnotation: true, // erroneous + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + base := &cosiapi.BucketAccess{ + ObjectMeta: meta.ObjectMeta{ + Name: "my-access", + Namespace: "tenant", + Finalizers: []string{ + cosiapi.ProtectionFinalizer, + "something-else", + }, + Annotations: map[string]string{ + "user-annotation": "value", + "key-only": "", + }, + CreationTimestamp: meta.NewTime(time.Now()), + Generation: 2, + UID: types.UID("qwerty"), + }, + Spec: cosiapi.BucketAccessSpec{ + BucketClaims: []cosiapi.BucketClaimAccess{ + { + BucketClaimName: "bc-1", + AccessMode: cosiapi.BucketAccessModeReadWrite, + AccessSecretName: "bc-1-creds", + }, + }, + BucketAccessClassName: "bac-standard", + Protocol: cosiapi.ObjectProtocolS3, + ServiceAccountName: "my-app", + }, + } + + copy := base.DeepCopy() + + if tt.isHandedOffToSidecar { + copy.Status.AccessedBuckets = []cosiapi.AccessedBucket{ + { + BucketName: "bc-asdfgh", + AccessMode: cosiapi.BucketAccessModeReadWrite, + }, + } + copy.Status.DriverName = "some.driver.io" + copy.Status.AuthenticationType = cosiapi.BucketAccessAuthenticationTypeKey + copy.Status.Parameters = map[string]string{} + } + + if tt.hasDeletionTimestamp { + copy.DeletionTimestamp = &meta.Time{Time: time.Now()} + } + + if tt.hasSidecarCleanupFinishedAnnotation { + copy.Annotations[cosiapi.SidecarCleanupFinishedAnnotation] = "" + } + + got := BucketAccessManagedBySidecar(copy) + assert.Equal(t, tt.want, got) + + // for all cases,applying the controller override annotation makes it controller-managed + copy.Annotations[cosiapi.ControllerManagementOverrideAnnotation] = "" + withOverride := BucketAccessManagedBySidecar(copy) + assert.False(t, withOverride) + }) + } +} diff --git a/internal/predicate/predicate.go b/internal/predicate/predicate.go index 3f5349c9..795f76d8 100644 --- a/internal/predicate/predicate.go +++ b/internal/predicate/predicate.go @@ -21,6 +21,9 @@ limitations under the License. package predicate import ( + "fmt" + + "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -29,6 +32,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" cosiapi "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2" + "sigs.k8s.io/container-object-storage-interface/internal/handoff" ) // AnyCreate returns a predicate that enqueues a reconcile for any Create event. @@ -109,6 +113,93 @@ func ProtectionFinalizerRemoved(s *runtime.Scheme) predicate.Funcs { return funcs } +// BucketAccessHandoffOccurred implements a predicate that enqueues a BucketAccess reconcile for +// Update events where the managing component of the BucketAccess changes, indicating that handoff +// between Controller and Sidecar has occurred in either direction. +// +// The predicate does not enqueue requests for any Create/Delete/Generic events. +// This ensures that other predicates can effectively filter out undesired non-Update events. +func BucketAccessHandoffOccurred(s *runtime.Scheme) predicate.Funcs { + funcs := allFalseFuncs() + funcs.UpdateFunc = func(e event.UpdateEvent) bool { + old := e.ObjectOld + new := e.ObjectNew + + logger := ctrl.Log.WithName("predicate") + + oldBa, ok := toTypedOrLogError[*cosiapi.BucketAccess](logger.WithValues("oldOrNew", "old"), s, old) + if !ok { + return false // not a BucketAccess, so don't manage it + } + newBa, ok := toTypedOrLogError[*cosiapi.BucketAccess](logger.WithValues("oldOrNew", "new"), s, new) + if !ok { + return false // not a BucketAccess, so don't manage it + } + + return handoffOccurred(logger, oldBa, newBa) + } + return funcs +} + +// Internal logic for determining if BucketAccess Controller-Sidecar handoff has occurred. +func handoffOccurred(logger logr.Logger, old, new *cosiapi.BucketAccess) bool { + oldIsSidecar := handoff.BucketAccessManagedBySidecar(old) + newIsSidecar := handoff.BucketAccessManagedBySidecar(new) + if oldIsSidecar != newIsSidecar { + toComponentName := func(isSidecar bool) string { + if isSidecar { + return "sidecar" + } + return "controller" + } + logger.Info("BucketAccess management handoff occurred", + "namespace", old.GetNamespace(), "name", old.GetName(), + "oldManagedBy", toComponentName(oldIsSidecar), + "newManagedBy", toComponentName(newIsSidecar)) + return true + } + return false +} + +// BucketAccessManagedBySidecar implements a predicate that enqueues a BucketAccess reconcile for +// any event if (and only if) the BucketAccess should be managed by the COSI Sidecar. +func BucketAccessManagedBySidecar(s *runtime.Scheme) predicate.Funcs { + return predicate.NewPredicateFuncs(func(object client.Object) bool { + ba, ok := toTypedOrLogError[*cosiapi.BucketAccess](ctrl.Log.WithName("predicate"), s, object) + if !ok { + return false // not a BucketAccess, so don't manage it + } + return handoff.BucketAccessManagedBySidecar(ba) + }) +} + +// BucketAccessManagedByController implements a predicate that enqueues a BucketAccess reconcile for +// any event if (and only if) the BucketAccess should be managed by the COSI Controller. +func BucketAccessManagedByController(s *runtime.Scheme) predicate.Funcs { + return predicate.NewPredicateFuncs(func(object client.Object) bool { + ba, ok := toTypedOrLogError[*cosiapi.BucketAccess](ctrl.Log.WithName("predicate"), s, object) + if !ok { + return false // not a BucketAccess, so don't manage it + } + // Note: cannot simply return predicate.Not() of BucketAccessManagedBySidecar() because + // any failed type conversion must return false for both Sidecar and Controller + return !handoff.BucketAccessManagedBySidecar(ba) + }) +} + +// Converts a client object to a typed object. Logs an error if conversion fails. +func toTypedOrLogError[T client.Object](logger logr.Logger, s *runtime.Scheme, object client.Object) (T, bool) { + typed, ok := object.(T) + if !ok { + logger.Error(nil, "failed to convert object with unexpected type", + "expectedType", fmt.Sprintf("%T", *new(T)), + "receivedType", fmt.Sprintf("%T", object), + "kind", inferKind(object, s), "namespace", object.GetNamespace(), "name", object.GetName()) + return *new(T), false + } + return typed, true +} + // Makes a best-effort attempt to infer a likely Kind for the object in the schema. // Useful because controller-runtime predicates don't have GVK info for objects, and logging object // kind in reusable predicates can help disambiguate resources in logs. diff --git a/internal/predicate/predicate_test.go b/internal/predicate/predicate_test.go new file mode 100644 index 00000000..10ad3fe7 --- /dev/null +++ b/internal/predicate/predicate_test.go @@ -0,0 +1,95 @@ +/* +Copyright 2025 The Kubernetes Authors. + +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 predicate + +import ( + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + + cosiapi "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2" +) + +func Test_toTypedOrLogError(t *testing.T) { + // ctrl.SetLogger(zap.New(zap.UseDevMode(true))) + // logger := ctrl.Log.WithName("predicate") + logger := logr.Discard() // comment this and uncomment above to locally test log messages + + scheme := runtime.NewScheme() + err := cosiapi.AddToScheme(scheme) + require.NoError(t, err) + + t.Run("matching type", func(t *testing.T) { + access := &cosiapi.BucketAccess{ + ObjectMeta: meta.ObjectMeta{ + Namespace: "ns", + Name: "name", + }, + } + accessObj := client.Object(access) + + gotObj, ok := toTypedOrLogError[*cosiapi.BucketAccess](logger, scheme, accessObj) + assert.Equal(t, access, gotObj) + assert.True(t, ok) + }) + + t.Run("nonmatching type", func(t *testing.T) { + claim := &cosiapi.BucketClaim{ + ObjectMeta: meta.ObjectMeta{ + Namespace: "ns", + Name: "name", + }, + } + claimObj := client.Object(claim) + + gotObj, ok := toTypedOrLogError[*cosiapi.BucketAccess](logger, scheme, claimObj) + assert.Empty(t, gotObj) + assert.False(t, ok) + }) +} + +func Test_handoffOccurred(t *testing.T) { + ctrl.SetLogger(zap.New(zap.UseDevMode(true))) + logger := ctrl.Log.WithName("predicate") + // logger := logr.Discard() // comment this and uncomment above to locally test log messages + + t.Run("no handoff", func(t *testing.T) { + old := &cosiapi.BucketAccess{} + new := &cosiapi.BucketAccess{} + + assert.False(t, handoffOccurred(logger, old, new)) + }) + + t.Run("handoff", func(t *testing.T) { + old := &cosiapi.BucketAccess{} + new := &cosiapi.BucketAccess{ + Status: cosiapi.BucketAccessStatus{ + DriverName: "something", + }, + } + + assert.True(t, handoffOccurred(logger, old, new)) + }) + +} diff --git a/sidecar/internal/reconciler/bucket.go b/sidecar/internal/reconciler/bucket.go index 746f41d3..8447665a 100644 --- a/sidecar/internal/reconciler/bucket.go +++ b/sidecar/internal/reconciler/bucket.go @@ -82,7 +82,7 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr } // On success, clear any errors in the status. - if bucket.Status.Error != nil { + if bucket.Status.Error != nil && !bucket.DeletionTimestamp.IsZero() { bucket.Status.Error = nil if err := r.Status().Update(ctx, bucket); err != nil { logger.Error(err, "failed to update BucketClaim status after reconcile success") diff --git a/vendor/sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2/bucketaccess_types.go b/vendor/sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2/bucketaccess_types.go index b8ba2670..9a38c277 100644 --- a/vendor/sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2/bucketaccess_types.go +++ b/vendor/sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2/bucketaccess_types.go @@ -167,7 +167,7 @@ type BucketClaimAccess struct { AccessSecretName string `json:"accessSecretName"` } -// AccessedBucket identifies a Bucket and corresponding access parameters. +// AccessedBucket identifies a Bucket and correlates it to a BucketClaimAccess from the spec. type AccessedBucket struct { // bucketName is the name of a Bucket the access should have permissions for. // +required @@ -175,9 +175,11 @@ type AccessedBucket struct { // +kubebuilder:validation:MaxLength=253 BucketName string `json:"bucketName"` - // accessMode is the Read/Write access mode that the access should have for the bucket. + // bucketClaimName must match a BucketClaimAccess's BucketClaimName from the spec. // +required - AccessMode BucketAccessMode `json:"accessMode"` + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=253 + BucketClaimName string `json:"bucketClaimName"` } // +kubebuilder:object:root=true diff --git a/vendor/sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2/definitions.go b/vendor/sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2/definitions.go index 1db1edf5..162b76fc 100644 --- a/vendor/sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2/definitions.go +++ b/vendor/sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2/definitions.go @@ -16,12 +16,36 @@ limitations under the License. package v1alpha2 +// Finalizers const ( // ProtectionFinalizer is applied to a COSI resource object to protect it from deletion while // COSI processes deletion of the object's intermediate and backend resources. ProtectionFinalizer = `objectstorage.k8s.io/protection` ) +// Annotations +const ( + // HasBucketAccessReferencesAnnotation : This annotation is applied by the COSI Controller to a + // BucketClaim when a BucketAccess that references the BucketClaim is created. The annotation + // remains for as long as any BucketAccess references the BucketClaim. Once all BucketAccesses + // that reference the BucketClaim are deleted, the annotation is removed. + HasBucketAccessReferencesAnnotation = `objectstorage.k8s.io/has-bucketaccess-references` + + // SidecarCleanupFinishedAnnotation : This annotation is applied by a COSI Sidecar to a managed + // BucketAccess when the resources is being deleted. The Sidecar calls the Driver to perform + // backend deletion actions and then hands off final deletion cleanup to the COSI Controller + // by setting this annotation on the resource. + SidecarCleanupFinishedAnnotation = `objectstorage.k8s.io/sidecar-cleanup-finished` + + // ControllerManagementOverrideAnnotation : This annotation can be applied to a resource by the + // COSI Controller in order to reclaim management of the resource temporarily when it would + // otherwise be managed by a COSI Sidecar. This is intended for scenarios where a bug in + // provisioning needs to be rectified by a newer version of the COSI Controller. Once the bug is + // resolved, the annotation should be removed to allow normal Sidecar handoff to occur. + ControllerManagementOverrideAnnotation = `objectstorage.k8s.io/controller-management-override` +) + +// Sidecar RPC definitions const ( // RpcEndpointDefault is the default RPC endpoint unix socket location. RpcEndpointDefault = "unix:///var/lib/cosi/cosi.sock"