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 validation of Site consistency between Device, Rack, Rackgroup, and PowerPanel #4247

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

Originally created by @candlerb on GitHub (Nov 5, 2020).

Originally assigned to: @jeremystretch on GitHub.

(Extracted from #4971)

Environment

  • Python version: 3.6.9
  • NetBox version: 2.9.8

Summary: missing validation of the consistency of links from Device, Rack and Rackgroup tree to Site.

Steps to Reproduce

Case 1: Device-Site not matching Rack-Site

1. Create sites A and B
2. Create rack R in site A
3. Create device D in site A and rack R
4. Change rack R to site B

Case 2: Rack-Site not matching Rackgroup-Site

  1. Create sites A and B
  2. Create rackgroup G in site A
  3. Create rack R in site A and rackgroup G
  4. Change rackgroup G to site B

Case 3: Rackgroup-Site not matching parent Rackgroup-Site

  1. Create sites A and B
  2. Create rackgroup G1 in site A
  3. Create rackgroup G2 in site A with parent rackgroup G1
  4. Change rackgroup G1 to site B

Case 4: These relationships also apply transitively: parent rackgroup -> child rackgroup -> rack -> device

  1. Create sites A and B
  2. Create rackgroup G1 in site A
  3. Create rackgroup G2 in site A with parent rackgroup G1
  4. Create rack R in site A and rackgroup G2
  5. Create device D in site A and rack R
  6. Change rackgroup G1 to site B

Expected Behavior

In case 1: either change to be rejected, or rack R's new site to be propagated to all Devices underneath it
In case 2: either change to be rejected, or rack group G's new site to be propagated to all Racks underneath it
In case 3: either change to be rejected, or rack group G1's new site to be propagated to all child and descendant rackgroups
In case 4: either change to be rejected, or rack group G1's new site to be propagated recursively to all child and descendant rackgroups, racks and devices

Observed Behavior

In case 1: device D is in site A but its rack is in site B
In case 2: rack R is in site A but its rackgroup is in site B
In case 3: rackgroup G2 is in site A but its parent rackgroup G1 is in site B
In case 4: device D, rack R, rackgroup G2 are all in site A but the ancestor rackgroup G1 is in site B

Additional Note

PowerPanel also has a link to Site and RackGroup, so a similar issue applies there too.

Originally created by @candlerb on GitHub (Nov 5, 2020). Originally assigned to: @jeremystretch on GitHub. (Extracted from #4971) ### Environment * Python version: 3.6.9 * NetBox version: 2.9.8 Summary: missing validation of the consistency of links from Device, Rack and Rackgroup tree to Site. ### Steps to Reproduce ~~Case 1: Device-Site not matching Rack-Site~~ ~~1. Create sites A and B~~ ~~2. Create rack R in site A~~ ~~3. Create device D in site A and rack R~~ ~~4. Change rack R to site B~~ Case 2: Rack-Site not matching Rackgroup-Site 1. Create sites A and B 2. Create rackgroup G in site A 3. Create rack R in site A and rackgroup G 4. Change rackgroup G to site B Case 3: Rackgroup-Site not matching parent Rackgroup-Site 1. Create sites A and B 2. Create rackgroup G1 in site A 3. Create rackgroup G2 in site A with parent rackgroup G1 4. Change rackgroup G1 to site B Case 4: These relationships also apply transitively: parent rackgroup -> child rackgroup -> rack -> device 1. Create sites A and B 2. Create rackgroup G1 in site A 3. Create rackgroup G2 in site A with parent rackgroup G1 4. Create rack R in site A and rackgroup G2 5. Create device D in site A and rack R 6. Change rackgroup G1 to site B ### Expected Behavior ~~In case 1: either change to be rejected, or rack R's new site to be propagated to all Devices underneath it~~ In case 2: either change to be rejected, or rack group G's new site to be propagated to all Racks underneath it In case 3: either change to be rejected, or rack group G1's new site to be propagated to all child and descendant rackgroups In case 4: either change to be rejected, or rack group G1's new site to be propagated recursively to all child and descendant rackgroups, racks and devices ### Observed Behavior ~~In case 1: device D is in site A but its rack is in site B~~ In case 2: rack R is in site A but its rackgroup is in site B In case 3: rackgroup G2 is in site A but its parent rackgroup G1 is in site B In case 4: device D, rack R, rackgroup G2 are all in site A but the ancestor rackgroup G1 is in site B ### Additional Note PowerPanel also has a link to Site and RackGroup, so a similar issue applies there too.
adam added the type: bugstatus: accepted labels 2025-12-29 18:34:13 +01:00
adam closed this issue 2025-12-29 18:34:13 +01:00
adam commented 2025-12-29 18:34:13 +01:00
Author
Owner
Copy Link

@DanSheps commented on GitHub (Dec 18, 2020):

I think the solution to this, would be to really use a concrete base model and have every other hierarchical model inherit off of that.

Example:

class Location(MPTTModel):
    name = models.CharField

class Region(Location):
    # Region specific stuff here

class Site(Location):
    # Site specific stuff here

class RackGroup(Location):
    # RG specific stuff here

This would allow you to enforce the integrity properly no matter where you are.

@DanSheps commented on GitHub (Dec 18, 2020): I think the solution to this, would be to really use a concrete base model and have every other hierarchical model inherit off of that. Example: ``` class Location(MPTTModel): name = models.CharField class Region(Location): # Region specific stuff here class Site(Location): # Site specific stuff here class RackGroup(Location): # RG specific stuff here ``` This would allow you to enforce the integrity properly no matter where you are.
adam commented 2025-12-29 18:34:13 +01:00
Author
Owner
Copy Link

@DanSheps commented on GitHub (Dec 22, 2020):

Here is the idea so far:

  • Move to a base model, "Location"; location would be MPTT
  • Name & Slug would be moved to location; all other data would remain on the inheritees
  • Possibly use polymorphic and polymorphic-tree
  • All organizational models would inherit "Location"
    • Region
    • Site
    • Rack Groups
    • Rack??
  • API Changish (we can point existing API's fields to new model and make them read_only)
  • Would close #4971 as well
  • Proposed Data Model: Netbox Diagram
@DanSheps commented on GitHub (Dec 22, 2020): Here is the idea so far: * Move to a base model, "Location"; location would be MPTT * Name & Slug would be moved to location; all other data would remain on the inheritees * Possibly use polymorphic and polymorphic-tree * All organizational models would inherit "Location" * Region * Site * Rack Groups * Rack?? * API Changish (we can point existing API's fields to new model and make them read_only) * Would close #4971 as well * Proposed Data Model: ![Netbox Diagram](https://user-images.githubusercontent.com/11049792/102920779-f09fa980-4450-11eb-9454-f59bc63af7b9.png)
adam commented 2025-12-29 18:34:13 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Dec 22, 2020):

I think we should evaluate less disruptive potential solutions before considering a major overhaul to these models. It's likely that much of this can be resolved without much effort.

Additionally, this is quite a bit to pack into one issue. IMO it should be broken into four separate concerns, as it's unlikely that all four cited behaviors will be addressed with a single change.

@jeremystretch commented on GitHub (Dec 22, 2020): I think we should evaluate less disruptive potential solutions before considering a major overhaul to these models. It's likely that much of this can be resolved without much effort. Additionally, this is quite a bit to pack into one issue. IMO it should be broken into four separate concerns, as it's unlikely that all four cited behaviors will be addressed with a single change.
adam commented 2025-12-29 18:34:13 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Dec 22, 2020):

Case 1: Device-Site not matching Rack-Site

I'm not able to reproduce this behavior on v2.10.2. After assigning the rack to the new site, the device's site is updated correctly, and a change record is created to reflect the change. (There is logic within Rack.save() which addresses this.)

@jeremystretch commented on GitHub (Dec 22, 2020): > Case 1: Device-Site not matching Rack-Site I'm not able to reproduce this behavior on v2.10.2. After assigning the rack to the new site, the device's site is updated correctly, and a change record is created to reflect the change. (There is logic within `Rack.save()` which addresses this.)
adam commented 2025-12-29 18:34:13 +01:00
Author
Owner
Copy Link

@candlerb commented on GitHub (Dec 22, 2020):

OK I probably got case 1 wrong then, so I have now struck it out. I'd checked all the clean logic but not save. I believe the rest are still valid.

@candlerb commented on GitHub (Dec 22, 2020): OK I probably got case 1 wrong then, so I have now struck it out. I'd checked all the `clean` logic but not `save`. I believe the rest are still valid.
adam commented 2025-12-29 18:34:13 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Dec 22, 2020):

@candlerb I think I've covered all of your cases in PR #5522. Please let me know what you think. Thanks for putting together such a detailed compilation!

@jeremystretch commented on GitHub (Dec 22, 2020): @candlerb I think I've covered all of your cases in PR #5522. Please let me know what you think. Thanks for putting together such a detailed compilation!
adam commented 2025-12-29 18:34:14 +01:00
Author
Owner
Copy Link

@candlerb commented on GitHub (Dec 23, 2020):

I've cherry-picked tested this onto 2.10.2 and done a brief test; it works.

The same issue remains for Power Panels - which are not associated with a rack, but are associated with a rackgroup and site. I can replicate as follows:

  1. Create sites A and B
  2. Create rackgroup G in site A
  3. Create power panel P in site A and rackgroup G
  4. Change rackgroup G to site B

Expected result: power panel P is now associated with site B (or update rejected)
Actual result: power panel P is still explicitly associated with site A

Depending on how that's fixed, I believe there's another problem.

Power feeds are associated with a PowerPanel and a nullable Rack (but not a site, since that's implied from the PowerPanel). The UI currently enforces that when you create a PowerFeed it must be in a rack in the same site as its parent PowerPanel.

However, if you edit a PowerPanel to change its Site (and RackGroup, if any), I think this could end up with its child PowerFeeds being in a Rack in a different Site. Since there's no way to choose the correct new Rack for the PowerFeeds, I think it would be necessary either to set all the child PowerFeeds Racks to None - or prevent the update in the first place.

A related problem occurs when you consider that a Rack may also be moved from one Site (and RackGroup) to another. You have:

      ,----------------------<
      |                   ,--< panel P1------.
      |                  +                   ^
site S1 ---< rackgroup G1                  feed F1
      |                  +                   v
      |                   `--< rack R1 +-----'
       `---------------------<

If you change rack R1 to a different rackgroup and site, then the feeds will be in a rack in the new site, while the panel is still in the old site.

However, whilst this is a violation of policy ("a power feed ought not to be in a rack in a different site than its power panel"), it's not an actual database inconsistency (such as the very first example in this post, which resulted in a power panel linked to site A and simultaneously to a rackgroup in site B)

Unrelated: I came across some other cases of def clean which don't have super().clean(). Should those be added?

@candlerb commented on GitHub (Dec 23, 2020): I've cherry-picked tested this onto 2.10.2 and done a brief test; it works. The same issue remains for Power Panels - which are not associated with a rack, but are associated with a rackgroup and site. I can replicate as follows: 1. Create sites A and B 2. Create rackgroup G in site A 3. Create power panel P in site A and rackgroup G 4. Change rackgroup G to site B Expected result: power panel P is now associated with site B (or update rejected) Actual result: power panel P is still explicitly associated with site A ----- Depending on how that's fixed, I believe there's another problem. Power *feeds* are associated with a PowerPanel and a nullable Rack (but not a site, since that's implied from the PowerPanel). The UI currently enforces that when you create a PowerFeed it must be in a rack in the same site as its parent PowerPanel. However, if you edit a PowerPanel to change its Site (and RackGroup, if any), I think this could end up with its child PowerFeeds being in a Rack in a different Site. Since there's no way to choose the correct new Rack for the PowerFeeds, I think it would be necessary either to set all the child PowerFeeds Racks to None - or prevent the update in the first place. A related problem occurs when you consider that a Rack may also be moved from one Site (and RackGroup) to another. You have: ``` ,----------------------< | ,--< panel P1------. | + ^ site S1 ---< rackgroup G1 feed F1 | + v | `--< rack R1 +-----' `---------------------< ``` If you change rack R1 to a different rackgroup and site, then the feeds will be in a rack in the new site, while the panel is still in the old site. However, whilst this is a violation of policy ("a power feed ought not to be in a rack in a different site than its power panel"), it's not an actual database inconsistency (such as the very first example in this post, which resulted in a power panel linked to site A and simultaneously to a rackgroup in site B) ----- Unrelated: I came across some other cases of `def clean` which don't have `super().clean()`. Should those be added?
adam commented 2025-12-29 18:34:14 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Dec 23, 2020):

The same issue remains for Power Panels

Okay, simple enough to include that model in the signal receiver as well.

Depending on how that's fixed, I believe there's another problem.

Could you copy your description above into a separate issue please? It's certainly related, but worthy of its own issue.

Unrelated: I came across some other cases of def clean which don't have super().clean(). Should those be added?

Sure. If you want to open a housekeeping issue calling out any specific locations I'll look into them.

@jeremystretch commented on GitHub (Dec 23, 2020): > The same issue remains for Power Panels Okay, simple enough to include that model in the signal receiver as well. > Depending on how that's fixed, I believe there's another problem. Could you copy your description above into a separate issue please? It's certainly related, but worthy of its own issue. > Unrelated: I came across some other cases of def clean which don't have super().clean(). Should those be added? Sure. If you want to open a housekeeping issue calling out any specific locations I'll look into them.
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#4247
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?