rawlinp commented on a change in pull request #5345:
URL: https://github.com/apache/trafficcontrol/pull/5345#discussion_r542541638
##########
File path: traffic_ops/traffic_ops_golang/routing/routes.go
##########
@@ -145,11 +145,9 @@ func Routes(d ServerData) ([]Route, []RawRoute,
http.Handler, error) {
* 3.x API
*/
////DeliveryServices
- {api.Version{3, 1}, http.MethodGet, `deliveryservices/?$`,
api.ReadHandler(&deliveryservice.TODeliveryService{}), auth.PrivLevelReadOnly,
Authenticated, nil, 22383173943, noPerlBypass},
{api.Version{3, 1}, http.MethodPost, `deliveryservices/?$`,
deliveryservice.CreateV31, auth.PrivLevelOperations, Authenticated, nil,
2064315323, noPerlBypass},
{api.Version{3, 1}, http.MethodPut, `deliveryservices/{id}/?$`,
deliveryservice.UpdateV31, auth.PrivLevelOperations, Authenticated, nil,
27665675673, noPerlBypass},
{api.Version{3, 1}, http.MethodPut,
`deliveryservices/{id}/safe/?$`, deliveryservice.UpdateSafe,
auth.PrivLevelOperations, Authenticated, nil, 2472107313, perlBypass},
Review comment:
I think this route is unnecessary as well -- the existing 3.0 route will
handle the 3.1 requests.
##########
File path: CHANGELOG.md
##########
@@ -5,6 +5,7 @@ The format is based on [Keep a
Changelog](http://keepachangelog.com/en/1.0.0/).
## [unreleased]
### Added
+- Traffic Ops: added a feature so that the user can specify
`maxRequestHeadersize` on a per delivery service basis
Review comment:
`maxRequestHeaderBytes`
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -187,10 +190,12 @@ type DeliveryServiceNullableV30 struct {
}
type DeliveryServiceV31 struct {
- DeliveryServiceNullableV30
+ DeliveryServiceV30
MaxRequestHeaderSize *int `json:"maxRequestHeaderSize"
db:"max_request_header_size"`
}
+type DeliveryServiceNullableV30 DeliveryServiceV31
Review comment:
We should add a comment here that this type should always alias the
_latest_ 3.x minor version struct.
##########
File path:
traffic_ops/app/db/migrations/2020113000000000_add_max_request_header_size_delivery_service.sql
##########
@@ -0,0 +1,22 @@
+/*
+
+ Licensed 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.
+*/
+
+-- +goose Up
+-- SQL in section 'Up' is executed when this migration is applied
+ALTER TABLE deliveryservice ADD COLUMN max_request_header_bytes int DEFAULT
131072;
Review comment:
I think since we `Sanitize()` this to the default value upon
creation/deletion, we can make this column `NOT NULL`.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -194,14 +194,39 @@ func CreateV30(w http.ResponseWriter, r *http.Request) {
return
}
- res, status, userErr, sysErr := createV30(w, r, inf, ds)
+ res, status, userErr, sysErr := createV31(w, r, inf, ds)
if userErr != nil || sysErr != nil {
api.HandleErr(w, r, inf.Tx.Tx, status, userErr, sysErr)
return
}
api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Deliveryservice creation
was successful.", []tc.DeliveryServiceNullableV30{*res})
}
+func CreateV30(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil)
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ ds := tc.DeliveryServiceV30{}
+ if err := json.NewDecoder(r.Body).Decode(&ds); err != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest,
errors.New("decoding: "+err.Error()), nil)
+ return
+ }
+
+ dsV30Nullable := tc.DeliveryServiceNullableV30{
Review comment:
`createV30` should take a `tc.DeliveryServiceV30` as input, so we
shouldn't need to convert it here.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -180,7 +180,7 @@ func CreateV15(w http.ResponseWriter, r *http.Request) {
api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Deliveryservice creation
was successful.", []tc.DeliveryServiceNullableV15{*res})
}
-func CreateV30(w http.ResponseWriter, r *http.Request) {
+func CreateV31(w http.ResponseWriter, r *http.Request) {
Review comment:
So on L191 below we should actually unmarshal the request into a
`DeliveryServiceV31` struct, which is then passed into `createV31`, and then
L202 writes `[]tc.DeliveryServiceV31`.
Basically, we want these minor-version specific "route entry" functions to
_not_ use the alias. That way, we don't have to update the existing "route
entry" functions when we add new minor versions.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -230,16 +255,19 @@ func createV14(w http.ResponseWriter, r *http.Request,
inf *api.APIInfo, reqDS t
}
func createV15(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, reqDS
tc.DeliveryServiceNullableV15) (*tc.DeliveryServiceNullableV15, int, error,
error) {
- dsV30 := tc.DeliveryServiceNullableV30{DeliveryServiceNullableV15:
reqDS}
- res, status, userErr, sysErr := createV30(w, r, inf, dsV30)
+ dsV30 := tc.DeliveryServiceV30{DeliveryServiceNullableV15: reqDS}
+ dsV30Nullable := tc.DeliveryServiceNullableV30{
Review comment:
`createV30` should take a `DeliveryServiceV30` as input, so this
conversion should be unnecessary.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -264,7 +292,7 @@ func createV30(w http.ResponseWriter, r *http.Request, inf
*api.APIInfo, ds tc.D
return nil, errCode, userErr, sysErr
}
- resultRows, err := tx.Query(insertQuery(),
+ resultRows, err := tx.Query(insertV31Query(),
Review comment:
the existing `insertQuery` function should be updated to include the new
field, so a specific `insertV31Query` should be unnecessary
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -588,14 +791,42 @@ func UpdateV30(w http.ResponseWriter, r *http.Request) {
id := inf.IntParams["id"]
+ ds := tc.DeliveryServiceV30{}
+ if err := json.NewDecoder(r.Body).Decode(&ds); err != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest,
errors.New("malformed JSON: "+err.Error()), nil)
+ return
+ }
+ ds.ID = &id
+
+ dsV30Nullable := tc.DeliveryServiceNullableV30{
+ DeliveryServiceV30: ds,
+ }
+ res, status, userErr, sysErr := updateV30(w, r, inf, &dsV30Nullable)
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, status, userErr, sysErr)
+ return
+ }
+ api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Deliveryservice update
was successful.", []tc.DeliveryServiceV30{*res})
+}
+
+func UpdateV31(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, nil, []string{"id"})
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ id := inf.IntParams["id"]
+
ds := tc.DeliveryServiceNullableV30{}
Review comment:
here we want to unmarshal into a `DeliveryServiceV31` struct which is
then passed to `updateV31`, and on L834 we want to write
`[]tc.DeliveryServiceV31`
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -588,14 +791,42 @@ func UpdateV30(w http.ResponseWriter, r *http.Request) {
id := inf.IntParams["id"]
+ ds := tc.DeliveryServiceV30{}
+ if err := json.NewDecoder(r.Body).Decode(&ds); err != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest,
errors.New("malformed JSON: "+err.Error()), nil)
+ return
+ }
+ ds.ID = &id
+
+ dsV30Nullable := tc.DeliveryServiceNullableV30{
Review comment:
Similar to `createV30`, `updateV30` should take a
`tc.DeliveryServiceV30` as input, so we shouldn't need to convert it here.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -1666,7 +2132,7 @@ func getTenantID(tx *sql.Tx, ds
*tc.DeliveryServiceNullableV30) (*int, error) {
return existingID, err
}
-func isTenantAuthorized(inf *api.APIInfo, ds *tc.DeliveryServiceNullableV30)
(bool, error) {
+func isTenantAuthorized(inf *api.APIInfo, ds *tc.DeliveryServiceV30) (bool,
error) {
Review comment:
This should take `DeliveryServiceNullableV30` as input
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -1139,7 +1604,7 @@ func getTypeFromID(id int, tx *sql.Tx) (tc.DSType, error)
{
return tc.DSTypeFromString(name), nil
}
-func updatePrimaryOrigin(tx *sql.Tx, user *auth.CurrentUser, ds
tc.DeliveryServiceNullableV30) error {
+func updatePrimaryOrigin(tx *sql.Tx, user *auth.CurrentUser, ds
tc.DeliveryServiceV30) error {
Review comment:
This should take `DeliveryServiceNullableV30` as input
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -1179,7 +1644,7 @@ func updatePrimaryOrigin(tx *sql.Tx, user
*auth.CurrentUser, ds tc.DeliveryServi
return nil
}
-func createPrimaryOrigin(tx *sql.Tx, user *auth.CurrentUser, ds
tc.DeliveryServiceNullableV30) error {
+func createPrimaryOrigin(tx *sql.Tx, user *auth.CurrentUser, ds
tc.DeliveryServiceV30) error {
Review comment:
This should take `DeliveryServiceNullableV30` as input
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -88,7 +88,7 @@ func (ds *TODeliveryService) GetType() string {
// IsTenantAuthorized checks that the user is authorized for both the delivery
service's existing tenant, and the new tenant they're changing it to (if
different).
func (ds *TODeliveryService) IsTenantAuthorized(user *auth.CurrentUser) (bool,
error) {
- return isTenantAuthorized(ds.ReqInfo, &ds.DeliveryServiceNullableV30)
+ return isTenantAuthorized(ds.ReqInfo, &ds.DeliveryServiceV30)
Review comment:
I think this can remain `DeliveryServiceNullableV30`
##########
File path: traffic_ops/testing/api/v3/deliveryservices_test.go
##########
@@ -176,38 +176,42 @@ func SSLDeliveryServiceCDNUpdateTest(t *testing.T) {
}
customDS := tc.DeliveryServiceNullableV30{
- DeliveryServiceNullableV15: tc.DeliveryServiceNullableV15{
- DeliveryServiceNullableV14:
tc.DeliveryServiceNullableV14{
- DeliveryServiceNullableV13:
tc.DeliveryServiceNullableV13{
- DeliveryServiceNullableV12:
tc.DeliveryServiceNullableV12{
- DeliveryServiceNullableV11:
tc.DeliveryServiceNullableV11{
- Active:
util.BoolPtr(true),
- CDNID:
util.IntPtr(oldCdn.ID),
- DSCP:
util.IntPtr(0),
- DisplayName:
util.StrPtr("displayName"),
- RoutingName:
util.StrPtr("routingName"),
- GeoLimit:
util.IntPtr(0),
- GeoProvider:
util.IntPtr(0),
- IPV6RoutingEnabled:
util.BoolPtr(false),
- InitialDispersion:
util.IntPtr(1),
- LogsEnabled:
util.BoolPtr(true),
- MissLat:
util.FloatPtr(0),
- MissLong:
util.FloatPtr(0),
- MultiSiteOrigin:
util.BoolPtr(false),
- OrgServerFQDN:
util.StrPtr("https://test.com";),
- Protocol:
util.IntPtr(2),
- QStringIgnore:
util.IntPtr(0),
- RangeRequestHandling:
util.IntPtr(0),
- RegionalGeoBlocking:
util.BoolPtr(false),
- TenantID:
util.IntPtr(1),
- TypeID:
util.IntPtr(types[0].ID),
- XMLID:
util.StrPtr("dsID"),
+ DeliveryServiceV30: tc.DeliveryServiceV30{
Review comment:
It's better to declare DS structs like this, so that the declaration
doesn't need updated with every minor version:
```
customDS := tc.DeliveryServiceNullableV30{}
customDS.Active = util.BoolPtr(true)
...
(etc)
```
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -45,6 +48,13 @@ type DeliveryServicesResponse struct {
Alerts
}
+// DeliveryServicesResponseV31 is the type of a response from the
+// /api/3.1/deliveryservices Traffic Ops endpoint.
+type DeliveryServicesResponseV31 struct {
Review comment:
this struct should be unnecessary -- we should be able to use
`DeliveryServicesResponseV30` since it uses the `DeliveryServiceNullableV30`
alias.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -230,16 +255,19 @@ func createV14(w http.ResponseWriter, r *http.Request,
inf *api.APIInfo, reqDS t
}
func createV15(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, reqDS
tc.DeliveryServiceNullableV15) (*tc.DeliveryServiceNullableV15, int, error,
error) {
- dsV30 := tc.DeliveryServiceNullableV30{DeliveryServiceNullableV15:
reqDS}
- res, status, userErr, sysErr := createV30(w, r, inf, dsV30)
+ dsV30 := tc.DeliveryServiceV30{DeliveryServiceNullableV15: reqDS}
+ dsV30Nullable := tc.DeliveryServiceNullableV30{
+ DeliveryServiceV30: dsV30,
+ }
+ res, status, userErr, sysErr := createV30(w, r, inf, dsV30Nullable)
if res != nil {
return &res.DeliveryServiceNullableV15, 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 createV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds
tc.DeliveryServiceNullableV30) (*tc.DeliveryServiceNullableV30, int, error,
error) {
+func createV31(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds
tc.DeliveryServiceNullableV30) (*tc.DeliveryServiceNullableV30, int, error,
error) {
Review comment:
This function should take a `DeliveryServiceV31` as input (and output),
but then we should immediately typecast it to `DeliveryServiceNullableV30`
within the function and typecast it _back_ to `DeliveryServiceV31` when
returning it.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -1654,7 +2120,7 @@ func GetDSSelectQuery() string {
}
// getTenantID returns the tenant Id of the given delivery service. Note it
may return a nil id and nil error, if the tenant ID in the database is nil.
-func getTenantID(tx *sql.Tx, ds *tc.DeliveryServiceNullableV30) (*int, error) {
+func getTenantID(tx *sql.Tx, ds *tc.DeliveryServiceV30) (*int, error) {
Review comment:
This should take `DeliveryServiceNullableV30` as input
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -248,7 +276,7 @@ func createV30(w http.ResponseWriter, r *http.Request, inf
*api.APIInfo, ds tc.D
return nil, http.StatusBadRequest, errors.New("invalid request:
" + err.Error()), nil
}
- if authorized, err := isTenantAuthorized(inf, &ds); err != nil {
+ if authorized, err := isTenantAuthorized(inf, &ds.DeliveryServiceV30);
err != nil {
Review comment:
following my previous comments this shouldn't be necessary
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -398,7 +427,7 @@ func createV30(w http.ResponseWriter, r *http.Request, inf
*api.APIInfo, ds tc.D
}
}
- if err := createPrimaryOrigin(tx, user, ds); err != nil {
+ if err := createPrimaryOrigin(tx, user, ds.DeliveryServiceV30); err !=
nil {
Review comment:
This function should take `DeliveryServiceNullableV30` as input, so this
change should be unnecessary.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -1954,3 +2490,72 @@ VALUES
($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16,$17,$18,$19,$20,$
RETURNING id, last_updated
`
}
+
+func insertV31Query() string {
Review comment:
the `insertQuery()` function should be updated instead, making this
function unnecessary
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -410,6 +439,178 @@ func createV30(w http.ResponseWriter, r *http.Request,
inf *api.APIInfo, ds tc.D
return &ds, http.StatusOK, nil, nil
}
+// 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.DeliveryServiceV30, int, error, error) {
Review comment:
So, if you look at `createV15`, this function should look at lot like
it. Basically we want it to take a `DeliveryServiceV30` struct as input and
return one as output. Then it should just convert the `DeliveryServiceV30` into
a `DeliveryServiceV31`, then pass that to `createV31` which does all the actual
creation.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -1819,6 +2286,75 @@ LEFT JOIN tenant ON ds.tenant_id = tenant.id
`
}
+func updateDSV31Query() string {
Review comment:
the `updateQuery()` function should be updated instead, making this
function unnecessary
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -629,117 +860,352 @@ WHERE
}
return nil, http.StatusInternalServerError, nil,
fmt.Errorf("querying delivery service ID %d: %s", *dsV13.ID, err.Error())
}
- if dsV13.DeepCachingType != nil {
- *dsV13.DeepCachingType =
tc.DeepCachingTypeFromString(string(*dsV13.DeepCachingType))
+ if dsV13.DeepCachingType != nil {
+ *dsV13.DeepCachingType =
tc.DeepCachingTypeFromString(string(*dsV13.DeepCachingType))
+ }
+
+ res, status, userErr, sysErr := updateV13(w, r, inf, &dsV13)
+ if res != nil {
+ return &res.DeliveryServiceNullableV12, status, userErr, sysErr
+ }
+ return nil, status, userErr, sysErr
+}
+
+func updateV13(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, reqDS
*tc.DeliveryServiceNullableV13) (*tc.DeliveryServiceNullableV13, int, error,
error) {
Review comment:
This is kind of confusing to review since it seems like this was all
added as new code, but I think things were just moved around.
For the update* functions, you should just rename `updateV30` to
`updateV31`, and change it to take in `DeliveryServiceV31` as input and output.
Then _within_ the function you should immediately cast it to
`DeliveryServiceNullableV30`. At the end of the function, you cast it back to
`DeliveryServiceV31` before returning it.
Then you can basically copy `updateV15` to `updateV30`, change it to take
`DeliveryServiceV30` as input and output, then change it to read the new
`max_request_header_bytes` from the DB before passing it to `updateV31`, where
the real handling is done.
----------------------------------------------------------------
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]
