Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Fix ip_allow config generation for mids to include rascal servers#4296

Merged
rob05c merged 5 commits intoapache:masterfrom
mhoppa:bug/fix_ip_allow_generation
Jan 17, 2020
Merged

Fix ip_allow config generation for mids to include rascal servers#4296
rob05c merged 5 commits intoapache:masterfrom
mhoppa:bug/fix_ip_allow_generation

Conversation

@mhoppa
Copy link
Copy Markdown
Contributor

@mhoppa mhoppa commented Jan 15, 2020 •
edited
Loading

What does this PR (Pull Request) do?

Currently in ip_allow.config generation for mids it does not include rascal server IPs causing them to be blocked on attempting to pull astats.

The behavior in the perl implementation is that it would include all servers of type Rascal as well as EDGEs that are in either parent/secondary cachegroup of the mid. https://github.com/apache/trafficcontrol/blob/master/traffic_ops/app/lib/API/Configs/ApacheTrafficServer.pm#L2210-L2228

TO GO API Bug

In the go TO implementation it checks the parent/secondary cachegroup on both Rascal AND Edge servers which is a mismatch of logic.

Going from is rascal OR (is edge AND edge.cg in (mid parent cg, mid secondary cg) to is rascal OR edge AND s.cg in (mid parent cg, mid secondary cg)

atstccfg Bug

In the atstccfg cfg it does not attempt to include rascal servers

Which Traffic Control components are affected by this PR?

  • Traffic Ops
  • atstccfg

What is the best way to verify this PR?

Generate the ip_allow.cfg for a mid and ensure the rascal servers are included via TO API and atstccfg

If this is a bug fix, what versions of Traffic Control are affected?

The following criteria are ALL met by this PR

  • This PR includes tests OR I have explained why tests are unnecessary
  • This PR includes documentation OR I have explained why documentation is unnecessary
  • This PR includes an update to CHANGELOG.md OR such an update is not necessary
  • This PR includes any and all required license headers
  • This PR ensures that database migration sequence is correct OR this PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

@mhoppa mhoppa requested a review from rob05c January 15, 2020 17:40
@rawlinp rawlinp added regression bug a bug in existing functionality introduced by a new version cache-config Cache config generation Traffic Ops related to Traffic Ops labels Jan 15, 2020
@mhoppa
Copy link
Copy Markdown
Contributor Author

mhoppa commented Jan 15, 2020

Looks to have the same issue in atstccfg as well https://github.com/apache/trafficcontrol/blob/master/traffic_ops/ort/atstccfg/cfgfile/ipallowdotconfig.go#L112-L117 need to test and look into that

@mhoppa mhoppa changed the title Fix Golang ip_allow config generation for mids to include rascal servers Fix ip_allow config generation for mids to include rascal servers Jan 15, 2020
@mhoppa
Copy link
Copy Markdown
Contributor Author

mhoppa commented Jan 15, 2020

TO GO and ATSTCCFG should both be fixed for mid ip_allow generation now

rob05c
rob05c suggested changes Jan 16, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@rob05c rob05c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! Looks good, tested manually, both atstccfg and TO return all the IPs of all RASCAL (monitor) servers on that Mid's CDN, either the IP or a range containing it.

Would you mind adding an integration test to traffic_ops/testing/api/v14/atsconfig_test.go to verify that (or copying one like hdr_rw_dot_config_test.go and making a new file, whatever's easier)?

@mhoppa
Copy link
Copy Markdown
Contributor Author

mhoppa commented Jan 16, 2020

Yeah I can do that!

@mhoppa
Copy link
Copy Markdown
Contributor Author

mhoppa commented Jan 16, 2020

API tests are modified to be more robust incase the rascal server is included in a range instead on its own. CRED goes to @rob05c for the IP4ToNum, IP4InRange implementations :)

rob05c
rob05c approved these changes Jan 17, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@rob05c rob05c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! API tests pass, unit tests pass. Thanks for doing those, I know it was more painful than either of us realized

@rob05c rob05c merged commit 6b11bd9 into apache:master Jan 17, 2020
@mhoppa mhoppa mentioned this pull request Jan 21, 2020
Merged
6 tasks
mhoppa added a commit to mhoppa/trafficcontrol that referenced this pull request Jan 21, 2020
@mhoppa
Fix ip_allow config generation for mids to include rascal servers (ap…
0c6d2b9 
…ache#4296)

* Fix ip_allow config generation to include rascal servers

* Fix ip_allow generation in atstccfg

* Add API tests for ip_allow

* Make tests more robust

(cherry picked from commit 6b11bd9)
rawlinp pushed a commit that referenced this pull request Jan 21, 2020
@mhoppa @rawlinp
Fix ip_allow config generation for mids to include rascal servers (#4296
253b9e7 
) (#4308)

* Fix ip_allow config generation to include rascal servers

* Fix ip_allow generation in atstccfg

* Add API tests for ip_allow

* Make tests more robust

(cherry picked from commit 6b11bd9)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@rob05c rob05c rob05c approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cache-config Cache config generation regression bug a bug in existing functionality introduced by a new version Traffic Ops related to Traffic Ops

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mhoppa @rob05c @rawlinp