Conversation
9c99def to
21ec570
Compare
|
Round 3 pushed as 21ec570: the change rebased onto #997, and a defect of its own fixed on top. Stacked on #997. #997 (approved) restructures The defect (fixed in Now each Pins, in
Both cases hold an entry in the backend: an index of an empty backend is trusted when it is opened, whatever its tree holds, so on an empty one the reopen trusts the index again — which is right, and which the cases are not about. Runs: |
maximthomas
left a comment
There was a problem hiding this comment.
praise: The change is decided where it can be re-answered, and the reasons are written next to the code.
- Asking each index what it was opened under (
DefaultIndex.encrypted,planIndexUpdatesatAttributeIndex.java:1139) instead of the configuration the attribute index holds — a replayed or given-up write is answered again by the index, andsetConfidentialleavingIndexcloses the road where it could not converge. - The suite switched outside the writes and compared with the suite, not the configuration (
EntryContainer.setIndexConfidentiality): idempotent, and in force before the added indexes bind their codecs. - The tree given up rather than left to the rebuild, with the exact reason: a V3 record read by the V2 codec decodes to a defined empty set (
EntryIDSet.java:490-494,:530-532— confirmed while reviewing), a search answered with nothing. - Two writes for delete and reopen, and the ordering argument at
AttributeIndex.java:1025-1029for which of them untrusts. - One test body per behaviour with a subclass per engine, and the two
ReplayedConfigChangeTestcases countingwrites()andattempts()rather than trusting the result code.
issue (blocking): On PDB a give-up of the reopen write leaves a live index whose tree is gone, and the same change asked for again is a silent no-op.
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java:1038, :1139
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/DefaultIndex.java:111-115
Write 3 commits: TRUSTED removed, the presence tree deleted. Write 4 opens it again, and afterOpen assigns encrypted = cryptoSuite.isEncrypted() (now true) and the V3 codec in memory before the commit. When that commit gives up (PDBStorage.java:696-728), Persistit rolls the tree creation back (measured in #998) while the binding stays: encrypted=true, trusted=false, no tree. The re-apply then finds isEncrypted() == newConfig.isConfidentialityEnabled() at :1139, gives nothing up, and answers SUCCESS with no message — the premise at :938-942 ("the index is as it was") does not hold on this road. Until the rebuild or a restart every cn add/modify and every (cn=*) search throws TreeNotFoundException through DefaultIndex.get (:286; the presence query has no trusted gate, IndexQueryFactoryImpl.java:358-365). JE recreates the tree empty and is not affected. Not run: no case arms write 4.
// DefaultIndex
private volatile EntryIDSetCodec plainCodec; // V1 or V2, from the COMPACTED flag
@Override
final void afterOpen(WriteableTransaction txn, boolean createOnDemand)
{
final EnumSet<IndexFlag> flags = state.getIndexFlags(txn, getName());
plainCodec = flags.contains(COMPACTED) ? CODEC_V2 : CODEC_V1;
bindCodec(cryptoSuite.isEncrypted());
trusted = flags.contains(TRUSTED);
...
}
/**
* Binds the codec to the setting given. What afterOpen binds is memory, and outlives a write the
* storage rolled back — which on PDB takes the tree opened in it with it.
*/
final void bindCodec(boolean encrypted)
{
this.encrypted = encrypted;
codec = encrypted ? new EntryIDSet.EntryIDSetCodecV3(plainCodec, cryptoSuite) : plainCodec;
}// AttributeIndex, around the reopen write (:1029-1039)
try
{
entryContainer.getRootContainer().getStorage().write(reopen);
}
catch (Exception e)
{
// The binding the reopen made is memory: put back the setting the index answered before, so that
// the change asked for again gives the tree up (absent on PDB, empty on JE) and opens it again.
for (MatchingRuleIndex givenUp : treesToGiveUp)
{
givenUp.bindCodec(!newConfiguration.isConfidentialityEnabled());
}
throw e;
}Pin, in ReplayedConfigChangeTest, next to the write-3 give-up case — red at the head on PDB at the addEntry, and red in again on both engines (no rebuild message):
backend.storage.failWithoutReplayOnWrite(4);
final ConfigChangeResult ccr = index.applyConfigurationChange(confidentialPresenceIndexCfg());
assertThat(ccr.getResultCode()).isEqualTo(serverErrorResultCode());
assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())).as("write 3 committed").doesNotContain(TRUSTED);
assertThat(cnIndex.isEncrypted()).as("the setting the tree is still to be opened under").isFalse();
final ConfigChangeResult again = index.applyConfigurationChange(confidentialPresenceIndexCfg());
assertThat(again.getResultCode()).isEqualTo(ResultCode.SUCCESS);
assertThat(ordinalsOf(again)).containsOnly(NOTE_CONFIG_INDEX_CONFIDENTIALITY_REQUIRES_REBUILD.ordinal());
assertThat(cnIndex.isEncrypted()).isTrue();
addEntry(backend, "user.1"); // TreeNotFoundException at the head on PDBsuggestion (non-blocking): The delete of the given-up tree is pinned on the enable side only; a delete-only-when-enabling mutant is green.
opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexConfidentialityChangeTestCase.java:215-236
disablingConfidentialityStopsEncryptingWhatTheIndexWritesNext reads only user.1 after the change. Under if (newConfiguration.isConfidentialityEnabled()) givenUp.delete(txn); in writeUntrust the kept V3 record [0x00, …] is read by the rebound V2 codec as size 0 — a defined empty set — so user.1 is written clear as {id1} and both asserts at :230-231 pass. That is the silent decode the comment at AttributeIndex.java:1085-1090 motivates the delete with, in the one direction it is not pinned (on the enable side the V3 codec falls through to its delegate, so a kept tree reads correctly with or without the delete).
// after attributeIndex.applyConfigurationChange(indexCfg(false, ENTRY_LIMIT)), before trust():
assertThat(recordCount(backend, presenceIndex(attributeIndex))).as("the encrypted tree, given up").isEqualTo(0);Red under the mutant (recordCount == 1), as :144 already is on the enable side.
suggestion (non-blocking): Both new ReplayedConfigChangeTest cases arm write 3; which of the two writes untrusts is not pinned.
opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java:888, :940
The replay case pins writes() == before + 4 (the count) and the give-up case pins TRUSTED present after a write-3 give-up — true both at the head (removal rolled back) and with setTrusted(txn, false) moved into the reopen write (never removed). The ordering AttributeIndex.java:1025-1029 argues for has no case. The write-4 case above pins it: doesNotContain(TRUSTED) after a write-4 give-up is red under that move. While there, the write-3 give-up case could assert treesOf(ec) still holds cnIndex.getName() — whether PDB rolls a deleteTree back is measured for creation only (#998); if it does not, the state between that give-up and the re-apply is a trusted index with no tree.
21ec570 to
7a841f9
Compare
|
Rebased onto #997's round 4 ( Verified after the rebase: Head now |
|
Round 4 pushed as The blocking issue. Confirmed exactly as described: Fixed by snapshotting what each index in Putting it back exposed a second failure on the same road, not in the report: the write which untrusts and deletes (the third) runs again on that re-apply — Pinned in The delete-only-when-enabling mutant. Confirmed, and fixed the way suggested: Which write untrusts. The new case above pins it as a side effect: Runs: the three |
…ex to the running backend setConfidential() only compared the setting with the one the index was opened with, so a change of confidentiality-enabled reached neither the CryptoSuite the indexes of the attribute share nor the codecs bound to it, and the comparison never converged: every later change of that index untrusted it again and asked for a rebuild which could not help. The change now puts the new setting in force on that suite before the indexes it adds bind their codecs, so the key hashed index enabling confidentiality creates encrypts its records as well, and gives up the trees of the indexes it keeps: one write untrusts and deletes them, a second opens them again, which is what binds their codecs to the setting now in force. Their records are in the encoding being given up, which the codec of the new one reads back as an empty set of entry IDs rather than failing, so leaving them would answer searches with no entries at all until the rebuild had run. Fixes OpenIdentityPlatform#992
…her its confidentiality changes Whether a change of confidentiality-enabled had anything to apply was decided by comparing the new configuration with the one this attribute index holds, and that one is replaced after the first of the writes. A write the storage gave up on - the bound of its retry loop spent, or a failure it does not replay - left the configuration reading as applied while the index was as it had been: its codec bound to the setting it was opened under, its tree in that encoding, TRUSTED still stored. Asked for again, the change found nothing changed, answered SUCCESS with no instruction, and the next open of the container read that tree with the codec of the other setting, which is the very state the change gives the tree up to avoid. Each kept index now holds the confidentiality its codec was bound to, and the plan asks the index: an index whose codec was bound under the other setting has its tree given up and is opened again, whichever configuration this attribute index holds. The suite the indexes share is put in force by the same comparison against what it carries, since that is idempotent. The instruction goes before the first write with the entry limit's, on the road OpenIdentityPlatform#991 closed for the limit, so a give-up leaves it in the result next to the failure. Pinned on the presence index, whose tree a change of confidentiality keeps: a conflict on the write which gives the tree up is replayed and reported once, and the index writes encrypted after it; a give-up on that write is reported, leaves TRUSTED stored, and is still a change when asked for again - red with the decision taken from the configuration, and red at "reported" with the report moved back behind the writes.
…he index answered before What DefaultIndex.afterOpen binds - the codec, and the confidentiality it answers under - is memory, and outlives a write the storage gave up on: the reopen AttributeIndex.applyConfigurationChange asks for runs that binding before the write which carries it commits, and nothing put it back when that commit did not. A change asked for again then found the index already agreeing with the new setting, answered SUCCESS with no instruction, and never reopened the tree the write before it had deleted - the index left degraded, untrusted, with no tree, until a rebuild or a restart nobody was told to run. Each index whose tree is about to be given up now has what it answered before snapshotted, and put back if the write which reopens it does not commit, so a re-apply still finds it to give up and open again rather than one which already agrees with a setting that write never durably reached. Putting it back exposed a second failure on the same road: the write which untrusts and deletes runs again on that re-apply, and its delete found nothing to delete a second time, since the first attempt's had already committed. Guarded by whether the tree is still there. Pinned in ReplayedConfigChangeTest on the write which reopens the tree (the fourth, once the third has committed) - the road neither of the two existing give-up cases exercises, both arming the third. And in IndexConfidentialityChangeTestCase, disablingConfidentialityStopsEncrypting- WhatTheIndexWritesNext now checks the tree given up is empty before the index is trusted again, not only what it writes next - a delete-only-on- enable mutant read back the disabled record as empty regardless.
3dd6ca3 to
76d5517
Compare
|
Rebased onto master now that #997 is in (squash-merged as Mechanical. The four Head now |
maximthomas
left a comment
There was a problem hiding this comment.
praise: The give-up of the reopen is now put back, and the road round 1 raised is closed.
IndexBindingis captured afterwriteUntrust(AttributeIndex.java:1028-1032), so a re-apply after a given-up write 4 findsisEncrypted()still differing and gives the tree up again — no silent no-op.aConfidentialityChangeIsReportedWhenTheWriteWhichReopensTheTreeGivesUp(ReplayedConfigChangeTest.java:977-1017) pins both halves asked for: write 3 untrusts and commits, write 4 is armed and given up,againreports the rebuild.- The disable road now asserts the presence tree is empty (
IndexConfidentialityChangeTestCase.java:225-229). - The rebase onto
b0970cc1a8is byte-for-byte mechanical; the stack is gone.
issue (blocking): revertFailedReopen puts back encrypted and codec, but not the trusted which afterOpen bound in the same write.
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/DefaultIndex.java:361-365, :117-122, :322-324; opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java:1140-1156
afterOpen binds three fields. On an empty container (createOnDemand && !trusted && entryContainer.isEmpty(txn)) it calls setTrusted(txn, true), which assigns the in-memory trusted before the state write. When the reopen's commit gives up, the flag and (on PDB) the tree are rolled back, revert() restores encrypted/codec, and memory stays trusted=true over no tree — the "trusted one holding nothing" the comment at AttributeIndex.java:1036-1041 says the ordering avoids. Empty backend + confidentiality toggled is the ordinary road (the setting goes on before the import). In the window reads and writes on PDB behave as untrusted (StorageRuntimeException → undefined), so alone it self-heals on re-apply or restart; but any add/modify/delete which fails in the window with this index in its IndexBuffer runs writeTrustState → index.setTrusted(txn, index.isTrusted()) (IndexBuffer.java:216) and commits TRUSTED for a tree which does not exist. Entries added meanwhile were skipped by update() (get() → undefined → return at :207); after a restart the tree is created empty and trusted, and an equality/presence search answers "nothing" for them, definitively.
// DefaultIndex
final void revertFailedReopen(boolean encrypted, EntryIDSetCodec codec, boolean trusted)
{
this.encrypted = encrypted;
this.codec = codec;
this.trusted = trusted;
}
// AttributeIndex.IndexBinding — captured after writeUntrust, so this is false
private final boolean trusted;
IndexBinding(MatchingRuleIndex index)
{
this.index = index;
this.encrypted = index.isEncrypted();
this.codec = index.codec();
this.trusted = index.isTrusted();
}
void revert()
{
index.revertFailedReopen(encrypted, codec, trusted);
}Pin: the write-4 case once more without addBaseEntry — red today, isTrusted() answers true:
backend.storage.failWithoutReplayOnWrite(4);
final ConfigChangeResult ccr = index.applyConfigurationChange(confidentialPresenceIndexCfg());
assertThat(ccr.getResultCode()).isEqualTo(serverErrorResultCode());
assertThat(cnIndex.isTrusted())
.as("the reopen which trusted the index over an empty backend gave up").isFalse();
assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())).doesNotContain(TRUSTED);suggestion (non-blocking): The codec half of revertFailedReopen has no pin: a mutant dropping this.codec = codec survives the suite.
opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java:1002
Between the give-up and the re-apply the index is untrusted and its tree is gone, so nothing decodes through the stale codec, and afterOpen rebinds it on the re-apply — no behavioural road turns red. The only pin is white-box, one line on the invariant the javadoc at DefaultIndex.java:353-360 states:
final EntryIDSetCodec before = cnIndex.codec();
// ... failWithoutReplayOnWrite(4), applyConfigurationChange ...
assertThat(cnIndex.codec()).as("the codec the given-up reopen bound is put back").isSameAs(before);Or leave it: with trusted added to the revert, the pin above covers the one half of it with an observable.
… with the codec DefaultIndex.afterOpen binds three fields, not two: over an empty entry container it trusts the index it opens, in memory before the flag is written, the same way it binds the codec. A give-up of the reopen put the codec and the confidentiality back, but not the trust - an index answering trusted over no tree. That answer is what an operation which then fails writes down for every index in its buffer (IndexBuffer.writeTrustState): TRUSTED stored for a tree the give-up took with it, while the index skipped the entries added meanwhile, which a search after a restart then never finds. IndexBinding holds the trust too, taken after the write which untrusts so it is the one that write committed, and revertFailedReopen puts the three back together. Pinned in ReplayedConfigChangeTest over an empty backend: the give-up leaves the index untrusted, an add the container refuses in the window writes no TRUSTED down for it, and the reopen asked for again trusts the index it opens. Red at the head on both the answer and the flag the failed add stored. The codec half of the revert gets its one-line pin alongside, in the write-4 case - nothing decodes through the codec before the re-apply binds it afresh, so the invariant is what there is to pin.
|
Round 5 pushed as The blocking issue. Confirmed as described. Pinned in The codec pin. Added as the one line suggested, in the write-4 case: Runs: |
Fixes #992
On master after #997 (
b0970cc1a8), which restructured the same method and which this change was stacked on until it merged. Four commits: this change in #997's shape; the fix of a defect of its own found while reviewing #997 (below); the give-up of the write which reopens the tree put back to what the index answered before it (below); and the trust that reopen binds put back with it (below).DefaultIndex.setConfidentialonly compared the setting with the one the index was opened with. Nothing moved theCryptoSuitethe indexes of the attribute share to the new value, and nothing bound their codecs again, soconfidentiality-enabledreached the running backend in neither direction, and the comparison never converged: every later change of that index -index-entry-limitincluded - untrusted it again and asked for a rebuild which could not help.The declared admin action of the property is that the index has to be rebuilt (
BackendIndexConfiguration.xml:230), not that the component has to be restarted, so this makes the implementation answer for what is documented.What the change does
AttributeIndex.applyConfigurationChangeasks each index it keeps whether its codec was bound under the setting now asked for -DefaultIndexholds the confidentiality it was opened under,setConfidentialleaves theIndexinterface - and for each index whose answer is no:CryptoSuite, throughEntryContainer.setIndexConfidentiality, which keeps the cipher of the backend - whenever the suite does not carry the setting asked for. Done before the write which opens the indexes this change adds, so the key hashed index that enabling confidentiality of an equality index creates binds its codec to the new setting and encrypts its records, rather than only hashing its keys. Applied outside the writes, which the storage may replay: it changes a live object and is idempotent;writeUntrust().Asked of the index rather than decided from the configuration this attribute index holds, because that configuration is replaced after the first write: after a write the storage gave up on it already read as applied while the index was as it had been - codec bound to the old setting, tree in that encoding, TRUSTED still stored - and the same change asked for again found nothing to do, so the next open of the container read that tree with the codec of the other setting. Asking the index makes the re-apply a change again.
The same gap opens on the second write, the one which reopens the tree: what it binds the index to answering is memory too, and outlives that write's own rollback. A give-up of it is put back to what the index answered before the write ran, so the change asked for again still finds it to give up rather than one already agreeing with a setting that write never durably reached; the first write, asked for again, finds the tree its own earlier give-up already deleted, and does not try to delete it a second time. What the reopen binds includes the trust: an index opened over an empty entry container is trusted on the spot, in memory before the flag is written, and a trust left behind by a give-up is what an operation which then fails writes down for every index in its buffer - TRUSTED stored for a tree the give-up took with it, while the index skipped the entries added meanwhile. It is put back with the codec.
The trees are given up rather than left to the rebuild because their records are in the encoding of the setting being given up, and the codec of the new one does not read them back as what they are: an encrypted record read as clear text decodes to an empty set of entry IDs rather than failing, which is a search answered with no entries at all. An empty untrusted index answers "undefined" instead, and so is not used until the rebuild has run. They are deleted rather than emptied record by record, which for an index of any size would be a transaction of its own making, and in two writes rather than one because the storage engines delete and create the tree of an index as operations of their own - as removing and adding an index does. That order also decides how an interrupted change ends: the write which deletes is the one which untrusts, so what is left is an index the next open of the container creates empty and keeps degraded, never a trusted one holding nothing.
DefaultIndex.codecbecomesvolatile, since it is now bound again on a live instance other threads are holding, andDefaultIndex.encryptedrecords what it was bound to;isEncrypted()answers from it, which is also the per-index answerbackendstat show-index-statusprints. Both - and the trust, whichafterOpenbinds the same way - are put back to what they were before the reopen if that write's commit does not.Tests
IndexConfidentialityChangeTestCase, with a subclass per storage engine (PDBIndexConfidentialityChangeTest,JEIndexConfidentialityChangeTest) because deleting and creating the tree of an index is the engine's own operation. Five behaviours each, all failing before the change:The disabling case also checks the tree given up is empty before the index is trusted again, not only what it writes next: a mutant which deletes the tree given up only where confidentiality is being enabled read the disabled record's encrypted bytes back as an empty set with either codec, and passed.
And four cases in
ReplayedConfigChangeTest, on a presence index, whose tree a confidentiality change keeps:aConfidentialityChangeUntrustsTheIndexWhenTheTransactionIsReplayed: a conflict on the write which gives the tree up is replayed, reported once, and the index writes encrypted after the reopen; a re-apply asks nothing and opens no third writeaConfidentialityChangeIsReportedWhenTheWriteWhichGivesUpTheTreeGivesUp: a give-up on that write is reported with the failure, leaves TRUSTED stored, and the same change asked for again untrusts, reports once and encryptsaConfidentialityChangeIsReportedWhenTheWriteWhichReopensTheTreeGivesUp: a give-up on the write which reopens the tree - the fourth, once the third has committed - leaves the index still to give up and open again when the same change is asked for, not one already agreeing with the setting that write never durably reached; and the codec that reopen bound is put backaGivenUpReopenOverAnEmptyBackendDoesNotLeaveTheIndexTrusted: the same give-up over an empty backend, where the reopen trusts the index it opens, leaves it untrusted; an add which fails in the window writes no TRUSTED down for it; and the re-apply trusts the index it opens againisTrusted()true over no tree, and the failed add stored TRUSTED for itRun green: the ten tests of
IndexConfidentialityChangeTestCaseand the four ofReplayedConfigChangeTesttogether withEncryptedPDBTestCase,EncryptedJETestCase,PDBTestCase,JETestCase,DefaultIndexTest,EntryIDSetTest,StateTest,OnDiskMergeImporterTest,VLVControlTestCaseandServerSideSortControlTestCase- 270 tests, no failures.Not in this change
An online
rebuild-indexof an index whose confidentiality was never changed is unaffected. The offline rebuild already applied the setting correctly, since it opens a container of its own; it still does.