[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5922: Per-Delivery Service TLS versions

[GitBox <gi...@apache.org>]

rawlinp commented on a change in pull request #5922:
URL: https://github.com/apache/trafficcontrol/pull/5922#discussion_r647658309



##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -192,18 +194,416 @@ type DeliveryServiceFieldsV31 struct {
 // DeliveryServiceV40 is a Delivery Service as it appears in version 4.0 of the
 // Traffic Ops API.
 type DeliveryServiceV40 struct {
-	DeliveryServiceFieldsV31
-	DeliveryServiceFieldsV30
-	DeliveryServiceFieldsV15
-	DeliveryServiceFieldsV14
-	DeliveryServiceFieldsV13
-	DeliveryServiceNullableFieldsV11
+	// Active dictates whether the Delivery Service is routed by Traffic Router.
+	Active bool `json:"active" db:"active"`

Review comment:
       Why are you making fields non-nullable? How else will we be able to distinguish between values that are empty vs null?

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -1070,87 +1189,88 @@ func updateV40(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, dsV40 *
 
 	if err != nil {
 		usrErr, sysErr, code := api.ParseDBError(err)
-		return nil, code, usrErr, sysErr
+		return nil, alerts, code, usrErr, sysErr
 	}
 	defer resultRows.Close()
 	if !resultRows.Next() {
-		return nil, http.StatusNotFound, errors.New("no delivery service found with this id"), nil
+		return nil, alerts, http.StatusNotFound, errors.New("no delivery service found with this id"), nil
 	}
-	lastUpdated := tc.TimeNoMod{}
+	var lastUpdated time.Time
 	if err := resultRows.Scan(&lastUpdated); err != nil {
-		return nil, http.StatusInternalServerError, nil, errors.New("scan updating delivery service: " + err.Error())
+		return nil, alerts, http.StatusInternalServerError, nil, errors.New("scan updating delivery service: " + err.Error())
 	}
 	if resultRows.Next() {
-		xmlID := ""
-		if ds.XMLID != nil {
-			xmlID = *ds.XMLID
-		}
-		return nil, http.StatusInternalServerError, nil, errors.New("updating delivery service " + xmlID + ": " + "this update affected too many rows: > 1")
+		return nil, alerts, http.StatusInternalServerError, nil, errors.New("updating delivery service " + ds.XMLID + ": " + "this update affected too many rows: > 1")
 	}
 
 	if ds.ID == nil {
-		return nil, http.StatusInternalServerError, nil, errors.New("missing id after update")
+		return nil, alerts, http.StatusInternalServerError, nil, errors.New("missing id after update")
 	}
-	if ds.XMLID == nil {
-		return nil, http.StatusInternalServerError, nil, errors.New("missing xml_id after update")
-	}
-	if ds.TypeID == nil {
-		return nil, http.StatusInternalServerError, nil, errors.New("missing type after update")
-	}
-	if ds.RoutingName == nil {
-		return nil, http.StatusInternalServerError, nil, errors.New("missing routing name after update")
+
+	if inf.Version != nil && inf.Version.Major >= 4 {

Review comment:
       This kind of goes against the underlying theme of the DS update/create functions, which is that the _latest_ function in the chain (in this case `createV40`/`updateV40`) doesn't have any version-specific logic like this. Rather, the `createV31`/`updateV31` functions are responsible for _upgrading_ the 3.1 request into a 4.0 request, passing it to the 4.0 handler for processing, then downgrading the result back to a 3.1 response.
   
   For update requests, that generally means reading the 4.0 fields from the DB to populate the 4.0 struct before passing it to `updateV40`.

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -416,88 +503,93 @@ func createV40(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, dsV40 t
 
 	if err != nil {
 		usrErr, sysErr, code := api.ParseDBError(err)
-		return nil, code, usrErr, sysErr
+		return nil, alerts, code, usrErr, sysErr
 	}
 	defer resultRows.Close()
 
 	id := 0
-	lastUpdated := tc.TimeNoMod{}
+	var lastUpdated time.Time
 	if !resultRows.Next() {
-		return nil, http.StatusInternalServerError, nil, errors.New("no deliveryservice request inserted, no id was returned")
+		return nil, alerts, http.StatusInternalServerError, nil, errors.New("no deliveryservice request inserted, no id was returned")
 	}
 	if err := resultRows.Scan(&id, &lastUpdated); err != nil {
-		return nil, http.StatusInternalServerError, nil, errors.New("could not scan id from insert: " + err.Error())
+		return nil, alerts, http.StatusInternalServerError, nil, errors.New("could not scan id from insert: " + err.Error())
 	}
 	if resultRows.Next() {
-		return nil, http.StatusInternalServerError, nil, errors.New("too many ids returned from deliveryservice request insert")
+		return nil, alerts, http.StatusInternalServerError, nil, errors.New("too many ids returned from deliveryservice request insert")
 	}
 	ds.ID = &id
 
 	if ds.ID == nil {
-		return nil, http.StatusInternalServerError, nil, errors.New("missing id after insert")
-	}
-	if ds.XMLID == nil {
-		return nil, http.StatusInternalServerError, nil, errors.New("missing xml_id after insert")
+		return nil, alerts, http.StatusInternalServerError, nil, errors.New("missing id after insert")
 	}
-	if ds.TypeID == nil {
-		return nil, http.StatusInternalServerError, nil, errors.New("missing type after insert")
-	}
-	dsType, err := getTypeFromID(*ds.TypeID, tx)
+	dsType, err := getTypeFromID(ds.TypeID, tx)
 	if err != nil {
-		return nil, http.StatusInternalServerError, nil, errors.New("getting delivery service type: " + err.Error())
+		return nil, alerts, http.StatusInternalServerError, nil, errors.New("getting delivery service type: " + err.Error())
 	}
 	ds.Type = &dsType
 
-	if err := createDefaultRegex(tx, *ds.ID, *ds.XMLID); err != nil {
-		return nil, http.StatusInternalServerError, nil, errors.New("creating default regex: " + err.Error())
+	// Don't touch TLSVersions if using old API versions

Review comment:
       This kind of goes against the underlying theme of the DS update/create functions, which is that the _latest_ function in the chain (in this case `createV40`/`updateV40`) doesn't have any version-specific logic like this. Rather, the `createV31`/`updateV31` functions are responsible for _upgrading_ the 3.1 request into a 4.0 request, passing it to the 4.0 handler for processing, then downgrading the result back to a 3.1 response.
   
   Since the `createV31` function doesn't need to do anything special other than embedding the 3.1 request fields into a new 4.0 struct (which it currently does), this version-specific logic can just be deleted. 




-- 
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:
users@infra.apache.org