Fix remote exec ignoring --detach-keys option

[shiavm006]

Problem

The remote client completely ignores the --detach-keys option for podman exec sessions. Users cannot detach from exec sessions using custom key sequences.

The ExecStartAndAttach() function in pkg/bindings/containers/attach.go hardcodes empty detach keys:

// Line 461 - BEFORE
_, err := detach.Copy(socket, options.InputStream, []byte{})
//                                                   ^^^^^^^^ HARDCODED!

// Line 482 - BEFORE  
_, err := detach.Copy(options.GetOutputStream(), socket, []byte{})
//                                                        ^^^^^^^^ HARDCODED!

This fix complements the recent fix for empty --detach-keys="" in PR #27425 , maintaining consistency across attach, run, start, and exec commands.

    None 

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shiavm006
Once this PR has been reviewed and has the lgtm label, please assign nalind for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[Luap99]

This doesn't test anything really, you must ensure you actually test the detach sequence. These test would pass with or without this patch.

It is has been a while since I looked at this but there are some rather nasty bugs around detaching logic, #25083 may be a starting point maybe with this change you can get it to work.

[shiavm006]

I looked into testing actual detach behavior but it requires PTY/interactive terminal setup which is tricky in automated tests.

should i ?
Keep a simpler test that just verifies no error (but document it doesn't test behavior)
I'll also look at #25083 to see if there's a pattern I can follow.

[shiavm006]

@Luap99
PR #25083 which has the proper interactive test in test/system/450-interactive.bats. I see that test is currently skipped for remote exec (issue #25089) pending additional infrastructure work.
Remove the inadequate test and Focus solely on the code fix ,Reference your existing test infrastructure in the commit message

[Luap99]

Currently we seems to send the detach keys to the server and handles it there. If I try podman-remote on main I get

$ bin/podman-remote exec -it --detach-keys=a,b magical_mestorf sh
/home/podman # Error: container 48bebf64fb8f79cc5b834c3fd75592560e131b381de98962556c30a11c70d618 exec session 720a9282e1db5fb3d3f2695897737ce41072c7414ae1805742712a60f5199ded is still running, cannot remove: exec session state improper

So basically the issue is that the remote client didn't know the server detached and tried to remove the exec session so it still make sense to move the detach keys to the client side so we can catch and handle the detach error here. And then you need to make sure the remote client handles the detach error and

With your patch here it actually worse

$ bin/podman-remote exec -it --detach-keys=a,b magical_mestorf sh
/home/podman # ERRO[0001] Failed to write input to service: detached from container

<<< it just hangs here now forever until you kill the exec session or remote service

So we definitely need the proper tests and code to make it actually work, just by adding the keys you make it worse unfortunately.

The reason the common change in #25083 is needed is because currently the detach check is broken in that it only works when bytes are read one by one which may not be always the case but it may not be strictly needed for this fix.

Overall the exec REST API interface is quite awkward but I don't see how we could chnage it much so we need to make it work with what we got.

[shiavm006]

Currently we seems to send the detach keys to the server and handles it there. If I try podman-remote on main I get

$ bin/podman-remote exec -it --detach-keys=a,b magical_mestorf sh
/home/podman # Error: container 48bebf64fb8f79cc5b834c3fd75592560e131b381de98962556c30a11c70d618 exec session 720a9282e1db5fb3d3f2695897737ce41072c7414ae1805742712a60f5199ded is still running, cannot remove: exec session state improper

So basically the issue is that the remote client didn't know the server detached and tried to remove the exec session so it still make sense to move the detach keys to the client side so we can catch and handle the detach error here. And then you need to make sure the remote client handles the detach error and

With your patch here it actually worse

$ bin/podman-remote exec -it --detach-keys=a,b magical_mestorf sh
/home/podman # ERRO[0001] Failed to write input to service: detached from container

<<< it just hangs here now forever until you kill the exec session or remote service

So we definitely need the proper tests and code to make it actually work, just by adding the keys you make it worse unfortunately.

The reason the common change in #25083 is needed is because currently the detach check is broken in that it only works when bytes are read one by one which may not be always the case but it may not be strictly needed for this fix.

Overall the exec REST API interface is quite awkward but I don't see how we could chnage it much so we need to make it work with what we got.

I see now that the full solution requires more comprehensive changes like those
in #25083, plus the additional remote infrastructure tracked in #25089. i git ur point so i think i should close this pr as it create it more worse so closing it