Repository: bookkeeper Updated Branches: refs/heads/master e44c73883 -> 0f16da896
BOOKKEEPER-1055: Optimize handling of masterKey in case it is empty On each request client and bookies are exchanging the ledger masterKey, which is a 20 bytes MAC digest of the ledger password. For each request there is a considerable overhead in allocating byte arrays when parsing the add/read requests. If the client is a passing an empty password, we should optimize the data path to skip all allocations (related to the masterKey) and instead rely on a static byte array. Author: Matteo Merli <[email protected]> Author: Matteo Merli <[email protected]> Reviewers: Enrico Olivelli <[email protected]>, Venkateswararao Jujjuri (JV) <None> Closes #156 from merlimat/empty-password Project: http://git-wip-us.apache.org/repos/asf/bookkeeper/repo Commit: http://git-wip-us.apache.org/repos/asf/bookkeeper/commit/0f16da89 Tree: http://git-wip-us.apache.org/repos/asf/bookkeeper/tree/0f16da89 Diff: http://git-wip-us.apache.org/repos/asf/bookkeeper/diff/0f16da89 Branch: refs/heads/master Commit: 0f16da8961521372b9bec953ab62c8913bb8dc31 Parents: e44c738 Author: Matteo Merli <[email protected]> Authored: Wed May 17 08:57:37 2017 +0200 Committer: eolivelli <[email protected]> Committed: Wed May 17 08:57:37 2017 +0200 ---------------------------------------------------------------------- .../bookkeeper/bookie/LedgerDescriptorImpl.java | 3 +- .../apache/bookkeeper/client/LedgerHandle.java | 16 ++++++- .../bookkeeper/client/MacDigestManager.java | 23 +++++---- .../bookkeeper/proto/BookieProtoEncoding.java | 50 ++++++++++++++++---- 4 files changed, 72 insertions(+), 20 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/bookkeeper/blob/0f16da89/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptorImpl.java ---------------------------------------------------------------------- diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptorImpl.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptorImpl.java index 6602392..f80bbc8 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptorImpl.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptorImpl.java @@ -22,7 +22,6 @@ package org.apache.bookkeeper.bookie; import io.netty.buffer.ByteBuf; -import io.netty.buffer.ByteBufUtil; import java.io.IOException; import java.util.Arrays; @@ -51,6 +50,8 @@ public class LedgerDescriptorImpl extends LedgerDescriptor { @Override void checkAccess(byte masterKey[]) throws BookieException, IOException { if (!Arrays.equals(this.masterKey, masterKey)) { + LOG.error("[{}] Requested master key {} does not match the cached master key {}", new Object[] { + this.ledgerId, Arrays.toString(masterKey), Arrays.toString(this.masterKey) }); throw BookieException.create(BookieException.Code.UnauthorizedAccessException); } } http://git-wip-us.apache.org/repos/asf/bookkeeper/blob/0f16da89/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java ---------------------------------------------------------------------- diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java index 4ae4f48..06f1d8c 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java @@ -24,6 +24,7 @@ import static com.google.common.base.Charsets.UTF_8; import io.netty.buffer.ByteBuf; import java.security.GeneralSecurityException; +import java.security.NoSuchAlgorithmException; import java.util.ArrayList; import java.util.Arrays; import java.util.Enumeration; @@ -92,6 +93,16 @@ public class LedgerHandle implements AutoCloseable { final Counter lacUpdateHitsCounter; final Counter lacUpdateMissesCounter; + // This empty master key is used when an empty password is provided which is the hash of an empty string + private final static byte[] emptyLedgerKey; + static { + try { + emptyLedgerKey = MacDigestManager.genDigest("ledger", new byte[0]); + } catch (NoSuchAlgorithmException e) { + throw new RuntimeException(e); + } + } + LedgerHandle(BookKeeper bk, long ledgerId, LedgerMetadata metadata, DigestType digestType, byte[] password) throws GeneralSecurityException, NumberFormatException { @@ -117,7 +128,10 @@ public class LedgerHandle implements AutoCloseable { } macManager = DigestManager.instantiate(ledgerId, password, digestType); - this.ledgerKey = MacDigestManager.genDigest("ledger", password); + + // If the password is empty, pass the same random ledger key which is generated by the hash of the empty + // password, so that the bookie can avoid processing the keys for each entry + this.ledgerKey = password.length > 0 ? MacDigestManager.genDigest("ledger", password) : emptyLedgerKey; distributionSchedule = new RoundRobinDistributionSchedule( metadata.getWriteQuorumSize(), metadata.getAckQuorumSize(), metadata.getEnsembleSize()); http://git-wip-us.apache.org/repos/asf/bookkeeper/blob/0f16da89/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/MacDigestManager.java ---------------------------------------------------------------------- diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/MacDigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/MacDigestManager.java index 920143f..fc526aa 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/MacDigestManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/MacDigestManager.java @@ -1,5 +1,3 @@ -package org.apache.bookkeeper.client; - /* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file @@ -18,11 +16,16 @@ package org.apache.bookkeeper.client; * limitations under the License. */ +package org.apache.bookkeeper.client; + +import static com.google.common.base.Charsets.UTF_8; + import io.netty.buffer.ByteBuf; import java.security.GeneralSecurityException; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; +import java.util.Arrays; import javax.crypto.Mac; import javax.crypto.spec.SecretKeySpec; @@ -30,13 +33,13 @@ import javax.crypto.spec.SecretKeySpec; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static com.google.common.base.Charsets.UTF_8; - -class MacDigestManager extends DigestManager { +public class MacDigestManager extends DigestManager { private final static Logger LOG = LoggerFactory.getLogger(MacDigestManager.class); - public static String DIGEST_ALGORITHM = "SHA-1"; - public static String KEY_ALGORITHM = "HmacSHA1"; + public static final String DIGEST_ALGORITHM = "SHA-1"; + public static final String KEY_ALGORITHM = "HmacSHA1"; + + public static final int MAC_CODE_LENGTH = 20; final byte[] passwd; @@ -58,10 +61,10 @@ class MacDigestManager extends DigestManager { public MacDigestManager(long ledgerId, byte[] passwd) throws GeneralSecurityException { super(ledgerId); - this.passwd = passwd; + this.passwd = Arrays.copyOf(passwd, passwd.length); } - static byte[] genDigest(String pad, byte[] passwd) throws NoSuchAlgorithmException { + public static byte[] genDigest(String pad, byte[] passwd) throws NoSuchAlgorithmException { MessageDigest digest = MessageDigest.getInstance(DIGEST_ALGORITHM); digest.update(pad.getBytes(UTF_8)); digest.update(passwd); @@ -70,7 +73,7 @@ class MacDigestManager extends DigestManager { @Override int getMacCodeLength() { - return 20; + return MAC_CODE_LENGTH; } http://git-wip-us.apache.org/repos/asf/bookkeeper/blob/0f16da89/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java ---------------------------------------------------------------------- diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java index 0fed3e5..cd71b26 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java @@ -21,8 +21,10 @@ package org.apache.bookkeeper.proto; import java.io.IOException; +import java.security.NoSuchAlgorithmException; import java.util.List; +import org.apache.bookkeeper.client.MacDigestManager; import org.apache.bookkeeper.proto.BookieProtocol.PacketHeader; import org.apache.bookkeeper.util.DoubleByteBuf; import org.slf4j.Logger; @@ -37,7 +39,6 @@ import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; import io.netty.buffer.ByteBufInputStream; import io.netty.buffer.ByteBufOutputStream; -import io.netty.buffer.ByteBufUtil; import io.netty.buffer.Unpooled; import io.netty.channel.ChannelHandler.Sharable; import io.netty.channel.ChannelHandlerContext; @@ -74,6 +75,16 @@ public class BookieProtoEncoding { static class RequestEnDeCoderPreV3 implements EnDecoder { final ExtensionRegistry extensionRegistry; + //This empty master key is used when an empty password is provided which is the hash of an empty string + private final static byte[] emptyPasswordMasterKey; + static { + try { + emptyPasswordMasterKey = MacDigestManager.genDigest("ledger", new byte[0]); + } catch (NoSuchAlgorithmException e) { + throw new RuntimeException(e); + } + } + RequestEnDeCoderPreV3(ExtensionRegistry extensionRegistry) { this.extensionRegistry = extensionRegistry; } @@ -134,29 +145,27 @@ public class BookieProtoEncoding { // packet format is different between ADDENTRY and READENTRY long ledgerId = -1; long entryId = BookieProtocol.INVALID_ENTRY_ID; - byte[] masterKey = null; + short flags = h.getFlags(); ServerStats.getInstance().incrementPacketsReceived(); switch (h.getOpCode()) { - case BookieProtocol.ADDENTRY: - // first read master key - masterKey = new byte[BookieProtocol.MASTER_KEY_LENGTH]; - packet.readBytes(masterKey, 0, BookieProtocol.MASTER_KEY_LENGTH); + case BookieProtocol.ADDENTRY: { + byte[] masterKey = readMasterKey(packet); // Read ledger and entry id without advancing the reader index ledgerId = packet.getLong(packet.readerIndex()); entryId = packet.getLong(packet.readerIndex() + 8); return new BookieProtocol.AddRequest(h.getVersion(), ledgerId, entryId, flags, masterKey, packet.retain()); + } case BookieProtocol.READENTRY: ledgerId = packet.readLong(); entryId = packet.readLong(); if ((flags & BookieProtocol.FLAG_DO_FENCING) == BookieProtocol.FLAG_DO_FENCING && h.getVersion() >= 2) { - masterKey = new byte[BookieProtocol.MASTER_KEY_LENGTH]; - packet.readBytes(masterKey, 0, BookieProtocol.MASTER_KEY_LENGTH); + byte[] masterKey = readMasterKey(packet); return new BookieProtocol.ReadRequest(h.getVersion(), ledgerId, entryId, flags, masterKey); } else { return new BookieProtocol.ReadRequest(h.getVersion(), ledgerId, entryId, flags); @@ -170,6 +179,31 @@ public class BookieProtoEncoding { return packet; } + + private static byte[] readMasterKey(ByteBuf packet) { + byte[] masterKey = null; + + // check if the master key is an empty master key + boolean isEmptyKey = true; + for (int i = 0; i < BookieProtocol.MASTER_KEY_LENGTH; i++) { + if (packet.getByte(packet.readerIndex() + i) != emptyPasswordMasterKey[i]) { + isEmptyKey = false; + break; + } + } + + if (isEmptyKey) { + // avoid new allocations if incoming master key is empty and use the static master key + masterKey = emptyPasswordMasterKey; + packet.readerIndex(packet.readerIndex() + BookieProtocol.MASTER_KEY_LENGTH); + } else { + // Master key is set, we need to copy and check it + masterKey = new byte[BookieProtocol.MASTER_KEY_LENGTH]; + packet.readBytes(masterKey, 0, BookieProtocol.MASTER_KEY_LENGTH); + } + + return masterKey; + } } static class ResponseEnDeCoderPreV3 implements EnDecoder {
