From fbd3949d3bcfa5f92732dbd5afaefe2d0bf2d18a Mon Sep 17 00:00:00 2001 From: "milena.krawczyk" Date: Tue, 9 Jun 2020 17:21:50 +0200 Subject: [PATCH 1/4] Resolve #1211 | Read only admin flag added to Mode Service --- .../tech/hermes/management/api/ModeEndpoint.java | 4 ++-- .../management/domain/health/HealthCheckTask.java | 11 +++++++---- .../hermes/management/domain/mode/ModeService.java | 10 ++++++++-- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/ModeEndpoint.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/ModeEndpoint.java index a02976afb5..f643e83b4d 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/ModeEndpoint.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/ModeEndpoint.java @@ -17,8 +17,8 @@ import static javax.ws.rs.core.MediaType.APPLICATION_JSON; import static javax.ws.rs.core.MediaType.TEXT_PLAIN; -import static pl.allegro.tech.hermes.management.domain.mode.ModeService.ManagementMode.READ_ONLY; import static pl.allegro.tech.hermes.management.domain.mode.ModeService.ManagementMode.READ_WRITE; +import static pl.allegro.tech.hermes.management.domain.mode.ModeService.ManagementMode.READ_ONLY_ADMIN; @Component @Path("/mode") @@ -51,7 +51,7 @@ public Response setMode(@QueryParam("mode") String mode) { modeService.setMode(READ_WRITE); break; case ModeService.READ_ONLY: - modeService.setMode(READ_ONLY); + modeService.setMode(READ_ONLY_ADMIN); break; default: return Response.status(Response.Status.BAD_REQUEST).build(); diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/health/HealthCheckTask.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/health/HealthCheckTask.java index 4a80112c0c..14e19ddd17 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/health/HealthCheckTask.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/health/HealthCheckTask.java @@ -57,10 +57,13 @@ private HealthCheckResult doHealthCheck(ZookeeperClient zookeeperClient) { } private void updateMode(List healthCheckResults) { - if (healthCheckResults.contains(HealthCheckResult.UNHEALTHY)) { - modeService.setMode(ModeService.ManagementMode.READ_ONLY); - } else { - modeService.setMode(ModeService.ManagementMode.READ_WRITE); + /* ReadOnly set by admin can be changed to ReadWrite only by admin */ + if (!modeService.isReadOnlySetByAdmin()) { + if (healthCheckResults.contains(HealthCheckResult.UNHEALTHY)) { + modeService.setMode(ModeService.ManagementMode.READ_ONLY); + } else { + modeService.setMode(ModeService.ManagementMode.READ_WRITE); + } } } diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/mode/ModeService.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/mode/ModeService.java index df1e2d92f0..b5fbbb1f63 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/mode/ModeService.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/mode/ModeService.java @@ -7,10 +7,12 @@ public class ModeService { public static final String READ_WRITE = "readWrite"; public static final String READ_ONLY = "readOnly"; + public static final String READ_ONLY_ADMIN = "readOnlyAdmin"; public enum ManagementMode { READ_WRITE(ModeService.READ_WRITE), - READ_ONLY(ModeService.READ_ONLY); + READ_ONLY(ModeService.READ_ONLY), + READ_ONLY_ADMIN(ModeService.READ_ONLY_ADMIN); private final String text; @@ -35,6 +37,10 @@ public void setMode(ManagementMode mode) { } public boolean isReadOnlyEnabled() { - return mode == ManagementMode.READ_ONLY; + return mode == ManagementMode.READ_ONLY || mode == ManagementMode.READ_ONLY_ADMIN; + } + + public boolean isReadOnlySetByAdmin() { + return mode == ManagementMode.READ_ONLY_ADMIN; } } From 23ebfaa5c1957f4f9b6e7bfa8453baa9482612f9 Mon Sep 17 00:00:00 2001 From: "milena.krawczyk" Date: Wed, 10 Jun 2020 11:05:35 +0200 Subject: [PATCH 2/4] Resolve #1211 | Tests added for READ_ONLY_ADMIN flag --- .../domain/health/HealthCheckTaskTest.groovy | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/health/HealthCheckTaskTest.groovy b/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/health/HealthCheckTaskTest.groovy index aed11162b0..55c0264055 100644 --- a/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/health/HealthCheckTaskTest.groovy +++ b/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/health/HealthCheckTaskTest.groovy @@ -90,6 +90,39 @@ class HealthCheckTaskTest extends MultiZookeeperIntegrationTest { successfulCounter.count() == 3 } + def "should not change status to READ_WRITE if READ_ONLY is set by admin"() { + given: + modeService.setMode(ModeService.ManagementMode.READ_ONLY_ADMIN) + + when: + zookeeper1.start() + healthCheckTask.run() + + then: + modeService.readOnlyEnabled + + and: + successfulCounter.count() == 2 + } + + def "should change status from READ_ONLY_ADMIN to READ_WRITE only by admin"() { + given: + modeService.setMode(ModeService.ManagementMode.READ_ONLY_ADMIN) + + when: + zookeeper1.start() + healthCheckTask.run() + + then: + modeService.readOnlyEnabled + + when: + modeService.setMode(ModeService.ManagementMode.READ_WRITE) + + then: + !modeService.readOnlyEnabled + } + static setupZookeeperPath(ZookeeperClient zookeeperClient, String path) { def healthCheckPathExists = zookeeperClient.curatorFramework .checkExists() From d54d18a05798ce1ece7d10814fff12926571089d Mon Sep 17 00:00:00 2001 From: "milena.krawczyk" Date: Wed, 10 Jun 2020 11:58:33 +0200 Subject: [PATCH 3/4] Resolve #1211 | Thread-safe version --- .../management/domain/health/HealthCheckTask.java | 11 ++++------- .../hermes/management/domain/mode/ModeService.java | 8 +++++--- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/health/HealthCheckTask.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/health/HealthCheckTask.java index 14e19ddd17..cbb79b8a8a 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/health/HealthCheckTask.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/health/HealthCheckTask.java @@ -57,13 +57,10 @@ private HealthCheckResult doHealthCheck(ZookeeperClient zookeeperClient) { } private void updateMode(List healthCheckResults) { - /* ReadOnly set by admin can be changed to ReadWrite only by admin */ - if (!modeService.isReadOnlySetByAdmin()) { - if (healthCheckResults.contains(HealthCheckResult.UNHEALTHY)) { - modeService.setMode(ModeService.ManagementMode.READ_ONLY); - } else { - modeService.setMode(ModeService.ManagementMode.READ_WRITE); - } + if (healthCheckResults.contains(HealthCheckResult.UNHEALTHY)) { + modeService.compareAndSwapMode(ModeService.ManagementMode.READ_WRITE, ModeService.ManagementMode.READ_ONLY); + } else { + modeService.compareAndSwapMode(ModeService.ManagementMode.READ_ONLY, ModeService.ManagementMode.READ_WRITE); } } diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/mode/ModeService.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/mode/ModeService.java index b5fbbb1f63..eeb583fdf8 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/mode/ModeService.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/mode/ModeService.java @@ -32,7 +32,7 @@ public ManagementMode getMode() { return mode; } - public void setMode(ManagementMode mode) { + public synchronized void setMode(ManagementMode mode) { this.mode = mode; } @@ -40,7 +40,9 @@ public boolean isReadOnlyEnabled() { return mode == ManagementMode.READ_ONLY || mode == ManagementMode.READ_ONLY_ADMIN; } - public boolean isReadOnlySetByAdmin() { - return mode == ManagementMode.READ_ONLY_ADMIN; + public synchronized void compareAndSwapMode(ManagementMode expectedMode, ManagementMode newMode) { + if (mode.equals(expectedMode)) { + setMode(newMode); + } } } From 002db3d230b092c4fc57277435787cdf654191e2 Mon Sep 17 00:00:00 2001 From: "milena.krawczyk" Date: Mon, 15 Jun 2020 10:50:03 +0200 Subject: [PATCH 4/4] Resolve #1211 | Changes after code review; admin endpoint accepts ReadOnlyAdmin flag as well --- .../allegro/tech/hermes/management/api/ModeEndpoint.java | 5 +++-- .../hermes/management/domain/health/HealthCheckTask.java | 4 ++-- .../tech/hermes/management/domain/mode/ModeService.java | 9 +++++---- .../management/domain/health/HealthCheckTaskTest.groovy | 8 +++----- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/ModeEndpoint.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/ModeEndpoint.java index f643e83b4d..c07ae31509 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/ModeEndpoint.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/ModeEndpoint.java @@ -48,10 +48,11 @@ public Response setMode(@QueryParam("mode") String mode) { } switch (mode) { case ModeService.READ_WRITE: - modeService.setMode(READ_WRITE); + modeService.setModeByAdmin(READ_WRITE); break; case ModeService.READ_ONLY: - modeService.setMode(READ_ONLY_ADMIN); + case ModeService.READ_ONLY_ADMIN: + modeService.setModeByAdmin(READ_ONLY_ADMIN); break; default: return Response.status(Response.Status.BAD_REQUEST).build(); diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/health/HealthCheckTask.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/health/HealthCheckTask.java index cbb79b8a8a..4a80112c0c 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/health/HealthCheckTask.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/health/HealthCheckTask.java @@ -58,9 +58,9 @@ private HealthCheckResult doHealthCheck(ZookeeperClient zookeeperClient) { private void updateMode(List healthCheckResults) { if (healthCheckResults.contains(HealthCheckResult.UNHEALTHY)) { - modeService.compareAndSwapMode(ModeService.ManagementMode.READ_WRITE, ModeService.ManagementMode.READ_ONLY); + modeService.setMode(ModeService.ManagementMode.READ_ONLY); } else { - modeService.compareAndSwapMode(ModeService.ManagementMode.READ_ONLY, ModeService.ManagementMode.READ_WRITE); + modeService.setMode(ModeService.ManagementMode.READ_WRITE); } } diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/mode/ModeService.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/mode/ModeService.java index eeb583fdf8..6b8583d0a5 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/mode/ModeService.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/mode/ModeService.java @@ -32,7 +32,7 @@ public ManagementMode getMode() { return mode; } - public synchronized void setMode(ManagementMode mode) { + public synchronized void setModeByAdmin(ManagementMode mode) { this.mode = mode; } @@ -40,9 +40,10 @@ public boolean isReadOnlyEnabled() { return mode == ManagementMode.READ_ONLY || mode == ManagementMode.READ_ONLY_ADMIN; } - public synchronized void compareAndSwapMode(ManagementMode expectedMode, ManagementMode newMode) { - if (mode.equals(expectedMode)) { - setMode(newMode); + public synchronized void setMode(ManagementMode newMode) { + /* READ_ONLY_ADMIN is a flag that can be changed only by admin */ + if (!mode.equals(ManagementMode.READ_ONLY_ADMIN)) { + this.mode = newMode; } } } diff --git a/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/health/HealthCheckTaskTest.groovy b/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/health/HealthCheckTaskTest.groovy index 55c0264055..bdff69be31 100644 --- a/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/health/HealthCheckTaskTest.groovy +++ b/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/health/HealthCheckTaskTest.groovy @@ -92,10 +92,9 @@ class HealthCheckTaskTest extends MultiZookeeperIntegrationTest { def "should not change status to READ_WRITE if READ_ONLY is set by admin"() { given: - modeService.setMode(ModeService.ManagementMode.READ_ONLY_ADMIN) + modeService.setModeByAdmin(ModeService.ManagementMode.READ_ONLY_ADMIN) when: - zookeeper1.start() healthCheckTask.run() then: @@ -107,17 +106,16 @@ class HealthCheckTaskTest extends MultiZookeeperIntegrationTest { def "should change status from READ_ONLY_ADMIN to READ_WRITE only by admin"() { given: - modeService.setMode(ModeService.ManagementMode.READ_ONLY_ADMIN) + modeService.setModeByAdmin(ModeService.ManagementMode.READ_ONLY_ADMIN) when: - zookeeper1.start() healthCheckTask.run() then: modeService.readOnlyEnabled when: - modeService.setMode(ModeService.ManagementMode.READ_WRITE) + modeService.setModeByAdmin(ModeService.ManagementMode.READ_WRITE) then: !modeService.readOnlyEnabled