Basic ipv6 support #112
Merged
Basic ipv6 support #112
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sebageek
force-pushed
the
basic-ipv6-support
branch
from
d9c6833 to
950e4d6
Compare
sebageek
force-pushed
the
basic-ipv6-support
branch
2 times, most recently
from
f360938 to
d2ffc25
Compare
sebageek
force-pushed
the
basic-ipv6-support
branch
2 times, most recently
from
0f3dfc5 to
c7f8e1f
Compare
In VBInterface.postflight() we only need to call _update(), as we're already scoped to a router (and have a context).
With this patch we're supporting IPv6 partly in our infrastructure, at least for internal networks. This enables the v6 address family on our VRFs (if there is an IPv6 subnet connected to the router) and we configure IPv6 addresses on interfaces where needed. For BGPVPN we currently skip all v6 subnets. For extraroutes there is still testing to be done.
sebageek
force-pushed
the
basic-ipv6-support
branch
from
c7f8e1f to
a8b064c
Compare
sebageek
force-pushed
the
basic-ipv6-support
branch
2 times, most recently
from
e7f3d91 to
f43a43b
Compare
mutax
reviewed
Jan 23, 2025
| @@ -65,6 +66,7 @@ def __init__(self, router_id, router_port, extra_atts): | |||
| self._primary_subnet_id = None | |||
| self.ip_address = self._ip_address() | |||
There was a problem hiding this comment.
For consistency reasons, does it make sense to rename this to self.ip4_addresses?
There was a problem hiding this comment.
I think that has been done in a later commit.
sebageek
force-pushed
the
basic-ipv6-support
branch
4 times, most recently
from
a3a4d53 to
55340ea
Compare
mutax
reviewed
Jan 29, 2025
sebageek
force-pushed
the
basic-ipv6-support
branch
from
55340ea to
54c1a93
Compare
mutax
previously approved these changes
Jan 29, 2025
sven-rosenzweig
previously approved these changes
Jan 30, 2025
sebageek
force-pushed
the
basic-ipv6-support
branch
4 times, most recently
from
fce7529 to
934ffc5
Compare
sven-rosenzweig
previously approved these changes
Feb 4, 2025
mutax
previously approved these changes
Feb 7, 2025
sebageek
force-pushed
the
basic-ipv6-support
branch
2 times, most recently
from
ce0bd2d to
8a94093
Compare
mutax
reviewed
Feb 11, 2025
We can now configure IPv6 routes on our hardware routers, including default gateways. As we now return an empty dict if there are no routes we should no longer see any route diffs when the router has no default route configured.
sebageek
force-pushed
the
basic-ipv6-support
branch
from
8a94093 to
535f528
Compare
mutax
previously approved these changes
Feb 13, 2025
toanju
reviewed
Feb 18, 2025
|
|
||
| for ext_ip in ext_ips: | ||
| ip_version = None | ||
| if 'subnet_id' in ext_ip: |
There was a problem hiding this comment.
Why do we need to fetch the ip version from the subnet? Can't we rely on the ip_address itself?
From my understanding, the info dict looks like:
"external_fixed_ips": [{"subnet_id": "uuid", "ip_address": "<ip-address>"}]
There was a problem hiding this comment.
Why not
if 'ip_address' in ext_ip:
blabla
elif 'subnet_id' in ext_ip:
blabla
There was a problem hiding this comment.
The info dict we're receiving here is not from the OpenStack database, but from the API request of the user, e.g. if the user types in os router set $router_id --external-gateway $extnet --fixed-ip ip-address=$ip --fixed-ip subnet=$subnet - this will result in an external gateway interface with two ips, one directly specified, one allocated from the subnet. we will only be having one of those entries in there in nearly all of the cases (I'm not sure adding both would be possible, but it would still be redundant). I can flip the if statement though, to prefer the IP if we happen to have both though.
There was a problem hiding this comment.
thanks for the detailed explanation. Not need to flip this from my side
Support announcing internal IPv6 CIDRs via BGPVPN, if we have them. This does not yet support route-targets / route-maps inside VRFs.
For our external prefixes we need support for IPv6 prefix lists.
We now support setting route-targets and export maps on the v6 address-family.
Route Maps can now reference IPv6 prefix lists. We also did some maintenance on the RouteMap class, like removing the unused enable_bgp flag and making sure that specifying both prefix_list and access_list don't override each other's match conditions (though this hasn't been used so far). As we don't know yet how we'll transport IPv6 prefixes from ACI/EVPN to our routers we'll for now leave the RoutePrefix to be v4 only and don't reference it for v6 addresses. Once we know how that part of the infra is going to work we'll fill it with life!
With the introduction of the new prefix list yang model in c626065 we also needed to introduce a cleanup method in preflight() that cleans up stale entries of a prefix list. This removal did not work properly, as we provided a seq as a string (number), while the Prefix class expects a PrefixSeq yang model. Therefore the cleanup in preflight failed with: AttributeError: asr1k_neutron_l3.models.netconf_yang.prefix in to_delete_dict 'str' object has no attribute 'no' When we convert the prefix list to a proper model, cleanup works.
We can now configure external interfaces with IPv6, including DAPNet support. This includes a rework of how the Router() class handles IP addresses and offers route redistribution via route-maps + prefix lists instead of explicit network statements. The InterfaceList now has three new methods to get the list of IPs for a router that are external, internal or routable. Routable IPs are defined as the list of internal IPs where the address scope of the subnet matches the address scope of the gateway (external) interface. The BGP AddressFamily class now supports advertising of routes via route-map. This means we have a route-map, which references prefix lists that contain routable and internal IPs + their extra routes. For routable IPs and their extraroutes we set a route target: routable IPs get the cloud vrf's id, routable extraroutes have a prepended 1 to the vrf id. This is very much the same as it is currently handled on the device, though the config was pre-provisioned and only referenced via route-maps on the respective network statements. This feature is currently only enabled for IPv6 default and can be enabled for IPv4 with the config option advertise_bgp_ipv4_routes_via_redistribute. The reason of doing this is that editing the BGP config tree currently locks up the whole device config due to requiring a full config resync. Prefix lists referenced by route-maps that do not contain any prefix don't appear as config on the device. The referencing route-map treats non-existing prefix-lists as "always matching". As our expectation of how this would work was empty prefix-list means "no match", we now have to adjust our expectations of this feature and always add an entry if the list would be empty otherwise. This is done via add_deny_if_empty, which adds a deny for everything. The seq 4242 was deliberately chosen to not collide with any of our existing rules, not because we saw any problems with this, but because we didn't want to end up in a situation where the device locks up because "there is already a rule with that seq", even though we generally think this should work.
Routers that do not have an external gateway also don't have a bridge domain associated with it, meaning the dynamic NAT list would always produce a diff. Therefore we delete the dynamic NAT list if there's nothing to configure and also make sure it doesn't appear in the diff.
We want to replicate the loop prevention implemented in a63bf94 for IPv4. This code should prevent traffic coming from the external interface from leaving the external interface again, if the vrf decides it needs to leave via the default route. This can happen when the device the router is connected to (e.g. ACI) thinks a destination is in a vrf, but the vrf doesn't have the destination anymore (due to removed fips, removed subnets or whatever). With IPv4 we had an explicit "set precedence routine" for internal packets, but when trying to configure this on the device cli it looks like this is omitted as being the default config. As we don't need to replicate default config we'll leave that part as-is. This change results in the following cli change for routers with an ipv6 address on the external interface: interface BD-VIF 9999 + ipv6 traffic-filter common ACL-EXT-TOS-V6 out + ipv6 policy route-map RM-EXT-TOS-V6 The access-list ACL-EXT-TOS-V6 and route-map RM-EXT-TOS-V6 need to be pre-provisioned.
We now have the first few tests that can create a router in the OpenStack database and generate sync data from this, which can then be used to instantiate the models used in the agent that then generate the netconf yang config for our devices. This means we can more or less now do end-to-end tests for DB-to-YANG tests, including coverage for if the Router() class correctly handles the data gotten from the DB and makes the right decisions. The current mock-implementation is a best effort approach to get us started with having tests. As the sync data is normally transferred via RPC / rabbitmq and then augmented with the address scope info we need to run some extra steps in the tests in get_sync_data() to have everything similar to what we have in production.
We want to be able to add and remove the address-family of a VRF as needed, so now we only enable it, if we also have an ip address for it - at least in the case of IPv6. For IPv4 we - for now - always keep the AF enabled to make sure we don't have any problems with the config transition. Once we are at a point were we make all device config dependant on the enable_ipv4 flag we can also think about removing the AF from teh VRF. We have a special case in the netconf-yang pulled from the device: an empty address-family config results in an empty tag, e.g. <ipv6/>, which then results in this config not appearing in the diff. A diff is needed for the object to actually be updated, so we add an extra flag to the Vrf netconf class, that creates an empty address family object if we find an empty tag on the device.
When providing multiple IP addresses on router create or router update, we used to take all of them in consideration for NAT pooling. This prevents user from updating their router with an IPv6 address (as "a router with two addresses is not allowed for NAT pooling") and also will fail when trying to create a NAT pool that contains IPv6 addresses. Now we split all provided addresses in two lists, one per address family. All IPv4 addresses go the regular NAT pooling / no NAT pooling path. For IPv6 we allow zero or one address. Providing more than one IPv6 address will show a nice error message to the user.
sebageek
force-pushed
the
basic-ipv6-support
branch
from
d202d0d to
a65b120
Compare
sven-rosenzweig
approved these changes
Feb 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is still some testing to be done. Also, the tests are not yet complete