test/system: Fix the IPv6 address mismatch error

[huangyum]

For IPv6 addresses within the same scope group, Linux kernel preserves insertion order, where the first inserted address appears last. As a result, the ordering inside pasta differs from the host, causing a test to fail when they compare only the first address on each side. Fix this by checking that the container’s first address matches any of the host’s addresses.

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

• Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
  commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
  the sign-off email address. See CONTRIBUTING.md
  for more information.
• Referenced issues using Fixes: #00000 in commit message (if applicable)
• Tests have been added/updated (or no tests are needed)
• Documentation has been updated (or no documentation changes are needed)
• All commits pass make validatepr (format/lint checks)
• Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

None

NONE

[Honny1]

Thanks, LGTM.

PTAL @Luap99

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Honny1, huangyum

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 [Honny1]

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

[Luap99]

cc @sbrivio-rh @dgibson

[Luap99]

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.

[huangyum]

Thanks for the review. Will update.

[Luap99]

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

[huangyum]

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

[sbrivio-rh]

cc @sbrivio-rh @dgibson

Thanks @Luap99 for the Cc:, I wasn't aware of this. This comes from https://bugs.passt.top/show_bug.cgi?id=175#c12 by the way.

I'll review it, I have some comments on top of what you already mentioned.

[sbrivio-rh]

Thanks @Luap99 for the Cc:, I wasn't aware of this.

Do we have a way to block merges for given paths if a given reviewer didn't have the chance to check yet? Or at least something like the Linux kernel's get_maintainer.pl script encouraged in the policy (similar to https://docs.kernel.org/process/submitting-patches.html#select-the-recipients-for-your-patch)?

Otherwise these changes risk just being LGTMed away. I'm not sure if anything automated is feasible / worth the effort though.

[sbrivio-rh]

Review of commit message:

For IPv6 addresses within the same scope group, Linux kernel preserves
insertion order, where the first inserted address appears last. As a

I guess you mean that it does not preserve the insertion order?

result, the ordering inside pasta differs from the host, causing a

it appears last in iproute2, but that's because of the order addresses are reported in via netlink -- pasta doesn't check how addresses appear to the user.

So perhaps instead of "appears" you could say "is reported last, via netlink". Otherwise it's not clear why pasta cares at all.

test to fail when they compare only the first address on each side.
Fix this by checking that the container’s first address matches any of
the host’s addresses.

In general:

• I think you should point out that you're fixing this in the kernel, and perhaps wait to submit this so that we have a reference for the kernel change (thread at https://lore.kernel.org/netdev/20251126083714.115172-1-yuhuang@redhat.com/).

• This needs a reference to the original issue, that is, https://bugs.passt.top/show_bug.cgi?id=175, otherwise it's not clear what issue this is covering.

[sbrivio-rh]

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).

[Luap99]

Thanks @Luap99 for the Cc:, I wasn't aware of this.

Do we have a way to block merges for given paths if a given reviewer didn't have the chance to check yet? Or at least something like the Linux kernel's get_maintainer.pl script encouraged in the policy (similar to https://docs.kernel.org/process/submitting-patches.html#select-the-recipients-for-your-patch)?

Hope other maintainers pay attention and ping the relevant persons and give them a chance to respond.

Process wise, in theory there is the CODEOWNERS file used by github which can list people based on files as owners. However in order to be listed there you need write permissions and well that kind of repo access depends on our Governance https://github.com/containers/podman/blob/main/GOVERNANCE.md
I don't think handing out write perms for people owning a few files would scale well as we don't have the concept of subsystem maintainers or anything like that.

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners

[huangyum]

Review of commit message:

Thank you @sbrivio-rh , I will update that later.

In general:

* I think you should point out that you're fixing this in the kernel, and perhaps wait to submit this so that we have a reference for the kernel change (thread at https://lore.kernel.org/netdev/20251126083714.115172-1-yuhuang@redhat.com/).

Should I hold this and wait til the kernel patch gets merged?

* This needs a reference to the original issue, that is, https://bugs.passt.top/show_bug.cgi?id=175, otherwise it's not clear what issue this is covering.

Sure, will add it.

[sbrivio-rh]

Should I hold this and wait til the kernel patch gets merged?

Personally I would, yes, but if you're trying to get this out of your way quicker, I think it would be important to mention that you plan to change this in the kernel in the near future, so that if we go back and look at Podman commit messages in the future it will be easy to find the corresponding reference some time later in the kernel history.

[huangyum]

Should I hold this and wait til the kernel patch gets merged?

Personally I would, yes, but if you're trying to get this out of your way quicker, I think it would be important to mention that you plan to change this in the kernel in the near future, so that if we go back and look at Podman commit messages in the future it will be easy to find the corresponding reference some time later in the kernel history.

Thanks for the suggestion! I'll hold off on updating this PR until the kernel patch is merged.

[sbrivio-rh]

I'll hold off on updating this PR until the kernel patch is merged.

Just for clarity: I don't think you really need to, it's more practical if you do.

But if you don't, at least you should mention the fact that you have a pending/proposed kernel change in the commit message here.