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 <[email protected]>
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(®exID); 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())
+ }
+}
