Skip to content

AWS peer discovery: add multi-hostname path support #14705

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

AWS peer discovery: add multi-hostname path support #14705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions deps/rabbitmq_peer_discovery_aws/priv/schema/rabbitmq_peer_discovery_aws.schema
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,35 @@ fun(Conf) ->
Value -> rabbit_peer_discovery_util:as_list(Value)
end
end}.

%% hostname_paths (multiple paths support)

{mapping, "cluster_formation.aws.hostname_path.$number", "rabbit.cluster_formation.peer_discovery_aws.aws_hostname_paths", [
{datatype, string}
]}.

{translation, "rabbit.cluster_formation.peer_discovery_aws.aws_hostname_paths",
fun(Conf) ->
Settings = cuttlefish_variable:filter_by_prefix("cluster_formation.aws.hostname_path", Conf),
%% Helper function for number validation
IsNumberString = fun(Str) ->
try
_ = list_to_integer(Str),
true
catch
_:_ -> false
end
end,
Copy link
Collaborator

@SimonUnge SimonUnge Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to avoid try catch statements, i.e you could do:

case string:to_integer(Str) of
              {_, ""} -> true;
              _ -> false
          end

%% Extract and sort numbered paths (hostname_path.1, hostname_path.2, etc.)
NumberedPaths = [{list_to_integer(N), V} ||
{["cluster_formation", "aws", "hostname_path", N], V} <- Settings,
IsNumberString(N)],
case NumberedPaths of
[] -> cuttlefish:unset();
_ ->
SortedPaths = lists:sort(NumberedPaths),
[rabbit_peer_discovery_util:as_list(V) || {_, V} <- SortedPaths]
end
end}.


96 changes: 80 additions & 16 deletions deps/rabbitmq_peer_discovery_aws/src/rabbit_peer_discovery_aws.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@
env_variable = "AWS_HOSTNAME_PATH",
default_value = ["privateDnsName"]
},
aws_hostname_paths => #peer_discovery_config_entry_meta{
type = list,
env_variable = "AWS_HOSTNAME_PATHS",
default_value = []
},
aws_use_private_ip => #peer_discovery_config_entry_meta{
type = atom,
env_variable = "AWS_USE_PRIVATE_IP",
Expand Down Expand Up @@ -318,10 +323,11 @@ get_hostname_name_from_reservation_set([], Accum) -> Accum;
get_hostname_name_from_reservation_set([{"item", RI}|T], Accum) ->
InstancesSet = proplists:get_value("instancesSet", RI),
Items = [Item || {"item", Item} <- InstancesSet],
HostnamePath = get_hostname_path(),
Hostnames = [get_hostname(HostnamePath, Item) || Item <- Items],
Hostnames2 = [Name || Name <- Hostnames, Name =/= ""],
get_hostname_name_from_reservation_set(T, Accum ++ Hostnames2).
HostnamePaths = get_hostname_paths(),
?LOG_DEBUG("AWS peer discovery: processing reservation with ~p instances using hostname paths: ~tp",
[length(Items), HostnamePaths]),
UniqueHostnames = extract_unique_hostnames(HostnamePaths, Items),
get_hostname_name_from_reservation_set(T, Accum ++ UniqueHostnames).

get_hostname_names(Path) ->
case rabbitmq_aws:api_get_request("ec2", Path) of
Expand All @@ -347,23 +353,66 @@ get_hostname_by_tags(Tags) ->
Names
end.

-spec get_hostname_path() -> path().
get_hostname_path() ->
UsePrivateIP = get_config_key(aws_use_private_ip, ?CONFIG_MODULE:config_map(?BACKEND_CONFIG_KEY)),
HostnamePath = get_config_key(aws_hostname_path, ?CONFIG_MODULE:config_map(?BACKEND_CONFIG_KEY)),
FinalPath = case HostnamePath of
["privateDnsName"] when UsePrivateIP -> ["privateIpAddress"];
P -> P
-spec get_hostname_paths() -> [path()].
get_hostname_paths() ->
M = ?CONFIG_MODULE:config_map(?BACKEND_CONFIG_KEY),
UsePrivateIP = get_config_key(aws_use_private_ip, M),
RawPaths = case get_config_key(aws_hostname_paths, M) of
Paths when is_list(Paths), Paths =/= [] ->
?LOG_DEBUG("AWS peer discovery using multiple hostname paths"),
Paths;
_ ->
%% Use single path configuration (including when Paths is [])
SinglePath = get_single_hostname_path_raw(M),
?LOG_DEBUG("AWS peer discovery using single hostname path"),
[SinglePath]
end,
?LOG_DEBUG("AWS peer discovery using hostname path: ~tp", [FinalPath]),
FinalPath.
%% Apply use_private_ip override to all paths consistently
FinalPaths = apply_private_ip_override(RawPaths, UsePrivateIP),
?LOG_DEBUG("AWS peer discovery final hostname paths: ~tp", [FinalPaths]),
FinalPaths.

-spec get_single_hostname_path_raw(map()) -> path().
get_single_hostname_path_raw(ConfigMap) ->
case get_config_key(aws_hostname_path, ConfigMap) of
undefined ->
["privateDnsName"];
P ->
P
end.

-spec apply_private_ip_override([path()], boolean()) -> [path()].
apply_private_ip_override(Paths, UsePrivateIP) ->
apply_private_ip_override(Paths, UsePrivateIP, []).
apply_private_ip_override([], _, Acc) ->
lists:reverse(Acc);
apply_private_ip_override([["privateDnsName"] | Paths], true, Acc0) ->
Acc1 = [["privateIpAddress"] | Acc0],
apply_private_ip_override(Paths, true, Acc1);
apply_private_ip_override([Path | Paths], UsePrivateIP, Acc0) ->
Acc1 = [Path | Acc0],
apply_private_ip_override(Paths, UsePrivateIP, Acc1).

-spec get_hostname(path(), props()) -> string().
get_hostname([], _Props) ->
?LOG_DEBUG("AWS peer discovery: empty hostname path provided"),
""; %% Handle empty paths gracefully
get_hostname(Path, Props) ->
List = lists:foldl(fun get_value/2, Props, Path),
List = try
lists:foldl(fun get_value/2, Props, Path)
catch
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you think there would be a need for a try/catch here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.erlang.org/doc/apps/stdlib/io_lib.html#latin1_char_list/1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to guard against get_value/2, not latin1_char_list. I'll update the try to scope this correctly like this (sorry for the confusion):

-spec get_hostname(path(), props()) -> string().
get_hostname([], _Props) -> 
    "";  %% Handle empty paths gracefully
get_hostname(Path, Props) ->
    List = try
        lists:foldl(fun get_value/2, Props, Path)
    catch
        _:_ -> ""
    end,
    case io_lib:latin1_char_list(List) of
        true -> List;
        _ -> ""
    end.

The reason why a catch for get_value/2 is needed is I noticed that if an invalid path is provided (e.g. this one when a third network interface doesn't exist on the instance)

cluster_formation.aws.hostname_path.2 = networkInterfaceSet,3,privateIpAddressesSet,1,privateIpAddress

I see this startup failure:

[error] <0.208.0> BOOT FAILED
[error] <0.208.0> ===========
[error] <0.208.0> Exception during startup:
[error] <0.208.0>
[error] <0.208.0> error:function_clause
[error] <0.208.0>
[error] <0.208.0>     lists:nth_1/2, line 301
[error] <0.208.0>         args: [1,[]]
[error] <0.208.0>     rabbit_peer_discovery_aws:get_value/2, line 416
[error] <0.208.0>     lists:foldl_1/3, line 2151
[error] <0.208.0>     rabbit_peer_discovery_aws:get_hostname/2, line 406
[error] <0.208.0>     rabbit_peer_discovery_aws:-extract_unique_hostnames/2-lc$^1/1-1-/4, line 465
[error] <0.208.0>     rabbit_peer_discovery_aws:-extract_unique_hostnames/2-lc$^1/1-1-/4, line 465
[error] <0.208.0>     rabbit_peer_discovery_aws:extract_unique_hostnames/2, line 465
[error] <0.208.0>     rabbit_peer_discovery_aws:get_hostname_name_from_reservation_set/2, line 327

During hostname migration scenarios we may encounter invalid paths like this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just account for that case in get_value/2 if it is a possibility?

https://github.com/rabbitmq/rabbitmq-server/pull/14705/files#r2417353623

try/catch should be very, very rarely used.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try catches like that are a bit dangerous as you are catching 'everything' that could go wrong there, even cases where a crash might be preferred to catch a bug. Train yourself to think of ways to not use them, unless cases where you are forced to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, in this case I wonder if there's a better way that doesn't add significant complexity though 🤔

Error:Reason ->
?LOG_DEBUG("AWS peer discovery: hostname extraction failed for path ~tp, error: ~tp:~tp",
[Path, Error, Reason]),
""
end,
case io_lib:latin1_char_list(List) of
true -> List;
_ -> ""
true ->
?LOG_DEBUG("AWS peer discovery: extracted hostname '~ts' from path ~tp", [List, Path]),
List;
_ ->
?LOG_DEBUG("AWS peer discovery: invalid hostname format from path ~tp, result: ~tp", [Path, List]),
""
end.

-spec get_value(string()|integer(), props()) -> props().
Expand Down Expand Up @@ -413,3 +462,18 @@ get_tags() ->
maps:from_list(Value);
_ -> Tags
end.

%% Helper functions for multiple hostname paths support

-spec extract_unique_hostnames([path()], [props()]) -> [string()].
extract_unique_hostnames(Paths, Items) ->
?LOG_DEBUG("AWS peer discovery: extracting hostnames using ~p paths for ~p items",
[length(Paths), length(Items)]),
%% Extract all hostnames from all paths for all items
AllHostnames = [get_hostname(Path, Item) || Path <- Paths, Item <- Items],
%% Filter out empty hostnames and remove duplicates
ValidHostnames = [Name || Name <- AllHostnames, Name =/= ""],
UniqueHostnames = lists:uniq(ValidHostnames),
?LOG_DEBUG("AWS peer discovery: extracted ~p total hostnames, ~p valid, ~p unique: ~tp",
[length(AllHostnames), length(ValidHostnames), length(UniqueHostnames), UniqueHostnames]),
UniqueHostnames.
30 changes: 30 additions & 0 deletions ...tmq_peer_discovery_aws/test/config_schema_SUITE_data/rabbitmq_peer_discovery_aws.snippets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,5 +103,35 @@
]}
]}
], [rabbitmq_peer_discovery_aws]
},
{aws_hostname_paths_multiple,
"cluster_formation.aws.hostname_path.1 = networkInterfaceSet,2,privateIpAddressesSet,1,privateDnsName
cluster_formation.aws.hostname_path.2 = privateDnsName
cluster_formation.aws.hostname_path.3 = privateIpAddress",
[
{rabbit, [
{cluster_formation, [
{peer_discovery_aws, [
{aws_hostname_paths, [
["networkInterfaceSet", 2, "privateIpAddressesSet", 1, "privateDnsName"],
["privateDnsName"],
["privateIpAddress"]
]}
]}
]}
]}
], [rabbitmq_peer_discovery_aws]
},
{aws_hostname_paths_single_numbered,
"cluster_formation.aws.hostname_path.1 = privateDnsName",
[
{rabbit, [
{cluster_formation, [
{peer_discovery_aws, [
{aws_hostname_paths, [["privateDnsName"]]}
]}
]}
]}
], [rabbitmq_peer_discovery_aws]
}
].
Loading