From 658ef32b9cc21eecf6cf8ecaa5e3cb6c4895b6db Mon Sep 17 00:00:00 2001 From: Zoltan Szabo Date: Wed, 30 Sep 2026 17:28:52 +0200 Subject: [PATCH] feat(servicecredentialbinding): make circuit breaker resumable Signed-off-by: Zoltan Szabo --- cmd/provider/main.go | 10 +- .../deploy-workload-provider-cf.mdx | 63 +++ internal/controller/custom_setup.go | 10 +- .../servicecredentialbinding/controller.go | 181 +++++-- .../controller_test.go | 459 ++++++++++++++++-- 5 files changed, 628 insertions(+), 95 deletions(-) diff --git a/cmd/provider/main.go b/cmd/provider/main.go index 0a8c47da..9a6fb00b 100644 --- a/cmd/provider/main.go +++ b/cmd/provider/main.go @@ -7,6 +7,7 @@ package main import ( "os" "path/filepath" + "strconv" "time" "gopkg.in/alecthomas/kingpin.v2" @@ -22,6 +23,7 @@ import ( "github.com/SAP/crossplane-provider-cloudfoundry/apis" provider "github.com/SAP/crossplane-provider-cloudfoundry/internal/controller" + "github.com/SAP/crossplane-provider-cloudfoundry/internal/controller/servicecredentialbinding" "github.com/SAP/crossplane-provider-cloudfoundry/internal/features" ) @@ -36,8 +38,14 @@ func main() { maxReconcileRate = app.Flag("max-reconcile-rate", "The global maximum rate per second at which resources may checked for drift from the desired state.").Default("10").Int() enableManagementPolicies = app.Flag("enable-management-policies", "Enable support for Management Policies.").Default("true").Envar("ENABLE_MANAGEMENT_POLICIES").Bool() + + scbMaxCreateAttempts = app.Flag("scb-max-create-attempts", "Consecutive ServiceCredentialBinding create attempts before creation is paused."). + Default(strconv.Itoa(servicecredentialbinding.DefaultMaxCreateAttempts)).Envar("SCB_MAX_CREATE_ATTEMPTS").Int() ) kingpin.MustParse(app.Parse(os.Args[1:])) + if *scbMaxCreateAttempts < 1 { + kingpin.Fatalf("--scb-max-create-attempts must be >= 1, got %d", *scbMaxCreateAttempts) + } zl := zap.New(zap.UseDevMode(*debug)) log := logging.NewLogrLogger(zl.WithName("provider-cloudfoundry")) @@ -87,6 +95,6 @@ func main() { log.Info("Alpha feature enabled", "flag", features.EnableBetaManagementPolicies) } - kingpin.FatalIfError(provider.CustomSetup(mgr, o), "Cannot setup custom controllers") + kingpin.FatalIfError(provider.CustomSetup(mgr, o, provider.Config{SCBMaxCreateAttempts: *scbMaxCreateAttempts}), "Cannot setup custom controllers") kingpin.FatalIfError(mgr.Start(ctrl.SetupSignalHandler()), "Cannot start controller manager") } diff --git a/docs/end-user-guides/deploy-workload-provider-cf.mdx b/docs/end-user-guides/deploy-workload-provider-cf.mdx index fe9630c8..c77910ff 100644 --- a/docs/end-user-guides/deploy-workload-provider-cf.mdx +++ b/docs/end-user-guides/deploy-workload-provider-cf.mdx @@ -271,6 +271,69 @@ metadata: - A TTL that's too long may increase security risks by keeping old credentials active longer than necessary ::: +#### Create failure protection + +If creating a `ServiceCredentialBinding` keeps failing, the provider stops after a limited number of consecutive attempts (default `5`) instead of retrying forever. An attempt is counted each time the provider has to create a new binding: the binding is not found in Cloud Foundry, or a rotation replacement is due. + +**Recognising a paused binding:** + +- The `Ready` condition is `False` with the message `Creation failed after attempts; reconciliation paused. ...` +- A `CreateAttemptsExhausted` warning event is recorded on the resource (`kubectl describe servicecredentialbinding `). +- The resource has the annotation `servicecredentialbinding.cloudfoundry.crossplane.io/max-retry-exceeded`, set to the time it was paused. + +While paused, the provider makes no Cloud Foundry calls for this resource. + +**Recovering:** + +1. Check Cloud Foundry for bindings left behind by the failed attempts and delete the ones that don't belong to the resource: + - `type: key`: list keys with `cf service-keys ` and delete them with `cf delete-service-key `. + - `type: app`: check the bound apps section of `cf service ` and unbind with `cf unbind-service `. + + Keep any binding whose GUID matches the resource's `crossplane.io/external-name` annotation, `status.atProvider.guid`, or an entry in `status.atProvider.retiredKeys`. These are still in use; for example, when a rotation replacement is what failed, the previous key is still the one your applications use. `cf service-key --guid` prints a key's GUID. +2. Fix the cause, for example the binding parameters or the service instance the binding points to. +3. Remove the annotation: + + ```shell + kubectl annotate servicecredentialbinding servicecredentialbinding.cloudfoundry.crossplane.io/max-retry-exceeded- + ``` + + The provider reconciles the resource straight away, with a full set of attempts. + +You can also delete a paused resource. Deleting it removes its bindings from Cloud Foundry, including retired rotation keys. + +:::warning Adding the annotation yourself pauses all reconciliation +You can add the same annotation (any value) to stop the provider from working on a binding. This pauses more than creation: while the annotation is present, the provider makes no Cloud Foundry calls for the resource. It doesn't apply spec changes, rotate keys, delete expired retired keys, or refresh the connection secret. The resource also shows `Ready=False` with the `Creation failed after attempts; reconciliation paused. ...` message, even though nothing failed. Deleting the resource still works. Remove the annotation to resume. +::: + +:::note Failed binding kept by Cloud Foundry +With some brokers a binding fails after Cloud Foundry has already created it, and Cloud Foundry keeps the failed binding. The resource then shows `Ready=False` with the broker's message. Fixing the parameters alone does not trigger a retry. Delete the failed binding in Cloud Foundry (`cf delete-service-key ` for `type: key`, `cf unbind-service ` for `type: app`); the provider then creates a new one. + +Each of these retries is a create attempt and counts toward the limit. A failed binding never resets the count; it resets only when a binding succeeds or is adopted. Once the limit is reached, the binding pauses as described above; follow **Recovering** to resume it. +::: + +**Changing the limit:** + +Set the limit for the whole provider with the `--scb-max-create-attempts` flag or the `SCB_MAX_CREATE_ATTEMPTS` environment variable. It must be at least `1`. For example, with a `DeploymentRuntimeConfig`: + +```yaml title="Example: raise the create attempt limit" +apiVersion: pkg.crossplane.io/v1beta1 +kind: DeploymentRuntimeConfig +metadata: + name: provider-cloudfoundry +spec: + deploymentTemplate: + spec: + selector: {} + template: + spec: + containers: + - name: package-runtime + args: + - --scb-max-create-attempts=10 +``` + +Reference it from the `Provider` with `spec.runtimeConfigRef.name: provider-cloudfoundry`. + ### Configure `Route` A route is a unique address/URL that enables our end users to reach our sample applications. diff --git a/internal/controller/custom_setup.go b/internal/controller/custom_setup.go index 560ba5da..4e43b69c 100644 --- a/internal/controller/custom_setup.go +++ b/internal/controller/custom_setup.go @@ -28,9 +28,15 @@ import ( "github.com/SAP/crossplane-provider-cloudfoundry/internal/controller/providerconfig" ) +// Config holds provider settings that controller.Options does not cover. +type Config struct { + // SCBMaxCreateAttempts is the ServiceCredentialBinding create-attempt limit. + SCBMaxCreateAttempts int +} + // CustomSetup creates all controllers with the supplied logger and adds them to // the supplied manager. -func CustomSetup(mgr ctrl.Manager, o controller.Options) error { +func CustomSetup(mgr ctrl.Manager, o controller.Options, cfg Config) error { for _, setup := range []func(ctrl.Manager, controller.Options) error{ providerconfig.Setup, app.Setup, @@ -43,7 +49,7 @@ func CustomSetup(mgr ctrl.Manager, o controller.Options) error { spacemembers.Setup, route.Setup, serviceinstance.Setup, - servicecredentialbinding.Setup, + servicecredentialbinding.SetupWithMaxCreateAttempts(cfg.SCBMaxCreateAttempts), spacequota.Setup, domain.Setup, serviceroutebinding.Setup, diff --git a/internal/controller/servicecredentialbinding/controller.go b/internal/controller/servicecredentialbinding/controller.go index 6b1b89ce..6f4d9a07 100644 --- a/internal/controller/servicecredentialbinding/controller.go +++ b/internal/controller/servicecredentialbinding/controller.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "strconv" + "time" xpv1 "github.com/crossplane/crossplane-runtime/v2/apis/common/v1" "github.com/crossplane/crossplane-runtime/v2/pkg/controller" @@ -45,45 +46,60 @@ const ( errExtractParams = "cannot extract specified parameters: %w" errUnknownState = "unknown last operation state for " + resourceType + " in " + externalSystem errUpdateCR = "cannot update managed resource" - maxCreateAttempts = 5 createAttemptsAnnotation = "crossplane-provider-cloudfoundry/create-attempts" + // MaxRetryExceededKey pauses reconciliation once create attempts are + // exhausted. Removing it resumes with a fresh budget. + MaxRetryExceededKey = "servicecredentialbinding.cloudfoundry.crossplane.io/max-retry-exceeded" + errPaused = "reconciliation of " + resourceType + " is paused by the " + MaxRetryExceededKey + " annotation" ) -// Setup adds a controller that reconciles ServiceCredentialBinding CR. -func Setup(mgr ctrl.Manager, o controller.Options) error { - name := managed.ControllerName(v1alpha1.ServiceCredentialBindingGroupKind) - - options := []managed.ReconcilerOption{ - managed.WithInitializers(), - managed.WithExternalConnector(&connector{ - kube: mgr.GetClient(), - usage: resource.NewLegacyProviderConfigUsageTracker(mgr.GetClient(), &apisv1beta1.ProviderConfigUsage{}), - }), - managed.WithLogger(o.Logger.WithValues("controller", name)), - managed.WithRecorder(event.NewAPIRecorder(mgr.GetEventRecorderFor(name))), - managed.WithPollInterval(o.PollInterval), - } +// DefaultMaxCreateAttempts is the default create-attempt limit. +const DefaultMaxCreateAttempts = 5 + +const reasonCreateAttemptsExhausted event.Reason = "CreateAttemptsExhausted" + +// SetupWithMaxCreateAttempts returns the controller setup for the given create-attempt limit. +func SetupWithMaxCreateAttempts(maxCreateAttempts int) func(ctrl.Manager, controller.Options) error { + return func(mgr ctrl.Manager, o controller.Options) error { + name := managed.ControllerName(v1alpha1.ServiceCredentialBindingGroupKind) + recorder := event.NewAPIRecorder(mgr.GetEventRecorderFor(name)) + + options := []managed.ReconcilerOption{ + managed.WithInitializers(), + managed.WithExternalConnector(&connector{ + kube: mgr.GetClient(), + usage: resource.NewLegacyProviderConfigUsageTracker(mgr.GetClient(), &apisv1beta1.ProviderConfigUsage{}), + recorder: recorder, + maxCreateAttempts: maxCreateAttempts, + }), + managed.WithLogger(o.Logger.WithValues("controller", name)), + managed.WithRecorder(recorder), + managed.WithPollInterval(o.PollInterval), + } - if o.Features.Enabled(features.EnableBetaManagementPolicies) { - options = append(options, managed.WithManagementPolicies()) - } + if o.Features.Enabled(features.EnableBetaManagementPolicies) { + options = append(options, managed.WithManagementPolicies()) + } - r := managed.NewReconciler(mgr, - resource.ManagedKind(v1alpha1.ServiceCredentialBindingGroupVersionKind), - options...) + r := managed.NewReconciler(mgr, + resource.ManagedKind(v1alpha1.ServiceCredentialBindingGroupVersionKind), + options...) - return ctrl.NewControllerManagedBy(mgr). - Named(name). - WithOptions(o.ForControllerRuntime()). - For(&v1alpha1.ServiceCredentialBinding{}). - Complete(ratelimiter.NewReconciler(name, r, o.GlobalRateLimiter)) + return ctrl.NewControllerManagedBy(mgr). + Named(name). + WithOptions(o.ForControllerRuntime()). + For(&v1alpha1.ServiceCredentialBinding{}). + Complete(ratelimiter.NewReconciler(name, r, o.GlobalRateLimiter)) + } } // A connector is expected to produce an external client when its Connect method // is called. type connector struct { - kube k8s.Client - usage resource.LegacyTracker + kube k8s.Client + usage resource.LegacyTracker + recorder event.Recorder + maxCreateAttempts int } // Connect typically produces an ExternalClient by: @@ -92,7 +108,8 @@ type connector struct { // 3. Getting the credentials specified by the ProviderConfig. // 4. Using the credentials to form a client. func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.ExternalClient, error) { - if _, ok := mg.(*v1alpha1.ServiceCredentialBinding); !ok { + cr, ok := mg.(*v1alpha1.ServiceCredentialBinding) + if !ok { return nil, errors.New(errWrongCRType) } @@ -100,6 +117,11 @@ func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.E return nil, fmt.Errorf(errTrackPCUsage, err) } + // Building the CF client already calls the CF API, so check the pause first. + if isCreatePaused(cr) { + return &pausedExternal{maxCreateAttempts: c.maxCreateAttempts}, nil + } + cf, err := clients.ClientFnBuilder(ctx, c.kube)(mg) if err != nil { return nil, fmt.Errorf(errNewClient, err) @@ -112,6 +134,8 @@ func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.E keyRotator: &scb.SCBKeyRotator{ SCBClient: client, }, + recorder: c.recorder, + maxCreateAttempts: c.maxCreateAttempts, } ext.observationStateHandler = ext // Use self as the default handler return ext, nil @@ -135,6 +159,8 @@ type external struct { scbClient scb.ServiceCredentialBinding keyRotator scb.KeyRotator observationStateHandler ObservationStateHandler + recorder event.Recorder + maxCreateAttempts int } // isBindingNotFoundError returns true if the error indicates the binding was not found @@ -155,18 +181,7 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex guid := meta.GetExternalName(cr) serviceBinding, err := scb.GetByIDOrSearch(ctx, c.scbClient, guid, cr.Spec.ForProvider) if isBindingNotFoundError(err) { - // The binding does not exist in Cloud Foundry. If we have exhausted the - // create attempts, settle into an Unavailable state instead of triggering - // another Create. Returning ResourceExists: true with a nil error stops the - // reconciler from calling Create again, which avoids an endless - // create-fail-requeue loop and prevents flooding CF with orphaned bindings. - if isCircuitBreakerTripped(cr) { - cr.SetConditions(xpv1.Unavailable().WithMessage( - fmt.Sprintf("Creation failed after %d attempts. Delete and recreate this resource to retry.", maxCreateAttempts), - )) - return managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil - } - return managed.ExternalObservation{ResourceExists: false}, nil + return c.observeMissing(ctx, cr) } if err != nil { return managed.ExternalObservation{}, fmt.Errorf(errGet, err) @@ -185,11 +200,12 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex cr.Status.AtProvider.GUID = serviceBinding.GUID cr.Status.AtProvider.CreatedAt = &metav1.Time{Time: serviceBinding.CreatedAt} - if c.keyRotator.RetireBinding(cr, serviceBinding) { + // Retiring during deletion would skip Delete() and leak the binding. + if cr.GetDeletionTimestamp().IsZero() && c.keyRotator.RetireBinding(cr, serviceBinding) { if err := c.kube.Status().Update(ctx, cr); err != nil { return managed.ExternalObservation{}, fmt.Errorf(errUpdateStatus, err) } - return managed.ExternalObservation{ResourceExists: false}, nil + return c.notExists(ctx, cr) } scb.UpdateObservation(&cr.Status.AtProvider, serviceBinding) @@ -197,6 +213,18 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex return c.observationStateHandler.HandleObservationState(serviceBinding, ctx, cr) } +// observeMissing handles a binding that is not found in Cloud Foundry. +func (c *external) observeMissing(ctx context.Context, cr *v1alpha1.ServiceCredentialBinding) (managed.ExternalObservation, error) { + // Delete() is not called for a missing binding, so remove retired keys here. + if !cr.GetDeletionTimestamp().IsZero() { + if err := c.keyRotator.DeleteRetiredKeys(ctx, cr); err != nil { + return managed.ExternalObservation{}, fmt.Errorf(errDeleteRetiredKeys, err) + } + return managed.ExternalObservation{ResourceExists: false}, nil + } + return c.notExists(ctx, cr) +} + // Create a ServiceCredentialBinding resource. func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.ExternalCreation, error) { cr, ok := mg.(*v1alpha1.ServiceCredentialBinding) @@ -370,13 +398,72 @@ func resetCreateAttempts(cr *v1alpha1.ServiceCredentialBinding) { meta.RemoveAnnotations(cr, createAttemptsAnnotation) } +// notExists reports a missing binding and pauses creation once the limit is +// reached. Marker and counter removal share one Update, so a resource with +// neither was resumed by a human. +func (c *external) notExists(ctx context.Context, cr *v1alpha1.ServiceCredentialBinding) (managed.ExternalObservation, error) { + if getCreateAttempts(cr) < c.maxCreateAttempts { + return managed.ExternalObservation{ResourceExists: false}, nil + } + meta.AddAnnotations(cr, map[string]string{MaxRetryExceededKey: time.Now().UTC().Format(time.RFC3339)}) + resetCreateAttempts(cr) + if err := c.kube.Update(ctx, cr); err != nil { + // The counter is still at the limit, so the next Observe trips again. + return managed.ExternalObservation{}, fmt.Errorf("%s: %w", errUpdateCR, err) + } + msg := recoveryMessage(c.maxCreateAttempts) + c.recorder.Event(cr, event.Warning(reasonCreateAttemptsExhausted, errors.New(msg))) + cr.SetConditions(xpv1.Unavailable().WithMessage(msg)) + return managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil +} + // isValidUUID returns true if the given string is a valid UUID func isValidUUID(s string) bool { return uuid.Validate(s) == nil } -// isCircuitBreakerTripped returns true if the maximum number of create attempts has been reached. -// If the resource is being deleted, the circuit breaker is bypassed to allow cleanup. -func isCircuitBreakerTripped(cr *v1alpha1.ServiceCredentialBinding) bool { - return getCreateAttempts(cr) >= maxCreateAttempts && cr.GetDeletionTimestamp().IsZero() +// isCreatePaused reports whether the pause marker is set and the resource is not being deleted. +func isCreatePaused(cr *v1alpha1.ServiceCredentialBinding) bool { + _, marked := cr.GetAnnotations()[MaxRetryExceededKey] + return marked && cr.GetDeletionTimestamp().IsZero() +} + +// recoveryMessage tells a human how to resume a paused resource. +func recoveryMessage(limit int) string { + return fmt.Sprintf("Creation failed after %d attempts; reconciliation paused. Verify the binding in Cloud Foundry "+ + "(orphaned bindings, parameters), then remove annotation %s to retry with %d fresh attempts.", + limit, MaxRetryExceededKey, limit) +} + +var _ managed.ExternalClient = &pausedExternal{} + +// pausedExternal makes no Cloud Foundry calls and reports the resource as up +// to date, so Create, Update and Delete are never called. +type pausedExternal struct { + maxCreateAttempts int +} + +func (p *pausedExternal) Observe(_ context.Context, mg resource.Managed) (managed.ExternalObservation, error) { + cr, ok := mg.(*v1alpha1.ServiceCredentialBinding) + if !ok { + return managed.ExternalObservation{}, errors.New(errWrongCRType) + } + cr.SetConditions(xpv1.Unavailable().WithMessage(recoveryMessage(p.maxCreateAttempts))) + return managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil +} + +func (p *pausedExternal) Create(context.Context, resource.Managed) (managed.ExternalCreation, error) { + return managed.ExternalCreation{}, errors.New(errPaused) +} + +func (p *pausedExternal) Update(context.Context, resource.Managed) (managed.ExternalUpdate, error) { + return managed.ExternalUpdate{}, errors.New(errPaused) +} + +func (p *pausedExternal) Delete(context.Context, resource.Managed) (managed.ExternalDelete, error) { + return managed.ExternalDelete{}, errors.New(errPaused) +} + +func (p *pausedExternal) Disconnect(context.Context) error { + return nil } diff --git a/internal/controller/servicecredentialbinding/controller_test.go b/internal/controller/servicecredentialbinding/controller_test.go index 35705525..f6335031 100644 --- a/internal/controller/servicecredentialbinding/controller_test.go +++ b/internal/controller/servicecredentialbinding/controller_test.go @@ -4,21 +4,26 @@ import ( "context" "errors" "fmt" + "maps" "strconv" + "strings" "testing" "time" cfresource "github.com/cloudfoundry/go-cfclient/v3/resource" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/mock" k8s "sigs.k8s.io/controller-runtime/pkg/client" xpv1 "github.com/crossplane/crossplane-runtime/v2/apis/common/v1" + "github.com/crossplane/crossplane-runtime/v2/pkg/event" "github.com/crossplane/crossplane-runtime/v2/pkg/meta" "github.com/crossplane/crossplane-runtime/v2/pkg/reconciler/managed" "github.com/crossplane/crossplane-runtime/v2/pkg/resource" "github.com/crossplane/crossplane-runtime/v2/pkg/test" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/ptr" "github.com/SAP/crossplane-provider-cloudfoundry/apis/resources/v1alpha1" @@ -125,6 +130,8 @@ func TestObserve(t *testing.T) { mg resource.Managed obs managed.ExternalObservation err error + // tripped: the kube Update received the marker and no counter. + tripped bool } scb := serviceCredentialBinding("key", withExternalName(guid), withServiceInstanceID(serviceInstanceGUID), withDefaultMetadataLabels()) @@ -132,6 +139,21 @@ func TestObserve(t *testing.T) { cfSucceeded := func() *cfresource.ServiceCredentialBinding { return &fake.NewServiceCredentialBinding("key").SetName(name).SetGUID(guid).SetServiceInstanceRef(serviceInstanceGUID).SetLastOperation(v1alpha1.LastOperationCreate, v1alpha1.LastOperationSucceeded).SetLabels(map[string]*string{"crossplane-kind": ptr.To("servicecredentialbinding.cloudfoundry.crossplane.io"), "crossplane-name": ptr.To("my-service-credential-binding")}).ServiceCredentialBinding } + cfNotFound := func() *fake.MockServiceCredentialBinding { + m := &fake.MockServiceCredentialBinding{} + m.On("Get", mock.Anything, guid).Return( + fake.ServiceCredentialBindingNil, + fake.ErrNoResultReturned, + ) + return m + } + noKeyRotator := func() *fake.MockKeyRotator { return &fake.MockKeyRotator{} } + deletingOverLimit := serviceCredentialBinding("key", + withExternalName(guid), + withServiceInstanceID(serviceInstanceGUID), + withDeletionTimestamp(), + withCreateAttempts(DefaultMaxCreateAttempts), + ) cases := map[string]struct { args args @@ -140,6 +162,13 @@ func TestObserve(t *testing.T) { kube k8s.Client keyRotator keyRotator observationStateHandler observationStateHandler + limit int // 0 means DefaultMaxCreateAttempts + wantDeleteRetiredKeys int + wantNoRetire bool + updateErr error + // wantKubeCalls is checked only on the default kube client. + wantKubeCalls []string + wantEvents []string }{ "Nil": { args: args{ @@ -325,42 +354,143 @@ func TestObserve(t *testing.T) { return m }, }, - "CircuitBreakerTripped_ResourceNotFound": { + "Trip_NotFound_LegacyCounter": { + // Also the upgrade path from the old counter-only breaker. args: args{ mg: serviceCredentialBinding("key", withExternalName(guid), withServiceInstanceID(serviceInstanceGUID), - withCreateAttempts(maxCreateAttempts), + withCreateAttempts(DefaultMaxCreateAttempts), ), }, want: want{ - obs: managed.ExternalObservation{ - ResourceExists: true, - ResourceUpToDate: true, - }, - err: nil, + obs: managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, + tripped: true, + }, + service: cfNotFound, + keyRotator: noKeyRotator, + wantKubeCalls: []string{"Update"}, + wantEvents: []string{"Warning/CreateAttemptsExhausted"}, + }, + "BelowLimit_NotFound": { + args: args{ + mg: serviceCredentialBinding("key", + withExternalName(guid), + withServiceInstanceID(serviceInstanceGUID), + withCreateAttempts(DefaultMaxCreateAttempts-1), + ), + }, + want: want{ + obs: managed.ExternalObservation{ResourceExists: false}, + }, + service: cfNotFound, + keyRotator: noKeyRotator, + }, + "Trip_UpdateFails": { + args: args{ + mg: serviceCredentialBinding("key", + withExternalName(guid), + withServiceInstanceID(serviceInstanceGUID), + withCreateAttempts(DefaultMaxCreateAttempts), + ), + }, + want: want{ + obs: managed.ExternalObservation{}, + err: fmt.Errorf("%s: %w", errUpdateCR, errCFClientError), + }, + service: cfNotFound, + keyRotator: noKeyRotator, + updateErr: errCFClientError, + wantKubeCalls: []string{"Update"}, + }, + "Trip_Retirement": { + // A forced rotation whose replacement keeps failing. + args: args{ + mg: serviceCredentialBinding("key", + withExternalName(guid), + withServiceInstanceID(serviceInstanceGUID), + withCreateAttempts(DefaultMaxCreateAttempts), + withRetiredKeys(guid), + ), + }, + want: want{ + obs: managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, + tripped: true, }, service: func() *fake.MockServiceCredentialBinding { m := &fake.MockServiceCredentialBinding{} - // Resource is not found and create attempts are exhausted, so - // Observe() settles into an Unavailable state (ResourceExists: true) - // instead of triggering another Create. - m.On("Get", mock.Anything, guid).Return( - fake.ServiceCredentialBindingNil, - fake.ErrNoResultReturned, - ) + m.On("Get", mock.Anything, guid).Return(cfSucceeded(), nil) return m }, keyRotator: func() *fake.MockKeyRotator { - return &fake.MockKeyRotator{} + m := &fake.MockKeyRotator{} + m.On("RetireBinding", mock.Anything, mock.Anything).Return(true) + return m }, + wantKubeCalls: []string{"StatusUpdate", "Update"}, + wantEvents: []string{"Warning/CreateAttemptsExhausted"}, }, - "CircuitBreakerTripped_AdoptionSucceeds": { + "Retirement_BelowLimit": { + args: args{ + mg: serviceCredentialBinding("key", + withExternalName(guid), + withServiceInstanceID(serviceInstanceGUID), + ), + }, + want: want{ + obs: managed.ExternalObservation{ResourceExists: false}, + }, + service: func() *fake.MockServiceCredentialBinding { + m := &fake.MockServiceCredentialBinding{} + m.On("Get", mock.Anything, guid).Return(cfSucceeded(), nil) + return m + }, + keyRotator: func() *fake.MockKeyRotator { + m := &fake.MockKeyRotator{} + m.On("RetireBinding", mock.Anything, mock.Anything).Return(true) + return m + }, + wantKubeCalls: []string{"StatusUpdate"}, + }, + "LimitHonoured_Low": { + args: args{ + mg: serviceCredentialBinding("key", + withExternalName(guid), + withServiceInstanceID(serviceInstanceGUID), + withCreateAttempts(2), + ), + }, + limit: 2, + want: want{ + obs: managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, + tripped: true, + }, + service: cfNotFound, + keyRotator: noKeyRotator, + wantKubeCalls: []string{"Update"}, + wantEvents: []string{"Warning/CreateAttemptsExhausted"}, + }, + "LimitHonoured_High": { + args: args{ + mg: serviceCredentialBinding("key", + withExternalName(guid), + withServiceInstanceID(serviceInstanceGUID), + withCreateAttempts(5), + ), + }, + limit: 10, + want: want{ + obs: managed.ExternalObservation{ResourceExists: false}, + }, + service: cfNotFound, + keyRotator: noKeyRotator, + }, + "Adoption_ResetsCounter": { args: args{ mg: serviceCredentialBinding("key", withExternalName("my-key-name"), withServiceInstanceID(serviceInstanceGUID), - withCreateAttempts(maxCreateAttempts), + withCreateAttempts(DefaultMaxCreateAttempts), ), }, want: want{ @@ -405,6 +535,7 @@ func TestObserve(t *testing.T) { ) return m }, + wantKubeCalls: []string{"Update"}, }, "InvalidGUIDFormat_FallsBackToSpecSearch": { args: args{ @@ -479,7 +610,94 @@ func TestObserve(t *testing.T) { ) return m }, - }} + }, + "Delete_Found_SkipsRetirement": { + // Found during deletion: must stay "exists" so Delete() runs. + args: args{ + mg: serviceCredentialBinding("key", + withExternalName(guid), + withServiceInstanceID(serviceInstanceGUID), + withDeletionTimestamp(), + ), + }, + want: want{ + obs: managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, + }, + service: func() *fake.MockServiceCredentialBinding { + m := &fake.MockServiceCredentialBinding{} + m.On("Get", mock.Anything, guid).Return(cfSucceeded(), nil) + return m + }, + keyRotator: func() *fake.MockKeyRotator { + m := &fake.MockKeyRotator{} + m.On("RetireBinding", mock.Anything, mock.Anything).Return(true) + return m + }, + observationStateHandler: func() *MockObservationStateHandler { + m := &MockObservationStateHandler{} + m.On("HandleObservationState", mock.Anything, mock.Anything, mock.Anything).Return( + managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil, + ) + return m + }, + wantNoRetire: true, + }, + "Delete_NotFound_DeletesRetiredKeys": { + args: args{ + mg: serviceCredentialBinding("key", + withExternalName(guid), + withServiceInstanceID(serviceInstanceGUID), + withDeletionTimestamp(), + withRetiredKeys("11111111-1111-1111-1111-111111111111", "22222222-2222-2222-2222-222222222222"), + ), + }, + want: want{ + obs: managed.ExternalObservation{ResourceExists: false}, + }, + service: cfNotFound, + keyRotator: func() *fake.MockKeyRotator { + m := &fake.MockKeyRotator{} + m.On("DeleteRetiredKeys", mock.Anything, mock.Anything).Return(nil) + return m + }, + wantDeleteRetiredKeys: 1, + }, + "Delete_NotFound_RetiredKeyCleanupFails": { + args: args{ + mg: serviceCredentialBinding("key", + withExternalName(guid), + withServiceInstanceID(serviceInstanceGUID), + withDeletionTimestamp(), + withRetiredKeys("11111111-1111-1111-1111-111111111111"), + ), + }, + want: want{ + obs: managed.ExternalObservation{}, + err: fmt.Errorf(errDeleteRetiredKeys, errCFClientError), + }, + service: cfNotFound, + keyRotator: func() *fake.MockKeyRotator { + m := &fake.MockKeyRotator{} + m.On("DeleteRetiredKeys", mock.Anything, mock.Anything).Return(errCFClientError) + return m + }, + wantDeleteRetiredKeys: 1, + }, + "Delete_NotFound_OverLimitDoesNotTrip": { + args: args{mg: deletingOverLimit.DeepCopy()}, + want: want{ + mg: deletingOverLimit.DeepCopy(), + obs: managed.ExternalObservation{ResourceExists: false}, + }, + service: cfNotFound, + keyRotator: func() *fake.MockKeyRotator { + m := &fake.MockKeyRotator{} + m.On("DeleteRetiredKeys", mock.Anything, mock.Anything).Return(nil) + return m + }, + wantDeleteRetiredKeys: 1, + }, + } for n, tc := range cases { t.Run(n, func(t *testing.T) { @@ -488,17 +706,36 @@ func TestObserve(t *testing.T) { if tc.observationStateHandler != nil { obsHandler = tc.observationStateHandler() } + var kubeCalls []string + // updated holds the annotations the last kube Update received. + var updated map[string]string kubeClient := tc.kube if kubeClient == nil { kubeClient = &test.MockClient{ - MockUpdate: test.NewMockUpdateFn(nil), + MockUpdate: func(_ context.Context, obj k8s.Object, _ ...k8s.UpdateOption) error { + kubeCalls = append(kubeCalls, "Update") + updated = maps.Clone(obj.GetAnnotations()) + return tc.updateErr + }, + MockStatusUpdate: func(context.Context, k8s.Object, ...k8s.SubResourceUpdateOption) error { + kubeCalls = append(kubeCalls, "StatusUpdate") + return nil + }, } } + limit := tc.limit + if limit == 0 { + limit = DefaultMaxCreateAttempts + } + kr := tc.keyRotator() + rec := &fakeRecorder{} c := &external{ kube: kubeClient, scbClient: tc.service(), - keyRotator: tc.keyRotator(), + keyRotator: kr, observationStateHandler: obsHandler, + maxCreateAttempts: limit, + recorder: rec, } obs, err := c.Observe(context.Background(), tc.args.mg) @@ -524,6 +761,50 @@ func TestObserve(t *testing.T) { t.Errorf("Observe(...): -want, +got:\n%s", diff) } } + if cr, ok := tc.args.mg.(*v1alpha1.ServiceCredentialBinding); ok { + readyMsg := cr.GetCondition(xpv1.TypeReady).Message + switch { + case tc.want.err != nil: + // A failed Observe must not claim the resource is paused. + if strings.Contains(readyMsg, MaxRetryExceededKey) { + t.Errorf("Observe(...): Ready message %q names %s although Observe failed", readyMsg, MaxRetryExceededKey) + } + case tc.want.tripped: + // Check what was persisted, not the in-memory object. + marker, marked := updated[MaxRetryExceededKey] + if !marked { + t.Errorf("Observe(...): kube Update did not receive the %s marker", MaxRetryExceededKey) + } else if _, err := time.Parse(time.RFC3339, marker); err != nil { + t.Errorf("Observe(...): marker value %q is not RFC3339: %v", marker, err) + } + if _, counted := updated[createAttemptsAnnotation]; counted { + t.Errorf("Observe(...): kube Update must not receive the counter annotation on trip") + } + if !strings.Contains(readyMsg, MaxRetryExceededKey) { + t.Errorf("Observe(...): Ready message %q does not name %s", readyMsg, MaxRetryExceededKey) + } + default: + if _, marked := cr.GetAnnotations()[MaxRetryExceededKey]; marked { + t.Errorf("Observe(...): want no %s marker, got one", MaxRetryExceededKey) + } + } + } + if tc.kube == nil { + if diff := cmp.Diff(tc.wantKubeCalls, kubeCalls, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("Observe(...): kube writes -want, +got:\n%s", diff) + } + } + gotEvents := make([]string, 0, len(rec.events)) + for _, e := range rec.events { + gotEvents = append(gotEvents, string(e.Type)+"/"+string(e.Reason)) + } + if diff := cmp.Diff(tc.wantEvents, gotEvents, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("Observe(...): events -want, +got:\n%s", diff) + } + kr.AssertNumberOfCalls(t, "DeleteRetiredKeys", tc.wantDeleteRetiredKeys) + if tc.wantNoRetire { + kr.AssertNotCalled(t, "RetireBinding", mock.Anything, mock.Anything) + } }) } } @@ -720,61 +1001,114 @@ func TestUpdate(t *testing.T) { } func TestConnector(t *testing.T) { - type args struct { - ctx context.Context - mg resource.Managed + errKube := errors.New("boom") + // Every Get fails, so a nil error proves Connect never built a CF client. + failingKube := func() k8s.Client { + return &test.MockClient{MockGet: test.NewMockGetFn(errKube)} } + errClientBuild := errors.New("cannot create a client for Cloud Foundry: cannot config cloudfoundry client: cannot get referenced ProviderConfig: boom") type want struct { - client managed.ExternalClient - err error + paused bool + tracked int + err error } cases := map[string]struct { - args args - want want + mg resource.Managed kube k8s.Client + want want }{ "WrongCRType": { - args: args{ - ctx: context.Background(), - mg: &v1alpha1.App{}, // Wrong type - }, - want: want{ - client: nil, - err: errors.New(errWrongCRType), - }, + mg: &v1alpha1.App{}, kube: &test.MockClient{}, + want: want{err: errors.New(errWrongCRType)}, + }, + "Paused": { + mg: serviceCredentialBinding("key", withProviderConfigRef("default"), withMaxRetryExceeded()), + kube: failingKube(), + want: want{paused: true, tracked: 1}, + }, + "PausedButDeleting": { + mg: serviceCredentialBinding("key", withProviderConfigRef("default"), withMaxRetryExceeded(), withDeletionTimestamp()), + kube: failingKube(), + want: want{tracked: 1, err: errClientBuild}, + }, + "NotPaused": { + mg: serviceCredentialBinding("key", withProviderConfigRef("default")), + kube: failingKube(), + want: want{tracked: 1, err: errClientBuild}, }, } for n, tc := range cases { t.Run(n, func(t *testing.T) { - t.Logf("Testing: %s", t.Name()) - + tracked := 0 c := &connector{ kube: tc.kube, - // Skip usage tracker testing for now as it requires more complex setup + usage: resource.LegacyTrackerFn(func(context.Context, resource.LegacyManaged) error { + tracked++ + return nil + }), + maxCreateAttempts: DefaultMaxCreateAttempts, } - client, err := c.Connect(tc.args.ctx, tc.args.mg) + client, err := c.Connect(context.Background(), tc.mg) if tc.want.err != nil && err != nil { if diff := cmp.Diff(tc.want.err.Error(), err.Error()); diff != "" { t.Errorf("Connect(...): want error string != got error string:\n%s", diff) } - } else { - if diff := cmp.Diff(tc.want.err, err); diff != "" { - t.Errorf("Connect(...): want error != got error:\n%s", diff) - } + } else if diff := cmp.Diff(tc.want.err, err); diff != "" { + t.Errorf("Connect(...): want error != got error:\n%s", diff) + } + if tc.want.err != nil && client != nil { + t.Errorf("Connect(...): expected nil client on error, got %T", client) } - if tc.want.client == nil && client != nil { - t.Errorf("Connect(...): expected nil client, got non-nil") + if _, paused := client.(*pausedExternal); paused != tc.want.paused { + t.Errorf("Connect(...): want paused client %t, got %T", tc.want.paused, client) + } + if tracked != tc.want.tracked { + t.Errorf("Connect(...): want %d usage tracking calls, got %d", tc.want.tracked, tracked) } }) } } +func TestPausedExternal(t *testing.T) { + ctx := context.Background() + p := &pausedExternal{maxCreateAttempts: 3} + cr := serviceCredentialBinding("key", withMaxRetryExceeded()) + + obs, err := p.Observe(ctx, cr) + if err != nil { + t.Fatalf("Observe(...): unexpected error: %v", err) + } + if diff := cmp.Diff(managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, obs); diff != "" { + t.Errorf("Observe(...): -want, +got:\n%s", diff) + } + ready := cr.GetCondition(xpv1.TypeReady) + if ready.Reason != xpv1.ReasonUnavailable { + t.Errorf("Observe(...): want Ready reason %q, got %q", xpv1.ReasonUnavailable, ready.Reason) + } + for _, s := range []string{MaxRetryExceededKey, "after 3 attempts", "with 3 fresh attempts"} { + if !strings.Contains(ready.Message, s) { + t.Errorf("Observe(...): Ready message %q does not contain %q", ready.Message, s) + } + } + + // Unreachable via the reconciler, but must still refuse to act. + for name, call := range map[string]func() error{ + "Create": func() error { _, err := p.Create(ctx, cr); return err }, + "Update": func() error { _, err := p.Update(ctx, cr); return err }, + "Delete": func() error { _, err := p.Delete(ctx, cr); return err }, + } { + if err := call(); err == nil || err.Error() != errPaused { + t.Errorf("%s(...): want error %q, got %v", name, errPaused, err) + } + } +} + func TestHandleObservationState(t *testing.T) { type args struct { serviceBinding *cfresource.ServiceCredentialBinding @@ -1402,3 +1736,38 @@ func withCreateAttempts(n int) modifier { }) } } + +func withProviderConfigRef(name string) modifier { + return func(r *v1alpha1.ServiceCredentialBinding) { + r.Spec.ProviderConfigReference = &xpv1.Reference{Name: name} + } +} + +func withMaxRetryExceeded() modifier { + return func(r *v1alpha1.ServiceCredentialBinding) { + meta.AddAnnotations(r, map[string]string{MaxRetryExceededKey: "2026-09-30T00:00:00Z"}) + } +} + +func withDeletionTimestamp() modifier { + return func(r *v1alpha1.ServiceCredentialBinding) { + now := metav1.Now() + r.SetDeletionTimestamp(&now) + } +} + +func withRetiredKeys(guids ...string) modifier { + return func(r *v1alpha1.ServiceCredentialBinding) { + for _, g := range guids { + r.Status.AtProvider.RetiredKeys = append(r.Status.AtProvider.RetiredKeys, &v1alpha1.SCBResource{GUID: g}) + } + } +} + +type fakeRecorder struct { + events []event.Event +} + +func (r *fakeRecorder) Event(_ runtime.Object, e event.Event) { r.events = append(r.events, e) } + +func (r *fakeRecorder) WithAnnotations(...string) event.Recorder { return r }