From 40f175988a78e46cbf488ed4b6ca59bb3c961ad9 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 23 May 2023 14:41:38 +0200 Subject: [PATCH 1/9] feat: stops operator if leader elector has no permission to lease object --- .../operator/LeaderElectionManager.java | 39 +++++++--- .../operator/LeaderElectionPermissionIT.java | 72 +++++++++++++++++++ ...er-elector-stop-noaccess-role-binding.yaml | 14 ++++ .../leader-elector-stop-role-noaccess.yaml | 9 +++ 4 files changed, 125 insertions(+), 9 deletions(-) create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java create mode 100644 operator-framework/src/test/resources/io/javaoperatorsdk/operator/leader-elector-stop-noaccess-role-binding.yaml create mode 100644 operator-framework/src/test/resources/io/javaoperatorsdk/operator/leader-elector-stop-role-noaccess.yaml diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java index 26f26afd1b..b5bcda0883 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java @@ -7,6 +7,7 @@ import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.client.KubernetesClientException; import io.fabric8.kubernetes.client.extended.leaderelection.LeaderCallbacks; import io.fabric8.kubernetes.client.extended.leaderelection.LeaderElectionConfig; import io.fabric8.kubernetes.client.extended.leaderelection.LeaderElector; @@ -20,16 +21,22 @@ public class LeaderElectionManager { private static final Logger log = LoggerFactory.getLogger(LeaderElectionManager.class); + public static final String NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE = + "No permission to lease resource."; + private LeaderElector leaderElector = null; private final ControllerManager controllerManager; private String identity; private CompletableFuture leaderElectionFuture; + private LeaderElectionConfig leaderElectionConfig; + private KubernetesClient client; public LeaderElectionManager(ControllerManager controllerManager) { this.controllerManager = controllerManager; } public void init(LeaderElectionConfiguration config, KubernetesClient client) { + this.client = client; this.identity = identity(config); final var leaseNamespace = config.getLeaseNamespace().orElseGet( @@ -42,18 +49,19 @@ public void init(LeaderElectionConfiguration config, KubernetesClient client) { } final var lock = new LeaseLock(leaseNamespace, config.getLeaseName(), identity); // releaseOnCancel is not used in the underlying implementation + leaderElectionConfig = new LeaderElectionConfig( + lock, + config.getLeaseDuration(), + config.getRenewDeadline(), + config.getRetryPeriod(), + leaderCallbacks(), + true, + config.getLeaseName()); + leaderElector = new LeaderElectorBuilder( client, ExecutorServiceManager.instance().executorService()) - .withConfig( - new LeaderElectionConfig( - lock, - config.getLeaseDuration(), - config.getRenewDeadline(), - config.getRetryPeriod(), - leaderCallbacks(), - true, - config.getLeaseName())) + .withConfig(leaderElectionConfig) .build(); } @@ -90,6 +98,7 @@ private String identity(LeaderElectionConfiguration config) { public void start() { if (isLeaderElectionEnabled()) { + checkLeaseAccess(); leaderElectionFuture = leaderElector.start(); } } @@ -99,4 +108,16 @@ public void stop() { leaderElectionFuture.cancel(false); } } + + private void checkLeaseAccess() { + try { + leaderElectionConfig.getLock().get(client); + } catch (KubernetesClientException e) { + if (e.getCode() == 403) { + throw new OperatorException(NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE, e); + } else { + throw e; + } + } + } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java new file mode 100644 index 0000000000..ba3518b32b --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java @@ -0,0 +1,72 @@ +package io.javaoperatorsdk.operator; + +import org.junit.jupiter.api.Test; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.rbac.Role; +import io.fabric8.kubernetes.api.model.rbac.RoleBinding; +import io.fabric8.kubernetes.client.ConfigBuilder; +import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.client.KubernetesClientBuilder; +import io.javaoperatorsdk.operator.api.config.LeaderElectionConfiguration; +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 static io.javaoperatorsdk.operator.LeaderElectionManager.NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +class LeaderElectionPermissionIT { + + KubernetesClient adminClient = new KubernetesClientBuilder().build(); + + @Test + void operatorStopsIfNoLeaderElectionPermission() { + applyRole(); + applyRoleBinding(); + + var client = new KubernetesClientBuilder().withConfig(new ConfigBuilder() + .withImpersonateUsername("leader-elector-stop-noaccess") + .build()).build(); + + var operator = new Operator(client, o -> { + o.withLeaderElectionConfiguration( + new LeaderElectionConfiguration("lease1", "default")); + o.withStopOnInformerErrorDuringStartup(false); + }); + operator.register(new TestReconciler(), o -> o.settingNamespace("default")); + + OperatorException exception = assertThrows( + OperatorException.class, + operator::start); + + assertThat(exception.getCause().getMessage()) + .isEqualTo(NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE); + } + + + @ControllerConfiguration + public static class TestReconciler implements Reconciler { + @Override + public UpdateControl reconcile(ConfigMap resource, Context context) + throws Exception { + throw new IllegalStateException("Should not get here"); + } + } + + private void applyRoleBinding() { + var clusterRoleBinding = ReconcilerUtils + .loadYaml(RoleBinding.class, this.getClass(), + "leader-elector-stop-noaccess-role-binding.yaml"); + adminClient.resource(clusterRoleBinding).createOrReplace(); + } + + private void applyRole() { + var role = ReconcilerUtils + .loadYaml(Role.class, this.getClass(), "leader-elector-stop-role-noaccess.yaml"); + adminClient.resource(role).createOrReplace(); + } + +} diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/leader-elector-stop-noaccess-role-binding.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/leader-elector-stop-noaccess-role-binding.yaml new file mode 100644 index 0000000000..0e374522d9 --- /dev/null +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/leader-elector-stop-noaccess-role-binding.yaml @@ -0,0 +1,14 @@ +apiVersion: rbac.authorization.k8s.io/v1 +# This cluster role binding allows anyone in the "manager" group to read secrets in any namespace. +kind: RoleBinding +metadata: + name: informer-rbac-startup-global + namespace: default +subjects: + - kind: User + name: leader-elector-stop-noaccess + apiGroup: rbac.authorization.k8s.io +roleRef: + kind: Role + name: leader-elector-stop-noaccess + apiGroup: rbac.authorization.k8s.io \ No newline at end of file diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/leader-elector-stop-role-noaccess.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/leader-elector-stop-role-noaccess.yaml new file mode 100644 index 0000000000..b08bbb5c5a --- /dev/null +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/leader-elector-stop-role-noaccess.yaml @@ -0,0 +1,9 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + namespace: default + name: leader-elector-stop-noaccess +rules: + - apiGroups: [ "" ] + resources: [ "configmaps" ] + verbs: [ "get", "watch", "list","post", "delete", "create" ] \ No newline at end of file From 787f04c845ab58a9e29584b7c0a02b5f0e5e9a26 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 23 May 2023 16:31:03 +0200 Subject: [PATCH 2/9] improved permission checking --- .../operator/LeaderElectionManager.java | 60 ++++++++++++------- .../operator/LeaderElectionPermissionIT.java | 2 +- 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java index b5bcda0883..162e9ef38c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java @@ -6,8 +6,10 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import io.fabric8.kubernetes.api.model.authorization.v1.ResourceAttributesBuilder; +import io.fabric8.kubernetes.api.model.authorization.v1.SelfSubjectAccessReview; +import io.fabric8.kubernetes.api.model.authorization.v1.SelfSubjectAccessReviewSpecBuilder; import io.fabric8.kubernetes.client.KubernetesClient; -import io.fabric8.kubernetes.client.KubernetesClientException; import io.fabric8.kubernetes.client.extended.leaderelection.LeaderCallbacks; import io.fabric8.kubernetes.client.extended.leaderelection.LeaderElectionConfig; import io.fabric8.kubernetes.client.extended.leaderelection.LeaderElector; @@ -28,8 +30,9 @@ public class LeaderElectionManager { private final ControllerManager controllerManager; private String identity; private CompletableFuture leaderElectionFuture; - private LeaderElectionConfig leaderElectionConfig; private KubernetesClient client; + private String leaseName; + private String leaseNamespace; public LeaderElectionManager(ControllerManager controllerManager) { this.controllerManager = controllerManager; @@ -38,7 +41,8 @@ public LeaderElectionManager(ControllerManager controllerManager) { public void init(LeaderElectionConfiguration config, KubernetesClient client) { this.client = client; this.identity = identity(config); - final var leaseNamespace = + this.leaseName = config.getLeaseName(); + leaseNamespace = config.getLeaseNamespace().orElseGet( () -> ConfigurationServiceProvider.instance().getClientConfiguration().getNamespace()); if (leaseNamespace == null) { @@ -47,21 +51,19 @@ public void init(LeaderElectionConfiguration config, KubernetesClient client) { log.error(message); throw new IllegalArgumentException(message); } - final var lock = new LeaseLock(leaseNamespace, config.getLeaseName(), identity); + final var lock = new LeaseLock(leaseNamespace, leaseName, identity); // releaseOnCancel is not used in the underlying implementation - leaderElectionConfig = new LeaderElectionConfig( - lock, - config.getLeaseDuration(), - config.getRenewDeadline(), - config.getRetryPeriod(), - leaderCallbacks(), - true, - config.getLeaseName()); - leaderElector = new LeaderElectorBuilder( client, ExecutorServiceManager.instance().executorService()) - .withConfig(leaderElectionConfig) + .withConfig(new LeaderElectionConfig( + lock, + config.getLeaseDuration(), + config.getRenewDeadline(), + config.getRetryPeriod(), + leaderCallbacks(), + true, + config.getLeaseName())) .build(); } @@ -110,14 +112,30 @@ public void stop() { } private void checkLeaseAccess() { - try { - leaderElectionConfig.getLock().get(client); - } catch (KubernetesClientException e) { - if (e.getCode() == 403) { - throw new OperatorException(NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE, e); - } else { - throw e; + var verbs = new String[] {"create", "update", "get"}; + for (String verb : verbs) { + var allowed = checkLeaseAccess(verb); + if (!allowed) { + throw new OperatorException(NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE); } } } + + private boolean checkLeaseAccess(String verb) { + var res = client.resource(selfSubjectReview(verb, leaseName, leaseNamespace)).create(); + return res.getStatus().getAllowed(); + } + + private SelfSubjectAccessReview selfSubjectReview(String verb, String name, String namespace) { + var res = new SelfSubjectAccessReview(); + res.setSpec(new SelfSubjectAccessReviewSpecBuilder() + .withResourceAttributes(new ResourceAttributesBuilder() + .withGroup("coordination.k8s.io") + .withNamespace(namespace) + .withName(name) + .withVerb(verb) + .build()) + .build()); + return res; + } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java index ba3518b32b..a543cbe5e9 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java @@ -3,6 +3,7 @@ import org.junit.jupiter.api.Test; import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.authorization.v1.*; import io.fabric8.kubernetes.api.model.rbac.Role; import io.fabric8.kubernetes.api.model.rbac.RoleBinding; import io.fabric8.kubernetes.client.ConfigBuilder; @@ -68,5 +69,4 @@ private void applyRole() { .loadYaml(Role.class, this.getClass(), "leader-elector-stop-role-noaccess.yaml"); adminClient.resource(role).createOrReplace(); } - } From 6ea8721c09ba6f546333e51a92e751ba13648180 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 23 May 2023 17:27:35 +0200 Subject: [PATCH 3/9] different approach --- .../operator/LeaderElectionManager.java | 41 +++++++------------ .../operator/LeaderElectionPermissionIT.java | 2 +- 2 files changed, 15 insertions(+), 28 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java index 162e9ef38c..c4f6d31c5a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java @@ -1,14 +1,13 @@ package io.javaoperatorsdk.operator; +import java.util.Arrays; import java.util.UUID; import java.util.concurrent.CompletableFuture; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import io.fabric8.kubernetes.api.model.authorization.v1.ResourceAttributesBuilder; -import io.fabric8.kubernetes.api.model.authorization.v1.SelfSubjectAccessReview; -import io.fabric8.kubernetes.api.model.authorization.v1.SelfSubjectAccessReviewSpecBuilder; +import io.fabric8.kubernetes.api.model.authorization.v1.*; import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.extended.leaderelection.LeaderCallbacks; import io.fabric8.kubernetes.client.extended.leaderelection.LeaderElectionConfig; @@ -112,30 +111,18 @@ public void stop() { } private void checkLeaseAccess() { - var verbs = new String[] {"create", "update", "get"}; - for (String verb : verbs) { - var allowed = checkLeaseAccess(verb); - if (!allowed) { - throw new OperatorException(NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE); - } + var verbs = Arrays.asList("create", "update", "get"); + SelfSubjectRulesReview review = new SelfSubjectRulesReview(); + review.setSpec(new SelfSubjectRulesReviewSpecBuilder().withNamespace(leaseNamespace).build()); + var reviewResult = client.resource(review).create(); + var foundRule = reviewResult.getStatus().getResourceRules().stream() + .filter(rule -> rule.getApiGroups().contains("coordination.k8s.io") + && rule.getResources().contains("leases") + && rule.getVerbs().containsAll(verbs)) + .findAny(); + if (foundRule.isEmpty()) { + throw new OperatorException(NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE + + " in namespace: " + leaseNamespace); } } - - private boolean checkLeaseAccess(String verb) { - var res = client.resource(selfSubjectReview(verb, leaseName, leaseNamespace)).create(); - return res.getStatus().getAllowed(); - } - - private SelfSubjectAccessReview selfSubjectReview(String verb, String name, String namespace) { - var res = new SelfSubjectAccessReview(); - res.setSpec(new SelfSubjectAccessReviewSpecBuilder() - .withResourceAttributes(new ResourceAttributesBuilder() - .withGroup("coordination.k8s.io") - .withNamespace(namespace) - .withName(name) - .withVerb(verb) - .build()) - .build()); - return res; - } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java index a543cbe5e9..e1c8435b8a 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java @@ -44,7 +44,7 @@ void operatorStopsIfNoLeaderElectionPermission() { operator::start); assertThat(exception.getCause().getMessage()) - .isEqualTo(NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE); + .contains(NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE); } From 0d2d2adf9c688febd981f6043a405ce061b46975 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 23 May 2023 17:39:17 +0200 Subject: [PATCH 4/9] fix for wildcard expression --- .../io/javaoperatorsdk/operator/LeaderElectionManager.java | 3 ++- .../io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java index c4f6d31c5a..4815d25992 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java @@ -115,10 +115,11 @@ private void checkLeaseAccess() { SelfSubjectRulesReview review = new SelfSubjectRulesReview(); review.setSpec(new SelfSubjectRulesReviewSpecBuilder().withNamespace(leaseNamespace).build()); var reviewResult = client.resource(review).create(); + log.debug("SelfSubjectRulesReview result: {}", reviewResult); var foundRule = reviewResult.getStatus().getResourceRules().stream() .filter(rule -> rule.getApiGroups().contains("coordination.k8s.io") && rule.getResources().contains("leases") - && rule.getVerbs().containsAll(verbs)) + && (rule.getVerbs().containsAll(verbs)) || rule.getVerbs().contains("*")) .findAny(); if (foundRule.isEmpty()) { throw new OperatorException(NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE + diff --git a/sample-operators/leader-election/src/test/java/io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java b/sample-operators/leader-election/src/test/java/io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java index 7932472aab..666ead1bc1 100644 --- a/sample-operators/leader-election/src/test/java/io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java +++ b/sample-operators/leader-election/src/test/java/io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java @@ -13,7 +13,6 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.condition.EnabledIfSystemProperty; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; import org.slf4j.Logger; @@ -52,7 +51,7 @@ class LeaderElectionE2E { @ParameterizedTest @ValueSource(strings = {"namespace-inferred-", ""}) // not for local mode by design - @EnabledIfSystemProperty(named = "test.deployment", matches = "remote") + // @EnabledIfSystemProperty(named = "test.deployment", matches = "remote") void otherInstancesTakesOverWhenSteppingDown(String yamlFilePrefix) { log.info("Applying custom resource"); applyCustomResource(); From d1f1352458db834f7d26024b8d74fcba1b8a84b0 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 23 May 2023 17:42:24 +0200 Subject: [PATCH 5/9] put pack annotation --- .../io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sample-operators/leader-election/src/test/java/io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java b/sample-operators/leader-election/src/test/java/io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java index 666ead1bc1..c6edb77daa 100644 --- a/sample-operators/leader-election/src/test/java/io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java +++ b/sample-operators/leader-election/src/test/java/io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java @@ -13,6 +13,7 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.condition.EnabledIfSystemProperty; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; import org.slf4j.Logger; @@ -51,7 +52,7 @@ class LeaderElectionE2E { @ParameterizedTest @ValueSource(strings = {"namespace-inferred-", ""}) // not for local mode by design - // @EnabledIfSystemProperty(named = "test.deployment", matches = "remote") + @EnabledIfSystemProperty(named = "test.deployment", matches = "remote") void otherInstancesTakesOverWhenSteppingDown(String yamlFilePrefix) { log.info("Applying custom resource"); applyCustomResource(); From fd8abf755492efc275b01f990268d37069827b35 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 23 May 2023 17:44:35 +0200 Subject: [PATCH 6/9] format --- .../io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sample-operators/leader-election/src/test/java/io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java b/sample-operators/leader-election/src/test/java/io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java index c6edb77daa..7932472aab 100644 --- a/sample-operators/leader-election/src/test/java/io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java +++ b/sample-operators/leader-election/src/test/java/io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java @@ -52,7 +52,7 @@ class LeaderElectionE2E { @ParameterizedTest @ValueSource(strings = {"namespace-inferred-", ""}) // not for local mode by design - @EnabledIfSystemProperty(named = "test.deployment", matches = "remote") + @EnabledIfSystemProperty(named = "test.deployment", matches = "remote") void otherInstancesTakesOverWhenSteppingDown(String yamlFilePrefix) { log.info("Applying custom resource"); applyCustomResource(); From fe8ab783db2194bd2cb3a186975df3f7399f5f6e Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 24 May 2023 10:09:16 +0200 Subject: [PATCH 7/9] check in LE init --- .../operator/LeaderElectionManager.java | 2 +- .../operator/LeaderElectionManagerTest.java | 3 ++- .../operator/MockKubernetesClient.java | 20 +++++++++++++++++++ .../operator/OperatorTest.java | 2 +- .../operator/LeaderElectionPermissionIT.java | 16 ++++++--------- 5 files changed, 30 insertions(+), 13 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java index 4815d25992..aa21811e55 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java @@ -50,6 +50,7 @@ public void init(LeaderElectionConfiguration config, KubernetesClient client) { log.error(message); throw new IllegalArgumentException(message); } + checkLeaseAccess(); final var lock = new LeaseLock(leaseNamespace, leaseName, identity); // releaseOnCancel is not used in the underlying implementation leaderElector = @@ -99,7 +100,6 @@ private String identity(LeaderElectionConfiguration config) { public void start() { if (isLeaderElectionEnabled()) { - checkLeaseAccess(); leaderElectionFuture = leaderElector.start(); } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/LeaderElectionManagerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/LeaderElectionManagerTest.java index 66b0030b5e..ab0e9b0109 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/LeaderElectionManagerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/LeaderElectionManagerTest.java @@ -9,6 +9,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.client.KubernetesClient; import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; import io.javaoperatorsdk.operator.api.config.LeaderElectionConfiguration; @@ -27,7 +28,7 @@ class LeaderElectionManagerTest { @BeforeEach void setUp() { ControllerManager controllerManager = mock(ControllerManager.class); - kubernetesClient = mock(KubernetesClient.class); + kubernetesClient = MockKubernetesClient.client(ConfigMap.class); leaderElectionManager = new LeaderElectionManager(controllerManager); } 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 f9f196c2de..0139b66254 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 @@ -6,6 +6,7 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.KubernetesResourceList; +import io.fabric8.kubernetes.api.model.authorization.v1.*; import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.V1ApiextensionAPIGroupDSL; import io.fabric8.kubernetes.client.dsl.*; @@ -70,6 +71,25 @@ public static KubernetesClient client(Class clazz, when(v1.customResourceDefinitions()).thenReturn(operation); when(operation.withName(any())).thenReturn(mock(Resource.class)); + var mockSelfSubjectReviewHandler = mock(NamespaceableResource.class); + when(mockSelfSubjectReviewHandler.create()) + .thenReturn(validSelfSubjectRulesReviewForLeaderElection()); + when(client.resource(any(SelfSubjectRulesReview.class))) + .thenReturn(mockSelfSubjectReviewHandler); + return client; } + + private static SelfSubjectRulesReview validSelfSubjectRulesReviewForLeaderElection() { + SelfSubjectRulesReview res = new SelfSubjectRulesReview(); + res.setStatus(new SubjectRulesReviewStatusBuilder() + .withResourceRules(new ResourceRuleBuilder() + .withApiGroups("coordination.k8s.io") + .withResources("leases") + .withVerbs("*") + .build()) + .build()); + return res; + } + } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java index 8a95431885..0c7d409d0e 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java @@ -43,7 +43,7 @@ void initOperator() { @Test @DisplayName("should throw `OperationException` when Configuration is null") - public void shouldThrowOperatorExceptionWhenConfigurationIsNull() { + void shouldThrowOperatorExceptionWhenConfigurationIsNull() { // use a ConfigurationService that doesn't automatically create configurations ConfigurationServiceProvider.reset(); ConfigurationServiceProvider.set(new AbstractConfigurationService(null)); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java index e1c8435b8a..37349072ee 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java @@ -32,18 +32,14 @@ void operatorStopsIfNoLeaderElectionPermission() { .withImpersonateUsername("leader-elector-stop-noaccess") .build()).build(); - var operator = new Operator(client, o -> { - o.withLeaderElectionConfiguration( - new LeaderElectionConfiguration("lease1", "default")); - o.withStopOnInformerErrorDuringStartup(false); - }); - operator.register(new TestReconciler(), o -> o.settingNamespace("default")); - OperatorException exception = assertThrows( - OperatorException.class, - operator::start); + OperatorException.class, () -> new Operator(client, o -> { + o.withLeaderElectionConfiguration( + new LeaderElectionConfiguration("lease1", "default")); + o.withStopOnInformerErrorDuringStartup(false); + })); - assertThat(exception.getCause().getMessage()) + assertThat(exception.getMessage()) .contains(NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE); } From 5d15051bffe5734442708086986caaabccf43372 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 24 May 2023 10:49:37 +0200 Subject: [PATCH 8/9] fix for config service reset --- .../io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java index 37349072ee..ec7f39916b 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java @@ -9,6 +9,7 @@ import io.fabric8.kubernetes.client.ConfigBuilder; import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.KubernetesClientBuilder; +import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; import io.javaoperatorsdk.operator.api.config.LeaderElectionConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; @@ -41,6 +42,7 @@ void operatorStopsIfNoLeaderElectionPermission() { assertThat(exception.getMessage()) .contains(NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE); + ConfigurationServiceProvider.reset(); } From b4868b46d579a97515d9bd1f5728770ddfab8070 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 24 May 2023 10:56:08 +0200 Subject: [PATCH 9/9] Revert "check in LE init" This reverts commit fe8ab783db2194bd2cb3a186975df3f7399f5f6e. --- .../operator/LeaderElectionManager.java | 2 +- .../operator/LeaderElectionManagerTest.java | 3 +-- .../operator/MockKubernetesClient.java | 20 ------------------- .../operator/OperatorTest.java | 2 +- .../operator/LeaderElectionPermissionIT.java | 16 +++++++++------ 5 files changed, 13 insertions(+), 30 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java index aa21811e55..4815d25992 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java @@ -50,7 +50,6 @@ public void init(LeaderElectionConfiguration config, KubernetesClient client) { log.error(message); throw new IllegalArgumentException(message); } - checkLeaseAccess(); final var lock = new LeaseLock(leaseNamespace, leaseName, identity); // releaseOnCancel is not used in the underlying implementation leaderElector = @@ -100,6 +99,7 @@ private String identity(LeaderElectionConfiguration config) { public void start() { if (isLeaderElectionEnabled()) { + checkLeaseAccess(); leaderElectionFuture = leaderElector.start(); } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/LeaderElectionManagerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/LeaderElectionManagerTest.java index ab0e9b0109..66b0030b5e 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/LeaderElectionManagerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/LeaderElectionManagerTest.java @@ -9,7 +9,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; -import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.client.KubernetesClient; import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; import io.javaoperatorsdk.operator.api.config.LeaderElectionConfiguration; @@ -28,7 +27,7 @@ class LeaderElectionManagerTest { @BeforeEach void setUp() { ControllerManager controllerManager = mock(ControllerManager.class); - kubernetesClient = MockKubernetesClient.client(ConfigMap.class); + kubernetesClient = mock(KubernetesClient.class); leaderElectionManager = new LeaderElectionManager(controllerManager); } 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 0139b66254..f9f196c2de 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 @@ -6,7 +6,6 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.KubernetesResourceList; -import io.fabric8.kubernetes.api.model.authorization.v1.*; import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.V1ApiextensionAPIGroupDSL; import io.fabric8.kubernetes.client.dsl.*; @@ -71,25 +70,6 @@ public static KubernetesClient client(Class clazz, when(v1.customResourceDefinitions()).thenReturn(operation); when(operation.withName(any())).thenReturn(mock(Resource.class)); - var mockSelfSubjectReviewHandler = mock(NamespaceableResource.class); - when(mockSelfSubjectReviewHandler.create()) - .thenReturn(validSelfSubjectRulesReviewForLeaderElection()); - when(client.resource(any(SelfSubjectRulesReview.class))) - .thenReturn(mockSelfSubjectReviewHandler); - return client; } - - private static SelfSubjectRulesReview validSelfSubjectRulesReviewForLeaderElection() { - SelfSubjectRulesReview res = new SelfSubjectRulesReview(); - res.setStatus(new SubjectRulesReviewStatusBuilder() - .withResourceRules(new ResourceRuleBuilder() - .withApiGroups("coordination.k8s.io") - .withResources("leases") - .withVerbs("*") - .build()) - .build()); - return res; - } - } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java index 0c7d409d0e..8a95431885 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java @@ -43,7 +43,7 @@ void initOperator() { @Test @DisplayName("should throw `OperationException` when Configuration is null") - void shouldThrowOperatorExceptionWhenConfigurationIsNull() { + public void shouldThrowOperatorExceptionWhenConfigurationIsNull() { // use a ConfigurationService that doesn't automatically create configurations ConfigurationServiceProvider.reset(); ConfigurationServiceProvider.set(new AbstractConfigurationService(null)); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java index ec7f39916b..d9a6ea0fdd 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/LeaderElectionPermissionIT.java @@ -33,14 +33,18 @@ void operatorStopsIfNoLeaderElectionPermission() { .withImpersonateUsername("leader-elector-stop-noaccess") .build()).build(); + var operator = new Operator(client, o -> { + o.withLeaderElectionConfiguration( + new LeaderElectionConfiguration("lease1", "default")); + o.withStopOnInformerErrorDuringStartup(false); + }); + operator.register(new TestReconciler(), o -> o.settingNamespace("default")); + OperatorException exception = assertThrows( - OperatorException.class, () -> new Operator(client, o -> { - o.withLeaderElectionConfiguration( - new LeaderElectionConfiguration("lease1", "default")); - o.withStopOnInformerErrorDuringStartup(false); - })); + OperatorException.class, + operator::start); - assertThat(exception.getMessage()) + assertThat(exception.getCause().getMessage()) .contains(NO_PERMISSION_TO_LEASE_RESOURCE_MESSAGE); ConfigurationServiceProvider.reset(); }