From 4b68cf806796aa6a473febc1c0ab547b777b0c56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Wed, 29 Jul 2026 17:44:29 +0200 Subject: [PATCH] fix: honour bulk dependent resource capabilities during reconciliation A `BulkDependentResource` may implement any subset of `Creator`, `Updater` and `Deleter`. `BulkDependentResourceInstance`, the internal per-item wrapper that reuses `AbstractDependentResource`'s reconcile logic, implements all three and delegates each call to the bulk resource with an unchecked cast. Its capabilities therefore have to be derived from the wrapped bulk resource. 0b8d8dd8e introduced the overridable `creatable()` / `updatable()` hooks for exactly this, but the wiring was never completed: * `BulkDependentResourceInstance` overrides `isCreatable()` / `isUpdatable()`, which `reconcile` never consults, so both overrides were dead code. * the inner create branch of `AbstractDependentResource.reconcile` still read the `creatable` field directly instead of calling `creatable()`. Since the wrapper implements all three interfaces, both fields are always true, so create and update were attempted regardless of what the bulk resource actually supports, failing with a ClassCastException in `BulkDependentResourceInstance.create` / `update`: java.lang.ClassCastException: class ...CreateAndDeleteOnlyBulk cannot be cast to class ...Updater at BulkDependentResourceInstance.update(BulkDependentResourceReconciler.java:115) at AbstractDependentResource.handleUpdate(AbstractDependentResource.java:204) A non-bulk dependent in the same situation just logs "implement Updater interface to modify it" and skips the operation. Fixes this by overriding `creatable()` / `updatable()` in the wrapper and by using `creatable()` in the create branch. `isCreatable()` / `isUpdatable()` now delegate to `creatable()` / `updatable()` so the two pairs cannot drift apart again. Adds regression tests for a create+delete-only and a delete-only bulk dependent; both fail without this change. --- .../dependent/AbstractDependentResource.java | 6 +- .../BulkDependentResourceReconciler.java | 4 +- ...BulkDependentResourceCapabilitiesTest.java | 145 ++++++++++++++++++ 3 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceCapabilitiesTest.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java index 8dc62b4ca7..a96c4b03ec 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java @@ -85,7 +85,7 @@ public ReconcileResult reconcile(P primary, Context

context) { protected ReconcileResult reconcile(P primary, R actualResource, Context

context) { if (creatable() || updatable()) { if (actualResource == null) { - if (creatable) { + if (creatable()) { var desired = getOrComputeDesired(context); throwIfNull(desired, primary, "Desired"); logForOperation("Creating", primary, desired); @@ -244,12 +244,12 @@ protected void handleDelete(P primary, R secondary, Context

context) { } protected boolean isCreatable() { - return creatable; + return creatable(); } @SuppressWarnings("unused") protected boolean isUpdatable() { - return updatable; + return updatable(); } @Override diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java index 23135f81b1..827961b77f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java @@ -148,12 +148,12 @@ public R create(R desired, P primary, Context

context) { } @Override - protected boolean isCreatable() { + protected boolean creatable() { return bulkDependentResource instanceof Creator; } @Override - protected boolean isUpdatable() { + protected boolean updatable() { return bulkDependentResource instanceof Updater; } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceCapabilitiesTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceCapabilitiesTest.java new file mode 100644 index 0000000000..bfd8873d9d --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceCapabilitiesTest.java @@ -0,0 +1,145 @@ +/* + * Copyright Java Operator SDK 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 io.javaoperatorsdk.operator.processing.dependent; + +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +import org.junit.jupiter.api.Test; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ConfigMapBuilder; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter; +import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult.Operation; +import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +/** + * A {@link BulkDependentResource} does not have to implement every one of {@link Creator}, {@link + * Updater} and {@link Deleter}. The capabilities of the bulk resource itself must therefore be + * honoured, not those of the internal per-item wrapper (which implements all three). + */ +@SuppressWarnings("unchecked") +class BulkDependentResourceCapabilitiesTest { + + private static final ConfigMap PRIMARY = + new ConfigMapBuilder().withNewMetadata().withName("primary").endMetadata().build(); + + @Test + void doesNotAttemptUpdateWhenBulkResourceIsNotUpdater() { + var result = new CreateAndDeleteOnlyBulk().reconcile(PRIMARY, mock(Context.class)); + + assertThat(result.getSingleOperation()).isEqualTo(Operation.NONE); + } + + @Test + void doesNotAttemptCreateWhenBulkResourceIsNotCreator() { + var result = new DeleteOnlyBulk().reconcile(PRIMARY, mock(Context.class)); + + assertThat(result.getResourceOperations()).isEmpty(); + } + + /** Can be created and deleted, but deliberately is not an {@link Updater}. */ + private static class CreateAndDeleteOnlyBulk extends BaseBulk + implements Creator, Deleter { + + @Override + public Map getSecondaryResources( + ConfigMap primary, Context context) { + // an actual resource exists, but differs from the desired one + var actual = new LinkedHashMap(); + actual.put("a", configMap(Map.of("key", "actual"))); + return actual; + } + + @Override + public ConfigMap create(ConfigMap desired, ConfigMap primary, Context context) { + throw new AssertionError("create must not be called, the resource already exists"); + } + } + + /** Can only be deleted: neither {@link Creator} nor {@link Updater}. */ + private static class DeleteOnlyBulk extends BaseBulk implements Deleter { + + @Override + public Map getSecondaryResources( + ConfigMap primary, Context context) { + return Map.of(); + } + } + + private abstract static class BaseBulk extends AbstractDependentResource + implements BulkDependentResource { + + @Override + public Map desiredResources(ConfigMap primary, Context context) { + var desired = new LinkedHashMap(); + desired.put("a", configMap(Map.of("key", "desired"))); + return desired; + } + + @Override + public void deleteTargetResource( + ConfigMap primary, ConfigMap resource, String key, Context context) {} + + @Override + public Result match( + ConfigMap actualResource, + ConfigMap desired, + ConfigMap primary, + Context context) { + return Result.computed(false, desired); + } + + @Override + public Result match( + ConfigMap resource, ConfigMap primary, Context context) { + return Result.computed(false, resource); + } + + @Override + protected Optional selectTargetSecondaryResource( + Set secondaryResources, ConfigMap primary, Context context) { + return Optional.empty(); + } + + @Override + protected void onCreated(ConfigMap primary, ConfigMap created, Context context) {} + + @Override + protected void onUpdated( + ConfigMap primary, ConfigMap updated, ConfigMap actual, Context context) {} + + @Override + public Class resourceType() { + return ConfigMap.class; + } + } + + private static ConfigMap configMap(Map data) { + return new ConfigMapBuilder() + .withNewMetadata() + .withName("a") + .endMetadata() + .withData(data) + .build(); + } +}