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

Add time exceeded flow for fip #230

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ts170710
Copy link

#211 #211 Add "ip4.src == <external_ip>" to match for distinguish between external router gateway and floatingIP to handle time exceeded packet.

@ts170710 ts170710 force-pushed the add_flow_for_fip branch 6 times, most recently from 3dff464 to 0228d0d Compare December 27, 2023 09:49
@ts170710 ts170710 closed this Dec 27, 2023
@ts170710 ts170710 reopened this Dec 27, 2023
@ts170710 ts170710 force-pushed the add_flow_for_fip branch 4 times, most recently from 73e6781 to 2868a16 Compare December 28, 2023 03:12
Add "ip4.src == <external_ip>" to match for distinguish between
external router gateway and floatingIP to handle time exceeded
packet.

Signed-off-by: Huanghailun <hailun.huang@easystack.cn>
Copy link
Collaborator

@dceara dceara left a comment

Choose a reason for hiding this comment

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

@ts170710 Thanks for your PR!

I guess I'm not sure I understand how this is supposed to be useful. We already have flows in IP_INPUT stage that check for ip.ttl == {0, 1} && !ip.later_frag packets and reply with Time Exceeded for those. Why isn't that enough?

@dceara
Copy link
Collaborator

dceara commented Jan 23, 2024

@ts170710 Thanks for your PR!

I guess I'm not sure I understand how this is supposed to be useful. We already have flows in IP_INPUT stage that check for ip.ttl == {0, 1} && !ip.later_frag packets and reply with Time Exceeded for those. Why isn't that enough?

@ts170710 just in case you missed my question above.

Thanks,
Dumitru

@ts170710
Copy link
Author

@ts170710 Thanks for your PR!
I guess I'm not sure I understand how this is supposed to be useful. We already have flows in IP_INPUT stage that check for ip.ttl == {0, 1} && !ip.later_frag packets and reply with Time Exceeded for those. Why isn't that enough?

@ts170710 just in case you missed my question above.

Thanks, Dumitru

@dceara Thank you for your comment!
As described in #211, ip.ttl == {0, 1} treats floatingIPs and router gateway IPs together. When traceroute to cloud instances associated with the router, the router gateway IP address gets replaced by ip4.dst <-> ip4.src, resulting in the inability to display the router gateway IP address in the traceroute results.

Copy link
Collaborator

@dceara dceara left a comment

Choose a reason for hiding this comment

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

Please add a new system test for the scenarios you're fixing now.

Also, your change seems to break existing system tests, e.g.:
https://github.com/ovn-org/ovn/actions/runs/7346305995/job/20000851003?pr=230#step:7:4323

  41: system-ovn.at:3597 DNAT and SNAT on distributed router - N/S - IPv6 -- ovn-northd -- parallelization=yes -- ovn_monitor_all=yes
      ovnnat
  42: system-ovn.at:3597 DNAT and SNAT on distributed router - N/S - IPv6 -- ovn-northd -- parallelization=yes -- ovn_monitor_all=no
      ovnnat
  45: system-ovn.at:3941 DNAT and SNAT on distributed router - E/W - IPv6 -- ovn-northd -- parallelization=yes -- ovn_monitor_all=yes
      ovnnat
  46: system-ovn.at:3941 DNAT and SNAT on distributed router - E/W - IPv6 -- ovn-northd -- parallelization=yes -- ovn_monitor_all=no
      ovnnat

ds_clear(ip_ds);
struct ovn_nat *nat_entry = &op->od->nat_entries[i];
if (!strcmp(nat_entry->nb->type, "dnat_and_snat")) {
ds_put_format(match, "ip4.src == %s && ", nat_entry->nb->external_ip);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about IPv6? Why only on l3dgw ports? Why not on gw routers too?

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.

None yet

2 participants