attach: fix attach when cuid is too long #1704
Conversation
|
LGTM |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude 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
giuseppe
force-pushed
the
attach-cuid-too-long
branch
2 times, most recently
from
f90c6be
to
8144a3f
Compare
|
Should this patch be in a separate Unix/Linux only code path? |
giuseppe
force-pushed
the
attach-cuid-too-long
branch
from
8144a3f
to
2c55a41
Compare
not sure, do we support anything else than Unix? |
|
also I am still unsure how to fix conmon (and then again in podman) when the substring gives a path that is already existing. It is a corner case but still can happen |
giuseppe
force-pushed
the
attach-cuid-too-long
branch
2 times, most recently
from
12d4b63
to
4aab5ee
Compare
|
and patch for conmon to fix the symlink length corner case: cri-o/cri-o#1875 |
|
I am really concerned about what happens during collisions - if we start a few hundred to a thousand containers, do we start hitting this frequently? We should try and make sure that a sizable portion of the UUID makes it in there. If we manually specify a libpod run path long enough, we could end up generating completely unusable attach paths like this (cut off everything except part of the path to the attach sockets directory) |
I think we should just error out and not accept any runroot that is longer than 40-50 characters so that we have enough room for the UUID and avoid such kind of issues. What do you think? I can add another patch to this PR if you like the idea. |
giuseppe
force-pushed
the
attach-cuid-too-long
branch
from
4aab5ee
to
3cd27a6
Compare
|
That sounds good too me - as long as we get most of the container ID in
there, we shouldn't have to worry about collisions
…On Fri, Oct 26, 2018, 03:27 Giuseppe Scrivano ***@***.***> wrote:
I am really concerned about what happens during collisions - if we start a
few hundred to a thousand containers, do we start hitting this frequently?
I think we should just error out and not accept any runroot that is longer
than 40-50 characters so that we have enough room for the UUID and avoid
such kind of issues. What do you think? I can add another patch to this PR
if you like the idea.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1704 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHYHCMDl6tpELgpnsz-4ZSsozXKa8erYks5uorlygaJpZM4X1G0L>
.
|
giuseppe
force-pushed
the
attach-cuid-too-long
branch
from
3cd27a6
to
a471e85
Compare
|
/retest |
giuseppe
force-pushed
the
attach-cuid-too-long
branch
from
a471e85
to
597ebfd
Compare
| socketPath = socketPath[0:maxUnixLength] | ||
| } | ||
|
|
||
| logrus.Debug("connecting to socket ", socketPath) |
There was a problem hiding this comment.
I think that should be Debugf("... %v", socketPath).
There was a problem hiding this comment.
+1, I'm surprised gofmt or govet didn't ping on this.
|
tests are passing and I've added the additional check |
|
LGTM but I prefer to wait for @mheon's final ack. |
|
|
||
| maxUnixLength := int(C.unix_path_length()) | ||
| if maxUnixLength < len(socketPath)-1 { | ||
| socketPath = socketPath[0:maxUnixLength] |
There was a problem hiding this comment.
Do we need to subtract one from maxUnixLength here to account for a trailing null?
There was a problem hiding this comment.
thanks for catching it. No it is not needed, I've tried both versions and I am surprised it doesn't crash. I've dropped the -1 and pushed a new version
|
One question on a possible off-by-one. Could just be me seeing things that aren't there, coffee isn't working yet |
conmon creates a symlink to avoid using a too long UNIX path. Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1641800 There is still one issue when the path length of the symlink has the same length of the attach socket parent directory since conmon fails to create the symlink, but that must be addressed in conmon first. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
attach-cuid-too-long
branch
from
597ebfd
to
c65b359
Compare
|
LGTM, @mheon you're it |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
conmon creates a symlink to avoid using a too long UNIX path.
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1641800
There is still one issue when the path length of the symlink has the
same length of the attach socket parent directory since conmon fails
to create the symlink, but that must be addressed in conmon first.
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com