Missing error message when Tagged VLAN with incompatible Site is set

[fknorn]

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.

[jeremystretch]

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.

[DanSheps]

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?

[jeremystretch]

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

[DanSheps]

Just checked, it only checks for untagged vlans.

[DanSheps]

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.

[hSaria]

@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)

netbox/netbox/dcim/forms.py

Line 2230 in 8a4293a

# Limit VLan choices to those in: global vlans, global groups, the current site's group, the current site

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]

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]

@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 f2c4906). Would that be okay or would you happen to know of a different way to get around this?

[DanSheps]

@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.

[hSaria]

@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

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]

@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]

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.