From 36e8ab18b03ac8fadcfe8e56174af1e2cef42a08 Mon Sep 17 00:00:00 2001 From: csviri Date: Fri, 21 Oct 2022 15:57:15 +0200 Subject: [PATCH 01/25] feat: rbac and informer related behaviors --- .../operator/RBACBehaviorIT.java | 27 ++++++++ .../RBACBehaviorTestCustomResource.java | 15 +++++ .../RBACBehaviorTestReconciler.java | 65 +++++++++++++++++++ 3 files changed, 107 insertions(+) create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestCustomResource.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestReconciler.java diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java new file mode 100644 index 0000000000..1d8cf080f7 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java @@ -0,0 +1,27 @@ +package io.javaoperatorsdk.operator; + +import org.junit.jupiter.api.Test; + +public class RBACBehaviorIT { + + + @Test + void startsUpWhenNoPermission() { + + } + + @Test + void notStartsUpWithoutPermissionIfInstructed() { + + } + + @Test + void resilientForLoosingPermissionForCustomResource() { + + } + + @Test + void resilientForLoosingPermissionForSecondaryResource() { + + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestCustomResource.java new file mode 100644 index 0000000000..6c6879e7c3 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestCustomResource.java @@ -0,0 +1,15 @@ +package io.javaoperatorsdk.operator.sample.rbacbehavior; + +import io.fabric8.kubernetes.api.model.Namespaced; +import io.fabric8.kubernetes.client.CustomResource; +import io.fabric8.kubernetes.model.annotation.Group; +import io.fabric8.kubernetes.model.annotation.ShortNames; +import io.fabric8.kubernetes.model.annotation.Version; + +@Group("sample.javaoperatorsdk") +@Version("v1") +@ShortNames("rbt") +public class RBACBehaviorTestCustomResource + extends CustomResource + implements Namespaced { +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestReconciler.java new file mode 100644 index 0000000000..62235aea23 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestReconciler.java @@ -0,0 +1,65 @@ +package io.javaoperatorsdk.operator.sample.rbacbehavior; + +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ObjectMeta; +import io.fabric8.kubernetes.client.KubernetesClient; +import io.javaoperatorsdk.operator.api.reconciler.*; +import io.javaoperatorsdk.operator.junit.KubernetesClientAware; +import io.javaoperatorsdk.operator.processing.event.source.EventSource; +import io.javaoperatorsdk.operator.support.TestExecutionInfoProvider; + +@ControllerConfiguration +public class RBACBehaviorTestReconciler + implements Reconciler, TestExecutionInfoProvider, + EventSourceInitializer, KubernetesClientAware { + + private final AtomicInteger numberOfExecutions = new AtomicInteger(0); + private KubernetesClient client; + + @Override + public UpdateControl reconcile( + RBACBehaviorTestCustomResource resource, + Context context) { + + return UpdateControl.noUpdate(); + } + + public int getNumberOfExecutions() { + return numberOfExecutions.get(); + } + + @Override + public Map prepareEventSources( + EventSourceContext context) { + + return EventSourceInitializer.nameEventSources(); + } + + ConfigMap configMap(String name, RBACBehaviorTestCustomResource resource) { + ConfigMap configMap = new ConfigMap(); + configMap.setMetadata(new ObjectMeta()); + configMap.getMetadata().setName(name); + configMap.getMetadata().setNamespace(resource.getMetadata().getNamespace()); + configMap.setData(new HashMap<>()); + configMap.getData().put(name, name); + HashMap labels = new HashMap<>(); + labels.put("multisecondary", "true"); + configMap.getMetadata().setLabels(labels); + configMap.addOwnerReference(resource); + return configMap; + } + + @Override + public KubernetesClient getKubernetesClient() { + return client; + } + + @Override + public void setKubernetesClient(KubernetesClient kubernetesClient) { + this.client = kubernetesClient; + } +} From 671c2c0385e68756cb803db269a85d75bb8ade73 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 24 Oct 2022 15:27:46 +0200 Subject: [PATCH 02/25] wip --- .../junit/LocallyRunOperatorExtension.java | 16 ++- .../operator/RBACBehaviorIT.java | 134 +++++++++++++++++- .../ConfigMapDependentResource.java | 35 +++++ .../RBACBehaviorTestReconciler.java | 50 ++----- .../operator/rback-test-full-access-role.yaml | 14 ++ .../operator/rback-test-no-cr-access.yaml | 10 ++ .../operator/rback-test-role-binding.yaml | 13 ++ 7 files changed, 225 insertions(+), 47 deletions(-) create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/ConfigMapDependentResource.java create mode 100644 operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml create mode 100644 operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-cr-access.yaml create mode 100644 operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-role-binding.yaml diff --git a/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java b/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java index 8f4d62341c..e852455ebf 100644 --- a/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java +++ b/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java @@ -101,6 +101,10 @@ public RegisteredController getRegisteredControllerForReconcile( return registeredControllers.get(getReconcilerOfType(type)); } + public Operator getOperator() { + return operator; + } + @SuppressWarnings("unchecked") @Override protected void before(ExtensionContext context) { @@ -159,12 +163,20 @@ protected void before(ExtensionContext context) { } private void applyCrd(String resourceTypeName) { + applyCrd(resourceTypeName, getKubernetesClient()); + } + + public static void applyCrd(Class resourceClass, KubernetesClient client) { + applyCrd(ReconcilerUtils.getResourceTypeName(resourceClass), client); + } + + public static void applyCrd(String resourceTypeName, KubernetesClient client) { String path = "/META-INF/fabric8/" + resourceTypeName + "-v1.yml"; - try (InputStream is = getClass().getResourceAsStream(path)) { + try (InputStream is = LocallyRunOperatorExtension.class.getResourceAsStream(path)) { if (is == null) { throw new IllegalStateException("Cannot find CRD at " + path); } - final var crd = getKubernetesClient().load(is); + final var crd = client.load(is); crd.createOrReplace(); Thread.sleep(CRD_READY_WAIT); // readiness is not applicable for CRD, just wait a little LOGGER.debug("Applied CRD with path: {}", path); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java index 1d8cf080f7..03682fdefe 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java @@ -1,27 +1,153 @@ package io.javaoperatorsdk.operator; +import java.time.Duration; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; + +import io.fabric8.kubernetes.api.model.Namespace; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.fabric8.kubernetes.api.model.rbac.ClusterRole; +import io.fabric8.kubernetes.api.model.rbac.ClusterRoleBinding; +import io.fabric8.kubernetes.client.ConfigBuilder; +import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.client.KubernetesClientBuilder; +import io.fabric8.kubernetes.client.utils.KubernetesResourceUtil; +import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; +import io.javaoperatorsdk.operator.sample.rbacbehavior.RBACBehaviorTestCustomResource; +import io.javaoperatorsdk.operator.sample.rbacbehavior.RBACBehaviorTestReconciler; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +class RBACBehaviorIT { + + // https://junit.org/junit5/docs/5.1.1/api/org/junit/jupiter/api/extension/TestInstancePostProcessor.html -public class RBACBehaviorIT { + KubernetesClient adminClient = new KubernetesClientBuilder().build(); + RBACBehaviorTestReconciler reconciler; + String actualNamespace; + @BeforeEach + void beforeEach(TestInfo testInfo) { + testInfo.getTestMethod().ifPresent(method -> { + actualNamespace = KubernetesResourceUtil.sanitizeName(method.getName()); + adminClient.resource(namespace()).createOrReplace(); + }); + + + } + + @AfterEach + void cleanup() { + adminClient.resource(testCustomResource()).delete(); + adminClient.resource(namespace()).delete(); + } @Test void startsUpWhenNoPermission() { + LocallyRunOperatorExtension.applyCrd(RBACBehaviorTestCustomResource.class, adminClient); + adminClient.resource(testCustomResource()).createOrReplace(); + // todo it should not throw exception + fullResourcesAccess(); + startOperator(); + + assertReconciled(); } + @Test - void notStartsUpWithoutPermissionIfInstructed() { + void resilientForLoosingPermissionForCustomResource() throws InterruptedException { + LocallyRunOperatorExtension.applyCrd(RBACBehaviorTestCustomResource.class, adminClient); + fullResourcesAccess(); + startOperator(); + noCustomResourceAccess(); + + Thread.sleep(3000); + adminClient.resource(testCustomResource()).createOrReplace(); + + await().pollDelay(Duration.ofMillis(300)).untilAsserted(() -> { + assertThat(reconciler.getNumberOfExecutions()).isEqualTo(0); + }); } @Test - void resilientForLoosingPermissionForCustomResource() { + void resilientForLoosingPermissionForSecondaryResource() { } @Test - void resilientForLoosingPermissionForSecondaryResource() { + void notStartsUpWithoutPermissionIfInstructed() { + + } + + @Test + RBACBehaviorTestCustomResource testCustomResource() { + RBACBehaviorTestCustomResource testCustomResource = new RBACBehaviorTestCustomResource(); + testCustomResource.setMetadata(new ObjectMetaBuilder() + .withNamespace(actualNamespace) + .withName("test1") + .build()); + return testCustomResource; + } + + + private void assertReconciled() { + await().untilAsserted(() -> { + assertThat(reconciler.getNumberOfExecutions()).isGreaterThan(0); + }); + } + + KubernetesClient clientUsingServiceAccount() { + KubernetesClient client = new KubernetesClientBuilder() + .withConfig(new ConfigBuilder() + .withImpersonateUsername("rbac-test-user") + .withNamespace(actualNamespace) + .build()) + .build(); + return client; + } + + Operator startOperator() { + reconciler = new RBACBehaviorTestReconciler(); + Operator operator = new Operator(clientUsingServiceAccount()); + operator.register(reconciler); + operator.installShutdownHook(); + operator.start(); + return operator; + } + + private void noCustomResourceAccess() { + applyClusterRole("rback-test-no-cr-access.yaml"); + applyClusterRoleBinding(); + } + + private void fullResourcesAccess() { + applyClusterRole("rback-test-full-access-role.yaml"); + applyClusterRoleBinding(); + } + + private void applyClusterRoleBinding() { + var clusterRoleBinding = ReconcilerUtils + .loadYaml(ClusterRoleBinding.class, this.getClass(), "rback-test-role-binding.yaml"); + adminClient.resource(clusterRoleBinding).createOrReplace(); + } + + private void applyClusterRole(String filename) { + var clusterRole = ReconcilerUtils + .loadYaml(ClusterRole.class, this.getClass(), filename); + adminClient.resource(clusterRole).createOrReplace(); + } + private Namespace namespace() { + Namespace n = new Namespace(); + n.setMetadata(new ObjectMetaBuilder() + .withName(actualNamespace) + .build()); + return n; } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/ConfigMapDependentResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/ConfigMapDependentResource.java new file mode 100644 index 0000000000..463543835f --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/ConfigMapDependentResource.java @@ -0,0 +1,35 @@ +package io.javaoperatorsdk.operator.sample.rbacbehavior; + +import java.util.Map; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ConfigMapBuilder; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.CRUDKubernetesDependentResource; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent; + +@KubernetesDependent(labelSelector = "app=rbac-test") +public class ConfigMapDependentResource + extends CRUDKubernetesDependentResource { + + public static final String DATA_KEY = "key"; + + public ConfigMapDependentResource() { + super(ConfigMap.class); + } + + @Override + protected ConfigMap desired(RBACBehaviorTestCustomResource primary, + Context context) { + return new ConfigMapBuilder() + .withMetadata(new ObjectMetaBuilder() + .withLabels(Map.of("app", "rbac-test")) + .withName(primary.getMetadata().getName()) + .withNamespace(primary.getMetadata().getNamespace()) + .build()) + .withData(Map.of(DATA_KEY, primary.getMetadata().getName())) + .build(); + + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestReconciler.java index 62235aea23..0f71e49b46 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestReconciler.java @@ -1,21 +1,19 @@ package io.javaoperatorsdk.operator.sample.rbacbehavior; -import java.util.HashMap; -import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; -import io.fabric8.kubernetes.api.model.ConfigMap; -import io.fabric8.kubernetes.api.model.ObjectMeta; import io.fabric8.kubernetes.client.KubernetesClient; -import io.javaoperatorsdk.operator.api.reconciler.*; -import io.javaoperatorsdk.operator.junit.KubernetesClientAware; -import io.javaoperatorsdk.operator.processing.event.source.EventSource; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; +import io.javaoperatorsdk.operator.api.reconciler.dependent.Dependent; import io.javaoperatorsdk.operator.support.TestExecutionInfoProvider; -@ControllerConfiguration +@ControllerConfiguration(dependents = @Dependent(type = ConfigMapDependentResource.class)) public class RBACBehaviorTestReconciler - implements Reconciler, TestExecutionInfoProvider, - EventSourceInitializer, KubernetesClientAware { + implements Reconciler, TestExecutionInfoProvider { + private final AtomicInteger numberOfExecutions = new AtomicInteger(0); private KubernetesClient client; @@ -24,7 +22,7 @@ public class RBACBehaviorTestReconciler public UpdateControl reconcile( RBACBehaviorTestCustomResource resource, Context context) { - + numberOfExecutions.addAndGet(1); return UpdateControl.noUpdate(); } @@ -32,34 +30,4 @@ public int getNumberOfExecutions() { return numberOfExecutions.get(); } - @Override - public Map prepareEventSources( - EventSourceContext context) { - - return EventSourceInitializer.nameEventSources(); - } - - ConfigMap configMap(String name, RBACBehaviorTestCustomResource resource) { - ConfigMap configMap = new ConfigMap(); - configMap.setMetadata(new ObjectMeta()); - configMap.getMetadata().setName(name); - configMap.getMetadata().setNamespace(resource.getMetadata().getNamespace()); - configMap.setData(new HashMap<>()); - configMap.getData().put(name, name); - HashMap labels = new HashMap<>(); - labels.put("multisecondary", "true"); - configMap.getMetadata().setLabels(labels); - configMap.addOwnerReference(resource); - return configMap; - } - - @Override - public KubernetesClient getKubernetesClient() { - return client; - } - - @Override - public void setKubernetesClient(KubernetesClient kubernetesClient) { - this.client = kubernetesClient; - } } diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml new file mode 100644 index 0000000000..ff8f79f559 --- /dev/null +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml @@ -0,0 +1,14 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + # "namespace" omitted since ClusterRoles are not namespaced + name: rbac-behavior +rules: + - apiGroups: [ "sample.javaoperatorsdk" ] + resources: [ "rbacbehaviortestcustomresources" ] + verbs: [ "get", "watch", "list","post", "delete" ] + - apiGroups: [""] + resources: [ "configmaps" ] + verbs: [ "get", "watch", "list","post", "delete","create" ] + + diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-cr-access.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-cr-access.yaml new file mode 100644 index 0000000000..8c6ae85aac --- /dev/null +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-cr-access.yaml @@ -0,0 +1,10 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + # "namespace" omitted since ClusterRoles are not namespaced + name: rbac-behavior +rules: + - apiGroups: [""] + resources: [ "configmaps" ] + verbs: [ "get", "watch", "list","post", "delete","create" ] + diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-role-binding.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-role-binding.yaml new file mode 100644 index 0000000000..5b9c9ee2d9 --- /dev/null +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-role-binding.yaml @@ -0,0 +1,13 @@ +apiVersion: rbac.authorization.k8s.io/v1 +# This cluster role binding allows anyone in the "manager" group to read secrets in any namespace. +kind: ClusterRoleBinding +metadata: + name: read-secrets-global +subjects: + - kind: User + name: rbac-test-user + apiGroup: rbac.authorization.k8s.io +roleRef: + kind: ClusterRole + name: rbac-behavior + apiGroup: rbac.authorization.k8s.io \ No newline at end of file From 0ab11b9781dcc6dd26b2a1cb28182ff90787a062 Mon Sep 17 00:00:00 2001 From: csviri Date: Mon, 24 Oct 2022 17:22:00 +0200 Subject: [PATCH 03/25] comment --- .../test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java index 03682fdefe..ce65fe1e44 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java @@ -25,6 +25,7 @@ class RBACBehaviorIT { // https://junit.org/junit5/docs/5.1.1/api/org/junit/jupiter/api/extension/TestInstancePostProcessor.html + // minikube start --extra-config=apiserver.min-request-timeout=3 KubernetesClient adminClient = new KubernetesClientBuilder().build(); RBACBehaviorTestReconciler reconciler; @@ -66,7 +67,7 @@ void resilientForLoosingPermissionForCustomResource() throws InterruptedExceptio startOperator(); noCustomResourceAccess(); - Thread.sleep(3000); + Thread.sleep(5000); adminClient.resource(testCustomResource()).createOrReplace(); await().pollDelay(Duration.ofMillis(300)).untilAsserted(() -> { From 56a00339a8d1498eed60e45b9fe422bc27c502ec Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 25 Oct 2022 10:23:15 +0200 Subject: [PATCH 04/25] wip --- .../operator/RBACBehaviorIT.java | 57 +++++++++++++++---- .../rback-test-no-configmap-access.yaml | 11 ++++ 2 files changed, 58 insertions(+), 10 deletions(-) create mode 100644 operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-configmap-access.yaml diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java index ce65fe1e44..db81d881d8 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java @@ -23,6 +23,7 @@ import static org.awaitility.Awaitility.await; class RBACBehaviorIT { + public static final String TEST_RESOURCE_NAME = "test1"; // https://junit.org/junit5/docs/5.1.1/api/org/junit/jupiter/api/extension/TestInstancePostProcessor.html // minikube start --extra-config=apiserver.min-request-timeout=3 @@ -33,6 +34,7 @@ class RBACBehaviorIT { @BeforeEach void beforeEach(TestInfo testInfo) { + LocallyRunOperatorExtension.applyCrd(RBACBehaviorTestCustomResource.class, adminClient); testInfo.getTestMethod().ifPresent(method -> { actualNamespace = KubernetesResourceUtil.sanitizeName(method.getName()); adminClient.resource(namespace()).createOrReplace(); @@ -48,37 +50,64 @@ void cleanup() { } @Test - void startsUpWhenNoPermission() { - LocallyRunOperatorExtension.applyCrd(RBACBehaviorTestCustomResource.class, adminClient); + void startsUpWhenNoPermissionToCustomResource() { + adminClient.resource(testCustomResource()).createOrReplace(); // todo it should not throw exception - fullResourcesAccess(); + setFullResourcesAccess(); startOperator(); assertReconciled(); } + @Test + void startsUpWhenNoPermissionToSecondaryResource() { + + } @Test void resilientForLoosingPermissionForCustomResource() throws InterruptedException { - LocallyRunOperatorExtension.applyCrd(RBACBehaviorTestCustomResource.class, adminClient); - fullResourcesAccess(); + setFullResourcesAccess(); startOperator(); - noCustomResourceAccess(); + setNoCustomResourceAccess(); - Thread.sleep(5000); + waitForWatchReconnect(); adminClient.resource(testCustomResource()).createOrReplace(); await().pollDelay(Duration.ofMillis(300)).untilAsserted(() -> { assertThat(reconciler.getNumberOfExecutions()).isEqualTo(0); }); + setFullResourcesAccess(); + assertReconciled(); } @Test void resilientForLoosingPermissionForSecondaryResource() { + setFullResourcesAccess(); + startOperator(); + setNoConfigMapAccess(); + + waitForWatchReconnect(); + adminClient.resource(testCustomResource()).createOrReplace(); + + await().pollDelay(Duration.ofMillis(300)).untilAsserted(() -> { + var cm = + adminClient.configMaps().inNamespace(actualNamespace).withName(TEST_RESOURCE_NAME).get(); + assertThat(cm).isNull(); + }); + + setFullResourcesAccess(); + assertReconciled(); + } + private static void waitForWatchReconnect() { + try { + Thread.sleep(5000); + } catch (InterruptedException e) { + throw new IllegalStateException(e); + } } @Test @@ -91,7 +120,7 @@ RBACBehaviorTestCustomResource testCustomResource() { RBACBehaviorTestCustomResource testCustomResource = new RBACBehaviorTestCustomResource(); testCustomResource.setMetadata(new ObjectMetaBuilder() .withNamespace(actualNamespace) - .withName("test1") + .withName(TEST_RESOURCE_NAME) .build()); return testCustomResource; } @@ -100,6 +129,9 @@ RBACBehaviorTestCustomResource testCustomResource() { private void assertReconciled() { await().untilAsserted(() -> { assertThat(reconciler.getNumberOfExecutions()).isGreaterThan(0); + var cm = + adminClient.configMaps().inNamespace(actualNamespace).withName(TEST_RESOURCE_NAME).get(); + assertThat(cm).isNotNull(); }); } @@ -122,12 +154,17 @@ Operator startOperator() { return operator; } - private void noCustomResourceAccess() { + private void setNoConfigMapAccess() { + applyClusterRole("rback-test-no-configmap-access.yaml"); + applyClusterRoleBinding(); + } + + private void setNoCustomResourceAccess() { applyClusterRole("rback-test-no-cr-access.yaml"); applyClusterRoleBinding(); } - private void fullResourcesAccess() { + private void setFullResourcesAccess() { applyClusterRole("rback-test-full-access-role.yaml"); applyClusterRoleBinding(); } diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-configmap-access.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-configmap-access.yaml new file mode 100644 index 0000000000..16e52add81 --- /dev/null +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-configmap-access.yaml @@ -0,0 +1,11 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + # "namespace" omitted since ClusterRoles are not namespaced + name: rbac-behavior +rules: + - apiGroups: [ "sample.javaoperatorsdk" ] + resources: [ "rbacbehaviortestcustomresources" ] + verbs: [ "get", "watch", "list","post", "delete" ] + + From f80575c42e297b7e8af1e52eed1832e91125640a Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 25 Oct 2022 12:45:54 +0200 Subject: [PATCH 05/25] wip --- .../test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java index db81d881d8..2e7b6a544b 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java @@ -104,7 +104,7 @@ void resilientForLoosingPermissionForSecondaryResource() { private static void waitForWatchReconnect() { try { - Thread.sleep(5000); + Thread.sleep(6000); } catch (InterruptedException e) { throw new IllegalStateException(e); } From 0e4ccc0b2ad72418efa8d451d6e9774e2dd1717c Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 25 Oct 2022 12:46:40 +0200 Subject: [PATCH 06/25] wip --- .../test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java index 2e7b6a544b..2fdfb8ee80 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java @@ -114,8 +114,7 @@ private static void waitForWatchReconnect() { void notStartsUpWithoutPermissionIfInstructed() { } - - @Test + RBACBehaviorTestCustomResource testCustomResource() { RBACBehaviorTestCustomResource testCustomResource = new RBACBehaviorTestCustomResource(); testCustomResource.setMetadata(new ObjectMetaBuilder() From 756d1f7639096c334eca4462981516f96116700b Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 25 Oct 2022 13:04:06 +0200 Subject: [PATCH 07/25] wip --- .../java/io/javaoperatorsdk/operator/RBACBehaviorIT.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java index 2fdfb8ee80..b84e37f51d 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java @@ -53,8 +53,8 @@ void cleanup() { void startsUpWhenNoPermissionToCustomResource() { adminClient.resource(testCustomResource()).createOrReplace(); - // todo it should not throw exception - setFullResourcesAccess(); + setNoCustomResourceAccess(); + // setFullResourcesAccess(); startOperator(); @@ -114,7 +114,7 @@ private static void waitForWatchReconnect() { void notStartsUpWithoutPermissionIfInstructed() { } - + RBACBehaviorTestCustomResource testCustomResource() { RBACBehaviorTestCustomResource testCustomResource = new RBACBehaviorTestCustomResource(); testCustomResource.setMetadata(new ObjectMetaBuilder() From 0243b8b878ffd247632a138ab08c153c29c83b45 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 25 Oct 2022 14:13:24 +0200 Subject: [PATCH 08/25] wip --- .../api/config/ConfigurationService.java | 4 ++ .../config/ConfigurationServiceOverrider.java | 13 ++++++ .../source/informer/InformerWrapper.java | 13 ++++-- .../operator/RBACBehaviorIT.java | 43 +++++++++++++------ .../operator/rback-test-full-access-role.yaml | 4 +- 5 files changed, 60 insertions(+), 17 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 72099f7fcb..6ea1f143f5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -152,6 +152,10 @@ default Optional getLeaderElectionConfiguration() { return Optional.empty(); } + default boolean stopOnInformerErrorDuringStartup() { + return true; + } + default Optional getInformerStoppedHandler() { return Optional.of((informer, ex) -> { if (ex != null) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index ee442c07fa..97f0db1967 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -27,6 +27,7 @@ public class ConfigurationServiceOverrider { private ExecutorService workflowExecutorService; private LeaderElectionConfiguration leaderElectionConfiguration; private InformerStoppedHandler informerStoppedHandler; + private Boolean stopOnInformerErrorDuringStartup; ConfigurationServiceOverrider(ConfigurationService original) { this.original = original; @@ -99,6 +100,12 @@ public ConfigurationServiceOverrider withInformerStoppedHandler(InformerStoppedH return this; } + public ConfigurationServiceOverrider withStopOnInformerErrorDuringStartup( + Boolean stopOnInformerErrorDuringStartup) { + this.stopOnInformerErrorDuringStartup = stopOnInformerErrorDuringStartup; + return this; + } + public ConfigurationService build() { return new BaseConfigurationService(original.getVersion(), cloner, objectMapper) { @Override @@ -171,6 +178,12 @@ public Optional getInformerStoppedHandler() { return informerStoppedHandler != null ? Optional.of(informerStoppedHandler) : original.getInformerStoppedHandler(); } + + @Override + public boolean stopOnInformerErrorDuringStartup() { + return stopOnInformerErrorDuringStartup != null ? stopOnInformerErrorDuringStartup + : super.stopOnInformerErrorDuringStartup(); + } }; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java index 90bf01267c..5f7c7e127c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java @@ -37,10 +37,17 @@ public InformerWrapper(SharedIndexInformer informer) { @Override public void start() throws OperatorException { try { - informer.run(); - + var configService = ConfigurationServiceProvider.instance(); + if (configService.stopOnInformerErrorDuringStartup()) { + informer.run(); + } else { + informer.start().exceptionally((e) -> { + log.error("Error", e); + return null; + }); + } // register stopped handler if we have one defined - ConfigurationServiceProvider.instance().getInformerStoppedHandler() + configService.getInformerStoppedHandler() .ifPresent(ish -> { final var stopped = informer.stopped(); if (stopped != null) { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java index b84e37f51d..732cfc134b 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java @@ -6,6 +6,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInfo; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.api.model.Namespace; import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; @@ -23,6 +25,9 @@ import static org.awaitility.Awaitility.await; class RBACBehaviorIT { + + private final static Logger log = LoggerFactory.getLogger(RBACBehaviorIT.class); + public static final String TEST_RESOURCE_NAME = "test1"; // https://junit.org/junit5/docs/5.1.1/api/org/junit/jupiter/api/extension/TestInstancePostProcessor.html @@ -39,8 +44,6 @@ void beforeEach(TestInfo testInfo) { actualNamespace = KubernetesResourceUtil.sanitizeName(method.getName()); adminClient.resource(namespace()).createOrReplace(); }); - - } @AfterEach @@ -51,13 +54,15 @@ void cleanup() { @Test void startsUpWhenNoPermissionToCustomResource() { - adminClient.resource(testCustomResource()).createOrReplace(); setNoCustomResourceAccess(); - // setFullResourcesAccess(); - startOperator(); + startOperator(false); + assertNotReconciled(); + + setFullResourcesAccess(); + waitForWatchReconnect(); assertReconciled(); } @@ -69,24 +74,23 @@ void startsUpWhenNoPermissionToSecondaryResource() { @Test void resilientForLoosingPermissionForCustomResource() throws InterruptedException { setFullResourcesAccess(); - startOperator(); + startOperator(true); setNoCustomResourceAccess(); waitForWatchReconnect(); adminClient.resource(testCustomResource()).createOrReplace(); - await().pollDelay(Duration.ofMillis(300)).untilAsserted(() -> { - assertThat(reconciler.getNumberOfExecutions()).isEqualTo(0); - }); + assertNotReconciled(); setFullResourcesAccess(); assertReconciled(); } + @Test void resilientForLoosingPermissionForSecondaryResource() { setFullResourcesAccess(); - startOperator(); + startOperator(true); setNoConfigMapAccess(); waitForWatchReconnect(); @@ -110,6 +114,13 @@ private static void waitForWatchReconnect() { } } + + private void assertNotReconciled() { + await().pollDelay(Duration.ofMillis(2000)).untilAsserted(() -> { + assertThat(reconciler.getNumberOfExecutions()).isEqualTo(0); + }); + } + @Test void notStartsUpWithoutPermissionIfInstructed() { @@ -144,9 +155,17 @@ KubernetesClient clientUsingServiceAccount() { return client; } - Operator startOperator() { + Operator startOperator(boolean stopOnInformerErrorDuringStartup) { reconciler = new RBACBehaviorTestReconciler(); - Operator operator = new Operator(clientUsingServiceAccount()); + + Operator operator = new Operator(clientUsingServiceAccount(), + co -> { + co.withStopOnInformerErrorDuringStartup(stopOnInformerErrorDuringStartup); + co.withInformerStoppedHandler((informer, ex) -> { + informer.start(); + log.error("Stop handler: {}", informer, ex); + }); + }); operator.register(reconciler); operator.installShutdownHook(); operator.start(); diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml index ff8f79f559..c27febb067 100644 --- a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml @@ -7,8 +7,8 @@ rules: - apiGroups: [ "sample.javaoperatorsdk" ] resources: [ "rbacbehaviortestcustomresources" ] verbs: [ "get", "watch", "list","post", "delete" ] - - apiGroups: [""] + - apiGroups: [ "" ] resources: [ "configmaps" ] - verbs: [ "get", "watch", "list","post", "delete","create" ] + verbs: [ "get", "watch", "list","post", "delete", "create" ] From cb269ff0635db5859934fc310ff3214bb8a6ea23 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 25 Oct 2022 16:26:36 +0200 Subject: [PATCH 09/25] wp --- .../source/informer/InformerWrapper.java | 29 ++++++++++++++----- .../operator/MockKubernetesClient.java | 18 +++--------- .../informer/InformerEventSourceTest.java | 3 +- .../operator/RBACBehaviorIT.java | 26 +++++++++++++---- 4 files changed, 47 insertions(+), 29 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java index 5f7c7e127c..3b11b7755d 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java @@ -3,6 +3,9 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Stream; @@ -11,6 +14,7 @@ import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.client.informers.ExceptionHandler; import io.fabric8.kubernetes.client.informers.ResourceEventHandler; import io.fabric8.kubernetes.client.informers.SharedIndexInformer; import io.fabric8.kubernetes.client.informers.cache.Cache; @@ -38,14 +42,6 @@ public InformerWrapper(SharedIndexInformer informer) { public void start() throws OperatorException { try { var configService = ConfigurationServiceProvider.instance(); - if (configService.stopOnInformerErrorDuringStartup()) { - informer.run(); - } else { - informer.start().exceptionally((e) -> { - log.error("Error", e); - return null; - }); - } // register stopped handler if we have one defined configService.getInformerStoppedHandler() .ifPresent(ish -> { @@ -66,6 +62,23 @@ public void start() throws OperatorException { } }); + if (!configService.stopOnInformerErrorDuringStartup()) { + informer.exceptionHandler((b, t) -> !ExceptionHandler.isDeserializationException(t)); + } + try { + var start = informer.start(); + // todo configurable timeout, also timeout for run? + start.toCompletableFuture().get(5000, TimeUnit.MILLISECONDS); + } catch (TimeoutException | ExecutionException e) { + log.warn("Informer startup error. Informer: {}", informer, e); + if (configService.stopOnInformerErrorDuringStartup()) { + throw new OperatorException(e); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException(e); + } + } catch (Exception e) { log.error("Couldn't start informer for " + versionedFullResourceName() + " resources", e); ReconcilerUtils.handleKubernetesClientException(e, diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/MockKubernetesClient.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/MockKubernetesClient.java index 2589f566da..076e13b15b 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/MockKubernetesClient.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/MockKubernetesClient.java @@ -17,7 +17,6 @@ import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.nullable; -import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -44,19 +43,10 @@ public static KubernetesClient client(Class clazz, when(resources.inAnyNamespace()).thenReturn(inAnyNamespace); when(inAnyNamespace.withLabelSelector(nullable(String.class))).thenReturn(filterable); SharedIndexInformer informer = mock(SharedIndexInformer.class); - CompletableFuture stopped = new CompletableFuture<>(); - when(informer.stopped()).thenReturn(stopped); - if (informerRunBehavior != null) { - doAnswer(invocation -> { - try { - informerRunBehavior.accept(null); - } catch (Exception e) { - stopped.completeExceptionally(e); - } - return null; - }).when(informer).run(); - } - doAnswer(invocation -> null).when(informer).stop(); + CompletableFuture informerStartRes = new CompletableFuture<>(); + informerStartRes.complete(null); + when(informer.start()).thenReturn(informerStartRes); + Indexer mockIndexer = mock(Indexer.class); when(informer.getIndexer()).thenReturn(mockIndexer); when(filterable.runnableInformer(anyLong())).thenReturn(informer); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java index e941e8390d..355dd23bb3 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java @@ -243,7 +243,8 @@ void filtersOnDeleteEvents() { verify(eventHandlerMock, never()).handleEvent(any()); } - @Test + // todo add integration test + // @Test void informerStoppedHandlerShouldBeCalledWhenInformerStops() { try { final var exception = new RuntimeException("Informer stopped exceptionally!"); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java index 732cfc134b..0a998b485f 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java @@ -17,12 +17,14 @@ import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.KubernetesClientBuilder; import io.fabric8.kubernetes.client.utils.KubernetesResourceUtil; +import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; import io.javaoperatorsdk.operator.sample.rbacbehavior.RBACBehaviorTestCustomResource; import io.javaoperatorsdk.operator.sample.rbacbehavior.RBACBehaviorTestReconciler; import static org.assertj.core.api.Assertions.assertThat; import static org.awaitility.Awaitility.await; +import static org.junit.jupiter.api.Assertions.assertThrows; class RBACBehaviorIT { @@ -52,13 +54,21 @@ void cleanup() { adminClient.resource(namespace()).delete(); } + @Test + void notStartsUpWithoutPermissionIfInstructed() { + adminClient.resource(testCustomResource()).createOrReplace(); + setNoCustomResourceAccess(); + + assertThrows(OperatorException.class, () -> startOperator(true)); + assertNotReconciled(); + } + @Test void startsUpWhenNoPermissionToCustomResource() { adminClient.resource(testCustomResource()).createOrReplace(); setNoCustomResourceAccess(); startOperator(false); - assertNotReconciled(); setFullResourcesAccess(); @@ -68,7 +78,15 @@ void startsUpWhenNoPermissionToCustomResource() { @Test void startsUpWhenNoPermissionToSecondaryResource() { + adminClient.resource(testCustomResource()).createOrReplace(); + setNoConfigMapAccess(); + startOperator(false); + assertNotReconciled(); + + setFullResourcesAccess(); + waitForWatchReconnect(); + assertReconciled(); } @Test @@ -121,11 +139,6 @@ private void assertNotReconciled() { }); } - @Test - void notStartsUpWithoutPermissionIfInstructed() { - - } - RBACBehaviorTestCustomResource testCustomResource() { RBACBehaviorTestCustomResource testCustomResource = new RBACBehaviorTestCustomResource(); testCustomResource.setMetadata(new ObjectMetaBuilder() @@ -156,6 +169,7 @@ KubernetesClient clientUsingServiceAccount() { } Operator startOperator(boolean stopOnInformerErrorDuringStartup) { + ConfigurationServiceProvider.reset(); reconciler = new RBACBehaviorTestReconciler(); Operator operator = new Operator(clientUsingServiceAccount(), From 6783d41a35e0b1230c922385108f606a340f9003 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 26 Oct 2022 11:01:07 +0200 Subject: [PATCH 10/25] tests --- .../api/config/ConfigurationService.java | 8 ++++ .../config/ConfigurationServiceOverrider.java | 13 ++++++- .../source/informer/InformerWrapper.java | 4 +- .../operator/MockKubernetesClient.java | 19 ++++++++-- .../informer/InformerEventSourceTest.java | 7 ++-- ...IT.java => InformerRelatedBehaviorIT.java} | 38 ++++++++++++------- .../ConfigMapDependentResource.java | 8 ++-- ...merRelatedBehaviorTestCustomResource.java} | 4 +- ...nformerRelatedBehaviorTestReconciler.java} | 12 +++--- .../operator/rback-test-full-access-role.yaml | 2 +- .../rback-test-no-configmap-access.yaml | 2 +- 11 files changed, 79 insertions(+), 38 deletions(-) rename operator-framework/src/test/java/io/javaoperatorsdk/operator/{RBACBehaviorIT.java => InformerRelatedBehaviorIT.java} (83%) rename operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/{rbacbehavior => informerrelatedbehavior}/ConfigMapDependentResource.java (76%) rename operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/{rbacbehavior/RBACBehaviorTestCustomResource.java => informerrelatedbehavior/InformerRelatedBehaviorTestCustomResource.java} (76%) rename operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/{rbacbehavior/RBACBehaviorTestReconciler.java => informerrelatedbehavior/InformerRelatedBehaviorTestReconciler.java} (67%) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 6ea1f143f5..fc61a80392 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -156,6 +156,14 @@ default boolean stopOnInformerErrorDuringStartup() { return true; } + /** + * Timeout for cache sync in milliseconds. In other words source start timeout. Note that is + * "stopOnInformerErrorDuringStartup" is true, this will be resulted in an operator stop. + */ + default int cacheSyncTimeout() { + return 2 * 60 * 1000; + } + default Optional getInformerStoppedHandler() { return Optional.of((informer, ex) -> { if (ex != null) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index 97f0db1967..9cb9dae521 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -28,6 +28,7 @@ public class ConfigurationServiceOverrider { private LeaderElectionConfiguration leaderElectionConfiguration; private InformerStoppedHandler informerStoppedHandler; private Boolean stopOnInformerErrorDuringStartup; + private Integer cacheSyncTimeout; ConfigurationServiceOverrider(ConfigurationService original) { this.original = original; @@ -101,11 +102,16 @@ public ConfigurationServiceOverrider withInformerStoppedHandler(InformerStoppedH } public ConfigurationServiceOverrider withStopOnInformerErrorDuringStartup( - Boolean stopOnInformerErrorDuringStartup) { + boolean stopOnInformerErrorDuringStartup) { this.stopOnInformerErrorDuringStartup = stopOnInformerErrorDuringStartup; return this; } + public ConfigurationServiceOverrider withCacheSyncTimeout(int cacheSyncTimeout) { + this.cacheSyncTimeout = cacheSyncTimeout; + return this; + } + public ConfigurationService build() { return new BaseConfigurationService(original.getVersion(), cloner, objectMapper) { @Override @@ -184,6 +190,11 @@ public boolean stopOnInformerErrorDuringStartup() { return stopOnInformerErrorDuringStartup != null ? stopOnInformerErrorDuringStartup : super.stopOnInformerErrorDuringStartup(); } + + @Override + public int cacheSyncTimeout() { + return cacheSyncTimeout != null ? cacheSyncTimeout : super.cacheSyncTimeout(); + } }; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java index 3b11b7755d..533a22c513 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java @@ -61,14 +61,12 @@ public void start() throws OperatorException { + fullResourceName + "/" + version); } }); - if (!configService.stopOnInformerErrorDuringStartup()) { informer.exceptionHandler((b, t) -> !ExceptionHandler.isDeserializationException(t)); } try { var start = informer.start(); - // todo configurable timeout, also timeout for run? - start.toCompletableFuture().get(5000, TimeUnit.MILLISECONDS); + start.toCompletableFuture().get(configService.cacheSyncTimeout(), TimeUnit.MILLISECONDS); } catch (TimeoutException | ExecutionException e) { log.warn("Informer startup error. Informer: {}", informer, e); if (configService.stopOnInformerErrorDuringStartup()) { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/MockKubernetesClient.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/MockKubernetesClient.java index 076e13b15b..81f8741de9 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/MockKubernetesClient.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/MockKubernetesClient.java @@ -17,8 +17,8 @@ import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.nullable; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; +import static org.mockito.Mockito.doAnswer; public class MockKubernetesClient { @@ -46,7 +46,20 @@ public static KubernetesClient client(Class clazz, CompletableFuture informerStartRes = new CompletableFuture<>(); informerStartRes.complete(null); when(informer.start()).thenReturn(informerStartRes); - + CompletableFuture stopped = new CompletableFuture<>(); + when(informer.stopped()).thenReturn(stopped); + when(informer.getApiTypeClass()).thenReturn(clazz); + if (informerRunBehavior != null) { + doAnswer(invocation -> { + try { + informerRunBehavior.accept(null); + } catch (Exception e) { + stopped.completeExceptionally(e); + } + return stopped; + }).when(informer).start(); + } + doAnswer(invocation -> null).when(informer).stop(); Indexer mockIndexer = mock(Indexer.class); when(informer.getIndexer()).thenReturn(mockIndexer); when(filterable.runnableInformer(anyLong())).thenReturn(informer); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java index 355dd23bb3..ec4ef7b5a8 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java @@ -19,6 +19,7 @@ import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_NAMESPACES_SET; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.atLeastOnce; @@ -243,8 +244,7 @@ void filtersOnDeleteEvents() { verify(eventHandlerMock, never()).handleEvent(any()); } - // todo add integration test - // @Test + @Test void informerStoppedHandlerShouldBeCalledWhenInformerStops() { try { final var exception = new RuntimeException("Informer stopped exceptionally!"); @@ -256,7 +256,8 @@ void informerStoppedHandlerShouldBeCalledWhenInformerStops() { MockKubernetesClient.client(Deployment.class, unused -> { throw exception; })); - informerEventSource.start(); + + assertThrows(RuntimeException.class, () -> informerEventSource.start()); verify(informerStoppedHandler, atLeastOnce()).onStop(any(), eq(exception)); } finally { ConfigurationServiceProvider.reset(); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java similarity index 83% rename from operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java rename to operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java index 0a998b485f..88f9b01abd 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/RBACBehaviorIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java @@ -19,29 +19,30 @@ import io.fabric8.kubernetes.client.utils.KubernetesResourceUtil; import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; -import io.javaoperatorsdk.operator.sample.rbacbehavior.RBACBehaviorTestCustomResource; -import io.javaoperatorsdk.operator.sample.rbacbehavior.RBACBehaviorTestReconciler; +import io.javaoperatorsdk.operator.sample.informerrelatedbehavior.InformerRelatedBehaviorTestCustomResource; +import io.javaoperatorsdk.operator.sample.informerrelatedbehavior.InformerRelatedBehaviorTestReconciler; import static org.assertj.core.api.Assertions.assertThat; import static org.awaitility.Awaitility.await; import static org.junit.jupiter.api.Assertions.assertThrows; -class RBACBehaviorIT { +class InformerRelatedBehaviorIT { - private final static Logger log = LoggerFactory.getLogger(RBACBehaviorIT.class); + private final static Logger log = LoggerFactory.getLogger(InformerRelatedBehaviorIT.class); public static final String TEST_RESOURCE_NAME = "test1"; - // https://junit.org/junit5/docs/5.1.1/api/org/junit/jupiter/api/extension/TestInstancePostProcessor.html // minikube start --extra-config=apiserver.min-request-timeout=3 KubernetesClient adminClient = new KubernetesClientBuilder().build(); - RBACBehaviorTestReconciler reconciler; + InformerRelatedBehaviorTestReconciler reconciler; String actualNamespace; + volatile boolean stopHandlerCalled = false; @BeforeEach void beforeEach(TestInfo testInfo) { - LocallyRunOperatorExtension.applyCrd(RBACBehaviorTestCustomResource.class, adminClient); + LocallyRunOperatorExtension.applyCrd(InformerRelatedBehaviorTestCustomResource.class, + adminClient); testInfo.getTestMethod().ifPresent(method -> { actualNamespace = KubernetesResourceUtil.sanitizeName(method.getName()); adminClient.resource(namespace()).createOrReplace(); @@ -124,6 +125,16 @@ void resilientForLoosingPermissionForSecondaryResource() { assertReconciled(); } + @Test + void callsStopHandlerOnStartupFail() { + setNoCustomResourceAccess(); + adminClient.resource(testCustomResource()).createOrReplace(); + + assertThrows(OperatorException.class, () -> startOperator(true)); + + await().untilAsserted(() -> assertThat(stopHandlerCalled).isTrue()); + } + private static void waitForWatchReconnect() { try { Thread.sleep(6000); @@ -132,15 +143,15 @@ private static void waitForWatchReconnect() { } } - private void assertNotReconciled() { await().pollDelay(Duration.ofMillis(2000)).untilAsserted(() -> { assertThat(reconciler.getNumberOfExecutions()).isEqualTo(0); }); } - RBACBehaviorTestCustomResource testCustomResource() { - RBACBehaviorTestCustomResource testCustomResource = new RBACBehaviorTestCustomResource(); + InformerRelatedBehaviorTestCustomResource testCustomResource() { + InformerRelatedBehaviorTestCustomResource testCustomResource = + new InformerRelatedBehaviorTestCustomResource(); testCustomResource.setMetadata(new ObjectMetaBuilder() .withNamespace(actualNamespace) .withName(TEST_RESOURCE_NAME) @@ -148,7 +159,6 @@ RBACBehaviorTestCustomResource testCustomResource() { return testCustomResource; } - private void assertReconciled() { await().untilAsserted(() -> { assertThat(reconciler.getNumberOfExecutions()).isGreaterThan(0); @@ -170,14 +180,14 @@ KubernetesClient clientUsingServiceAccount() { Operator startOperator(boolean stopOnInformerErrorDuringStartup) { ConfigurationServiceProvider.reset(); - reconciler = new RBACBehaviorTestReconciler(); + reconciler = new InformerRelatedBehaviorTestReconciler(); Operator operator = new Operator(clientUsingServiceAccount(), co -> { co.withStopOnInformerErrorDuringStartup(stopOnInformerErrorDuringStartup); + co.withCacheSyncTimeout(3000); co.withInformerStoppedHandler((informer, ex) -> { - informer.start(); - log.error("Stop handler: {}", informer, ex); + stopHandlerCalled = true; }); }); operator.register(reconciler); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/ConfigMapDependentResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informerrelatedbehavior/ConfigMapDependentResource.java similarity index 76% rename from operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/ConfigMapDependentResource.java rename to operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informerrelatedbehavior/ConfigMapDependentResource.java index 463543835f..c9747cbd00 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/ConfigMapDependentResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informerrelatedbehavior/ConfigMapDependentResource.java @@ -1,4 +1,4 @@ -package io.javaoperatorsdk.operator.sample.rbacbehavior; +package io.javaoperatorsdk.operator.sample.informerrelatedbehavior; import java.util.Map; @@ -11,7 +11,7 @@ @KubernetesDependent(labelSelector = "app=rbac-test") public class ConfigMapDependentResource - extends CRUDKubernetesDependentResource { + extends CRUDKubernetesDependentResource { public static final String DATA_KEY = "key"; @@ -20,8 +20,8 @@ public ConfigMapDependentResource() { } @Override - protected ConfigMap desired(RBACBehaviorTestCustomResource primary, - Context context) { + protected ConfigMap desired(InformerRelatedBehaviorTestCustomResource primary, + Context context) { return new ConfigMapBuilder() .withMetadata(new ObjectMetaBuilder() .withLabels(Map.of("app", "rbac-test")) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informerrelatedbehavior/InformerRelatedBehaviorTestCustomResource.java similarity index 76% rename from operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestCustomResource.java rename to operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informerrelatedbehavior/InformerRelatedBehaviorTestCustomResource.java index 6c6879e7c3..fa59c15831 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestCustomResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informerrelatedbehavior/InformerRelatedBehaviorTestCustomResource.java @@ -1,4 +1,4 @@ -package io.javaoperatorsdk.operator.sample.rbacbehavior; +package io.javaoperatorsdk.operator.sample.informerrelatedbehavior; import io.fabric8.kubernetes.api.model.Namespaced; import io.fabric8.kubernetes.client.CustomResource; @@ -9,7 +9,7 @@ @Group("sample.javaoperatorsdk") @Version("v1") @ShortNames("rbt") -public class RBACBehaviorTestCustomResource +public class InformerRelatedBehaviorTestCustomResource extends CustomResource implements Namespaced { } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informerrelatedbehavior/InformerRelatedBehaviorTestReconciler.java similarity index 67% rename from operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestReconciler.java rename to operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informerrelatedbehavior/InformerRelatedBehaviorTestReconciler.java index 0f71e49b46..baeff08478 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/rbacbehavior/RBACBehaviorTestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informerrelatedbehavior/InformerRelatedBehaviorTestReconciler.java @@ -1,4 +1,4 @@ -package io.javaoperatorsdk.operator.sample.rbacbehavior; +package io.javaoperatorsdk.operator.sample.informerrelatedbehavior; import java.util.concurrent.atomic.AtomicInteger; @@ -11,17 +11,17 @@ import io.javaoperatorsdk.operator.support.TestExecutionInfoProvider; @ControllerConfiguration(dependents = @Dependent(type = ConfigMapDependentResource.class)) -public class RBACBehaviorTestReconciler - implements Reconciler, TestExecutionInfoProvider { +public class InformerRelatedBehaviorTestReconciler + implements Reconciler, TestExecutionInfoProvider { private final AtomicInteger numberOfExecutions = new AtomicInteger(0); private KubernetesClient client; @Override - public UpdateControl reconcile( - RBACBehaviorTestCustomResource resource, - Context context) { + public UpdateControl reconcile( + InformerRelatedBehaviorTestCustomResource resource, + Context context) { numberOfExecutions.addAndGet(1); return UpdateControl.noUpdate(); } diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml index c27febb067..024f298462 100644 --- a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml @@ -5,7 +5,7 @@ metadata: name: rbac-behavior rules: - apiGroups: [ "sample.javaoperatorsdk" ] - resources: [ "rbacbehaviortestcustomresources" ] + resources: [ "informerrelatedbehaviortestcustomresources" ] verbs: [ "get", "watch", "list","post", "delete" ] - apiGroups: [ "" ] resources: [ "configmaps" ] diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-configmap-access.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-configmap-access.yaml index 16e52add81..bac8c1149f 100644 --- a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-configmap-access.yaml +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-configmap-access.yaml @@ -5,7 +5,7 @@ metadata: name: rbac-behavior rules: - apiGroups: [ "sample.javaoperatorsdk" ] - resources: [ "rbacbehaviortestcustomresources" ] + resources: [ "informerrelatedbehaviortestcustomresources" ] verbs: [ "get", "watch", "list","post", "delete" ] From 9a83f7d35ec3a629c38d31073c38394054673ec1 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 26 Oct 2022 11:13:58 +0200 Subject: [PATCH 11/25] docs --- .../api/config/ConfigurationService.java | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index fc61a80392..6a016340ca 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -152,18 +152,35 @@ default Optional getLeaderElectionConfiguration() { return Optional.empty(); } + /** + *

+ * if true, operator stops if there are some issues with informers + * {@link io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource} or + * {@link io.javaoperatorsdk.operator.processing.event.source.controller.ControllerResourceEventSource} + * on startup. Other event sources may also respect this flag. + *

+ *

+ * if false, the startup will ignore recoverable errors, caused for example by RBAC issues, and + * will try to reconnect periodically in the background. + *

+ */ default boolean stopOnInformerErrorDuringStartup() { return true; } /** * Timeout for cache sync in milliseconds. In other words source start timeout. Note that is - * "stopOnInformerErrorDuringStartup" is true, this will be resulted in an operator stop. + * "stopOnInformerErrorDuringStartup" is true the operator will stop on timeout. Default is 2 + * minutes. */ default int cacheSyncTimeout() { return 2 * 60 * 1000; } + /** + * Handler for an informer stop. Informer stops if there is a non-recoverable error. Like received + * a resource that cannot be deserialized. + */ default Optional getInformerStoppedHandler() { return Optional.of((informer, ex) -> { if (ex != null) { From 4a8e23b215a0ccde33199b8813c9b41c6f99117a Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 26 Oct 2022 11:25:45 +0200 Subject: [PATCH 12/25] test category --- .../operator/InformerRelatedBehaviorIT.java | 24 ++++++++++--------- pom.xml | 16 +++++++++++++ 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java index 88f9b01abd..0c35dc9d99 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java @@ -2,12 +2,7 @@ import java.time.Duration; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.TestInfo; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import org.junit.jupiter.api.*; import io.fabric8.kubernetes.api.model.Namespace; import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; @@ -26,14 +21,21 @@ import static org.awaitility.Awaitility.await; import static org.junit.jupiter.api.Assertions.assertThrows; +/** + * The test relies on a special minikube configuration: "min-request-timeout" to have a very low value, see: + * "minikube start --extra-config=apiserver.min-request-timeout=3" + * + *

+ * This is important when tests are affected by permission changes, since the watch permissions are + * just checked when established a watch request. So minimal request timeout is set to make sure + * that with periodical watch reconnect the permission is tested again. + *

+ */ +@Tag("WatchPermissionAwareTest") class InformerRelatedBehaviorIT { - private final static Logger log = LoggerFactory.getLogger(InformerRelatedBehaviorIT.class); - public static final String TEST_RESOURCE_NAME = "test1"; - - // minikube start --extra-config=apiserver.min-request-timeout=3 - + KubernetesClient adminClient = new KubernetesClientBuilder().build(); InformerRelatedBehaviorTestReconciler reconciler; String actualNamespace; diff --git a/pom.xml b/pom.xml index 521ab7bffb..9e21b8d131 100644 --- a/pom.xml +++ b/pom.xml @@ -372,6 +372,22 @@ **/*Test.java **/*E2E.java + WatchPermissionAwareTest + + + + + + + + minimal-watch-timeout-dependent-it + + + + org.apache.maven.plugins + maven-surefire-plugin + + WatchPermissionAwareTest From 4b9bcb7e3dd2b69710dcf7b7d2ef3cdbe429c2c1 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 26 Oct 2022 11:29:57 +0200 Subject: [PATCH 13/25] format fix --- .../javaoperatorsdk/operator/InformerRelatedBehaviorIT.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java index 0c35dc9d99..1267d5155e 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java @@ -22,8 +22,8 @@ import static org.junit.jupiter.api.Assertions.assertThrows; /** - * The test relies on a special minikube configuration: "min-request-timeout" to have a very low value, see: - * "minikube start --extra-config=apiserver.min-request-timeout=3" + * The test relies on a special minikube configuration: "min-request-timeout" to have a very low + * value, see: "minikube start --extra-config=apiserver.min-request-timeout=3" * *

* This is important when tests are affected by permission changes, since the watch permissions are @@ -35,7 +35,7 @@ class InformerRelatedBehaviorIT { public static final String TEST_RESOURCE_NAME = "test1"; - + KubernetesClient adminClient = new KubernetesClientBuilder().build(); InformerRelatedBehaviorTestReconciler reconciler; String actualNamespace; From 1b4c0e5cfe7553b8e89dd303acec2ac4f2c98179 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 26 Oct 2022 11:52:55 +0200 Subject: [PATCH 14/25] PR workflow update --- .github/workflows/pr.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index e7ba5d1efc..188207996d 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -53,3 +53,12 @@ jobs: driver: 'docker' - name: Run integration tests run: ./mvnw ${MAVEN_ARGS} -B package -P no-unit-tests --file pom.xml + - name: Set up Minikube + uses: manusa/actions-setup-minikube@v2.7.0 + with: + minikube version: 'v1.26.0' + kubernetes version: ${{ matrix.kubernetes }} + driver: 'docker' + start args: '--extra-config=apiserver.min-request-timeout=3' + - name: Run integration tests + run: ./mvnw ${MAVEN_ARGS} -B package -P minimal-watch-timeout-dependent-it --file pom.xml \ No newline at end of file From 959c544e83903737c414a470198b7531e50edc84 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 26 Oct 2022 13:04:12 +0200 Subject: [PATCH 15/25] PR CI facelift --- .github/workflows/pr.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 188207996d..e38bc77726 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -53,12 +53,12 @@ jobs: driver: 'docker' - name: Run integration tests run: ./mvnw ${MAVEN_ARGS} -B package -P no-unit-tests --file pom.xml - - name: Set up Minikube + - name: Adjusting Minikube Settings uses: manusa/actions-setup-minikube@v2.7.0 with: minikube version: 'v1.26.0' kubernetes version: ${{ matrix.kubernetes }} driver: 'docker' start args: '--extra-config=apiserver.min-request-timeout=3' - - name: Run integration tests + - name: Run WatchPermissionAwareTest Category run: ./mvnw ${MAVEN_ARGS} -B package -P minimal-watch-timeout-dependent-it --file pom.xml \ No newline at end of file From 384462dd17ccd080220c507d7d2f2a1e3bb508ac Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 26 Oct 2022 13:09:43 +0200 Subject: [PATCH 16/25] fix: watch test run ? --- pom.xml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pom.xml b/pom.xml index 9e21b8d131..cfe9f71bb4 100644 --- a/pom.xml +++ b/pom.xml @@ -387,6 +387,9 @@ org.apache.maven.plugins maven-surefire-plugin + + **/*IT.java + WatchPermissionAwareTest From 64638e8f3abc8ebe6b64c9d19227b17c7b5d883a Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 26 Oct 2022 13:39:59 +0200 Subject: [PATCH 17/25] fix running test --- ...latedBehaviorIT.java => InformerRelatedBehaviorTest.java} | 5 ++++- pom.xml | 3 --- 2 files changed, 4 insertions(+), 4 deletions(-) rename operator-framework/src/test/java/io/javaoperatorsdk/operator/{InformerRelatedBehaviorIT.java => InformerRelatedBehaviorTest.java} (98%) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorTest.java similarity index 98% rename from operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java rename to operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorTest.java index 1267d5155e..4b25a47f46 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorTest.java @@ -30,9 +30,12 @@ * just checked when established a watch request. So minimal request timeout is set to make sure * that with periodical watch reconnect the permission is tested again. *

+ *

+ * The test does not end with "IT" since that did not work with tags. + *

*/ @Tag("WatchPermissionAwareTest") -class InformerRelatedBehaviorIT { +class InformerRelatedBehaviorTest { public static final String TEST_RESOURCE_NAME = "test1"; diff --git a/pom.xml b/pom.xml index cfe9f71bb4..9e21b8d131 100644 --- a/pom.xml +++ b/pom.xml @@ -387,9 +387,6 @@ org.apache.maven.plugins maven-surefire-plugin - - **/*IT.java - WatchPermissionAwareTest From c5f28d8cb5a4bfe8f588e20ab8723fded0b9d318 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 26 Oct 2022 13:42:18 +0200 Subject: [PATCH 18/25] not run integration tests with tags with unit tests --- pom.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/pom.xml b/pom.xml index 9e21b8d131..d83531c6dd 100644 --- a/pom.xml +++ b/pom.xml @@ -307,6 +307,7 @@ **/*IT.java **/*E2E.java + WatchPermissionAwareTest From a8c9e8df9a8325c87265d13643d2f342a9b9f481 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 26 Oct 2022 14:59:36 +0200 Subject: [PATCH 19/25] test run fix --- .../event/source/informer/InformerWrapper.java | 3 +++ ...viorTest.java => InformerRelatedBehaviorITS.java} | 5 ++--- pom.xml | 12 +++++++++--- 3 files changed, 14 insertions(+), 6 deletions(-) rename operator-framework/src/test/java/io/javaoperatorsdk/operator/{InformerRelatedBehaviorTest.java => InformerRelatedBehaviorITS.java} (98%) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java index 533a22c513..ffb3b7d15e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java @@ -66,6 +66,9 @@ public void start() throws OperatorException { } try { var start = informer.start(); + // note that in case we don't put here timeout and stopOnInformerErrorDuringStartup is + // false, and there is a rbac issue the get never returns; therefore operator never really + // starts start.toCompletableFuture().get(configService.cacheSyncTimeout(), TimeUnit.MILLISECONDS); } catch (TimeoutException | ExecutionException e) { log.warn("Informer startup error. Informer: {}", informer, e); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorTest.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorITS.java similarity index 98% rename from operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorTest.java rename to operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorITS.java index 4b25a47f46..e3ddb3a1e0 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorTest.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorITS.java @@ -31,11 +31,10 @@ * that with periodical watch reconnect the permission is tested again. *

*

- * The test does not end with "IT" since that did not work with tags. + * The test ends with "ITS" (Special) since it needs to run separately from other ITs *

*/ -@Tag("WatchPermissionAwareTest") -class InformerRelatedBehaviorTest { +class InformerRelatedBehaviorITS { public static final String TEST_RESOURCE_NAME = "test1"; diff --git a/pom.xml b/pom.xml index d83531c6dd..953b8befc6 100644 --- a/pom.xml +++ b/pom.xml @@ -373,7 +373,6 @@ **/*Test.java **/*E2E.java - WatchPermissionAwareTest @@ -388,7 +387,14 @@ org.apache.maven.plugins maven-surefire-plugin - WatchPermissionAwareTest + + **/*ITS.java + + + **/*Test.java + **/*E2E.java + **/*IT.java + @@ -418,7 +424,6 @@ release - org.apache.maven.plugins maven-surefire-plugin @@ -426,6 +431,7 @@ **/*IT.java **/*E2E.java + **/InformerRelatedBehaviorTest.java From 6b8cfb69d0170865267d9c92ea6a1334c318e92c Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 26 Oct 2022 15:08:56 +0200 Subject: [PATCH 20/25] action name --- .github/workflows/pr.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index e38bc77726..f3ec0d3397 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -53,7 +53,7 @@ jobs: driver: 'docker' - name: Run integration tests run: ./mvnw ${MAVEN_ARGS} -B package -P no-unit-tests --file pom.xml - - name: Adjusting Minikube Settings + - name: Adjust Minikube Min Request Timeout Setting uses: manusa/actions-setup-minikube@v2.7.0 with: minikube version: 'v1.26.0' From 2dff5520d2231cb86e36831880534e2e262b056d Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 26 Oct 2022 15:19:14 +0200 Subject: [PATCH 21/25] naming --- .github/workflows/pr.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index f3ec0d3397..ec64e93a39 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -60,5 +60,5 @@ jobs: kubernetes version: ${{ matrix.kubernetes }} driver: 'docker' start args: '--extra-config=apiserver.min-request-timeout=3' - - name: Run WatchPermissionAwareTest Category + - name: Run Special Integration Test run: ./mvnw ${MAVEN_ARGS} -B package -P minimal-watch-timeout-dependent-it --file pom.xml \ No newline at end of file From 336f759995e79f557a324ea6ab8b5abd1b65a80e Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 26 Oct 2022 15:19:24 +0200 Subject: [PATCH 22/25] naming --- .github/workflows/pr.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index ec64e93a39..e5627d1e0a 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -60,5 +60,5 @@ jobs: kubernetes version: ${{ matrix.kubernetes }} driver: 'docker' start args: '--extra-config=apiserver.min-request-timeout=3' - - name: Run Special Integration Test + - name: Run Special Integration Tests run: ./mvnw ${MAVEN_ARGS} -B package -P minimal-watch-timeout-dependent-it --file pom.xml \ No newline at end of file From cd1c5bf66fb6280480229b7ad7896d4699feb311 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 27 Oct 2022 09:24:09 +0200 Subject: [PATCH 23/25] fixes from CR --- .../operator/api/config/ConfigurationService.java | 5 +++-- .../operator/api/config/ConfigurationServiceOverrider.java | 7 ++++--- .../processing/event/source/informer/InformerWrapper.java | 3 ++- .../operator/InformerRelatedBehaviorITS.java | 2 +- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 6a016340ca..16faa0e9f0 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -1,5 +1,6 @@ package io.javaoperatorsdk.operator.api.config; +import java.time.Duration; import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; @@ -173,8 +174,8 @@ default boolean stopOnInformerErrorDuringStartup() { * "stopOnInformerErrorDuringStartup" is true the operator will stop on timeout. Default is 2 * minutes. */ - default int cacheSyncTimeout() { - return 2 * 60 * 1000; + default Duration cacheSyncTimeout() { + return Duration.ofMinutes(2); } /** diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index 9cb9dae521..2b8aee9708 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -1,5 +1,6 @@ package io.javaoperatorsdk.operator.api.config; +import java.time.Duration; import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; @@ -28,7 +29,7 @@ public class ConfigurationServiceOverrider { private LeaderElectionConfiguration leaderElectionConfiguration; private InformerStoppedHandler informerStoppedHandler; private Boolean stopOnInformerErrorDuringStartup; - private Integer cacheSyncTimeout; + private Duration cacheSyncTimeout; ConfigurationServiceOverrider(ConfigurationService original) { this.original = original; @@ -107,7 +108,7 @@ public ConfigurationServiceOverrider withStopOnInformerErrorDuringStartup( return this; } - public ConfigurationServiceOverrider withCacheSyncTimeout(int cacheSyncTimeout) { + public ConfigurationServiceOverrider withCacheSyncTimeout(Duration cacheSyncTimeout) { this.cacheSyncTimeout = cacheSyncTimeout; return this; } @@ -192,7 +193,7 @@ public boolean stopOnInformerErrorDuringStartup() { } @Override - public int cacheSyncTimeout() { + public Duration cacheSyncTimeout() { return cacheSyncTimeout != null ? cacheSyncTimeout : super.cacheSyncTimeout(); } }; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java index ffb3b7d15e..77d2977cab 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java @@ -69,7 +69,8 @@ public void start() throws OperatorException { // note that in case we don't put here timeout and stopOnInformerErrorDuringStartup is // false, and there is a rbac issue the get never returns; therefore operator never really // starts - start.toCompletableFuture().get(configService.cacheSyncTimeout(), TimeUnit.MILLISECONDS); + start.toCompletableFuture().get(configService.cacheSyncTimeout().toMillis(), + TimeUnit.MILLISECONDS); } catch (TimeoutException | ExecutionException e) { log.warn("Informer startup error. Informer: {}", informer, e); if (configService.stopOnInformerErrorDuringStartup()) { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorITS.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorITS.java index e3ddb3a1e0..bfc0e4c02f 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorITS.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/InformerRelatedBehaviorITS.java @@ -189,7 +189,7 @@ Operator startOperator(boolean stopOnInformerErrorDuringStartup) { Operator operator = new Operator(clientUsingServiceAccount(), co -> { co.withStopOnInformerErrorDuringStartup(stopOnInformerErrorDuringStartup); - co.withCacheSyncTimeout(3000); + co.withCacheSyncTimeout(Duration.ofMillis(3000)); co.withInformerStoppedHandler((informer, ex) -> { stopHandlerCalled = true; }); From 66bc0398ec06db97628cd7c49880e447ae834350 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 27 Oct 2022 09:48:10 +0200 Subject: [PATCH 24/25] fixes --- .../event/source/informer/InformerEventSourceTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java index ec4ef7b5a8..8b5c0202c0 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java @@ -257,6 +257,8 @@ void informerStoppedHandlerShouldBeCalledWhenInformerStops() { throw exception; })); + // by default informer fails to start if there is an exception in the client on start. + // Throws the exception further. assertThrows(RuntimeException.class, () -> informerEventSource.start()); verify(informerStoppedHandler, atLeastOnce()).onStop(any(), eq(exception)); } finally { From d6329fec9129de64afcb2702b15b96bfd022dbd9 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 27 Oct 2022 10:31:33 +0200 Subject: [PATCH 25/25] improved logging --- .../processing/event/source/informer/InformerWrapper.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java index 77d2977cab..601cb0c10c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java @@ -72,9 +72,11 @@ public void start() throws OperatorException { start.toCompletableFuture().get(configService.cacheSyncTimeout().toMillis(), TimeUnit.MILLISECONDS); } catch (TimeoutException | ExecutionException e) { - log.warn("Informer startup error. Informer: {}", informer, e); if (configService.stopOnInformerErrorDuringStartup()) { + log.error("Informer startup error. Operator will be stopped. Informer: {}", informer, e); throw new OperatorException(e); + } else { + log.warn("Informer startup error. Will periodically retry. Informer: {}", informer, e); } } catch (InterruptedException e) { Thread.currentThread().interrupt();