Skip to content
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.

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

config: add nat helpers #543

Merged
merged 8 commits into from
May 21, 2024
Merged

config: add nat helpers #543

merged 8 commits into from
May 21, 2024

Conversation

filippocarletti
Copy link
Member

@filippocarletti filippocarletti commented May 20, 2024

Needed to allow active ftp sessions
Note: windows FTP client only uses active FTP

#544

Copy link
Member

@gsanchietti gsanchietti left a comment

Choose a reason for hiding this comment

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

I'm not sure this is the correct approach: we already have other helpers that must be explicitly loaded, see https://dev.nethsecurity.org/design/nat_helpers/

I'd prefer to keep the same behavior for all nat helpers.

@filippocarletti
Copy link
Member Author

I'd agree, but the previous version loaded the FTP helper by default for a reason. Upgrade's users will need to read the docs and enable this single helper.

@filippocarletti
Copy link
Member Author

You can tell that ftp is special because it has a package only for itself.

@gsanchietti
Copy link
Member

gsanchietti commented May 21, 2024

I'd agree, but the previous version loaded the FTP helper by default for a reason. Upgrade's users will need to read the docs and enable this single helper.

The previous release was loading all available helpers, including the ones that now are not loaded like sip-alg.
This change is already documented: https://docs.nethsecurity.org/en/latest/migration.html#migrated-configurations
So, in case of migration, users will already need to fix NAT helpers configuration.

I propose to change the behavior of all NAT helpers:

  • install them by default, remove the package since is almost impossible to restore it in case of upgrades
  • disable all helpers by default on new installations
  • enable all helpers by default on migration
  • change the documentation to reflect the new behavior

You can tell that ftp is special because it has a package only for itself.

I know that having the ftp module loaded by default should not harm, but I still prefer to have the same configuration for all NAT helpers.

Disable ftp NAT helper by default.
Make sure to save the list of loaded NAT helpers across
upgrades
Preserving packages across upgrades and restores is
not supported on OpenWrt
@gsanchietti
Copy link
Member

@filippocarletti could u take a look?

@cotosso
Copy link
Contributor

cotosso commented May 21, 2024

I'm afraid that not having the ftp helper enabled by default will cause a lot of support requests and give the wrong impression of a product that doesn't work properly.
Since it is still a widely used protocol I would prefer to make an exception for ftp.

New helper to load all configured kernel modules
and set their parameters.

If the script fails to set a parameter, it exists
with special code 99
@gsanchietti
Copy link
Member

I'm afraid that not having the ftp helper enabled by default will cause a lot of support requests and give the wrong impression of a product that doesn't work properly. Since it is still a widely used protocol I would prefer to make an exception for ftp.

In NethServer 7, having NAT helpers enabled by default caused many headache in the past.

Having the same behavior for all helpers is easier to maintain, explain and support.
When the NAT helpers will have their own page inside the UI, the behavior will be straightforward.

@cotosso
Copy link
Contributor

cotosso commented May 21, 2024

When the NAT helpers will have their own page inside the UI, the behavior will be straightforward.

In this case I agree with you @gsanchietti

@gsanchietti gsanchietti merged commit d42e52c into main May 21, 2024
1 check passed
@gsanchietti gsanchietti deleted the nathelper branch May 21, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants