starred/netbox
1
0
Fork 0
mirror of https://github.com/netbox-community/netbox.git synced 2026-01-11 21:10:29 +01:00
Code Issues 273 Packages Projects Releases 10 Wiki Activity

Missing error message when Tagged VLAN with incompatible Site is set #2941

New Issue
Closed
opened 2025-12-29 18:23:47 +01:00 by adam · 12 comments
adam commented 2025-12-29 18:23:47 +01:00
Owner
Copy Link

Originally created by @fknorn on GitHub (Oct 10, 2019).

Environment

  • Python version: Python version: 3.6.8
  • NetBox version: NetBox version: 2.6.5

Steps to Reproduce

  1. Create VLAN 123 associated with Site AAA
  2. Create Device at Site BBB and Interface
  3. Edit interface and set 802.1Q Mode to Tagged
  4. Select VLAN 123 as Tagged VLAN
  5. Apply

Expected Behavior

An Error message.

Observed Behavior

No error message, and the VLAN does not get set.

Comment

A error message is correctly produced if the Mode is Access and the VLAN is selected in the Untagged input field instead. Then we get "The untagged VLAN (123) must belong to the same site as the interface's parent device/VM, or it must be global". The same message should be show there too.

Originally created by @fknorn on GitHub (Oct 10, 2019). <!-- NOTE: This form is only for reproducible bugs. If you need assistance with NetBox installation, or if you have a general question, DO NOT open an issue. Instead, post to our mailing list: https://groups.google.com/forum/#!forum/netbox-discuss Please describe the environment in which you are running NetBox. Be sure that you are running an unmodified instance of the latest stable release before submitting a bug report. --> ### Environment * Python version: Python version: 3.6.8 * NetBox version: NetBox version: 2.6.5 <!-- Describe in detail the exact steps that someone else can take to reproduce this bug using the current stable release of NetBox (or the current beta release where applicable). Begin with the creation of any necessary database objects and call out every operation being performed explicitly. If reporting a bug in the REST API, be sure to reconstruct the raw HTTP request(s) being made: Don't rely on a wrapper like pynetbox. --> ### Steps to Reproduce 1. Create VLAN 123 associated with Site AAA 2. Create Device at Site BBB and Interface 3. Edit interface and set 802.1Q Mode to Tagged 4. Select VLAN 123 as Tagged VLAN 5. Apply <!-- What did you expect to happen? --> ### Expected Behavior An Error message. <!-- What happened instead? --> ### Observed Behavior No error message, and the VLAN does not get set. ### Comment A error message is correctly produced if the Mode is Access and the VLAN is selected in the Untagged input field instead. Then we get _"The untagged VLAN (123) must belong to the same site as the interface's parent device/VM, or it must be global"_. The same message should be show there too.
adam added the type: bugstatus: accepted labels 2025-12-29 18:23:47 +01:00
adam closed this issue 2025-12-29 18:23:47 +01:00
adam commented 2025-12-29 18:23:47 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Oct 10, 2019):

One issue is that the list of VLANs is not currently filtered by site/group assignment. This would actually be very difficult to implement fully given the current state of logic around API-based select fields. It would probably be best handled by introducing a new filter that returns all available VLANs for a given site (e.g. available_for_site=foo).

The other issue is that when an invalid VLAN is selected, we do not raise a validation error. I'm going to say that the scope of this particular bug report should be limited to this second issue only.

@jeremystretch commented on GitHub (Oct 10, 2019): One issue is that the list of VLANs is not currently filtered by site/group assignment. This would actually be very difficult to implement fully given the current state of logic around API-based select fields. It would probably be best handled by introducing a new filter that returns all available VLANs for a given site (e.g. `available_for_site=foo`). The other issue is that when an invalid VLAN is selected, we do not raise a validation error. I'm going to say that the scope of this particular bug report should be limited to this second issue only.
adam commented 2025-12-29 18:23:48 +01:00
Author
Owner
Copy Link

@DanSheps commented on GitHub (Oct 10, 2019):

The other issue is that when an invalid VLAN is selected, we do not raise a validation error. I'm going to say that the scope of this particular bug report should be limited to this second issue only.

I thought we did this in the clean() logic, did we not?

@DanSheps commented on GitHub (Oct 10, 2019): > The other issue is that when an invalid VLAN is selected, we do not raise a validation error. I'm going to say that the scope of this particular bug report should be limited to this second issue only. I thought we did this in the clean() logic, did we not?
adam commented 2025-12-29 18:23:48 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Oct 10, 2019):

If we do, it's not working. I was able to confirm the reported bug.

@jeremystretch commented on GitHub (Oct 10, 2019): If we do, it's not working. I was able to confirm the reported bug.
adam commented 2025-12-29 18:23:48 +01:00
Author
Owner
Copy Link

@DanSheps commented on GitHub (Oct 10, 2019):

Just checked, it only checks for untagged vlans.

@DanSheps commented on GitHub (Oct 10, 2019): Just checked, it only checks for untagged vlans.
adam commented 2025-12-29 18:23:48 +01:00
Author
Owner
Copy Link

@DanSheps commented on GitHub (Nov 14, 2019):

I am going to give this a stab and see if there is a way to have it perform more then one query for the API as well.

@DanSheps commented on GitHub (Nov 14, 2019): I am going to give this a stab and see if there is a way to have it perform more then one query for the API as well.
adam commented 2025-12-29 18:23:49 +01:00
Author
Owner
Copy Link

@hSaria commented on GitHub (Dec 31, 2019):

@jeremystretch can this be resolved the same way as additional_query_params in #3809?

If I'm understanding this correctly, the list of VLAN choices should be limited to global (null) and to the site of the device.

I've seen the logic in InterfaceForm (update: the code is no longer being used as it is being overridden by APISelectMultiple widget for the tagged_vlans field)
8a4293a4cc/netbox/dcim/forms.py (L2230)

but functionally, the result will be the same because

  • a VLAN can be in a global VLAN group only if it has no site, and
  • a VLAN can be in a site VLAN group only it it is in the same site.

Stated differently

  • a VLAN in a site cannot be in a global VLAN group, and
  • a VLAN not in a site cannot be in a site VLAN group.

Basically, the restrictions on the VLAN's site and group membership were already applied during the creation of the VLAN.

And if I'm not mistaken, the filtering logic for vlan_choices in InterfaceForm doesn't even need to check the VLAN groups' site due to the same reason – just gotta check the VLAN's site is null or that of the device.

I've tested the behaviour of the two things above (additional_query_params and cleaning the filter logic for vlan_choices) and they work as expected. Forcing an incorrect combination fails as it does right now.

@hSaria commented on GitHub (Dec 31, 2019): @jeremystretch can this be resolved the same way as `additional_query_params` in #3809? If I'm understanding this correctly, the list of VLAN choices should be limited to global (null) and to the site of the device. I've seen the logic in `InterfaceForm` (update: the code is no longer being used as it is being overridden by `APISelectMultiple` widget for the `tagged_vlans` field) https://github.com/netbox-community/netbox/blob/8a4293a4ccad0601532a6bdad63cee18a5f7bea1/netbox/dcim/forms.py#L2230 but functionally, the result will be the same because * a VLAN can be in a global VLAN group **only** if it has no site, and * a VLAN can be in a site VLAN group **only** it it is in the same site. Stated differently * a VLAN in a site **cannot** be in a global VLAN group, and * a VLAN not in a site **cannot** be in a site VLAN group. Basically, the restrictions on the VLAN's site and group membership were already applied during the creation of the VLAN. And if I'm not mistaken, the filtering logic for `vlan_choices` in `InterfaceForm` doesn't even need to check the VLAN **groups'** site due to the same reason – just gotta check the VLAN's site is null or that of the device. I've tested the behaviour of the two things above (`additional_query_params` and cleaning the filter logic for `vlan_choices`) and they work as expected. Forcing an incorrect combination fails as it does right now.
adam commented 2025-12-29 18:23:49 +01:00
Author
Owner
Copy Link

@hSaria commented on GitHub (Dec 31, 2019):

Turns out that because the tagged VLANs aren't cleaned, the update goes through to the database and is visible through the API. The VLAN is not present in the edit page so you can still (unknowingly) update it to a clean state by not including any of the invalid choices.

At any rate, I'm working on a PR for this.

@hSaria commented on GitHub (Dec 31, 2019): Turns out that because the tagged VLANs aren't cleaned, the update goes through to the database and is visible through the API. The VLAN is not present in the edit page so you can still (unknowingly) update it to a clean state by not including any of the invalid choices. At any rate, I'm working on a PR for this.
adam commented 2025-12-29 18:23:49 +01:00
Author
Owner
Copy Link

@hSaria commented on GitHub (Dec 31, 2019):

@jeremystretch I've got the validation fix ready, but in order to only show the valid choices in the VLAN selection, I'm gonna need to modify how additional_query_params works in order to put multiple values for the same field (an extension to what you did in f2c49063f8). Would that be okay or would you happen to know of a different way to get around this?

@hSaria commented on GitHub (Dec 31, 2019): @jeremystretch I've got the [validation fix](https://github.com/netbox-community/netbox/compare/develop...hSaria:3589-interface-tagged-vlans) ready, but in order to only show the valid choices in the VLAN selection, I'm gonna need to modify how `additional_query_params` works in order to put multiple values for the same field (an extension to what you did in https://github.com/netbox-community/netbox/commit/f2c49063f85e1b0a3a42bfe1e16ac5ffb4939bc7). Would that be okay or would you happen to know of a different way to get around this?
adam commented 2025-12-29 18:23:49 +01:00
Author
Owner
Copy Link

@DanSheps commented on GitHub (Jan 1, 2020):

@hSaria

The filtering logic needs to remain in place for init(). If not, you will have a timeout if you have a large number of vlans.

If you want to start a PR for this, I can take a look at it more too.

@DanSheps commented on GitHub (Jan 1, 2020): @hSaria The filtering logic needs to remain in place for init(). If not, you will have a timeout if you have a large number of vlans. If you want to start a PR for this, I can take a look at it more too.
adam commented 2025-12-29 18:23:49 +01:00
Author
Owner
Copy Link

@hSaria commented on GitHub (Jan 1, 2020):

@DanSheps, I see the pointing you're raising. I'll update it.

The current state is that it loads the valid options but they will not be seen because of the widget.

<select name="tagged_vlans" class="netbox-select2-api form-control select2-hidden-accessible" data-url="/api/ipam/vlans/" data-full="" display-field="display_name" data-multiple="1" placeholder="None" id="id_tagged_vlans" multiple="" tabindex="-1" aria-hidden="true">
  <optgroup label="Global">
    <option value="2" selected="">2 (vlan2)</option>
  </optgroup>
  <optgroup label="global">
  </optgroup>
  <optgroup label="London">
    <option value="1">1 (vlan1)</option>
  </optgroup>
</select>

If you click on the select, the APISelectMultiple widget returns all of the VLANs and ignores the choices that were preloaded

image

By removing the code, it is loading all of the VLANs.

<select name="tagged_vlans" class="netbox-select2-api form-control select2-hidden-accessible" data-url="/api/ipam/vlans/" data-full="" display-field="display_name" data-additional-query-param-site_id="null" data-multiple="1" placeholder="None" id="id_tagged_vlans" multiple="" tabindex="-1" aria-hidden="true">
  <option value="3">3 (vlan3)</option>
  <option value="4">100 (100)</option>
  <option value="1">1 (vlan1)</option>
  <option value="2" selected="">2 (vlan2)</option>
</select>

It's odd that it would load all of the options, but I suppose that's because it inherits from Django's form.SelectMultiple. Wouldn't hurt for APISelect classes to be optimized in that regard. In fact, the only options that need to be preloaded onto the form are the ones that are currently selected as they need to be displayed. The rest are automatically added by the APISelect classes when one of them is picked. (Update: created #3812 for this optimization.)

An alternative would to change the widget to StaticSelect2Multiple, but I'd personally steer away from that because

  • it'll load the entire list of valid VLANs, where as APISelectMultiple will only load 50 at a time and more as you scroll,
  • the page will always load the VLANs even when an interface might not need it (what you pointed out, but limited to all the valid VLANs), and
  • it will make it so you would have to reload the page in order to get the new list of VLANs (APISelectMultiple would only require closing and reopening the selection after you created the VLAN).

What I'm currently trying for this issue is including two site_id parameters (similar to #3809), but that's currently only possible by using two different methods – filter_for on the non-existent site field and additional_query_params on the VLAN fields.

@hSaria commented on GitHub (Jan 1, 2020): @DanSheps, I see the pointing you're raising. I'll update it. The current state is that it loads the valid options but they will not be seen because of the widget. ```html <select name="tagged_vlans" class="netbox-select2-api form-control select2-hidden-accessible" data-url="/api/ipam/vlans/" data-full="" display-field="display_name" data-multiple="1" placeholder="None" id="id_tagged_vlans" multiple="" tabindex="-1" aria-hidden="true"> <optgroup label="Global"> <option value="2" selected="">2 (vlan2)</option> </optgroup> <optgroup label="global"> </optgroup> <optgroup label="London"> <option value="1">1 (vlan1)</option> </optgroup> </select> ``` If you click on the select, the `APISelectMultiple` widget returns *all* of the VLANs and ignores the choices that were preloaded <img width="552" alt="image" src="https://user-images.githubusercontent.com/34197532/71640744-ead11700-2c87-11ea-9e4e-b955c3fc43f7.png"> By removing the code, it is loading all of the VLANs. ```html <select name="tagged_vlans" class="netbox-select2-api form-control select2-hidden-accessible" data-url="/api/ipam/vlans/" data-full="" display-field="display_name" data-additional-query-param-site_id="null" data-multiple="1" placeholder="None" id="id_tagged_vlans" multiple="" tabindex="-1" aria-hidden="true"> <option value="3">3 (vlan3)</option> <option value="4">100 (100)</option> <option value="1">1 (vlan1)</option> <option value="2" selected="">2 (vlan2)</option> </select> ``` It's odd that it would load all of the options, but I suppose that's because it inherits from Django's `form.SelectMultiple`. Wouldn't hurt for `APISelect` classes to be optimized in that regard. In fact, the only options that need to be preloaded onto the form are the ones that are currently selected as they need to be displayed. The rest are automatically added by the `APISelect` classes when one of them is picked. (Update: created #3812 for this optimization.) An alternative would to change the widget to `StaticSelect2Multiple`, but I'd personally steer away from that because * it'll load the entire list of *valid* VLANs, where as `APISelectMultiple` will only load 50 at a time and more as you scroll, * the page will always load the VLANs even when an interface might not need it (what you pointed out, but limited to *all* the valid VLANs), and * it will make it so you would have to reload the page in order to get the new list of VLANs (`APISelectMultiple` would only require closing and reopening the selection after you created the VLAN). What I'm currently trying for this issue is including two `site_id` parameters (similar to #3809), but that's currently only possible by using two different methods – `filter_for` on the non-existent site field and `additional_query_params` on the VLAN fields.
adam commented 2025-12-29 18:23:50 +01:00
Author
Owner
Copy Link

@hSaria commented on GitHub (Jan 1, 2020):

@DanSheps #3813 will solve the unnecessary load of options, so there won't be any need for the the manual choices on the API-based select field (only selected values are loaded).

The current untagged and tagged VLANs select fields are still not filtering the VLAN's site to current or global, so that's still left to be done.

@hSaria commented on GitHub (Jan 1, 2020): @DanSheps #3813 will solve the unnecessary load of options, so there won't be any need for the the manual choices on the API-based select field (only selected values are loaded). The current untagged and tagged VLANs select fields are still not filtering the VLAN's site to current or global, so that's still left to be done.
adam commented 2025-12-29 18:23:50 +01:00
Author
Owner
Copy Link

@hSaria commented on GitHub (Jan 1, 2020):

Found a workaround if we want to avoid modifying the existing code for handling additional parameters.

For the widget, add the site_id=null parameter (as intended).

additional_query_params={
    'site_id': 'null'
    }

In the init, add the following

self.fields['untagged_vlan'].widget.attrs['1-data-additional-query-param-site_id'] = self.parent_obj.site.pk
self.fields['tagged_vlans'].widget.attrs['1-data-additional-query-param-site_id'] = self.parent_obj.site.pk

This works around the current code in forms.js. Specifically

if (attr.name.includes("data-additional-query-param-")){
    var param_name = attr.name.split("data-additional-query-param-")[1];

I'll add this to a PR and we'll see how we go about this. Assuming we get more instances of this, it might be better to find a long-term way of adding multiple values to the same parameter.

@hSaria commented on GitHub (Jan 1, 2020): Found a workaround if we want to avoid modifying the existing code for handling additional parameters. For the widget, add the `site_id=null` parameter (as intended). ```python additional_query_params={ 'site_id': 'null' } ``` In the init, add the following ```python self.fields['untagged_vlan'].widget.attrs['1-data-additional-query-param-site_id'] = self.parent_obj.site.pk self.fields['tagged_vlans'].widget.attrs['1-data-additional-query-param-site_id'] = self.parent_obj.site.pk ``` This works around the current code in forms.js. Specifically ```javascript if (attr.name.includes("data-additional-query-param-")){ var param_name = attr.name.split("data-additional-query-param-")[1]; ``` I'll add this to a PR and we'll see how we go about this. Assuming we get more instances of this, it might be better to find a long-term way of adding multiple values to the same parameter.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
beta
breaking change
complexity: high
complexity: low
complexity: medium
needs milestone
netbox
pending closure
plugin candidate
pull-request
Mirrored from GitHub Pull Request
 severity: high
severity: low
severity: medium
status: accepted
status: backlog
status: blocked
status: duplicate
status: needs owner
status: needs triage
status: revisions needed
status: under review
topic: GraphQL
topic: Internationalization
topic: OpenAPI
topic: UI/UX
topic: cabling
topic: event rules
topic: htmx navigation
topic: industrialization
topic: migrations
topic: plugins
topic: scripts
topic: templating
topic: testing
type: bug
type: deprecation
type: documentation
type: feature
type: housekeeping
type: translation
No Label status: accepted type: bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adam
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/netbox#2941
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?