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

T160: nat64: Implement Jool-based NAT64 translator #1993

Closed
wants to merge 1 commit into from

Conversation

frebib
Copy link
Contributor

@frebib frebib commented May 10, 2023

Change Summary

Implement NAT64 using jool following existing nat44 syntax

# https://nicmx.github.io/Jool/en/config-atomic.html#nat64 minimal example
set nat64 source rule 10 description "blah blah"
set nat64 source rule 10 source prefix '64:ffb9::/96'
# TODO global options are not implemented yet
set nat64 source rule 10 option lowest-mtu 1500
# https://nicmx.github.io/Jool/en/usr-flags-pool4.html#options
set nat64 source rule 10 translation pool 1 address '192.168.0.0/24'
set nat64 source rule 10 translation pool 1 port '1-65535'
set nat64 source rule 10 translation pool 1 protocol tcp
set nat64 source rule 10 translation pool 1 protocol udp
set nat64 source rule 10 translation pool 1 protocol icmp
# bib (4->6 DNAT mapping)
# TODO decide config structure

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

Component(s) name

nat64

Proposed changes

How to test

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@vyosbot vyosbot requested review from a team, dmbaturin, sarthurdev, zdc, jestabro, sever-sever and c-po and removed request for a team May 10, 2023 18:41
interface-definitions/nat64.xml.in Outdated Show resolved Hide resolved
interface-definitions/nat64.xml.in Outdated Show resolved Hide resolved
interface-definitions/nat64.xml.in Outdated Show resolved Hide resolved
src/conf_mode/nat64.py Outdated Show resolved Hide resolved
@frebib frebib force-pushed the nat64 branch 2 times, most recently from 884d81e to 1fa6068 Compare May 13, 2023 15:06
@frebib frebib changed the title [WIP] nat64: T160: Implement Jool-based NAT64 translator [nat64: T160: Implement Jool-based NAT64 translator May 13, 2023
@frebib frebib changed the title [nat64: T160: Implement Jool-based NAT64 translator nat64: T160: Implement Jool-based NAT64 translator May 13, 2023
@frebib frebib marked this pull request as ready for review May 13, 2023 15:32
@vyosbot vyosbot requested a review from a team May 13, 2023 15:32
@frebib
Copy link
Contributor Author

frebib commented May 13, 2023

Still to do is:

  • op mode
  • documentation
  • nat46 (bib) translation
  • maximum 1 per namespace check

Opening this up for review with what there is before I add anything else. I think this works correctly as it is

@frebib frebib changed the title nat64: T160: Implement Jool-based NAT64 translator T160: nat64: Implement Jool-based NAT64 translator May 14, 2023
src/conf_mode/nat64.py Outdated Show resolved Hide resolved
@jack9603301
Copy link
Contributor

Welcome and look forward to the merger

config["pool4"] = pool4

# pylint: disable=invalid-name
with open(f"{JOOL_CONFIG_DIR}/{name}.json", "w", encoding="utf-8") as f:
Copy link
Member

Choose a reason for hiding this comment

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

Can you please use vyos.utils.file.read_file() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is writing a file. I see vyos.utils.file.read_json() but no write_json(). Should I create a write_json() and use that?

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry, then please use: vyos.utils.file.write_file() instead. Example: https://github.com/vyos/vyos-1x/blob/current/src/conf_mode/container.py#L364

interface-definitions/nat64.xml.in Outdated Show resolved Hide resolved
src/conf_mode/nat64.py Outdated Show resolved Hide resolved
from vyos.config import Config
from vyos.configdict import dict_merge, is_node_changed
from vyos.util import check_kmod, cmd, dict_search, run
from vyos.xml import defaults
Copy link
Member

Choose a reason for hiding this comment

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

Why import defaults? I see no reference to <defaultValue> node in your XML definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Almost certainly a copy-paste and me not understanding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it looks like it's being used here https://github.com/vyos/vyos-1x/pull/1993/files#diff-301b6d4ad94f8356451b06c8710ea3b527e8dba33dc9727cdcb83997c05d8966R49

Should I just remove that defaults block entirely?

Copy link
Member

Choose a reason for hiding this comment

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

If you do not consume any defaults, please drop it

@c-po
Copy link
Member

c-po commented Aug 20, 2023

Can you please also rebase this code to the latest current HEAD? Other then that it looks like a good start.

Signed-off-by: Joe Groocock <me@frebib.net>
@dmbaturin
Copy link
Member

Need to add nftables support and re-evaluate after 1.4 is branched off.

@frebib
Copy link
Contributor Author

frebib commented Aug 31, 2023

jool upstream doesn't support nftables (yet). NICMx/Jool#273 (comment). Netfilter mode is probably the best we can do

I do think it's worth holding on this until nftables support is available. I don't like the current implementation at all. The only reason I didn't just use a container for this is because setting up the routing is hard

@github-actions
Copy link

github-actions bot commented Oct 9, 2023

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@sever-sever
Copy link
Member

@frebib could you re-base?

sever-sever added a commit to sever-sever/vyos-1x that referenced this pull request Dec 5, 2023
Add NAT64

The original PR vyos#1993
There are no responces from the author of PR and PR had conflicts

Write fixed for JSON write files and update the base
Deleted unused default values
Simple changes

Example:
```
set nat64 source rule 100 source prefix '64:ff9b::/96'
set nat64 source rule 100 translation pool 10 address '192.168.122.10'
set nat64 source rule 100 translation pool 10 port '1-65535'
```
@sever-sever sever-sever mentioned this pull request Dec 5, 2023
12 tasks
@sever-sever
Copy link
Member

I created a new PR #2573 with the required changes and updated the base.

sever-sever added a commit to sever-sever/vyos-1x that referenced this pull request Dec 5, 2023
Add NAT64

The original PR vyos#1993
There are no responces from the author of PR and PR had conflicts

Write fixed for JSON write files and update the base
Deleted unused default values
Simple changes

Example:
```
set nat64 source rule 100 source prefix '64:ff9b::/96'
set nat64 source rule 100 translation pool 10 address '192.168.122.10'
set nat64 source rule 100 translation pool 10 port '1-65535'
```
@frebib
Copy link
Contributor Author

frebib commented Dec 5, 2023

Apologies for the radio silence here, it's a busy time of the year. I probably won't have much time to work on this for the next couple of weeks if you'd still like me to pick anything up here. Alternatively I'm happy for someone else to pick this up. I do feel like my crude implementation needs a lot of work, even if it does work just fine

@sever-sever
Copy link
Member

I created a new PR #2573 with the required changes and updated the base.

Updated PR #2578

@sever-sever
Copy link
Member

merged in PR #2578

@sever-sever sever-sever closed this Dec 7, 2023
@frebib frebib deleted the nat64 branch December 7, 2023 08:03
@frebib
Copy link
Contributor Author

frebib commented Dec 7, 2023

Thanks for pushing this over the line @sever-sever

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants