This is an automated email from the ASF dual-hosted git repository.
zrhoffman pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficcontrol.git
The following commit(s) were added to refs/heads/master by this push:
new d69f2a9 Take topology-based DSes into account when checking DSS
assignments (#6408)
d69f2a9 is described below
commit d69f2a99ea982224b0830f19e9335267f9244336
Author: Rawlin Peters <[email protected]>
AuthorDate: Mon Dec 6 16:29:47 2021 -0700
Take topology-based DSes into account when checking DSS assignments (#6408)
* Take topology-based DSes into account when checking DSS assignments
In order to prevent accidental ouages, TO currently has validation which
prevents things like deleting the last server that is assigned to an
active delivery service. However, this validation ignored the fact that
delivery services can now be assigned to a topology instead of using DSS
assignments. This prevents a number of actually valid things, such as
safely setting caches to ADMIN_DOWN, assigning MSO servers to
topology-based DSes, and removing legacy DSS EDGE assignments from
topology-based DSes. This fixes that validation in order to take
topology-based DSes into account.
Closes: #6392
* Fix GROUP BY
* Add TO API test
---
CHANGELOG.md | 1 +
.../testing/api/v4/serverupdatestatus_test.go | 57 ++++++++++++++++++++++
.../deliveryservice/servers/delete.go | 7 +--
.../deliveryservice/servers/servers.go | 30 ++++++------
traffic_ops/traffic_ops_golang/server/servers.go | 11 +++--
.../server/servers_assignment.go | 11 +++--
6 files changed, 90 insertions(+), 27 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 846f16b..4abcb1d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -31,6 +31,7 @@ The format is based on [Keep a
Changelog](http://keepachangelog.com/en/1.0.0/).
- [#6255](https://github.com/apache/trafficcontrol/issues/6255) - Unreadable
Prod Mode CDN Notifications in Traffic Portal
- Fixed broken `GET /cdns/routing` Traffic Ops API
- [#6259](https://github.com/apache/trafficcontrol/issues/6259) - Traffic
Portal No Longer Allows Spaces in Server Object "Router Port Name"
+- [#6392](https://github.com/apache/trafficcontrol/issues/6392) - Traffic Ops
prevents assigning ORG servers to topology-based delivery services (as well as
a number of other valid operations being prohibited by "last server assigned to
DS" validations which don't apply to topology-based delivery services)
- [#6175](https://github.com/apache/trafficcontrol/issues/6175) - POST request
to /api/4.0/phys_locations accepts mismatch values for regionName.
- [#6285](https://github.com/apache/trafficcontrol/issues/6285) - The Traffic
Ops Postinstall script will work in CentOS 7, even if Python 3 is installed
- [#5373](https://github.com/apache/trafficcontrol/issues/5373) - Traffic
Monitor logs not consistent
diff --git a/traffic_ops/testing/api/v4/serverupdatestatus_test.go
b/traffic_ops/testing/api/v4/serverupdatestatus_test.go
index 98f1d26..eac7ebb 100644
--- a/traffic_ops/testing/api/v4/serverupdatestatus_test.go
+++ b/traffic_ops/testing/api/v4/serverupdatestatus_test.go
@@ -28,6 +28,63 @@ import (
client "github.com/apache/trafficcontrol/traffic_ops/v4-client"
)
+func TestServerUpdateStatusLastAssigned(t *testing.T) {
+ WithObjs(t, []TCObj{CDNs, Types, Tenants, Parameters, Profiles,
Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers,
ServiceCategories, Topologies, DeliveryServices}, func() {
+ opts := client.NewRequestOptions()
+ opts.QueryParameters.Set("hostName", "atlanta-edge-01")
+ resp, _, err := TOSession.GetServers(opts)
+ if err != nil {
+ t.Fatalf("cannot get server by hostname: %v", err)
+ }
+ if len(resp.Response) != 1 {
+ t.Fatalf("Expected a server named 'atlanta-edge-01' to
exist")
+ }
+ edge := resp.Response[0]
+ opts = client.NewRequestOptions()
+ opts.QueryParameters.Set("xmlId", "ds-top")
+ dsResp, _, err := TOSession.GetDeliveryServices(opts)
+ if err != nil {
+ t.Fatalf("cannot get delivery service by xmlId: %v",
err)
+ }
+ if len(resp.Response) != 1 {
+ t.Fatalf("Expected one delivery service with xmlId
'ds-top' to exist")
+ }
+ // temporarily unassign the topology in order to assign an EDGE
+ ds := dsResp.Response[0]
+ tmpTop := *ds.Topology
+ ds.Topology = nil
+ ds.FirstHeaderRewrite = nil
+ ds.LastHeaderRewrite = nil
+ ds.InnerHeaderRewrite = nil
+ _, _, err = TOSession.UpdateDeliveryService(*ds.ID, ds,
client.RequestOptions{})
+ if err != nil {
+ t.Fatalf("cannot update delivery service 'ds-top': %v",
err)
+ }
+ _, _, err = TOSession.CreateDeliveryServiceServers(*ds.ID,
[]int{*edge.ID}, true, client.RequestOptions{})
+ if err != nil {
+ t.Fatalf("cannot create delivery service server: %v",
err)
+ }
+ // reassign the topology
+ ds.Topology = &tmpTop
+ _, _, err = TOSession.UpdateDeliveryService(*ds.ID, ds,
client.RequestOptions{})
+ if err != nil {
+ t.Fatalf("cannot update delivery service 'ds-top': %v",
err)
+ }
+ // attempt to set the edge to OFFLINE
+ _, _, err = TOSession.UpdateServerStatus(*edge.ID,
tc.ServerPutStatus{
+ Status: util.JSONNameOrIDStr{Name:
util.StrPtr("OFFLINE")},
+ OfflineReason: util.StrPtr("testing")},
client.RequestOptions{})
+ if err != nil {
+ t.Errorf("setting edge to OFFLINE when it's the only
edge assigned to a topology-based delivery service - expected: no error,
actual: %v", err)
+ }
+ // remove EDGE assignment
+ _, _, err = TOSession.CreateDeliveryServiceServers(*ds.ID,
[]int{}, true, client.RequestOptions{})
+ if err != nil {
+ t.Errorf("removing delivery service servers from
topology-based delivery service - expected: no error, actual: %v", err)
+ }
+ })
+}
+
func TestServerUpdateStatus(t *testing.T) {
WithObjs(t, []TCObj{CDNs, Types, Parameters, Profiles, Statuses,
Divisions, Regions, PhysLocations, CacheGroups, Servers}, func() {
//TODO: DON'T hard-code server hostnames!
diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go
b/traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go
index c9857f7..b4fa6a5 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go
+++ b/traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go
@@ -77,7 +77,7 @@ SELECT (
// back to the user, and an error that must not be shown to the user. If the
server is, in fact,
// the last available edge or origin assigned to the Delivery Service, the
"user error" will be
// set to an appropriate, non-nil value.
-func checkLastAvailableEdgeOrOrigin(dsID, serverID int, usesMSO bool, tx
*sql.Tx) (int, error, error) {
+func checkLastAvailableEdgeOrOrigin(dsID, serverID int, usesMSO, hasTopology
bool, tx *sql.Tx) (int, error, error) {
var isLastEdge bool
var isLastOrigin bool
if tx == nil {
@@ -87,7 +87,7 @@ func checkLastAvailableEdgeOrOrigin(dsID, serverID int,
usesMSO bool, tx *sql.Tx
if err != nil {
return http.StatusInternalServerError, nil,
fmt.Errorf("checking if server #%d is the last one assigned to DS #%d: %v",
serverID, dsID, err)
}
- if isLastEdge {
+ if isLastEdge && !hasTopology {
return http.StatusConflict, fmt.Errorf("removing server #%d
from active Delivery Service #%d would leave it with no REPORTED/ONLINE EDGE
servers", serverID, dsID), nil
}
if usesMSO && isLastOrigin {
@@ -151,7 +151,8 @@ func deleteDSS(w http.ResponseWriter, r *http.Request) {
if *ds.Active {
usesMSO := ds.MultiSiteOrigin == nil || *ds.MultiSiteOrigin
- errCode, userErr, sysErr = checkLastAvailableEdgeOrOrigin(dsID,
serverID, usesMSO, tx)
+ hasTopology := ds.Topology != nil
+ errCode, userErr, sysErr = checkLastAvailableEdgeOrOrigin(dsID,
serverID, usesMSO, hasTopology, tx)
if userErr != nil || sysErr != nil {
api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
return
diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
b/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
index e5bdc88..4be3c76 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
+++ b/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
@@ -564,26 +564,28 @@ func validateDSSAssignments(tx *sql.Tx, ds DSInfo,
serverInfos []tc.ServerInfo,
}
}
}
- // Prevent the user from deleting all the servers in an active
DS
- if len(ids) == 0 {
+ // Prevent the user from deleting all the servers in an active,
non-topology-based DS
+ if len(ids) == 0 && ds.Topology == nil {
return fmt.Errorf("this server assignment leaves Active
Delivery Service #%d without any '%s' or '%s' servers", ds.ID,
tc.CacheStatusOnline, tc.CacheStatusReported), nil, http.StatusConflict
}
// prevent the user from deleting all ORG servers from an
active, MSO-enabled DS
if ds.UseMultiSiteOrigin && newOrgCount < 1 {
return fmt.Errorf("this server assignment leaves
Active, MSO-enabled Delivery Service #%d without any '%s' or '%s' %s servers",
ds.ID, tc.CacheStatusOnline, tc.CacheStatusReported, tc.OriginTypeName), nil,
http.StatusConflict
}
- // The following check is necessary because of the following:
- // Consider a brand new active DS that has no server
assignments.
- // Now, you wish to assign an online/ reported ORG server to it.
- // Since this is a new DS and it didnt have any "pre existing"
online/ reported EDGEs, this should be possible.
- // However, if that DS had a couple of online/ reported EDGEs
assigned to it, and now if you wanted to "replace"
- // that assignment with the new assignment of an online/
reported ORG, this should be prohibited by TO.
- currentlyHasAvailableEdgesAssigned, err :=
hasAvailableEdgesCurrentlyAssigned(tx, ds.ID)
- if err != nil {
- return nil, fmt.Errorf("checking for pre existing
ONLINE/ REPORTED EDGES: %v", err), http.StatusInternalServerError
- }
- if (currentlyHasAvailableEdgesAssigned && newAvailableEdgeCount
< 1) || !anyAvailableServers {
- return fmt.Errorf("this server assignment leaves Active
Delivery Service #%d without any '%s' or '%s' servers", ds.ID,
tc.CacheStatusOnline, tc.CacheStatusReported), nil, http.StatusConflict
+ if ds.Topology == nil {
+ // The following check is necessary because of the
following:
+ // Consider a brand new active DS that has no server
assignments.
+ // Now, you wish to assign an online/ reported ORG
server to it.
+ // Since this is a new DS and it didnt have any "pre
existing" online/ reported EDGEs, this should be possible.
+ // However, if that DS had a couple of online/ reported
EDGEs assigned to it, and now if you wanted to "replace"
+ // that assignment with the new assignment of an
online/ reported ORG, this should be prohibited by TO.
+ currentlyHasAvailableEdgesAssigned, err :=
hasAvailableEdgesCurrentlyAssigned(tx, ds.ID)
+ if err != nil {
+ return nil, fmt.Errorf("checking for pre
existing ONLINE/ REPORTED EDGES: %v", err), http.StatusInternalServerError
+ }
+ if (currentlyHasAvailableEdgesAssigned &&
newAvailableEdgeCount < 1) || !anyAvailableServers {
+ return fmt.Errorf("this server assignment
leaves Active Delivery Service #%d without any '%s' or '%s' servers", ds.ID,
tc.CacheStatusOnline, tc.CacheStatusReported), nil, http.StatusConflict
+ }
}
}
diff --git a/traffic_ops/traffic_ops_golang/server/servers.go
b/traffic_ops/traffic_ops_golang/server/servers.go
index e87c0fe..8ceb3b5 100644
--- a/traffic_ops/traffic_ops_golang/server/servers.go
+++ b/traffic_ops/traffic_ops_golang/server/servers.go
@@ -1908,13 +1908,13 @@ func Create(w http.ResponseWriter, r *http.Request) {
}
const lastServerTypeOfDSesQuery = `
-SELECT ds.id, ds.multi_site_origin
+SELECT ds.id, ds.multi_site_origin, ds.topology
FROM deliveryservice_server dss
JOIN server s ON dss.server = s.id
JOIN type t ON s.type = t.id
JOIN deliveryservice ds ON dss.deliveryservice = ds.id
WHERE t.name LIKE $1 AND ds.active
-GROUP BY ds.id, ds.multi_site_origin
+GROUP BY ds.id, ds.multi_site_origin, ds.topology
HAVING COUNT(dss.server) = 1 AND $2 = ANY(ARRAY_AGG(dss.server));
`
@@ -1941,16 +1941,17 @@ func
getActiveDeliveryServicesThatOnlyHaveThisServerAssigned(id int, serverType
if err != nil {
return ids, fmt.Errorf("querying: %v", err)
}
- defer rows.Close()
+ defer log.Close(rows, "closing rows from
getActiveDeliveryServicesThatOnlyHaveThisServerAssigned")
for rows.Next() {
var dsID int
var mso bool
- err = rows.Scan(&dsID, &mso)
+ var topology *string
+ err = rows.Scan(&dsID, &mso, &topology)
if err != nil {
return ids, fmt.Errorf("scanning: %v", err)
}
- if isEdge || (isOrigin && mso) {
+ if (isEdge && topology == nil) || (isOrigin && mso) {
ids = append(ids, dsID)
}
}
diff --git a/traffic_ops/traffic_ops_golang/server/servers_assignment.go
b/traffic_ops/traffic_ops_golang/server/servers_assignment.go
index 33e53f2..d9f0d87 100644
--- a/traffic_ops/traffic_ops_golang/server/servers_assignment.go
+++ b/traffic_ops/traffic_ops_golang/server/servers_assignment.go
@@ -63,7 +63,7 @@ func getConfigFile(prefix string, xmlId string) string {
}
const lastServerInActiveDeliveryServicesQuery = `
-SELECT d.id, d.multi_site_origin
+SELECT d.id, d.multi_site_origin, d.topology
FROM deliveryservice d
INNER JOIN deliveryservice_server dss ON dss.deliveryservice = d.id
INNER JOIN server s ON s.id = dss.server
@@ -79,7 +79,7 @@ WHERE d.id IN (
AND NOT (dss.deliveryservice = ANY($2::BIGINT[]))
AND (st.name = '` + string(tc.CacheStatusOnline) + `' OR st.name = '` +
string(tc.CacheStatusReported) + `')
AND t.name LIKE $3
-GROUP BY d.id, d.multi_site_origin
+GROUP BY d.id, d.multi_site_origin, d.topology
HAVING COUNT(dss.server) = 1
`
@@ -100,15 +100,16 @@ func checkForLastServerInActiveDeliveryServices(serverID
int, serverType string,
if err != nil {
return violations, fmt.Errorf("querying: %v", err)
}
- defer rows.Close()
+ defer log.Close(rows, "closing rows in
checkForLastServerInActiveDeliveryServices")
for rows.Next() {
var violation int
var mso bool
- if err = rows.Scan(&violation, &mso); err != nil {
+ var topology *string
+ if err = rows.Scan(&violation, &mso, &topology); err != nil {
return violations, fmt.Errorf("scanning: %v", err)
}
- if isEdge || (isOrigin && mso) {
+ if (isEdge && topology == nil) || (isOrigin && mso) {
violations = append(violations, violation)
}
}
