rob05c commented on a change in pull request #3104: Add monitoring.json
snapshotting
URL: https://github.com/apache/trafficcontrol/pull/3104#discussion_r240659712
##########
File path: traffic_ops/traffic_ops_golang/routes.go
##########
@@ -142,7 +141,7 @@ func Routes(d ServerData) ([]Route, []RawRoute,
http.Handler, error) {
{1.4, http.MethodGet, `cdns/dnsseckeys/refresh/?(\.json)?$`,
cdn.RefreshDNSSECKeys, auth.PrivLevelOperations, Authenticated, nil},
//CDN: Monitoring: Traffic Monitor
- {1.1, http.MethodGet,
`cdns/{cdn}/configs/monitoring(\.json)?$`, monitoring.Get,
auth.PrivLevelReadOnly, Authenticated, nil},
+ {1.1, http.MethodGet,
`cdns/{cdn}/configs/monitoring(\.json)?$`,
crconfig.SnapshotGetMonitoringHandler, auth.PrivLevelReadOnly, Authenticated,
nil},
Review comment:
It does change the behavior, because the existing behavior is fundamentally
a bug, and it's never correct to use non-snapshotted data for monitoring.
>do you feel that there is value in knowing what the non-snapped Monitor
config would look like? Seems to me like we should have two seperate end
points, one for the monitoring snapshot and one for the raw monitoring.config
file.
I think that sounds good in theory, but in practice, I'm very concerned that
someone might use the data, and it's always wrong to use it for anything
practical. The monitoring data must always be synchronized with the CRConfig
snapshot, or the race conditions in #1738 will happen.
But I can see the use case, e.g. for GUIs like the Portal to be able to
present a diff. The CRConfig has a "raw" endpoint at
`/cdns/{cdn}/snapshot/new`. Following that convention, what if we add a new
un-snapshotted endpoint at `cdns/{cdn}/configs/monitoring/new`, and extensively
document in the API Documentation, above the route in the `routes.go` file, and
a GoDoc comment over the function itself, that it MUST NOT ever be used for any
logic decisions, but only for presenting users with a comparison. How does that
sound?
I understand the use case, but it really worries me if anyone were ever to
use it for anything, they'd reintroduce the subtle and dangerous bug.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
