pierky / arouteserver

A tool to automatically build (and test) feature-rich configurations for BGP route servers.
https://arouteserver.readthedocs.org/
GNU General Public License v3.0
288 stars 46 forks source link

adding multihop config option support #61

Closed nicko170 closed 3 years ago

nicko170 commented 3 years ago

Have some content caches that require multihop sessions from route servers - this should allow that as a new config option on a client basis, but might need some help again on the tests and possibly documentation :-)

pierky commented 3 years ago

Thanks for the PR! I'll take care of finishing it up and then I'll get back to you.

pierky commented 3 years ago

Hi @nicko170, please note that some preliminary tests show that BIRD seems to not support multihop when the secondary option is used for the routing table (thus, sorted tables are used).

The error that the integration tests show is BGP in recursive mode prohibits sorted table.

I was also able to find some similar conclusions in the slides here https://www.de-cix.net/_Resources/Persistent/fba89bc19381b6784df99d2a78d4a11ebb7583c2/DE-CIX-route-server-testframework.pdf (page n. 16).

Usage of secondary sorted tables is a very deep foundation for the BIRD configurations built by ARouteServer, so at the moment I'm inclined towards implementing this option only for OpenBGPD, which seems supporting it.

nicko170 commented 3 years ago

Hey @pierky

Good catch, I picked it up on friday and meant to update this :-)

It all works nicely if add_path is disabled, so I think maybe we can leave bird support as long as there's a check to ensure add_path and multihop aren't used together?

With add_path disabled, the tables are not sorted and the configuration is valid and works with bird happy (deploying it on Friday morning) and multihop is nice and happy too.

pierky commented 3 years ago

It all works nicely if add_path is disabled, so I think maybe we can leave bird support as long as there's a check to ensure add_path and multihop aren't used together?

Oh, add_path... I'll check it again after being sure it's not used, and I'll implement the check like you said!

Thanks again

pierky commented 3 years ago

Hello @nicko170,

I've tried to build a config with multihop again, and no add path, and it was still broken.

After further investigation, the incompatibility seems to show up in configurations where the path hiding prevention feature is turned on (which is by default in ARouteServer). That feature is based on BIRD's secondary sorted tables, and multihop is not compatible with sorted tables (multihop implicitly uses gateway recursive, that prevents using sorted tables, that are in turn needed by the secondary option; and also, Multihop BGP cannot use direct gateway mode - cit.).

I'll look into a way to add the option to BIRD with a proper check that will prevent both features (multihop and path hiding prevention) from being enabled at the same time.

pierky commented 3 years ago

Oh btw, I'd like to ask you a clarification on your statement "With add_path disabled, the tables are not sorted": maybe you meant "path hiding prevention" instead of add-path?

pierky commented 3 years ago

WIP on nicko170-multihop_support branch here https://github.com/pierky/arouteserver/tree/nicko170-multihop_support

nicko170 commented 3 years ago

Sorry -- Yes, path_hiding not add_path. I should have checked before commenting :)

return {
            "cfg": {
                "router_id": route_server["ipv4"],
                "rs_as": exchange["asn"],
                "add_path": False,
                "filtering": {
                    "reject_policy": {"policy": "tag_and_reject",},
                    "never_via_route_servers": {"peering_db": False, },
                    "global_black_list_pref": None,
                    "ipv4_pref_len": {"max": 24, "min": 8},
                    "ipv6_pref_len": {"max": 48, "min": 12},
                    "irrdb": {
                        "enforce_origin_in_as_set": True,
                        "enforce_prefix_in_as_set": False,
                        "tag_as_set": False,
                        "use_arin_bulk_whois_data": {"enabled": False},
                        "use_rpki_roas_as_route_objects": {"enabled": False},
                    },
                    "max_as_path_len": 32,
                    "max_prefix": {
                        "action": "restart",
                        "peering_db": True,
                        "restart_after": 5,
                    },
                    "next_hop": {"policy": "same-as"},
                    "reject_invalid_as_in_as_path": True,
                    "rpki_bgp_origin_validation": {
                        "enabled": True,
                        "reject_invalid": True,
                    },
                },
                "graceful_shutdown": {"enabled": True},
                "gtsm": False,
                "passive": False,
                "path_hiding": False,
                "prepend_rs_as": False,
                "rpki_roas": {"source": "rtr"},
            }
        }

&&

 clients:
    - asn: 65535
      description: "PEER"
      ip:
        - x.x.x.x
      cfg:
        multihop: 3
        filtering:
          max_prefix:
            action: restart
            limit_ipv4: 0
            limit_ipv6: 0

Is a working set of options.

pierky commented 3 years ago

Hello @nicko170, I've pushed the new feature to the dev branch and I've also build a pre-release version of ARouteServer that includes it. The final work was done in this branch, where I added some more stuff on top of your commits.

Release 1.1.0a1 is out on PyPi Test, in case you wanna test it: on the doc website you can find some instructions on how to install a pre-release from PyPi, just in case you need help on that process.

Eventually this feature will be included in the next release of the tool.

Thanks a lot for your contribution, I'm going to close this PR for the time being. Feel free to reach me out again on this topic if you believe something is wrong.

pierky commented 3 years ago

Hello @nicko170, just to let you know that 1.1.0 is out, and it includes this feature.

Thanks for the contribution!