TaylorCFrey commented on a change in pull request #5492:
URL: https://github.com/apache/trafficcontrol/pull/5492#discussion_r570314639
##########
File path: traffic_monitor/towrap/towrap.go
##########
@@ -352,7 +354,9 @@ func (s TrafficOpsSessionThreadsafe) CRConfigRaw(cdn
string) ([]byte, error) {
b, reqInf, e := ss.GetCRConfig(cdn)
err = e
data = b
- remoteAddr = reqInf.RemoteAddr.String()
+ if reqInf.RemoteAddr != nil {
+ remoteAddr = reqInf.RemoteAddr.String()
Review comment:
This code looks good to me, but what happens if we don't provide a value
for `remoteAddr` later on down the line? According to the GitHub issue this
will likely fix that particular panic, but do we end up with a panic later
since the `remoteAddr` is potentially nil? Should we give it an empty string or
use `localHostIP` like it does later if there was an error?
##########
File path: traffic_monitor/towrap/towrap.go
##########
@@ -343,7 +343,9 @@ func (s TrafficOpsSessionThreadsafe) CRConfigRaw(cdn
string) ([]byte, error) {
b, reqInf, e := ss.GetCRConfig(cdn)
err = e
data = b
- remoteAddr = reqInf.RemoteAddr.String()
+ if reqInf.RemoteAddr != nil {
+ remoteAddr = reqInf.RemoteAddr.String()
Review comment:
Should we log the error here and below? If there's an error, should we
try to recover or let it fall through?
----------------------------------------------------------------
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]
