Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,19 @@ namespace KubeOps.Abstractions.Reconciliation.Controller;
public interface IEntityController<TEntity>
where TEntity : IKubernetesObject<V1ObjectMeta>
{
/// <summary>
/// Returns <c>true</c> when this controller is responsible for the given entity.
/// The default implementation returns <c>true</c>, preserving single-controller backward-compatible behaviour.
///
/// When multiple controllers are registered for the same entity type the reconciler asks every
/// controller whether it <see cref="ShouldHandle"/> the entity and dispatches to all that claim
/// responsibility in registration order. Typical use cases: filtering by labels, annotations,
/// namespace, status conditions, or any other entity-derived predicate the consumer needs.
/// </summary>
/// <param name="entity">The entity the reconciler is about to dispatch.</param>
/// <returns>A <see cref="ValueTask{Boolean}"/> that resolves to <c>true</c> if this controller should reconcile the entity.</returns>
ValueTask<bool> ShouldHandle(TEntity entity) => ValueTask.FromResult(true);
Comment thread
stevefan1999-personal marked this conversation as resolved.
Outdated

/// <summary>
/// Reconciles the state of the specified entity with the desired state.
/// This method is triggered for `added` and `modified` events from the watcher.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,19 @@ namespace KubeOps.Abstractions.Reconciliation.Finalizer;
public interface IEntityFinalizer<TEntity>
where TEntity : IKubernetesObject<V1ObjectMeta>
{
/// <summary>
/// Returns <c>true</c> when this finalizer is responsible for the given entity.
/// The default implementation returns <c>true</c>, preserving backward-compatible behaviour.
///
/// When <see cref="KubeOps.Abstractions.Builder.OperatorSettings.AutoAttachFinalizers"/> is enabled,
/// only finalizers that return <c>true</c> from <see cref="ShouldHandle"/> are attached to the entity.
/// Once attached, the finalizer is dispatched by its identifier as usual — <see cref="ShouldHandle"/>
/// acts as a one-time responsibility claim at attach time, not an ongoing gate.
/// </summary>
/// <param name="entity">The entity the reconciler is considering for this finalizer.</param>
/// <returns>A <see cref="ValueTask{Boolean}"/> that resolves to <c>true</c> if this finalizer should claim the entity.</returns>
ValueTask<bool> ShouldHandle(TEntity entity) => ValueTask.FromResult(true);

/// <summary>
Comment thread
stevefan1999-personal marked this conversation as resolved.
/// Finalize an entity that is pending for deletion.
/// </summary>
Expand Down
5 changes: 4 additions & 1 deletion src/KubeOps.Operator/Builder/OperatorBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ public IOperatorBuilder AddController<TImplementation, TEntity>()
where TImplementation : class, IEntityController<TEntity>
where TEntity : IKubernetesObject<V1ObjectMeta>
{
Services.TryAddScoped<IEntityController<TEntity>, TImplementation>();
// TryAddEnumerable dedupes by (ServiceType, ImplementationType), so calling AddController

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure this AI comment is really needed 😄

// with the same TImplementation twice registers it only once — while still allowing
// distinct implementations to coexist for the same TEntity.
Services.TryAddEnumerable(ServiceDescriptor.Scoped<IEntityController<TEntity>, TImplementation>());
Services.TryAddSingleton<IReconciler<TEntity>, Reconciler<TEntity>>();
Comment thread
stevefan1999-personal marked this conversation as resolved.

// Requeue
Expand Down
81 changes: 73 additions & 8 deletions src/KubeOps.Operator/Reconciliation/Reconciler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using k8s.Models;

using KubeOps.Abstractions.Builder;
using KubeOps.Abstractions.Entities;
using KubeOps.Abstractions.Reconciliation;
using KubeOps.Abstractions.Reconciliation.Controller;
using KubeOps.Abstractions.Reconciliation.Finalizer;
Expand Down Expand Up @@ -115,8 +116,11 @@ await entityQueue
cancellationToken);

await using var scope = serviceProvider.CreateAsyncScope();
var controller = scope.ServiceProvider.GetRequiredService<IEntityController<TEntity>>();
var result = await controller.DeletedAsync(reconciliationContext.Entity, cancellationToken);
var result = await DispatchToMatchingControllers(
scope.ServiceProvider,
reconciliationContext.Entity,
(ctrl, entity, ct) => ctrl.DeletedAsync(entity, ct),
cancellationToken);

if (result.IsSuccess)
{
Expand All @@ -139,19 +143,80 @@ await entityQueue
{
var finalizers = scope.ServiceProvider.GetKeyedServices<IEntityFinalizer<TEntity>>(KeyedService.AnyKey);

var anyFinalizerAdded = finalizers
.Aggregate(
false,
(changed, finalizer) => entity.AddFinalizer(finalizer.GetIdentifierName(entity)) || changed);
var anyFinalizerAdded = false;
foreach (var finalizer in finalizers)
{
cancellationToken.ThrowIfCancellationRequested();
if (!await finalizer.ShouldHandle(entity))
{
continue;
}

anyFinalizerAdded = entity.AddFinalizer(finalizer.GetIdentifierName(entity)) || anyFinalizerAdded;
}
Comment thread
stevefan1999-personal marked this conversation as resolved.

if (anyFinalizerAdded)
{
entity = await client.UpdateAsync(entity, cancellationToken);
}
}

var controller = scope.ServiceProvider.GetRequiredService<IEntityController<TEntity>>();
return await controller.ReconcileAsync(entity, cancellationToken);
return await DispatchToMatchingControllers(
scope.ServiceProvider,
entity,
(ctrl, e, ct) => ctrl.ReconcileAsync(e, ct),
cancellationToken);
}

/// <summary>
/// Gets all <see cref="IEntityController{TEntity}"/> registrations whose <see cref="IEntityController{TEntity}.ShouldHandle"/>
/// returns <c>true</c> for the given entity, then calls <paramref name="operation"/> on each in registration order.
/// On the first failure the chain is short-circuited and that failure result is returned.
/// If no controller is registered at all the result is a configuration-error failure; if controllers are
/// registered but none claim responsibility, a success result is returned and a warning is logged.
/// </summary>
private async Task<ReconciliationResult<TEntity>> DispatchToMatchingControllers(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be honest I am pretty unsure about the possibility to have more than 1 controller being responsible for an entity - shouldn't the pattern be more like:

  • if there == 1 controller -> dispatch
  • if there == 0 -> finish with not dispatch
  • if there > 1 -> error

IServiceProvider services,
TEntity entity,
Func<IEntityController<TEntity>, TEntity, CancellationToken, Task<ReconciliationResult<TEntity>>> operation,
CancellationToken cancellationToken)
{
var registeredControllers = services.GetServices<IEntityController<TEntity>>().ToList();
if (registeredControllers.Count == 0)
{
return ReconciliationResult<TEntity>.Failure(
entity,
$"No IEntityController<{typeof(TEntity).Name}> registered. Did you forget to call AddController<T, TEntity>() on the operator builder?");
}
Comment on lines +193 to +199

Copilot AI Apr 11, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cancellation is checked only after resolving/enumerating controllers via GetServices(...).ToList(). If cancellation is already requested, this still constructs all scoped controllers (and could run their constructors) unnecessarily. Consider calling cancellationToken.ThrowIfCancellationRequested() before resolving controllers (and similarly before enumerating finalizers) to avoid work after cancellation.

Copilot uses AI. Check for mistakes.

var responsibleControllers = new List<IEntityController<TEntity>>();
foreach (var controller in registeredControllers)
{
cancellationToken.ThrowIfCancellationRequested();
if (await controller.ShouldHandle(entity))
{
responsibleControllers.Add(controller);
}
}

if (responsibleControllers.Count == 0)
{
logger.LogWarning(
"""No responsible controller found for "{Kind}/{Name}". Skipping.""",
entity.Kind,
entity.Name());
return ReconciliationResult<TEntity>.Success(entity);
}
Comment thread
stevefan1999-personal marked this conversation as resolved.
Outdated

ReconciliationResult<TEntity> result = ReconciliationResult<TEntity>.Success(entity);
foreach (var controller in responsibleControllers)
{
cancellationToken.ThrowIfCancellationRequested();
result = await operation(controller, result.Entity, cancellationToken);
if (!result.IsSuccess) return result;

Copilot AI Apr 11, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ShouldHandle is evaluated for all controllers against the initial entity before any controller runs, but the dispatched controllers receive the potentially mutated result.Entity. This means a controller can be invoked even if it would return false for the entity state it actually receives. Consider evaluating ShouldHandle just-in-time per controller using the current entity (or document that ShouldHandle is evaluated only on the pre-dispatch entity).

Copilot uses AI. Check for mistakes.
}

return result;

Copilot AI Apr 11, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the multi-controller loop, any RequeueAfter requested by an earlier controller can be overwritten/dropped by a later controller that returns success with RequeueAfter == null, because result is replaced each iteration. This can change behavior when adding an additional controller (e.g., an auditing controller) and prevent intended requeues. Consider aggregating RequeueAfter across all successful controller results (e.g., keep the earliest/non-null) while still passing the latest Entity through the chain.

Suggested change
foreach (var controller in responsibleControllers)
{
cancellationToken.ThrowIfCancellationRequested();
result = await operation(controller, result.Entity, cancellationToken);
if (!result.IsSuccess) return result;
}
return result;
var requeueAfter = result.RequeueAfter;
foreach (var controller in responsibleControllers)
{
cancellationToken.ThrowIfCancellationRequested();
result = await operation(controller, result.Entity, cancellationToken);
if (!result.IsSuccess) return result;
if (result.RequeueAfter is not null &&
(requeueAfter is null || result.RequeueAfter < requeueAfter))
{
requeueAfter = result.RequeueAfter;
}
}
return ReconciliationResult<TEntity>.Success(result.Entity, requeueAfter);

Copilot uses AI. Check for mistakes.
}

private async Task<ReconciliationResult<TEntity>> ReconcileFinalizersSequential(TEntity entity, CancellationToken cancellationToken)
Expand Down
70 changes: 70 additions & 0 deletions test/KubeOps.Operator.Test/Builder/OperatorBuilder.Test.cs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,67 @@ public void Should_Add_Leader_Elector()
s.Lifetime == ServiceLifetime.Singleton);
}

[Fact]
public void Should_Allow_Multiple_Controllers_For_Same_Entity_Type()
{
_builder.AddController<TestController, V1OperatorIntegrationTestEntity>();
_builder.AddController<SecondTestController, V1OperatorIntegrationTestEntity>();

var registrations = _builder.Services
.Where(s =>
s.ServiceType == typeof(IEntityController<V1OperatorIntegrationTestEntity>) &&
s.Lifetime == ServiceLifetime.Scoped)
.ToList();

registrations.Should().HaveCount(2);
registrations.Should().Contain(s => s.ImplementationType == typeof(TestController));
registrations.Should().Contain(s => s.ImplementationType == typeof(SecondTestController));
}

[Fact]
public void Should_Dedupe_Identical_Controller_Registrations()
{
_builder.AddController<TestController, V1OperatorIntegrationTestEntity>();
_builder.AddController<TestController, V1OperatorIntegrationTestEntity>();

var registrations = _builder.Services
.Where(s => s.ServiceType == typeof(IEntityController<V1OperatorIntegrationTestEntity>))
.ToList();

registrations.Should().HaveCount(1);
registrations.Should().ContainSingle(s => s.ImplementationType == typeof(TestController));
}

[Fact]
public void Should_Resolve_All_Controllers_For_Same_Entity_Type()
{
_builder.AddController<TestController, V1OperatorIntegrationTestEntity>();
_builder.AddController<SecondTestController, V1OperatorIntegrationTestEntity>();

var provider = _builder.Services.BuildServiceProvider();
var controllers = provider
.GetServices<IEntityController<V1OperatorIntegrationTestEntity>>()
.ToList();

controllers.Should().HaveCount(2);
controllers.Should().ContainItemsAssignableTo<IEntityController<V1OperatorIntegrationTestEntity>>();
controllers.Select(c => c.GetType()).Should().Contain(typeof(TestController));
controllers.Select(c => c.GetType()).Should().Contain(typeof(SecondTestController));

Copilot AI Apr 11, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test resolves scoped IEntityController<T> instances directly from the root ServiceProvider. In production this is typically done from an IServiceScope (and can fail if ValidateScopes is enabled). Consider creating a scope (provider.CreateScope()) and resolving controllers from scope.ServiceProvider to better reflect runtime behavior.

Copilot uses AI. Check for mistakes.
}

[Fact]
public void Should_Not_Register_Duplicate_ResourceWatcher_For_Multiple_Controllers()
{
_builder.AddController<TestController, V1OperatorIntegrationTestEntity>();
_builder.AddController<SecondTestController, V1OperatorIntegrationTestEntity>();

_builder.Services
.Where(s =>
s.ServiceType == typeof(IHostedService) &&
s.ImplementationType == typeof(ResourceWatcher<V1OperatorIntegrationTestEntity>))
.Should().HaveCount(1);
}

[Fact]
public void Should_Add_LeaderAwareResourceWatcher()
{
Expand All @@ -152,6 +213,15 @@ public Task<ReconciliationResult<V1OperatorIntegrationTestEntity>> DeletedAsync(
Task.FromResult(ReconciliationResult<V1OperatorIntegrationTestEntity>.Success(entity));
}

private sealed class SecondTestController : IEntityController<V1OperatorIntegrationTestEntity>
{
public Task<ReconciliationResult<V1OperatorIntegrationTestEntity>> ReconcileAsync(V1OperatorIntegrationTestEntity entity, CancellationToken cancellationToken) =>
Task.FromResult(ReconciliationResult<V1OperatorIntegrationTestEntity>.Success(entity));

public Task<ReconciliationResult<V1OperatorIntegrationTestEntity>> DeletedAsync(V1OperatorIntegrationTestEntity entity, CancellationToken cancellationToken) =>
Task.FromResult(ReconciliationResult<V1OperatorIntegrationTestEntity>.Success(entity));
}

private sealed class TestFinalizer : IEntityFinalizer<V1OperatorIntegrationTestEntity>
{
public Task<ReconciliationResult<V1OperatorIntegrationTestEntity>> FinalizeAsync(V1OperatorIntegrationTestEntity entity, CancellationToken cancellationToken) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public async Task Elector_Throws_Should_Retry()

var electionLock = Mock.Of<ILock>();

var electionLockSubsequentCallEvent = new AutoResetEvent(false);
var subsequentCallTcs = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
bool hasElectionLockThrown = false;
Mock.Get(electionLock)
.Setup(el => el.GetAsync(It.IsAny<CancellationToken>()))
Expand All @@ -34,7 +34,7 @@ public async Task Elector_Throws_Should_Retry()
if (hasElectionLockThrown)
{
// Signal to the test that a subsequent call has been made.
electionLockSubsequentCallEvent.Set();
subsequentCallTcs.TrySetResult(true);

// Delay returning for a long time, allowing the test to stop the background service, in turn cancelling the cancellation token.
await Task.Delay(TimeSpan.FromSeconds(10), cancellationToken);
Expand All @@ -54,8 +54,9 @@ public async Task Elector_Throws_Should_Retry()
await leaderElectionBackgroundService.StartAsync(CancellationToken.None);

// Starting the background service should result in the lock attempt throwing, and then a subsequent attempt being made.
// Wait for the subsequent event to be signalled, if we time out the test fails. The retry delay requires us to wait at least 3 seconds.
electionLockSubsequentCallEvent.WaitOne(TimeSpan.FromMilliseconds(3100)).Should().BeTrue();
// Wait for the retry to be signalled; use a generous timeout so CI scheduling jitter doesn't cause false failures.
var completed = await Task.WhenAny(subsequentCallTcs.Task, Task.Delay(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken));
completed.Should().Be(subsequentCallTcs.Task, "the leader elector should retry after throwing");

await leaderElectionBackgroundService.StopAsync(CancellationToken.None);
}
Expand Down
Loading
Loading