diff --git a/lib/towerops/devices/device.ex b/lib/towerops/devices/device.ex index 0804389a..5271ba66 100644 --- a/lib/towerops/devices/device.ex +++ b/lib/towerops/devices/device.ex @@ -84,7 +84,8 @@ defmodule Towerops.Devices.Device do :snmp_community, :snmp_port ]) - |> validate_required([:name, :ip_address, :site_id]) + |> validate_required([:ip_address, :site_id]) + |> validate_name() |> validate_length(:name, min: 2, max: 200) |> validate_length(:description, max: 1000) |> validate_ip_address() @@ -93,6 +94,19 @@ defmodule Towerops.Devices.Device do |> foreign_key_constraint(:site_id) end + defp validate_name(changeset) do + snmp_enabled = get_field(changeset, :snmp_enabled, false) + name = get_field(changeset, :name) + + # Name is required only when SNMP is not enabled (ICMP only mode) + # When SNMP is enabled, name can be empty and will be populated from sysName during discovery + if !snmp_enabled and (is_nil(name) or name == "") do + add_error(changeset, :name, "can't be blank (required when SNMP is disabled)") + else + changeset + end + end + defp validate_ip_address(changeset) do case get_change(changeset, :ip_address) do nil -> diff --git a/lib/towerops/snmp/discovery.ex b/lib/towerops/snmp/discovery.ex index 7bf23d44..adcbd7c9 100644 --- a/lib/towerops/snmp/discovery.ex +++ b/lib/towerops/snmp/discovery.ex @@ -103,6 +103,7 @@ defmodule Towerops.Snmp.Discovery do with {:ok, _} <- Client.test_connection(client_opts), {:ok, system_info} <- discover_system(client_opts), + {:ok, device} <- update_device_name_from_snmp(device, system_info), profile = select_profile(system_info), {:ok, device_info} <- build_device_info(client_opts, system_info, profile), {:ok, interfaces} <- discover_interfaces(client_opts, profile), @@ -354,4 +355,25 @@ defmodule Towerops.Snmp.Discovery do Logger.info("Saved #{length(neighbors)} neighbors for device #{device_id}") :ok end + + @spec update_device_name_from_snmp(DeviceSchema.t(), system_info()) :: + {:ok, DeviceSchema.t()} | {:error, term()} + defp update_device_name_from_snmp(device, system_info) do + # Only update device name if it's currently empty and we got a sysName from SNMP + if (is_nil(device.name) or device.name == "") and Map.has_key?(system_info, :sys_name) do + sys_name = Map.get(system_info, :sys_name) + + if sys_name && sys_name != "" do + Logger.info("Updating device name from SNMP sysName: #{sys_name}") + + device + |> Ecto.Changeset.change(name: sys_name) + |> Repo.update() + else + {:ok, device} + end + else + {:ok, device} + end + end end diff --git a/lib/towerops_web/live/device_live/form.html.heex b/lib/towerops_web/live/device_live/form.html.heex index 1bf73274..36fe30d1 100644 --- a/lib/towerops_web/live/device_live/form.html.heex +++ b/lib/towerops_web/live/device_live/form.html.heex @@ -28,7 +28,17 @@
<.form for={@form} id="device-form" phx-change="validate" phx-submit="save"> - <.input field={@form[:name]} type="text" label="Device Name" required /> + <.input + field={@form[:name]} + type="text" + label="Device Name" + required={@monitoring_mode == "icmp_only"} + /> + <%= if @monitoring_mode == "snmp_and_icmp" do %> +

+ Leave blank to automatically populate from SNMP device name (sysName) during discovery. +

+ <% end %> <.input field={@form[:ip_address]} type="text" label="IP Address" required /> diff --git a/priv/repo/migrations/20260117222456_make_device_name_nullable.exs b/priv/repo/migrations/20260117222456_make_device_name_nullable.exs new file mode 100644 index 00000000..40162840 --- /dev/null +++ b/priv/repo/migrations/20260117222456_make_device_name_nullable.exs @@ -0,0 +1,9 @@ +defmodule Towerops.Repo.Migrations.MakeDeviceNameNullable do + use Ecto.Migration + + def change do + alter table(:devices) do + modify :name, :string, null: true + end + end +end diff --git a/test/towerops/snmp/discovery_test.exs b/test/towerops/snmp/discovery_test.exs index 83ddf65b..8a046e16 100644 --- a/test/towerops/snmp/discovery_test.exs +++ b/test/towerops/snmp/discovery_test.exs @@ -486,5 +486,137 @@ defmodule Towerops.Snmp.DiscoveryTest do # Verify only good device was created assert Repo.get_by(Device, device_id: device1.id) end + + test "updates device name from SNMP sysName when device name is empty", %{ + organization: _organization, + site: site + } do + # Create device without a name + {:ok, device} = + Towerops.Devices.create_device(%{ + name: nil, + ip_address: "192.168.1.200", + site_id: site.id, + snmp_enabled: true, + snmp_version: "2c", + snmp_community: "public", + snmp_port: 161 + }) + + # Mock successful connection test + expect(SnmpMock, :get, fn _, "1.3.6.1.2.1.1.3.0", _ -> + {:ok, {:timeticks, 12_345}} + end) + + # Mock system info discovery with sysName + UCD sensor OIDs + # 6 system info OIDs + 3 load OIDs + 2 memory OIDs = 11 total + expect(SnmpMock, :get, 11, fn _, oid, _ -> + case oid do + "1.3.6.1.2.1.1.1.0" -> + {:ok, {:octet_string, "Linux switch01 4.4.0"}} + + "1.3.6.1.2.1.1.2.0" -> + {:ok, {:object_identifier, [1, 3, 6, 1, 4, 1, 8072, 3, 2, 10]}} + + "1.3.6.1.2.1.1.3.0" -> + {:ok, {:timeticks, 12_345}} + + "1.3.6.1.2.1.1.4.0" -> + {:ok, {:octet_string, "admin@example.com"}} + + "1.3.6.1.2.1.1.5.0" -> + {:ok, {:octet_string, "discovered-switch-name"}} + + "1.3.6.1.2.1.1.6.0" -> + {:ok, {:octet_string, "Server Room"}} + + _ -> + {:error, :no_such_object} + end + end) + + # Mock interface and sensor discovery - no interfaces/sensors for simplicity + # Walk called 10 times: + # - LM sensors: temp devices (1), temp values (1), fan devices (1), fan values (1), + # voltage devices (1), voltage values (1) + # - Base sensors fallback (1) + # - Interfaces (1) + # - Neighbors: LLDP (1), CDP (1) + expect(SnmpMock, :walk, 10, fn _, _, _ -> + {:ok, []} + end) + + assert {:ok, _snmp_device} = Discovery.discover_device(device) + + # Verify the device name was updated with sysName + updated_device = Towerops.Devices.get_device!(device.id) + assert updated_device.name == "discovered-switch-name" + end + + test "does not update device name from SNMP when device already has a name", %{ + organization: _organization, + site: site + } do + # Create device with a name + {:ok, device} = + Towerops.Devices.create_device(%{ + name: "My Custom Name", + ip_address: "192.168.1.201", + site_id: site.id, + snmp_enabled: true, + snmp_version: "2c", + snmp_community: "public", + snmp_port: 161 + }) + + # Mock successful connection test + expect(SnmpMock, :get, fn _, "1.3.6.1.2.1.1.3.0", _ -> + {:ok, {:timeticks, 12_345}} + end) + + # Mock system info discovery with different sysName + UCD sensor OIDs + # 6 system info OIDs + 3 load OIDs + 2 memory OIDs = 11 total + expect(SnmpMock, :get, 11, fn _, oid, _ -> + case oid do + "1.3.6.1.2.1.1.1.0" -> + {:ok, {:octet_string, "Linux switch02 4.4.0"}} + + "1.3.6.1.2.1.1.2.0" -> + {:ok, {:object_identifier, [1, 3, 6, 1, 4, 1, 8072, 3, 2, 10]}} + + "1.3.6.1.2.1.1.3.0" -> + {:ok, {:timeticks, 12_345}} + + "1.3.6.1.2.1.1.4.0" -> + {:ok, {:octet_string, "admin@example.com"}} + + "1.3.6.1.2.1.1.5.0" -> + {:ok, {:octet_string, "different-switch-name"}} + + "1.3.6.1.2.1.1.6.0" -> + {:ok, {:octet_string, "Server Room"}} + + _ -> + {:error, :no_such_object} + end + end) + + # Mock interface and sensor discovery + # Walk called 10 times: + # - LM sensors: temp devices (1), temp values (1), fan devices (1), fan values (1), + # voltage devices (1), voltage values (1) + # - Base sensors fallback (1) + # - Interfaces (1) + # - Neighbors: LLDP (1), CDP (1) + expect(SnmpMock, :walk, 10, fn _, _, _ -> + {:ok, []} + end) + + assert {:ok, _snmp_device} = Discovery.discover_device(device) + + # Verify the device name was NOT changed + updated_device = Towerops.Devices.get_device!(device.id) + assert updated_device.name == "My Custom Name" + end end end diff --git a/test/towerops_web/live/device_live_test.exs b/test/towerops_web/live/device_live_test.exs index 0fce338f..222e4ec1 100644 --- a/test/towerops_web/live/device_live_test.exs +++ b/test/towerops_web/live/device_live_test.exs @@ -347,6 +347,75 @@ defmodule ToweropsWeb.DeviceLiveTest do assert path == ~p"/users/log-in" end + test "allows creating device without name when SNMP enabled", %{ + conn: conn, + organization: organization, + site: site + } do + {:ok, view, _html} = live(conn, ~p"/orgs/#{organization.slug}/devices/new") + + html = + view + |> form("#device-form", + device: %{ + name: "", + ip_address: "192.168.1.150", + site_id: site.id, + snmp_community: "public" + } + ) + |> render_submit() + + # Should succeed without validation error + assert html =~ "Device created successfully" + refute html =~ "can't be blank" + + # Verify device was created with empty name + device_list = Towerops.Devices.list_organization_devices(organization.id) + new_device = Enum.find(device_list, &(&1.ip_address == "192.168.1.150")) + assert new_device + assert new_device.name == nil or new_device.name == "" + end + + test "shows helper text about SNMP name population when name is empty", %{ + conn: conn, + organization: organization + } do + {:ok, _view, html} = live(conn, ~p"/orgs/#{organization.slug}/devices/new") + + # Should show helper text about name being populated from SNMP + assert html =~ "Leave blank to use SNMP device name" or + html =~ "populated from SNMP" or + html =~ "sysName" + end + + test "requires name when ICMP Only mode (no SNMP)", %{ + conn: conn, + organization: organization, + site: site + } do + {:ok, view, _html} = live(conn, ~p"/orgs/#{organization.slug}/devices/new") + + # Switch to ICMP Only mode + view + |> element("button", "ICMP Only") + |> render_click() + + html = + view + |> form("#device-form", + device: %{ + name: "", + ip_address: "192.168.1.151", + site_id: site.id + } + ) + |> render_submit() + + # Should show validation error for empty name in ICMP Only mode + assert html =~ "can't be blank" or html =~ "required" + end + test "defaults to SNMP & ICMP monitoring mode", %{conn: conn, organization: organization} do {:ok, _view, html} = live(conn, ~p"/orgs/#{organization.slug}/devices/new")