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/02 19:22:56 UTC
[GitHub] [trafficcontrol] rawlinp opened a new pull request #5598: TR: cache and reuse RRSIG records to avoid unnecessary re-signing
rawlinp opened a new pull request #5598:
URL: https://github.com/apache/trafficcontrol/pull/5598
## What does this PR (Pull Request) do?
Add a new TR profile parameter: 'dnssec.rrsig.cache.enabled'. When set
to 'true', RRSIG records will be cached and reused when the same RRset
is re-signed. This heavily reduces the amount of CPU used to re-sign
zones after a change is made that requires re-signing large numbers of
zones (e.g. ONLINING/OFFLINING Traffic Routers). Also, this causes
Traffic Router to startup much quicker, because duplicate work is
eliminated during the initial zone signing process.
As DNSSEC keys are removed/replaced, the RRSIG cache is cleaned up, and
entries that are no longer necessary are removed. RRsets that have a
cached RRSIG record that is more than halfway to expiring will be
re-signed.
Setting 'dnssec.rrsig.cache.enabled' to 'false' will clear the cache and
prevent it from being used for new requests until re-enabled again.
## Which Traffic Control components are affected by this PR?
- Documentation
- Traffic Router
## What is the best way to verify this PR?
Run the TR unit and integration tests, ensure they still pass. NOTE: the github checks currently run the unit tests but not the integration tests.
Create a new parameter:
```
name: dnssec.rrsig.cache.enabled
config file: CRConfig.json
value: true
```
Assign it to your Traffic Router profile, then snapshot. Verify that TR DNSSEC signing and validation all performs as expected. Set a TR server to `OFFLINE`, snapshot, and reverify.
## The following criteria are ALL met by this PR
- [x] This PR includes tests
- [x] This PR includes documentation
- [x] This PR includes an update to CHANGELOG.md
- [x] This PR includes any and all required license headers
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] zrhoffman merged pull request #5598: TR: cache and reuse RRSIG records to avoid unnecessary re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman merged pull request #5598:
URL: https://github.com/apache/trafficcontrol/pull/5598
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5598: TR: cache and reuse RRSIG records to avoid unnecessary re-signing
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5598:
URL: https://github.com/apache/trafficcontrol/pull/5598#discussion_r587659885
##########
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:
`getAlgorithm()` looks unused, too. A breakpoint I set inside it was never tripped.
##########
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:
`getPrivateKeyBytes()` seems unused.
##########
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:
Nit: Since there is only 1 expression within `newKeyMap.forEach()`'s lambda, this can just be
```java
newKeyMap.forEach((name, keyPairs) -> keyPairs.forEach(keypair -> {
```
##########
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:
Nit: This can be just
```java
newOrChangedKeyNames.forEach(name -> LOGGER.info("Found new or changed key for " + name));
```
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5598: TR: cache and reuse RRSIG records to avoid unnecessary re-signing
Posted by GitBox <gi...@apache.org>.
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