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

Specifying a site during the creation or import of a prefix prevents seleting a VLAN unassigned to a site #12622

Closed
dhenschen opened this issue May 16, 2023 · 8 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application

Comments

@dhenschen
Copy link
Contributor

NetBox version

v3.5.1

Python version

3.10

Steps to Reproduce

Steps for reproducing creating a new Prefix:

  1. Login to the netbox Demo site at https://demo.netbox.dev
  2. Import the following VLAN from with the YAML format under IPAM > VLANS > VLANs > Import
vid: 1000
name: management
status: active
  1. Create a new Prefix from IPAM > PREFIXES > Prefixes > Add.
  2. Set the Prefix field to 10.1.0.0/24
  3. Set the Site field to DM-Akron
  4. Look for the new management VLAN in the filtered VLAN set. The vlan is not listed when it seems like it should be since it is not assigned to a site

Steps for reproducing importing a new Prefix:

  1. Login to the netbox Demo site at https://demo.netbox.dev
  2. Import the following VLAN from with the YAML format under IPAM > VLANS > VLANs > Import
vid: 1000
name: management
status: active
  1. Import a new Prefix from IPAM > PREFIXES > Prefixes > Import with the following YAML
prefix: 10.1.1.0/24
status: active
site: DM-Akron
vlan: 1000

Expected Behavior

For both creation and import, I expect to be able to assign VLANs from the same site as the prefix or VLANs without a site assignment. This is the same expectation found in issue #11619.

Observed Behavior

  • For the creation, any vlan without a site assigned is not available for selection if the prefix is assigned a site. However, If one sets the vlan before setting the site, the management vlan can be set during creation. This work around is not convenient or intuitive to users.
  • For the import a toast pops up in the UI that states Record 1 vlan: Object not found: 1000

This is similar to #11619, but it impacts the IPAM app instead of DCIM. I felt that the root cause and fix was different enough that it warranted a separate issue.

I'm interesting in working this issue if it is approved.

@dhenschen dhenschen added the type: bug A confirmed report of unexpected behavior in the application label May 16, 2023
@DanSheps
Copy link
Member

DanSheps commented May 16, 2023

The simple solution is to switch to the selectors for filtering.

This might require some backend changes through as well.

@dhenschen
Copy link
Contributor Author

dhenschen commented May 17, 2023

Possible Feature Request

@DanSheps , I appreciate your suggestion. I elaborated on my thoughts in next two sections below.

Benefits of VLAN Selector

I believe implementing a selector similar to the site selector could enhance the user experience and address another issue outlined below.

Currently, when creating a prefix without specifying a site, the filter dialog displays all VLANs that match, without providing any context about the site they belong to. This lack of context can make it challenging for users to determine the VLAN they actually want to assign, especially when there are overlaps in names and vids. Below is an example of this from the demo data.
image

By adopting a selector approach similar to the site selector, we can provide users with the necessary information and context to make informed decisions when selecting VLANs for their prefixes.

Downsides of VLAN Selector

However, this above example could be mitigated by the user choosing a site before selecting the VLAN. Adding a selector may require some additional verification checks to make sure the VLAN selection and site selection do not contradict.

I did a test and it doesn't look like there is any current checks in NetBox to ensure that the site of a VLAN matches the site of a Prefix.

  1. Create a new Prefix from IPAM > PREFIXES > Prefixes > Add.
  2. Set the Prefix field to 10.1.0.0/24
  3. Set the Site field to DM-Akron
  4. Set the Site field to the Data (100) VLAN of DM-Akron
  5. Set the Site field to DM-Binghamton
  6. Click Create

This creates a prefix belonging to New York / DM-Binghamton, but the vlan belongs to Ohio / DM-Akron. One is able to create this mismatch in v3.5.1.

Current Bugfix

It is possible to fix this bug by enhancing some existing query filters. I opened the following draft pull request to outline the needed changes: #12634

dhenschen added a commit to dhenschen/netbox that referenced this issue May 17, 2023
dhenschen added a commit to dhenschen/netbox that referenced this issue May 17, 2023
dhenschen added a commit to dhenschen/netbox that referenced this issue May 17, 2023
@dhenschen dhenschen changed the title When a site is specified during the creation or import of a prefix, one cannot select a VLAN that does not have a site assignment Specifying a site during the creation or import of a prefix prevents seleting a VLAN unassigned to a site May 17, 2023
dhenschen added a commit to dhenschen/netbox that referenced this issue May 17, 2023
dhenschen added a commit to dhenschen/netbox that referenced this issue May 18, 2023
dhenschen added a commit to dhenschen/netbox that referenced this issue May 18, 2023
…hout site

This commit also adds tests to verify the import changes implemented
in this commit.
dhenschen added a commit to dhenschen/netbox that referenced this issue May 18, 2023
dhenschen added a commit to dhenschen/netbox that referenced this issue May 25, 2023
dhenschen added a commit to dhenschen/netbox that referenced this issue May 25, 2023
… site

This commit also adds tests to verify the import changes implemented
in this commit.
dhenschen added a commit to dhenschen/netbox that referenced this issue May 25, 2023
@dhenschen
Copy link
Contributor Author

dhenschen commented May 25, 2023

If this issue is accepted, I would like to take ownership of it.

I've implemented a fix for the current filter UI that would resolve the bug discussed in this issue. See #12634 I also implemented the addition of the selector. See my comment below.

As for the bigger selector discussion, I think that could also be addressed as part of this issue in a separate PR. However, I'd like to see the bug addressed first. I thought the scope of the selector was going to be a significantly larger change. It turns out I am wrong. Please see the comment below.

dhenschen added a commit to dhenschen/netbox that referenced this issue May 25, 2023
dhenschen added a commit to dhenschen/netbox that referenced this issue May 25, 2023
dhenschen added a commit to dhenschen/netbox that referenced this issue May 25, 2023
… site

This commit also adds tests to verify the import changes implemented
in this commit.
dhenschen added a commit to dhenschen/netbox that referenced this issue May 25, 2023
dhenschen added a commit to dhenschen/netbox that referenced this issue May 25, 2023
@dhenschen
Copy link
Contributor Author

dhenschen commented May 25, 2023

@DanSheps, I've discovered that the changes to implement a selector are extremely simple as you stated in your previous comment. Based on the work already completed in https://github.com/netbox-community/netbox/pull/11952/files, the diff to add a selector for VLAN in the prefix creation UI (IPAM > PREFIXES > Prefixes > Add) is as follows:

diff --git a/netbox/ipam/forms/model_forms.py b/netbox/ipam/forms/model_forms.py
index cf8117bf7..a4af2ad19 100644
--- a/netbox/ipam/forms/model_forms.py
+++ b/netbox/ipam/forms/model_forms.py
@@ -211,10 +211,8 @@ class PrefixForm(TenancyForm, NetBoxModelForm):
     vlan = DynamicModelChoiceField(
         queryset=VLAN.objects.all(),
         required=False,
+        selector=True,
         label=_('VLAN'),
-        query_params={
-            'site_id': '$site',
-        }
     )
     role = DynamicModelChoiceField(
         queryset=Role.objects.all(),

I tested this locally off of the develop branch and it accomplishes exactly what I'd like to see from this reported bug. Below is a screenshot created by following Steps to Reproduce included in this issue description.

image

@MarcHagen I think this change would also address what you reported in issue: #12619

image

I have the changes on saved on a local branch of my fork. I would be interested in opening a Pull Request to implement this selector. See https://github.com/dhenschen/netbox/tree/12622-fix-assigning-vlan-without-site-to-prefix

@DanSheps
Copy link
Member

Would you like to implement the changes?

@dhenschen
Copy link
Contributor Author

@DanSheps Yes, I would like to implement the changes. Thanks!

@ITJamie
Copy link
Contributor

ITJamie commented May 29, 2023

This would also solve the issue we have just found where the removal of the vlan_groups from the prefix model makes it hard to find the correct vlan to assign a prefix to on a site with many prefixes with the same vid or name

@DanSheps DanSheps added the status: accepted This issue has been accepted for implementation label May 29, 2023
dhenschen added a commit to dhenschen/netbox that referenced this issue Jun 1, 2023
dhenschen added a commit to dhenschen/netbox that referenced this issue Jun 1, 2023
… site

This commit also adds tests to verify the import changes implemented
in this commit.
dhenschen added a commit to dhenschen/netbox that referenced this issue Jun 1, 2023
dhenschen added a commit to dhenschen/netbox that referenced this issue Jun 1, 2023
@MarcHagen
Copy link
Contributor

@dhenschen Thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
Development

No branches or pull requests

4 participants