Introduces Address type to be used in secondary IPv4 and IPv6 inspect data structure

[fgimenez]

What this PR does / why we need it:

Fixes discrepancy between the types used in inspect for docker and podman. This causes a panic when using the docker client against podman when the secondary IP fields in the NetworkSettings inspect field are populated.

How to verify it

• podman system service --log-level debug -t 0 unix:///${HOME}/podman.sock > /var/log/podman.log 2>&1 &
• Create a container with SecondaryIPV6Address populated in inspect's NetworkSettings
• ln -s /root/podman.sock /var/run/docker.sock
• Use docker cli to inspect the container:

inspected, err := cli.ContainerInspect(context.Background(), container.ID)
if err != nil {
  log.Panicf("could not inspect container %v\n", err)
} 

Which issue(s) this PR fixes:

Fixes #12165

Signed-off-by: Federico Gimenez [email protected]

[rhatdan]

@Luap99 @mheon @baude PTAL

[Luap99]

@fgimenez How do you create such a container?

[Luap99]

I think we might want to fix this only for the compat API and not regular inspect, @mheon WDYT?

[fgimenez]

@fgimenez How do you create such a container?

I'm finding this issue on CI doing something similar to:

$ sudo podman run --privileged -ti --entrypoint /bin/bash quay.io/kubevirtci/bootstrap:podman-test16
# inside the container
$ podman system service --log-level debug -t 0 unix:///${HOME}/podman.sock > /var/log/podman.log 2>&1 &
$ git clone https://github.com/kubevirt/kubevirtci && cd kubevirtci
$ make cluster-up # fails with default podman 3.4.1, succeeds with the changes in this PR
$ podman inspect k8s-1.20-node01
[
    {
................
        "NetworkSettings": {
            "EndpointID": "",
            "Gateway": "10.88.0.1",
            "IPAddress": "10.88.0.2",
            "IPPrefixLen": 16,
            "IPv6Gateway": "",
            "GlobalIPv6Address": "",
            "GlobalIPv6PrefixLen": 0,
            "SecondaryIPv6Addresses": [
                "2001:db8:1::2/64"
            ],
..............

Will try to find a simpler example

[mheon]

Tend to agree with @Luap99 that this should be compat API only - this looks like a breaking change to me, and while we are in a breaking-change mode for 4.0, I don't see a good reason we should change inspect now, instead of just fixing the Docker-compat endpoint.

[fgimenez]

@Luap99 @mheon sgtm, where should I add the changes to make them compat API only?

[Luap99]

podman/pkg/api/handlers/compat/containers.go

Lines 350 to 357 in c0351a7

	n, err := json.Marshal(inspect.NetworkSettings)
	if err != nil {
	return nil, err
	}
	networkSettings := types.SummaryNetworkSettings{}
	if err := json.Unmarshal(n, &networkSettings); err != nil {
	return nil, err
	}

This is where the error happens. We cannot use json.marshal/unmarshal because the types do not match apparently so you would need to manually assign the value to the new struct.

[fgimenez]

I think we might want to fix this only for the compat API and not regular inspect

Ok done, checked in our use case and works fine, also added a few unit tests. @Luap99 @mheon PTAL

[rhatdan]

@fgimenez Tests are failing.

[fgimenez]

@fgimenez Tests are failing.

@rhatdan thanks for the heads up! ok that error should be fixed now, I see now this test failing https://github.com/containers/podman/pull/12174/checks?check_run_id=4149334267 not sure if it is related to the changes in the PR?

[rhatdan]

@Luap99 @jwhonce @mheon PTAL

[Luap99]

I think you also have to set the PrefixLen field in this struct, although I am not sure how we could get this value here if we do not change the type in libpod inspect as well.

[fgimenez]

Indeed, I assumed that the strings in the current SecondaryIPv6Addresses's []string in libpod inspect represent actual addresses, not CIDRs, so the default value of PrefixLen of 0 should be ok, is this correct?

[fgimenez]

If the strings in libpod's inspect output can contain CIDRs then maybe we could try to parse them, split by / if present or something like that, wdyt?

[fgimenez]

Indeed, I assumed that the strings in the current SecondaryIPv6Addresses's []string in libpod inspect represent actual addresses, not CIDRs, so the default value of PrefixLen of 0 should be ok, is this correct?

Sorry, this was wrong, if SecondaryIPv6Addresses's []string in libpod inspect represent actual addresses then PrefixLen should be 64, not 0 (32 for IPv4). I've pushed changes to fix it, PTAL.

[Luap99]

I think this is not correct. You should have the actual ip/prefix for this container, hard coding it to 64 or 32 is wrong.
TBH looking at this I think your original patch works better. We are on 4.0 so breaking changes are possible. I do not think there many users of the secondary ip field anyway.
@mheon WDYT?

[mheon]

Changing the secondary IP field in podman inspect itself?

You're probably right that there are almost no users. I'm not strongly opposed to making a breaking change here.

[TomSweeneyRedHat]

Changes LGTM
one last failure on the "fedora-34 rootless host" test, I've restarted.

[flouthoc]

@fgimenez Needs a small test for inspect. :) or you could test CI via adding NO TESTS NEEDED to your commit message and add test later when everything is green.

[fgimenez]

@fgimenez Needs a small test for inspect. :) or you could test CI via adding NO TESTS NEEDED to your commit message and add test later when everything is green.

@flouthoc ok I've added a unit test for the changes, PTAL

[flouthoc]

@containers/podman-maintainers Could there be any issues with CI I think some unrelated tests are failing.

[mheon]

#12343 will fix CI once it merges, after that this will need to be rebased

[fgimenez]

#12343 will fix CI once it merges, after that this will need to be rebased

cool thx, I see it is already merged, rebased this one

[Luap99]

@fgimenez Could you squash your commits into one please, otherwise LGTM.

[fgimenez]

@fgimenez Could you squash your commits into one please, otherwise LGTM.

sure, done

[Luap99]

LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

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

[Luap99]

/lgtm
/hold

[Luap99]

/hold cancel