Allow users to disable detach keys #4441
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan 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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
Fixes: #4208 |
| Override the key sequence for detaching a container. Format is a single character `[a-Z]` or | ||
| a comma separated sequence of `ctrl-<value>`, where `<value>` is one of: | ||
| `a-z`, `@`, `^`, `[`, `\\`, `]`, `^` or `_`. | ||
| Specify the key sequence for detaching a container. Format is a single character `[a-Z]` or `ctrl-<value>` where `<value>` is one of: `a-z`, `@`, `^`, `[`, `,` or `_`. Specifying "" will disable this feature. The default is *ctrl-p,ctrl-q*. |
There was a problem hiding this comment.
It's not a single character, though - it's one or more characters.
There was a problem hiding this comment.
you say "[a-Z]" and then "a-z" is it all lower chars or are upper allowed too?
There was a problem hiding this comment.
That is how the Code works. And this is how Docker was designed.
/bin/podman run -ti --detach-keys "Z" fedora /bin/sh
sh-5.0#
./bin/podman run -ti --detach-keys "ctrl-Z" fedora /bin/sh
Error: invalid detach keys: Unknown character: 'ctrl-Z'
./bin/podman run -ti --detach-keys "ctrl-z" fedora /bin/sh
sh-5.0#
There was a problem hiding this comment.
OK, then I think you need to change "one of : a-z" to "one of: a-Z" or I'd prefer "one of: a-z, A-Z"
| @@ -152,7 +152,7 @@ func (c *Container) attachToExec(streams *AttachStreams, keys string, resize <-c | |||
| func processDetachKeys(keys string) ([]byte, error) { | |||
| // Check the validity of the provided keys first | |||
| if len(keys) == 0 { | |||
| keys = define.DefaultDetachKeys | |||
| return []byte{}, nil | |||
There was a problem hiding this comment.
I don't know if this is safe for old containers - we might be disabling detach for every container created prior to this
There was a problem hiding this comment.
Yes, although I don't know a way around this. Is the detach-key stored somewhere? It does not get shown by podman inspect
There was a problem hiding this comment.
Actually after looking at the code, we don't save the detach keys anywhere. So every podman start|exec|attach will use the default keys unless overridden. We don't record the any of the following, as far as I can see
PodmanCommand
DetachKeys string
Latest bool
NoStdin bool
SigProxy bool
}
This makes the
podman create options ignored, I believe, because podman start will not use them.
Also podman run/start/create options, will not be used by podman exec, which matches what we have now, and I don't think it should block getting this in. But we could discuss whether or not these options should be recorded.
There was a problem hiding this comment.
Nice. I was worried we were in a real bind here.
I'd like to get detach keys added to the DB but that sounds like another PR.
| Override the key sequence for detaching a container. Format is a single character `[a-Z]` or | ||
| a comma separated sequence of `ctrl-<value>`, where `<value>` is one of: | ||
| `a-z`, `@`, `^`, `[`, `\\`, `]`, `^` or `_`. | ||
| Specify the key sequence for detaching a container. Format is a single character `[a-Z]` or `ctrl-<value>` where `<value>` is one of: `a-z`, `@`, `^`, `[`, `,` or `_`. Specifying "" will disable this feature. The default is *ctrl-p,ctrl-q*. |
rhatdan
force-pushed
the
detach
branch
2 times, most recently
from
275737c
to
8cf5098
Compare
|
/test-images |
|
/test images |
If user specifies --detach-keys="", this will disable the feature. Adding define.DefaultDetachKeys to help screen to help identify detach keys. Updated man pages with additonal information. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
|
LGTM |
|
@rhatdan you still have So just double checking, the format can be any single upper or lower case alpha character, or it can be one or more alpha lower case characters (and other special chars)? Upper case can't be used if more than once character is used? |
|
Seems to be the way that it works. Although I think it would be kind of crazy to use a single character. |
|
@rhatdan I find that really odd, but thanks for the 411. A belated LGTM then. |
github-actions
bot
added
the
locked - please file new issue/PR
If user specifies --detach-keys="", this will disable the feature.
Adding define.DefaultDetachKeys to help screen to help identify detach keys.
Updated man pages with additonal information.
Signed-off-by: Daniel J Walsh dwalsh@redhat.com