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

[Bug]: Automatic name-to-slug feature doesn't remove umlauts like Netbox GUI does #808

Open
jantari opened this issue Jul 7, 2022 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@jantari
Copy link

jantari commented Jul 7, 2022

Ansible NetBox Collection version

3.7.1

Ansible version

ansible [core 2.12.7]
  config file = /builds/it/fortigates/ansible.cfg
  configured module search path = ['/root/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.10/site-packages/ansible
  ansible collection location = /root/.ansible/collections:/usr/share/ansible/collections
  executable location = /usr/bin/ansible
  python version = 3.10.4 (main, Apr 30 2022, 16:49:16) [GCC 11.2.1 20220219]
  jinja version = 3.1.2
  libyaml = True

NetBox version

v3.2.2

Python version

3.10

Steps to Reproduce

- name: Reproduce bug
  netbox.netbox.netbox_site:
    netbox_url: '{{ netbox_url }}'
    netbox_token: '{{ netbox_token }}'
    state: present
    data:
      name: '{{ item }}'
      tags: [FortiGate-Sync]
  loop:
    - Königliches Rechenzentrum
    - Skåne
    - Keep an eye on this site 👀

Expected Behavior

The three site names should be "slugified" to:

  • knigliches-rechenzentrum
  • skne
  • keep-an-eye-on-this-site

Observed Behavior

The task fails all site names with "umlauts" and creates the site with the emoji, but with a different slug than what the Netbox UI would use:

TASK [Reproduce slugification bug] ************************************************************************************
failed: [localhost] (item=Königliches Rechenzentrum) => changed=false
  ansible_loop_var: item
  item: Königliches Rechenzentrum
  msg: '{"slug":["Enter a valid \"slug\" consisting of letters, numbers, underscores or hyphens."]}'
failed: [localhost] (item=Skåne) => changed=false
  ansible_loop_var: item
  item: Skåne
  msg: '{"slug":["Enter a valid \"slug\" consisting of letters, numbers, underscores or hyphens."]}'
changed: [localhost] => (item=Keep an eye on this site 👀)

Netbox 3.2.2 UI slug for the last site:
image

What the netbox_site ansible action created (note the trailing hyphen):
image


I've prepared a PR to fix both of these issues (umlauts causing an error and emojis producing a trailing slash)

@jantari jantari added the bug Something isn't working label Jul 7, 2022
jantari pushed a commit to jantari/netbox_ansible_modules that referenced this issue Jul 7, 2022
@sc68cal
Copy link
Contributor

sc68cal commented Jul 7, 2022

I believe this needs to be fixed in netbox. Specifically, letters should mean any valid UTF8 character, for a slug

@jantari
Copy link
Author

jantari commented Jul 7, 2022

Maybe, maybe not. I definitely see the value in only allowing basic ASCII characters for values like the slug that essentially act as a unique identifier. If they'd allow RTL, non-breaking spaces, all the letter-lookalikes and other such Unicode goodness that wouldn't really result in a net usability benefit imo

The displayname of all assets is already fully Unicode capable.

Whether Netbox changes their rules for characters allowed in slugs in the future or not, I think this module should stay aligned with their behavior and currently that is to strip these characters out. Personally I'm from Germany and perfectly OK with that :)

@sc68cal
Copy link
Contributor

sc68cal commented Jul 7, 2022

I see netbox-community/netbox#3741

In fact, netbox-community/netbox#3741 (comment) says that

the fix for netbox-community/netbox#3721 now allows general Unicode characters in the slug. Hence the code which generates the slug could be changed to allow everything through (except spaces to dashes)

So, again, I don't think coercing everything into ASCII is the correct approach.

@jantari
Copy link
Author

jantari commented Jul 8, 2022

  1. Auto-creation of slugs for non-ASCII users netbox#3741 is just a UI issue and was closed as being adressed by Non-ascii tag creates empty slug, exception django.urls.exceptions.NoReverseMatch netbox#3721 which in turn was closed by netbox-community/netbox@606f3da - but from my testing and by this users comment Auto-creation of slugs for non-ASCII users netbox#3741 (comment) it is not actually resolved. The UI still strips all non-ASCII characters from autosuggested slugs and this is easy to verify. So Auto-creation of slugs for non-ASCII users netbox#3741, while only a UI-side issue not relating directly to ansible, is in fact not fixed and may have been closed in error.

  2. If you read the diff and release note of netbox-community/netbox@606f3da, you'll see that it only ever applied to the slugs of tags specifically. The slugification logic of all other objects (e.g. sites) was not changed to allow unicode from what I can see.

  3. Even WITH the changes made in netbox-community/netbox@606f3da (shipped in Netbox 2.7.2) and netbox-community/netbox@8d547e9 (shipped in Netbox 2.7.3), the creation of a tag with unicode characters in its slug still fails in the latest Netbox 3.2.2 both through the GUI and through the API / this ansible module:

    - name: Reproduce slug error with tags that are supposed to allow unicode
      netbox.netbox.netbox_tag:
        netbox_url: '{{ netbox_url }}'
        netbox_token: '{{ netbox_token }}'
        state: present
        data:
          name: 'Testing Unicode: 台灣'
          slug: 'testing-unicode-台灣'

    As you can see I took the exact test case from netbox-community/netbox@606f3da#diff-a3fd33970129d5cb92c21fe1dba39cae6f22ab4b1e0c39d903cdd128f24b1de8 and yet it still didn't work:

    TASK [Reproduce slugification bug with tags] *********************************************************************
    fatal: [localhost]: FAILED! => changed=false
      msg: '{"slug":["Enter a valid \"slug\" consisting of letters, numbers, underscores or hyphens."]}'
    
  4. Remember that I'm not arguing for or against allowing unicode in slugs - that is a discussion for the Netbox application and its repository. The problem in this issue, with the ansible collection / modules, is that they will generate and attempt to pass a slug to the Netbox API that is simply not allowed today and therefore results in a hard error. Hard errors due to invalid data is a bug, no matter what you personally think of Netbox' present-day behavior. That can be addresses separately in their repository if unicode support in slugs is important to you.

  5. The documentation for this ansible collection repeatedly states: "This [referring to the slug] is auto-generated following NetBox rules if not provided" example but as is shown and reproducible here, that is not true. The slug is generated differently from NetBox rules, again (imo) fairly clearly making this a bug


I am a little sorry for the lengthy response, I am not a native english speaker and I don't mean any of it as an attack.

If Netbox' rules for this ever change, PR #809 can be rolled back to support the new behavior (aka unicode) and everything is going to be fine - but most importantly, no hard errors for the users due to invalid slugs.

@sc68cal
Copy link
Contributor

sc68cal commented Jul 11, 2022

Hi,

Thank you for your lengthy response. Your research matches a lot of what I also researched, and I think what I am driving towards is that we need to fix this bug in NetBox itself, rather than destructively replacing user inputs with ASCII only characters in the Ansible modules for NetBox. Specifically, the fact that every other object's slugify code needs to be fixed, not just the tag object.

I should have made this more explicit in my responses, so I apologize.

(P.S. Do not worry about your responses coming off as an attack. In fact, unless you had not said anything, to my mind you come off as a native english speaker, so do not feel obligated to apologize)

@jantari
Copy link
Author

jantari commented Jul 25, 2022

Ok.

The workaround we'll have to implement until that is addressed is to manually implement the correct slugification logic in ansible/jinja - posting because this may help others who run into this same issue:

- name: Properly slugify site or other object names
  debug:
    msg: '{{ item | regex_replace("(?a)[^\-\.\w\s]", "") | trim | regex_replace("[\-\.\s]+", "-") | lower }}'
  loop:
    - Königliches Rechenzentrum
    - Skåne
    - Keep an eye on this site 👀

task output:

TASK [Properly slugify site or other object names] *******************************************************************
ok: [localhost] => (item=Königliches Rechenzentrum) => {
    "msg": "knigliches-rechenzentrum"
}
ok: [localhost] => (item=Skåne) => {
    "msg": "skne"
}
ok: [localhost] => (item=Keep an eye on this site 👀) => {
    "msg": "keep-an-eye-on-this-site"
}

use in a full task to manually pass a valid slug:

- name: Reproduce bug
  netbox.netbox.netbox_site:
    netbox_url: '{{ netbox_url }}'
    netbox_token: '{{ netbox_token }}'
    state: present
    data:
      name: '{{ item }}'
      slug: '{{ item | regex_replace("(?a)[^\-\.\w\s]", "") | trim | regex_replace("[\-\.\s]+", "-") | lower }}'
      tags: [FortiGate-Sync]
  loop:
    - Königliches Rechenzentrum
    - Skåne
    - Keep an eye on this site 👀

EDIT:

This custom slug will then also have to be passed to every single task that creates an object which references an object whose slug would be auto-guessed wrong by this module. E.g. Once you've created a site "Skåne" with the proper slug "skne" then when you attempt to create a VLAN referencing that site:

- name: Create VLANs
  netbox.netbox.netbox_vlan:
    netbox_url: '{{ netbox_url }}'
    netbox_token: '{{ netbox_token }}'
    state: present
    data:
      name: "{{ item.name }}"
      site: '{{ item.site }}'
      description: "Funny VLAN for bug repro"
      vid: "{{ item.vlanid }}"
      tags: [FortiGate-Sync]
  loop:
    - name: Funny-VLAN
      site: Skåne
      vlanid: 42

you will get an error again because the value passed for "site" is auto-slugified wrong again and so the site cannot be found:

msg: 'Could not resolve id of site: skåne'

so the manual slugification workaround will have to be applied at every single task e.g.:

- name: Create VLANs
  netbox.netbox.netbox_vlan:
    netbox_url: '{{ netbox_url }}'
    netbox_token: '{{ netbox_token }}'
    state: present
    data:
      name: "{{ item.name }}"
      site: '{{ item.site | regex_replace("(?a)[^\-\.\w\s]", "") | trim | regex_replace("[\-\.\s]+", "-") | lower }}'
      description: "Funny VLAN for bug repro"
      vid: "{{ item.vlanid }}"
      tags: [FortiGate-Sync]
  loop:
    - name: Funny-VLAN
      site: Skåne
      vlanid: 42

@sc68cal
Copy link
Contributor

sc68cal commented Aug 2, 2022

I am looking for issues that we can re-open on the NetBox server side. netbox-community/netbox#3741 would be ideal to re-open but it is locked and I cannot unlock.

Worst case I will open a new issue and link everything

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants