[BOUNTY-2] Explicit Configuration & General internal NAT API

[matkt]

Signed-off-by: Karim TAAM karim.t2am@gmail.com

PR description

Create a general internal API to support Network Address Translation (NAT) technologies in Besu. The goal of this work is to offer developers a consistent API for performing NAT tasks so that I can abstractly add support for new NAT systems (e.g. Docker, Kubernetes, etc.) with a minimum amount of code changes. As a second NAT style, explicit configuration for all ports and IP addresses would be available.

A few comments

Java APIs to determine if a node is in a NAT environment, and then identify the kind of NAT environment

The method is passed using the --nat-method parameter. If the method is not passed in argument the client will try to detect it automatically. Automatic detection is being developed.
it is possible to call isNATEnvironment to get the result

Java APIs to enable/disable the NAT port forwarding if the environment requires it, as well as APIs to determine if such calls are needed or making the enable/disable calls a no-op when it is unneeded.

As requested, the generic API does not allow to enable or disable the port forwading. requestPortForward and releasePortForward are only available in the UPNP implementation

Ability to explicitly configure the external IP to broadcast without regards to NAT or other considerations.

A new command-line parameter --nat-external-ip-usage-enabled has been added to allow the user to force the usage of the external ip addresss, otherwise the p2pAdvertisedHost is used. The default value is true

[shemnon]

It's hard for me to provide quality feedback because this PR is too large, 1,600 changed lines. I can't wrap my head around it in anything less than an hour. The few comments I left were before I realized that was why I was having a hard time thinking deeply about it. This needs to be split up into several smaller PRs. Here's some ideas on how

• A PR that just refactors the existing UPNP/None options into the NatManager
• A PR that renames and moves files with no other changes to the desired places and names.
• A PR to add network reporting files (besu.network)
• A PR to add reporting in the Admin Info
• A PR that adds manual NAT settings into the newly refactored NatManager
• A PR that adds docker support into the newly refactored NatManager
• A PR that adds K8s support into the newly refactoed NatManager.

It looks like docker, k8s, and manual are not in this PR, but there are some config options set up for them. Don't add these until the code adding them is brought in, it makes the PR look like it's doing more than it intends.

Also, minimize moving methods around if possible, it increases the diff count and makes it difficult to see if any changes were done in those methods. At least 200 of the diff lines are just moving within classes.

The refactoring may make for a somewhat large PR, but try and minimize what else is going on to keep it as slim as possible.

[shemnon]

This should be implicit by --nat-method=MANUAL.

[shemnon]

We also need a MANUAL value where the client just has the values plugged via configuration.

[shemnon]

Generally we treat initialism as words, so UpperCamelCase would be -> NatMethod

https://google.github.io/styleguide/javaguide.html#s5.3-camel-case

[matkt]

It's hard for me to provide quality feedback because this PR is too large, 1,600 changed lines. I can't wrap my head around it in anything less than an hour. The few comments I left were before I realized that was why I was having a hard time thinking deeply about it. This needs to be split up into several smaller PRs. Here's some ideas on how

• A PR that just refactors the existing UPNP/None options into the NatManager
• A PR that renames and moves files with no other changes to the desired places and names.
• A PR to add network reporting files (besu.network)
• A PR to add reporting in the Admin Info
• A PR that adds manual NAT settings into the newly refactored NatManager
• A PR that adds docker support into the newly refactored NatManager
• A PR that adds K8s support into the newly refactoed NatManager.

It looks like docker, k8s, and manual are not in this PR, but there are some config options set up for them. Don't add these until the code adding them is brought in, it makes the PR look like it's doing more than it intends.

Also, minimize moving methods around if possible, it increases the diff count and makes it difficult to see if any changes were done in those methods. At least 200 of the diff lines are just moving within classes.

The refactoring may make for a somewhat large PR, but try and minimize what else is going on to keep it as slim as possible.

OK thanks for your answer. Indeed there is a lot of change in this PR. I start splitting my PR and I get back to you quickly with a smaller PR

[matkt]

It's hard for me to provide quality feedback because this PR is too large, 1,600 changed lines. I can't wrap my head around it in anything less than an hour. The few comments I left were before I realized that was why I was having a hard time thinking deeply about it. This needs to be split up into several smaller PRs. Here's some ideas on how

• A PR that just refactors the existing UPNP/None options into the NatManager
• A PR that renames and moves files with no other changes to the desired places and names.
• A PR to add network reporting files (besu.network)
• A PR to add reporting in the Admin Info
• A PR that adds manual NAT settings into the newly refactored NatManager
• A PR that adds docker support into the newly refactored NatManager
• A PR that adds K8s support into the newly refactoed NatManager.

It looks like docker, k8s, and manual are not in this PR, but there are some config options set up for them. Don't add these until the code adding them is brought in, it makes the PR look like it's doing more than it intends.

Also, minimize moving methods around if possible, it increases the diff count and makes it difficult to see if any changes were done in those methods. At least 200 of the diff lines are just moving within classes.

The refactoring may make for a somewhat large PR, but try and minimize what else is going on to keep it as slim as possible.

Hi @shemnon

I just published a smaller PR in my private repo in order to simplify the review. I have also detailed as much as possible my modifications in the description. Hoping that it facilitates the review. it will make a merge to a branch that I would push to the master of the official repository at the end of the bounty

Link to the PR : matkt#3