Resolves IP Lookups #270

[apoorv-2204]

Description

We must check the IP retrieved for given node, (in production) belongs to Public IP. It should not be static or NAT IP.

• We should validate IP is_public? only in prod mode and not in dev mode.
• If in dev mode , if we get Static/ NAT IP we should fall back to IPIFY to get Public IP
  Fixes # (IPLookup is unable to detect and avoid forbidden IP ranges #270 )

Type of change

Please delete options that are not relevant.

• Bug fix (Fix static ip in archethic Snap)
• New feature (In dev single node || dev docker mode we should not fallback to public Ip)

How Has This Been Tested?

• Add unit Tests
• Run node in mix prod , it must use nat and then fallback to Ipify

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[roychowdhuryrohit-dev]

LGTM so far well done. Correct log message outputting as expected for private ip NAT.

2022-05-11 16:13:10.976 [warning] Cannot use NAT IP lookup - "NAT: Private IP "
2022-05-11 16:13:10.976 [info] Trying IPFY as fallback
2022-05-11 16:13:11.648 [info] Node IP discovered: HIDDEN
2022-05-11 16:13:11.649 [info] Not previous synchronization date
2022-05-11 16:13:11.649 [info] We are using the default one
2022-05-11 16:13:11.649 [info] Node bootstrapping...

[ghost]

Function ending with ? should return a boolean , it's the convention in Elixir codebases.
So your function should check the validity of the public ip it's all. The fallback should be part of the rest of caller function

[apoorv-2204]

Function ending with ? should return a boolean , it's the convention in Elixir codebases. So your function should check the validity of the public ip it's all. The fallback should be part of the rest of caller function

My bad . on it,

[ghost]

The point is not to rename it but simplify it a bit.
You just have to check the validity of the ip otherwise call the existing fallback method with something like this:

with {:ok, ip} <- apply(provider, :get_node_ip, []),
        true <- Networking.valid_ip?(ip) do
    Logger.info("Node IP discovered: #{:inet.ntoa(ip)} by #{provider}")
    ip
else
  false ->
    fallback(provider, "NAT: Private IP")
  {:error, reason} ->
    fallback(provider, reason)
end

[apoorv-2204]

The point is not to rename it but simplify it a bit. You just have to check the validity of the ip otherwise call the existing fallback method with something like this:

with {:ok, ip} <- apply(provider, :get_node_ip, []),
        true <- Networking.valid_ip?(ip) do
    Logger.info("Node IP discovered: #{:inet.ntoa(ip)} by #{provider}")
    ip
else
  false ->
    fallback(provider, "NAT: Private IP")
  {:error, reason} ->
    fallback(provider, reason)
end

I agree , my bad

[ghost]

Can you update the PR description from the template described ?

Also it would be great to add some unit tests

[ghost]

There are still improvements to be done, and you should avoid removing stuff already present, as it can lead to regression

[apoorv-2204]

There are still improvements to be done, and you should avoid removing stuff already present, as it can lead to regression

Sure, I will do the required changes.
thx

[apoorv-2204]

MIX_ENV=prod iex -S mix

[apoorv-2204]

As per req: node in :prod crashes when given ARCHETHIC_NETWORKING_IMPL=STATIC

[ghost]

As per req: node in :prod crashes when given ARCHETHIC_NETWORKING_IMPL=STATIC

For static, we should set env var ARCHETHIC_STATIC_IP

[apoorv-2204]

As per req: node in :prod crashes when given ARCHETHIC_NETWORKING_IMPL=STATIC

For static, we should set env var ARCHETHIC_STATIC_IP

There is no fallback for static, when ran in with NAT, gets public ip.
And it is accessible via 127.0.0.1

[ghost]

otherwise it seems good 👍