You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficcontrol.apache.org by mi...@apache.org on 2021/01/20 23:13:57 UTC
[trafficcontrol] branch master updated: Dont show changelog msg for
primary origin if it wasn't updated (#5410)
This is an automated email from the ASF dual-hosted git repository.
mitchell852 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 9930731 Dont show changelog msg for primary origin if it wasn't updated (#5410)
9930731 is described below
commit 993073146197cf0a6523816c134ee86e9309b8f9
Author: Srijeet Chatterjee <30...@users.noreply.github.com>
AuthorDate: Wed Jan 20 16:12:56 2021 -0700
Dont show changelog msg for primary origin if it wasn't updated (#5410)
* Dont show changelog msg for primary origin if it wasn't updated
---
CHANGELOG.md | 2 +
.../deliveryservice/deliveryservices.go | 54 +++++++++++++---------
.../deliveryservice/deliveryservices_test.go | 45 +++++++++++++++++-
3 files changed, 76 insertions(+), 25 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4fcbb71..99b59a5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,6 +17,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
- Traffic Ops: Added validation to ensure that the `weight` parameter of `parent.config` is a float
### Fixed
+- [#5335](https://github.com/apache/trafficcontrol/issues/5335) - Don't create a change log entry if the delivery service primary origin hasn't changed
+- [#5333](https://github.com/apache/trafficcontrol/issues/5333) - Don't create a change log entry for any delivery service consistent hash query params updates
- [#5341](https://github.com/apache/trafficcontrol/issues/5341) - For a DS with existing SSLKeys, fixed HTTP status code from 403 to 400 when updating CDN and Routing Name (in TO) and made CDN and Routing Name fields immutable (in TP).
- [#5192](https://github.com/apache/trafficcontrol/issues/5192) - Fixed TO log warnings when generating snapshots for topology-based delivery services.
- [#5284](https://github.com/apache/trafficcontrol/issues/5284) - Fixed error message when creating a server with non-existent profile
diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
index 454df88..d47899e 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
+++ b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
@@ -49,6 +49,13 @@ type TODeliveryService struct {
tc.DeliveryServiceNullableV30
}
+// TODeliveryServiceOldDetails is the struct to store the old details while updating a DS.
+type TODeliveryServiceOldDetails struct {
+ OldOrgServerFqdn *string
+ OldCdnName string
+ OldRoutingName string
+}
+
func (ds TODeliveryService) MarshalJSON() ([]byte, error) {
return json.Marshal(ds.DeliveryServiceNullableV30)
}
@@ -396,11 +403,9 @@ func createV31(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, dsV31 t
return nil, http.StatusInternalServerError, nil, errors.New("creating default regex: " + err.Error())
}
- if c, err := createConsistentHashQueryParams(tx, *ds.ID, ds.ConsistentHashQueryParams); err != nil {
+ if _, err := createConsistentHashQueryParams(tx, *ds.ID, ds.ConsistentHashQueryParams); err != nil {
usrErr, sysErr, code := api.ParseDBError(err)
return nil, code, usrErr, sysErr
- } else {
- api.CreateChangeLogRawTx(api.ApiChange, "DS: "+*ds.XMLID+", ID: "+strconv.Itoa(*ds.ID)+", ACTION: Created "+strconv.Itoa(c)+" consistent hash query params", user, tx)
}
matchlists, err := GetDeliveryServicesMatchLists([]string{*ds.XMLID}, tx)
@@ -844,22 +849,23 @@ func updateV31(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, dsV31 *
return nil, http.StatusInternalServerError, nil, errors.New("getting delivery service type during update: " + err.Error())
}
- oldCDNName := ""
- oldRoutingName := ""
errCode := http.StatusOK
var userErr error
var sysErr error
+ var oldDetails TODeliveryServiceOldDetails
if dsType.HasSSLKeys() {
- oldCDNName, oldRoutingName, userErr, sysErr, errCode = getOldDetails(*ds.ID, tx)
+ oldDetails, userErr, sysErr, errCode = getOldDetails(*ds.ID, tx)
if userErr != nil || sysErr != nil {
return nil, errCode, userErr, sysErr
}
- exists, err := getSSLVersion(*ds.XMLID, tx)
+ sslKeysExist, err := getSSLVersion(*ds.XMLID, tx)
if err != nil {
return nil, http.StatusInternalServerError, nil, fmt.Errorf("querying delivery service with sslKeyVersion failed: %s", err)
}
- if exists && (oldCDNName != *ds.CDNName || oldRoutingName != *ds.RoutingName) {
- return nil, http.StatusBadRequest, errors.New("delivery service has ssl keys that cannot be automatically changed, therefore CDN and routing name are immutable"), nil
+ if ds.CDNName != nil && ds.RoutingName != nil {
+ if sslKeysExist && (oldDetails.OldCdnName != *ds.CDNName || oldDetails.OldRoutingName != *ds.RoutingName) {
+ return nil, http.StatusBadRequest, errors.New("delivery service has ssl keys that cannot be automatically changed, therefore CDN and routing name are immutable"), nil
+ }
}
}
@@ -1015,8 +1021,10 @@ func updateV31(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, dsV31 *
return nil, http.StatusInternalServerError, nil, errors.New("ensuring ds parameters:: " + err.Error())
}
- if err := updatePrimaryOrigin(tx, user, ds); err != nil {
- return nil, http.StatusInternalServerError, nil, errors.New("updating delivery service: " + err.Error())
+ if oldDetails.OldOrgServerFqdn != nil && ds.OrgServerFQDN != nil && *oldDetails.OldOrgServerFqdn != *ds.OrgServerFQDN {
+ if err := updatePrimaryOrigin(tx, user, ds); err != nil {
+ return nil, http.StatusInternalServerError, nil, errors.New("updating delivery service: " + err.Error())
+ }
}
ds.LastUpdated = &lastUpdated
@@ -1029,11 +1037,9 @@ func updateV31(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, dsV31 *
api.CreateChangeLogRawTx(api.ApiChange, "DS: "+*ds.XMLID+", ID: "+strconv.Itoa(*ds.ID)+", ACTION: Deleted "+strconv.FormatInt(c, 10)+" consistent hash query params", user, tx)
}
- if c, err := createConsistentHashQueryParams(tx, *ds.ID, ds.ConsistentHashQueryParams); err != nil {
+ if _, err = createConsistentHashQueryParams(tx, *ds.ID, ds.ConsistentHashQueryParams); err != nil {
usrErr, sysErr, code := api.ParseDBError(err)
return nil, code, usrErr, sysErr
- } else {
- api.CreateChangeLogRawTx(api.ApiChange, "DS: "+*ds.XMLID+", ID: "+strconv.Itoa(*ds.ID)+", ACTION: Created "+strconv.Itoa(c)+" consistent hash query params", user, tx)
}
if err := api.CreateChangeLogRawErr(api.ApiChange, "Updated ds: "+*ds.XMLID+" id: "+strconv.Itoa(*ds.ID), user, tx); err != nil {
@@ -1172,23 +1178,25 @@ func selectMaxLastUpdatedQuery(where string) string {
select max(last_updated) as t from last_deleted l where l.table_name='deliveryservice') as res`
}
-func getOldDetails(id int, tx *sql.Tx) (string, string, error, error, int) {
+func getOldDetails(id int, tx *sql.Tx) (TODeliveryServiceOldDetails, error, error, int) {
q := `
-SELECT ds.routing_name, cdn.name
+SELECT ds.routing_name, cdn.name,
+(SELECT o.protocol::text || '://' || o.fqdn || rtrim(concat(':', o.port::text), ':')
+FROM origin o
+WHERE o.deliveryservice = ds.id
+AND o.is_primary) as org_server_fqdn
FROM deliveryservice as ds
JOIN cdn ON ds.cdn_id = cdn.id
WHERE ds.id=$1
`
- routingName := ""
- cdnName := ""
- if err := tx.QueryRow(q, id).Scan(&routingName, &cdnName); err != nil {
+ var oldDetails TODeliveryServiceOldDetails
+ if err := tx.QueryRow(q, id).Scan(&oldDetails.OldRoutingName, &oldDetails.OldCdnName, &oldDetails.OldOrgServerFqdn); err != nil {
if err == sql.ErrNoRows {
- return "", "", fmt.Errorf("querying delivery service %v host name: no such delivery service exists", id), nil, http.StatusNotFound
+ return oldDetails, fmt.Errorf("querying delivery service %v host name: no such delivery service exists", id), nil, http.StatusNotFound
}
- return "", "", nil, fmt.Errorf("querying delivery service %v host name: "+err.Error(), id), http.StatusInternalServerError
+ return oldDetails, nil, fmt.Errorf("querying delivery service %v host name: "+err.Error(), id), http.StatusInternalServerError
}
-
- return cdnName, routingName, nil, nil, http.StatusOK
+ return oldDetails, nil, nil, http.StatusOK
}
func getTypeFromID(id int, tx *sql.Tx) (tc.DSType, error) {
diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_test.go b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_test.go
index 555419b..4c83f94 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_test.go
+++ b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_test.go
@@ -31,7 +31,48 @@ import (
"gopkg.in/DATA-DOG/go-sqlmock.v1"
)
-func TestGetOldDetails(t *testing.T) {
+func TestGetDetails(t *testing.T) {
+ mockDB, mock, err := sqlmock.New()
+ if err != nil {
+ t.Fatalf("an error '%s' was not expected when opening a stub database connection", err)
+ }
+ defer mockDB.Close()
+
+ db := sqlx.NewDb(mockDB, "sqlmock")
+ defer db.Close()
+
+ rows := sqlmock.NewRows([]string{"routing_name", "name", "origin_server_fqdn"})
+ rows.AddRow("cdn", "foo", "http://123.34.32.21:9090")
+
+ rows2 := sqlmock.NewRows([]string{"ds_name", "type", "pattern", "coalesce"})
+ rows2.AddRow("testDS", "HOST_REGEXP", ".*\\.testDS\\..*", 0)
+
+ mock.ExpectBegin()
+ mock.ExpectQuery("SELECT ds.routing_name, cdn.name").WillReturnRows(rows)
+ mock.ExpectQuery("SELECT ds.xml_id as ds_name, t.name as type, r.pattern").WillReturnRows(rows2)
+
+ oldDetails, userErr, sysErr, code := getOldDetails(1, db.MustBegin().Tx)
+ if userErr != nil || sysErr != nil {
+ t.Fatalf("didn't expect an error but got user err %v, sys err %v", userErr, sysErr)
+ }
+ if code != http.StatusOK {
+ t.Fatalf("expected status OK 200, but got %d", code)
+ }
+ if oldDetails.OldOrgServerFqdn == nil {
+ t.Fatalf("old org server fqdn is nil")
+ }
+ if *oldDetails.OldOrgServerFqdn != "http://123.34.32.21:9090" {
+ t.Errorf("expected old org server fqdn to be http://123.34.32.21:9090, but got %v", *oldDetails.OldOrgServerFqdn)
+ }
+ if oldDetails.OldRoutingName != "cdn" {
+ t.Errorf("expected old routing name to be cdn, but got %v", oldDetails.OldRoutingName)
+ }
+ if oldDetails.OldCdnName != "foo" {
+ t.Errorf("expected old cdn name to be foo, but got %v", oldDetails.OldCdnName)
+ }
+}
+
+func TestGetOldDetailsError(t *testing.T) {
expected := `querying delivery service 1 host name: no such delivery service exists`
mockDB, mock, err := sqlmock.New()
if err != nil {
@@ -45,7 +86,7 @@ func TestGetOldDetails(t *testing.T) {
rows := sqlmock.NewRows([]string{""})
mock.ExpectBegin()
mock.ExpectQuery("SELECT ds.routing_name, cdn.name").WillReturnRows(rows)
- _, _, userErr, _, code := getOldDetails(1, db.MustBegin().Tx)
+ _, userErr, _, code := getOldDetails(1, db.MustBegin().Tx)
if userErr == nil {
t.Fatalf("expected error %v, but got none", expected)
}