Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PeerGroupAddressFamily and PeerEndpointAddressFamily #132

Merged
merged 12 commits into from
Sep 27, 2023

Conversation

glennmatthews
Copy link
Contributor

Closes #26.

  • Add PeerGroupAddressFamily and PeerEndpointAddressFamily data models.
  • Add extra_attributes to AddressFamily model.
  • Remove import_policy, export_policy, and multipath from PeerGroupTemplate, PeerGroup, and PeerEndpoint models, as per IOS XR CLI and the openconfig-bgp YANG model, these are address-family-specific attributes. (I'm open to revisiting this, as I do see that the Juniper config CLI configures policies at the peer-group level rather than at the peer-group-address-family level...)
  • Add UI, API, etc. for the new models
  • Add/update tests and documentation

image

image

image

image

Copy link
Contributor

@mzbroch mzbroch left a comment

Choose a reason for hiding this comment

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

I like the direction of the changes!
Adding more specific models requires more UI/API interactions from the user to create a new peering.

I think I could add following features:

  • in the Routing Instance create form , allow to create Routing-Instance specific AFs based on AddressFamilyTemplate
  • in the Peering form , auto-create PeerEndpointAddressFamily based on selected available AFs
  • in the PeerGroup create form , allow to auto-create PeerGroupAddressFamily

Those should be out of scope for this PR , but let's see if we could add them next ?

nautobot_bgp_models/models.py Outdated Show resolved Hide resolved
nautobot_bgp_models/models.py Show resolved Hide resolved
super().__init__(*args, **kwargs)

if self.initial.get("routing_instance"):
self.fields["routing_instance"].disabled = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we should be dropping this? My idea was to prevent moving PeerGroups between Routing Instances

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This prevents us from prepopulating the routing_instance via query parameters, e.g. on the RoutingInstance detail page the "Add a Peer Group" button that I added links to /plugins/bgp-models/peer-groups/add/?routing_instance=<uuid>. Might just need to rework how this logic is written in order to still enforce no changes to an existing peer-group routing_instance?

@@ -124,6 +139,17 @@
),
),
),
NavMenuItem(
link="plugins:nautobot_bgp_models:peerendpointaddressfamily_list",
name="Peer Endpoint Address-families (AFI-SAFI)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Long term, I think we should "activate" address families during peering creation

So that in the "create" peering form we just enable specific address families and this results in PeerEndpointAddressFamili objects created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, this would be nice from a UX standpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be partly addressed by #135.

Comment on lines -489 to 490
"import_policy": ["peer_group", "peer_group.peergroup_template"],
"source_ip": ["peer_group"],
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: Without a data migration, this will result in the data loss for existing users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but we're still pre-1.0 here, and I'd argue the existing data is "wrong". Let's document it in the change log for sure, but I can't see a way to do a data migration since there's no indication which afi_safi the policies should be migrated to.

@mzbroch
Copy link
Contributor

mzbroch commented Sep 27, 2023

LGTM, ship it!

@mzbroch mzbroch self-requested a review September 27, 2023 14:41
@glennmatthews glennmatthews merged commit 50a033b into develop Sep 27, 2023
16 checks passed
@glennmatthews glennmatthews deleted the u/glennmatthews-afis-galore branch September 27, 2023 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Peer Endpoint and Peer Group Contexts
2 participants