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/19 22:49:16 UTC
[GitHub] [trafficcontrol] rawlinp opened a new pull request #4702: Prohibit assigning topology-based delivery services to caches
rawlinp opened a new pull request #4702:
URL: https://github.com/apache/trafficcontrol/pull/4702
**NOTE:** this PR depends on https://github.com/apache/trafficcontrol/pull/4692, which should be merged first. The commit that belongs to this PR is https://github.com/apache/trafficcontrol/commit/d2ca60750f5c0e83fd854c9cbadafa23f5451e09, so only that commit should be reviewed.
## What does this PR (Pull Request) do?
Update the following endpoints to prevent assigning caches to delivery services that have a topology assigned:
```
POST servers/{id}/deliveryservices
POST deliveryservices/{xml_id}/servers
POST deliveryserviceserver
POST cachegroups/{id}/deliveryservices
```
- [x] This PR is related to #4572 but does not complete it.
## Which Traffic Control components are affected by this PR?
- Traffic Control Client (Go)
- Traffic Ops
## What is the best way to verify this PR?
Run the unit tests. Run the TO API tests (negative tests were added for all 4 routes). Verify they all pass.
## The following criteria are ALL met by this PR
- [x] This PR includes tests
- [x] New validation, no docs changes 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 commented on a change in pull request #4702: Prohibit assigning topology-based delivery services to caches
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4702:
URL: https://github.com/apache/trafficcontrol/pull/4702#discussion_r432172465
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -37,6 +39,52 @@ func TestDeliveryServiceServersWithRequiredCapabilities(t *testing.T) {
})
}
+func AssignServersToTopologyBasedDeliveryService(t *testing.T) {
+ ds, _, err := TOSession.GetDeliveryServiceByXMLIDNullable("ds-top")
+ if err != nil {
+ t.Fatalf("cannot GET delivery service 'ds-top': %s", err.Error())
+ }
+ if len(ds) != 1 {
+ t.Fatalf("expected one delivery service: 'ds-top', actual: %v", ds)
+ }
+ if ds[0].Topology == nil {
+ t.Fatal("expected delivery service: 'ds-top' to have a non-nil Topology, actual: nil")
+ }
+ serversResp, _, err := TOSession.GetServers()
+ servers := []tc.Server{}
+ for _, s := range serversResp {
+ if s.CDNID == *ds[0].CDNID && s.Type == tc.CacheTypeEdge.String() {
+ servers = append(servers, s)
+ }
+ }
+ if len(servers) < 1 {
+ t.Fatalf("expected: at least one EDGE in cdn %s, actual: %v", *ds[0].CDNName, servers)
+ }
+ serverNames := []string{}
+ for _, s := range servers {
+ if s.CDNID == *ds[0].CDNID && s.Type == tc.CacheTypeEdge.String() {
+ serverNames = append(serverNames, s.HostName)
+ } else {
+ t.Fatalf("expected only EDGE servers in cdn '%s', actual: %v", *ds[0].CDNName, servers)
+ }
+ }
+ _, reqInf, err := TOSession.AssignServersToDeliveryService(serverNames, "ds-top")
+ if err == nil {
+ t.Fatal("assigning servers to topology-based delivery service - expected: error, actual: nil error")
+ }
+ if reqInf.StatusCode < http.StatusBadRequest || reqInf.StatusCode >= http.StatusInternalServerError {
+ t.Fatalf("assigning servers to topology-based delivery service - expected: 400-level status code, actual: %d", reqInf.StatusCode)
+ }
+
+ _, reqInf, err = TOSession.CreateDeliveryServiceServers(*ds[0].ID, []int{servers[0].ID}, false)
+ if err == nil {
+ t.Fatal("creating delliveryserviceserver assignment for topology-based delivery service - expected: error, actual: nil error")
Review comment:
Too many Ls
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -37,6 +39,52 @@ func TestDeliveryServiceServersWithRequiredCapabilities(t *testing.T) {
})
}
+func AssignServersToTopologyBasedDeliveryService(t *testing.T) {
+ ds, _, err := TOSession.GetDeliveryServiceByXMLIDNullable("ds-top")
+ if err != nil {
+ t.Fatalf("cannot GET delivery service 'ds-top': %s", err.Error())
+ }
+ if len(ds) != 1 {
+ t.Fatalf("expected one delivery service: 'ds-top', actual: %v", ds)
+ }
+ if ds[0].Topology == nil {
+ t.Fatal("expected delivery service: 'ds-top' to have a non-nil Topology, actual: nil")
+ }
+ serversResp, _, err := TOSession.GetServers()
+ servers := []tc.Server{}
+ for _, s := range serversResp {
+ if s.CDNID == *ds[0].CDNID && s.Type == tc.CacheTypeEdge.String() {
+ servers = append(servers, s)
+ }
+ }
+ if len(servers) < 1 {
+ t.Fatalf("expected: at least one EDGE in cdn %s, actual: %v", *ds[0].CDNName, servers)
+ }
+ serverNames := []string{}
+ for _, s := range servers {
+ if s.CDNID == *ds[0].CDNID && s.Type == tc.CacheTypeEdge.String() {
+ serverNames = append(serverNames, s.HostName)
+ } else {
+ t.Fatalf("expected only EDGE servers in cdn '%s', actual: %v", *ds[0].CDNName, servers)
+ }
+ }
+ _, reqInf, err := TOSession.AssignServersToDeliveryService(serverNames, "ds-top")
+ if err == nil {
+ t.Fatal("assigning servers to topology-based delivery service - expected: error, actual: nil error")
+ }
+ if reqInf.StatusCode < http.StatusBadRequest || reqInf.StatusCode >= http.StatusInternalServerError {
+ t.Fatalf("assigning servers to topology-based delivery service - expected: 400-level status code, actual: %d", reqInf.StatusCode)
+ }
+
+ _, reqInf, err = TOSession.CreateDeliveryServiceServers(*ds[0].ID, []int{servers[0].ID}, false)
+ if err == nil {
+ t.Fatal("creating delliveryserviceserver assignment for topology-based delivery service - expected: error, actual: nil error")
+ }
+ if reqInf.StatusCode < http.StatusBadRequest || reqInf.StatusCode >= http.StatusInternalServerError {
+ t.Fatalf("creating delliveryserviceserver assignment for topology-based delivery service - expected: 400-level status code, actual: %d", reqInf.StatusCode)
Review comment:
Ls
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -677,6 +737,34 @@ func GetCacheGroupNameFromID(tx *sql.Tx, id int64) (tc.CacheGroupName, bool, err
return tc.CacheGroupName(name), true, nil
}
+// GetDeliveryServicesWithTopologies returns a list containing the delivery services in the given dsIDs
+// list that have a topology assigned. An error indicates unexpected errors that occurred when querying.
+func GetDeliveryServicesWithTopologies(tx *sql.Tx, dsIDs []int) ([]int, error) {
+ q := `
+SELECT
+ id
+FROM
+ deliveryservice
+WHERE
+ id = ANY($1::bigint[])
+ AND topology IS NOT NULL
+`
+ rows, err := tx.Query(q, pq.Array(dsIDs))
+ if err != nil {
+ return nil, errors.New("querying deliveryservice topologies: " + err.Error())
+ }
+ defer rows.Close()
Review comment:
Consider deferring log.Close() to handle the potential error
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -598,6 +598,66 @@ func GetServerNameFromID(tx *sql.Tx, id int) (string, bool, error) {
return name, true, nil
}
+type ServerHostNameAndType struct {
+ HostName string
+ Type string
+}
+
+func GetServerHostNamesAndTypesFromIDs(tx *sql.Tx, ids []int) ([]ServerHostNameAndType, error) {
+ qry := `
+SELECT
+ s.host_name,
+ t.name
+FROM
+ server s JOIN type t ON s.type = t.id
+WHERE
+ s.id = ANY($1)
+`
+ rows, err := tx.Query(qry, pq.Array(ids))
+ if err != nil {
+ return nil, errors.New("querying server host names and types: " + err.Error())
+ }
+ defer rows.Close()
+
+ servers := []ServerHostNameAndType{}
+ for rows.Next() {
+ s := ServerHostNameAndType{}
+ if err := rows.Scan(&s.HostName, &s.Type); err != nil {
+ return nil, errors.New("scanning server host name and type: " + err.Error())
+ }
+ servers = append(servers, s)
+ }
+ return servers, nil
+}
+
+// GetServerTypesFromHostNames returns the host names and types of the given server host names or an error if any occur.
+func GetServerTypesFromHostNames(tx *sql.Tx, hostNames []string) ([]ServerHostNameAndType, error) {
+ qry := `
+SELECT
+ s.host_name,
+ t.name
+FROM
+ server s JOIN type t ON s.type = t.id
+WHERE
+ s.host_name = ANY($1)
+`
+ rows, err := tx.Query(qry, pq.Array(hostNames))
+ if err != nil {
+ return nil, errors.New("querying server host names and types: " + err.Error())
+ }
+ defer rows.Close()
Review comment:
Consider deferring log.Close() to handle the potential error
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -598,6 +598,66 @@ func GetServerNameFromID(tx *sql.Tx, id int) (string, bool, error) {
return name, true, nil
}
+type ServerHostNameAndType struct {
+ HostName string
+ Type string
+}
+
+func GetServerHostNamesAndTypesFromIDs(tx *sql.Tx, ids []int) ([]ServerHostNameAndType, error) {
+ qry := `
+SELECT
+ s.host_name,
+ t.name
+FROM
+ server s JOIN type t ON s.type = t.id
+WHERE
+ s.id = ANY($1)
+`
+ rows, err := tx.Query(qry, pq.Array(ids))
+ if err != nil {
+ return nil, errors.New("querying server host names and types: " + err.Error())
+ }
+ defer rows.Close()
Review comment:
Consider deferring log.Close() to handle the potential error
----------------------------------------------------------------
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] dneuman64 merged pull request #4702: Prohibit assigning topology-based delivery services to caches
Posted by GitBox <gi...@apache.org>.
dneuman64 merged pull request #4702:
URL: https://github.com/apache/trafficcontrol/pull/4702
----------------------------------------------------------------
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 #4702: Prohibit assigning topology-based delivery services to caches
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4702:
URL: https://github.com/apache/trafficcontrol/pull/4702#discussion_r432774331
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -598,6 +598,66 @@ func GetServerNameFromID(tx *sql.Tx, id int) (string, bool, error) {
return name, true, nil
}
+type ServerHostNameAndType struct {
+ HostName string
+ Type string
+}
+
+func GetServerHostNamesAndTypesFromIDs(tx *sql.Tx, ids []int) ([]ServerHostNameAndType, error) {
+ qry := `
+SELECT
+ s.host_name,
+ t.name
+FROM
+ server s JOIN type t ON s.type = t.id
+WHERE
+ s.id = ANY($1)
+`
+ rows, err := tx.Query(qry, pq.Array(ids))
+ if err != nil {
+ return nil, errors.New("querying server host names and types: " + err.Error())
+ }
+ defer rows.Close()
Review comment:
fixed in bab0027b5999970f74cf85e4abe15e7398f3392f
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -677,6 +737,34 @@ func GetCacheGroupNameFromID(tx *sql.Tx, id int64) (tc.CacheGroupName, bool, err
return tc.CacheGroupName(name), true, nil
}
+// GetDeliveryServicesWithTopologies returns a list containing the delivery services in the given dsIDs
+// list that have a topology assigned. An error indicates unexpected errors that occurred when querying.
+func GetDeliveryServicesWithTopologies(tx *sql.Tx, dsIDs []int) ([]int, error) {
+ q := `
+SELECT
+ id
+FROM
+ deliveryservice
+WHERE
+ id = ANY($1::bigint[])
+ AND topology IS NOT NULL
+`
+ rows, err := tx.Query(q, pq.Array(dsIDs))
+ if err != nil {
+ return nil, errors.New("querying deliveryservice topologies: " + err.Error())
+ }
+ defer rows.Close()
Review comment:
fixed in bab0027b5999970f74cf85e4abe15e7398f3392f
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -37,6 +39,52 @@ func TestDeliveryServiceServersWithRequiredCapabilities(t *testing.T) {
})
}
+func AssignServersToTopologyBasedDeliveryService(t *testing.T) {
+ ds, _, err := TOSession.GetDeliveryServiceByXMLIDNullable("ds-top")
+ if err != nil {
+ t.Fatalf("cannot GET delivery service 'ds-top': %s", err.Error())
+ }
+ if len(ds) != 1 {
+ t.Fatalf("expected one delivery service: 'ds-top', actual: %v", ds)
+ }
+ if ds[0].Topology == nil {
+ t.Fatal("expected delivery service: 'ds-top' to have a non-nil Topology, actual: nil")
+ }
+ serversResp, _, err := TOSession.GetServers()
+ servers := []tc.Server{}
+ for _, s := range serversResp {
+ if s.CDNID == *ds[0].CDNID && s.Type == tc.CacheTypeEdge.String() {
+ servers = append(servers, s)
+ }
+ }
+ if len(servers) < 1 {
+ t.Fatalf("expected: at least one EDGE in cdn %s, actual: %v", *ds[0].CDNName, servers)
+ }
+ serverNames := []string{}
+ for _, s := range servers {
+ if s.CDNID == *ds[0].CDNID && s.Type == tc.CacheTypeEdge.String() {
+ serverNames = append(serverNames, s.HostName)
+ } else {
+ t.Fatalf("expected only EDGE servers in cdn '%s', actual: %v", *ds[0].CDNName, servers)
+ }
+ }
+ _, reqInf, err := TOSession.AssignServersToDeliveryService(serverNames, "ds-top")
+ if err == nil {
+ t.Fatal("assigning servers to topology-based delivery service - expected: error, actual: nil error")
+ }
+ if reqInf.StatusCode < http.StatusBadRequest || reqInf.StatusCode >= http.StatusInternalServerError {
+ t.Fatalf("assigning servers to topology-based delivery service - expected: 400-level status code, actual: %d", reqInf.StatusCode)
+ }
+
+ _, reqInf, err = TOSession.CreateDeliveryServiceServers(*ds[0].ID, []int{servers[0].ID}, false)
+ if err == nil {
+ t.Fatal("creating delliveryserviceserver assignment for topology-based delivery service - expected: error, actual: nil error")
Review comment:
fixed in bab0027b5999970f74cf85e4abe15e7398f3392f
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -37,6 +39,52 @@ func TestDeliveryServiceServersWithRequiredCapabilities(t *testing.T) {
})
}
+func AssignServersToTopologyBasedDeliveryService(t *testing.T) {
+ ds, _, err := TOSession.GetDeliveryServiceByXMLIDNullable("ds-top")
+ if err != nil {
+ t.Fatalf("cannot GET delivery service 'ds-top': %s", err.Error())
+ }
+ if len(ds) != 1 {
+ t.Fatalf("expected one delivery service: 'ds-top', actual: %v", ds)
+ }
+ if ds[0].Topology == nil {
+ t.Fatal("expected delivery service: 'ds-top' to have a non-nil Topology, actual: nil")
+ }
+ serversResp, _, err := TOSession.GetServers()
+ servers := []tc.Server{}
+ for _, s := range serversResp {
+ if s.CDNID == *ds[0].CDNID && s.Type == tc.CacheTypeEdge.String() {
+ servers = append(servers, s)
+ }
+ }
+ if len(servers) < 1 {
+ t.Fatalf("expected: at least one EDGE in cdn %s, actual: %v", *ds[0].CDNName, servers)
+ }
+ serverNames := []string{}
+ for _, s := range servers {
+ if s.CDNID == *ds[0].CDNID && s.Type == tc.CacheTypeEdge.String() {
+ serverNames = append(serverNames, s.HostName)
+ } else {
+ t.Fatalf("expected only EDGE servers in cdn '%s', actual: %v", *ds[0].CDNName, servers)
+ }
+ }
+ _, reqInf, err := TOSession.AssignServersToDeliveryService(serverNames, "ds-top")
+ if err == nil {
+ t.Fatal("assigning servers to topology-based delivery service - expected: error, actual: nil error")
+ }
+ if reqInf.StatusCode < http.StatusBadRequest || reqInf.StatusCode >= http.StatusInternalServerError {
+ t.Fatalf("assigning servers to topology-based delivery service - expected: 400-level status code, actual: %d", reqInf.StatusCode)
+ }
+
+ _, reqInf, err = TOSession.CreateDeliveryServiceServers(*ds[0].ID, []int{servers[0].ID}, false)
+ if err == nil {
+ t.Fatal("creating delliveryserviceserver assignment for topology-based delivery service - expected: error, actual: nil error")
+ }
+ if reqInf.StatusCode < http.StatusBadRequest || reqInf.StatusCode >= http.StatusInternalServerError {
+ t.Fatalf("creating delliveryserviceserver assignment for topology-based delivery service - expected: 400-level status code, actual: %d", reqInf.StatusCode)
Review comment:
fixed in bab0027b5999970f74cf85e4abe15e7398f3392f
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -598,6 +598,66 @@ func GetServerNameFromID(tx *sql.Tx, id int) (string, bool, error) {
return name, true, nil
}
+type ServerHostNameAndType struct {
+ HostName string
+ Type string
+}
+
+func GetServerHostNamesAndTypesFromIDs(tx *sql.Tx, ids []int) ([]ServerHostNameAndType, error) {
+ qry := `
+SELECT
+ s.host_name,
+ t.name
+FROM
+ server s JOIN type t ON s.type = t.id
+WHERE
+ s.id = ANY($1)
+`
+ rows, err := tx.Query(qry, pq.Array(ids))
+ if err != nil {
+ return nil, errors.New("querying server host names and types: " + err.Error())
+ }
+ defer rows.Close()
+
+ servers := []ServerHostNameAndType{}
+ for rows.Next() {
+ s := ServerHostNameAndType{}
+ if err := rows.Scan(&s.HostName, &s.Type); err != nil {
+ return nil, errors.New("scanning server host name and type: " + err.Error())
+ }
+ servers = append(servers, s)
+ }
+ return servers, nil
+}
+
+// GetServerTypesFromHostNames returns the host names and types of the given server host names or an error if any occur.
+func GetServerTypesFromHostNames(tx *sql.Tx, hostNames []string) ([]ServerHostNameAndType, error) {
+ qry := `
+SELECT
+ s.host_name,
+ t.name
+FROM
+ server s JOIN type t ON s.type = t.id
+WHERE
+ s.host_name = ANY($1)
+`
+ rows, err := tx.Query(qry, pq.Array(hostNames))
+ if err != nil {
+ return nil, errors.New("querying server host names and types: " + err.Error())
+ }
+ defer rows.Close()
Review comment:
fixed in bab0027b5999970f74cf85e4abe15e7398f3392f
----------------------------------------------------------------
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