Introduce IPAM Namespaces #3378

jathanism · 2023-02-28T20:10:34Z

Closes: #3337, #3429, #3681

What's Changed

This is the very rough around the edges prototype of IPAM Namespaces. It is not yet complete.

It has drifted somewhat from what was discussed in the design chats and what is scoped on #3337, due to the experimentation in the data design during prototyping.

The gist

VRFs can have prefixes assigned to them
VRFs can have devices assigned to them
VRFs can be used across devices
Any number of unique VRFs may be assigned to any number of devices, therefore allowing VRF names to be reused across devices
An RD is only significant upon assignment of a VRF to a device; therefore the same VRF can be assigned to multiple devices, each with a distinct (including null) RD if so desired
Any number of unique Prefixes may be assigned to any number of VRFs, so long as they are in the same namespace

Changes that drifted related to VRF/RD

VRF has FK to Namespace instead of deriving namespace through RD
VRF has m2m to Device, Prefix with through tables for each
VRF through table for Device is where RD is assigned; validation asserts that RD (if set) namespace matches VRF namespace
VRF through table for Device; validation asserts that if set, VRF namespace must match Prefix namespace

Caveats

This will likely not work in any API or UI views. It's rough.
This is tested at the ORM only at this time, expecting manual creation of objects to assert functionality.
There are fields commented out in serializers/views/forms/filters so that database migrations work without complaining

TODO

Explanation of Change(s)
Added change log fragment(s) (for more information see the documentation)
Attached Screenshots, Payload Example
Unit, Integration Tests
Documentation Updates (when adding/changing features)
Example Plugin Updates (when adding/changing features)
Outline Remaining Work, Constraints from Design

bryanculver · 2023-03-01T13:39:49Z

@jathanism Can you describe the why we deviated from the original design? As far as I can tell this moves the validation into Python as opposed to schema/DB. Is this long-term desirable? What were the limitation with the original?

An RD is only significant upon assignment of a VRF to a device; therefore the same VRF can be assigned to multiple devices, each with a distinct (including null) RD if so desired

Is this, could this not be, done behind the scenes from the API interfaces? Where we get schema performant validation but simplified APIs for integrators? Is that the plan and this just a compromise for the prototype?

glennmatthews · 2023-03-01T13:42:31Z

Could you add a class diagram similar to #3337 (comment) and #3245 (comment)? I'm having a hard time telling from the text description alone how this implementation differs from those and what the implications are.

jathanism · 2023-03-01T21:27:39Z

Could you add a class diagram similar to #3337 (comment) and #3245 (comment)? I'm having a hard time telling from the text description alone how this implementation differs from those and what the implications are.

@jathanism Can you describe the why we deviated from the original design? As far as I can tell this moves the validation into Python as opposed to schema/DB. Is this long-term desirable? What were the limitation with the original?

An RD is only significant upon assignment of a VRF to a device; therefore the same VRF can be assigned to multiple devices, each with a distinct (including null) RD if so desired

Is this, could this not be, done behind the scenes from the API interfaces? Where we get schema performant validation but simplified APIs for integrators? Is that the plan and this just a compromise for the prototype?

Context, descriptions and some Mermaid diagrams have been provided in the issue: #3337 (comment)

jathanism · 2023-03-01T21:30:48Z

Could you add a class diagram similar to #3337 (comment) and #3245 (comment)? I'm having a hard time telling from the text description alone how this implementation differs from those and what the implications are.

This is the output from:

nautobot-server graph_models -a -g -I Namespace,Prefix,IPAddress,RouteDistinguisher,VRF,VRFDeviceAssignment,VRFPrefixAssignment,Interface,Device` -o nautobot_ipam_namespace.png

The boxes for Device and Interface were cropped out of this render for conciseness, but you can see the relationship lines pointing to them at the top of the image.

HanlinMiao · 2023-03-07T02:20:22Z

#3395
This is the implementation based on the original discussion we had. @jathanism and I collaborated on this.

jathanism · 2023-03-09T00:28:12Z

Latest changes:

Got UI/API working with bare minimum effort to keep things flowing.

Namespace

Went with default "Global" namespace for foreign keys FOR NOW just to keep things moving
Basic API/UI views

General

Had to remove vrf from select_related for Prefix, IPAddress in about 7,000 places

RouteDistinguisher

Basic API/UI views

VRF

VRF edit includes namespace/devices, prefixes limited by namespace
VRF detail includes devices/prefixes
Filter by namespace now possible

Prefix

Prefix.objects.annotate_tree now filters on namespace not VRF
Edit view includes namespace
List view groups by namepace
Filter by namespace now possible
Prefix.get_duplicates() changed to a noop (returns queryset.none())
Available prefixes now filtered by namespace instead of VRF

IPAddress

WIP: Not filtering on VRF, but also not updated for Prefix parenting yet

Generic

Had to patch ObjectEditView to catch ValidationError for PrefixEditView because of the unique_together constraint ValidationError/IntegrityError not being caught the form validation due to network and prefix_length fields being excluded from validation due to not being included in the form fields.

Not Done

Linking RD to VRF when assigned to Devices "somehow"
Show assigned VRFs in Device detail view
Assigning a VRF to an Interface
Showing assigned VRF in Interface detail view

nautobot/dcim/models/device_components.py

jathanism · 2023-03-10T03:35:22Z

nautobot/ipam/filters.py

@@ -44,6 +46,19 @@
 )


+class NamespaceFilterSet(NautobotFilterSet):


Needs LocatableModelFilterSetMixin.

bryanculver

Notes:

I think there was unanimous agreement that there shouldn't be a VRF property on IPAddress so I've updated places that might have caused issues in testing in UI (instead of commenting out), there might be some utility in a helper method to get the parent prefix's vrf

bryanculver · 2023-03-10T19:36:20Z

nautobot/dcim/views.py

@@ -1714,7 +1714,8 @@ class InterfaceView(generic.ObjectView):
    def get_extra_context(self, request, instance):
        # Get assigned IP addresses
        ipaddress_table = InterfaceIPAddressTable(
-            data=instance.ip_addresses.restrict(request.user, "view").select_related("vrf", "tenant"),
+            # data=instance.ip_addresses.restrict(request.user, "view").select_related("vrf", "tenant"),


VRF relationship moves to Interface so this becomes unnecessary.

bryanculver · 2023-03-10T19:51:01Z

nautobot/extras/views.py

@@ -1828,7 +1828,7 @@ def get_extra_context(self, request, instance):
                "device_type",
            )
            ipaddress = instance.ip_addresses.select_related(
-                "vrf",
+                # "vrf",


VRF is either a property of Prefix or Interface, not needed here.

Notes:

I think there was unanimous agreement that there shouldn't be a VRF property on IPAddress so I've updated places that might have caused issues in testing in UI (instead of commenting out), there might be some utility in a helper method to get the parent prefix's vrf

Oh yeah, for sure, I stopped at the places that were causing utter breakage in other views. I didn't actually complete the necessary work for IPAddress.

bryanculver · 2023-03-10T20:47:54Z

nautobot/ipam/signals.py

+def ipam_object_saved(sender, instance, raw=False, **kwargs):
+    """
+    Forcibly call `full_clean()` when a new IPAM object is manually saved to prevent creation of
+    invalid objects.
+    """
+    if raw:
+        return
+    instance.full_clean()


This is going to create some noise, as it's causing a bunch of tests to blow up everywhere. See: https://github.com/nautobot/nautobot/pull/3378/files#diff-00a7372c809230ca828aa70f90352848b6654faadb78e4762a8f55ee6c59ce4fR584-R585

That is correct. This is there for now to force model validation in lieu of having formsets for m2m intermediate relationships for the prototype. I don't think it's worth creating the formset code right now if we're just going to rip it out and replace it for the v2 UI.

I disabled it for Prefix and VRF.

bryanculver · 2023-03-10T21:36:03Z

nautobot/ipam/models.py

+
+
+@extras_features(
+    "custom_fields",


👀 Silly git

bryanculver · 2023-03-10T21:39:46Z

I was reviewing the test changes in the prototype and the changes being made in #2403 and I still have some thoughts around the VRF-Prefix M2M:

Intuitively we will be adding an optional FK on (VM)Interface to VRF.
Considering IPAM Overhaul: Assign IP Address to Multiple Interfaces #2403 change, or even the old behavior where an Interface can have many IPs, it would be expected that if I have a VRF set on the Interface, I should only see the IPAddress selections from Prefixes that are a part of the same VRF
- Hand-wavey: We should eventually as a part of UX make this easier for the user to assign the VRF/Prefixes to each other but call out that we are building it for them (think modal popup "add prefix to VRF" when editing Interface)
If a Prefix was allowed to be a part of multiple VRFs (via M2M) then is that address assignable because it's through a different VRF or is that not a separate network?
- Interface(ETH0) is a member of VRF(Orange)
- Interface(ETH1) is a member of VRF(Blue)
- Prefix(10.0.0.0/24) is a member of VRF(Orange) and VRF(Blue)
- I want to assign IPAddress(10.0.0.100) which has a (concrete relationship to Prefix(10.0.0.0/24)) to Interface(ETH0), this would work but then it's an assignment to Interface(ETH1) would be :sus: because
- These are effectively the same network (and impose the same VRF) and I would get an address conflict, or at least would have to be a "backup" IP and have additional validation check
- These are different networks and then I should not be using the same IPAddress object and that would require a different Namespace and VRF entirely

Also, Prefix-VRF was non-M2M before and this never came up before as a limitation, but it was a uniqueness constraint and it would create a unique object because it was implied to be a different network and on the off-chance you did want them to be the same you would have to manage the duplicates manually between these prefixes.

Though there's probably a single sentence that unravels my train of thought here...

- Signal to forcefully call `full_clean()` on assignment of device(s) to VRF or prefix(es) to VRF Modify Interface IPAddress assignment to m2m - Did not yet remove `assigned_object` or any of that jazz from IPAddress - Committed signals (forgot in a previous commit) Remove `IPAddress.vrf` - Clarify unique constraints for VRFDeviceAssignment, - add m2m signal for VRF <-> PRefix Refactor to flatten migrations and implement VMInterface <-> IPAddress m2m Linting Default Namespace will always be created with name "Global" for use in default FK value for RouteDistinguisher, VRF, and Prefix.

- Also make Black happy. Yay!

Namespace - Went with default "Global" namespace for foreign keys FOR NOW just to keep things moving General - Had to remove `vrf` from `select_related` for `Prefix`, `IPAddress` in about 7,000 places VRF - VRF edit includes namespace/devices, prefixes limited by namespace - VRF detail includes devices/prefixes - Filter by namespace now possible Prefix - `Prefix.objects.annotate_tree` now filters on namespace not VRF - Edit view includes namespace - List view groups by namepace - Filter by namespace now possible - `Prefix.get_duplicates()` changed to a noop (returns `queryset.none()`) - Available prefixes now filtered by namespace instead of VRF IPAddress - WIP: Not filtering on VRF, but also not updated for Prefix parenting yet Generic - Had to patch `ObjectEditView` to catch `ValidationError` for `PrefixEditView` because of the `unique_together` constraint ValidationError/IntegrityError not being caught the form validation due to `network` and `prefix_length` fields being excluded from validation due to no t being included in the form fields.

@receiver

- Fixed lingering issues where `vrf` was still in `select_related` in some cases - Refactored signal handlers to use @receiver decorator - Added `VRFDeviceAssignmentTable` for use in displaying devices assigned to a VRF in RouteDistinguisher detail view. - Added a custom detail view template for RouteDistinguisher

Based on today's demo review... - Namespace - Added `description` field - Added `location` foreign key - Added `@extras_features` - Added assigned VRFs, Prefixes to detail view - RouteDistinguisher - Removed entirely - Revered back to `VRF.rd` field - VRF - Restored `rd` field - Added `namespace` to bulk edit - Added assigned Devices, Prefixes to detail view - VRFDeviceAssignment - Added `rd` field - Added `name` field - If either `rd` or `name` are not set on assignment, they are inherited from the VRF - Added a new `vrf_device_associated` signal handler for `m2m_changed.post_add` to hack m2m validation until a formset is put into place for Device/VRF edit views - Prefix - Added `namespace` to bulk edit - Added assigned VRFs to detail view

- Also improved UI error display slightly.

…utobot/nautobot into u/jathanism-namespace-data-migrations

gsnider2195 · 2023-06-01T19:19:43Z

nautobot/core/tests/test_graphql.py

@@ -228,6 +228,7 @@ def test_extend_schema_null_field_choices(self):
        self.assertIsInstance(getattr(schema, "resolve_mode"), types.FunctionType)


+@skip("Something really funky is broken here. Setup fails due to content types not existing...")


Fix before merging?

nautobot/extras/tests/test_api.py

…rations IPAM Namespace Data Migrations

…337-ipam-namespace

- Prefix is now using default natural key fields. - IPAddress tests will need to be revisited after broadcast/prefix_length fields have been removed

…nautobot/nautobot into prototype/jathanism-3337-ipam-namespace

Co-authored-by: Bryan Culver <bryan.culver@networktocode.com> Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> Co-authored-by: Gary Snider <gary.snider@networktocode.com>

jathanism marked this pull request as draft February 28, 2023 20:10

jathanism changed the title ~~Prototype/jathanism 3337 ipam namespace~~ Prototype: IPAM Namespaces Feb 28, 2023

jathanism mentioned this pull request Mar 1, 2023

Prototype: Network Namespaces, RouteDistinguisher, VRF Models #3337

Closed

6 tasks

jathanism changed the title ~~Prototype: IPAM Namespaces~~ [Prototype] IPAM Namespaces modified from original design. Mar 7, 2023

jathanism force-pushed the prototype/jathanism-3337-ipam-namespace branch 3 times, most recently from 1968fa4 to 5dbd1c9 Compare March 9, 2023 00:20

jathanism force-pushed the prototype/jathanism-3337-ipam-namespace branch from 5dbd1c9 to fccb6c5 Compare March 9, 2023 00:58

bryanculver linked an issue Mar 9, 2023 that may be closed by this pull request

Prototype: Network Namespaces, RouteDistinguisher, VRF Models #3337

Closed

6 tasks

jathanism commented Mar 10, 2023

View reviewed changes

nautobot/dcim/models/device_components.py Show resolved Hide resolved

jathanism commented Mar 10, 2023

View reviewed changes

bryanculver mentioned this pull request Mar 10, 2023

[Prototype] IPAM Namespace Implementations based on original discussion #3395

Closed

7 tasks

bryanculver changed the title ~~[Prototype] IPAM Namespaces modified from original design.~~ Introduce IPAM Namespaces Mar 10, 2023

bryanculver reviewed Mar 10, 2023

View reviewed changes

nautobot/ipam/models.py Outdated

@extras_features(

"custom_fields",

Copy link

Member

bryanculver Mar 10, 2023

Choose a reason for hiding this comment

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

👀 Silly git

jathanism and others added 8 commits March 14, 2023 13:58

UI/API views for Namespace, RouteDistinguisher

ac46161

- Also make Black happy. Yay!

Updates trying to get tests going

9e51b7c

Addressing merge issues

4db87ac

Rebase against next to pull in Interface/IP m2m changes.

3931978

- Also improved UI error display slightly.

jathanism and others added 3 commits June 1, 2023 10:37

Make "Global" namespace name a constant.

90c13bd

fix interface/ip/prefix/vrf namespace mismatch

d334ddd

Merge branch 'u/jathanism-namespace-data-migrations' of github.com:na…

b976ccd

…utobot/nautobot into u/jathanism-namespace-data-migrations

gsnider2195 reviewed Jun 1, 2023

View reviewed changes

nautobot/extras/tests/test_api.py Outdated Show resolved Hide resolved

gsnider2195 and others added 16 commits June 1, 2023 15:15

Merge pull request #3759 from nautobot/u/jathanism-namespace-data-mig…

35c59e8

…rations IPAM Namespace Data Migrations

fix some tests, a bug in the queryset and add some tests

1c75442

Merge remote-tracking branch 'origin/next' into prototype/jathanism-3…

2c57e31

…337-ipam-namespace

fix bad merge

6112900

Got view tests passing for VRF and Prefix

fb0d338

- Prefix is now using default natural key fields. - IPAddress tests will need to be revisited after broadcast/prefix_length fields have been removed

reorder migrations

ec44764

Merge branch 'prototype/jathanism-3337-ipam-namespace' of github.com:…

8cd9530

…nautobot/nautobot into prototype/jathanism-3337-ipam-namespace

reintroduce skipped test

18b8863

re-enable skipped tests

d02f40c

re-enable skipped tests

63b493f

re-enable skipped tests

6704da6

Fix pylint

28495a9

fix tests

b4dc932

Merge branch 'prototype/jathanism-3337-ipam-namespace' of github.com:…

c3abba4

…nautobot/nautobot into prototype/jathanism-3337-ipam-namespace

fix flake8

a910c63

Merge branch 'next' into prototype/jathanism-3337-ipam-namespace

80f73ba

bryanculver marked this pull request as ready for review June 2, 2023 19:20

bryanculver approved these changes Jun 2, 2023

View reviewed changes

gsnider2195 merged commit 03eaac7 into next Jun 2, 2023
19 checks passed

gsnider2195 deleted the prototype/jathanism-3337-ipam-namespace branch June 2, 2023 19:21

bryanculver mentioned this pull request Jun 3, 2023

IPAM Overhaul: Data Migration for Namespace, VRF, Prefix, IP Address #3681

Closed

gsnider2195 mentioned this pull request Jun 6, 2023

Re-enable skipped tests in next branch #3862

Merged

7 tasks

glennmatthews mentioned this pull request Jul 12, 2023

Revert 'temporary' workaround code from views/generic, add appropriate IPAM-specific form fixes in its place #4115

Merged

5 tasks

jvanderaa mentioned this pull request Jul 8, 2024

Support VRF device list assigment nautobot/nautobot-ansible#365

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce IPAM Namespaces #3378

Introduce IPAM Namespaces #3378

jathanism commented Feb 28, 2023 •

edited by bryanculver

Loading

bryanculver commented Mar 1, 2023

glennmatthews commented Mar 1, 2023

jathanism commented Mar 1, 2023

jathanism commented Mar 1, 2023

HanlinMiao commented Mar 7, 2023

jathanism commented Mar 9, 2023 •

edited

Loading

jathanism Mar 10, 2023

bryanculver left a comment

bryanculver Mar 10, 2023

bryanculver Mar 10, 2023

jathanism Mar 13, 2023

bryanculver Mar 10, 2023

jathanism Mar 13, 2023

jathanism Mar 14, 2023

bryanculver Mar 10, 2023

bryanculver commented Mar 10, 2023

gsnider2195 Jun 1, 2023

		@@ -44,6 +46,19 @@
		)


		class NamespaceFilterSet(NautobotFilterSet):

		@@ -228,6 +228,7 @@ def test_extend_schema_null_field_choices(self):
		self.assertIsInstance(getattr(schema, "resolve_mode"), types.FunctionType)


		@skip("Something really funky is broken here. Setup fails due to content types not existing...")

Introduce IPAM Namespaces #3378

Introduce IPAM Namespaces #3378

Conversation

jathanism commented Feb 28, 2023 • edited by bryanculver Loading

Closes: #3337, #3429, #3681

What's Changed

The gist

Changes that drifted related to VRF/RD

Caveats

TODO

bryanculver commented Mar 1, 2023

glennmatthews commented Mar 1, 2023

jathanism commented Mar 1, 2023

jathanism commented Mar 1, 2023

HanlinMiao commented Mar 7, 2023

jathanism commented Mar 9, 2023 • edited Loading

Latest changes:

Namespace

General

RouteDistinguisher

VRF

Prefix

IPAddress

Generic

Not Done

Choose a reason for hiding this comment

bryanculver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanculver commented Mar 10, 2023

Choose a reason for hiding this comment

jathanism commented Feb 28, 2023 •

edited by bryanculver

Loading

jathanism commented Mar 9, 2023 •

edited

Loading