podman-py icon indicating copy to clipboard operation
podman-py copied to clipboard

Container object uses wrong key path for ports on listed containers.

Open MattBelle opened this issue 1 year ago • 2 comments

The container object is currently pulling ports from Container.NetworkSettings.Ports:

https://github.com/containers/podman-py/blob/8a4cbf3c8aaf90bfc36b5611288b9de9becca469/podman/domain/containers.py#L58-L63

This works when you are inspecting an individual container like:

container = podman_client.containers.get(some_container_id)
print(container.ports)

but fails when you get containers from a list operation eg:

for container in podman_client.containers.list():
    print(container.ports)

Some brief playing around with the raw REST API makes it look like the object schema returned from those 2 endpoints differs dramatically. The correct spot in the list schema appears to be Container.Ports, but the format of the data would still need to be massaged if we want a coherent schema in the SDK.

Single container inspect returns:

{
'18630/tcp': [
            {
                'HostIp': '0.0.0.0',
                'HostPort': '45753',
            },
        ]
}

while the listed container payload is:

 [
        {
            'host_ip': '',
            'container_port': 18630,
            'host_port': 45753,
            'range': 1,
            'protocol': 'tcp',
        },
    ]

MattBelle avatar Oct 28 '24 22:10 MattBelle

Assuming we want Container objects with identical IDs to have equivalent property values regardless of their source there are couple different solutions I can think of:

  1. Manually attempt to build the format provided by the inspect command. Only issue I can see with this is that the host_ip doesn't seem to be getting properly populated and there doesn't seem to be another spot in the response to pull that data from.
  2. Post-process what is returned by the list endpoint with individual GET operations. Eg something like:
[podman_client.containers.get(container.id) for container in podman_client.containers.list()]

This obviously has the downside of processing overhead. I could also see this being unwanted behavior if someone explicitly wanted the schema returned by the LIST object.

  1. Some kinda of background upgrade procedure. This would be similar to solution 2, but would be lazily evaluated. EG if someone tried to access the ports for example we would "upgrade" the object by doing the GET behind the scenes and update the field states accordingly.
  2. Make the list object a different type of python object to be more transparent to the users that they need to be treated differently.

I'd be happy to implement the fix, but am relatively new to contributing to and using this project so I would be interested in hearing the thoughts/preferences of people more experienced before I settle on a particular type of solution.

MattBelle avatar Oct 29 '24 01:10 MattBelle

I'd be happy to implement the fix, but am relatively new to contributing to and using this project so I would be interested in hearing the thoughts/preferences of people more experienced before I settle on a particular type of solution.

Thanks for the report. You can always ask and me or other maintainers will try to help you with reviews and advices.

So, my quick take on the issue.

Regarding your first comment

This works when you are inspecting an individual container like: ... but fails when you get containers from a list operation eg: ...

This can be solved by

for container in podman_client.containers.list():
    container.reload()
    print(container.ports)

We know that the reload call is expensive so we don't fetch the status while doing list. A possible solution to this is to implement the sparse keyword. See another issue here


Some brief playing around with the raw REST API makes it look like the object schema returned from those 2 endpoints differs dramatically. The correct spot in the list schema appears to be Container.Ports, but the format of the data would still need to be massaged if we want a coherent schema in the SDK.

This I am not sure why it happens but changing the payload needs to be done on podman side and it will break the API anyway.


About your second comment.

Options 1 and 2:

I would avoid the post process for the reasons you said

Option 3:

This is probably the best approach

Option 4:

I would avoid since it would break the api


Given that the issue seems to be the difference in get and list behavior I would squeeze the requirements to change the list method to use the sparse keyword. This will not break the api, and it will also add the overhead of reloading the containers on-demand.

What do you think @MattBelle ?

inknos avatar Oct 29 '24 11:10 inknos