rawlinp commented on a change in pull request #5345:
URL: https://github.com/apache/trafficcontrol/pull/5345#discussion_r543803113
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -428,8 +445,8 @@ func (ds *DeliveryServiceNullable) validateTypeFields(tx
*sql.Tx) error {
if *ds.RangeRequestHandling == 3 {
return
validation.Validate(ds.RangeSliceBlockSize, validation.Required,
// Per Slice Plugin
implementation
- validation.Min(262144),
// 256KiB
-
validation.Max(33554432), // 32MiB
+
validation.Min(MinRangeSliceBlockSize), // 256KiB
Review comment:
We should do some basic validation for `MaxRequestHeaderBytes` as well
-- like it should be greater than zero.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -699,62 +759,301 @@ WHERE
}
func updateV15(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, reqDS
*tc.DeliveryServiceNullableV15) (*tc.DeliveryServiceNullableV15, int, error,
error) {
- dsV30 := tc.DeliveryServiceNullableV30{DeliveryServiceNullableV15:
*reqDS}
+ dsV31 := tc.DeliveryServiceV31{
+ DeliveryServiceV30: tc.DeliveryServiceV30{
+ DeliveryServiceNullableV15: *reqDS,
+ },
+ }
// query the DB for existing 3.0 fields in order to "upgrade" this 1.5
request into a 3.0 request
query := `
SELECT
ds.topology,
ds.first_header_rewrite,
ds.inner_header_rewrite,
ds.last_header_rewrite,
- ds.service_category
+ ds.service_category,
+ ds.max_request_header_bytes
FROM
deliveryservice ds
WHERE
ds.id = $1`
if err := inf.Tx.Tx.QueryRow(query, *reqDS.ID).Scan(
- &dsV30.Topology,
- &dsV30.FirstHeaderRewrite,
- &dsV30.InnerHeaderRewrite,
- &dsV30.LastHeaderRewrite,
- &dsV30.ServiceCategory,
+ &dsV31.Topology,
+ &dsV31.FirstHeaderRewrite,
+ &dsV31.InnerHeaderRewrite,
+ &dsV31.LastHeaderRewrite,
+ &dsV31.ServiceCategory,
+ &dsV31.MaxRequestHeaderBytes,
); err != nil {
if err == sql.ErrNoRows {
- return nil, http.StatusNotFound, fmt.Errorf("delivery
service ID %d not found", *dsV30.ID), nil
+ return nil, http.StatusNotFound, fmt.Errorf("delivery
service ID %d not found", *dsV31.ID), nil
}
- return nil, http.StatusInternalServerError, nil,
fmt.Errorf("querying delivery service ID %d: %s", *dsV30.ID, err.Error())
+ return nil, http.StatusInternalServerError, nil,
fmt.Errorf("querying delivery service ID %d: %s", *dsV31.ID, err.Error())
}
- res, status, userErr, sysErr := updateV30(w, r, inf, &dsV30)
+ res, status, userErr, sysErr := updateV31(w, r, inf, &dsV31)
if res != nil {
return &res.DeliveryServiceNullableV15, status, userErr, sysErr
}
return nil, status, userErr, sysErr
}
-func updateV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds
*tc.DeliveryServiceNullableV30) (*tc.DeliveryServiceNullableV30, int, error,
error) {
+func updateV31(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds
*tc.DeliveryServiceV31) (*tc.DeliveryServiceV31, int, error, error) {
tx := inf.Tx.Tx
user := inf.User
+ dsNullableV30 := tc.DeliveryServiceNullableV30(*ds)
+ if err := dsNullableV30.Validate(tx); err != nil {
+ return nil, http.StatusBadRequest, errors.New("invalid request:
" + err.Error()), nil
+ }
+
+ if authorized, err := isTenantAuthorized(inf, &dsNullableV30); err !=
nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("checking tenant: " + err.Error())
+ } else if !authorized {
+ return nil, http.StatusForbidden, errors.New("not authorized on
this tenant"), nil
+ }
+
+ if dsNullableV30.XMLID == nil {
+ return nil, http.StatusBadRequest, errors.New("missing
xml_id"), nil
+ }
+ if dsNullableV30.ID == nil {
+ return nil, http.StatusBadRequest, errors.New("missing id"), nil
+ }
+
+ dsType, ok, err := getDSType(tx, *dsNullableV30.XMLID)
+ if !ok {
+ return nil, http.StatusNotFound, errors.New("delivery service
'" + *dsNullableV30.XMLID + "' not found"), nil
+ }
+ if err != nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("getting delivery service type during update: " + err.Error())
+ }
- if err := ds.Validate(tx); err != nil {
+ // oldHostName will be used to determine if SSL Keys need updating -
this will be empty if the DS doesn't have SSL keys, because DS types without
SSL keys may not have regexes, and thus will fail to get a host name.
+ oldHostName := ""
+ oldCDNName := ""
+ oldRoutingName := ""
+ if dsType.HasSSLKeys() {
+ oldHostName, oldCDNName, oldRoutingName, err =
getOldDetails(*dsNullableV30.ID, tx)
+ if err != nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("getting existing delivery service hostname: " + err.Error())
+ }
+ }
+
+ // TODO change DeepCachingType to implement sql.Valuer and sql.Scanner,
so sqlx struct scan can be used.
+ deepCachingType := tc.DeepCachingType("").String()
+ if dsNullableV30.DeepCachingType != nil {
+ deepCachingType = dsNullableV30.DeepCachingType.String() //
necessary, because DeepCachingType's default needs to insert the string, not
"", and Query doesn't call .String().
+ }
+
+ userErr, sysErr, errCode := api.CheckIfUnModified(r.Header, inf.Tx,
*dsNullableV30.ID, "deliveryservice")
+ if userErr != nil || sysErr != nil {
+ return nil, errCode, userErr, sysErr
+ }
+
+ if errCode, userErr, sysErr = dbhelpers.CheckTopology(inf.Tx,
dsNullableV30); userErr != nil || sysErr != nil {
+ return nil, errCode, userErr, sysErr
+ }
+
+ if dsNullableV30.Topology != nil {
+ requiredCapabilities, err :=
dbhelpers.GetDSRequiredCapabilitiesFromID(*dsNullableV30.ID, tx)
+ if err != nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("getting existing DS required capabilities: " + err.Error())
+ }
+ if len(requiredCapabilities) > 0 {
+ if userErr, sysErr, status :=
EnsureTopologyBasedRequiredCapabilities(tx, *dsNullableV30.ID,
*dsNullableV30.Topology, requiredCapabilities); userErr != nil || sysErr != nil
{
+ return nil, status, userErr, sysErr
+ }
+ }
+ }
+
+ resultRows, err := tx.Query(updateDSQuery(),
+ &dsNullableV30.Active,
+ &dsNullableV30.CacheURL,
+ &dsNullableV30.CCRDNSTTL,
+ &dsNullableV30.CDNID,
+ &dsNullableV30.CheckPath,
+ &deepCachingType,
+ &dsNullableV30.DisplayName,
+ &dsNullableV30.DNSBypassCNAME,
+ &dsNullableV30.DNSBypassIP,
+ &dsNullableV30.DNSBypassIP6,
+ &dsNullableV30.DNSBypassTTL,
+ &dsNullableV30.DSCP,
+ &dsNullableV30.EdgeHeaderRewrite,
+ &dsNullableV30.GeoLimitRedirectURL,
+ &dsNullableV30.GeoLimit,
+ &dsNullableV30.GeoLimitCountries,
+ &dsNullableV30.GeoProvider,
+ &dsNullableV30.GlobalMaxMBPS,
+ &dsNullableV30.GlobalMaxTPS,
+ &dsNullableV30.FQPacingRate,
+ &dsNullableV30.HTTPBypassFQDN,
+ &dsNullableV30.InfoURL,
+ &dsNullableV30.InitialDispersion,
+ &dsNullableV30.IPV6RoutingEnabled,
+ &dsNullableV30.LogsEnabled,
+ &dsNullableV30.LongDesc,
+ &dsNullableV30.LongDesc1,
+ &dsNullableV30.LongDesc2,
+ &dsNullableV30.MaxDNSAnswers,
+ &dsNullableV30.MidHeaderRewrite,
+ &dsNullableV30.MissLat,
+ &dsNullableV30.MissLong,
+ &dsNullableV30.MultiSiteOrigin,
+ &dsNullableV30.OriginShield,
+ &dsNullableV30.ProfileID,
+ &dsNullableV30.Protocol,
+ &dsNullableV30.QStringIgnore,
+ &dsNullableV30.RangeRequestHandling,
+ &dsNullableV30.RegexRemap,
+ &dsNullableV30.RegionalGeoBlocking,
+ &dsNullableV30.RemapText,
+ &dsNullableV30.RoutingName,
+ &dsNullableV30.SigningAlgorithm,
+ &dsNullableV30.SSLKeyVersion,
+ &dsNullableV30.TenantID,
+ &dsNullableV30.TRRequestHeaders,
+ &dsNullableV30.TRResponseHeaders,
+ &dsNullableV30.TypeID,
+ &dsNullableV30.XMLID,
+ &dsNullableV30.AnonymousBlockingEnabled,
+ &dsNullableV30.ConsistentHashRegex,
+ &dsNullableV30.MaxOriginConnections,
+ &dsNullableV30.EcsEnabled,
+ &dsNullableV30.RangeSliceBlockSize,
+ &dsNullableV30.Topology,
+ &dsNullableV30.FirstHeaderRewrite,
+ &dsNullableV30.InnerHeaderRewrite,
+ &dsNullableV30.LastHeaderRewrite,
+ &dsNullableV30.ServiceCategory,
+ &dsNullableV30.MaxRequestHeaderBytes,
+ &dsNullableV30.ID)
+
+ if err != nil {
+ usrErr, sysErr, code := api.ParseDBError(err)
+ return nil, code, usrErr, sysErr
+ }
+ defer resultRows.Close()
+ if !resultRows.Next() {
+ return nil, http.StatusNotFound, errors.New("no delivery
service found with this id"), nil
+ }
+ lastUpdated := tc.TimeNoMod{}
+ if err := resultRows.Scan(&lastUpdated); err != nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("scan updating delivery service: " + err.Error())
+ }
+ if resultRows.Next() {
+ xmlID := ""
+ if dsNullableV30.XMLID != nil {
+ xmlID = *dsNullableV30.XMLID
+ }
+ return nil, http.StatusInternalServerError, nil,
errors.New("updating delivery service " + xmlID + ": " + "this update affected
too many rows: > 1")
+ }
+
+ if dsNullableV30.ID == nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("missing id after update")
+ }
+ if dsNullableV30.XMLID == nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("missing xml_id after update")
+ }
+ if dsNullableV30.TypeID == nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("missing type after update")
+ }
+ if dsNullableV30.RoutingName == nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("missing routing name after update")
+ }
+ newDSType, err := getTypeFromID(*dsNullableV30.TypeID, tx)
+ if err != nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("getting delivery service type after update: " + err.Error())
+ }
+ dsNullableV30.Type = &newDSType
+
+ cdnDomain, err := getCDNDomain(*dsNullableV30.ID, tx) // need to get
the domain again, in case it changed.
+ if err != nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("getting CDN domain after update: " + err.Error())
+ }
+
+ matchLists, err :=
GetDeliveryServicesMatchLists([]string{*dsNullableV30.XMLID}, tx)
+ if err != nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("getting matchlists after update: " + err.Error())
+ }
+ if ml, ok := matchLists[*dsNullableV30.XMLID]; !ok {
+ return nil, http.StatusInternalServerError, nil, errors.New("no
matchlists after update")
+ } else {
+ dsNullableV30.MatchList = &ml
+ }
+
+ // newHostName will be used to determine if SSL Keys need updating -
this will be empty if the DS doesn't have SSL keys, because DS types without
SSL keys may not have regexes, and thus will fail to get a host name.
+ newHostName := ""
+ newCDNName := ""
+ if dsType.HasSSLKeys() {
+ newCDNName, err = getCDNName(*dsNullableV30.CDNID, tx)
+ newHostName, err = getHostName(dsNullableV30.Protocol,
*dsNullableV30.Type, *dsNullableV30.RoutingName, *dsNullableV30.MatchList,
cdnDomain)
+ if err != nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("getting cdnname/hostname after update: " + err.Error())
+ }
+ }
+
+ if newDSType.HasSSLKeys() && (oldHostName != newHostName || oldCDNName
!= newCDNName || oldRoutingName != *dsNullableV30.RoutingName) {
+ return nil, http.StatusForbidden, nil, errors.New("delivery
service has ssl keys that cannot be automatically changed")
+ }
+
+ if err := EnsureParams(tx, *dsNullableV30.ID, *dsNullableV30.XMLID,
dsNullableV30.EdgeHeaderRewrite, dsNullableV30.MidHeaderRewrite,
dsNullableV30.RegexRemap, dsNullableV30.CacheURL,
dsNullableV30.SigningAlgorithm, newDSType, dsNullableV30.MaxOriginConnections);
err != nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("ensuring dsNullableV30 parameters:: " + err.Error())
+ }
+
+ if err := updatePrimaryOrigin(tx, user, dsNullableV30); err != nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("updating delivery service: " + err.Error())
+ }
+
+ dsNullableV30.LastUpdated = &lastUpdated
+
+ // the update may change or delete the query params -- delete existing
and re-add if any provided
+ q := `DELETE FROM deliveryservice_consistent_hash_query_param WHERE
deliveryservice_id = $1`
+ if res, err := tx.Exec(q, *dsNullableV30.ID); err != nil {
+ return nil, http.StatusInternalServerError, nil,
fmt.Errorf("deleting consistent hash query params for dsNullableV30 %s: %s",
*dsNullableV30.XMLID, err.Error())
+ } else if c, _ := res.RowsAffected(); c > 0 {
+ api.CreateChangeLogRawTx(api.ApiChange, "DS:
"+*dsNullableV30.XMLID+", ID: "+strconv.Itoa(*dsNullableV30.ID)+", ACTION:
Deleted "+strconv.FormatInt(c, 10)+" consistent hash query params", user, tx)
+ }
+
+ if c, err := createConsistentHashQueryParams(tx, *dsNullableV30.ID,
dsNullableV30.ConsistentHashQueryParams); err != nil {
+ usrErr, sysErr, code := api.ParseDBError(err)
+ return nil, code, usrErr, sysErr
+ } else {
+ api.CreateChangeLogRawTx(api.ApiChange, "DS:
"+*dsNullableV30.XMLID+", ID: "+strconv.Itoa(*dsNullableV30.ID)+", ACTION:
Created "+strconv.Itoa(c)+" consistent hash query params", user, tx)
+ }
+
+ if err := api.CreateChangeLogRawErr(api.ApiChange, "Updated
dsNullableV30: "+*dsNullableV30.XMLID+" id: "+strconv.Itoa(*dsNullableV30.ID),
user, tx); err != nil {
+ return nil, http.StatusInternalServerError, nil,
errors.New("writing change log entry: " + err.Error())
+ }
+ ds = (*tc.DeliveryServiceV31)(&dsNullableV30)
+ return ds, http.StatusOK, nil, nil
+}
+
+func updateV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds
*tc.DeliveryServiceV30) (*tc.DeliveryServiceV30, int, error, error) {
Review comment:
Per my earlier comment above, this function should look just like
`createV30`, except with the DB query that reads the existing value of the new
field.
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -32,6 +32,9 @@ import (
*/
const DefaultRoutingName = "cdn"
+const DefaultMaxHeaderSize = 131072
Review comment:
we should rename this `DefaultMaxRequestHeaderBytes` to match the new
name
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -239,91 +261,101 @@ func createV15(w http.ResponseWriter, r *http.Request,
inf *api.APIInfo, reqDS t
}
// create creates the given ds in the database, and returns the DS with its id
and other fields created on insert set. On error, the HTTP status code, user
error, and system error are returned. The status code SHOULD NOT be used, if
both errors are nil.
-func createV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds
tc.DeliveryServiceNullableV30) (*tc.DeliveryServiceNullableV30, int, error,
error) {
+func createV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds
tc.DeliveryServiceV30) (*tc.DeliveryServiceV30, int, error, error) {
+ dsV31 := tc.DeliveryServiceV31{DeliveryServiceV30: ds}
+ res, status, userErr, sysErr := createV31(w, r, inf, dsV31)
+ if res != nil {
+ return &res.DeliveryServiceV30, status, userErr, sysErr
+ }
+ return nil, status, userErr, sysErr
+}
+
+// create creates the given ds in the database, and returns the DS with its id
and other fields created on insert set. On error, the HTTP status code, user
error, and system error are returned. The status code SHOULD NOT be used, if
both errors are nil.
+func createV31(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds
tc.DeliveryServiceV31) (*tc.DeliveryServiceV31, int, error, error) {
Review comment:
We should rename `ds` to `dsV13`, then we can use `ds` instead of
`dsNullableV30` on L278. Essentially, we don't want to be distracted by the
versioning stuff throughout the method. Also it cuts down on the number of
lines we have to change when adding new versions.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -699,62 +759,301 @@ WHERE
}
func updateV15(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, reqDS
*tc.DeliveryServiceNullableV15) (*tc.DeliveryServiceNullableV15, int, error,
error) {
Review comment:
So this function should actually stay exactly the same as it was before.
Basically, we want to rename `updateV30` to `updateV31`, change it to accept
and return V31 structs, then copy `updateV15` to `updateV30`, change it to
accept V30 structs, upgrade them to V31 by changing the query to only select
the new V31 field from the DB, then pass it to `updateV31` for the actual
handling.
It's kind of hard to draw in a review, but the functions are designed to
chain together like so:
```
UpdateV12 -> updateV12
| ^
V |
UpdateV13 -> updateV13
| ^
V |
... ...
UpdateV31 -> updateV31 <------(this latest handler does all the actual
handling)
```
So each API version is like a link in a chain. For adding a new version, you
just add a new link to the end of the chain, and you don't have to worry about
earlier links in the chain (other than the one you're linking to). Does that
make sense?
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -699,62 +759,301 @@ WHERE
}
func updateV15(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, reqDS
*tc.DeliveryServiceNullableV15) (*tc.DeliveryServiceNullableV15, int, error,
error) {
- dsV30 := tc.DeliveryServiceNullableV30{DeliveryServiceNullableV15:
*reqDS}
+ dsV31 := tc.DeliveryServiceV31{
+ DeliveryServiceV30: tc.DeliveryServiceV30{
+ DeliveryServiceNullableV15: *reqDS,
+ },
+ }
// query the DB for existing 3.0 fields in order to "upgrade" this 1.5
request into a 3.0 request
query := `
SELECT
ds.topology,
ds.first_header_rewrite,
ds.inner_header_rewrite,
ds.last_header_rewrite,
- ds.service_category
+ ds.service_category,
+ ds.max_request_header_bytes
FROM
deliveryservice ds
WHERE
ds.id = $1`
if err := inf.Tx.Tx.QueryRow(query, *reqDS.ID).Scan(
- &dsV30.Topology,
- &dsV30.FirstHeaderRewrite,
- &dsV30.InnerHeaderRewrite,
- &dsV30.LastHeaderRewrite,
- &dsV30.ServiceCategory,
+ &dsV31.Topology,
+ &dsV31.FirstHeaderRewrite,
+ &dsV31.InnerHeaderRewrite,
+ &dsV31.LastHeaderRewrite,
+ &dsV31.ServiceCategory,
+ &dsV31.MaxRequestHeaderBytes,
); err != nil {
if err == sql.ErrNoRows {
- return nil, http.StatusNotFound, fmt.Errorf("delivery
service ID %d not found", *dsV30.ID), nil
+ return nil, http.StatusNotFound, fmt.Errorf("delivery
service ID %d not found", *dsV31.ID), nil
}
- return nil, http.StatusInternalServerError, nil,
fmt.Errorf("querying delivery service ID %d: %s", *dsV30.ID, err.Error())
+ return nil, http.StatusInternalServerError, nil,
fmt.Errorf("querying delivery service ID %d: %s", *dsV31.ID, err.Error())
}
- res, status, userErr, sysErr := updateV30(w, r, inf, &dsV30)
+ res, status, userErr, sysErr := updateV31(w, r, inf, &dsV31)
if res != nil {
return &res.DeliveryServiceNullableV15, status, userErr, sysErr
}
return nil, status, userErr, sysErr
}
-func updateV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds
*tc.DeliveryServiceNullableV30) (*tc.DeliveryServiceNullableV30, int, error,
error) {
+func updateV31(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds
*tc.DeliveryServiceV31) (*tc.DeliveryServiceV31, int, error, error) {
Review comment:
similar to my comment for `createV31`, we should rename this parameter
to `dsV31`, then use `ds` instead of `dsNullableV30` on L803
----------------------------------------------------------------
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:
[email protected]
