Merge pull request #85 from Onemind-Services-LLC/housekeeping/cleanup

Author: ryanmerolle

.devcontainer/docker-compose.override.yml

@@ -5,7 +5,7 @@ services:
       dockerfile: Dockerfile-plugin_dev
       context: .
     ports:
-      - 8000:8080
+      - "8000:8080"
     volumes:
       - ../:/opt/netbox/netbox/netbox-acls
       - ~/.gitconfig:/home/vscode/.gitconfig:z,ro

.devcontainer/initializers/cables.yml

@@ -3,7 +3,7 @@
 # ```
 #   termination_x_name -> name of interface
 #   termination_x_device -> name of the device interface belongs to
-#   termination_x_class -> required if different than 'Interface' which is the default
+#   termination_x_class -> required if different from 'Interface' which is the default
 # ```
 #
 # Supported termination classes: Interface, ConsolePort, ConsoleServerPort, FrontPort, RearPort, PowerPort, PowerOutlet

.gitignore

@@ -161,3 +161,5 @@ cython_debug/
 
 # VS Code
 .vscode/
+# JetBrains
+.idea/

CONTRIBUTING.md

@@ -3,9 +3,9 @@
 ## Reporting Bugs
 
 * First, ensure that you're running the [latest stable version](https://github.com/netbox-community/netbox/releases)
-of NetBox or this plugin [latest stable version](https://github.com/ryanmerolle/netbox-acls/releases).
+of NetBox or this plugin is at [latest stable version](https://github.com/ryanmerolle/netbox-acls/releases).
 If you're running an older version, it's possible that the bug has already been fixed
-or you are running a version of the plugin not tested with the NetBox version
+or, you are running a version of the plugin not tested with the NetBox version
 you are running [Compatibility Matrix](./README.md#compatibility).
 
 * Next, check the GitHub [issues list](https://github.com/ryanmerolle/netbox-acls/issues)

README.md

@@ -19,7 +19,7 @@ Based on the NetBox plugin tutorial by [jeremystretch](https://github.com/jeremy
 - [demo repository](https://github.com/netbox-community/netbox-plugin-demo)
 - [tutorial](https://github.com/netbox-community/netbox-plugin-tutorial)
 
-All credit should go to Jeremy.  Thanks Jeremy!
+All credit should go to Jeremy. Thanks, Jeremy!
 
 This project just looks to build on top of this framework and model presented.
 
@@ -69,12 +69,12 @@ PLUGINS_CONFIG = {
 To develop this plugin further one can use the included .devcontainer configuration. This configuration creates a docker container which includes a fully working netbox installation. Currently it should work when using WSL 2. For this to work make sure you have Docker Desktop installed and the WSL 2 integrations activated.
 
 1. In the WSL terminal, enter `code` to run Visual studio code.
-1. Install the devcontainer extension "ms-vscode-remote.remote-containers"
-1. Press Ctrl+Shift+P and use the "Dev Container: Clone Repository in Container Volume" function to clone this repository. This will take a while depending on your computer
-1. If you'd like the netbox instance to be prepopulated run `make Makefile example_initializers` and `make Makefile load_initializers`
-1. Start the netbox instance using `make Makefile all`
+2. Install the devcontainer extension "ms-vscode-remote.remote-containers"
+3. Press Ctrl+Shift+P and use the "Dev Container: Clone Repository in Container Volume" function to clone this repository. This will take a while depending on your computer
+4. If you'd like the netbox instance to be prepopulated run `make Makefile example_initializers` and `make Makefile load_initializers`
+5. Start the netbox instance using `make Makefile all`
 
-Your netbox instance will be served under 0.0.0.0:8000 so it should now be available under localhost:8000.
+Your netbox instance will be served under 0.0.0.0:8000, so it should now be available under localhost:8000.
 
 ## Screenshots
 

netbox_acls/api/serializers.py

@@ -9,6 +9,7 @@
 from ipam.api.serializers import NestedPrefixSerializer
 from netbox.api.fields import ContentTypeField
 from netbox.api.serializers import NetBoxModelSerializer
+from netbox.constants import NESTED_SERIALIZER_PREFIX
 from rest_framework import serializers
 from utilities.api import get_serializer_for_model
 
@@ -82,7 +83,10 @@ class Meta:
 
     @swagger_serializer_method(serializer_or_field=serializers.DictField)
     def get_assigned_object(self, obj):
-        serializer = get_serializer_for_model(obj.assigned_object, prefix="Nested")
+        serializer = get_serializer_for_model(
+            obj.assigned_object,
+            prefix=NESTED_SERIALIZER_PREFIX,
+        )
         context = {"request": self.context["request"]}
         return serializer(obj.assigned_object, context=context).data
 
@@ -96,6 +100,7 @@ def validate(self, data):
 
         # Check that the GFK object is valid.
         if "assigned_object_type" in data and "assigned_object_id" in data:
+            # TODO: This can removed after https://github.com/netbox-community/netbox/issues/10221 is fixed.
             try:
                 assigned_object = data[  # noqa: F841
                     "assigned_object_type"
@@ -161,21 +166,25 @@ class Meta:
 
     @swagger_serializer_method(serializer_or_field=serializers.DictField)
     def get_assigned_object(self, obj):
-        serializer = get_serializer_for_model(obj.assigned_object, prefix="Nested")
+        serializer = get_serializer_for_model(
+            obj.assigned_object,
+            prefix=NESTED_SERIALIZER_PREFIX,
+        )
         context = {"request": self.context["request"]}
         return serializer(obj.assigned_object, context=context).data
 
     def validate(self, data):
         """
-        Validate the AccessList django model model's inputs before allowing it to update the instance.
+        Validate the AccessList django model's inputs before allowing it to update the instance.
           - Check that the GFK object is valid.
           - Check that the associated interface's parent host has the selected ACL defined.
         """
         error_message = {}
         acl_host = data["access_list"].assigned_object
 
-        # Check that the GFK object is vlaid.
+        # Check that the GFK object is valid.
         if "assigned_object_type" in data and "assigned_object_id" in data:
+            # TODO: This can removed after https://github.com/netbox-community/netbox/issues/10221 is fixed.
             try:
                 assigned_object = data[  # noqa: F841
                     "assigned_object_type"
@@ -200,6 +209,8 @@ def validate(self, data):
                 .get_object_for_this_type(id=data["assigned_object_id"])
                 .virtual_machine
             )
+        else:
+            interface_host = None
         # Check that the associated interface's parent host has the selected ACL defined.
         if acl_host != interface_host:
             error_acl_not_assigned_to_host = (
@@ -253,7 +264,7 @@ class Meta:
 
     def validate(self, data):
         """
-        Validate the ACLStandardRule django model model's inputs before allowing it to update the instance:
+        Validate the ACLStandardRule django model's inputs before allowing it to update the instance:
           - Check if action set to remark, but no remark set.
           - Check if action set to remark, but source_prefix set.
         """
@@ -322,7 +333,7 @@ class Meta:
 
     def validate(self, data):
         """
-        Validate the ACLExtendedRule django model model's inputs before allowing it to update the instance:
+        Validate the ACLExtendedRule django model's inputs before allowing it to update the instance:
           - Check if action set to remark, but no remark set.
           - Check if action set to remark, but source_prefix set.
           - Check if action set to remark, but source_ports set.

netbox_acls/choices.py

@@ -51,9 +51,12 @@ class ACLAssignmentDirectionChoices(ChoiceSet):
     Defines the direction of the application of the ACL on an associated interface.
     """
 
+    DIRECTION_INGRESS = "ingress"
+    DIRECTION_EGRESS = "egress"
+
     CHOICES = [
-        ("ingress", "Ingress", "blue"),
-        ("egress", "Egress", "purple"),
+        (DIRECTION_INGRESS, "Ingress", "blue"),
+        (DIRECTION_EGRESS, "Egress", "purple"),
     ]
 
 
@@ -62,9 +65,12 @@ class ACLTypeChoices(ChoiceSet):
     Defines the choices availble for the Access Lists plugin specific to ACL type.
     """
 
+    TYPE_STANDARD = "standard"
+    TYPE_EXTENDED = "extended"
+
     CHOICES = [
-        ("extended", "Extended", "purple"),
-        ("standard", "Standard", "blue"),
+        (TYPE_EXTENDED, "Extended", "purple"),
+        (TYPE_STANDARD, "Standard", "blue"),
     ]
 
 
@@ -73,8 +79,12 @@ class ACLProtocolChoices(ChoiceSet):
     Defines the choices availble for the Access Lists plugin specific to ACL Rule protocol.
     """
 
+    PROTOCOL_ICMP = "icmp"
+    PROTOCOL_TCP = "tcp"
+    PROTOCOL_UDP = "udp"
+
     CHOICES = [
-        ("icmp", "ICMP", "purple"),
-        ("tcp", "TCP", "blue"),
-        ("udp", "UDP", "orange"),
+        (PROTOCOL_ICMP, "ICMP", "purple"),
+        (PROTOCOL_TCP, "TCP", "blue"),
+        (PROTOCOL_UDP, "UDP", "orange"),
     ]

netbox_acls/forms/models.py

@@ -22,6 +22,7 @@
     VMInterface,
 )
 
+from ..choices import ACLTypeChoices
 from ..models import (
     AccessList,
     ACLExtendedRule,
@@ -236,8 +237,12 @@ def clean(self):
                 "name": [error_same_acl_name],
             }
         # Check if Access List has no existing rules before change the Access List's type.
-        if (acl_type == "extended" and self.instance.aclstandardrules.exists()) or (
-            acl_type == "standard" and self.instance.aclextendedrules.exists()
+        if (
+            acl_type == ACLTypeChoices.TYPE_EXTENDED
+            and self.instance.aclstandardrules.exists()
+        ) or (
+            acl_type == ACLTypeChoices.TYPE_STANDARD
+            and self.instance.aclextendedrules.exists()
         ):
             error_message["type"] = [
                 "This ACL has ACL rules associated, CANNOT change ACL type.",
@@ -312,10 +317,6 @@ class ACLInterfaceAssignmentForm(NetBoxModelForm):
         ),
     )
     comments = CommentField()
-    tags = DynamicModelMultipleChoiceField(
-        queryset=Tag.objects.all(),
-        required=False,
-    )
 
     def __init__(self, *args, **kwargs):
 
@@ -355,7 +356,7 @@ def clean(self):
         """
         Validates form inputs before submitting:
           - Check if both interface and vminterface are set.
-          - Check if neither interface or vminterface are set.
+          - Check if neither interface nor vminterface are set.
           - Check that an interface's parent device/virtual_machine is assigned to the Access List.
           - Check that an interface's parent device/virtual_machine is assigned to the Access List.
           - Check for duplicate entry. (Because of GFK)
@@ -367,7 +368,6 @@ def clean(self):
         direction = cleaned_data.get("direction")
         interface = cleaned_data.get("interface")
         vminterface = cleaned_data.get("vminterface")
-        assigned_object = cleaned_data.get("assigned_object")
 
         # Check if both interface and vminterface are set.
         if interface and vminterface:
@@ -403,40 +403,42 @@ def clean(self):
             ).pk
             access_list_host = AccessList.objects.get(pk=access_list.pk).assigned_object
 
-        # Check that an interface's parent device/virtual_machine is assigned to the Access List.
-        if access_list_host != host:
-            error_acl_not_assigned_to_host = "Access List not present on selected host."
-            error_message |= {
-                "access_list": [error_acl_not_assigned_to_host],
-                assigned_object_type: [error_acl_not_assigned_to_host],
-                host_type: [error_acl_not_assigned_to_host],
-            }
-        # Check for duplicate entry.
-        if ACLInterfaceAssignment.objects.filter(
-            access_list=access_list,
-            assigned_object_id=assigned_object_id,
-            assigned_object_type=assigned_object_type_id,
-            direction=direction,
-        ).exists():
-            error_duplicate_entry = "An ACL with this name is already associated to this interface & direction."
-            error_message |= {
-                "access_list": [error_duplicate_entry],
-                "direction": [error_duplicate_entry],
-                assigned_object_type: [error_duplicate_entry],
-            }
-        # Check that the interface does not have an existing ACL applied in the direction already.
-        if ACLInterfaceAssignment.objects.filter(
-            assigned_object_id=assigned_object_id,
-            assigned_object_type=assigned_object_type_id,
-            direction=direction,
-        ).exists():
-            error_interface_already_assigned = (
-                "Interfaces can only have 1 Access List assigned in each direction."
-            )
-            error_message |= {
-                "direction": [error_interface_already_assigned],
-                assigned_object_type: [error_interface_already_assigned],
-            }
+            # Check that an interface's parent device/virtual_machine is assigned to the Access List.
+            if access_list_host != host:
+                error_acl_not_assigned_to_host = (
+                    "Access List not present on selected host."
+                )
+                error_message |= {
+                    "access_list": [error_acl_not_assigned_to_host],
+                    assigned_object_type: [error_acl_not_assigned_to_host],
+                    host_type: [error_acl_not_assigned_to_host],
+                }
+            # Check for duplicate entry.
+            if ACLInterfaceAssignment.objects.filter(
+                access_list=access_list,
+                assigned_object_id=assigned_object_id,
+                assigned_object_type=assigned_object_type_id,
+                direction=direction,
+            ).exists():
+                error_duplicate_entry = "An ACL with this name is already associated to this interface & direction."
+                error_message |= {
+                    "access_list": [error_duplicate_entry],
+                    "direction": [error_duplicate_entry],
+                    assigned_object_type: [error_duplicate_entry],
+                }
+            # Check that the interface does not have an existing ACL applied in the direction already.
+            if ACLInterfaceAssignment.objects.filter(
+                assigned_object_id=assigned_object_id,
+                assigned_object_type=assigned_object_type_id,
+                direction=direction,
+            ).exists():
+                error_interface_already_assigned = (
+                    "Interfaces can only have 1 Access List assigned in each direction."
+                )
+                error_message |= {
+                    "direction": [error_interface_already_assigned],
+                    assigned_object_type: [error_interface_already_assigned],
+                }
 
         if error_message:
             raise forms.ValidationError(error_message)
@@ -460,7 +462,7 @@ class ACLStandardRuleForm(NetBoxModelForm):
     access_list = DynamicModelChoiceField(
         queryset=AccessList.objects.all(),
         query_params={
-            "type": "standard",
+            "type": ACLTypeChoices.TYPE_STANDARD,
         },
         help_text=mark_safe(
             "<b>*Note:</b> This field will only display Standard ACLs.",
@@ -473,10 +475,6 @@ class ACLStandardRuleForm(NetBoxModelForm):
         help_text=help_text_acl_rule_logic,
         label="Source Prefix",
     )
-    tags = DynamicModelMultipleChoiceField(
-        queryset=Tag.objects.all(),
-        required=False,
-    )
 
     fieldsets = (
         ("Access List Details", ("access_list", "description", "tags")),
@@ -542,17 +540,14 @@ class ACLExtendedRuleForm(NetBoxModelForm):
     access_list = DynamicModelChoiceField(
         queryset=AccessList.objects.all(),
         query_params={
-            "type": "extended",
+            "type": ACLTypeChoices.TYPE_EXTENDED,
         },
         help_text=mark_safe(
             "<b>*Note:</b> This field will only display Extended ACLs.",
         ),
         label="Access List",
     )
-    tags = DynamicModelMultipleChoiceField(
-        queryset=Tag.objects.all(),
-        required=False,
-    )
+
     source_prefix = DynamicModelChoiceField(
         queryset=Prefix.objects.all(),
         required=False,

netbox_acls/models/access_list_rules.py

@@ -8,7 +8,7 @@
 from django.urls import reverse
 from netbox.models import NetBoxModel
 
-from ..choices import ACLProtocolChoices, ACLRuleActionChoices
+from ..choices import ACLProtocolChoices, ACLRuleActionChoices, ACLTypeChoices
 from .access_lists import AccessList
 
 __all__ = (
@@ -86,7 +86,7 @@ class ACLStandardRule(ACLRule):
         on_delete=models.CASCADE,
         to=AccessList,
         verbose_name="Standard Access List",
-        limit_choices_to={"type": "standard"},
+        limit_choices_to={"type": ACLTypeChoices.TYPE_STANDARD},
         related_name="aclstandardrules",
     )
 

netbox_acls/template_content.py

@@ -31,6 +31,9 @@ def right_page(self):
         elif ctype.model == "vminterface":
             parent_type = "virtual_machine"
             parent_id = obj.virtual_machine.pk
+        else:
+            parent_type = None
+            parent_id = None
 
         return self.render(
             "inc/assigned_interface/access_lists.html",