Add ability to handle non-existent & malformed Checks supplied to cat…

…alog (#103)

lib/wanda/catalog.ex

@@ -11,6 +11,8 @@ defmodule Wanda.Catalog do
     Value
   }
 
+  require Logger
+
   @default_severity :critical
 
   @doc """
@@ -22,26 +24,43 @@ defmodule Wanda.Catalog do
     |> Path.join("/*")
     |> Path.wildcard()
     |> Enum.map(&Path.basename(&1, ".yaml"))
-    |> Enum.map(&get_check(&1))
+    |> get_checks()
   end
 
   @doc """
   Get a check from the catalog.
   """
-  @spec get_check(String.t()) :: Check.t()
+  @spec get_check(String.t()) :: {:ok, Check.t()} | {:error, any}
   def get_check(check_id) do
-    get_catalog_path()
-    |> Path.join("#{check_id}.yaml")
-    |> YamlElixir.read_from_file!()
-    |> map_check()
+    with path <- Path.join(get_catalog_path(), "#{check_id}.yaml"),
+         {:ok, file_content} <- YamlElixir.read_from_file(path),
+         {:ok, check} <- map_check(file_content) do
+      {:ok, check}
+    else
+      {:error, :malformed_check} = error ->
+        Logger.error(
+          "Check with ID #{check_id} is malformed. Check if all the required fields are present."
+        )
+
+        error
+
+      {:error, reason} = error ->
+        Logger.error("Error getting Check with ID #{check_id}: #{inspect(reason)}")
+        error
+    end
   end
 
   @doc """
   Get specific checks from the catalog.
   """
   @spec get_checks([String.t()]) :: [Check.t()]
   def get_checks(checks_id) do
-    Enum.map(checks_id, &get_check/1)
+    Enum.flat_map(checks_id, fn check_id ->
+      case get_check(check_id) do
+        {:ok, check} -> [check]
+        {:error, _} -> []
+      end
+    end)
   end
 
   defp get_catalog_path do
@@ -59,19 +78,22 @@ defmodule Wanda.Catalog do
            "expectations" => expectations
          } = check
        ) do
-    %Check{
-      id: id,
-      name: name,
-      group: group,
-      description: description,
-      remediation: remediation,
-      severity: map_severity(check),
-      facts: Enum.map(facts, &map_fact/1),
-      values: map_values(check),
-      expectations: Enum.map(expectations, &map_expectation/1)
-    }
+    {:ok,
+     %Check{
+       id: id,
+       name: name,
+       group: group,
+       description: description,
+       remediation: remediation,
+       severity: map_severity(check),
+       facts: Enum.map(facts, &map_fact/1),
+       values: map_values(check),
+       expectations: Enum.map(expectations, &map_expectation/1)
+     }}
   end
 
+  defp map_check(_), do: {:error, :malformed_check}
+
   defp map_severity(%{"severity" => "critical"}), do: :critical
   defp map_severity(%{"severity" => "warning"}), do: :warning
   defp map_severity(_), do: @default_severity

lib/wanda/executions/server.ex

@@ -34,6 +34,13 @@ defmodule Wanda.Executions.Server do
       |> Target.get_checks_from_targets()
       |> Catalog.get_checks()
 
+    checks_ids = Enum.map(checks, & &1.id)
+
+    targets =
+      Enum.map(targets, fn %{checks: target_checks} = target ->
+        %Target{target | checks: Enum.filter(target_checks, fn check -> check in checks_ids end)}
+      end)
+
     maybe_start_execution(execution_id, group_id, targets, checks, env, config)
   end
 

test/catalog_test.exs

@@ -15,17 +15,18 @@ defmodule Wanda.CatalogTest do
     test "should return the whole catalog" do
       catalog_path = Application.fetch_env!(:wanda, Wanda.Catalog)[:catalog_path]
 
-      files =
+      valid_files =
         catalog_path
         |> File.ls!()
         |> Enum.sort()
+        |> Enum.filter(fn file -> file != "malformed_check.yaml" end)
 
       catalog = Catalog.get_catalog()
-      assert length(files) == length(catalog)
+      assert length(valid_files) == length(catalog)
 
       Enum.with_index(catalog, fn check, index ->
         file_name =
-          files
+          valid_files
           |> Enum.at(index)
           |> Path.basename(".yaml")
 
@@ -34,77 +35,87 @@ defmodule Wanda.CatalogTest do
     end
 
     test "should load a check from a yaml file properly" do
-      assert %Check{
-               id: "expect_check",
-               name: "Test check",
-               group: "Test",
-               description: "Just a check\n",
-               remediation: "## Remediation\nRemediation text\n",
-               severity: :critical,
-               facts: [
-                 %Fact{
-                   name: "jedi",
-                   gatherer: "wandalorian",
-                   argument: "-o"
-                 },
-                 %Fact{
-                   name: "other_fact",
-                   gatherer: "no_args_gatherer",
-                   argument: ""
-                 }
-               ],
-               values: [
-                 %Value{
-                   conditions: [
-                     %Condition{expression: "some_expression", value: 10},
-                     %Condition{expression: "some_other_expression", value: 15}
-                   ],
-                   default: 5,
-                   name: "expected_value"
-                 },
-                 %Value{
-                   conditions: [
-                     %Condition{expression: "some_third_expression", value: 5}
-                   ],
-                   default: 10,
-                   name: "expected_higher_value"
-                 }
-               ],
-               expectations: [
-                 %Expectation{
-                   name: "some_expectation",
-                   type: :expect,
-                   expression: "facts.jedi == values.expected_value"
-                 },
-                 %Expectation{
-                   name: "some_other_expectation",
-                   type: :expect,
-                   expression: "facts.jedi > values.expected_higher_value"
-                 }
-               ]
-             } = Catalog.get_check("expect_check")
+      assert {:ok,
+              %Check{
+                id: "expect_check",
+                name: "Test check",
+                group: "Test",
+                description: "Just a check\n",
+                remediation: "## Remediation\nRemediation text\n",
+                severity: :critical,
+                facts: [
+                  %Fact{
+                    name: "jedi",
+                    gatherer: "wandalorian",
+                    argument: "-o"
+                  },
+                  %Fact{
+                    name: "other_fact",
+                    gatherer: "no_args_gatherer",
+                    argument: ""
+                  }
+                ],
+                values: [
+                  %Value{
+                    conditions: [
+                      %Condition{expression: "some_expression", value: 10},
+                      %Condition{expression: "some_other_expression", value: 15}
+                    ],
+                    default: 5,
+                    name: "expected_value"
+                  },
+                  %Value{
+                    conditions: [
+                      %Condition{expression: "some_third_expression", value: 5}
+                    ],
+                    default: 10,
+                    name: "expected_higher_value"
+                  }
+                ],
+                expectations: [
+                  %Expectation{
+                    name: "some_expectation",
+                    type: :expect,
+                    expression: "facts.jedi == values.expected_value"
+                  },
+                  %Expectation{
+                    name: "some_other_expectation",
+                    type: :expect,
+                    expression: "facts.jedi > values.expected_higher_value"
+                  }
+                ]
+              }} = Catalog.get_check("expect_check")
     end
 
     test "should load a expect_same expectation type" do
-      assert %Check{
-               values: [],
-               expectations: [
-                 %Expectation{
-                   name: "some_expectation",
-                   type: :expect_same,
-                   expression: "facts.jedi"
-                 }
-               ]
-             } = Catalog.get_check("expect_same_check")
+      assert {:ok,
+              %Check{
+                values: [],
+                expectations: [
+                  %Expectation{
+                    name: "some_expectation",
+                    type: :expect_same,
+                    expression: "facts.jedi"
+                  }
+                ]
+              }} = Catalog.get_check("expect_same_check")
     end
 
     test "should load a warning severity" do
-      assert %Check{severity: :warning} = Catalog.get_check("warning_severity_check")
+      assert {:ok, %Check{severity: :warning}} = Catalog.get_check("warning_severity_check")
+    end
+
+    test "should return an error for non-existent check" do
+      assert {:error, _} = Catalog.get_check("non_existent_check")
+    end
+
+    test "should return an error for malformed check" do
+      assert {:error, :malformed_check} = Catalog.get_check("malformed_check")
     end
 
     test "should load multiple checks" do
       assert [%Check{id: "expect_check"}, %Check{id: "expect_same_check"}] =
-               Catalog.get_checks(["expect_check", "expect_same_check"])
+               Catalog.get_checks(["expect_check", "non_existent_check", "expect_same_check"])
     end
   end
 end

test/executions/server_test.exs

@@ -89,9 +89,7 @@ defmodule Wanda.Executions.ServerTest do
       group_id = UUID.uuid4()
       targets = build_list(2, :target, checks: ["expect_check"])
 
-      expect(Wanda.Messaging.Adapters.Mock, :publish, 0, fn _, _ ->
-        :ok
-      end)
+      expect(Wanda.Messaging.Adapters.Mock, :publish, 0, fn _, _ -> :ok end)
 
       start_supervised!(
         {Server,
@@ -262,5 +260,27 @@ defmodule Wanda.Executions.ServerTest do
       assert {:error, :no_checks_selected} =
                Server.start_execution(UUID.uuid4(), UUID.uuid4(), targets, %{})
     end
+
+    test "should execute existing checks if non-existent checks are selected" do
+      group_id = UUID.uuid4()
+
+      [%{agent_id: agent_1}, %{agent_id: agent_2}] =
+        targets = [
+          build(:target, checks: ["expect_check", "non_existing"]),
+          build(:target, checks: ["expect_same_check", "non_existing"])
+        ]
+
+      assert :ok = Server.start_execution(UUID.uuid4(), group_id, targets, %{})
+
+      pid = :global.whereis_name({Server, group_id})
+      %{targets: actual_targets} = :sys.get_state(pid)
+
+      expected_targets = [
+        build(:target, agent_id: agent_1, checks: ["expect_check"]),
+        build(:target, agent_id: agent_2, checks: ["expect_same_check"])
+      ]
+
+      assert actual_targets == expected_targets
+    end
   end
 end

test/fixtures/catalog/malformed_check.yaml

@@ -0,0 +1,11 @@
+id: malformed_check
+name: Malformed check
+group: Test
+description: |
+  A malformed check
+remediation: |
+  ## Remediation
+  Remediation text
+expectations:
+  - name: some_expectation
+    expect_same: facts.jedi