Support changing host veth name

[M1cha]

Depends on:
containers/common#2245
containers/podman#24535

[M1cha]

I don't know why, but I can't get the issue linked properly.

[Luap99]

instead of using default I think it is cleaner to wrap this in an Option<>. While that makes accessing values a step more I think it carries the intend better that there can be empty.

[M1cha]

Agree, fixed.

[Luap99]

please use parse_option() as split out host_interface_name as constant like the other options

I know parsing a String cannot fail so the error handling is not needed technically but I think it better to use it consistently.

[M1cha]

Fixed. Do we need to make clear which options come from the network and which from the per-network-config? With the current code, the messages and variables look the same for both kinds of options.

[Luap99]

And also this needs tests, please have a look at test/100-bridge-iptables.bats
This should have a test cases where it fails if the veth name exists on the host and one where it works fine and you check the correct interface was created.

[Luap99]

    host.create_link(host_veth).map_err(|err| match err {
        NetavarkError::Netlink(ref e) if -e.raw_code() == libc::EEXIST => NetavarkError::wrap(
            format!(
                "create veth pair: interface {} already exists on container namespace",
                data.container_interface_name
            ),
            err,
        ),
        _ => NetavarkError::wrap("create veth pair", err),
    })?;

This will likely return a wrong/misleading error now. Given both interface are created in one call it would be impossible to know which name conflicted. Maybe we can check if veth_name not empty then chnage the message to hint at both possibilities

[M1cha]

Does this change cause the integration test failures? because they seem unrelated at first glance.

[Luap99]

not ok 108 nftables - check error message from netns thread
# (from function `assert' in file test/helpers.bash, line 348,
#  from function `assert_json' in file test/helpers.bash, line 382,
#  in test file test/250-bridge-nftables.bats, line 359)
#   `assert_json ".error" "create veth pair: interface eth0 already exists on container namespace: Netlink error: File exists (os error 17)" "interface exists on netns"' failed
#  nsenter -n -m -w -t 23168 ip link set lo up
#  nsenter -n -m -w -t 23170 ip link add eth0 type dummy
#  nsenter -n -m -w -t 23168 ./bin/netavark --rootless false --config /tmp/netavark_bats.zFocvy/config --file /var/tmp/netavark/test/testfiles/simplebridge.json setup /proc/23170/ns/net
# {"error":"create veth pair: interface eth0 already exists on container namespace or  exists on host namespace: Netlink error: File exists (os error 17)"}
# [ rc=1 (expected) ]
#  jq -r .error
# create veth pair: interface eth0 already exists on container namespace or  exists on host namespace: Netlink error: File exists (os error 17)
# #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
# #|     FAIL: interface exists on netns
# #| expected: 'create veth pair: interface eth0 already exists on container namespace: Netlink error: File exists (os error 17)'
# #|   actual: 'create veth pair: interface eth0 already exists on container namespace or  exists on host namespace: Netlink error: File exists (os error 17)'
# #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Yes it is your new error message, it seems your condition in the code is inverted.

[Luap99]

need to use !is_empty() or better swap both branches around

[M1cha]

agree, fixed

[Luap99]

please assert your exact expected error message here

[M1cha]

I gotta say, it's pretty cool that the tests can do this. done.

[mheon]

LGTM

[Luap99]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, M1cha

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment