You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2021/12/22 22:34:14 UTC
[GitHub] [trafficcontrol] srijeet0406 opened a new pull request #6443: TM should not overwrite monitoring snapshot data with crconfig snapsot data
srijeet0406 opened a new pull request #6443:
URL: https://github.com/apache/trafficcontrol/pull/6443
<!--
Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
If this closes or relates to an existing issue, please reference it using one of the following:
Closes: #6376
Related: #ISSUE
If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://apache.org/security regarding vulnerability disclosure.
-->
This PR closes https://github.com/apache/trafficcontrol/issues/6376
<!-- **^ Add meaningful description above** --><hr>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
Feel free to add the name of a tool or script that is affected but not on the list.
-->
- Traffic Monitor
- Traffic Ops
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your PR.
If your PR has tests (and most should), provide the steps needed to run the tests.
If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
Make sure all the tests run. Follow the steps in https://github.com/apache/trafficcontrol/issues/6376 and make sure you see the expected results.
## If this is a bugfix, which Traffic Control versions contained the bug?
<!-- Delete this section if the PR is not a bugfix, or if the bug is only in the master branch.
Examples:
- 5.1.2
- 5.1.3 (RC1)
-->
- master
## PR submission checklist
- [x] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
- [x] This PR does not have documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you 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.
-->
--
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
[GitHub] [trafficcontrol] shamrickus merged pull request #6443: TM should not overwrite monitoring snapshot data with crconfig snapsot data
Posted by GitBox <gi...@apache.org>.
shamrickus merged pull request #6443:
URL: https://github.com/apache/trafficcontrol/pull/6443
--
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
[GitHub] [trafficcontrol] TaylorCFrey commented on a change in pull request #6443: TM should not overwrite monitoring snapshot data with crconfig snapsot data
Posted by GitBox <gi...@apache.org>.
TaylorCFrey commented on a change in pull request #6443:
URL: https://github.com/apache/trafficcontrol/pull/6443#discussion_r777734624
##########
File path: traffic_monitor/towrap/towrap.go
##########
@@ -690,8 +690,20 @@ func CreateMonitorConfig(crConfig tc.CRConfig, mc *tc.TrafficMonitorConfigMap) (
// Dump the "live" monitoring.json monitors, and populate with the
// "snapshotted" CRConfig
- mc.TrafficMonitor = map[string]tc.TrafficMonitor{}
+ if mc == nil {
+ return mc, errors.New("no TM configmap data")
+ }
+ //mc.TrafficMonitor = map[string]tc.TrafficMonitor{}
for name, mon := range crConfig.Monitors {
+ if tmData, ok := mc.TrafficMonitor[name]; ok {
+ if tmData.IP != "" && tmData.IP6 != "" {
+ continue
+ } else {
+ mc.TrafficMonitor[name] = tc.TrafficMonitor{}
+ }
+ } else {
+ continue
+ }
// monitorProfile = *mon.Profile
Review comment:
Commented code to be removed.
##########
File path: traffic_monitor/towrap/towrap.go
##########
@@ -690,8 +690,20 @@ func CreateMonitorConfig(crConfig tc.CRConfig, mc *tc.TrafficMonitorConfigMap) (
// Dump the "live" monitoring.json monitors, and populate with the
// "snapshotted" CRConfig
- mc.TrafficMonitor = map[string]tc.TrafficMonitor{}
+ if mc == nil {
+ return mc, errors.New("no TM configmap data")
+ }
+ //mc.TrafficMonitor = map[string]tc.TrafficMonitor{}
Review comment:
Commented code to be removed.
##########
File path: traffic_monitor/towrap/towrap.go
##########
@@ -690,8 +690,20 @@ func CreateMonitorConfig(crConfig tc.CRConfig, mc *tc.TrafficMonitorConfigMap) (
Review comment:
I believe the comment(s) above can be cleaned up. It looks like issue #3528 was resolved in a previous MR.
##########
File path: traffic_ops/traffic_ops_golang/monitoring/monitoring.go
##########
@@ -294,6 +295,32 @@ AND cdn.name = $3
cacheStatus := tc.CacheStatusFromString(status.String)
if ttype.String == tc.MonitorTypeName {
+ var ipStr, ipStr6 string
+ var gotBothIPs bool
Review comment:
Are we gauranteed to get both ipv4 and ipv6? Will there be a case where one or the other alone is acceptable?
##########
File path: traffic_ops/traffic_ops_golang/monitoring/monitoring.go
##########
@@ -294,6 +295,32 @@ AND cdn.name = $3
cacheStatus := tc.CacheStatusFromString(status.String)
if ttype.String == tc.MonitorTypeName {
+ var ipStr, ipStr6 string
+ var gotBothIPs bool
+ if _, ok := interfacesByNameAndServer[int(serverID.Int64)]; ok {
+ for _, interf := range interfacesByNameAndServer[int(serverID.Int64)] {
+ for _, ipAddress := range interf.IPAddresses {
+ ip := net.ParseIP(ipAddress.Address)
+ if ip == nil {
+ continue
+ }
+ if ip.To4() != nil {
+ ipStr = ipAddress.Address
+ } else if ip.To16() != nil {
+ ipStr6 = ipAddress.Address
+ }
+ if ipStr != "" && ipStr6 != "" {
+ gotBothIPs = true
+ break
+ }
+ }
+ if gotBothIPs {
+ break
+ }
+ }
+ } else {
+ fmt.Println(interfacesByNameAndServer)
Review comment:
Where is this printing to?
##########
File path: traffic_monitor/towrap/towrap.go
##########
@@ -690,8 +690,20 @@ func CreateMonitorConfig(crConfig tc.CRConfig, mc *tc.TrafficMonitorConfigMap) (
// Dump the "live" monitoring.json monitors, and populate with the
// "snapshotted" CRConfig
- mc.TrafficMonitor = map[string]tc.TrafficMonitor{}
+ if mc == nil {
+ return mc, errors.New("no TM configmap data")
+ }
+ //mc.TrafficMonitor = map[string]tc.TrafficMonitor{}
for name, mon := range crConfig.Monitors {
+ if tmData, ok := mc.TrafficMonitor[name]; ok {
+ if tmData.IP != "" && tmData.IP6 != "" {
+ continue
+ } else {
+ mc.TrafficMonitor[name] = tc.TrafficMonitor{}
Review comment:
It looks like, if I'm reading this correctly, if there isn't a matching `TrafficMonitor` struct instance, we create one here and assign it. However, a few lines down, we create a new `TrafficMonitor` struct regardless with
```
m := tc.TrafficMonitor{}
```
and, ultimately, assign it to the name/key in the map. I'm wondering if these if-statement are actually necessary or doing what is intended.
Since we're still created a new struct and assigning it I'm not entirely sure we aren't still overwriting.
##########
File path: traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go
##########
@@ -702,6 +701,10 @@ func setupMockGetMonitoringServers(mock sqlmock.Sqlmock, monitor Monitor, router
}
}
}
+ // Add an interface and ip addresses for the monitor cache
+ interfaceRows = interfaceRows.AddRow("monitorCacheInterface", nil, 1500, false, 2)
+ ipAddressRows = ipAddressRows.AddRow("5.6.7.8", "10.0.0.0", true, 2, "monitorCacheInterface")
Review comment:
Any case or instance where we would need a negative test? Such as missing ip4v/ipv6/both?
--
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
[GitHub] [trafficcontrol] shamrickus merged pull request #6443: TM should not overwrite monitoring snapshot data with crconfig snapsot data
Posted by GitBox <gi...@apache.org>.
shamrickus merged pull request #6443:
URL: https://github.com/apache/trafficcontrol/pull/6443
--
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
[GitHub] [trafficcontrol] TaylorCFrey commented on a change in pull request #6443: TM should not overwrite monitoring snapshot data with crconfig snapsot data
Posted by GitBox <gi...@apache.org>.
TaylorCFrey commented on a change in pull request #6443:
URL: https://github.com/apache/trafficcontrol/pull/6443#discussion_r779779589
##########
File path: traffic_monitor/towrap/towrap.go
##########
@@ -680,20 +680,22 @@ func (s TrafficOpsSessionThreadsafe) MonitorCDN(hostName string) (string, error)
// Traffic Monitors and Delivery Services found in a CDN Snapshot, and wipe out
Review comment:
I believe we can remove the "[...] and wipe out all of those..." phrase as well since we will no longer be doing that.
--
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
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6443: TM should not overwrite monitoring snapshot data with crconfig snapsot data
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6443:
URL: https://github.com/apache/trafficcontrol/pull/6443#discussion_r778230833
##########
File path: traffic_monitor/towrap/towrap.go
##########
@@ -690,8 +690,20 @@ func CreateMonitorConfig(crConfig tc.CRConfig, mc *tc.TrafficMonitorConfigMap) (
// Dump the "live" monitoring.json monitors, and populate with the
// "snapshotted" CRConfig
- mc.TrafficMonitor = map[string]tc.TrafficMonitor{}
+ if mc == nil {
+ return mc, errors.New("no TM configmap data")
+ }
+ //mc.TrafficMonitor = map[string]tc.TrafficMonitor{}
for name, mon := range crConfig.Monitors {
+ if tmData, ok := mc.TrafficMonitor[name]; ok {
+ if tmData.IP != "" && tmData.IP6 != "" {
+ continue
+ } else {
+ mc.TrafficMonitor[name] = tc.TrafficMonitor{}
Review comment:
Good catch, this should be fixed now.
--
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
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6443: TM should not overwrite monitoring snapshot data with crconfig snapsot data
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6443:
URL: https://github.com/apache/trafficcontrol/pull/6443#discussion_r778228157
##########
File path: traffic_ops/traffic_ops_golang/monitoring/monitoring.go
##########
@@ -294,6 +295,32 @@ AND cdn.name = $3
cacheStatus := tc.CacheStatusFromString(status.String)
if ttype.String == tc.MonitorTypeName {
+ var ipStr, ipStr6 string
+ var gotBothIPs bool
Review comment:
We are not guaranteed to get both ipv4 and ipv6, but if there is a case where we have both, we break out of the loop. Else, we keep looping till the end, and take the values of whichever of ipv4 or ipv6 is not empty.
--
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