From 35662c2c48146cc1fdcce3024e76bbd97ea7aa1c Mon Sep 17 00:00:00 2001
From: "Zaiming (Stone) Shi" <zmstone@gmail.com>
Date: Tue, 20 Feb 2024 14:58:52 +0100
Subject: [PATCH] fix(schema): validate cluster strategy and node name

If cluster strategy is configured as `dns`, the node name
must be IP address
---
 apps/emqx_conf/src/emqx_conf_schema.erl       | 59 +++++++++++++++-
 .../emqx_conf/test/emqx_conf_schema_tests.erl | 69 +++++++++++++++++++
 changes/ce/fix-12541.en.md                    |  3 +
 3 files changed, 129 insertions(+), 2 deletions(-)
 create mode 100644 changes/ce/fix-12541.en.md

diff --git a/apps/emqx_conf/src/emqx_conf_schema.erl b/apps/emqx_conf/src/emqx_conf_schema.erl
index 914470ba47..5229abf102 100644
--- a/apps/emqx_conf/src/emqx_conf_schema.erl
+++ b/apps/emqx_conf/src/emqx_conf_schema.erl
@@ -42,6 +42,8 @@
 %% internal exports for `emqx_enterprise_schema' only.
 -export([ensure_unicode_path/2, convert_rotation/2, log_handler_common_confs/2]).
 
+-define(DEFAULT_NODE_NAME, <<"emqx@127.0.0.1">>).
+
 %% Static apps which merge their configs into the merged emqx.conf
 %% The list can not be made a dynamic read at run-time as it is used
 %% by nodetool to generate app.<time>.config before EMQX is started
@@ -123,7 +125,8 @@ roots() ->
         lists:flatmap(fun roots/1, common_apps()).
 
 validations() ->
-    hocon_schema:validations(emqx_schema) ++
+    [{check_node_name_and_discovery_strategy, fun validate_cluster_strategy/1}] ++
+        hocon_schema:validations(emqx_schema) ++
         lists:flatmap(fun hocon_schema:validations/1, common_apps()).
 
 common_apps() ->
@@ -359,7 +362,7 @@ fields("node") ->
             sc(
                 string(),
                 #{
-                    default => <<"emqx@127.0.0.1">>,
+                    default => ?DEFAULT_NODE_NAME,
                     'readOnly' => true,
                     importance => ?IMPORTANCE_HIGH,
                     desc => ?DESC(node_name)
@@ -1436,3 +1439,55 @@ ensure_unicode_path(Path, Opts) ->
 
 log_level() ->
     hoconsc:enum([debug, info, notice, warning, error, critical, alert, emergency, all]).
+
+validate_cluster_strategy(#{<<"node">> := _, <<"cluster">> := _} = Conf) ->
+    Name = hocon_maps:get("node.name", Conf),
+    [_Prefix, Host] = re:split(Name, "@", [{return, list}, unicode]),
+    Strategy = hocon_maps:get("cluster.discovery_strategy", Conf),
+    Type = hocon_maps:get("cluster.dns.record_type", Conf),
+    validate_dns_cluster_strategy(Strategy, Type, Host);
+validate_cluster_strategy(_) ->
+    true.
+
+validate_dns_cluster_strategy(dns, srv, _Host) ->
+    ok;
+validate_dns_cluster_strategy(dns, Type, Host) ->
+    case is_ip_addr(unicode:characters_to_list(Host), Type) of
+        true ->
+            ok;
+        false ->
+            throw(#{
+                explain =>
+                    "Node name must be of name@IP format "
+                    "for DNS cluster discovery strategy with " ++ atom_to_list(Type) ++
+                    "record type.",
+                domain => unicode:characters_to_list(Host)
+            })
+    end;
+validate_dns_cluster_strategy(_Other, _Type, _Name) ->
+    true.
+
+is_ip_addr(Host, Type) ->
+    case inet:parse_address(Host) of
+        {ok, Ip} ->
+            AddrType = address_type(Ip),
+            case
+                (AddrType =:= ipv4 andalso Type =:= a) orelse
+                    (AddrType =:= ipv6 andalso Type =:= aaaa)
+            of
+                true ->
+                    true;
+                false ->
+                    throw(#{
+                        explain => "Node name address " ++ atom_to_list(AddrType) ++
+                            " is incompatible with DNS record type " ++ atom_to_list(Type),
+                        record_type => Type,
+                        address_type => address_type(Ip)
+                    })
+            end;
+        _ ->
+            false
+    end.
+
+address_type(IP) when tuple_size(IP) =:= 4 -> ipv4;
+address_type(IP) when tuple_size(IP) =:= 8 -> ipv6.
diff --git a/apps/emqx_conf/test/emqx_conf_schema_tests.erl b/apps/emqx_conf/test/emqx_conf_schema_tests.erl
index 6aafd99fc7..977109caed 100644
--- a/apps/emqx_conf/test/emqx_conf_schema_tests.erl
+++ b/apps/emqx_conf/test/emqx_conf_schema_tests.erl
@@ -619,3 +619,72 @@ load_and_check_test_() ->
             end)
         end}
     ].
+
+%% erlfmt-ignore
+dns_record_conf(NodeName, DnsRecordType) ->
+    "
+             node {
+                name = \"" ++ NodeName ++ "\"
+                data_dir = \"data\"
+                cookie = cookie
+                max_ports = 2048
+                process_limit = 10240
+             }
+             cluster {
+                name = emqxcl
+                discovery_strategy = dns
+                dns.record_type = " ++ atom_to_list(DnsRecordType) ++"
+             }
+    ".
+
+a_record_with_non_ip_node_name_test_() ->
+    Test = fun(DnsRecordType) ->
+        {ok, ConfMap} = hocon:binary(dns_record_conf("emqx@local.host", DnsRecordType), #{
+            format => map
+        }),
+        ?assertThrow(
+            {emqx_conf_schema, [
+                #{
+                    reason := integrity_validation_failure,
+                    result := #{domain := "local.host"},
+                    kind := validation_error,
+                    validation_name := check_node_name_and_discovery_strategy
+                }
+            ]},
+            hocon_tconf:check_plain(emqx_conf_schema, ConfMap, #{required => false}, [node, cluster])
+        )
+    end,
+    [
+        {"a record", fun() -> Test(a) end},
+        {"aaaa record", fun() -> Test(aaaa) end}
+    ].
+
+dns_record_type_incompatiblie_with_node_host_ip_format_test_() ->
+    Test = fun(Ip, DnsRecordType) ->
+        {ok, ConfMap} = hocon:binary(dns_record_conf("emqx@" ++ Ip, DnsRecordType), #{format => map}),
+        ?assertThrow(
+            {emqx_conf_schema, [
+                #{
+                    reason := integrity_validation_failure,
+                    result := #{
+                        record_type := DnsRecordType,
+                        address_type := _
+                    },
+                    kind := validation_error,
+                    validation_name := check_node_name_and_discovery_strategy
+                }
+            ]},
+            hocon_tconf:check_plain(emqx_conf_schema, ConfMap, #{required => false}, [node, cluster])
+        )
+    end,
+    [
+        {"ipv4 address", fun() -> Test("::1", a) end},
+        {"ipv6 address", fun() -> Test("127.0.0.1", aaaa) end}
+    ].
+
+dns_srv_record_is_ok_test() ->
+    {ok, ConfMap} = hocon:binary(dns_record_conf("emqx@local.host", srv), #{format => map}),
+    ?assertMatch(
+        Value when is_map(Value),
+        hocon_tconf:check_plain(emqx_conf_schema, ConfMap, #{required => false}, [node, cluster])
+    ).
diff --git a/changes/ce/fix-12541.en.md b/changes/ce/fix-12541.en.md
new file mode 100644
index 0000000000..93749185ed
--- /dev/null
+++ b/changes/ce/fix-12541.en.md
@@ -0,0 +1,3 @@
+Added a config validation to check if `node.name` is compatible with `cluster.discover_strategy`.
+
+For `dns` strategy with `a` or `aaaa` record types, all nodes must use (static) IP address as host name.