cli-plugins/socket: remove use of deprecated distribution uuid package #4872
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thaJeztah
added
status/2-code-review
kind/refactor
thaJeztah
force-pushed
the
use_non_deprecated_uuid
branch
from
ebd60c0 to
6a18d84
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4872 +/- ##
=======================================
Coverage 61.31% 61.32%
=======================================
Files 287 287
Lines 20058 20063 +5
=======================================
+ Hits 12298 12303 +5
Misses 6867 6867
Partials 893 893 |
Member
Author
|
@krissetto @laurazard ptal 🤗 |
thaJeztah
commented
Feb 13, 2024
thaJeztah
force-pushed
the
use_non_deprecated_uuid
branch
from
6a18d84 to
f4fff92
Compare
thaJeztah
changed the title
Member
Author
|
Updated; I'll squash later if we all agree 👍 |
Benehiko
approved these changes
Feb 13, 2024
Member
There was a problem hiding this comment.
Maybe we could add a test to prove collisions aren't possible for certain number of calls? I know it uses crypto/rand, so it shouldn't have a problem, but might be good to have a test so that this behavior doesn't change in future.
Other than that it LGTM :)
Member
Author
|
Pushed another option to just use a local utility, based on the |
Member
Author
|
(I'lll squash commits if we agree, so don't merge yet |
Collaborator
|
I'm happy with us defining a local utility for this! Performance shouldn't be a huge concern as long as we don't start taking multiple milliseconds since this should generally only happen once per plugin invocation, and randomness isn't worrying either (it really only serves to let us come up with something that hopefully doesn't collide with any other currently opened socket). |
krissetto
approved these changes
Feb 13, 2024
vvoland
approved these changes
Feb 13, 2024
Collaborator
vvoland
left a comment
vvoland
left a comment
There was a problem hiding this comment.
LGTM; I really wouldn't worry about collisions here. It's 32 bytes (2^256) - I'm pretty sure it won't be a problem on a properly working system for any human being ever 😄
thaJeztah
force-pushed
the
use_non_deprecated_uuid
branch
from
ea173d3 to
2e1478c
Compare
thaJeztah
changed the title
Member
Author
|
Thanks all! I squashed the commits; we can merge once CI finishes |
vvoland
requested changes
Feb 13, 2024
Collaborator
vvoland
left a comment
vvoland
left a comment
There was a problem hiding this comment.
Ahh I missed the failure:
- FAIL: TestSetupConn (0.00s)
--- FAIL: TestSetupConn/updates_conn_when_connected (0.00s)
socket_test.go:20: assertion failed: error is not nil: listen unix /var/folders/gh/0cww6vn93nv_jjsw3xkm0b4m0000gn/T/docker_cli_44b21e358063d1d8813046be2cb6c105391492639decad3d90af0f5fd193f090: bind: invalid argument
--- FAIL: TestSetupConn/allows_reconnects (0.00s)
socket_test.go:34: assertion failed: error is not nil: listen unix /var/folders/gh/0cww6vn93nv_jjsw3xkm0b4m0000gn/T/docker_cli_25007de380ec8be00ed832285a3304c4170b547c2d87f4431145867e23f5bb52: bind: invalid argument
--- FAIL: TestSetupConn/does_not_leak_sockets_to_local_directory (0.00s)
socket_test.go:51: assertion failed: error is not nil: listen unix /var/folders/gh/0cww6vn93nv_jjsw3xkm0b4m0000gn/T/docker_cli_ac4e8a31d2b46b186de29d235c13f52cecca70a2f8fa23e85c3f0e227a67f5ae: bind: invalid argument
--- FAIL: TestConnectAndWait (0.00s)
--- FAIL: TestConnectAndWait/calls_cancel_func_on_EOF (0.00s)
socket_test.go:83: assertion failed: error is not nil: listen unix /var/folders/gh/0cww6vn93nv_jjsw3xkm0b4m0000gn/T/docker_cli_ae5b44d10a06f5d2bef1e114be4c0bf1bc94426d9c72c8694a1b97505497ed10: bind: invalid argument: failed to setup listener
--- FAIL: TestConnectAndWait/connect_goroutine_exits_after_EOF (0.00s)
socket_test.go:106: assertion failed: error is not nil: listen unix /var/folders/gh/0cww6vn93nv_jjsw3xkm0b4m0000gn/T/docker_cli_bc4bec58d28c315fd4558146986a442e99262fe7f9d381e7bc56345d88621dbb: bind: invalid argument: failed to setup listener
FAIL
I think the filename is too long now
Member
Author
|
Oh! Probably yes. I also missed that it failed 😞 Let me have a look |
The "github.com/docker/distribution" module moved to the distribution
org ("github.com/docker/distribution/v3"), and the new module deprecated
and removed the uuid package in favor of Google's UUID package.
While we still depend on the old module through packages and as an indirect
dependency, we may want to try avoid using it.
This patch replaces the use for the socket package, and replaces it for a
local utility, taking the same approach as `stringid.GenerateRandomID()`,
which should be random enough for this purpose.
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
use_non_deprecated_uuid
branch
from
2e1478c to
3b5e814
Compare
thaJeztah
commented
Feb 13, 2024
| } | ||
|
|
||
| func randomID() string { | ||
| b := make([]byte, 16) |
Member
Author
There was a problem hiding this comment.
changed to 16 bytes; which is the same as the UUID (output is slightly shorter as it doesn't add hyphen in between;
before: 332becff-7147-44cc-ac72-8ab20eac060e
after: 68298c4c32a19c2b1cc636649c465d84
Member
Author
|
Hm... one more failure; what's this? |
Member
Author
|
Hmm.. other failure now. I have a feeling we may have some flakiness (don't immediately see how this PR would change anything there 🤔) |
Collaborator
|
Oh, that's weird. I wonder if we broke something, I don't think the |
Member
Author
|
Yeah, that one struck me as weird as well. At least it's not the path length this time ( That said, I know the macOS check is a bit flaky on its own (tends to fail on Looks like race-detector found some issues, but haven't looked at them yet output below; |
Collaborator
|
Looks like we lack synchronization around the cli/cli-plugins/socket/socket.go Lines 38 to 48 in 3b5e814 and also access it in tests: cli/cli-plugins/socket/socket_test.go Line 128 in 3b5e814 It might be only a tests issue though, since the overwrite should happen only once because we only expect one connection to the socket? |
vvoland
approved these changes
Feb 14, 2024
Collaborator
vvoland
left a comment
vvoland
left a comment
There was a problem hiding this comment.
Anyway, it's definitely unrelated to this PR, so this one LGTM.
Member
Author
Yes, was looking at that code Yesterday, and wondering if that should terminate the loop if it returns an error. Looks like we currently never terminate the loop.. I just tried running these tests in a loop every 0.5 seconds for ~15 minutes (so ~ 1767 times), and didn't get a failure, so for now I blame some general flakiness for those above. Let's bring this one in. But perhaps someone should have a look at the above things. |
Collaborator
|
I think originally the idea was to explicitly allow plugins to redial the socket (for whatever end), but that doesn't sound super important right now and we can introduce it later if we want to. I'm okay with either explicitly only allowing one connection (terminate the loop after error) or protect access to cc @Benehiko if you're looking to dig in to something |
Member
Author
|
Yeah, I wasn't 100% sure yet for the best approach there. If were successfully able to connect, it would block, but on any error it would fall through, and call the |
Member
|
TL;DR The problem is that the I don't have enough context of why the var conn *net.UnixConn
// sets up an inifinte loop in the background
listener, err := SetupConn(&conn)
// poll the conn for non nill
pollConnNotNil(t, conn) // <-- can succeed or fail here
// closes the connection
conn.Close() // <-- can succeed or fail here// Close closes the connection.
func (c *conn) Close() error {
if !c.ok() {
return syscall.EINVAL
}
err := c.fd.Close() // <--- fails
if err != nil {
err = &OpError{Op: "close", Net: c.fd.net, Source: c.fd.laddr, Addr: c.fd.raddr, Err: err}
}
return err
}go test -v -race .
---
WARNING: DATA RACE
Read at 0x00c00013322c by goroutine 16:
internal/poll.(*FD).Close()
/usr/lib/go/src/internal/poll/fd_unix.go:112 +0x98
net.(*netFD).Close()
/usr/lib/go/src/net/fd_posix.go:37 +0x3d
net.(*conn).Close()
/usr/lib/go/src/net/net.go:203 +0x6a
github.com/docker/cli/cli-plugins/socket.TestConnectAndWait.func1()
/home/benehiko/go/src/github.com/docker/cli/cli-plugins/socket/socket_test.go:107 +0x2dd
testing.tRunner()
/usr/lib/go/src/testing/testing.go:1595 +0x261
testing.(*T).Run.func1()
/usr/lib/go/src/testing/testing.go:1648 +0x44Solutions:
A solution i can commit now is to wrap // UnixConnSafe is a wrapper around a UnixConn
// that makes it concurrency-safe.
type UnixConnSafe struct {
*net.UnixConn
SocketLock *sync.Mutex
}
func NewUnixConnSafe() *UnixConnSafe {
return &UnixConnSafe{
UnixConn: &net.UnixConn{},
SocketLock: &sync.Mutex{},
}
}
func (c *UnixConnSafe) Close() error {
c.SocketLock.Lock()
defer c.SocketLock.Unlock()
if c.UnixConn == nil {
return nil
}
return c.UnixConn.Close()
}
func (c *UnixConnSafe) IsNil() bool {
c.SocketLock.Lock()
defer c.SocketLock.Unlock()
return c.UnixConn == nil
} |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
relates to:
cli-plugins/socket: remove use of deprecated distribution uuid package
The "github.com/docker/distribution" module moved to the distribution
org ("github.com/docker/distribution/v3"), and the new module deprecated
and removed the uuid package in favor of Google's UUID package.
While we still depend on the old module through packages and as an indirect
dependency, we may want to try avoid using it.
This patch replaces the use for the socket package, and replaces it for a
local utility, taking the same approach as
stringid.GenerateRandomID(),which should be random enough for this purpose.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)