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/10/02 17:06:22 UTC
[GitHub] [trafficcontrol] rawlinp opened a new pull request #5093: Validate DS topology w.r.t. required capabilities
rawlinp opened a new pull request #5093:
URL: https://github.com/apache/trafficcontrol/pull/5093
## What does this PR (Pull Request) do?
When adding a required capability to a topology-based delivery service,
there must be at least one server (in the DS's CDN) per cachegroup in
the topology with all the required capabilities.
## Which Traffic Control components are affected by this PR?
- Traffic Ops
## What is the best way to verify this PR?
Review the TO API tests, verify they're sufficient, and verify the tests pass.
Manually attempt to add a DS required capability to a DS that is assigned to a Topology that contains at least one cachegroup that doesn't meet the criteria (must have at least one server in the DS's CDN with the required capabilities).
## The following criteria are ALL met by this PR
- [x] This PR includes tests
- [x] Validation, no docs necessary
- [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)
----------------------------------------------------------------
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 #5093: Validate addition of DS required capabilities w.r.t its topology
Posted by GitBox <gi...@apache.org>.
zrhoffman merged pull request #5093:
URL: https://github.com/apache/trafficcontrol/pull/5093
----------------------------------------------------------------
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 #5093: Validate DS topology w.r.t. required capabilities
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5093:
URL: https://github.com/apache/trafficcontrol/pull/5093#discussion_r499762043
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_required_capabilities.go
##########
@@ -329,6 +340,72 @@ func (rc *RequiredCapability) checkServerCap() (error, error, int) {
return nil, nil, http.StatusOK
}
+// EnsureTopologyBasedRequiredCapabilities ensures that at least one server per cachegroup
+// in this delivery service's topology has this delivery service's required capabilities.
+func EnsureTopologyBasedRequiredCapabilities(tx *sql.Tx, dsID int, requiredCapabilities []string) (error, error, int) {
+ q := `
+SELECT
+ s.id,
+ c.name,
+ ARRAY_REMOVE(ARRAY_AGG(ssc.server_capability ORDER BY ssc.server_capability), NULL) AS capabilities
+FROM server s
+LEFT JOIN server_server_capability ssc ON ssc.server = s.id
+JOIN cachegroup c ON c.id = s.cachegroup
+JOIN topology_cachegroup tc ON tc.cachegroup = c.name
+JOIN deliveryservice d ON d.topology = tc.topology
+WHERE
+ d.id = $1
+ AND s.cdn_id = (SELECT cdn_id FROM deliveryservice WHERE id = $1)
Review comment:
Should there be a test for that? Looks like all of the DSRC tests are on 1 CDN right now.
----------------------------------------------------------------
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] rob05c commented on pull request #5093: Validate DS topology w.r.t. required capabilities
Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #5093:
URL: https://github.com/apache/trafficcontrol/pull/5093#issuecomment-702868594
Should this warn, but not error?
For example, someone might want to assign just one particular server, or just a few servers, to a test DS. This makes that impossible, without a custom topology.
Or, someone might have an actual capability, like an ATS plugin, that they've deployed to a number of caches, but maybe they have some small cachegroups without many caches, that they don't really want to upgrade, and they're ok with clients going to a nearby large cachegroup instead.
So maybe they get a message, "Warning: no servers in cachegroup 'LittleRock', requests for this Delivery Service will not be routed to that Cachegroup!" But maybe they're ok with that, because their Nashville datacenter is nearby and large.
----------------------------------------------------------------
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 #5093: Validate DS topology w.r.t. required capabilities
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5093:
URL: https://github.com/apache/trafficcontrol/pull/5093#issuecomment-703776255
Should the title be adjusted to make it clearer that this is for POST and PUT `/deliveryservices_required_capabilities` (and not `PUT /deliveryservices`)?
----------------------------------------------------------------
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 #5093: Validate addition of DS required capabilities w.r.t its topology
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5093:
URL: https://github.com/apache/trafficcontrol/pull/5093#discussion_r499924098
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_required_capabilities.go
##########
@@ -329,6 +340,72 @@ func (rc *RequiredCapability) checkServerCap() (error, error, int) {
return nil, nil, http.StatusOK
}
+// EnsureTopologyBasedRequiredCapabilities ensures that at least one server per cachegroup
+// in this delivery service's topology has this delivery service's required capabilities.
+func EnsureTopologyBasedRequiredCapabilities(tx *sql.Tx, dsID int, requiredCapabilities []string) (error, error, int) {
+ q := `
+SELECT
+ s.id,
+ c.name,
+ ARRAY_REMOVE(ARRAY_AGG(ssc.server_capability ORDER BY ssc.server_capability), NULL) AS capabilities
+FROM server s
+LEFT JOIN server_server_capability ssc ON ssc.server = s.id
+JOIN cachegroup c ON c.id = s.cachegroup
+JOIN topology_cachegroup tc ON tc.cachegroup = c.name
+JOIN deliveryservice d ON d.topology = tc.topology
+WHERE
+ d.id = $1
+ AND s.cdn_id = (SELECT cdn_id FROM deliveryservice WHERE id = $1)
Review comment:
Tests seem to pass for me with `s.cdn_id = d.cdn_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] rawlinp commented on pull request #5093: Validate DS topology w.r.t. required capabilities
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5093:
URL: https://github.com/apache/trafficcontrol/pull/5093#issuecomment-702884627
The rationale behind this validation is that operators should be able to look at a DS's topology and know that reality matches the given information. If we allowed this with a warning, a DS's topology would no longer match reality.
I agree with you in that the downside is that custom topologies would have to be made for the certain use cases you've laid out, but I think having the information match reality is more important.
----------------------------------------------------------------
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 #5093: Validate DS topology w.r.t. required capabilities
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5093:
URL: https://github.com/apache/trafficcontrol/pull/5093#discussion_r499325438
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_required_capabilities.go
##########
@@ -329,6 +340,72 @@ func (rc *RequiredCapability) checkServerCap() (error, error, int) {
return nil, nil, http.StatusOK
}
+// EnsureTopologyBasedRequiredCapabilities ensures that at least one server per cachegroup
+// in this delivery service's topology has this delivery service's required capabilities.
+func EnsureTopologyBasedRequiredCapabilities(tx *sql.Tx, dsID int, requiredCapabilities []string) (error, error, int) {
+ q := `
+SELECT
+ s.id,
+ c.name,
+ ARRAY_REMOVE(ARRAY_AGG(ssc.server_capability ORDER BY ssc.server_capability), NULL) AS capabilities
+FROM server s
+LEFT JOIN server_server_capability ssc ON ssc.server = s.id
+JOIN cachegroup c ON c.id = s.cachegroup
+JOIN topology_cachegroup tc ON tc.cachegroup = c.name
+JOIN deliveryservice d ON d.topology = tc.topology
+WHERE
+ d.id = $1
+ AND s.cdn_id = (SELECT cdn_id FROM deliveryservice WHERE id = $1)
Review comment:
Because we already have `d.id = $1`, comparing against the result of this subquery produces the same result as filtering by `s.cdn_id = d.cdn_id`, right?
----------------------------------------------------------------
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 #5093: Validate DS topology w.r.t. required capabilities
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5093:
URL: https://github.com/apache/trafficcontrol/pull/5093#issuecomment-703773661
This PR fixes #5098
----------------------------------------------------------------
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 #5093: Validate addition of DS required capabilities w.r.t its topology
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5093:
URL: https://github.com/apache/trafficcontrol/pull/5093#discussion_r500359870
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_required_capabilities.go
##########
@@ -329,6 +340,72 @@ func (rc *RequiredCapability) checkServerCap() (error, error, int) {
return nil, nil, http.StatusOK
}
+// EnsureTopologyBasedRequiredCapabilities ensures that at least one server per cachegroup
+// in this delivery service's topology has this delivery service's required capabilities.
+func EnsureTopologyBasedRequiredCapabilities(tx *sql.Tx, dsID int, requiredCapabilities []string) (error, error, int) {
+ q := `
+SELECT
+ s.id,
+ c.name,
+ ARRAY_REMOVE(ARRAY_AGG(ssc.server_capability ORDER BY ssc.server_capability), NULL) AS capabilities
+FROM server s
+LEFT JOIN server_server_capability ssc ON ssc.server = s.id
+JOIN cachegroup c ON c.id = s.cachegroup
+JOIN topology_cachegroup tc ON tc.cachegroup = c.name
+JOIN deliveryservice d ON d.topology = tc.topology
+WHERE
+ d.id = $1
+ AND s.cdn_id = (SELECT cdn_id FROM deliveryservice WHERE id = $1)
Review comment:
Simplified in https://github.com/apache/trafficcontrol/pull/5093/commits/f3a6849cf41a33b36779d0fd6b89b136eb953cf5
----------------------------------------------------------------
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 #5093: Validate DS topology w.r.t. required capabilities
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5093:
URL: https://github.com/apache/trafficcontrol/pull/5093#discussion_r499751129
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_required_capabilities.go
##########
@@ -329,6 +340,72 @@ func (rc *RequiredCapability) checkServerCap() (error, error, int) {
return nil, nil, http.StatusOK
}
+// EnsureTopologyBasedRequiredCapabilities ensures that at least one server per cachegroup
+// in this delivery service's topology has this delivery service's required capabilities.
+func EnsureTopologyBasedRequiredCapabilities(tx *sql.Tx, dsID int, requiredCapabilities []string) (error, error, int) {
+ q := `
+SELECT
+ s.id,
+ c.name,
+ ARRAY_REMOVE(ARRAY_AGG(ssc.server_capability ORDER BY ssc.server_capability), NULL) AS capabilities
+FROM server s
+LEFT JOIN server_server_capability ssc ON ssc.server = s.id
+JOIN cachegroup c ON c.id = s.cachegroup
+JOIN topology_cachegroup tc ON tc.cachegroup = c.name
+JOIN deliveryservice d ON d.topology = tc.topology
+WHERE
+ d.id = $1
+ AND s.cdn_id = (SELECT cdn_id FROM deliveryservice WHERE id = $1)
Review comment:
Originally, I left this out of the where clause, but it included servers from other CDNs. That was unexpected, since I thought exactly what you thought. I'm not totally sure why, but this condition is necessary.
----------------------------------------------------------------
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 #5093: Validate addition of DS required capabilities w.r.t its topology
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5093:
URL: https://github.com/apache/trafficcontrol/pull/5093#discussion_r499762941
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_required_capabilities.go
##########
@@ -329,6 +340,72 @@ func (rc *RequiredCapability) checkServerCap() (error, error, int) {
return nil, nil, http.StatusOK
}
+// EnsureTopologyBasedRequiredCapabilities ensures that at least one server per cachegroup
+// in this delivery service's topology has this delivery service's required capabilities.
+func EnsureTopologyBasedRequiredCapabilities(tx *sql.Tx, dsID int, requiredCapabilities []string) (error, error, int) {
+ q := `
+SELECT
+ s.id,
+ c.name,
+ ARRAY_REMOVE(ARRAY_AGG(ssc.server_capability ORDER BY ssc.server_capability), NULL) AS capabilities
+FROM server s
+LEFT JOIN server_server_capability ssc ON ssc.server = s.id
+JOIN cachegroup c ON c.id = s.cachegroup
+JOIN topology_cachegroup tc ON tc.cachegroup = c.name
+JOIN deliveryservice d ON d.topology = tc.topology
+WHERE
+ d.id = $1
+ AND s.cdn_id = (SELECT cdn_id FROM deliveryservice WHERE id = $1)
Review comment:
Sure, I can add a test for that.
----------------------------------------------------------------
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 #5093: Validate addition of DS required capabilities w.r.t its topology
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5093:
URL: https://github.com/apache/trafficcontrol/pull/5093#discussion_r499919461
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_required_capabilities.go
##########
@@ -329,6 +340,72 @@ func (rc *RequiredCapability) checkServerCap() (error, error, int) {
return nil, nil, http.StatusOK
}
+// EnsureTopologyBasedRequiredCapabilities ensures that at least one server per cachegroup
+// in this delivery service's topology has this delivery service's required capabilities.
+func EnsureTopologyBasedRequiredCapabilities(tx *sql.Tx, dsID int, requiredCapabilities []string) (error, error, int) {
+ q := `
+SELECT
+ s.id,
+ c.name,
+ ARRAY_REMOVE(ARRAY_AGG(ssc.server_capability ORDER BY ssc.server_capability), NULL) AS capabilities
+FROM server s
+LEFT JOIN server_server_capability ssc ON ssc.server = s.id
+JOIN cachegroup c ON c.id = s.cachegroup
+JOIN topology_cachegroup tc ON tc.cachegroup = c.name
+JOIN deliveryservice d ON d.topology = tc.topology
+WHERE
+ d.id = $1
+ AND s.cdn_id = (SELECT cdn_id FROM deliveryservice WHERE id = $1)
Review comment:
added in https://github.com/apache/trafficcontrol/pull/5093/commits/a108472cfd0db10b0d135459347d62ec484acdf5
----------------------------------------------------------------
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