-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement dynamic hash registration #62
Conversation
Made one small fix around the comment from @stevvooe. I think I fulfilled the proposed ask. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff
algorithm.go
Outdated
algorithmsLock.Lock() | ||
defer algorithmsLock.Unlock() | ||
|
||
if _, ok := algorithms[algorithm]; ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We made the standard keys lowercase. Perhaps we should force the same here of all keys (and comment as such) so that "SHA256" doesn't confuse with "sha256"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vbatts Should I add a validator based on the semantic here? https://github.com/opencontainers/image-spec/blob/v1.0.1/descriptor.md#digests
This adds a new interface to go-digest allowing for it to act as a registry for various hash implementations. It includes the new "CryptoHash" interface which hashers must implement. By default the SHA variants that were historically available are available. The cryptoHash interface mimics crypto.Hash, but is a subset of the methods that we require. crypto.Hash is a concrete type that makes it hard to bring new hash function implementations in. The primary impetus is to allow for out-of-tree hash implementations to be added. For example, if someone wanted to add the out-of-tree BLAKE3 AVX implementation, they could do that. Right now, if two versions of the same implementation are added, the one that is added first will take precedence, but in the future, we can add the ability to force registration. Signed-off-by: Sargun Dhillon <sargun@sargun.me>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This adds a new interface to go-digest allowing for it to act as a registry
for various hash implementations. It includes the new "CryptoHash" interface
which hashers must implement. By default the SHA variants that were
historically available are available.
The cryptoHash interface mimics crypto.Hash, but is a subset of the methods
that we require. crypto.Hash is a concrete type that makes it hard to bring
new hash function implementations in.
The primary impetus is to allow for out-of-tree hash implementations to be
added. For example, if someone wanted to add the out-of-tree BLAKE3 AVX
implementation, they could do that. Right now, if two versions of the same
implementation are added, the one that is added first will take precedence,
but in the future, we can add the ability to force registration.
Signed-off-by: Sargun Dhillon sargun@sargun.me