Ability to extend functionality of existing matchers like `SSABasedGenericKubernetesResourceMatcher`

[murillio4]

Is your feature request related to a problem? Please describe.
While addressing the issue discussed in #2509 regarding matching Quantity values, I created a custom matcher to extend the functionality of SSABasedGenericKubernetesResourceMatcher. My goal was to build upon the original matcher, preserving much of its existing functionality. However, when I attempted to override the matches function, I found that the other necessary methods were private, preventing me from utilizing them in my override. This forced me to clone the entire class to achieve the desired functionality.

Describe the solution you'd like
I would like to be able to create custom SSABased matchers by extending SSABasedGenericKubernetesResourceMatcher without needing to clone it entirely.

Describe alternatives you've considered
One alternative is to make certain helper methods—such as checkIfFieldManagerExists, sanitizeState, keepOnlyManagedFields, and removeIrrelevantValues—protected instead of private. Doing this may also require that some constants be readable at the subclass level, allowing for greater flexibility and customization when extending the base class.

[csviri]

Absolutely, feel free to come up with a PR, we can have a default set also of such rules. Would be nice to make it extensible maybe to have custom functions that normalises the resources.

[murillio4]

Sorry for the delay! 😊 I’ll aim to create a PR for this within the week.

[csviri]

thx @murillio4 !
note that there is an other related PR: #2565

[Donnerbart]

We would also have a use-case to customize this matcher. I didn't implement this yet in our project, we still use some copied code from the GenericKubernetesResourceMatcher, but this is giving us more and more headaches.

For us it would probably be the easiest to have a simple method like this that can be overridden, giving access to the internal data structures:

  public boolean matches(R actual, R desired, Context<?> context) {
    ...

    var matches = prunedActual.equals(desiredMap);
    if (!matches && log.isDebugEnabled() && LoggingUtils.isNotSensitiveResource(desired)) {
      var diff = getDiff(prunedActual, desiredMap, objectMapper);
      log.debug(
          "Diff between actual and desired state for resource: {} with name: {} in namespace: {} is:\n{}",
          actual.getKind(), actual.getMetadata().getName(), actual.getMetadata().getNamespace(),
          diff);
    }
    return matches(actual, desired, prunedActual, desiredMap, context, matches);
  }

  /**
   * Returns the {@code defaultResult} result.
   * <p>
   * Can be overridden to customize the matcher result.
   *
   * @param actual the actual resource
   * @param desired the desired resource
   * @param actualMap the actual resource as map
   * @param desiredMap the desired resource as map
   * @param context the current context
   * @param defaultResult the match result determined by the
   *        {@link #matches(HasMetadata, HasMetadata, Context)} method
   * @return the {@code defaultResult}
   */
  @SuppressWarnings("unused")
  protected boolean matches(R actual, R desired,
      Map<String, Object> actualMap, Map<String, Object> desiredMap,
      Context<?> context, boolean defaultResult) {
    return defaultResult;
  }

EDIT: We could use then a customized SSA matcher in our public class StatefulSetResource extends CRUDKubernetesDependentResource<StatefulSet, HiveMQPlatform> in a custom match() method. I think we would prefer this over a more generic way to add more sanitizers via the operator config or something like that. We only need this for the StatefulSet, but no other resources.

But for that use-case we need to do a lot of customization to trigger specific states of our operator, depending on the changes that were detected (e.g. just a replicas change -> SCALING operation, just a log level change -> SET_LOG_LEVEL operation, image or config change -> ROLLING_RESTART operation). That's why I'd like to have access to the internal data structures, which are already cleaned up and sanitized, to analyze them further for these specific changes.

EDIT 2: We would also need access to the resources themselves, updated the code snippet.

[Donnerbart]

I'm just playing around with this a little and we would actually need access to the primary resource. Not sure if we would need access to the raw actual and desired resource. But that one is not even handed into the parent match method right now.

So more like something like this? https://gist.github.com/Donnerbart/f8c15193f88009cecb8e8134dcdb8ccb

[Donnerbart]

The patched SSABasedGenericKubernetesResourceMatcher from my last comment works fine for our use-case.

The biggest issue is unit testing though. We run into the same problem like SSABasedGenericKubernetesResourceMatcherTest in this repo: To test the matches() method the actual instance needs a valid managedFields entry. For integration tests this isn't a problem, because there you have a real K8s cluster that will generate these fields automatically. For unit tests though there is no client-side implementation for this available (at least to my knowledge). Also the fabric8 mock webserver doesn't support SSA to provide the managed fields entry.

In this repo this was solved by using YAML files (and the ones I've added were dumped from a real K8s cluster). To do this programmatically you can create a valid managedFields entry like this:

    private static void setManagedFields(final HasMetadata resource) {
        final var managedFieldsEntry = new ManagedFieldsEntryBuilder().withApiVersion("apps/v1")
                .withFieldsType("FieldsV1")
                .withManager(CONTROLLER_NAME)
                .withOperation(SSABasedGenericKubernetesResourceMatcher.APPLY_OPERATION)
                .withNewFieldsV1()
                .addToAdditionalProperties(generateFieldsV1(resource))
                .endFieldsV1()
                .withTime(Instant.now().toString())
                .build();
        resource.getMetadata().setManagedFields(List.of(managedFieldsEntry));
    }

The problem though is the generateFieldsV1(resource) method, that would need to imitate the K8s server implementation to serialize the fields from the given resource. Especially a generic implementation that can handle all K8s resources seems to be quite hard to implement to me (not sure if that can be done without reflection to get all field names etc.). Not sure this is something we would like to add to the fabric8 kubernetes-client (maybe just to the mock webserver, so it's just available for testing).

[murillio4]

We would also have a use-case to customize this matcher. I didn't implement this yet in our project, we still use some copied code from the GenericKubernetesResourceMatcher, but this is giving us more and more headaches.

For us it would probably be the easiest to have a simple method like this that can be overridden, giving access to the internal data structures:

  public boolean matches(R actual, R desired, Context<?> context) {
    ...

    var matches = prunedActual.equals(desiredMap);
    if (!matches && log.isDebugEnabled() && LoggingUtils.isNotSensitiveResource(desired)) {
      var diff = getDiff(prunedActual, desiredMap, objectMapper);
      log.debug(
          "Diff between actual and desired state for resource: {} with name: {} in namespace: {} is:\n{}",
          actual.getKind(), actual.getMetadata().getName(), actual.getMetadata().getNamespace(),
          diff);
    }
    return matches(actual, desired, prunedActual, desiredMap, context, matches);
  }

  /**
   * Returns the {@code defaultResult} result.
   * <p>
   * Can be overridden to customize the matcher result.
   *
   * @param actual the actual resource
   * @param desired the desired resource
   * @param actualMap the actual resource as map
   * @param desiredMap the desired resource as map
   * @param context the current context
   * @param defaultResult the match result determined by the
   *        {@link #matches(HasMetadata, HasMetadata, Context)} method
   * @return the {@code defaultResult}
   */
  @SuppressWarnings("unused")
  protected boolean matches(R actual, R desired,
      Map<String, Object> actualMap, Map<String, Object> desiredMap,
      Context<?> context, boolean defaultResult) {
    return defaultResult;
  }

EDIT: We could use then a customized SSA matcher in our public class StatefulSetResource extends CRUDKubernetesDependentResource<StatefulSet, HiveMQPlatform> in a custom match() method. I think we would prefer this over a more generic way to add more sanitizers via the operator config or something like that. We only need this for the StatefulSet, but no other resources.

But for that use-case we need to do a lot of customization to trigger specific states of our operator, depending on the changes that were detected (e.g. just a replicas change -> SCALING operation, just a log level change -> SET_LOG_LEVEL operation, image or config change -> ROLLING_RESTART operation). That's why I'd like to have access to the internal data structures, which are already cleaned up and sanitized, to analyze them further for these specific changes.

EDIT 2: We would also need access to the resources themselves, updated the code snippet.

I implemented a similar solution in one of our operators, making sanitizeState, keepOnlyManagedFields, and removeIrrelevantValues protected. However, I don't feel this is an ideal approach and haven't been able to work more on this, so I haven't created a PR.

The patched SSABasedGenericKubernetesResourceMatcher from my last comment works fine for our use-case.

The biggest issue is unit testing though. We run into the same problem like SSABasedGenericKubernetesResourceMatcherTest in this repo: To test the matches() method the actual instance needs a valid managedFields entry. For integration tests this isn't a problem, because there you have a real K8s cluster that will generate these fields automatically. For unit tests though there is no client-side implementation for this available (at least to my knowledge). Also the fabric8 mock webserver doesn't support SSA to provide the managed fields entry.

In this repo this was solved by using YAML files (and the ones I've added were dumped from a real K8s cluster). To do this programmatically you can create a valid managedFields entry like this:

    private static void setManagedFields(final HasMetadata resource) {
        final var managedFieldsEntry = new ManagedFieldsEntryBuilder().withApiVersion("apps/v1")
                .withFieldsType("FieldsV1")
                .withManager(CONTROLLER_NAME)
                .withOperation(SSABasedGenericKubernetesResourceMatcher.APPLY_OPERATION)
                .withNewFieldsV1()
                .addToAdditionalProperties(generateFieldsV1(resource))
                .endFieldsV1()
                .withTime(Instant.now().toString())
                .build();
        resource.getMetadata().setManagedFields(List.of(managedFieldsEntry));
    }

The problem though is the generateFieldsV1(resource) method, that would need to imitate the K8s server implementation to serialize the fields from the given resource. Especially a generic implementation that can handle all K8s resources seems to be quite hard to implement to me (not sure if that can be done without reflection to get all field names etc.). Not sure this is something we would like to add to the fabric8 kubernetes-client (maybe just to the mock webserver, so it's just available for testing).

For testing, I also attempted to find a programmatic solution but couldn't make it work, so we rely on YAML files for unit tests.

@Donnerbart's solution, using a protected matches method for custom logic, seems like a solid approach to me. It maintains compatibility with the current implementation and allows customization without overexposing functionality. Could this work as a solution?

public boolean matches(R actual, R desired, Context<?> context) {
  // Omitted logic

   var matches = matches(actual, desired, prunedActual, desiredMap, context);
   
    if (!matches && log.isDebugEnabled() && LoggingUtils.isNotSensitiveResource(desired)) {
      var diff = getDiff(prunedActual, desiredMap, objectMapper);
      log.debug(
          "Diff between actual and desired state for resource: {} with name: {} in namespace: {} is:\n{}",
          actual.getKind(), actual.getMetadata().getName(), actual.getMetadata().getNamespace(),
          diff);
    }
    return matches;
}


protected boolean matches(R actual, R desired,
    Map<String, Object> actualMap, Map<String, Object> desiredMap,
    Context<?> context) {
    // Custom logic
    return actualMap.equals(desiredMap);
}

I'm just playing around with this a little and we would actually need access to the primary resource. Not sure if we would need access to the raw actual and desired resource. But that one is not even handed into the parent match method right now.

So more like something like this? https://gist.github.com/Donnerbart/f8c15193f88009cecb8e8134dcdb8ccb

@Donnerbart, in your patch, I noticed you're also passing the primary resource. Are you relying on this for your matching logic?

EDIT:

I'm just playing around with this a little and we would actually need access to the primary resource

Seems like i missed this part. My bad.