[trafficcontrol] branch master updated: Prevents DS regexes with non-consecutive order from generating invalid CRconfig/Snapshot (#4910)

[mi...@apache.org]

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 43c098b  Prevents DS regexes with non-consecutive order from generating invalid CRconfig/Snapshot (#4910)
43c098b is described below

commit 43c098b3128e20668ba443d36ed09a33adb76f4c
Author: Srijeet Chatterjee <30...@users.noreply.github.com>
AuthorDate: Fri Aug 14 14:51:51 2020 -0600

    Prevents DS regexes with non-consecutive order from generating invalid CRconfig/Snapshot (#4910)
    
    * Making sure we have only one regex per order, and the missing orders dont show up as nulls
    
    * go fmt
    
    * Code review fixes
    
    * changelog entry
    
    * Add comment
    
    * formatting
---
 CHANGELOG.md                                       |  1 +
 .../traffic_ops_golang/crconfig/deliveryservice.go | 20 ++++---
 traffic_ops/traffic_ops_golang/crconfig/handler.go |  1 -
 .../deliveryservicesregexes.go                     | 25 ++++++++
 .../deliveryservicesregexes_test.go                | 66 ++++++++++++++++++++++
 5 files changed, 105 insertions(+), 8 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index eaf25ca..78d6d1b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -55,6 +55,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Fixed ORT atstccfg helper log to append and not overwrite old logs. Also changed to log to /var/log/ort and added a logrotate to the RPM. See the ORT README.md for details.
 - Added Delivery Service Raw Remap `__RANGE_DIRECTIVE__` directive to allow inserting the Range Directive after the Raw Remap text. This allows Raw Remaps which manipulate the Range.
 - Added an option for `coordinateRange` in the RGB configuration file, so that in case a client doesn't have a postal code, we can still determine if it should be allowed or not, based on whether or not the latitude/ longitude of the client falls within the supplied ranges. [Related github issue](https://github.com/apache/trafficcontrol/issues/4372)
+- Fixed #3548 - Prevents DS regexes with non-consecutive order from generating invalid CRconfig/snapshot.
 
 ### Changed
 - Changed some Traffic Ops Go Client methods to use `DeliveryServiceNullable` inputs and outputs.
diff --git a/traffic_ops/traffic_ops_golang/crconfig/deliveryservice.go b/traffic_ops/traffic_ops_golang/crconfig/deliveryservice.go
index 96fd277..028ee7e 100644
--- a/traffic_ops/traffic_ops_golang/crconfig/deliveryservice.go
+++ b/traffic_ops/traffic_ops_golang/crconfig/deliveryservice.go
@@ -481,6 +481,8 @@ order by dr.set_number asc
 		return nil, nil, errors.New("querying deliveryservices: " + err.Error())
 	}
 	defer rows.Close()
+	// a map to keep track of the ds name and the last order of the regex for that ds
+	dsNameOrderMap := make(map[string]int)
 
 	for rows.Next() {
 		pattern := ""
@@ -491,12 +493,14 @@ order by dr.set_number asc
 		if err := rows.Scan(&pattern, &ttype, &dstype, &setnum, &dsname); err != nil {
 			return nil, nil, errors.New("scanning deliveryservice regexes: " + err.Error())
 		}
-
+		if _, ok := dsNameOrderMap[dsname]; !ok {
+			dsNameOrderMap[dsname] = 0
+		} else {
+			dsNameOrderMap[dsname] = dsNameOrderMap[dsname] + 1
+		}
 		protocolStr := getProtocolStr(dstype)
 
-		for len(dsmatchsets[dsname]) <= setnum {
-			dsmatchsets[dsname] = append(dsmatchsets[dsname], nil) // TODO change to not insert empties? Current behavior emulates old Perl CRConfig
-		}
+		dsmatchsets[dsname] = append(dsmatchsets[dsname], nil)
 
 		matchType := ""
 		switch ttype {
@@ -511,10 +515,12 @@ order by dr.set_number asc
 			continue
 		}
 
-		if dsmatchsets[dsname][setnum] == nil {
-			dsmatchsets[dsname][setnum] = &tc.MatchSet{}
+		// If there are gaps between two or more DS regex orders, do not add these missing orders in the final list.
+		// Instead, skip over them and add the next regex with a valid order.
+		if dsmatchsets[dsname][dsNameOrderMap[dsname]] == nil {
+			dsmatchsets[dsname][dsNameOrderMap[dsname]] = &tc.MatchSet{}
 		}
-		matchset := dsmatchsets[dsname][setnum]
+		matchset := dsmatchsets[dsname][dsNameOrderMap[dsname]]
 		matchset.Protocol = protocolStr
 		matchset.MatchList = append(matchset.MatchList, tc.MatchList{MatchType: matchType, Regex: pattern})
 
diff --git a/traffic_ops/traffic_ops_golang/crconfig/handler.go b/traffic_ops/traffic_ops_golang/crconfig/handler.go
index 327edf7..21da66e 100644
--- a/traffic_ops/traffic_ops_golang/crconfig/handler.go
+++ b/traffic_ops/traffic_ops_golang/crconfig/handler.go
@@ -219,7 +219,6 @@ func snapshotHandler(w http.ResponseWriter, r *http.Request, deprecated bool) {
 		api.HandleErrOptionalDeprecation(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err, deprecated, &alt)
 		return
 	}
-
 	monitoringJSON, err := monitoring.GetMonitoringJSON(inf.Tx.Tx, cdn)
 	if err != nil {
 		api.HandleErrOptionalDeprecation(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New(r.RemoteAddr+" getting monitoring.json data: "+err.Error()), deprecated, &alt)
diff --git a/traffic_ops/traffic_ops_golang/deliveryservicesregexes/deliveryservicesregexes.go b/traffic_ops/traffic_ops_golang/deliveryservicesregexes/deliveryservicesregexes.go
index 0f4952c..75fbb65 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservicesregexes/deliveryservicesregexes.go
+++ b/traffic_ops/traffic_ops_golang/deliveryservicesregexes/deliveryservicesregexes.go
@@ -226,6 +226,11 @@ func Post(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if err := validateDSRegexOrder(tx, inf.IntParams["dsid"], dsr.SetNumber); err != nil {
+		api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, err, nil)
+		return
+	}
+
 	regexID := 0
 	if err := tx.QueryRow(`INSERT INTO regex (pattern, type) VALUES ($1, $2) RETURNING id`, dsr.Pattern, dsr.Type).Scan(&regexID); err != nil {
 		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("inserting deliveryserviceregex regex: "+err.Error()))
@@ -298,6 +303,11 @@ func Put(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if err := validateDSRegexOrder(tx, inf.IntParams["dsid"], dsr.SetNumber); err != nil {
+		api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, err, nil)
+		return
+	}
+
 	if err := validateDSRegexType(tx, dsr.Type); err != nil {
 		api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, err, nil)
 		return
@@ -332,6 +342,21 @@ func validateDSRegexType(tx *sql.Tx, typeID int) error {
 	return err
 }
 
+func validateDSRegexOrder(tx *sql.Tx, dsID int, order int) error {
+	var ds int
+	if order < 0 {
+		return errors.New("cannot add regex with order < 0")
+	}
+	err := tx.QueryRow(`
+select deliveryservice from deliveryservice_regex 
+where deliveryservice = $1 and set_number = $2`,
+		dsID, order).Scan(&ds)
+	if err == nil {
+		return errors.New("cannot add regex, another regex with the same order exists")
+	}
+	return nil
+}
+
 func Delete(w http.ResponseWriter, r *http.Request) {
 	inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"dsid", "regexid"}, []string{"dsid", "regexid"})
 	if userErr != nil || sysErr != nil {
diff --git a/traffic_ops/traffic_ops_golang/deliveryservicesregexes/deliveryservicesregexes_test.go b/traffic_ops/traffic_ops_golang/deliveryservicesregexes/deliveryservicesregexes_test.go
new file mode 100644
index 0000000..2a9bf5e
--- /dev/null
+++ b/traffic_ops/traffic_ops_golang/deliveryservicesregexes/deliveryservicesregexes_test.go
@@ -0,0 +1,66 @@
+package deliveryservicesregexes
+
+import (
+	"github.com/jmoiron/sqlx"
+	"gopkg.in/DATA-DOG/go-sqlmock.v1"
+	"testing"
+)
+
+/*
+ * 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 TestValidateDSRegexOrder(t *testing.T) {
+	expected := `cannot add regex, another regex with the same order exists`
+	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()
+	cols := []string{"deliveryservice"}
+	rows := sqlmock.NewRows(cols)
+	rows = rows.AddRow(
+		1,
+	)
+	mock.ExpectBegin()
+	mock.ExpectQuery("select").WithArgs(1, 3).WillReturnRows(rows)
+	tx := db.MustBegin().Tx
+	err = validateDSRegexOrder(tx, 1, 3)
+	if err == nil {
+		t.Fatal("Expected error but got nil")
+	}
+	if err.Error() != expected {
+		t.Fatalf("Expected error was %v, got %v", expected, err.Error())
+	}
+	mock.ExpectQuery("select").WithArgs(1, 4).WillReturnRows(nil)
+	mock.ExpectCommit()
+	err = validateDSRegexOrder(tx, 1, 3)
+	if err != nil {
+		t.Fatalf("Expect no error, got %v", err.Error())
+	}
+	err = validateDSRegexOrder(tx, 1, -1)
+	if err == nil {
+		t.Fatal("Expect error saying cannot add regex with order < 0, got nothing")
+	}
+	if err.Error() != "cannot add regex with order < 0" {
+		t.Errorf("Expected error detail to be 'cannot add regex with order <0', got %v", err.Error())
+	}
+}