Adapt firewall-port to IPv6

[benjamreis]

No description provided.

[lindig]

This generally looks good; would like @rosslagerwall to take a look at the handling o IP tables.

[lindig]

I am not comfortable with raising Not_found. This function should return an option and force the caller to handle this or raise a meaningful error here.

[psafont]

List.nth_opt pifs 0 would do this

[lindig]

@benjamreis any comments on our comments?

[benjamreis]

@benjamreis any comments on our comments?

Hi yeah sorry - I was trying to apply your comments and tst them locally but I'm hitting wall after wall.
I'm not sure what to do when there's no mangamnt address to determine the IP family.

I'll rework the shell option handling but i hav a lot on my plate so I'm often interrupted during my development. I'll try to rcommit something asap

[benjamreis]

What if instead getting the management address type from th XAPI I'd read in xnsource-inventory?

[benjamreis]

I don't understand the failure of the tests. Looks unrelated to my changes - should I rebase on a newer master? 🤔

[lindig]

Yes, always stay on top of master

[gthvn1]

I don't understand the failure of the tests. Looks unrelated to my changes - should I rebase on a newer master? 🤔

👋, it is really strange. I tried to understand the issue and I noticed something that might help pinpoint the issue:
When I modify the function get_management_iface_primary_address_type to avoid calling the Xapi inventory lookup the test passes. It seems that the problem is tied to the addition of this lookup:

+let get_management_iface_primary_address_type =
+  Record_util.primary_address_type_of_string
+    (Xapi_inventory.lookup Xapi_inventory._management_address_type)

Using strace, I observed that with your patch, the test _build/default/ocaml/tests/test_bounded_psq.exe attempts to access /etc/xensource-inventory. After this, there are attempts to access temporary files like /etc/435s2f.tmp, which results in the reported error: Fatal error: exception Sys_error("/etc/128f1f.tmp: Permission denied").

Without your patch, these file accesses don’t occur during the test (still spotted using strace). This suggests that something related to the inventory lookup might not be mocked correctly in the test environment.

I don't understand enough how the tests are setup and why the lookup is causing this behavior but I hope my observations help clarify the issue or evoke some ideas from someone more familiar with the test setup!

Let me know if there is anything I can do to test further as I can run tests locally now.

[benjamreis]

It seems I'm missing a mock of the inventory file somewher for the tests but for the life of me I can't find anything... any leads would be very welcomed 😇

Beside that we noticed that the rules in iptables are different from the ones in ip6tables:

• iptables uses ctstate: ACCEPT tcp anywhere anywhere ctstate NEW tcp dpt:http
• ip6tables uses state: ACCEPT tcp anywhere anywhere state NEW tcp dpt:http

and so when calling firewall-port for ipv6 the rules is not removed because the chain given is with ctstate... and hen opening the port we get 2 lines with ctstate and state... so my question is should change the rule given to ip6tables in ipv6 or should the default ip6 rules should be migrated towards ctstate as well?

Thx for the help 🙏

[lindig]

I believe in the Git test environment /etc/xensource-inventory does not exist, hence Xapi_inventory.lookup Xapi_inventory._management_address_type does not work. @psafont This file only exists in a deployment. So it's probably necessary to adjust the test.