Skip to content

test/system: Fix the IPv6 address mismatch error #27623

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.

Sign up for GitHub

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

Open
huangyum wants to merge 1 commit into
base: main
Choose a base branch
from
Open

test/system: Fix the IPv6 address mismatch error #27623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions test/system/505-networking-pasta.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,10 +307,10 @@ function pasta_test_do() {
run_podman run --rm --net=pasta $IMAGE ip -j -6 address show

local container_address="$(ipv6_get_addr_global "${output}")"
local host_address="$(default_addr 6)"
local host_addresses="$(all_addr 6)"

assert "${container_address}" = "${host_address}" \
"Container address not matching host"
echo "${host_addresses}" | grep -qw "${container_address}"
assert $? -eq 0 "Container address not matching host"
Comment on lines +312 to +313
Copy link
Member

@Luap99 Luap99 Nov 28, 2025

Choose a reason for hiding this comment

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

can't you just use assert "${host_addresses}" =~ "${container_address}"?

Right this the assert for exit code is unnecessary as the command run with set -e meaining if the grep fails it exits right away without reaching the exit code check anyway.
And then then generally leads to a poor error message compared to our assert helper.

Copy link
Author

@huangyum huangyum Dec 3, 2025

Choose a reason for hiding this comment

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

Thanks for the review. Will update.

}

@test "IPv6 address assignment" {
Expand Down
8 changes: 8 additions & 0 deletions test/system/helpers.network.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,3 +449,11 @@ function default_addr() {
local expr='[.[0].addr_info[] | select(.deprecated != true)][0].local'
ip -j -"${ip_ver}" addr show "${ifname}" | jq -rM "${expr}"
}

function all_addr() {
Copy link
Collaborator

@sbrivio-rh sbrivio-rh Nov 30, 2025

Choose a reason for hiding this comment

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

I just realised that there's now a default_addr() function here as a result of 4c1c4c0 ("Vendor latest c/common and fix tests"), while it was introduced by fe8355b ("pasta: Fix pasta tests to work on hosts with multiple interfaces") intended as a local helper.

The original functions I introduced for that purpose were ipv{4,6}_get_addr_global() by the way. So, we already have a bit of duplication, but I don't think that's a good reason to add more. For example, there are already two issues introduced/not fixed as a result of this duplication:

  • commit d418391 ("test, pasta: Ignore deprecated addresses in tests") was applied to default_addr() only, not to ipv6_get_addr_global(), which might suffer from the same issue
  • all the other helpers have the usual kerneldoc-style documentation, with a description of what they do and their arguments, but default_ifname() and default_addr() don't (it looks like Pasta networking tests don't work on a host with multiple interfaces #19007 wasn't actually reviewed before merging it)

Having a function that returns all host-side addresses and checking that the first global unicast one assigned in the container fixes the issue that, given host (initial namespace) IPv6 addresses h1, h2, h3, reported in that order, and container addresses c1 (matching h3), c2 (matching h2) , c3 (matching h1), we'll actually find that c1 matches h3 and make the test pass.

But it doesn't fix the fact that, for example, if we have host address h1, and container addresses c1 (not matching any host address) and c2 (matching h1), the test should pass as pasta copied all the addresses from the host (that's the expectation). However, it will fail, because we fetch only the first address in the container.

So, ideally, we should check that all the host addresses are reflected in the container, to match what pasta currently does and what Podman expects (and might actually strictly need, at some point).

If that's too complicated or beyond the scope of this fix, we could at least make this slightly more future-proof and keep duplication to a minimum by:

  • making all these functions (ipv{4,6}_get_addr_global(), default_addr()) return by default all the addresses in the given scope, sorted in their numeric order or an arbitrary order if it's easier (as long as the order is the same for functions used on the two sides)
  • adding an optional documented parameter that can be used to limit the output to a single address

...then, once somebody has time to implement the proper test, there could be another function iterating on the addresses found on the host and making sure that each one of them are present on the container (it's trivial if addresses are sorted).

local ip_ver="${1}"
local ifname="${2:-$(default_ifname "${ip_ver}")}"

local expr='[.[0].addr_info[] | select(.deprecated != true)]'
ip -j -"${ip_ver}" addr show "${ifname}" | jq -rM "${expr}" | grep local | cut -d'"' -f4 | tr '\n' ' '
Copy link
Member

@Luap99 Luap99 Nov 28, 2025

Choose a reason for hiding this comment

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

Why all this grep, cur, tr thing? If we only want the local addresses we can just add them to the jq filter.

'[.[0].addr_info[] | select(.deprecated != true)].[].local'

which seems to return the same result unless I am missing something

Copy link
Author

@huangyum huangyum Dec 3, 2025

Choose a reason for hiding this comment

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

Yes, that would return the same result. I will update in next push.

}