GenerateRandomID has a check to verify if the generated ID was numeric. This
check was added because a container's short-ID is used as default hostname for
containers, which isn't allowed to be consisting of only numbers (see [moby#3869]
and https://bugzilla.redhat.com/show_bug.cgi?id=1059122.
Producing an random ID with only numbers is a rare corner-case, but the check
would always be executed and wasn't optimized.
This patch applies some optimizations:
- The code was using `strconv.ParseUInt`, which has additional checks for
signs ("+" or "-"); `hex.EncodeToString` would never produce these, so
we can use `strconv.ParseInt` instead (which doesn't have these checks).
- The code was using `TruncateID(id)` to get the short-ID. The `TruncateID`
function is designed to also handle digests, and for that checks for
the given ID to contain colons (`:`), which it would split to remove
the algorithm (`sha256:`) before truncating to the short-ID length.
That check wasn't needed either, because those would not be produced
by `hex.EncodeToString`, so instead, we can just truncate the ID.
- Finally, all we _really_ need to check for is if the ID consists of only
numeric characters (`0-9`) so, let's do just that; if any non-numeric
value is found, the ID is valid, and we can terminate the loop.
I did some basic benchmark to compare all of the above in isolation;
- BenchmarkParseInt: `strconv.ParseInt(TruncateID(id), 10, 64)`
- BenchmarkParseUInt: `strconv.ParseUint(TruncateID(id), 10, 64)`
- BenchmarkParseUIntNoTrunc: `strconv.ParseUint(id[:shortLen], 10, 64)`
- BenchmarkAllNum: `allNum(id[:shortLen])`
Results of the above:
BenchmarkParseInt-10 1713937 691.0 ns/op 480 B/op 18 allocs/op
BenchmarkParseIntNoTrunc-10 3385483 356.1 ns/op 480 B/op 18 allocs/op
BenchmarkParseUInt-10 2112538 567.7 ns/op 384 B/op 12 allocs/op
BenchmarkParseUIntNoTrunc-10 4325847 266.7 ns/op 384 B/op 12 allocs/op
BenchmarkAllNum-10 77277264 15.29 ns/op 0 B/op 0 allocs/op
Difference for `GenerateRandomID` as a whole is less dramatic, as in most
cases `ParseInt` would bail out early, but still saves some allocations, and
performance is ~14% better:
BenchmarkGenerateRandomID-10 2807764 424.5 ns/op 240 B/op 6 allocs/op
BenchmarkGenerateRandomIDNew-10 3288866 366.6 ns/op 160 B/op 3 allocs/op
[moby#3869]: https://github.com/moby/moby/issues/3869
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These tests were named confusingly as they're testing `TruncateID`.
While renaming, let's also combine them into a single test using
a test-table, so that the test-cases can carry some description
what they're testing.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- `IsShortID` was deprecated in 2100a70741
- `ValidateID` was deprecated in e19e6cf7f4
Both are part of 27.0, so we can remove these.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This slightly simplifies TruncateID() by only
trimming the string if needed.
Also improved unit-tests for this package;
- Add a test for GenerateNonCryptoID()
- Add a test for shortening a sha-256 ID
- Make TestShortenId() more "unit", by using a fixed string, instead of calling GenerateRandomID()
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
As reported in #11294, the Docker daemon will execute contains it
shouldn't run in the event that a requested tag is present in an image's
ID. This leads to the wrong image being started up silently.
This change reduces the risk of such a collision by using the short ID
iff the actual revOrTag looks like a short ID (not that it necessarily
is).
Signed-off-by: Richard Burnison <rburnison@ebay.com>
