Servers interfaces

[ocket8888]

What does this PR (Pull Request) do?

• This PR fixes Update /servers and /servers/{{ID}} to use data from the interfaces tables #4612

This PR adds support for the new server interfaces tables to all methods of the
/servers and /servers/{{ID}} TO API endpoints.

Regarding "CDN-in-a-Box automated data insertion has been updated to use the
new format..." from #4612: The enroller has instead been pinned to version 2.x
of the TO API, which works just as well.

This also adds the necessary documentation.

Finally, this PR removes the configuration file generation route handlers for parent.config, remap.config, and ip_allow.config, as they were in conflict with the changes and have been deprecated for at least a major release.

However, that does break CDN-in-a-Box for the forseeable future (to be exact, until #4671 is merged)

Which Traffic Control components are affected by this PR?

• CDN in a Box
• Documentation
• Traffic Control Client
• Traffic Ops

What is the best way to verify this PR?

Run all unit and API/client integration tests, perform some manual HTTP requests
to verify expected behavior. Also build and read the documentation.

The following criteria are ALL met by this PR

• This PR includes tests
• This PR includes documentation
• This PR includes an update to CHANGELOG.md
• This PR includes any and all required license headers
• This PR does not include a database migration
• This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

[shamrickus]

Bunch of comments but it's only two distinct issues

[shamrickus]

LGTM

[rawlinp]

TO API v3 tests don't build now due to me adding more usage of the TO Go client methods you're changing (sorry):

# github.com/apache/trafficcontrol/traffic_ops/testing/api/v3 [github.com/apache/trafficcontrol/traffic_ops/testing/api/v3.test]
./deliveryserviceservers_test.go:54:45: not enough arguments in call to TOSession.GetServers
	have ()
	want (*url.Values)
./deliveryserviceservers_test.go:56:14: cannot range over serversResp (type tc.ServersV3Response)
./servers_to_deliveryservice_assignment_test.go:161:16: invalid operation: s.CommonServerProperties.CDNName == "cdn1" (mismatched types *string and string)
./servers_to_deliveryservice_assignment_test.go:162:11: cannot use &s (type *tc.ServerNullable) as type *tc.Server in assignment
./servers_to_deliveryservice_assignment_test.go:170:25: TOSession.GetServerByHostName undefined (type *"github.com/apache/trafficcontrol/traffic_ops/client".Session has no field or method GetServerByHostName)
FAIL	github.com/apache/trafficcontrol/traffic_ops/testing/api/v3 [build failed]

Also, the v1 and v2 tests fail, looks like it's failing to delete servers:

servers_test.go:149: cannot DELETE Server by ID: 500 Internal Server Error[500] - Error requesting Traffic Ops http://localhost:30003/api/2.0/servers/1 {"alerts":[{"text":"Internal Server Error","level":"error"}]}
         - {[]}

[rawlinp]

Unit tests and TO API tests pass. This PR breaks CIAB, creating servers in TP, and snapshotting. In the future, please refrain from breaking core functionality that would be required for other developers to complete their own development and testing. It's not really fair to hold the master branch hostage until your own PRs are reviewed and merged to fix those issues. PRs should be atomic and as focused as possible.

[ocket8888]

In the future I'll definitely be using feature branches instead. I was just under the impression that was frowned upon prior to starting this effort.

[rawlinp]

IMO feature branches are not the solution either. This PR could've done without a bunch of extraneous changes yet included the changes that kept everything functional. For instance: this commit is minuscule in comparison to the size of this PR, yet probably could've been included while other extraneous changes could've been excluded: a835459.

[ocket8888]

I think the problem was that I misunderstood what was meant by "feature branch" initially. I think you mean "feature branch" to say "a branch of apache/trafficcontrol that contains the entire body of work necessary to implement servers having multiple interfaces in every facet of every component" whereas I mean "A big branch of changes across multiple components that provides all of the changes necessary to implement a feature - which may or may not mean everything it takes to implement servers having multiple interfaces."

For example, you did a feature branch with topologies. You don't call it a feature branch, but I would, because it was a branch that encapsulated a feature - the feature of including and being able to manipulate ATC topologies. Just because it didn't totally fulfil the requirements of a blueprint doesn't make it not a feature branch, IMO.

To be clear I'm not against using a "feature branch" by either of those definitions. I just thought they were frowned upon equally, so I tried to do things without them. Imagine my surprise when a Topologies feature branch sprung up.

[rawlinp]

There was no feature branch for Topologies -- all the work we've done on Topologies so far has been made in atomic PRs to master.

Our definition of feature branches is the same. I'm not specifying "within this repo" either. Everyone creates a "feature branch" in their own forks in order to create a PR. Every PR to master essentially is a feature branch. At the end of the day, I don't think we should ever be opening massive PRs that take a tremendous amount of time in one sitting to review/test/merge. Most new features can always be broken down into smaller, atomic PRs that don't break things when merged on their own. Throughout reviewing this PR, I've tried to describe to you how that could've been done from the get-go, so that we can try to break things down into smaller, atomic PRs in the future. Creating a feature branch goes entirely against that idea, because it means reviewing and merging one massive PR into master that completely implements an entire new feature.

[ocket8888]

"There was no feature branch for Topologies..."

"Our definition of feature branches is the same."

Those statements are contradictory (so are "There was no feature branch for Topologies..." and "Every PR to master essentially is a feature branch." lol). What you did for Topologies was what I would call a "feature branch", because it reached across multiple components and/or API endpoints. Its scope was broader than an atomic unit of work done by one person to one aspect of one component. Your definition seems to be more closely related to the "size" of the PR than its "scope".

The only way the /servers endpoint could be changed to not break snapshots would be if that was in the same PR (which was a relatively small change, as it turned out), and to avoid breaking Traffic Portal it would need to be in the same PR as well (although in my defence that wouldn't have been the case if TP stayed on the stable 2.0 API, which was a change I deliberately hoped to avoid making for exactly this reason when adding the 3.0 API). Which all sounds fine to me, except that I'd also call that a feature branch.

I understand how to do feature branches - obviously, because I already said I'd prefer to do them going forward. You don't need to convince me, I'm on board. I just needed to know that was an option; this was just a misunderstanding.

[rawlinp]

Ok, I think I am beginning to understand your definition of a feature branch. Earlier, you said this:

A big branch of changes across multiple components that provides all of the changes necessary to implement a feature

Your definition lies in the fact that one PR includes changes to multiple components at once? That is not necessarily true -- you can have a feature branch that only affects a single component.

I am generally in favor of PRs that only affect a single component at a time (assuming those changes can live on their own without breaking things), but that has nothing to do with feature branches. What I mean by a "feature branch" would be this:

1. You create a new branch in this repo called dual-homing. This is the feature branch.
2. You implement certain parts of the dual-homing feature and open PRs to the dual-homing branch. The size and scope of the PR, how many components they affect, is irrelevant.
3. Three months pass, and multiple PRs have been opened and merged into the dual-homing branch. The "dual-homing" feature is now basically done. Meanwhile, master has continued to move forward with other changes.
4. Now the dual-homing branch is ready to merge into master assuming it's been rebased regularly. The diff is massive. Ideally, someone would review the entirety of the diff before merging dual-homing into master. If the PRs into dual-homing were thoroughly reviewed, and dual-homing was frequently rebased onto master, maybe this final review can be quick.
5. dual-homing is merged into master. Chaos ensues. Everyone that had open PRs into master now all have merge conflicts, and they're probably not easily resolved since there are three months worth of changes to sift through.

That is essentially what I mean by "feature branches", and I would really like to avoid them. What you're describing as a "feature branch" is just a PR with more than one "atom" of a feature. I think it's fine to have multiple "atoms" in a single PR assuming they can still live on their own without breaking things and aren't so numerous that the PR is a major burden to review due to its size and scope. For me, I'd start to look at breaking up a PR into atoms once it breaks the 500 LOC range. To use TO/TP as an example, if you're adding a new Delivery Service field, you could implement the new field in TO as one PR, then add the new field to TP in a second PR. The TO change can live on its own without breaking things -- yet it can't really be broken down further since it's a simple field addition. That would be one "atom". The TP change would be the second "atom". Both of them combined are probably still a relatively small PR, so you could lump both atoms together in the same PR.

[ocket8888]

Yes, that's all exactly correct and highlights the very misunderstanding I only realized after a few PRs.