You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2021/03/04 19:18:28 UTC
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5598: TR: cache and reuse RRSIG records to avoid unnecessary re-signing
rawlinp commented on a change in pull request #5598:
URL: https://github.com/apache/trafficcontrol/pull/5598#discussion_r587747440
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/RRSIGCacheKey.java
##########
@@ -0,0 +1,56 @@
+/*
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.comcast.cdn.traffic_control.traffic_router.core.dns;
+
+import java.util.Arrays;
+
+public class RRSIGCacheKey {
+ final private byte[] privateKeyBytes;
+ final private int algorithm;
+
+ public RRSIGCacheKey(final byte[] privateKeyBytes, final int algorithm) {
+ this.privateKeyBytes = privateKeyBytes;
+ this.algorithm = algorithm;
+ }
+
+ public byte[] getPrivateKeyBytes() {
+ return privateKeyBytes;
+ }
Review comment:
You're right -- I'll remove
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/SignatureManager.java
##########
@@ -177,29 +200,69 @@ public void run() {
};
}
- private boolean hasNewKeys(final Map<String, List<DnsSecKeyPair>> keyMap, final Map<String, List<DnsSecKeyPair>> newKeyMap) {
- for (final String key : newKeyMap.keySet()) {
- if (!keyMap.containsKey(key)) {
- return true;
+ private void cleanRRSIGCache(final Map<String, List<DnsSecKeyPair>> oldKeyMap, final Map<String, List<DnsSecKeyPair>> newKeyMap) {
+ synchronized (RRSIGCacheLock) {
+ if (RRSIGCache.isEmpty() || oldKeyMap == null || getKeyDifferences(oldKeyMap, newKeyMap).isEmpty()) {
+ return;
+ }
+ final int oldKeySize = RRSIGCache.size();
+ final int oldRRSIGSize = RRSIGCache.values().stream().map(Map::size).reduce(0, Integer::sum);
+ final long now = new Date().getTime();
+ final ConcurrentMap<RRSIGCacheKey, ConcurrentMap<RRsetKey, RRSIGRecord>> newRRSIGCache = new ConcurrentHashMap<>();
+ newKeyMap.forEach((name, keyPairs) -> {
+ keyPairs.forEach(keypair -> {
Review comment:
Sure, will collapse it
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/RRSIGCacheKey.java
##########
@@ -0,0 +1,56 @@
+/*
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.comcast.cdn.traffic_control.traffic_router.core.dns;
+
+import java.util.Arrays;
+
+public class RRSIGCacheKey {
+ final private byte[] privateKeyBytes;
+ final private int algorithm;
+
+ public RRSIGCacheKey(final byte[] privateKeyBytes, final int algorithm) {
+ this.privateKeyBytes = privateKeyBytes;
+ this.algorithm = algorithm;
+ }
+
+ public byte[] getPrivateKeyBytes() {
+ return privateKeyBytes;
+ }
+
+ public int getAlgorithm() {
+ return algorithm;
+ }
Review comment:
You're right -- I'll remove
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/dns/SignatureManager.java
##########
@@ -177,29 +200,69 @@ public void run() {
};
}
- private boolean hasNewKeys(final Map<String, List<DnsSecKeyPair>> keyMap, final Map<String, List<DnsSecKeyPair>> newKeyMap) {
- for (final String key : newKeyMap.keySet()) {
- if (!keyMap.containsKey(key)) {
- return true;
+ private void cleanRRSIGCache(final Map<String, List<DnsSecKeyPair>> oldKeyMap, final Map<String, List<DnsSecKeyPair>> newKeyMap) {
+ synchronized (RRSIGCacheLock) {
+ if (RRSIGCache.isEmpty() || oldKeyMap == null || getKeyDifferences(oldKeyMap, newKeyMap).isEmpty()) {
+ return;
+ }
+ final int oldKeySize = RRSIGCache.size();
+ final int oldRRSIGSize = RRSIGCache.values().stream().map(Map::size).reduce(0, Integer::sum);
+ final long now = new Date().getTime();
+ final ConcurrentMap<RRSIGCacheKey, ConcurrentMap<RRsetKey, RRSIGRecord>> newRRSIGCache = new ConcurrentHashMap<>();
+ newKeyMap.forEach((name, keyPairs) -> {
+ keyPairs.forEach(keypair -> {
+ final RRSIGCacheKey cacheKey = new RRSIGCacheKey(keypair.getPrivate().getEncoded(), keypair.getDNSKEYRecord().getAlgorithm());
+ final ConcurrentMap<RRsetKey, RRSIGRecord> cacheValue = RRSIGCache.get(cacheKey);
+ if (cacheValue != null) {
+ cacheValue.entrySet().removeIf(e -> e.getValue().getExpire().getTime() <= now);
+ newRRSIGCache.put(cacheKey, cacheValue);
+ }
+ });
+ });
+ RRSIGCache = newRRSIGCache;
+ final int keySize = RRSIGCache.size();
+ final int RRSIGSize = RRSIGCache.values().stream().map(Map::size).reduce(0, Integer::sum);
+ LOGGER.info("DNSSEC keys were changed or removed so RRSIG cache was cleaned. Old key size: " + oldKeySize +
+ ", new key size: " + keySize + ", old RRSIG size: " + oldRRSIGSize + ", new RRSIG size: " + RRSIGSize);
+ }
+ }
+
+ // return the key names from newKeyMap that are different or missing from oldKeyMap
+ private Set<String> getKeyDifferences(final Map<String, List<DnsSecKeyPair>> newKeyMap, final Map<String, List<DnsSecKeyPair>> oldKeyMap) {
+ final Set<String> newKeyNames = new HashSet<>();
+ for (final String newName : newKeyMap.keySet()) {
+ if (!oldKeyMap.containsKey(newName)) {
+ newKeyNames.add(newName);
+ continue;
}
- for (final DnsSecKeyPair newKeyPair : newKeyMap.get(key)) {
+ for (final DnsSecKeyPair newKeyPair : newKeyMap.get(newName)) {
boolean matched = false;
- for (final DnsSecKeyPair keyPair : keyMap.get(key)) {
+ for (final DnsSecKeyPair keyPair : oldKeyMap.get(newName)) {
if (newKeyPair.equals(keyPair)) {
matched = true;
break;
}
}
if (!matched) {
- LOGGER.info("Found new or changed key for " + newKeyPair.getName());
- return true; // has a new key because we didn't find a match
+ newKeyNames.add(newKeyPair.getName());
+ break;
}
}
}
+ return newKeyNames;
+ }
+ private boolean hasNewKeys(final Map<String, List<DnsSecKeyPair>> oldKeyMap, final Map<String, List<DnsSecKeyPair>> newKeyMap) {
+ final Set<String> newOrChangedKeyNames = getKeyDifferences(newKeyMap, oldKeyMap);
+ if (!newOrChangedKeyNames.isEmpty()) {
+ newOrChangedKeyNames.forEach(name -> {
+ LOGGER.info("Found new or changed key for " + name);
+ });
Review comment:
Sure, will collapse it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org