store: replace namesgenerator with neutral builder IDs#3813
store: replace namesgenerator with neutral builder IDs#3813crazy-max wants to merge 1 commit intodocker:masterfrom
Conversation
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
|
|
||
| const generatedNamePrefix = "builder_" | ||
|
|
||
| var newID = identity.NewID |
There was a problem hiding this comment.
Wondering if we need the complexity of identity here; would something like this give enough randomness?
import (
"math/rand/v2"
"encoding/hex"
)
func NewID() string {
var b [8]byte // 64 bits
rand.Read(b[:])
return hex.EncodeToString(b[:]) // 16 hex chars
}There was a problem hiding this comment.
Oh; and I just recalled I did something similar for k8s some time ago;
buildx/driver/kubernetes/util/generate.go
Lines 56 to 75 in d5d3d3d
|
|
||
| var namePattern = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9\.\-_]*$`) | ||
|
|
||
| const generatedNamePrefix = "builder_" |
There was a problem hiding this comment.
As we're changing, might as well use - instead of _ (which should produce a more correct hostname for the container if there's ever a need for that).
There was a problem hiding this comment.
I used builder_ intentionally because Buildx already derives runtime names through the buildx_buildkit_ prefix, so underscores are already part of the existing naming convention here. And it's a bit easier to read and copy/paste consistently alongside the derived node and runtime names.
| "github.com/pkg/errors" | ||
| ) | ||
|
|
||
| var namePattern = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9\.\-_]*$`) |
There was a problem hiding this comment.
Not for this PR probably , but looks like we have some different patterns in different packages;
Lines 40 to 41 in 09f288c
I know these regexes are rather memory-hungry; we could even rewrite these to a basic function to loop; I recall we did similar changes elsewhere, e.g.; https://github.com/moby/moby/blob/dff719e3674958407416fd1d8a35db998f128da2/daemon/internal/stringid/stringid.go#L62-L70
This removes the remaining
github.com/docker/dockerdependency from Buildx by replacing the oldnamesgeneratorusage in the store with neutral autogenerated builder names. The new format usesbuilder_plus a shortidentity.NewID()suffix, which keeps generated names predictable enough for current Buildx behavior without carrying the daemon-style human-readable naming scheme.The existing driver-side runtime name derivation is unchanged, so autogenerated builders still flow through the current
buildx_buildkit_prefixing path for derived node and container names. In practice that means a builder such asbuilder_abc123def456still becomesbuilder_abc123def4560at the node level andbuildx_buildkit_builder_abc123def4560at the runtime object level, with Kubernetes continuing to normalize underscores to hyphens when deriving deployment names.This is the smaller follow-up to the earlier work in #3354. The in-tree human-readable replacement there was not the right long-term direction. The concern raised in the earlier discussion was not just the dependency itself, but also that Buildx should not keep copying the daemon's naming style for builder identities. Using a neutral generated identifier keeps this change scoped to dependency removal, without coupling it to a broader redesign of builder presentation.
This PR intentionally doesn't try to solve the separate TOCTOU and ownership issues discussed in #3354 and the linked follow-ups. Buildx still reserves a builder name in the local store first and later derives node and container names from it, so there is still a window where conflicting objects can appear between reservation and container creation even though this is more unlikely with this change. So that needs explicit follow-up work around ownership labeling and verification before reuse or removal, and it should be handled separately.