Ensure All-Users:refs/meta/external-ids doesn't cause split-brains The DESIRED policy was introduced in the early stages of the multi-site to work around locking problems with the global ref-database, which have later been fixed with the introduction of a locking mechanism[1]. Due to the concurrent nature of All-Users:refs/meta/external-ids, the DESIRED policy had been applied by default to avoid causing continuous client failures. The DESIRED policy however, is obsolete and prone to cause spit-brains due to the fact that nothing, really, is done to prevent them: this should not be applied to a crucial core ref such as refs/meta/external-ids. Remove DESIRED policy (that was only used for All-Users:refs/meta/external-ids) and instead make sure that it is REQUIRED to be in sync with the global ref-database. [1] I5395c22ffdf87fcc3247ea9a5d3afc57df60cfe9 Bug: Issue 13462 Change-Id: I6c8fb571e5467fd5596fb8ee1fcdf0179350a051
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/DefaultSharedRefEnforcement.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/DefaultSharedRefEnforcement.java index 63bab09..01cd5c5 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/DefaultSharedRefEnforcement.java +++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/DefaultSharedRefEnforcement.java
@@ -14,22 +14,11 @@ package com.googlesource.gerrit.plugins.multisite.validation.dfsrefdb; -import com.google.common.base.MoreObjects; -import com.google.common.collect.ImmutableMap; - public class DefaultSharedRefEnforcement implements SharedRefEnforcement { - private static final ImmutableMap<String, EnforcePolicy> PREDEF_ENFORCEMENTS = - ImmutableMap.of("All-Users:refs/meta/external-ids", EnforcePolicy.DESIRED); - @Override public EnforcePolicy getPolicy(String projectName, String refName) { - if (isRefToBeIgnoredBySharedRefDb(refName)) { - return EnforcePolicy.IGNORED; - } - - return MoreObjects.firstNonNull( - PREDEF_ENFORCEMENTS.get(projectName + ":" + refName), EnforcePolicy.REQUIRED); + return isRefToBeIgnoredBySharedRefDb(refName) ? EnforcePolicy.IGNORED : EnforcePolicy.REQUIRED; } @Override
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/SharedRefEnforcement.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/SharedRefEnforcement.java index 4de6dea..100def2 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/SharedRefEnforcement.java +++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/SharedRefEnforcement.java
@@ -18,7 +18,6 @@ public interface SharedRefEnforcement { public enum EnforcePolicy { IGNORED, - DESIRED, REQUIRED; }
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index 662f46e..67e942f 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -76,7 +76,7 @@ ```ref-database.enforcementRules.<policy>``` : Level of consistency enforcement across sites on a project:refs basis. - Supports multiple values for enforcing the policy on multiple projects or refs. + Supports two values for enforcing the policy on multiple projects or refs. If the project or ref is omitted, apply the policy to all projects or all refs. The <policy> can be one of the following values: @@ -86,18 +86,14 @@ The user transaction is cancelled. The Gerrit GUI (or the Git client) receives an HTTP 500 - Internal Server Error. - 2. DESIRED - Validate the git ref-update against the shared ref-database. - Any misaligned is logged in errors_log file but the user operation is allowed - to continue successfully. - - 3. IGNORED - Ignore any validation against the shared ref-database. + 2. IGNORED - Ignore any validation against the shared ref-database. *Example:* ``` [ref-database "enforcementRules"] - DESIRED = AProject:/refs/heads/feature + IGNORED = AProject:/refs/heads/feature ``` - Relax the alignment with the shared ref-database for AProject on refs/heads/feature. + Ignore the alignment with the shared ref-database for AProject on refs/heads/feature. Defaults: No rules = All projects are REQUIRED to be consistent on all refs. \ No newline at end of file
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/CustomSharedRefEnforcementByProjectTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/CustomSharedRefEnforcementByProjectTest.java index 4ee3c85..f3008a2 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/CustomSharedRefEnforcementByProjectTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/CustomSharedRefEnforcementByProjectTest.java
@@ -35,16 +35,11 @@ sharedRefDbConfig.setStringList( SharedRefDatabase.SECTION, SharedRefDatabase.SUBSECTION_ENFORCEMENT_RULES, - EnforcePolicy.DESIRED.name(), + EnforcePolicy.IGNORED.name(), Arrays.asList( "ProjectOne", "ProjectTwo:refs/heads/master/test", "ProjectTwo:refs/heads/master/test2")); - sharedRefDbConfig.setString( - SharedRefDatabase.SECTION, - SharedRefDatabase.SUBSECTION_ENFORCEMENT_RULES, - EnforcePolicy.IGNORED.name(), - ":refs/heads/master/test"); refEnforcement = newCustomRefEnforcement(sharedRefDbConfig); } @@ -53,32 +48,32 @@ public void projectOneShouldReturnDesiredForAllRefs() { Ref aRef = newRef("refs/heads/master/2", AN_OBJECT_ID_1); assertThat(refEnforcement.getPolicy("ProjectOne", aRef.getName())) - .isEqualTo(EnforcePolicy.DESIRED); + .isEqualTo(EnforcePolicy.IGNORED); } @Test public void projectOneEnforcementShouldAlwaysPrevail() { Ref aRef = newRef("refs/heads/master/test", AN_OBJECT_ID_1); assertThat(refEnforcement.getPolicy("ProjectOne", aRef.getName())) - .isEqualTo(EnforcePolicy.DESIRED); - } - - @Test - public void aNonListedProjectShouldIgnoreRefForMasterTest() { - Ref aRef = newRef("refs/heads/master/test", AN_OBJECT_ID_1); - assertThat(refEnforcement.getPolicy("NonListedProject", aRef.getName())) .isEqualTo(EnforcePolicy.IGNORED); } @Test - public void projectTwoSpecificRefShouldReturnDesiredPolicy() { + public void aNonListedProjectShouldRequireRefForMasterTest() { + Ref aRef = newRef("refs/heads/master/test", AN_OBJECT_ID_1); + assertThat(refEnforcement.getPolicy("NonListedProject", aRef.getName())) + .isEqualTo(EnforcePolicy.REQUIRED); + } + + @Test + public void projectTwoSpecificRefShouldReturnIgnoredPolicy() { Ref refOne = newRef("refs/heads/master/test", AN_OBJECT_ID_1); Ref refTwo = newRef("refs/heads/master/test2", AN_OBJECT_ID_1); assertThat(refEnforcement.getPolicy("ProjectTwo", refOne.getName())) - .isEqualTo(EnforcePolicy.DESIRED); + .isEqualTo(EnforcePolicy.IGNORED); assertThat(refEnforcement.getPolicy("ProjectTwo", refTwo.getName())) - .isEqualTo(EnforcePolicy.DESIRED); + .isEqualTo(EnforcePolicy.IGNORED); } @Test @@ -103,8 +98,8 @@ } @Test - public void getProjectPolicyForProjectOneShouldRetrunDesired() { - assertThat(refEnforcement.getPolicy("ProjectOne")).isEqualTo(EnforcePolicy.DESIRED); + public void getProjectPolicyForProjectOneShouldReturnIgnored() { + assertThat(refEnforcement.getPolicy("ProjectOne")).isEqualTo(EnforcePolicy.IGNORED); } @Test @@ -120,7 +115,7 @@ @Test public void getProjectPolicyForNonListedProjectWhenSingleProject() { SharedRefEnforcement customEnforcement = - newCustomRefEnforcementWithValue(EnforcePolicy.DESIRED, ":refs/heads/master"); + newCustomRefEnforcementWithValue(EnforcePolicy.IGNORED, ":refs/heads/master"); assertThat(customEnforcement.getPolicy("NonListedProject")).isEqualTo(EnforcePolicy.REQUIRED); } @@ -128,7 +123,7 @@ @Test public void getANonListedProjectWhenOnlyOneProjectIsListedShouldReturnRequired() { SharedRefEnforcement customEnforcement = - newCustomRefEnforcementWithValue(EnforcePolicy.DESIRED, "AProject:"); + newCustomRefEnforcementWithValue(EnforcePolicy.IGNORED, "AProject:"); assertThat(customEnforcement.getPolicy("NonListedProject", "refs/heads/master")) .isEqualTo(EnforcePolicy.REQUIRED); }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/DefaultSharedRefEnforcementTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/DefaultSharedRefEnforcementTest.java index b462408..2964e15 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/DefaultSharedRefEnforcementTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/DefaultSharedRefEnforcementTest.java
@@ -66,6 +66,13 @@ .isEqualTo(EnforcePolicy.REQUIRED); } + @Test + public void allUsersExternalIdsRefShouldBeRequired() { + Ref refOne = newRef("refs/meta/external-ids", AN_OBJECT_ID_1); + assertThat(refEnforcement.getPolicy("All-Users", refOne.getName())) + .isEqualTo(EnforcePolicy.REQUIRED); + } + @Override public String testBranch() { return "fooBranch";