Skip to content

Improved default setup for External Dependent Resource for selecting and/or matching resources #2972

@csviri

Description

@csviri

Problem statement

In v5.0, we removed the resource discriminator concept to select target resources for a Dependent resource if there are multiple candidates in the EventSource caches. In other words, we must associate a resource in EventSource caches with the DependentResource. If there are multiple such resources for the DependentResource resource type, a logic must be in place to select the target one. So in v5 we introduced a way to automatically do this based on the desired state. For Kubernetes resources, this is more straightforward. Based on the desired state, we can take its name and namespace, and if there are, for example, multiple secondary ConfigMaps in Informer caches, we select the one with the name and namespace that matches the desired state.

This is not that nice for external dependent resources. There we use the generic logic from AbsractDependentResource, we list all the resources and we check that the desired is equals with the one in the cache:

@Override
public Optional<R> getSecondaryResource(P primary, Context<P> context) {
var secondaryResources = context.getSecondaryResources(resourceType());
if (secondaryResources.isEmpty()) {
return Optional.empty();
} else {
return selectTargetSecondaryResource(secondaryResources, primary, context);
}
}
/**
* Selects the actual secondary resource matching the desired state derived from the primary
* resource when several resources of the same type are found in the context. This method allows
* for optimized implementations in subclasses since this default implementation will check each
* secondary candidates for equality with the specified desired state, which might end up costly.
*
* @param secondaryResources to select the target resource from
* @param primary the primary resource
* @param context the context in which this method is called
* @return the matching secondary resource or {@link Optional#empty()} if none matches
* @throws IllegalStateException if more than one candidate is found, in which case some other
* mechanism might be necessary to distinguish between candidate secondary resources
*/
protected Optional<R> selectTargetSecondaryResource(
Set<R> secondaryResources, P primary, Context<P> context) {
R desired = desired(primary, context);
var targetResources = secondaryResources.stream().filter(r -> r.equals(desired)).toList();
if (targetResources.size() > 1) {
throw new IllegalStateException(
"More than one secondary resource related to primary: " + targetResources);
}
return targetResources.isEmpty() ? Optional.empty() : Optional.of(targetResources.get(0));
}

Therefore, it is assumed that the user overrides the equals method for matching purposes.

However, we probably were not cautious enough, since in the default setup the equals method is also used by default for matching the resources, for external dependent resources:

@Override
public Matcher.Result<R> match(R resource, P primary, Context<P> context) {
var desired = desired(primary, context);
return Matcher.Result.computed(resource.equals(desired), desired);
}

What is wrong, since if the user overrides the equals for the purpose of selecting the target resource, the same equals will always match the selected resource with the desired.

Although in practice the user is able to override both methods, so the matching logic like here and also the target selection logic like here:

@Override
protected Optional<ExternalResource> selectTargetSecondaryResource(
Set<ExternalResource> secondaryResources,
ExternalStateCustomResource primary,
Context<ExternalStateCustomResource> context) {
var id = getResourceID(primary);
return id.flatMap(k -> secondaryResources.stream().filter(e -> e.getId().equals(k)).findAny());
}

it is not nice that the default implementation forces the user to have a proper solution out of the box. In addition to that, especially for new users, it can be quite hard to discover/understand the situation and see what method needs to be overridden.

Proposed Solution

For the next minor release, I propose the following solution:

Have an interface that user can implement on the beans representing the external resource - like Schema in mysql sample.
Let's call that interface for now as ExternalDependentIDProvider.

What will provide the ID that can be later checked for equality for selecting the target resources.
If the bean does not implement it for external resource we would be default throw an exception propting the user that either implement this interface or override the selectTargetSecondaryResource method:

@Override
  protected Optional<R> selectTargetSecondaryResource(
      Set<R> secondaryResources, P primary, Context<P> context) {
    R desired = desired(primary, context);
    List<R> targetResources;
    if (desired instanceof ExternalDependentIDProvider<?> desiredWithId) {
      targetResources =
          secondaryResources.stream()
              .filter(r -> ((ExternalDependentIDProvider<?>) r).id().equals(desiredWithId.id()))
              .toList();
    } else {
      throw new IllegalStateException("Either implement ExternalDependentIDProvider in your resource or override selectTargetSecondaryResource.");
    }
    if (targetResources.size() > 1) {
      throw new IllegalStateException(
          "More than one secondary resource related to primary: " + targetResources);
    }
    return targetResources.isEmpty() ? Optional.empty() : Optional.of(targetResources.get(0));
  }

The matching logic could stay for now on equals.

See related PR:
#2970

Additional notes:

  • This is somewhat a breaking change, but we are fixing a broken default setup, and if somebody already did an override on those methods, that will be backwards compatible. Of course, we will provide a nice documentation in the release notes.
  • @florianknecht pointed out that CacheKeyMapper could be reused as the new ID providing interface. I personally don't like to reuse such an interface just because it somewhat resables the semantics. Would rather have it as a separate
    interface now so in case if we would need to change any of it in the future, we don't have to think about both functionalities. But I'm open for discussion.

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions