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 2020/05/27 20:10:23 UTC
[GitHub] [trafficcontrol] zrhoffman opened a new pull request #4724: Supporting routing for topology-based delivery services
zrhoffman opened a new pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724
<!--
************ STOP!! ************
If this Pull Request is intended to fix a security vulnerability, DO NOT submit it! Instead, contact
the Apache Software Foundation Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://www.apache.org/security/ regarding vulnerability disclosure.
-->
## What does this PR (Pull Request) do?
<!-- Explain the changes you made here. If this fixes an Issue, identify it by
replacing the text in the checkbox item with the Issue number e.g.
- [x] This PR fixes #9001 OR is not related to any Issue
^ This will automatically close Issue number 9001 when the Pull Request is
merged (The '#' is important).
Be sure you check the box properly, see the "The following criteria are ALL
met by this PR" section for details.
-->
- [x] This PR fixes #4571 by updating /snapshot endpoints and Traffic Router for Topologies.<!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this
Pull Request. Also, feel free to add the name of a tool or script that is
affected but not on the list.
Additionally, if this Pull Request does NOT affect documentation, please
explain why documentation is not required. -->
- Documentation
- Traffic Ops
- Traffic Router
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your Pull Request. If
it includes tests (and most should), outline here the steps needed to run the
tests. If not, lay out the manual testing procedure and please explain why
tests are unnecessary for this Pull Request. -->
* Run Traffic Ops unit tests
* Run Traffic Router integration tests
## If this is a bug fix, what versions of Traffic Control are affected?
<!-- If this PR fixes a bug, please list here all of the affected versions - to
the best of your knowledge. It's also pretty helpful to include a commit hash
of where 'master' is at the time this PR is opened (if it affects master),
because what 'master' means will change over time. For example, if this PR
fixes a bug that's present in master (at commit hash '1df853c8'), in v4.0.0,
and in the current 4.0.1 Release candidate (e.g. RC1), then this list would
look like:
- master (1df853c8)
- 4.0.0
- 4.0.1 (RC1)
If you don't know what other versions might have this bug, AND don't know how
to find the commit hash of 'master', then feel free to leave this section
blank (or, preferably, delete it entirely).
-->
Not a bug fix.
## The following criteria are ALL met by this PR
<!-- Check the boxes to signify that the associated statement is true. To
"check a box", replace the space inside of the square brackets with an 'x'.
e.g.
- [ x] <- Wrong
- [x ] <- Wrong
- [] <- Wrong
- [*] <- Wrong
- [x] <- Correct!
-->
- [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 include a database migration
- [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)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you 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.
-->
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r444519499
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/edge/CacheRegister.java
##########
@@ -129,6 +130,12 @@ public void setCacheMap(final Map<String,Cache> map) {
return allCaches;
}
+ public Set<DeliveryServiceMatcher> getDeliveryServiceMatchers(final DeliveryService deliveryService) {
+ return this.deliveryServiceMatchers.stream()
+ .filter(deliveryServiceMatcher -> deliveryServiceMatcher.getDeliveryService() == deliveryService)
Review comment:
Do you purposely mean to compare the references here using `==` rather than `getDeliveryService().getId().equals(deliveryService.getId())` to compare the xml_id?
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r442584880
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandler.java
##########
@@ -491,21 +491,36 @@ private void parseTopologyConfig(final JsonNode allTopologies, final Map<String,
deliveryServiceMap.forEach((xmlId, ds) -> {
final List<DeliveryServiceReference> dsReferences = new ArrayList<>();
+ final List<String> dsNames = new ArrayList<>(); // for stats
Stream.of(ds.getTopology())
- .filter((topologyName) -> !Objects.isNull(topologyName) && topologyMap.containsKey(topologyName))
- .flatMap((topologyName) -> topologyMap.get(topologyName).stream())
- .flatMap((node) -> cacheRegister.getCacheLocation(node).getCaches().stream())
- .filter((cache) -> ds.hasRequiredCapabilities(cache.getCapabilities()))
- .forEach((cache) -> {
+ .filter(topologyName -> !Objects.isNull(topologyName) && topologyMap.containsKey(topologyName))
+ .flatMap(topologyName -> {
+ statMap.put(ds.getId(), dsNames);
+ return topologyMap.get(topologyName).stream();
+ })
+ .flatMap(node -> cacheRegister.getCacheLocation(node).getCaches().stream())
+ .filter(cache -> ds.hasRequiredCapabilities(cache.getCapabilities()))
+ .forEach(cache -> {
+ cacheRegister.getDeliveryServiceMatchers(ds).stream()
+ .flatMap(deliveryServiceMatcher -> deliveryServiceMatcher.getRequestMatchers().stream())
+ .map(requestMatcher -> requestMatcher.getPattern().pattern())
+ .forEach(pattern -> {
+ final String remap = ds.getRemap(pattern);
+ final String fqdn = pattern.contains(".*") && !ds.isDns()
+ ? cache.getId() + "." + remap
+ : remap;
+ dsNames.add(getDsName(fqdn, tld));
+ try {
+ dsReferences.add(new DeliveryServiceReference(ds.getId(), fqdn));
Review comment:
The alias-detecting condition was catching DNS DS URLs, fixed in 18f2f3a15. I am getting proper responses back for DNS DSes.
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r442492247
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandler.java
##########
@@ -454,6 +476,31 @@ private void parseCacheConfig(final JsonNode contentServers, final CacheRegister
return deliveryServiceMap;
}
+ private void parseTopologyConfig(final JsonNode allTopologies, final Map<String, DeliveryService> deliveryServiceMap, final CacheRegister cacheRegister) {
+ final Map<String, List<String>> topologyMap = new HashMap<>();
+ allTopologies.fieldNames().forEachRemaining((String topologyName) -> {
+ final List<String> nodes = new ArrayList<>();
+ allTopologies.get(topologyName).get("nodes").forEach((JsonNode cache) -> nodes.add(cache.textValue()));
+ topologyMap.put(topologyName, nodes);
+ });
+
+ deliveryServiceMap.forEach((xmlId, ds) -> {
+ final List<DeliveryServiceReference> dsReferences = new ArrayList<>();
+ try {
+ dsReferences.add(new DeliveryServiceReference(ds.getId(), ds.getDomain()));
Review comment:
As of af6c35ecd, all delivery service URLs are considered when parsing the topologies config (including initialization of stat tracking)
----------------------------------------------------------------
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 pull request #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#issuecomment-648916618
NullPointer exceptions in ConsistentHasherTest, DeliveryServiceHTTPRoutingMissesTest, and DeliveryServiceTest fixed in e162d16bd.
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r440404360
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandler.java
##########
@@ -454,6 +476,31 @@ private void parseCacheConfig(final JsonNode contentServers, final CacheRegister
return deliveryServiceMap;
}
+ private void parseTopologyConfig(final JsonNode allTopologies, final Map<String, DeliveryService> deliveryServiceMap, final CacheRegister cacheRegister) {
+ final Map<String, List<String>> topologyMap = new HashMap<>();
+ allTopologies.fieldNames().forEachRemaining((String topologyName) -> {
+ final List<String> nodes = new ArrayList<>();
+ allTopologies.get(topologyName).get("nodes").forEach((JsonNode cache) -> nodes.add(cache.textValue()));
+ topologyMap.put(topologyName, nodes);
+ });
+
+ deliveryServiceMap.forEach((xmlId, ds) -> {
+ final List<DeliveryServiceReference> dsReferences = new ArrayList<>();
+ try {
+ dsReferences.add(new DeliveryServiceReference(ds.getId(), ds.getDomain()));
Review comment:
Data generated locally in c6b7ed474d
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r435607518
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/ds/DeliveryService.java
##########
@@ -590,6 +610,14 @@ public String getRoutingName() {
return routingName;
}
+ public String getTopology() {
+ return topology;
+ }
+
+ public boolean hasRequiredCapabilities(final Set<String> capabilities) {
+ return this.requiredCapabilities.containsAll(capabilities);
Review comment:
Logic flipped in 01f871c17
##########
File path: traffic_router/core/src/test/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandlerTest.java
##########
@@ -15,12 +15,11 @@
package com.comcast.cdn.traffic_control.traffic_router.core.config;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;
Review comment:
Using explicit imports in 9e680217f
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandler.java
##########
@@ -18,19 +18,11 @@
import java.io.IOException;
import java.net.UnknownHostException;
import java.net.URL;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Date;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import java.util.TreeSet;
-import java.util.Iterator;
+import java.util.*;
Review comment:
Using explicit imports in 9e680217f
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/cache/Cache.java
##########
@@ -18,11 +18,7 @@
import java.net.Inet6Address;
import java.net.InetAddress;
import java.net.UnknownHostException;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
Review comment:
Using explicit imports in 9e680217f (and increased Intellij's threshold for using wildcard imports to 9999 classes, because -1 just always uses it)
##########
File path: traffic_ops/traffic_ops_golang/crconfig/topologies.go
##########
@@ -0,0 +1,66 @@
+package crconfig
+
+import (
+ "database/sql"
+ "errors"
+ "fmt"
+ "github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/lib/pq"
+)
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+
+func makeTopologies(tx *sql.Tx) (map[string]tc.CRConfigTopology, error) {
+ query := `
+SELECT
+ t.name,
+ (SELECT ARRAY_AGG(tc.cachegroup ORDER BY tc.cachegroup)
+ FROM topology_cachegroup tc
+ JOIN cachegroup c ON c.name = tc.cachegroup
+ JOIN type ON type.id = c.type
+ WHERE t.name = tc.topology
+ AND type.name = $1
+ ) AS nodes
+FROM topology t
+ORDER BY t.name
+`
+ var rows *sql.Rows
+ var err error
+ if rows, err = tx.Query(query, tc.CacheGroupEdgeTypeName); err != nil {
+ return nil, errors.New("querying topologies: " + err.Error())
+ }
+
+ topologies := map[string]tc.CRConfigTopology{}
+ for rows.Next() {
+ topology := tc.CRConfigTopology{}
+ var name string
+ if err = rows.Scan(
+ &name,
+ pq.Array(&topology.Nodes),
+ ); err != nil {
+ return nil, errors.New("scanning topology: " + err.Error())
+ }
+ topologies[name] = topology
+ }
+ if err = rows.Close(); err != nil {
Review comment:
Aw you got me. Deferring in 51fc49d66
##########
File path: traffic_ops/traffic_ops_golang/crconfig/topologies.go
##########
@@ -0,0 +1,66 @@
+package crconfig
+
+import (
+ "database/sql"
+ "errors"
+ "fmt"
+ "github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/lib/pq"
+)
+
+/*
Review comment:
Apache header moved in 242a9fd66
##########
File path: docs/source/api/v3/cdns_name_snapshot_new.rst
##########
@@ -118,6 +119,7 @@ Response Structure
:contentServers: An object containing keys which are the (short) hostnames of the :term:`Edge-Tier cache servers` in the CDN; the values corresponding to those keys are routing information for said servers
:cacheGroup: A string that is the :ref:`cache-group-name` of the :term:`Cache Group` to which the server belongs
+ :capabilities: An array of this Cache Group's :term:`Server Capabilities`. If the Cache Group has no Server Capabilities, this field is omitted.
Review comment:
Corrected to Cache Server in 64507498a
##########
File path: docs/source/api/v3/cdns_name_snapshot.rst
##########
@@ -359,184 +370,204 @@ Response Structure
"dnssec.enabled": "false",
"domain_name": "mycdn.ciab.test",
"federationmapping.polling.interval": "60000",
- "federationmapping.polling.url": "https://${toHostname}/api/3.0/federations",
+ "federationmapping.polling.url": "https://${toHostname}/api/2.0/federations/all",
"geolocation.polling.interval": "86400000",
"geolocation.polling.url": "https://trafficops.infra.ciab.test:443/GeoLite2-City.mmdb.gz",
"keystore.maintenance.interval": "300",
"neustar.polling.interval": "86400000",
"neustar.polling.url": "https://trafficops.infra.ciab.test:443/neustar.tar.gz",
"soa": {
- "admin": "twelve_monkeys",
- "expire": "604800",
- "minimum": "30",
- "refresh": "28800",
- "retry": "7200"
+ "admin": "twelve_monkeys",
+ "expire": "604800",
+ "minimum": "30",
+ "refresh": "28800",
+ "retry": "7200"
},
"steeringmapping.polling.interval": "60000",
"ttls": {
- "A": "3600",
- "AAAA": "3600",
- "DNSKEY": "30",
- "DS": "30",
- "NS": "3600",
- "SOA": "86400"
+ "A": "3600",
+ "AAAA": "3600",
+ "DNSKEY": "30",
+ "DS": "30",
+ "NS": "3600",
+ "SOA": "86400"
},
"zonemanager.cache.maintenance.interval": "300",
"zonemanager.threadpool.scale": "0.50"
- },
- "contentServers": {
+ },
+ "contentRouters": {
+ "trafficrouter": {
+ "api.port": "3333",
+ "fqdn": "trafficrouter.infra.ciab.test",
+ "httpsPort": 443,
+ "ip": "172.26.0.15",
+ "ip6": "",
+ "location": "CDN_in_a_Box_Edge",
+ "port": 80,
+ "profile": "CCR_CIAB",
+ "secure.api.port": "3443",
+ "status": "ONLINE"
+ }
+ },
+ "contentServers": {
"edge": {
- "cacheGroup": "CDN_in_a_Box_Edge",
- "fqdn": "edge.infra.ciab.test",
- "hashCount": 999,
- "hashId": "edge",
- "httpsPort": 443,
- "interfaceName": "eth0",
- "ip": "172.16.239.100",
- "ip6": "fc01:9400:1000:8::100",
- "locationId": "CDN_in_a_Box_Edge",
- "port": 80,
- "profile": "ATS_EDGE_TIER_CACHE",
- "status": "REPORTED",
- "type": "EDGE",
- "deliveryServices": {
- "demo1": [
- "edge.demo1.mycdn.ciab.test"
- ]
- },
- "routingDisabled": 0
+ "cacheGroup": "CDN_in_a_Box_Edge",
+ "capabilities": [
+ "heat-vision"
Review comment:
 <sup>:fire: :fire:</sup>
##########
File path: docs/source/api/v3/cdns_name_snapshot.rst
##########
@@ -119,6 +120,7 @@ Response Structure
:contentServers: An object containing keys which are the (short) hostnames of the :term:`Edge-Tier cache servers` in the CDN; the values corresponding to those keys are routing information for said servers
:cacheGroup: A string that is the :ref:`cache-group-name` of the :term:`Cache Group` to which the server belongs
+ :capabilities: An array of this Cache Group's :term:`Server Capabilities`. If the Cache Group has no Server Capabilities, this field is omitted.
Review comment:
Corrected to Cache Server in 64507498a
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r442558486
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandler.java
##########
@@ -491,21 +491,36 @@ private void parseTopologyConfig(final JsonNode allTopologies, final Map<String,
deliveryServiceMap.forEach((xmlId, ds) -> {
final List<DeliveryServiceReference> dsReferences = new ArrayList<>();
+ final List<String> dsNames = new ArrayList<>(); // for stats
Stream.of(ds.getTopology())
- .filter((topologyName) -> !Objects.isNull(topologyName) && topologyMap.containsKey(topologyName))
- .flatMap((topologyName) -> topologyMap.get(topologyName).stream())
- .flatMap((node) -> cacheRegister.getCacheLocation(node).getCaches().stream())
- .filter((cache) -> ds.hasRequiredCapabilities(cache.getCapabilities()))
- .forEach((cache) -> {
+ .filter(topologyName -> !Objects.isNull(topologyName) && topologyMap.containsKey(topologyName))
+ .flatMap(topologyName -> {
+ statMap.put(ds.getId(), dsNames);
+ return topologyMap.get(topologyName).stream();
+ })
+ .flatMap(node -> cacheRegister.getCacheLocation(node).getCaches().stream())
+ .filter(cache -> ds.hasRequiredCapabilities(cache.getCapabilities()))
+ .forEach(cache -> {
+ cacheRegister.getDeliveryServiceMatchers(ds).stream()
+ .flatMap(deliveryServiceMatcher -> deliveryServiceMatcher.getRequestMatchers().stream())
+ .map(requestMatcher -> requestMatcher.getPattern().pattern())
+ .forEach(pattern -> {
+ final String remap = ds.getRemap(pattern);
+ final String fqdn = pattern.contains(".*") && !ds.isDns()
+ ? cache.getId() + "." + remap
+ : remap;
+ dsNames.add(getDsName(fqdn, tld));
+ try {
+ dsReferences.add(new DeliveryServiceReference(ds.getId(), fqdn));
Review comment:
Ok, this is better, but it still doesn't seem to account for DNS-routed delivery services. Referring to my example above, it _appears_ to be adding `xmlId.cdnDomain` to `dsReferences`, when really I think that is supposed to be `routingName.xmlId.cdnDomain`. From my example, it should add `bar.bar.cdn.example.com` not `bar.example.cdn.com`.
Does that make sense? If I create that DNS-routed delivery service, assign it to a Topology, snapshot, then `dig @localhost -p1053 bar.bar.example.cdn.com a`, I should get a NOERROR response back with the cache IP addresses, but I'm getting NXDOMAIN. Are you getting proper responses back?
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r437056996
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandler.java
##########
@@ -454,6 +476,31 @@ private void parseCacheConfig(final JsonNode contentServers, final CacheRegister
return deliveryServiceMap;
}
+ private void parseTopologyConfig(final JsonNode allTopologies, final Map<String, DeliveryService> deliveryServiceMap, final CacheRegister cacheRegister) {
+ final Map<String, List<String>> topologyMap = new HashMap<>();
+ allTopologies.fieldNames().forEachRemaining((String topologyName) -> {
+ final List<String> nodes = new ArrayList<>();
+ allTopologies.get(topologyName).get("nodes").forEach((JsonNode cache) -> nodes.add(cache.textValue()));
+ topologyMap.put(topologyName, nodes);
+ });
+
+ deliveryServiceMap.forEach((xmlId, ds) -> {
+ final List<DeliveryServiceReference> dsReferences = new ArrayList<>();
+ try {
+ dsReferences.add(new DeliveryServiceReference(ds.getId(), ds.getDomain()));
Review comment:
So I think `ds.getDomain()` is wrong here. Looking at how this is done for non-Topology-based DSes: https://github.com/apache/trafficcontrol/blob/master/traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandler.java#L393
in the CRConfig those names are not just the DS domain -- they're the full cache hostnames (like in the example delivery URLs) plus any CNAME aliases:
```
"contentServers": {
"edge-01": {
"deliveryServices": {
"foo": [
"edge-01.foo.mycdn.example.com",
"blah.example.com",
"bar.someotherdomain.example.com"
],
```
I attempted to curl a topology-based DS, but was 302'd to just the DS domain (e.g. foo.mycdn.example.com) rather than the actual cache (e.g. edge-01.mycdn.example.com) as expected. I think we may need to basically replicate the functionality of TO's CRConfig generation for those hostname arrays in TR for topology-based DSes. I think we should have enough information in CRConfig to generate that data locally, since I think we may be able to use the `matchsets` array on the DS objects.
Here is how it's done in TO: https://github.com/apache/trafficcontrol/blob/master/traffic_ops/traffic_ops_golang/crconfig/servers.go#L257
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r440450805
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandler.java
##########
@@ -454,6 +476,31 @@ private void parseCacheConfig(final JsonNode contentServers, final CacheRegister
return deliveryServiceMap;
}
+ private void parseTopologyConfig(final JsonNode allTopologies, final Map<String, DeliveryService> deliveryServiceMap, final CacheRegister cacheRegister) {
+ final Map<String, List<String>> topologyMap = new HashMap<>();
+ allTopologies.fieldNames().forEachRemaining((String topologyName) -> {
+ final List<String> nodes = new ArrayList<>();
+ allTopologies.get(topologyName).get("nodes").forEach((JsonNode cache) -> nodes.add(cache.textValue()));
+ topologyMap.put(topologyName, nodes);
+ });
+
+ deliveryServiceMap.forEach((xmlId, ds) -> {
+ final List<DeliveryServiceReference> dsReferences = new ArrayList<>();
+ try {
+ dsReferences.add(new DeliveryServiceReference(ds.getId(), ds.getDomain()));
Review comment:
So I think that covers the "normal" use case, where a delivery service doesn't have any "vanity URLs" -- created via adding another HOST_REGEXP with set > 0, e.g. foo.example.com -- but does it handle the Vanity URL use case?
Say my "normal" URL is cdn.myds.cdn.example.com (xml ID is "myds"), if you add another HOST_REGEXP value "foo.example.com" with order = 1, "foo.example.com" becomes the Vanity URL. This works by having the foo.example.com CNAME to cdn.myds.cdn.example.com. So the `Host` client header that TR sees is "foo.example.com", and it needs to match that URL to the "myds" delivery service somehow.
Looking closely at https://github.com/apache/trafficcontrol/blob/master/traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandler.java#L398-L408 though, it looks like really pertains to `statMap` handling, which makes me think we'll probably _route_ the request fine, but it might not be counted for the right DS in that `statsMap`? That `statsMap` stuff should be http://localhost:3333/crs/stats by the way.
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r436651873
##########
File path: docs/source/api/v3/cdns_name_snapshot.rst
##########
@@ -359,184 +370,204 @@ Response Structure
"dnssec.enabled": "false",
"domain_name": "mycdn.ciab.test",
"federationmapping.polling.interval": "60000",
- "federationmapping.polling.url": "https://${toHostname}/api/3.0/federations",
+ "federationmapping.polling.url": "https://${toHostname}/api/2.0/federations/all",
"geolocation.polling.interval": "86400000",
"geolocation.polling.url": "https://trafficops.infra.ciab.test:443/GeoLite2-City.mmdb.gz",
"keystore.maintenance.interval": "300",
"neustar.polling.interval": "86400000",
"neustar.polling.url": "https://trafficops.infra.ciab.test:443/neustar.tar.gz",
"soa": {
- "admin": "twelve_monkeys",
- "expire": "604800",
- "minimum": "30",
- "refresh": "28800",
- "retry": "7200"
+ "admin": "twelve_monkeys",
+ "expire": "604800",
+ "minimum": "30",
+ "refresh": "28800",
+ "retry": "7200"
},
"steeringmapping.polling.interval": "60000",
"ttls": {
- "A": "3600",
- "AAAA": "3600",
- "DNSKEY": "30",
- "DS": "30",
- "NS": "3600",
- "SOA": "86400"
+ "A": "3600",
+ "AAAA": "3600",
+ "DNSKEY": "30",
+ "DS": "30",
+ "NS": "3600",
+ "SOA": "86400"
},
"zonemanager.cache.maintenance.interval": "300",
"zonemanager.threadpool.scale": "0.50"
- },
- "contentServers": {
+ },
+ "contentRouters": {
+ "trafficrouter": {
+ "api.port": "3333",
+ "fqdn": "trafficrouter.infra.ciab.test",
+ "httpsPort": 443,
+ "ip": "172.26.0.15",
+ "ip6": "",
+ "location": "CDN_in_a_Box_Edge",
+ "port": 80,
+ "profile": "CCR_CIAB",
+ "secure.api.port": "3443",
+ "status": "ONLINE"
+ }
+ },
+ "contentServers": {
"edge": {
- "cacheGroup": "CDN_in_a_Box_Edge",
- "fqdn": "edge.infra.ciab.test",
- "hashCount": 999,
- "hashId": "edge",
- "httpsPort": 443,
- "interfaceName": "eth0",
- "ip": "172.16.239.100",
- "ip6": "fc01:9400:1000:8::100",
- "locationId": "CDN_in_a_Box_Edge",
- "port": 80,
- "profile": "ATS_EDGE_TIER_CACHE",
- "status": "REPORTED",
- "type": "EDGE",
- "deliveryServices": {
- "demo1": [
- "edge.demo1.mycdn.ciab.test"
- ]
- },
- "routingDisabled": 0
+ "cacheGroup": "CDN_in_a_Box_Edge",
+ "capabilities": [
+ "heat-vision"
Review comment:
Using a realistic server capability in 4d7c0bc33
----------------------------------------------------------------
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 merged pull request #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
rawlinp merged pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r444563736
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/edge/CacheRegister.java
##########
@@ -129,6 +130,12 @@ public void setCacheMap(final Map<String,Cache> map) {
return allCaches;
}
+ public Set<DeliveryServiceMatcher> getDeliveryServiceMatchers(final DeliveryService deliveryService) {
+ return this.deliveryServiceMatchers.stream()
+ .filter(deliveryServiceMatcher -> deliveryServiceMatcher.getDeliveryService() == deliveryService)
Review comment:
Nope! Comparing xml_ids in 1f9d31e444.
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/ds/DeliveryService.java
##########
@@ -76,13 +77,19 @@
@JsonIgnore
private final String domain;
@JsonIgnore
+ private final String tld;
+ @JsonIgnore
+ private final Pattern wildcardPattern = Pattern.compile("^\\(\\.\\*\\\\\\.\\|\\^\\)|^\\.\\*\\\\\\.|\\\\\\.\\.\\*");
Review comment:
Included a comment that explains an unescaped version of the regex in 067427f975
----------------------------------------------------------------
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 pull request #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#issuecomment-648530907
Tests pass now in 68c2181c0.
----------------------------------------------------------------
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 pull request #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#issuecomment-648904567
That appears to have fixed ConfigHandlerTest, but I'm still seeing errors on the other ones:
```
Results :
Tests in error:
DeliveryServiceTest.itAddsRequiredCapabilities:86 » NullPointer
DeliveryServiceTest.itConfiguresRequestHeadersFromJSON:77 » NullPointer
DeliveryServiceTest.itExtractsQueryParams:68 » NullPointer
DeliveryServiceTest.itHandlesDuplicatesInConsistentHashQueryParams:55 » NullPointer
DeliveryServiceTest.itHandlesLackOfConsistentHashQueryParamsInJSON:46 » NullPointer
DeliveryServiceTest.itHandlesLackOfRequestHeaderNamesInJSON:38 » NullPointer
ConsistentHasherTest.itHashesQueryParams:216 » NullPointer
DeliveryServiceHTTPRoutingMissesTest.before:52 » NullPointer
DeliveryServiceHTTPRoutingMissesTest.before:52 » NullPointer
DeliveryServiceHTTPRoutingMissesTest.before:52 » NullPointer
Tests run: 211, Failures: 0, Errors: 10, Skipped: 23
```
----------------------------------------------------------------
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 pull request #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#issuecomment-640955437
Rebased to resolve merge conflicts. As @rawlinp [noted](/apache/trafficcontrol/pull/4765#issuecomment-640947401), no code needed to be changed, all it took was deleting a couple of empty conflict sections in Node.java.
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r444533031
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/ds/DeliveryService.java
##########
@@ -76,13 +77,19 @@
@JsonIgnore
private final String domain;
@JsonIgnore
+ private final String tld;
+ @JsonIgnore
+ private final Pattern wildcardPattern = Pattern.compile("^\\(\\.\\*\\\\\\.\\|\\^\\)|^\\.\\*\\\\\\.|\\\\\\.\\.\\*");
Review comment:
Just looking at this I can't tell what the _actual_ regex is supposed to be with all these escaping backslashes. Can you include a comment that includes the unescaped version of the regex?
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r442518777
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandler.java
##########
@@ -491,21 +491,36 @@ private void parseTopologyConfig(final JsonNode allTopologies, final Map<String,
deliveryServiceMap.forEach((xmlId, ds) -> {
final List<DeliveryServiceReference> dsReferences = new ArrayList<>();
+ final List<String> dsNames = new ArrayList<>(); // for stats
Stream.of(ds.getTopology())
- .filter((topologyName) -> !Objects.isNull(topologyName) && topologyMap.containsKey(topologyName))
- .flatMap((topologyName) -> topologyMap.get(topologyName).stream())
- .flatMap((node) -> cacheRegister.getCacheLocation(node).getCaches().stream())
- .filter((cache) -> ds.hasRequiredCapabilities(cache.getCapabilities()))
- .forEach((cache) -> {
+ .filter(topologyName -> !Objects.isNull(topologyName) && topologyMap.containsKey(topologyName))
+ .flatMap(topologyName -> {
+ statMap.put(ds.getId(), dsNames);
+ return topologyMap.get(topologyName).stream();
+ })
+ .flatMap(node -> cacheRegister.getCacheLocation(node).getCaches().stream())
+ .filter(cache -> ds.hasRequiredCapabilities(cache.getCapabilities()))
+ .forEach(cache -> {
+ cacheRegister.getDeliveryServiceMatchers(ds).stream()
+ .flatMap(deliveryServiceMatcher -> deliveryServiceMatcher.getRequestMatchers().stream())
+ .map(requestMatcher -> requestMatcher.getPattern().pattern())
+ .forEach(pattern -> {
+ final String remap = ds.getRemap(pattern);
+ final String fqdn = pattern.contains(".*") && !ds.isDns()
+ ? cache.getId() + "." + remap
+ : remap;
+ dsNames.add(getDsName(fqdn, tld));
+ try {
+ dsReferences.add(new DeliveryServiceReference(ds.getId(), fqdn));
Review comment:
This seems to be a little bit different from the legacy (deliveryservice-server) behavior in `parseCacheConfig`. That legacy behavior seemed to only add the "standard" `routingName.xmlId.cdnDomain` FQDN for DNS deliveryservices or the `hostName.xmlId.cdnDomain` FQDN for HTTP deliveryservices into `dsReferences` -- not the "alias" FQDNs. That is because the first server-deliveryService reference is never the "alias" FQDN. As far as I can tell, it is only the `statTracker` that seems to need the "alias" FQDNs.
Basically, if a cache "cache01" belongs to a topology that has the given delivery services assigned:
```
xml_id = foo, routing_name = foo, routing_type = HTTP, 0th pattern = .*\.foo\..*
xml_id = bar, routing_name = bar, routing_type = DNS, 0th pattern = .*\.bar\..*
```
I would expect only the following FQDNs to be added to the cache's `DeliveryServiceReferences`:
```
cache01.foo.cdn.example.com
bar.bar.cdn.example.com
```
The aliases, if the delivery services have any, would *not* be added to the cache's `DeliveryServiceReferences`, but would only be added to the `dsNames` in the `statMap` (in addition to the aforementioned "standard" FQDNs).
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r442572811
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandler.java
##########
@@ -491,21 +491,36 @@ private void parseTopologyConfig(final JsonNode allTopologies, final Map<String,
deliveryServiceMap.forEach((xmlId, ds) -> {
final List<DeliveryServiceReference> dsReferences = new ArrayList<>();
+ final List<String> dsNames = new ArrayList<>(); // for stats
Stream.of(ds.getTopology())
- .filter((topologyName) -> !Objects.isNull(topologyName) && topologyMap.containsKey(topologyName))
- .flatMap((topologyName) -> topologyMap.get(topologyName).stream())
- .flatMap((node) -> cacheRegister.getCacheLocation(node).getCaches().stream())
- .filter((cache) -> ds.hasRequiredCapabilities(cache.getCapabilities()))
- .forEach((cache) -> {
+ .filter(topologyName -> !Objects.isNull(topologyName) && topologyMap.containsKey(topologyName))
+ .flatMap(topologyName -> {
+ statMap.put(ds.getId(), dsNames);
+ return topologyMap.get(topologyName).stream();
+ })
+ .flatMap(node -> cacheRegister.getCacheLocation(node).getCaches().stream())
+ .filter(cache -> ds.hasRequiredCapabilities(cache.getCapabilities()))
+ .forEach(cache -> {
+ cacheRegister.getDeliveryServiceMatchers(ds).stream()
+ .flatMap(deliveryServiceMatcher -> deliveryServiceMatcher.getRequestMatchers().stream())
+ .map(requestMatcher -> requestMatcher.getPattern().pattern())
+ .forEach(pattern -> {
+ final String remap = ds.getRemap(pattern);
+ final String fqdn = pattern.contains(".*") && !ds.isDns()
+ ? cache.getId() + "." + remap
+ : remap;
+ dsNames.add(getDsName(fqdn, tld));
+ try {
+ dsReferences.add(new DeliveryServiceReference(ds.getId(), fqdn));
Review comment:
Oops, it was prepending the xmlId for DNS DSes. Changed it in 35f96f518 to prepend the routing name instead.
> Are you getting proper responses back?
Not for a DNS DS, I missed that detail in the examples you provided.
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r434826150
##########
File path: docs/source/api/v3/cdns_name_snapshot.rst
##########
@@ -119,6 +120,7 @@ Response Structure
:contentServers: An object containing keys which are the (short) hostnames of the :term:`Edge-Tier cache servers` in the CDN; the values corresponding to those keys are routing information for said servers
:cacheGroup: A string that is the :ref:`cache-group-name` of the :term:`Cache Group` to which the server belongs
+ :capabilities: An array of this Cache Group's :term:`Server Capabilities`. If the Cache Group has no Server Capabilities, this field is omitted.
Review comment:
I think you mean "Server" instead of "Cache Group" here
##########
File path: docs/source/api/v3/cdns_name_snapshot_new.rst
##########
@@ -118,6 +119,7 @@ Response Structure
:contentServers: An object containing keys which are the (short) hostnames of the :term:`Edge-Tier cache servers` in the CDN; the values corresponding to those keys are routing information for said servers
:cacheGroup: A string that is the :ref:`cache-group-name` of the :term:`Cache Group` to which the server belongs
+ :capabilities: An array of this Cache Group's :term:`Server Capabilities`. If the Cache Group has no Server Capabilities, this field is omitted.
Review comment:
ditto previous comment
##########
File path: docs/source/api/v3/cdns_name_snapshot.rst
##########
@@ -359,184 +370,204 @@ Response Structure
"dnssec.enabled": "false",
"domain_name": "mycdn.ciab.test",
"federationmapping.polling.interval": "60000",
- "federationmapping.polling.url": "https://${toHostname}/api/3.0/federations",
+ "federationmapping.polling.url": "https://${toHostname}/api/2.0/federations/all",
"geolocation.polling.interval": "86400000",
"geolocation.polling.url": "https://trafficops.infra.ciab.test:443/GeoLite2-City.mmdb.gz",
"keystore.maintenance.interval": "300",
"neustar.polling.interval": "86400000",
"neustar.polling.url": "https://trafficops.infra.ciab.test:443/neustar.tar.gz",
"soa": {
- "admin": "twelve_monkeys",
- "expire": "604800",
- "minimum": "30",
- "refresh": "28800",
- "retry": "7200"
+ "admin": "twelve_monkeys",
+ "expire": "604800",
+ "minimum": "30",
+ "refresh": "28800",
+ "retry": "7200"
},
"steeringmapping.polling.interval": "60000",
"ttls": {
- "A": "3600",
- "AAAA": "3600",
- "DNSKEY": "30",
- "DS": "30",
- "NS": "3600",
- "SOA": "86400"
+ "A": "3600",
+ "AAAA": "3600",
+ "DNSKEY": "30",
+ "DS": "30",
+ "NS": "3600",
+ "SOA": "86400"
},
"zonemanager.cache.maintenance.interval": "300",
"zonemanager.threadpool.scale": "0.50"
- },
- "contentServers": {
+ },
+ "contentRouters": {
+ "trafficrouter": {
+ "api.port": "3333",
+ "fqdn": "trafficrouter.infra.ciab.test",
+ "httpsPort": 443,
+ "ip": "172.26.0.15",
+ "ip6": "",
+ "location": "CDN_in_a_Box_Edge",
+ "port": 80,
+ "profile": "CCR_CIAB",
+ "secure.api.port": "3443",
+ "status": "ONLINE"
+ }
+ },
+ "contentServers": {
"edge": {
- "cacheGroup": "CDN_in_a_Box_Edge",
- "fqdn": "edge.infra.ciab.test",
- "hashCount": 999,
- "hashId": "edge",
- "httpsPort": 443,
- "interfaceName": "eth0",
- "ip": "172.16.239.100",
- "ip6": "fc01:9400:1000:8::100",
- "locationId": "CDN_in_a_Box_Edge",
- "port": 80,
- "profile": "ATS_EDGE_TIER_CACHE",
- "status": "REPORTED",
- "type": "EDGE",
- "deliveryServices": {
- "demo1": [
- "edge.demo1.mycdn.ciab.test"
- ]
- },
- "routingDisabled": 0
+ "cacheGroup": "CDN_in_a_Box_Edge",
+ "capabilities": [
+ "heat-vision"
Review comment:
😆 🔥
##########
File path: traffic_ops/traffic_ops_golang/crconfig/topologies.go
##########
@@ -0,0 +1,66 @@
+package crconfig
+
+import (
+ "database/sql"
+ "errors"
+ "fmt"
+ "github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/lib/pq"
+)
+
+/*
Review comment:
nit: the apache header is pretty consistently placed between the package declaration and the imports
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/cache/Cache.java
##########
@@ -18,11 +18,7 @@
import java.net.Inet6Address;
import java.net.InetAddress;
import java.net.UnknownHostException;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
Review comment:
nit: I think intellij probably did this for you, but I think we pretty consistently try to use explicit imports of only what we use
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandler.java
##########
@@ -18,19 +18,11 @@
import java.io.IOException;
import java.net.UnknownHostException;
import java.net.URL;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Date;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import java.util.TreeSet;
-import java.util.Iterator;
+import java.util.*;
Review comment:
nit: I think intellij probably did this for you, but I think we pretty consistently try to use explicit imports of only what we use
##########
File path: traffic_router/core/src/test/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandlerTest.java
##########
@@ -15,12 +15,11 @@
package com.comcast.cdn.traffic_control.traffic_router.core.config;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;
Review comment:
see previous nits
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/ds/DeliveryService.java
##########
@@ -590,6 +610,14 @@ public String getRoutingName() {
return routingName;
}
+ public String getTopology() {
+ return topology;
+ }
+
+ public boolean hasRequiredCapabilities(final Set<String> capabilities) {
+ return this.requiredCapabilities.containsAll(capabilities);
Review comment:
I think this logic might be backwards. The _server_ needs to have _all_ the capabilities that the DS requires. So I think this should be `capabilities.containsAll(requiredCapabilities)`, and consider renaming `capabilities` to `serverCapabilities` to make it more clear.
##########
File path: traffic_ops/traffic_ops_golang/crconfig/topologies.go
##########
@@ -0,0 +1,66 @@
+package crconfig
+
+import (
+ "database/sql"
+ "errors"
+ "fmt"
+ "github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/lib/pq"
+)
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+
+func makeTopologies(tx *sql.Tx) (map[string]tc.CRConfigTopology, error) {
+ query := `
+SELECT
+ t.name,
+ (SELECT ARRAY_AGG(tc.cachegroup ORDER BY tc.cachegroup)
+ FROM topology_cachegroup tc
+ JOIN cachegroup c ON c.name = tc.cachegroup
+ JOIN type ON type.id = c.type
+ WHERE t.name = tc.topology
+ AND type.name = $1
+ ) AS nodes
+FROM topology t
+ORDER BY t.name
+`
+ var rows *sql.Rows
+ var err error
+ if rows, err = tx.Query(query, tc.CacheGroupEdgeTypeName); err != nil {
+ return nil, errors.New("querying topologies: " + err.Error())
+ }
+
+ topologies := map[string]tc.CRConfigTopology{}
+ for rows.Next() {
+ topology := tc.CRConfigTopology{}
+ var name string
+ if err = rows.Scan(
+ &name,
+ pq.Array(&topology.Nodes),
+ ); err != nil {
+ return nil, errors.New("scanning topology: " + err.Error())
+ }
+ topologies[name] = topology
+ }
+ if err = rows.Close(); err != nil {
Review comment:
Closing the rows should be up on L49 in a `defer` statement (maybe using `log.Close` too as you always suggest 😃 )
----------------------------------------------------------------
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 #4724: Supporting routing for topology-based delivery services
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4724:
URL: https://github.com/apache/trafficcontrol/pull/4724#discussion_r442535187
##########
File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/config/ConfigHandler.java
##########
@@ -491,21 +491,36 @@ private void parseTopologyConfig(final JsonNode allTopologies, final Map<String,
deliveryServiceMap.forEach((xmlId, ds) -> {
final List<DeliveryServiceReference> dsReferences = new ArrayList<>();
+ final List<String> dsNames = new ArrayList<>(); // for stats
Stream.of(ds.getTopology())
- .filter((topologyName) -> !Objects.isNull(topologyName) && topologyMap.containsKey(topologyName))
- .flatMap((topologyName) -> topologyMap.get(topologyName).stream())
- .flatMap((node) -> cacheRegister.getCacheLocation(node).getCaches().stream())
- .filter((cache) -> ds.hasRequiredCapabilities(cache.getCapabilities()))
- .forEach((cache) -> {
+ .filter(topologyName -> !Objects.isNull(topologyName) && topologyMap.containsKey(topologyName))
+ .flatMap(topologyName -> {
+ statMap.put(ds.getId(), dsNames);
+ return topologyMap.get(topologyName).stream();
+ })
+ .flatMap(node -> cacheRegister.getCacheLocation(node).getCaches().stream())
+ .filter(cache -> ds.hasRequiredCapabilities(cache.getCapabilities()))
+ .forEach(cache -> {
+ cacheRegister.getDeliveryServiceMatchers(ds).stream()
+ .flatMap(deliveryServiceMatcher -> deliveryServiceMatcher.getRequestMatchers().stream())
+ .map(requestMatcher -> requestMatcher.getPattern().pattern())
+ .forEach(pattern -> {
+ final String remap = ds.getRemap(pattern);
+ final String fqdn = pattern.contains(".*") && !ds.isDns()
+ ? cache.getId() + "." + remap
+ : remap;
+ dsNames.add(getDsName(fqdn, tld));
+ try {
+ dsReferences.add(new DeliveryServiceReference(ds.getId(), fqdn));
Review comment:
Not creating `DeliveryServiceReference`s for aliases in 2e509d420 (but still adding them to `statMap`).
----------------------------------------------------------------
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