-
Notifications
You must be signed in to change notification settings - Fork 16
Enhancement: Improved UPF routing and NAT handling #38
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
base: main
Are you sure you want to change the base?
Enhancement: Improved UPF routing and NAT handling #38
Conversation
081a98a
to
fd8760a
Compare
Hi @hxngillani, thanks for your contribution Hi @llpeterson, @mbilal92, can you please review this PR? Thanks! |
{% if '1' in core.upf.additional_upfs %} \ | ||
sudo iptables -t nat -C POSTROUTING -s {{ core.upf.additional_upfs['1'].ue_ip_pool }} -o {{ core.data_iface }} -j MASQUERADE || \ | ||
sudo iptables -t nat -A POSTROUTING -s {{ core.upf.additional_upfs['1'].ue_ip_pool }} -o {{ core.data_iface }} -j MASQUERADE; \ | ||
{% endif %} \ | ||
{% if '2' in core.upf.additional_upfs %} \ | ||
sudo iptables -t nat -C POSTROUTING -s {{ core.upf.additional_upfs['2'].ue_ip_pool }} -o {{ core.data_iface }} -j MASQUERADE || \ | ||
sudo iptables -t nat -A POSTROUTING -s {{ core.upf.additional_upfs['2'].ue_ip_pool }} -o {{ core.data_iface }} -j MASQUERADE; \ | ||
{% endif %} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if someone wants to deploy more than 2 additional UPFs? Would it be better to replace these if
statements by a for
loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for review great idea let me do that and test if it works will then amend in PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fd8760a
to
cea71ab
Compare
roles/upf/tasks/install.yml
Outdated
|
||
# ignore_errors: yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ignore_errors
is not used, I think it is better to just remove it
# ignore_errors: yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gab-arrobo I thought ignore_errors was added for some reason, but it was already there should i remove it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gab-arrobo i for now i have removed as it is not being used
[Route] | ||
Gateway={{ core.upf.default_upf.ip.core }} | ||
Destination={{ core.upf.default_upf.ue_ip_pool }} | ||
|
||
# Additional UPFs - Dynamically Generated Routes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hxngillani, this approach works well if you know the number of UPFs to deploy upfront. However, if you add more UPFs later, you’d need to rerun the setup. It would be better to make this independent, allowing users to deploy additional UPFs without reconfiguring the system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i agree my idea was to put routing related configs in route section because when we enable multiple upfs in main.yml i assumed these will be deployed so route will configure but yeah route config while ups is added is also fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i will amend with previous route configuration :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great, I’d recommend creating a router configuration file as part of the additional UPF role.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbilal92 done please check
@hxngillani left a comment. Let's keep support to add additional UPFs apart from the router. |
470ec9e
to
fda8928
Compare
@@ -10,4 +10,4 @@ Address={{ core.upf.core_subnet }} | |||
|
|||
[Route] | |||
Gateway={{ core.upf.default_upf.ip.core }} | |||
Destination={{ core.upf.default_upf.ue_ip_pool }} | |||
Destination={{ core.upf.default_upf.ue_ip_pool }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add an empty line to the files that you modified and have the red symbol
c537d64
to
d73e970
Compare
@mbilal92, any update on the review for this PR? Thanks! |
@gab-arrobo, looks good to me. |
@mbilal92 for testing i used below commands to make sure that UE NAT service is UP and running and MASQUERADE rules show expected subnets and also i used UERANSIM to connected UE with additional upf and i able to access internet successfully.
|
Hi all, I ran into this issue (improperly configured NAT for additional UPFs) and found the following simple fix: Add the following at the end of deps/5gc/roles/upf/tasks/install.yml:
Then, any time a new UPF is added, simply do I'm a bit worried that this PR would require a |
Hi @hxngillani, what are your thoughts about Jed's comment? Hi @jrmcclurg, feel free to open a PR with your solution. |
Thanks @gab-arrobo. @hxngillani please let me know if my approach above works in your setup. I haven't done extensive testing with it, so I may be missing something. If it works for you, I can make it into a PR. |
@hxngillani, please rebase your PR. Thanks! |
- Replaced static aether-ue-nat.service with dynamic aether-ue-nat.service.j2 - Updated install.yaml to use Jinja2 templating for NAT service - Improved 20-aether-core.network to dynamically add routes for additional UPFs - Modified add UPF module to check for existing routes before adding - Ensured consistent route management between core network and UPF installation Signed-off-by: hxngillani <[email protected]>
Signed-off-by: hxngillani <[email protected]>
d73e970
to
8985a13
Compare
@jrmcclurg Thank you for the solution. can you please try to reboot your system and check if routes that you are configuring in install are persistent? |
@hxngillani I can confirm that the NAT routes installed by my fix are not persistent after a restart. However, I have also found that many other routes are also not persistent (e.g., those installed by the many So I think your PR is targeting an important issue (persistence of routes), but it only addresses it in the context of UPF-related routes. Much more work would be needed to fully address that issue. More importantly, I am concerned about the need to do |
Hi all, I have added my fix as a PR. |
Enhancement: Dynamic UPF Routing and NAT Handling
Changes Made:
aether-ue-nat.service
withaether-ue-nat.service.j2
for dynamic NAT configuration..install.yaml
to generate the NAT service dynamically based on UPF configuration.20-aether-core.network
to dynamically add routes for additional UPFs.Why This Change?
aether-ue-nat.service
was static, meaning new UPFs required manual modifications.RTNETLINK answers: File exists
when adding new UPFs.Testing & Validation:
Notes for Reviewers: