[#1050] Document the replication repair control, and test that a repaired modify stays on its replica

[vharseko]

Follow-up to #1050, where an administrator asked how to give entries back the entryUUID values another directory had assigned to them, without breaking the replication topology.

The answer is the replication repair request control (1.3.6.1.4.1.26027.1.5.2), which the server has had since the ForgeRock days but which was only listed by its OID among the supported controls. This PR:

• names the control in the reference appendix (asciidoc and docbook) and says what it does: the attributes replication owns can be written, on the replica the request is sent to only, without a message to the replication servers and without a trace in ds-sync-hist. Both entries also say what else the control turns off, since it marks the whole request a synchronization operation: schema and value-syntax checking, the pre-operation plugins, and password policy handling - a userPassword value in such a request is stored in the clear;
• adds "Repairing an Entry on One Replica" to the replication chapter of the administration guide. It gives the ldapmodify --control procedure to run on each replica, the check to make afterwards, what the control stops checking, and who may use it: a client with bypass-acl or one an ACI allows. It ends with a warning about repairing some replicas and not others. While that lasts, every replicated change to the entry is dropped in either direction with no error-log record: a modify, a rename, a delete. An entry added under the repaired entry, or moved under it, becomes an unresolved conflict entry on the other side;
• makes the server check the control against the ACI of the controls on an add and a delete. On those two operations the replication plugin sees the request before the backend checks its controls, and MultimasterReplication.findDomain() used to take the repair control off the request without asking whether the client may use it. Now a control the client may not use is left on the request, for the backend to drop or refuse, as on a modify. The question is asked at the entry being repaired, on an add as well. Because those two operations also reach the plugin before the backend applies proxied authorization, a repair is refused on an add or a delete that carries a proxied authorization control: the answer would otherwise be given for the bound client rather than for the identity the request runs as. The chapter and both appendix entries say so;
• turns ReplicationRepairControlTest, which covered the add case on an unreplicated suffix, into a test over a replicated suffix with a broker listening on the replication server. It now also checks that a modify of entryUUID and nsUniqueId under the control succeeds where the same modify without it is refused, reaches neither the replication server nor the error log, leaves the history of the entry untouched, and leaves the entry replicating as any other under its repaired entryUUID; that the control has to be sent as not critical on a modify; that a client which no ACI lets use the control gets no repair on a modify, an add or a delete, while one an ACI does let use it gets the repair; that a critical control such a client sends on a delete is refused with UNAVAILABLE_CRITICAL_EXTENSION rather than dropped; that an ACI targeted at some entries lets the client repair a delete and an add of those entries only; and that a proxied add or delete is not repaired, even for a proxy with bypass-acl. ReplicationRepairControlStandaloneTest checks the repaired modify on a server with no replication domain. (A line-break missing from the LDIF of the add case is restored on the way: the entry was being added with an object class named personobjectClass: organizationalPerson, which the skipped schema check let through.)

Verified on this head, with ReplicationRepairControlTest and ReplicationRepairControlStandaloneTest:

• unmutated: Tests run: 10, Failures: 0 and Tests run: 1, Failures: 0; the whole org/opends/server/replication/plugin/** package in one run: Tests run: 332, Failures: 0.
• with the proxied-authorization refusal disabled, a refused control dropped instead of left on the request (it.remove() before the break), and the ACI asked at dn.parent(), in one run: exactly three cases are red, each on its own assertion. aProxiedAddOrDeleteIsNotRepaired fails with "a proxied add was repaired", aCriticalRepairControlAClientMayNotUseIsRefusedOnADelete with "a critical control the client may not use was dropped, and the delete published", theRepairControlAciIsJudgedAtTheEntryBeingRepaired with "a delete inside the ACI's target was published".
• the ACI asked at the parent entry on an add only: red on the add arm of theRepairControlAciIsJudgedAtTheEntryBeingRepaired, "an add inside the ACI's target was not repaired". The proxied refusal kept for an add only: red on the delete arm of aProxiedAddOrDeleteIsNotRepaired, "a proxied delete was repaired".
• the javadoc gate (-Pprecommit package on JDK 11) passes, and turns red with error: reference not found when a broken {@link} is put into the javadoc of findDomain.

Measured on the previous head (807791f0c5), where the ACI check on an add and a delete was added:

• with the ACI check in findDomain() disabled, and the domains.isEmpty() early return moved above the repair loop, in one run: exactly three cases are red, each on its own assertion. aClientNoAciLetsUseTheControlGetsNoRepair fails with "a client no ACI lets use the control repaired an add". aDeleteByAClientNoAciLetsUseTheControlIsPublished fails with "the delete was not published". aRepairedModifySetsTheEntryUUIDWithNoReplicationDomain fails with the NO-USER-MODIFICATION refusal of the repaired modify.

Measured on an earlier head (b30ee7d2fb), where the cases below were added:

• op.setDontSynchronize(true) deleted from the repair branch of MultimasterReplication.findDomain: red, but at connection.modify() itself, with NullPointerException (Objects.java:220 TreeMap.java:381 TreeMap.java:291 RemotePendingChanges.java:272 LDAPReplicationDomain.java:2402 MultimasterReplication.java:741 ...) - with no OperationContext on the operation, synchronize() takes the replay branch into remotePendingChanges.commit(null). No assertion of the class is reached, so this mutant pins none of them; the previous revision of this description said it did, and was wrong.
• that deletion together with the return null after it.remove(), which hands the repair to the replication layer as a change of its own: the modify succeeds and the class is red on the error-log assertion, msgID=9 msg=Internal Error : Operation ModifyOperation(connID=1, opID=4, dn=cn=repair me,o=test) change number ... was not found in pending list. That is the behaviour the Relax Rules control has on a replicated suffix (A Relax Rules change on a replicated suffix is not replicated and logs an internal error #1051), which is what the assertion is there for.
• LocalBackendModifyOperation.processRequestControls() given a branch for the control, so the modify road stops refusing it when it is critical - which is what the add road does today: red on aCriticalRepairControlIsRefusedOnAModify.
• the ds-sync-hist assertion is held by two independent things, so no single change turns it red: findDomain() returns null for a repair, so MultimasterReplication.doPreOperation() never reaches the history write, and EntryHistorical.setHistoricalAttrToOperation() computes records only if (!modifyOperation.isSynchronizationOperation()). Letting the history write see the operation alone leaves the class green; breaking both - the mutant above plus that guard - turns it red, with entryUUID:<csn>:repl:... and nsUniqueId:<csn>:repl:... added to the entry. The assertion reads the attribute by name and compares the whole value set, and the case first checks that there is a history to keep, so it is not comparing nothing with nothing.

The asciidoc is rendered by the man-pages profile of opendj-doc-generated-ref, which activates on Linux only, so the build-maven (ubuntu-latest, *) legs are what covers it.

The Relax Rules control, which looks like the alternative for this, behaves differently on a replicated suffix - see #1051.

Related: #1050.

[maximthomas]

praise: The control finally has a name and a procedure, and the test now reads the topology instead of trusting it.

• reference/appendix-controls.adoc:200-206 turns a bare OID in the supported-controls list into an entry that says what the control does and links to the procedure.
• ReplicationRepairControlTest moved from an unreplicated suffix to a replicated one with a broker on the replication server (:107), so "the repair is not published" is read off the wire by nextUpdate() (:304) rather than assumed.
• The LDIF line break restored at :139/:195 is the same bug the docs are about: personobjectClass: organizationalPerson got in because the control had turned schema checking off.

---

issue (blocking): The partial-repair WARNING names only a later conflict resolution; in fact every replicated change to the entry is dropped, in both directions, while the replicas disagree on its entryUUID.

opendj-doc-generated-ref/src/main/asciidoc/admin-guide/chap-replication.adoc:2041-2044

With replica A repaired to X and replica B still at Y, a modify applied on A is published with ctx entryUUID X (LDAPReplicationDomain:2207-2208). Replayed on B, the ctx != null arm reads Y and returns StopProcessing(NO_SUCH_OBJECT, null) (:2219-2235); solveNamingConflict then calls findEntryDN(X), finds nothing on B, and takes the else arm — numResolvedNamingConflicts.incrementAndGet(); return ConflictResolution.NOTHING_TO_DO (:4105-4119). There is no error-log record on that arm, only the monitor counter. So a client write accepted on one replica is discarded on the other with no trace — which is the state the procedure itself creates between its "apply the change to each replica in turn" steps, and the durable state the WARNING is about. The reader cannot derive that from "the replicas no longer agree on which entry that is".

[WARNING]
======
A repair that reaches some replicas and not others leaves the topology inconsistent. A replica initialized from one that was not repaired takes the old values with it, and when replication later resolves a conflict on the entry by its `entryUUID`, the replicas no longer agree on which entry that is.

Until every replica carries the repaired value, a change made to the entry on one side of the difference is also discarded on the other: the replayed change names the entry by the `entryUUID` of the replica it came from, the receiving replica finds no entry with that value, and the change is dropped without an error-log record - only the `resolved-naming-conflicts` monitor attribute moves.
======

---

issue (blocking): The documented contract is narrower than the control: it marks the whole request a synchronization operation, so schema and value-syntax checking, every pre-operation plugin and password handling are off for everything that request carries.

opendj-doc-generated-ref/src/main/asciidoc/admin-guide/chap-replication.adoc:1994, reference/appendix-controls.adoc:206, docbkx/admin-guide/appendix-controls.xml:354

MultimasterReplication.findDomain runs op.setSynchronizationOperation(true); op.setDontSynchronize(true); for the operation (:163-164) — nothing narrows it to entryUUID/ds-sync-hist. Downstream, LocalBackendModifyOperation.mustCheckSchema() is !isSynchronizationOperation() && ...isCheckSchema() (:1202-1204) and gates the entry schema check (:467) and the value-syntax check on the add-values (:1175) and replace-values (:1352) roads — the road the documented example takes, so replace: entryUUID with a mistyped non-UUID value returns "MODIFY operation successful"; invokePreModifyPlugins() runs only if (!isSynchronizationOperation()) (:323-330), skipping attribute uniqueness and referential integrity; passwordChanged is gated on the same flag (:800). The NO-USER-MODIFICATION refusal itself is t.isNoUserModification() && !isInternalOrSynchro(m) (:729-734, :770), which is why this PR's own test can repair nsUniqueId (ReplicationRepairControlTest:279-281) — an attribute replication does not own, that nothing in the server writes (resource/schema/00-core.ldif:454-457). The appendix sentence "for writing the attributes replication owns" has a counter-example in this PR.

The replication repair request control (OID `1.3.6.1.4.1.26027.1.5.2`) lets an administrator write these attributes anyway. A request that carries the control changes the replica it is sent to, and that replica only: the change is not published to the replication servers, and it is not recorded in the history of the entry. The control marks the whole request a synchronization operation, so schema and value-syntax checking, the pre-operation plugins - attribute uniqueness, referential integrity - and password policy handling do not run for it either: a mistyped value is accepted without an error. Put nothing but the repair in the request. Use it when the replicas already agree on the data, but an entry must be given specific values - for example, the `entryUUID` that another directory service assigned to it before a migration, and that client applications still hold.

The same clause belongs in both appendix entries, which today say only "for writing the attributes replication owns".

---

issue (non-blocking): The mutant the description reports is killed by the modify blowing up, not by the new assertions — neither of them is evaluated.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplicationRepairControlTest.java:257, :260-262

Measured, two runs of the class at this head. Unmutated: tests=3 failures=0, suite 110.067 s, aRepairedModifySetsTheEntryUUIDOnThisReplicaOnly green in 4.148 s. With op.setDontSynchronize(true); deleted from the OID_REPLICATION_REPAIR_CONTROL branch of findDomain: tests=3 failures=1 skipped=1, the method red in 0.137 s with org.forgerock.opendj.ldap.LdapException: Other: Worker Thread 3 encountered an uncaught exception ... NullPointerException (Objects.java:220 TreeMap.java:381 TreeMap.java:291 RemotePendingChanges.java:265 LDAPReplicationDomain.java:2313 MultimasterReplication.java:740 LocalBackendModifyOperation.java:1666) — no AssertionError, no frame in the test file. With the operation still isSynchronizationOperation() and carrying no OperationContext, synchronize() takes the replay branch into remotePendingChanges.commit(null), TreeMap.get(null) throws NPE, and the catch (NoSuchElementException) at LDAPReplicationDomain:2315 — the only road to ERR_OPERATION_NOT_FOUND_IN_PENDING — never fires. The third case is TestNG-skipped under that mutant, so it pins nothing about it either.

    TestCaseUtils.ERROR_TEXT_WRITER.clear();
    connection.modify(repairRequest(true, repairedUUID, repairedNsUniqueId));
    ...
    final List<String> records = new ArrayList<>(TestCaseUtils.ERROR_TEXT_WRITER.getMessages());
    assertThat(records).as("the repair was reported as a change missing from the pending list")
        .noneMatch(record -> record.contains(NOT_IN_PENDING));
    assertThat(records).as("the repair was reported to the error log")
        .noneMatch(record -> record.contains("severity=ERROR"));

Under that mutant the worker thread's uncaught NullPointerException is itself an ERROR record, so the second line is red for a reason the test states — but only if it is reached, which means taking the connection.modify in a try/finally. Short of that, the "Verified:" line of the description should say what was measured: the class goes red at connection.modify(), not at the assertions.

---

issue (non-blocking): The repaired entryUUID is the same literal the add case gives to another entry, so the suffix ends the class with two entries under one entryUUID.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplicationRepairControlTest.java:237, :146, :203

d5b910d8-47cb-4ac0-9e5f-0f4a77de58d4 is both the value testRepairControl adds to uid=test.repair,o=test and the value aRepairedModifySetsTheEntryUUIDOnThisReplicaOnly repairs cn=repair me,o=test to. Nothing reads by UUID today, so nothing fails; a case that goes through findEntryDN — a conflict-resolution case, or the partial-repair scenario the WARNING describes — would pick either entry. A second literal costs one line, and it is the exact corruption this control can cause.

---

issue (non-blocking): NOT_IN_PENDING is the bare prefix "msgID=9", which matches any message whose ordinal begins with 9, from any bundle.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplicationRepairControlTest.java:71-72

ERR_OPERATION_NOT_FOUND_IN_PENDING has ordinal 9, and both publishers on ERROR_TEXT_WRITER write the plain ordinal with no resource name — [<time>] category=... severity=... msgID=<ordinal> msg=... (TextErrorLogPublisher:460-470; the msgID=<resource>-<ordinal> form is ThreadFilterTextErrorLogPublisher:82, which is not wired to this writer). TestCaseUtils:543-548 adds the tool-startup publisher with every Severity, so msgID=90, msgID=97, msgID=912 of any subsystem also match. The failure direction is a false red, not a false green.

  private static final String NOT_IN_PENDING =
      "msgID=" + ERR_OPERATION_NOT_FOUND_IN_PENDING.get("", "").ordinal() + " ";

The record always has msg= after the ordinal, so the trailing space makes the match exact.

---

suggestion (non-blocking): The second thing the docs promise — "it is not recorded in the history of the entry" — is asserted nowhere.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplicationRepairControlTest.java:252-262

The class reads entryUUID and nsUniqueId off the repaired entry and never reads ds-sync-hist. At this head the property holds because the second findDomain returns null at MultimasterReplication:143 (if (op.dontSynchronize()) return null;), so doPreOperation never reaches historicalInformation.setHistoricalAttrToOperation(modifyOperation) (:485). Drop that guard and the repaired entry grows a history record while the backend write still happens — the client call then dies in post-op with the NPE above, which is why the read has to survive it.

    final String historyBeforeTheRepair = attributeOfRepairedEntry("ds-sync-hist");
    TestCaseUtils.ERROR_TEXT_WRITER.clear();
    try
    {
      connection.modify(repairRequest(true, repairedUUID, repairedNsUniqueId));
    }
    finally
    {
      assertThat(attributeOfRepairedEntry("ds-sync-hist"))
          .as("the repair was recorded in the history of the entry")
          .isEqualTo(historyBeforeTheRepair);
    }

---

suggestion (non-blocking): Nothing in the class sends the control critical, so the chapter's "send it as not critical" rule — and the it.remove() that makes it matter — is pinned by nothing.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplicationRepairControlTest.java:286, opendj-doc-generated-ref/src/main/asciidoc/admin-guide/chap-replication.adoc:2019

GenericControl.newControl(OID) and the tool's bare -J 1.3.6.1.4.1.26027.1.5.2 both build new GenericControl(oid, false, null). The sentence the chapter now makes a rule is true and asymmetric, which is what makes it worth a case: on a modify the backend's control check runs first (LocalBackendModifyOperation:408, refusing at :711) and conflict resolution only at :431, so a critical control is refused; on an add the order is inverted (LocalBackendAddOperation:270 then :403), so MultimasterReplication:170 takes the control off first and a critical control works. Deleting that it.remove() today leaves all three cases green.

    final ModifyRequest critical = Requests.newModifyRequest(REPAIRED_DN)
        .addModification(ModificationType.REPLACE, "entryUUID", repairedUUID)
        .addControl(GenericControl.newControl(OID_REPLICATION_REPAIR_CONTROL, true));
    assertThatThrownBy(() -> connection.modify(critical))
        .isInstanceOf(LdapException.class)
        .extracting(e -> ((LdapException) e).getResult().getResultCode())
        .isEqualTo(ResultCode.UNAVAILABLE_CRITICAL_EXTENSION);

Or: send the add of testRepairControl with -J 1.3.6.1.4.1.26027.1.5.2:true, which succeeds today and returns 12 once it.remove() is gone.

---

suggestion (non-blocking): nextUpdate() spends one budget on both the reads that expect a message and the reads that expect none.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplicationRepairControlTest.java:304-306

SECONDS.toNanos(4) is the whole deadline, and :235 and :272 use it as "long enough for the replication server to forward". Locally the positive read cost ~0.1 s of the method's 4.148 s — the rest is the one negative wait — and the five build-maven (ubuntu-latest, *) cells are green at this head, so the margin holds today. Worth separating only if a case is added that waits for a message the server has to open a cursor for.

---

nitpick (non-blocking): The example sends a bare --control <oid> where the same chapter spells the criticality out.

opendj-doc-generated-ref/src/main/asciidoc/admin-guide/chap-replication.adoc:2009

:1529 and :1754 both write --control "1.3.6.1.4.1.26027.1.5.4:false". The default is what the prose at :2019 says — a one-element --control value builds new GenericControl(oid, false, null) — so nothing is wrong; --control "1.3.6.1.4.1.26027.1.5.2:false" just matches the chapter's existing form and leaves the reader nothing to take on trust.

[vharseko]

Round 2. Every point checked against the code; measurements below are from runs of ReplicationRepairControlTest on this head.

issue (blocking) - the WARNING names only a later conflict resolution. Taken, and widened. The road is as you describe: the ctx == null arm of handleConflictResolution(PreOperationModifyOperation) builds the context from getEntryUUID(modifiedEntry), so the published change carries the repaired UUID; on the other replica the ctx != null arm returns StopProcessing(NO_SUCH_OBJECT, null), solveNamingConflict(ModifyOperation, ModifyMsg) finds nothing for findEntryDN() and returns NOTHING_TO_DO, and the replay loop then does replayDone = true; recordChangeResolved(csn) - no record anywhere, only the counter.

One addition: a delete goes the same way. handleConflictResolution(PreOperationDeleteOperation) returns the same NO_SUCH_OBJECT when the UUIDs disagree, and solveNamingConflict(DeleteOperation, LDAPUpdateMsg) takes the currentDN == null arm - numResolvedNamingConflicts.incrementAndGet(); return NOTHING_TO_DO. So in that window a deletion accepted on one replica leaves the entry standing on the other, which reads differently to an administrator than a lost modify. The paragraph says both, and ends by tying the window back to the procedure's own "while nothing else changes the entry".

issue (blocking) - the documented contract is narrower than the control. Taken, in both appendix entries and in the chapter. Confirmed what the flag reaches: mustCheckSchema() gates the entry check and the syntax check on both the add-values and the replace-values roads; invokePreModifyPlugins() is skipped whole, which is UniqueAttributePlugin, ReferentialIntegrityPlugin.doPreOperation(PreOperationModifyOperation) and LastModPlugin - so modifiersName and modifyTimestamp do not move either; nsUniqueId is NO-USER-MODIFICATION in 00-core.ldif and written by nothing in src/main, which is the counter-example you point at.

One thing named more sharply than in your text: password handling is not only "policy checks skipped". preProcessPasswordModification() runs under if (!isSynchronizationOperation()), and the encoding itself lives inside it - preProcessPasswordAddOrReplace() ends with builder.addAll(pwPolicyState.encodePassword(v)). So a userPassword value carried in a repair request is stored exactly as it arrived, in the clear. The docs say that rather than "a mistyped value is accepted".

For the mistyped entryUUID: it is accepted, and the value is stored - UUIDEqualityMatchingRuleImpl.normalize() does throw on it, but AttributeIndex.indexEntry() swallows the DecodeException into logger.traceException(e), so the write succeeds and the value is simply not indexed.

issue (non-blocking) - the mutant is killed by the modify blowing up. Correct, measured: with op.setDontSynchronize(true) deleted the class is red at connection.modify() with NullPointerException (Objects.java:220 TreeMap.java:381 TreeMap.java:291 RemotePendingChanges.java:272 LDAPReplicationDomain.java:2402 MultimasterReplication.java:741 ...), at 0.3 s, with no frame in the test file. The "Verified:" line of the description is rewritten to say that.

The assertion is not unpinned, though - it wanted a second line removed. Deleting op.setDontSynchronize(true) and the return null after it.remove() hands the repair to the replication layer as a change of its own: the modify then succeeds and the class is red on the error-log assertion itself, with msgID=9 msg=Internal Error : Operation ModifyOperation(connID=1, opID=4, dn=cn=repair me,o=test) change number ... was not found in pending list. That is the shape the Relax Rules control has on a replicated suffix (#1051), which is the regression the assertion is for. The description now names that mutant.

The blanket noneMatch(record -> record.contains("severity=ERROR")) from your snippet I did not take: it is wider than what the docs promise and would fail on any unrelated ERROR the broker or the replication server writes in the same window. The try/finally I did take - around the history assertion below, which gives it to the whole case.

issue (non-blocking) - the repaired entryUUID is the add case's literal. Taken. REPAIRED_UUID / REPAIRED_NS_UNIQUE_ID are now the values the chapter uses in its example, distinct from the ones testRepairControl() gives uid=test.repair, with a comment saying why.

issue (non-blocking) - NOT_IN_PENDING is a bare prefix. Taken, with the trailing space, and the comment says what it is for.

suggestion (non-blocking) - "not recorded in the history" is asserted nowhere. Taken, in the try/finally form.

Two changes to your snippet. First, the read: parseAttribute(...).asString() on "*", "+" came back empty, so the assertion as written compared nothing with nothing - I found that only because the mutants that should have moved the history left it green. The helper now asks for the attribute by name as well, returns the whole value set, and the case asserts up front that there is a history to keep.

Second, what it pins. Two independent things keep the history clean, and no single change turns the assertion red: findDomain() returns null for a repair so doPreOperation() never reaches the write, and setHistoricalAttrToOperation() computes records only if (!modifyOperation.isSynchronizationOperation()). Passing the operation through to the history write alone - findDomain(operationDN, null) in doPreOperation - leaves the class green. Breaking both turns it red, with entryUUID:<csn>:repl:... and nsUniqueId:<csn>:repl:... on the entry. The description says this rather than claiming a one-line mutant.

suggestion (non-blocking) - nothing sends the control critical. Taken, as the modify case. Measured: with a branch for the control added to LocalBackendModifyOperation.processRequestControls() - so the modify road stops refusing a critical control it now knows, which is what the add road does - the new case is red.

Your second option, sending the add critical, I did not take. It does pin it.remove(), which the modify case does not (the modify is refused at processRequestControls() before findDomain() is ever called, with that line or without it). But it would fix as expected the add/modify asymmetry itself - a critical control accepted on add, refused on modify - which is the thing the chapter tells the administrator not to rely on. I would rather leave that unpinned than pin it as intended behaviour. If the asymmetry is worth a look on its own, I will open an issue for it.

suggestion (non-blocking) - nextUpdate() spends one budget on both kinds of read. Left as is, on your own condition: the case added here is a refusal, so it waits for no message the server has to open a cursor for.

nitpick - bare --control <oid>. Taken, --control "1.3.6.1.4.1.26027.1.5.2:false", matching :1529 and :1754.

[maximthomas]

praise: the new commit answers round 1 point by point and corrects its own evidence.

• The WARNING now says what a half-done repair costs, in both directions: the change is dropped without an error-log record and only resolved-naming-conflicts moves (chap-replication.adoc:2047). The contract paragraph (:1996) and both appendix entries say the control makes the whole request a synchronization operation.
• The PR description now says the setDontSynchronize mutant goes red at connection.modify() with no assertion reached, and adds a two-part mutant that does reach the error-log assertion.
• aCriticalRepairControlIsRefusedOnAModify pins the chapter's "send it as not critical" rule, and NOT_IN_PENDING now ends with a space, so msgID=9 no longer matches msgID=90–msgID=99.

---

issue (non-blocking): The procedure's precondition and the WARNING cover only changes to the repaired entry, not changes to the entries below it.

opendj-doc-generated-ref/src/main/asciidoc/admin-guide/chap-replication.adoc:2001, :2047

Suppose one replica has the repaired entryUUID and another still has the old one. An entry added under the repaired entry is replayed with the parent's entryUUID from the replica where it was added. The receiving replica finds no parent with that value (LDAPReplicationDomain.java:2018-2039). solveNamingConflict then renames the new entry to a conflict RDN directly under the base DN, raises ALERT_TYPE_REPLICATION_UNRESOLVED_CONFLICT and increments the unresolved-naming-conflicts counter (:4952-4969). A modify DN that moves an entry under the repaired entry is marked as a conflict the same way (:4862-4867). Nothing is lost silently, but for a container entry "while nothing else changes the entry" does not tell the reader to freeze its subtree too.

. Apply the change to each replica in turn, while nothing else changes the entry or the entries below it:

In the WARNING, after the modify/delete sentence: "An entry added under the repaired entry, or moved under it, on one side of the difference becomes an unresolved conflict entry directly under the base DN on the other side, and replication raises an alert."

---

issue (non-blocking): The section says the control lets "an administrator" write these attributes, but it does not say that the control is subject to access control.

opendj-doc-generated-ref/src/main/asciidoc/admin-guide/chap-replication.adoc:1994, opendj-doc-generated-ref/src/main/asciidoc/reference/appendix-controls.adoc:206, opendj-doc-generated-ref/src/main/docbkx/admin-guide/appendix-controls.xml:354

On a modify, processRequestControls() (LocalBackendModifyOperation.java:408) runs removeAllDisallowedControls before the replication plugin sees the control (:431). For a client without the bypass-acl privilege, the control ACI decides, and neither default global targetcontrol ACI (resource/config/config.ldif:84, :86) lists 1.3.6.1.4.1.26027.1.5.2. A delegated administrator who follows the procedure exactly, with the control not critical, has the control dropped silently. The modify then fails with the NO-USER-MODIFICATION refusal, the same error as a request with no control, and that error points at the schema, not at the missing ACI. The example binds as cn=Directory Manager, which has bypass-acl, so the example works as written. The test configuration grants targetcontrol="*", so no test can reach this case.

The server honours the control on a modify only for a client that has the `bypass-acl` privilege, as the root DNs do, or that an ACI allows to use it (`targetcontrol="1.3.6.1.4.1.26027.1.5.2"`). For any other client the server drops the control, which is not critical, and refuses the modify as if the control had not been sent.

---

suggestion (non-blocking): The rewrite removes the only test of the repair control on a server with no replication domain.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplicationRepairControlTest.java:119, opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/MultimasterReplication.java:158-173, :177

At the base, testRepairControl ran in a JVM with no domain. Its -J 1.3.6.1.4.1.26027.1.5.2 add returned 0 only because the repair-control loop in findDomain (:158-173) runs before the if (domains.isEmpty()) return null; early return (:177). The comment at :178-182 presents that early return as a cost saving, which makes moving it above the loop a natural edit. With that change, the base class goes red with 53, but this class stays green: setUp configures a domain at :119 before any case runs. No other file under src/test refers to the control. The chapter's own use case, setting the entryUUID another service assigned before a migration, is often run before replication is enabled.

/** The repair control on a server with no replication domain configured. */
public class ReplicationRepairControlStandaloneTest extends ReplicationTestCase
{
  // The base's testRepairControl, unchanged: no configureReplication() anywhere in this class.
}

Pin: the base's testRepairControl in a class that configures no domain. Its assertEquals(LDAPModify.run(...), 0) goes red (53) when :177 is moved above :158. It costs one more server start.

---

suggestion (non-blocking): The msg == null branch of nextUpdate() does not describe what the broker does.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplicationRepairControlTest.java:383-386

broker.receive() calls receive(false, true, false) with reconnectOnFailure set. If the session drops, the broker goes into reStart(...) and keeps looping. It returns null only after stop() has set shutdown, and this class calls stop() only in tearDown. So a lost session never produces "the broker session is gone": nextUpdate() blocks in the reconnect loop past its 4 s budget, which it checks only between receive() calls. Nothing in the class drops the session, so this is a misleading message, not a flake.

      if (msg == null)
      {
        throw new AssertionError("the broker was stopped");
      }

[vharseko]

Round 3, 807791f0c5. The branch is rebased onto the current master first (clean, no conflicts); the round is the last commit. Every point checked against the code, and all four taken - the second one further than the text you proposed, because following it turned up a fix.

issue (non-blocking) - the precondition and the WARNING cover only the repaired entry. Taken, with one correction to the proposed sentence. The add road is as you describe: the parent's entryUUID is not found (LDAPReplicationDomain, handleConflictResolution(PreOperationAddOperation)), and solveNamingConflict(AddOperation, AddMsg) renames the new entry to a conflict RDN under the base DN through addConflict(), with the alert. A modify DN is different: findEntryDN(newSuperiorID) returns null, and markConflictEntry(op, currentDN, ...) only puts ds-sync-conflict on the entry where it is, with the same alert and the unresolved counter. The entry is not moved under the base DN, so on the receiving side it stays under its old parent. The WARNING says both, separately.

There is also a third road you did not list: a rename of the repaired entry itself. solveNamingConflict(ModifyDNOperation, ...) takes the currentDN == null arm (numResolvedNamingConflicts.incrementAndGet(); return NOTHING_TO_DO), so it is lost silently, like the modify and the delete. The sentence now names it. The step of the procedure reads "while nothing else changes the entry or the entries below it".

issue (non-blocking) - the control is subject to access control. Taken, and it turned out not to be a documentation point only. On a modify the road is as you describe: processRequestControls() → removeAllDisallowedControls() runs before handleConflictResolution(), so a client with no bypass-acl and no targetcontrol ACI has the control dropped. On an add and a delete, though, the order is the other way round. LocalBackendAddOperation calls handleConflictResolution before processControls(parentDN), and LocalBackendDeleteOperation calls it before handleRequestControls(). MultimasterReplication.findDomain() had taken the control off the request by the time the backend checked it. So on those two operations the ACI of the controls was never consulted.

The fix is in findDomain(): before it honours the control, mayUseRepairControl() asks AccessControlHandler.isAllowed(dn, op, control). A control the client may not use is left on the request, and the backend then treats it as any other disallowed control: it drops a non-critical one and refuses a critical one with 12. The modify and modify DN roads do not change. The paragraph in the chapter and both appendix entries now say that the server honours the control only for bypass-acl or an ACI, and what happens otherwise, delete included: a delete sent with a dropped control is carried out and replicated as any other.

"No test can reach this" held only while the test configuration's targetcontrol="*" global ACI was left in place. The new cases take it out for their length, as RelaxRulesTestCase does, and restore it in finally:

• aClientNoAciLetsUseTheControlGetsNoRepair: a user with allow(all) on *||+ under the suffix and no bypass-acl. The modify is refused with CONSTRAINT_VIOLATION, the add with entryUUID with UNWILLING_TO_PERFORM, and nothing is published.
• aDeleteByAClientNoAciLetsUseTheControlIsPublished: the same user's delete with the control reaches the broker as a DeleteMsg.
• aClientAnAciLetsUseTheControlRepairs: with a global targetcontrol="1.3.6.1.4.1.26027.1.5.2" ACI for that user, the add keeps the entryUUID it was given and is not published.

The delete case has a method of its own so that the add assertion cannot shadow it.

suggestion (non-blocking) - no test of the control without a replication domain. Taken, as ReplicationRepairControlStandaloneTest. It configures no domain and first asserts MultimasterReplication.getNumberOfDomains() is zero, so that it cannot run silently where it does not mean to. The failsafe run is a single JVM, and paranoiaCheck() fails any replication class that leaves a domain behind. The case is the chapter's own: a modify of entryUUID, refused without the control and applied with it. I chose it over the base's add because it is the migration scenario you name.

suggestion (non-blocking) - the msg == null message. Taken: "the broker was stopped", with a line saying why. receive() reconnects with reconnectOnFailure set and returns null only after stop().

Measured on this head with -Pprecommit verify -pl opendj-server-legacy -am -Dit.test=ReplicationRepairControlTest,ReplicationRepairControlStandaloneTest:

• unmutated: Tests run: 7, Failures: 0 and Tests run: 1, Failures: 0;
• with the ACI check in findDomain() disabled, and the domains.isEmpty() early return moved above the repair loop, in one run: exactly three cases are red, each on its own assertion. aClientNoAciLetsUseTheControlGetsNoRepair fails with "a client no ACI lets use the control repaired an add". aDeleteByAClientNoAciLetsUseTheControlIsPublished fails with "the delete was not published". aRepairedModifySetsTheEntryUUIDWithNoReplicationDomain fails with the NO-USER-MODIFICATION refusal of the repaired modify. The other five stay green.

Since the round now changes server code, not only docs and tests, I have asked for your review again.

[maximthomas]

PR #1052 — review feedback, round 3 (head 807791f)

Review action: Approve — mergeable as is: nothing blocks; the new add/delete gate has two non-blocking edges (proxied auth, the add's DN), and two test pins follow.

---

praise: the control-ACI gap on add and delete is closed where it opens, and the round-2 doc points are all in.

• MultimasterReplication.findDomain (:165) now asks before it honours the repair control, and leaves a refused control on the request, so removeAllDisallowedControls drops or refuses it as it does any other control.
• The mutation run in the description (ACI check disabled → aClientNoAciLetsUseTheControlGetsNoRepair, aDeleteByAClientNoAciLetsUseTheControlIsPublished and aRepairedModifySetsTheEntryUUIDWithNoReplicationDomain red, each on its own assertion) pins the gate on the add and delete roads.
• chap-replication.adoc:1998 says who may use the control, and :2051 covers the entries below the repaired one (conflict entry under the base DN, alert, unresolved-naming-conflicts).

---

issue (non-blocking): On an add and a delete, the repair right is judged for the bound client, not for the identity a proxied-auth control makes the operation run as.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/MultimasterReplication.java:165, :218-222; opendj-server-legacy/src/main/java/org/opends/server/workflowelement/localbackend/LocalBackendAddOperation.java:298, :432; opendj-server-legacy/src/main/java/org/opends/server/workflowelement/localbackend/LocalBackendDeleteOperation.java:223, :232

findDomain runs from handleConflictResolution (add :298, delete :223). The backend applies proxied authorization later, in processControls / handleRequestControls (add :432, delete :232). So when isAllowed runs, skipAccessCheck → ClientConnection.hasPrivilege(BYPASS_ACL, op) still sees the bound DN, and the ACI container is built from the bound entry. Suppose an account B holds the repair right and the proxied-auth privilege, and proxies as a user V whom no ACI lets use the control. B's add or delete is repaired while it runs as V. The same request as a modify has the control dropped, because there the check comes after proxy evaluation. It also works the other way: an account without the right, proxying as an entitled V, gets no repair. Nothing is gained beyond B's own rights, but the javadoc at :214, "The backend asks the same question of every control", does not hold here, and no case sends proxied auth. The smallest fix is to refuse the repair in that case and add one sentence to the docs:

// import org.opends.server.types.OperationType;
private static boolean mayUseRepairControl(DN dn, Operation op, Control control)
{
  final OperationType type = op.getOperationType();
  if ((type == OperationType.ADD || type == OperationType.DELETE) && carriesProxiedAuth(op))
  {
    // On an add and a delete the backend applies the proxied authorization after this plugin:
    // the question below would be answered for the bound client, not for the one the operation runs as.
    return false;
  }
  try
  {
    return AccessControlConfigManager.getInstance().getAccessControlHandler().isAllowed(dn, op, control);
  }
  catch (DirectoryException e)
  {
    logger.traceException(e);
    return false;
  }
}

private static boolean carriesProxiedAuth(Operation op)
{
  for (Control c : op.getRequestControls())
  {
    if (OID_PROXIED_AUTH_V1.equals(c.getOID()) || OID_PROXIED_AUTH_V2.equals(c.getOID()))
    {
      return true;
    }
  }
  return false;
}

Pin: send the add arm of aClientNoAciLetsUseTheControlGetsNoRepair on the Directory Manager connection, with ProxiedAuthV2RequestControl.newControl("dn:" + USER_DN) next to the repair control, and expect UNWILLING_TO_PERFORM with nothing published. It is red at this head.
Or: the complete fix is to evaluate proxied auth and strip disallowed controls before handleConflictResolution on the add and delete roads, as the modify road does. That reorders the backend, and is more than this PR needs.

---

question (non-blocking): Is it deliberate that an add is judged at the new entry's DN, while the backend judges every other add control at the parent DN?

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/MultimasterReplication.java:464, :214-222; opendj-server-legacy/src/main/java/org/opends/server/workflowelement/localbackend/LocalBackendAddOperation.java:273, :943

On an add, findDomain(addOperation.getEntryDN(), …) passes the entry DN to isAllowed, while processControls(parentDN) → removeAllDisallowedControls(parentDN, this) judges at the parent. Delete, modify and modify DN use the entry DN on both sides. With untargeted control ACIs, the only kind the defaults and the tests use, both DNs give the same answer. With a scoped one they differ. targetscope="onelevel" on ou=C grants the repair on an add of cn=x,ou=C, which the backend's rule for add controls would refuse. targetscope="base" on ou=C refuses it where that rule would allow it: the non-critical control then does nothing, and the add fails on entryUUID. If the entry DN is deliberate, it matches what a later modify or delete of the entry is judged at: correct the javadoc at :214-216, and say in the docs that the ACI is evaluated at the entry being repaired. If it is not, pass the parent DN for an add. Minor either way.

---

suggestion (non-blocking): No test covers the critical arm of the new refusal: every repair control the unentitled client sends is non-critical.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplicationRepairControlTest.java:383, :401, :436, :462

The deny branch (MultimasterReplication.java:165-174) leaves the control on the request. On a delete, removeAllDisallowedControls then throws UNAVAILABLE_CRITICAL_EXTENSION for a critical control. USER_DN sends only non-critical controls, and the one critical send (:333) is Directory Manager on a modify, which the backend refuses before the plugin runs. So the mutant it.remove(); break; in the deny branch survives every case: under it, a critical control from a client that may not use it is dropped, and the delete is carried out and published. This is not the critical-add pin declined in round 2: that client is refused on every road, so the pin fixes no add/modify asymmetry.

@Test
public void aCriticalRepairControlAClientMayNotUseIsRefusedOnADelete() throws Exception
{
  final String dn = "cn=kept by a critical control," + TEST_ROOT_DN_STRING;
  TestCaseUtils.addEntry("dn: " + dn, "objectClass: top", "objectClass: person",
      "sn: kept", "cn: kept by a critical control");
  assertThat(nextUpdate()).as("the add of the entry was not published").isNotNull();

  final Runnable restoreTheGlobalControlAci = withGlobalControlAci(null);
  try (Connection user = connectAsUser())
  {
    user.delete(Requests.newDeleteRequest(dn)
        .addControl(GenericControl.newControl(OID_REPLICATION_REPAIR_CONTROL, true)));
    throw new AssertionError("a critical control the client may not use was dropped from a delete");
  }
  catch (LdapException e)
  {
    assertThat(e.getResult().getResultCode()).isEqualTo(ResultCode.UNAVAILABLE_CRITICAL_EXTENSION);
  }
  finally
  {
    restoreTheGlobalControlAci.run();
  }
  assertThat(nextUpdate()).as("a refused delete was published").isNull();
}

Pin: the delete succeeds under it.remove(); break;, so the AssertionError turns the case red.

---

suggestion (non-blocking): No test covers the DN the ACI check is made at: every control ACI the cases use is global and untargeted.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/ReplicationRepairControlTest.java:93-97

REPAIR_CONTROL_ACI and ANY_CONTROL_ACI have no target, so isAllowed answers the same whatever DN mayUseRepairControl passes it. A mutant passing the domain's base DN or dn.parent() keeps every case green. The pin below uses a delete, where the plugin and the backend both judge at the entry DN. The add is left out until the question above is answered.

@Test
public void theRepairControlAciIsJudgedAtTheDeletedEntry() throws Exception
{
  final String repairable = "cn=repairable," + TEST_ROOT_DN_STRING;
  final String other = "cn=not repairable," + TEST_ROOT_DN_STRING;
  TestCaseUtils.addEntry("dn: " + repairable, "objectClass: top", "objectClass: person",
      "sn: repairable", "cn: repairable");
  assertThat(nextUpdate()).as("the add of the entry was not published").isNotNull();
  TestCaseUtils.addEntry("dn: " + other, "objectClass: top", "objectClass: person",
      "sn: not repairable", "cn: not repairable");
  assertThat(nextUpdate()).as("the add of the entry was not published").isNotNull();

  final Runnable restoreTheGlobalControlAci = withGlobalControlAci(
      "(target=\"ldap:///" + repairable + "\")(targetcontrol=\"" + OID_REPLICATION_REPAIR_CONTROL + "\")"
      + "(version 3.0; acl \"Repair control on one entry\"; allow(read) userdn=\"ldap:///" + USER_DN + "\";)");
  try (Connection user = connectAsUser())
  {
    user.delete(Requests.newDeleteRequest(other)
        .addControl(GenericControl.newControl(OID_REPLICATION_REPAIR_CONTROL)));
    assertThat(nextUpdate()).as("a delete outside the ACI's target was repaired").isInstanceOf(DeleteMsg.class);
    user.delete(Requests.newDeleteRequest(repairable)
        .addControl(GenericControl.newControl(OID_REPLICATION_REPAIR_CONTROL)));
    assertThat(nextUpdate()).as("a delete inside the ACI's target was published").isNull();
  }
  finally
  {
    restoreTheGlobalControlAci.run();
  }
}

Pin: the base-DN, dn.parent() and root-DN mutants all refuse the second delete, which is then published, so the last assertion is red.

[vharseko]

Round 4, f4b154560d. The branch is rebased onto the current master first (clean, no conflicts); the round is the last commit. All four points checked against the code and taken.

issue (non-blocking) - proxied auth on an add and a delete. Taken, as you proposed. The order is as you describe: handleConflictResolution runs before evaluateProxyAuthControls on both roads (add processControls, delete handleRequestControls), so hasPrivilege(BYPASS_ACL, op) and the ACI container still see the bound client. On a modify and a modify DN the proxy is applied first, so the check there already answers for the identity the request runs as, and the refusal is limited to ADD and DELETE. mayUseRepairControl returns false when such a request carries a proxied authorization control, v1 or v2; the control stays on the request, and the backend then judges it for the proxied identity as any other control. The javadoc no longer says the backend asks the same question of every control, and the chapter and both appendix entries say that the control is not honoured on a proxied add or delete.

One change to the pin: the proxy is not Directory Manager. The default root privileges do not include proxied-auth, so a proxied add from Directory Manager fails with AUTHORIZATION_DENIED at this head and with the fix alike, and pins nothing. aProxiedAddOrDeleteIsNotRepaired binds as a user with bypass-acl and proxied-auth, proxies as the test user with the global control ACI removed, and checks both roads: the add with entryUUID is refused with UNWILLING_TO_PERFORM and not published, and the delete is published as a DeleteMsg.

question (non-blocking) - the entry DN on an add. Deliberate: the right is to repair this entry, and it is judged at the same DN on all four operations - the one a later modify or delete of the entry is judged at. Where the two DNs disagree, the outcome is still decided here: if the entry DN refuses and the parent allows, the backend keeps a control it does not act on, and the add fails on entryUUID as it would without it. The javadoc now says the question is asked at the entry being repaired, on an add as well, and so does the chapter.

suggestion (non-blocking) - the critical arm of the refusal. Taken, as aCriticalRepairControlAClientMayNotUseIsRefusedOnADelete, with one change: the case reads the broker before it asserts the result code. Under the it.remove(); break; mutant the delete is carried out and published, and a case which throws first leaves that DeleteMsg for the next case to read - in the first mutant run it turned testRepairControl red as well.

suggestion (non-blocking) - the DN the ACI check is made at. Taken, as theRepairControlAciIsJudgedAtTheEntryBeingRepaired, with the add arm too now that the question is answered: a global ACI targeted at cn=repairable*,o=test lets the user repair the delete and the add of those entries, while the delete of another entry is published and the add of another entry with entryUUID is refused with UNWILLING_TO_PERFORM.

Measured on this head with -Pprecommit verify -pl opendj-server-legacy -am:

• unmutated: ReplicationRepairControlTest Tests run: 10, Failures: 0, ReplicationRepairControlStandaloneTest Tests run: 1, Failures: 0; the whole org/opends/server/replication/plugin/** package in one run, Tests run: 332, Failures: 0.
• the proxied refusal disabled, it.remove() before the break of the refusal, and the ACI asked at dn.parent(), in one run: exactly the three new cases are red, each on its own assertion ("a proxied add was repaired", "a critical control the client may not use was dropped, and the delete published", "a delete inside the ACI's target was published").
• the ACI asked at the parent on an add only: red on the add arm, "an add inside the ACI's target was not repaired". The proxied refusal kept for an add only: red on the delete arm, "a proxied delete was repaired".

Since the round changes server code again, I have asked for your review again.