A concrete example: https://gist.github.com/tyler-8/0a99763cae01c97e8f80f5aca09db968

I did a quick pass in the UI and found some things, ofc to be integrated with all the checks in the current reports that can be converted to field validators:

• site.slug: lower case, maybe also 5 chars?
• device.name: valid hostname (not FQDN, so no dots) regex, in addition if a 4 digit number is present the first digit must match the DC's digit prefix rule (eqiad => 1, etc.)
• device.asset_tag: custom WMF asset tag regex, to be removed from reports
• virtual_chassis.domain: FQDN regex (unless already validated by netbox)
• console_port.name: unclear from the data, most have console0, some console-re[0-1], Ripe atlas devices have different values
• console_server_port.name: port[1-9][0-9]?
• power_port.name: PSU[1-2] (the current report checks for PSU\d|PEM \d|Power Supply \d, we still need all of them?)
• power_outlet.name: a positive integer
• interface.name: see the INTERFACES_REGEXP in the current report
• cables

Agreed, we need to keep a close look at any risk of performance hit (eg. the ones that iterate over all objects), but a lot of reports/tests could be replaced by those validators.

We had an IRC chat with @Volans about its actual implementation especially regarding:

• How it's loaded (dynamic or not)
  • do we need to reload uwsgi?
  • Do we have to define them in the dynamic config or can we define them in the regular configuration.py?
• How to structure them
  • One multiple validator per model (eg. for multiple conditions), one validator per model?

So far the idea is to:

• Use the dotted-path format (more dynamic/flexible/explicit)
• Use 1 validator per model for performance reasons
• Put the file in netbox-deploy
  • No risk to break Netbox with a change to netbox-extra
  • Somehow have it exposed in the venv (TODO figure out how)
• Ideally use the configuration.py file config knob to keep all the config in a single location and benefit from git history

Adding also IPAddress.dns_name to the list (see T313960#8404985 for an example of typo)

I got it working by doing the following steps:

[...]
CUSTOM_VALIDATORS = {
    'ipam.ipaddress': (
        'validate_ipaddress.ValidateIpAddress',
    )
}

Then created the following file:

from extras.validators import CustomValidator

class ValidateIpAddress(CustomValidator):

    def validate(self, instance):
        if instance.dns_name.endswith('.'):
            self.fail(f"Invalid DNS name (shouldn't end with a dot)")

Technically though ln -s /etc/netbox/validate_ipaddress.py /srv/deployment/netbox/deploy/src/netbox/validate_ipaddress.py but it's not relevant here.

Then restarted sudo service uwsgi-netbox restart

And I'm now getting the proper error message when editing or creating an IPaddress.

Note that editing the validate_ipaddress.py file doesn't require a Netbox reload, only the configuration.py (as usual).

For the next steps I'm suggesting we:

1. create a validation directory in netbox-extra with models as subdirs (eg. ipam or dcim)
2. symlink that directory to /srv/deployment/netbox/deploy/src/netbox/validators (Unfortunately there is no "VALIDATE_ROOT" like there is "REPORTS_ROOT" global variables.)
3. Create various (even bare) <modelname>.py with a generically named main class (so the full path dotted path is for example validators.ipam.ipaddress.main)
4. Deploy them
5. Add them to configuration.py
6. Start adding actual validation logic to those files (and test on netbox-next/dev)
7. Merge them to netbox-extra, deploy to prod

The main advantage is to prevent back and forth between the Puppet and netbox-extra repo and not risking to break prod.

symlink that directory to /srv/deployment/netbox/deploy/src/netbox/validators

that will need to be part of the makefile

For the rest the plan looks sane to me.

Looks good to me, on possible change

create a validation directory in netbox-extra with models as subdirs (eg. ipam or dcim)
Create various (even bare) <modelname>.py with a generically named main class (so the full path dotted path is for example validators.ipam.ipaddress.main

The above may be overkill i think we could probably get away with less files e.g

/srv/deployment/netbox/deploy/src/netbox/validators/{ipam,dcmi}.py

from extras.validators import CustomValidator

class ValidateIpAddress(CustomValidator):

    def validate(self, instance):
        if instance.dns_name.endswith('.'):
            self.fail(f"Invalid DNS name (shouldn't end with a dot)")

[...]
CUSTOM_VALIDATORS = {
    'ipam.ipaddress': (
        'ipam.ValidateIpAddress',
    )
}

Change 889958 had a related patch set uploaded (by Ayounsi; author: Ayounsi):

[operations/software/netbox-extras@master] Add validator classes for some objects

https://gerrit.wikimedia.org/r/889958

Change 889959 had a related patch set uploaded (by Ayounsi; author: Ayounsi):

[operations/puppet@production] Netbox: activate validators

https://gerrit.wikimedia.org/r/889959

Change 889963 had a related patch set uploaded (by Ayounsi; author: Ayounsi):

[operations/software/netbox-deploy@wmf-next] Validators: add symlink to netbox-extra

https://gerrit.wikimedia.org/r/889963

I tested the above on netbox-next and fixed some bugs I found.

One oversight from yesterday is that in some cases uwsgi needs to be restarted when modifying the validator files. So not sure if there is a way to do so, or if we need to move those files in a different repo (Eg. netbox-deploy or Puppet). Not having to do a deployment at each change would be preferred though.

The above may be overkill i think we could probably get away with less files e.g
/srv/deployment/netbox/deploy/src/netbox/validators/{ipam,dcmi}.py

I find it cleaner to have one file per models. But let me know if you feel strongly about it.

In T310590#8625183, @ayounsi wrote:

The above may be overkill i think we could probably get away with less files e.g
/srv/deployment/netbox/deploy/src/netbox/validators/{ipam,dcmi}.py

I find it cleaner to have one file per models. But let me know if you feel strongly about it.

I'm inclined to agree, the one file per model, nested in a dir for validators, seems clean. Especially as the number of them grow.

In T310590#8625183, @ayounsi wrote:

The above may be overkill i think we could probably get away with less files e.g
/srv/deployment/netbox/deploy/src/netbox/validators/{ipam,dcmi}.py

I find it cleaner to have one file per models. But let me know if you feel strongly about it.

Ack good with me, no strong view

Change 900242 had a related patch set uploaded (by Jbond; author: jbond):

[operations/cookbooks@master] sre.netbox.deploy-extras: create a cookbook to deploy netbox-extras

https://gerrit.wikimedia.org/r/900242

Change 900316 had a related patch set uploaded (by Jbond; author: Ayounsi):

[operations/puppet@production] Netbox: introduce support for validators

https://gerrit.wikimedia.org/r/900316

Change 900317 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] netbox: add validators to canary host

https://gerrit.wikimedia.org/r/900317

Change 900318 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] netbox: add validators to production host

https://gerrit.wikimedia.org/r/900318

Change 889959 abandoned by Ayounsi:

[operations/puppet@production] Netbox: activate validators

Reason:

Moved to Ie4ac8c55e0423e7f5a8c93315dad5f6d31522529

https://gerrit.wikimedia.org/r/889959

Change 900316 merged by Jbond:

[operations/puppet@production] Netbox: introduce support for validators

https://gerrit.wikimedia.org/r/900316

Change 900242 merged by jenkins-bot:

[operations/cookbooks@master] sre.netbox.deploy-extras: create a cookbook to deploy netbox-extras

https://gerrit.wikimedia.org/r/900242

Change 900317 merged by Ayounsi:

[operations/puppet@production] netbox: add validators to canary host

https://gerrit.wikimedia.org/r/900317

The first round of validators is live on netbox-next. Please let me know if any bugs.

Change 889963 merged by Ayounsi:

[operations/software/netbox-deploy@wmf-next] Validators: add symlink to netbox-extra

https://gerrit.wikimedia.org/r/889963

Change 889958 merged by jenkins-bot:

[operations/software/netbox-extras@master] Add validator classes for some objects

https://gerrit.wikimedia.org/r/889958

Change 900318 merged by Ayounsi:

[operations/puppet@production] netbox: add validators to production host

https://gerrit.wikimedia.org/r/900318

Validators are now live in prod!

Change 917876 had a related patch set uploaded (by Ayounsi; author: Ayounsi):

[operations/software/netbox-extras@master] Validators: improve device name, add interface/outlet

https://gerrit.wikimedia.org/r/917876

Regarding the port/interfaces names, unlike interface names, the power and console ports name don't change often and haven't alerted regularly, so I suggest we keep them as reports for now (except the one I implemented in https://gerrit.wikimedia.org/r/917876 )

I hit this error when adding interfaces via the API/pynetbox.

em1 = nb.dcim.interfaces.create(name='em1', device=device.id, type='1000base-t', mgmt_only=True, enabled=False)

Results in:

pynetbox.core.query.RequestError: The request failed with code 400 Bad Request: {'__all__': ['Invalid count_ipaddresses (must not be set on disabled interfaces)']}

No IPs are being set, but must be some internal issue as it's created. Creating it and then immediately disabling does work:

em1 = nb.dcim.interfaces.create(name='em1', device=device.id, type='1000base-t', mgmt_only=True, enabled=True)
em1.enabled = False
em1.save()

Change 920656 had a related patch set uploaded (by Ayounsi; author: Ayounsi):

[operations/software/netbox-extras@master] interface validator: workaround bug with count_ipaddresses

https://gerrit.wikimedia.org/r/920656

Change 920690 had a related patch set uploaded (by Ayounsi; author: Ayounsi):

[operations/puppet@production] netbox-next: enable poweroutlet validator

https://gerrit.wikimedia.org/r/920690

Change 920691 had a related patch set uploaded (by Ayounsi; author: Ayounsi):

[operations/puppet@production] Netbox prod: add poweroutlet validator

https://gerrit.wikimedia.org/r/920691

Change 920690 merged by Ayounsi:

[operations/puppet@production] netbox-next: enable poweroutlet validator

https://gerrit.wikimedia.org/r/920690

Change 920656 merged by jenkins-bot:

[operations/software/netbox-extras@master] interface validator: workaround bug with count_ipaddresses

https://gerrit.wikimedia.org/r/920656

Change 917876 merged by jenkins-bot:

[operations/software/netbox-extras@master] Validators: improve device name, add interface/outlet

https://gerrit.wikimedia.org/r/917876

Change 920691 merged by Ayounsi:

[operations/puppet@production] Netbox prod: add poweroutlet validator

https://gerrit.wikimedia.org/r/920691

Change 923406 had a related patch set uploaded (by Cathal Mooney; author: Cathal Mooney):

[operations/software/netbox-extras@master] Fix cable validator to allow editing of existing cable

https://gerrit.wikimedia.org/r/923406

^^ this patch is to deal with an issue I hit modifying cable (changing status from planned to connected).

>>> cable = nb.dcim.cables.get(label="2023020702")
>>> cable.status.value
'planned'
>>> cable.status='connected'
>>> cable.save()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.10/dist-packages/pynetbox-6.6.2-py3.10.egg/pynetbox/core/response.py", line 538, in save
    if req.patch(updates):
  File "/usr/local/lib/python3.10/dist-packages/pynetbox-6.6.2-py3.10.egg/pynetbox/core/query.py", line 427, in patch
    return self._make_call(verb="patch", data=data)
  File "/usr/local/lib/python3.10/dist-packages/pynetbox-6.6.2-py3.10.egg/pynetbox/core/query.py", line 287, in _make_call
    raise RequestError(req)
pynetbox.core.query.RequestError: The request failed with code 400 Bad Request: {'__all__': ['Duplicate label with 2023020702']}

Seems to work ok, tested on netbox-next.

Change 923406 merged by jenkins-bot:

[operations/software/netbox-extras@master] Fix cable validator to allow editing of existing cable

https://gerrit.wikimedia.org/r/923406