[GitHub] [trafficcontrol] rawlinp commented on a diff in pull request #6569: T3C Race Condition Update

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

rawlinp commented on code in PR #6569:
URL: https://github.com/apache/trafficcontrol/pull/6569#discussion_r843338972


##########
traffic_ops/traffic_ops_golang/server/update.go:
##########
@@ -112,42 +112,161 @@ func UpdateHandler(w http.ResponseWriter, r *http.Request) {
 		return !strings.HasPrefix(strings.ToLower(s), "f")
 	}
 
-	updatedPtr := (*bool)(nil)
+	values := new(updateValues)
+
 	if hasUpdated {
 		updatedBool := strToBool(updated)
-		updatedPtr = &updatedBool
+		values.configUpdateBool = &updatedBool
 	}
-	revalUpdatedPtr := (*bool)(nil)
+
 	if hasRevalUpdated {
 		revalUpdatedBool := strToBool(revalUpdated)
-		revalUpdatedPtr = &revalUpdatedBool
+		values.revalUpdateBool = &revalUpdatedBool
 	}
 
-	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), updatedPtr, revalUpdatedPtr); err != nil {
+	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), *values); err != nil {
 		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("setting updated statuses: "+err.Error()))
 		return
 	}
 
 	respMsg := "successfully set server '" + hostName + "'"
 	if hasUpdated {
-		respMsg += " updated=" + strconv.FormatBool(strToBool(updated))
+		respMsg += " updated=" + updated
 	}
 	if hasRevalUpdated {
-		respMsg += " reval_updated=" + strconv.FormatBool(strToBool(revalUpdated))
+		respMsg += " reval_updated=" + revalUpdated
 	}
 
 	api.WriteAlerts(w, r, http.StatusOK, tc.CreateAlerts(tc.SuccessLevel, respMsg))
 }
 
-// setUpdateStatuses sets the upd_pending and reval_pending columns of a server.
-// If updatePending or revalPending is nil, that value is not changed.
-func setUpdateStatuses(tx *sql.Tx, serverID int64, updatePending *bool, revalPending *bool) error {
-	if updatePending == nil && revalPending == nil {
-		return errors.New("either updatePending or revalPending must not be nil")
+type updateValues struct {
+	configUpdateBool *bool // Deprecated, prefer timestamps
+	revalUpdateBool  *bool // Deprecated, prefer timestamps
+	configUpdateTime *time.Time
+	configApplyTime  *time.Time
+	revalUpdateTime  *time.Time
+	revalApplyTime   *time.Time
+}
+
+func parseQueryParams(params map[string]string) (*updateValues, error) {
+	var paramValues updateValues
+
+	// Verify query string parameters
+	configUpdatedBoolParam, hasConfigUpdatedBoolParam := params["updated"]     // Deprecated, but still required for backwards compatibility
+	revalUpdatedBoolParam, hasRevalUpdatedBoolParam := params["reval_updated"] // Deprecated, but still required for backwards compatibility
+	configUpdateTimeParam, hasConfigUpdateTimeParam := params["config_update_time"]
+	revalidateUpdateTimeParam, hasRevalidateUpdateTimeParam := params["revalidate_update_time"]
+	configApplyTimeParam, hasConfigApplyTimeParam := params["config_apply_time"]
+	revalidateApplyTimeParam, hasRevalidateApplyTimeParam := params["revalidate_apply_time"]
+
+	if !hasConfigApplyTimeParam && !hasRevalidateApplyTimeParam &&
+		!hasConfigUpdateTimeParam && !hasRevalidateUpdateTimeParam &&
+		!hasConfigUpdatedBoolParam && !hasRevalUpdatedBoolParam {
+		return nil, errors.New("must pass at least one query parameter: 'config_apply_time', 'revalidate_apply_time', 'config_update_time', 'revalidate_update_time' (may also pass bool `update` `reval_updated`)")

Review Comment:
   we probably shouldn't use backticks in error messages, safer to stick with single quotes



##########
traffic_ops/traffic_ops_golang/server/update.go:
##########
@@ -112,42 +112,161 @@ func UpdateHandler(w http.ResponseWriter, r *http.Request) {
 		return !strings.HasPrefix(strings.ToLower(s), "f")
 	}
 
-	updatedPtr := (*bool)(nil)
+	values := new(updateValues)
+
 	if hasUpdated {
 		updatedBool := strToBool(updated)
-		updatedPtr = &updatedBool
+		values.configUpdateBool = &updatedBool
 	}
-	revalUpdatedPtr := (*bool)(nil)
+
 	if hasRevalUpdated {
 		revalUpdatedBool := strToBool(revalUpdated)
-		revalUpdatedPtr = &revalUpdatedBool
+		values.revalUpdateBool = &revalUpdatedBool
 	}
 
-	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), updatedPtr, revalUpdatedPtr); err != nil {
+	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), *values); err != nil {
 		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("setting updated statuses: "+err.Error()))
 		return
 	}
 
 	respMsg := "successfully set server '" + hostName + "'"
 	if hasUpdated {
-		respMsg += " updated=" + strconv.FormatBool(strToBool(updated))
+		respMsg += " updated=" + updated
 	}
 	if hasRevalUpdated {
-		respMsg += " reval_updated=" + strconv.FormatBool(strToBool(revalUpdated))
+		respMsg += " reval_updated=" + revalUpdated
 	}
 
 	api.WriteAlerts(w, r, http.StatusOK, tc.CreateAlerts(tc.SuccessLevel, respMsg))
 }
 
-// setUpdateStatuses sets the upd_pending and reval_pending columns of a server.
-// If updatePending or revalPending is nil, that value is not changed.
-func setUpdateStatuses(tx *sql.Tx, serverID int64, updatePending *bool, revalPending *bool) error {
-	if updatePending == nil && revalPending == nil {
-		return errors.New("either updatePending or revalPending must not be nil")
+type updateValues struct {
+	configUpdateBool *bool // Deprecated, prefer timestamps
+	revalUpdateBool  *bool // Deprecated, prefer timestamps
+	configUpdateTime *time.Time
+	configApplyTime  *time.Time
+	revalUpdateTime  *time.Time
+	revalApplyTime   *time.Time
+}
+
+func parseQueryParams(params map[string]string) (*updateValues, error) {
+	var paramValues updateValues
+
+	// Verify query string parameters
+	configUpdatedBoolParam, hasConfigUpdatedBoolParam := params["updated"]     // Deprecated, but still required for backwards compatibility
+	revalUpdatedBoolParam, hasRevalUpdatedBoolParam := params["reval_updated"] // Deprecated, but still required for backwards compatibility
+	configUpdateTimeParam, hasConfigUpdateTimeParam := params["config_update_time"]
+	revalidateUpdateTimeParam, hasRevalidateUpdateTimeParam := params["revalidate_update_time"]
+	configApplyTimeParam, hasConfigApplyTimeParam := params["config_apply_time"]
+	revalidateApplyTimeParam, hasRevalidateApplyTimeParam := params["revalidate_apply_time"]
+
+	if !hasConfigApplyTimeParam && !hasRevalidateApplyTimeParam &&
+		!hasConfigUpdateTimeParam && !hasRevalidateUpdateTimeParam &&
+		!hasConfigUpdatedBoolParam && !hasRevalUpdatedBoolParam {
+		return nil, errors.New("must pass at least one query parameter: 'config_apply_time', 'revalidate_apply_time', 'config_update_time', 'revalidate_update_time' (may also pass bool `update` `reval_updated`)")
+
+	}
+	// Prevent collision between booleans and timestamps
+	if (hasConfigUpdateTimeParam || hasConfigApplyTimeParam) && hasConfigUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `updated` along with either `config_update_time` or `config_apply_time`")
+
+	}
+	if (hasRevalidateUpdateTimeParam || hasRevalidateApplyTimeParam) && hasRevalUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `reval_updated` along with either `revalidate_update_time` or `revalidate_apply_time`")

Review Comment:
   we probably shouldn't use backticks in error messages, safer to stick with single quotes



##########
traffic_ops/traffic_ops_golang/server/update.go:
##########
@@ -112,42 +112,161 @@ func UpdateHandler(w http.ResponseWriter, r *http.Request) {
 		return !strings.HasPrefix(strings.ToLower(s), "f")
 	}
 
-	updatedPtr := (*bool)(nil)
+	values := new(updateValues)
+
 	if hasUpdated {
 		updatedBool := strToBool(updated)
-		updatedPtr = &updatedBool
+		values.configUpdateBool = &updatedBool
 	}
-	revalUpdatedPtr := (*bool)(nil)
+
 	if hasRevalUpdated {
 		revalUpdatedBool := strToBool(revalUpdated)
-		revalUpdatedPtr = &revalUpdatedBool
+		values.revalUpdateBool = &revalUpdatedBool
 	}
 
-	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), updatedPtr, revalUpdatedPtr); err != nil {
+	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), *values); err != nil {
 		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("setting updated statuses: "+err.Error()))
 		return
 	}
 
 	respMsg := "successfully set server '" + hostName + "'"
 	if hasUpdated {
-		respMsg += " updated=" + strconv.FormatBool(strToBool(updated))
+		respMsg += " updated=" + updated
 	}
 	if hasRevalUpdated {
-		respMsg += " reval_updated=" + strconv.FormatBool(strToBool(revalUpdated))
+		respMsg += " reval_updated=" + revalUpdated
 	}
 
 	api.WriteAlerts(w, r, http.StatusOK, tc.CreateAlerts(tc.SuccessLevel, respMsg))
 }
 
-// setUpdateStatuses sets the upd_pending and reval_pending columns of a server.
-// If updatePending or revalPending is nil, that value is not changed.
-func setUpdateStatuses(tx *sql.Tx, serverID int64, updatePending *bool, revalPending *bool) error {
-	if updatePending == nil && revalPending == nil {
-		return errors.New("either updatePending or revalPending must not be nil")
+type updateValues struct {
+	configUpdateBool *bool // Deprecated, prefer timestamps
+	revalUpdateBool  *bool // Deprecated, prefer timestamps
+	configUpdateTime *time.Time
+	configApplyTime  *time.Time
+	revalUpdateTime  *time.Time
+	revalApplyTime   *time.Time
+}
+
+func parseQueryParams(params map[string]string) (*updateValues, error) {
+	var paramValues updateValues
+
+	// Verify query string parameters
+	configUpdatedBoolParam, hasConfigUpdatedBoolParam := params["updated"]     // Deprecated, but still required for backwards compatibility
+	revalUpdatedBoolParam, hasRevalUpdatedBoolParam := params["reval_updated"] // Deprecated, but still required for backwards compatibility
+	configUpdateTimeParam, hasConfigUpdateTimeParam := params["config_update_time"]
+	revalidateUpdateTimeParam, hasRevalidateUpdateTimeParam := params["revalidate_update_time"]
+	configApplyTimeParam, hasConfigApplyTimeParam := params["config_apply_time"]
+	revalidateApplyTimeParam, hasRevalidateApplyTimeParam := params["revalidate_apply_time"]
+
+	if !hasConfigApplyTimeParam && !hasRevalidateApplyTimeParam &&
+		!hasConfigUpdateTimeParam && !hasRevalidateUpdateTimeParam &&
+		!hasConfigUpdatedBoolParam && !hasRevalUpdatedBoolParam {
+		return nil, errors.New("must pass at least one query parameter: 'config_apply_time', 'revalidate_apply_time', 'config_update_time', 'revalidate_update_time' (may also pass bool `update` `reval_updated`)")
+
+	}
+	// Prevent collision between booleans and timestamps
+	if (hasConfigUpdateTimeParam || hasConfigApplyTimeParam) && hasConfigUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `updated` along with either `config_update_time` or `config_apply_time`")
+
+	}
+	if (hasRevalidateUpdateTimeParam || hasRevalidateApplyTimeParam) && hasRevalUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `reval_updated` along with either `revalidate_update_time` or `revalidate_apply_time`")
+
+	}
+
+	// Validate and parse parameters before attempting to apply them (don't want to partially apply various status before an error)
+	// Timestamps
+	if hasConfigUpdateTimeParam {
+		configUpdateTime, err := time.Parse(time.RFC3339Nano, configUpdateTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'config_update_time' must be valid RFC3339Nano format")
+		}
+		paramValues.configUpdateTime = &configUpdateTime
+	}
+
+	if hasRevalidateUpdateTimeParam {
+		revalUpdateTime, err := time.Parse(time.RFC3339Nano, revalidateUpdateTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'revalidate_update_time' must be valid RFC3339Nano format")
+		}
+		paramValues.revalUpdateTime = &revalUpdateTime
+	}
+
+	if hasConfigApplyTimeParam {
+		configApplyTime, err := time.Parse(time.RFC3339Nano, configApplyTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'config_apply_time' must be valid RFC3339Nano format")
+		}
+		paramValues.configApplyTime = &configApplyTime
+	}
+
+	if hasRevalidateApplyTimeParam {
+		revalApplyTime, err := time.Parse(time.RFC3339Nano, revalidateApplyTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'revalidate_apply_time' must be valid RFC3339Nano format")
+		}
+		paramValues.revalApplyTime = &revalApplyTime
+	}
+
+	// Booleans
+	configUpdatedBool := strings.ToLower(configUpdatedBoolParam)
+	revalUpdatedBool := strings.ToLower(revalUpdatedBoolParam)
+
+	if hasConfigUpdatedBoolParam && configUpdatedBool != `t` && configUpdatedBool != `true` && configUpdatedBool != `f` && configUpdatedBool != `false` {
+		return nil, errors.New("query parameter 'updated' must be 'true' or 'false'")
+	}
+	if hasRevalUpdatedBoolParam && revalUpdatedBool != `t` && revalUpdatedBool != `true` && revalUpdatedBool != `f` && revalUpdatedBool != `false` {
+		return nil, errors.New("query parameter 'reval_updated' must be 'true' or 'false'")
+	}
+
+	strToBool := func(s string) bool {
+		return strings.HasPrefix(s, "t")
+	}
+
+	if hasConfigUpdatedBoolParam {
+		updateBool := strToBool(configUpdatedBool)
+		paramValues.configUpdateBool = &updateBool
+	}
+
+	if hasRevalUpdatedBoolParam {
+		revalUpdatedBool := strToBool(revalUpdatedBool)
+		paramValues.revalUpdateBool = &revalUpdatedBool
+	}
+	return &paramValues, nil
+}
+
+// setUpdateStatuses set timestamps for config update/apply and revalidation
+// update/apply. If any value is nil, no changes occur
+func setUpdateStatuses(tx *sql.Tx, serverID int64, values updateValues) error {
+
+	if values.configUpdateTime != nil {
+		if err := dbhelpers.QueueUpdateForServerWithTime(tx, serverID, *values.configUpdateTime); err != nil {
+			return fmt.Errorf("setting config apply time: %w", err)
+		}
+	}
+
+	if values.revalUpdateTime != nil {
+		if err := dbhelpers.QueueRevalForServerWithTime(tx, serverID, *values.revalUpdateTime); err != nil {
+			return fmt.Errorf("setting reval apply time: %w", err)
+		}
+	}

Review Comment:
   I thought the server (TO) was supposed to ignore these times? `t3c` wouldn't be sending the update_times anyways, right?



##########
cache-config/testing/ort-tests/tc-fixtures.json:
##########
@@ -1825,7 +1825,7 @@
           "lastUpdated": "2018-01-19T19:01:21.499423+00:00",
           "name": "trafficserver",
           "secure": true,
-          "value": "CHANGEME"
+          "value": "8.1.4-0.965df952e.el8.x86_64"

Review Comment:
   I'm not positive about this, but I think this is meant to be `CHANGEME` so that you can overwrite it with whatever version you're trying to test?



##########
traffic_ops/traffic_ops_golang/server/update.go:
##########
@@ -112,42 +112,161 @@ func UpdateHandler(w http.ResponseWriter, r *http.Request) {
 		return !strings.HasPrefix(strings.ToLower(s), "f")
 	}
 
-	updatedPtr := (*bool)(nil)
+	values := new(updateValues)
+
 	if hasUpdated {
 		updatedBool := strToBool(updated)
-		updatedPtr = &updatedBool
+		values.configUpdateBool = &updatedBool
 	}
-	revalUpdatedPtr := (*bool)(nil)
+
 	if hasRevalUpdated {
 		revalUpdatedBool := strToBool(revalUpdated)
-		revalUpdatedPtr = &revalUpdatedBool
+		values.revalUpdateBool = &revalUpdatedBool
 	}
 
-	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), updatedPtr, revalUpdatedPtr); err != nil {
+	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), *values); err != nil {
 		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("setting updated statuses: "+err.Error()))
 		return
 	}
 
 	respMsg := "successfully set server '" + hostName + "'"
 	if hasUpdated {
-		respMsg += " updated=" + strconv.FormatBool(strToBool(updated))
+		respMsg += " updated=" + updated
 	}
 	if hasRevalUpdated {
-		respMsg += " reval_updated=" + strconv.FormatBool(strToBool(revalUpdated))
+		respMsg += " reval_updated=" + revalUpdated
 	}
 
 	api.WriteAlerts(w, r, http.StatusOK, tc.CreateAlerts(tc.SuccessLevel, respMsg))
 }
 
-// setUpdateStatuses sets the upd_pending and reval_pending columns of a server.
-// If updatePending or revalPending is nil, that value is not changed.
-func setUpdateStatuses(tx *sql.Tx, serverID int64, updatePending *bool, revalPending *bool) error {
-	if updatePending == nil && revalPending == nil {
-		return errors.New("either updatePending or revalPending must not be nil")
+type updateValues struct {
+	configUpdateBool *bool // Deprecated, prefer timestamps
+	revalUpdateBool  *bool // Deprecated, prefer timestamps
+	configUpdateTime *time.Time
+	configApplyTime  *time.Time
+	revalUpdateTime  *time.Time
+	revalApplyTime   *time.Time
+}
+
+func parseQueryParams(params map[string]string) (*updateValues, error) {
+	var paramValues updateValues
+
+	// Verify query string parameters
+	configUpdatedBoolParam, hasConfigUpdatedBoolParam := params["updated"]     // Deprecated, but still required for backwards compatibility
+	revalUpdatedBoolParam, hasRevalUpdatedBoolParam := params["reval_updated"] // Deprecated, but still required for backwards compatibility
+	configUpdateTimeParam, hasConfigUpdateTimeParam := params["config_update_time"]
+	revalidateUpdateTimeParam, hasRevalidateUpdateTimeParam := params["revalidate_update_time"]
+	configApplyTimeParam, hasConfigApplyTimeParam := params["config_apply_time"]
+	revalidateApplyTimeParam, hasRevalidateApplyTimeParam := params["revalidate_apply_time"]
+
+	if !hasConfigApplyTimeParam && !hasRevalidateApplyTimeParam &&
+		!hasConfigUpdateTimeParam && !hasRevalidateUpdateTimeParam &&
+		!hasConfigUpdatedBoolParam && !hasRevalUpdatedBoolParam {
+		return nil, errors.New("must pass at least one query parameter: 'config_apply_time', 'revalidate_apply_time', 'config_update_time', 'revalidate_update_time' (may also pass bool `update` `reval_updated`)")
+
+	}
+	// Prevent collision between booleans and timestamps
+	if (hasConfigUpdateTimeParam || hasConfigApplyTimeParam) && hasConfigUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `updated` along with either `config_update_time` or `config_apply_time`")
+
+	}
+	if (hasRevalidateUpdateTimeParam || hasRevalidateApplyTimeParam) && hasRevalUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `reval_updated` along with either `revalidate_update_time` or `revalidate_apply_time`")
+
+	}
+
+	// Validate and parse parameters before attempting to apply them (don't want to partially apply various status before an error)
+	// Timestamps
+	if hasConfigUpdateTimeParam {
+		configUpdateTime, err := time.Parse(time.RFC3339Nano, configUpdateTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'config_update_time' must be valid RFC3339Nano format")
+		}
+		paramValues.configUpdateTime = &configUpdateTime
+	}
+
+	if hasRevalidateUpdateTimeParam {
+		revalUpdateTime, err := time.Parse(time.RFC3339Nano, revalidateUpdateTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'revalidate_update_time' must be valid RFC3339Nano format")

Review Comment:
   we should include the original parsing error in this message -- would be helpful to users 



##########
traffic_ops/traffic_ops_golang/server/update.go:
##########
@@ -112,42 +112,161 @@ func UpdateHandler(w http.ResponseWriter, r *http.Request) {
 		return !strings.HasPrefix(strings.ToLower(s), "f")
 	}
 
-	updatedPtr := (*bool)(nil)
+	values := new(updateValues)
+
 	if hasUpdated {
 		updatedBool := strToBool(updated)
-		updatedPtr = &updatedBool
+		values.configUpdateBool = &updatedBool
 	}
-	revalUpdatedPtr := (*bool)(nil)
+
 	if hasRevalUpdated {
 		revalUpdatedBool := strToBool(revalUpdated)
-		revalUpdatedPtr = &revalUpdatedBool
+		values.revalUpdateBool = &revalUpdatedBool
 	}
 
-	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), updatedPtr, revalUpdatedPtr); err != nil {
+	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), *values); err != nil {
 		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("setting updated statuses: "+err.Error()))
 		return
 	}
 
 	respMsg := "successfully set server '" + hostName + "'"
 	if hasUpdated {
-		respMsg += " updated=" + strconv.FormatBool(strToBool(updated))
+		respMsg += " updated=" + updated
 	}
 	if hasRevalUpdated {
-		respMsg += " reval_updated=" + strconv.FormatBool(strToBool(revalUpdated))
+		respMsg += " reval_updated=" + revalUpdated
 	}
 
 	api.WriteAlerts(w, r, http.StatusOK, tc.CreateAlerts(tc.SuccessLevel, respMsg))
 }
 
-// setUpdateStatuses sets the upd_pending and reval_pending columns of a server.
-// If updatePending or revalPending is nil, that value is not changed.
-func setUpdateStatuses(tx *sql.Tx, serverID int64, updatePending *bool, revalPending *bool) error {
-	if updatePending == nil && revalPending == nil {
-		return errors.New("either updatePending or revalPending must not be nil")
+type updateValues struct {
+	configUpdateBool *bool // Deprecated, prefer timestamps
+	revalUpdateBool  *bool // Deprecated, prefer timestamps
+	configUpdateTime *time.Time
+	configApplyTime  *time.Time
+	revalUpdateTime  *time.Time
+	revalApplyTime   *time.Time
+}
+
+func parseQueryParams(params map[string]string) (*updateValues, error) {
+	var paramValues updateValues
+
+	// Verify query string parameters
+	configUpdatedBoolParam, hasConfigUpdatedBoolParam := params["updated"]     // Deprecated, but still required for backwards compatibility
+	revalUpdatedBoolParam, hasRevalUpdatedBoolParam := params["reval_updated"] // Deprecated, but still required for backwards compatibility
+	configUpdateTimeParam, hasConfigUpdateTimeParam := params["config_update_time"]
+	revalidateUpdateTimeParam, hasRevalidateUpdateTimeParam := params["revalidate_update_time"]
+	configApplyTimeParam, hasConfigApplyTimeParam := params["config_apply_time"]
+	revalidateApplyTimeParam, hasRevalidateApplyTimeParam := params["revalidate_apply_time"]
+
+	if !hasConfigApplyTimeParam && !hasRevalidateApplyTimeParam &&
+		!hasConfigUpdateTimeParam && !hasRevalidateUpdateTimeParam &&
+		!hasConfigUpdatedBoolParam && !hasRevalUpdatedBoolParam {
+		return nil, errors.New("must pass at least one query parameter: 'config_apply_time', 'revalidate_apply_time', 'config_update_time', 'revalidate_update_time' (may also pass bool `update` `reval_updated`)")
+
+	}
+	// Prevent collision between booleans and timestamps
+	if (hasConfigUpdateTimeParam || hasConfigApplyTimeParam) && hasConfigUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `updated` along with either `config_update_time` or `config_apply_time`")
+
+	}
+	if (hasRevalidateUpdateTimeParam || hasRevalidateApplyTimeParam) && hasRevalUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `reval_updated` along with either `revalidate_update_time` or `revalidate_apply_time`")
+
+	}
+
+	// Validate and parse parameters before attempting to apply them (don't want to partially apply various status before an error)
+	// Timestamps
+	if hasConfigUpdateTimeParam {
+		configUpdateTime, err := time.Parse(time.RFC3339Nano, configUpdateTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'config_update_time' must be valid RFC3339Nano format")

Review Comment:
   we should include the original parsing error in this message -- would be helpful to users 



##########
cache-config/testing/ort-tests/tc-fixtures.json:
##########
@@ -2222,7 +2222,7 @@
           "lastUpdated": "2018-01-19T19:01:21.499423+00:00",
           "name": "trafficserver",
           "secure": true,
-          "value": "CHANGEME"
+          "value": "8.1.4-0.965df952e.el8.x86_64"

Review Comment:
   I'm not positive about this, but I think this is meant to be `CHANGEME` so that you can overwrite it with whatever version you're trying to test?



##########
traffic_ops/traffic_ops_golang/server/update.go:
##########
@@ -112,42 +112,161 @@ func UpdateHandler(w http.ResponseWriter, r *http.Request) {
 		return !strings.HasPrefix(strings.ToLower(s), "f")
 	}
 
-	updatedPtr := (*bool)(nil)
+	values := new(updateValues)
+
 	if hasUpdated {
 		updatedBool := strToBool(updated)
-		updatedPtr = &updatedBool
+		values.configUpdateBool = &updatedBool
 	}
-	revalUpdatedPtr := (*bool)(nil)
+
 	if hasRevalUpdated {
 		revalUpdatedBool := strToBool(revalUpdated)
-		revalUpdatedPtr = &revalUpdatedBool
+		values.revalUpdateBool = &revalUpdatedBool
 	}
 
-	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), updatedPtr, revalUpdatedPtr); err != nil {
+	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), *values); err != nil {
 		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("setting updated statuses: "+err.Error()))
 		return
 	}
 
 	respMsg := "successfully set server '" + hostName + "'"
 	if hasUpdated {
-		respMsg += " updated=" + strconv.FormatBool(strToBool(updated))
+		respMsg += " updated=" + updated
 	}
 	if hasRevalUpdated {
-		respMsg += " reval_updated=" + strconv.FormatBool(strToBool(revalUpdated))
+		respMsg += " reval_updated=" + revalUpdated
 	}
 
 	api.WriteAlerts(w, r, http.StatusOK, tc.CreateAlerts(tc.SuccessLevel, respMsg))
 }
 
-// setUpdateStatuses sets the upd_pending and reval_pending columns of a server.
-// If updatePending or revalPending is nil, that value is not changed.
-func setUpdateStatuses(tx *sql.Tx, serverID int64, updatePending *bool, revalPending *bool) error {
-	if updatePending == nil && revalPending == nil {
-		return errors.New("either updatePending or revalPending must not be nil")
+type updateValues struct {
+	configUpdateBool *bool // Deprecated, prefer timestamps
+	revalUpdateBool  *bool // Deprecated, prefer timestamps
+	configUpdateTime *time.Time
+	configApplyTime  *time.Time
+	revalUpdateTime  *time.Time
+	revalApplyTime   *time.Time
+}
+
+func parseQueryParams(params map[string]string) (*updateValues, error) {
+	var paramValues updateValues
+
+	// Verify query string parameters
+	configUpdatedBoolParam, hasConfigUpdatedBoolParam := params["updated"]     // Deprecated, but still required for backwards compatibility
+	revalUpdatedBoolParam, hasRevalUpdatedBoolParam := params["reval_updated"] // Deprecated, but still required for backwards compatibility
+	configUpdateTimeParam, hasConfigUpdateTimeParam := params["config_update_time"]
+	revalidateUpdateTimeParam, hasRevalidateUpdateTimeParam := params["revalidate_update_time"]
+	configApplyTimeParam, hasConfigApplyTimeParam := params["config_apply_time"]
+	revalidateApplyTimeParam, hasRevalidateApplyTimeParam := params["revalidate_apply_time"]
+
+	if !hasConfigApplyTimeParam && !hasRevalidateApplyTimeParam &&
+		!hasConfigUpdateTimeParam && !hasRevalidateUpdateTimeParam &&
+		!hasConfigUpdatedBoolParam && !hasRevalUpdatedBoolParam {
+		return nil, errors.New("must pass at least one query parameter: 'config_apply_time', 'revalidate_apply_time', 'config_update_time', 'revalidate_update_time' (may also pass bool `update` `reval_updated`)")
+
+	}
+	// Prevent collision between booleans and timestamps
+	if (hasConfigUpdateTimeParam || hasConfigApplyTimeParam) && hasConfigUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `updated` along with either `config_update_time` or `config_apply_time`")

Review Comment:
   we probably shouldn't use backticks in error messages, safer to stick with single quotes



##########
traffic_ops/traffic_ops_golang/server/update.go:
##########
@@ -112,42 +112,161 @@ func UpdateHandler(w http.ResponseWriter, r *http.Request) {
 		return !strings.HasPrefix(strings.ToLower(s), "f")
 	}
 
-	updatedPtr := (*bool)(nil)
+	values := new(updateValues)
+
 	if hasUpdated {
 		updatedBool := strToBool(updated)
-		updatedPtr = &updatedBool
+		values.configUpdateBool = &updatedBool
 	}
-	revalUpdatedPtr := (*bool)(nil)
+
 	if hasRevalUpdated {
 		revalUpdatedBool := strToBool(revalUpdated)
-		revalUpdatedPtr = &revalUpdatedBool
+		values.revalUpdateBool = &revalUpdatedBool
 	}
 
-	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), updatedPtr, revalUpdatedPtr); err != nil {
+	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), *values); err != nil {
 		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("setting updated statuses: "+err.Error()))
 		return
 	}
 
 	respMsg := "successfully set server '" + hostName + "'"
 	if hasUpdated {
-		respMsg += " updated=" + strconv.FormatBool(strToBool(updated))
+		respMsg += " updated=" + updated
 	}
 	if hasRevalUpdated {
-		respMsg += " reval_updated=" + strconv.FormatBool(strToBool(revalUpdated))
+		respMsg += " reval_updated=" + revalUpdated
 	}
 
 	api.WriteAlerts(w, r, http.StatusOK, tc.CreateAlerts(tc.SuccessLevel, respMsg))
 }
 
-// setUpdateStatuses sets the upd_pending and reval_pending columns of a server.
-// If updatePending or revalPending is nil, that value is not changed.
-func setUpdateStatuses(tx *sql.Tx, serverID int64, updatePending *bool, revalPending *bool) error {
-	if updatePending == nil && revalPending == nil {
-		return errors.New("either updatePending or revalPending must not be nil")
+type updateValues struct {
+	configUpdateBool *bool // Deprecated, prefer timestamps
+	revalUpdateBool  *bool // Deprecated, prefer timestamps
+	configUpdateTime *time.Time
+	configApplyTime  *time.Time
+	revalUpdateTime  *time.Time
+	revalApplyTime   *time.Time
+}
+
+func parseQueryParams(params map[string]string) (*updateValues, error) {
+	var paramValues updateValues
+
+	// Verify query string parameters
+	configUpdatedBoolParam, hasConfigUpdatedBoolParam := params["updated"]     // Deprecated, but still required for backwards compatibility
+	revalUpdatedBoolParam, hasRevalUpdatedBoolParam := params["reval_updated"] // Deprecated, but still required for backwards compatibility
+	configUpdateTimeParam, hasConfigUpdateTimeParam := params["config_update_time"]
+	revalidateUpdateTimeParam, hasRevalidateUpdateTimeParam := params["revalidate_update_time"]
+	configApplyTimeParam, hasConfigApplyTimeParam := params["config_apply_time"]
+	revalidateApplyTimeParam, hasRevalidateApplyTimeParam := params["revalidate_apply_time"]
+
+	if !hasConfigApplyTimeParam && !hasRevalidateApplyTimeParam &&
+		!hasConfigUpdateTimeParam && !hasRevalidateUpdateTimeParam &&
+		!hasConfigUpdatedBoolParam && !hasRevalUpdatedBoolParam {
+		return nil, errors.New("must pass at least one query parameter: 'config_apply_time', 'revalidate_apply_time', 'config_update_time', 'revalidate_update_time' (may also pass bool `update` `reval_updated`)")
+
+	}
+	// Prevent collision between booleans and timestamps
+	if (hasConfigUpdateTimeParam || hasConfigApplyTimeParam) && hasConfigUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `updated` along with either `config_update_time` or `config_apply_time`")
+
+	}
+	if (hasRevalidateUpdateTimeParam || hasRevalidateApplyTimeParam) && hasRevalUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `reval_updated` along with either `revalidate_update_time` or `revalidate_apply_time`")
+
+	}
+
+	// Validate and parse parameters before attempting to apply them (don't want to partially apply various status before an error)
+	// Timestamps
+	if hasConfigUpdateTimeParam {
+		configUpdateTime, err := time.Parse(time.RFC3339Nano, configUpdateTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'config_update_time' must be valid RFC3339Nano format")
+		}
+		paramValues.configUpdateTime = &configUpdateTime
+	}
+
+	if hasRevalidateUpdateTimeParam {
+		revalUpdateTime, err := time.Parse(time.RFC3339Nano, revalidateUpdateTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'revalidate_update_time' must be valid RFC3339Nano format")
+		}
+		paramValues.revalUpdateTime = &revalUpdateTime
+	}
+
+	if hasConfigApplyTimeParam {
+		configApplyTime, err := time.Parse(time.RFC3339Nano, configApplyTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'config_apply_time' must be valid RFC3339Nano format")
+		}
+		paramValues.configApplyTime = &configApplyTime
+	}
+
+	if hasRevalidateApplyTimeParam {
+		revalApplyTime, err := time.Parse(time.RFC3339Nano, revalidateApplyTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'revalidate_apply_time' must be valid RFC3339Nano format")
+		}
+		paramValues.revalApplyTime = &revalApplyTime
+	}
+
+	// Booleans
+	configUpdatedBool := strings.ToLower(configUpdatedBoolParam)
+	revalUpdatedBool := strings.ToLower(revalUpdatedBoolParam)
+
+	if hasConfigUpdatedBoolParam && configUpdatedBool != `t` && configUpdatedBool != `true` && configUpdatedBool != `f` && configUpdatedBool != `false` {
+		return nil, errors.New("query parameter 'updated' must be 'true' or 'false'")
+	}
+	if hasRevalUpdatedBoolParam && revalUpdatedBool != `t` && revalUpdatedBool != `true` && revalUpdatedBool != `f` && revalUpdatedBool != `false` {

Review Comment:
   nit: we should probably just use `strconv.ParseBool` here instead of checking these 4 strings specifically



##########
traffic_ops/traffic_ops_golang/server/update.go:
##########
@@ -112,42 +112,161 @@ func UpdateHandler(w http.ResponseWriter, r *http.Request) {
 		return !strings.HasPrefix(strings.ToLower(s), "f")
 	}
 
-	updatedPtr := (*bool)(nil)
+	values := new(updateValues)
+
 	if hasUpdated {
 		updatedBool := strToBool(updated)
-		updatedPtr = &updatedBool
+		values.configUpdateBool = &updatedBool
 	}
-	revalUpdatedPtr := (*bool)(nil)
+
 	if hasRevalUpdated {
 		revalUpdatedBool := strToBool(revalUpdated)
-		revalUpdatedPtr = &revalUpdatedBool
+		values.revalUpdateBool = &revalUpdatedBool
 	}
 
-	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), updatedPtr, revalUpdatedPtr); err != nil {
+	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), *values); err != nil {
 		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("setting updated statuses: "+err.Error()))
 		return
 	}
 
 	respMsg := "successfully set server '" + hostName + "'"
 	if hasUpdated {
-		respMsg += " updated=" + strconv.FormatBool(strToBool(updated))
+		respMsg += " updated=" + updated
 	}
 	if hasRevalUpdated {
-		respMsg += " reval_updated=" + strconv.FormatBool(strToBool(revalUpdated))
+		respMsg += " reval_updated=" + revalUpdated
 	}
 
 	api.WriteAlerts(w, r, http.StatusOK, tc.CreateAlerts(tc.SuccessLevel, respMsg))
 }
 
-// setUpdateStatuses sets the upd_pending and reval_pending columns of a server.
-// If updatePending or revalPending is nil, that value is not changed.
-func setUpdateStatuses(tx *sql.Tx, serverID int64, updatePending *bool, revalPending *bool) error {
-	if updatePending == nil && revalPending == nil {
-		return errors.New("either updatePending or revalPending must not be nil")
+type updateValues struct {
+	configUpdateBool *bool // Deprecated, prefer timestamps
+	revalUpdateBool  *bool // Deprecated, prefer timestamps
+	configUpdateTime *time.Time
+	configApplyTime  *time.Time
+	revalUpdateTime  *time.Time
+	revalApplyTime   *time.Time
+}
+
+func parseQueryParams(params map[string]string) (*updateValues, error) {
+	var paramValues updateValues
+
+	// Verify query string parameters
+	configUpdatedBoolParam, hasConfigUpdatedBoolParam := params["updated"]     // Deprecated, but still required for backwards compatibility
+	revalUpdatedBoolParam, hasRevalUpdatedBoolParam := params["reval_updated"] // Deprecated, but still required for backwards compatibility
+	configUpdateTimeParam, hasConfigUpdateTimeParam := params["config_update_time"]
+	revalidateUpdateTimeParam, hasRevalidateUpdateTimeParam := params["revalidate_update_time"]
+	configApplyTimeParam, hasConfigApplyTimeParam := params["config_apply_time"]
+	revalidateApplyTimeParam, hasRevalidateApplyTimeParam := params["revalidate_apply_time"]
+
+	if !hasConfigApplyTimeParam && !hasRevalidateApplyTimeParam &&
+		!hasConfigUpdateTimeParam && !hasRevalidateUpdateTimeParam &&
+		!hasConfigUpdatedBoolParam && !hasRevalUpdatedBoolParam {
+		return nil, errors.New("must pass at least one query parameter: 'config_apply_time', 'revalidate_apply_time', 'config_update_time', 'revalidate_update_time' (may also pass bool `update` `reval_updated`)")
+
+	}
+	// Prevent collision between booleans and timestamps
+	if (hasConfigUpdateTimeParam || hasConfigApplyTimeParam) && hasConfigUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `updated` along with either `config_update_time` or `config_apply_time`")
+
+	}
+	if (hasRevalidateUpdateTimeParam || hasRevalidateApplyTimeParam) && hasRevalUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `reval_updated` along with either `revalidate_update_time` or `revalidate_apply_time`")
+
+	}
+
+	// Validate and parse parameters before attempting to apply them (don't want to partially apply various status before an error)
+	// Timestamps
+	if hasConfigUpdateTimeParam {
+		configUpdateTime, err := time.Parse(time.RFC3339Nano, configUpdateTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'config_update_time' must be valid RFC3339Nano format")
+		}
+		paramValues.configUpdateTime = &configUpdateTime
+	}
+
+	if hasRevalidateUpdateTimeParam {
+		revalUpdateTime, err := time.Parse(time.RFC3339Nano, revalidateUpdateTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'revalidate_update_time' must be valid RFC3339Nano format")
+		}
+		paramValues.revalUpdateTime = &revalUpdateTime
+	}
+
+	if hasConfigApplyTimeParam {
+		configApplyTime, err := time.Parse(time.RFC3339Nano, configApplyTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'config_apply_time' must be valid RFC3339Nano format")
+		}
+		paramValues.configApplyTime = &configApplyTime
+	}
+
+	if hasRevalidateApplyTimeParam {
+		revalApplyTime, err := time.Parse(time.RFC3339Nano, revalidateApplyTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'revalidate_apply_time' must be valid RFC3339Nano format")

Review Comment:
   we should include the original parsing error in this message -- would be helpful to users 



##########
traffic_ops/traffic_ops_golang/server/update.go:
##########
@@ -112,42 +112,161 @@ func UpdateHandler(w http.ResponseWriter, r *http.Request) {
 		return !strings.HasPrefix(strings.ToLower(s), "f")
 	}
 
-	updatedPtr := (*bool)(nil)
+	values := new(updateValues)
+
 	if hasUpdated {
 		updatedBool := strToBool(updated)
-		updatedPtr = &updatedBool
+		values.configUpdateBool = &updatedBool
 	}
-	revalUpdatedPtr := (*bool)(nil)
+
 	if hasRevalUpdated {
 		revalUpdatedBool := strToBool(revalUpdated)
-		revalUpdatedPtr = &revalUpdatedBool
+		values.revalUpdateBool = &revalUpdatedBool
 	}
 
-	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), updatedPtr, revalUpdatedPtr); err != nil {
+	if err := setUpdateStatuses(inf.Tx.Tx, int64(serverID), *values); err != nil {
 		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("setting updated statuses: "+err.Error()))
 		return
 	}
 
 	respMsg := "successfully set server '" + hostName + "'"
 	if hasUpdated {
-		respMsg += " updated=" + strconv.FormatBool(strToBool(updated))
+		respMsg += " updated=" + updated
 	}
 	if hasRevalUpdated {
-		respMsg += " reval_updated=" + strconv.FormatBool(strToBool(revalUpdated))
+		respMsg += " reval_updated=" + revalUpdated
 	}
 
 	api.WriteAlerts(w, r, http.StatusOK, tc.CreateAlerts(tc.SuccessLevel, respMsg))
 }
 
-// setUpdateStatuses sets the upd_pending and reval_pending columns of a server.
-// If updatePending or revalPending is nil, that value is not changed.
-func setUpdateStatuses(tx *sql.Tx, serverID int64, updatePending *bool, revalPending *bool) error {
-	if updatePending == nil && revalPending == nil {
-		return errors.New("either updatePending or revalPending must not be nil")
+type updateValues struct {
+	configUpdateBool *bool // Deprecated, prefer timestamps
+	revalUpdateBool  *bool // Deprecated, prefer timestamps
+	configUpdateTime *time.Time
+	configApplyTime  *time.Time
+	revalUpdateTime  *time.Time
+	revalApplyTime   *time.Time
+}
+
+func parseQueryParams(params map[string]string) (*updateValues, error) {
+	var paramValues updateValues
+
+	// Verify query string parameters
+	configUpdatedBoolParam, hasConfigUpdatedBoolParam := params["updated"]     // Deprecated, but still required for backwards compatibility
+	revalUpdatedBoolParam, hasRevalUpdatedBoolParam := params["reval_updated"] // Deprecated, but still required for backwards compatibility
+	configUpdateTimeParam, hasConfigUpdateTimeParam := params["config_update_time"]
+	revalidateUpdateTimeParam, hasRevalidateUpdateTimeParam := params["revalidate_update_time"]
+	configApplyTimeParam, hasConfigApplyTimeParam := params["config_apply_time"]
+	revalidateApplyTimeParam, hasRevalidateApplyTimeParam := params["revalidate_apply_time"]
+
+	if !hasConfigApplyTimeParam && !hasRevalidateApplyTimeParam &&
+		!hasConfigUpdateTimeParam && !hasRevalidateUpdateTimeParam &&
+		!hasConfigUpdatedBoolParam && !hasRevalUpdatedBoolParam {
+		return nil, errors.New("must pass at least one query parameter: 'config_apply_time', 'revalidate_apply_time', 'config_update_time', 'revalidate_update_time' (may also pass bool `update` `reval_updated`)")
+
+	}
+	// Prevent collision between booleans and timestamps
+	if (hasConfigUpdateTimeParam || hasConfigApplyTimeParam) && hasConfigUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `updated` along with either `config_update_time` or `config_apply_time`")
+
+	}
+	if (hasRevalidateUpdateTimeParam || hasRevalidateApplyTimeParam) && hasRevalUpdatedBoolParam {
+		return nil, errors.New("conflicting parameters. may not pass `reval_updated` along with either `revalidate_update_time` or `revalidate_apply_time`")
+
+	}
+
+	// Validate and parse parameters before attempting to apply them (don't want to partially apply various status before an error)
+	// Timestamps
+	if hasConfigUpdateTimeParam {
+		configUpdateTime, err := time.Parse(time.RFC3339Nano, configUpdateTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'config_update_time' must be valid RFC3339Nano format")
+		}
+		paramValues.configUpdateTime = &configUpdateTime
+	}
+
+	if hasRevalidateUpdateTimeParam {
+		revalUpdateTime, err := time.Parse(time.RFC3339Nano, revalidateUpdateTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'revalidate_update_time' must be valid RFC3339Nano format")
+		}
+		paramValues.revalUpdateTime = &revalUpdateTime
+	}
+
+	if hasConfigApplyTimeParam {
+		configApplyTime, err := time.Parse(time.RFC3339Nano, configApplyTimeParam)
+		if err != nil {
+			return nil, errors.New("query parameter 'config_apply_time' must be valid RFC3339Nano format")

Review Comment:
   we should include the original parsing error in this message -- would be helpful to users 



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org