Skip to content
This repository was archived by the owner on Feb 24, 2020. It is now read-only.

store: add support for multiple hashAlgorithms.#2479

Closed
sgotti wants to merge 1 commit intorkt:masterfrom
sgotti:store_hashalgorithms
Closed

store: add support for multiple hashAlgorithms.#2479
sgotti wants to merge 1 commit intorkt:masterfrom
sgotti:store_hashalgorithms

Conversation

@sgotti
Copy link
Contributor

@sgotti sgotti commented Apr 21, 2016

I'm not sure how in future multiple image formats (like to OCI image spec) will be implemented.
Based on #2313 my initial idea was to use different kind of stores (for example ACIStore and OCIStore) since they have different peculiarities and logics.

This patch is the first part of a store refactor to support multiple kind of stores and adds support for multiple hash algorithms.

/cc @philips

store: add support for multiple hashAlgorithms.

Actually the store just uses sha512 hash keys (half sized) since the
appc/spec only uses this kind of hash.

This patch adds a hashAlgorithm interface and implements two
hash algorithms (the current sha512 with half sized keys like the one
currently used, and a sha256) and converts the store to use this
interface.

This will be useful for future implementations where multiple hash types
should be supported (like OCI image digests).

Related to #2313

@sgotti sgotti force-pushed the store_hashalgorithms branch from 8bcebd3 to 83048b9 Compare April 21, 2016 12:42
Actually the store just uses sha512 hash keys (half sized) since the
appc/spec only uses this kind of hash.

This patch adds a hashAlgorithm interface and implements two
hash algorithms (the current sha512 with half sized keys like the one
currently used, and a sha256) and converts the store to use this
interface.

This will be useful for future implementations where multiple hash types
should be supported (like OCI image digests).

Related to rkt#2313
@sgotti sgotti force-pushed the store_hashalgorithms branch from 83048b9 to 78d85e2 Compare April 21, 2016 12:52
"strings"
)

type HashAlgorithm interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a top-level description of this interface?

@s-urbaniak
Copy link
Contributor

Thanks! There's a couple godoc's missing (even when they're an interface implementation I think it's worth godoc'ing them". I am a bit worried about the reflect call.

@sgotti
Copy link
Contributor Author

sgotti commented Apr 29, 2016

@s-urbaniak Thanks for the review, I'll be quite busy in the next days, but I'm working on it and I'm also trying to make it handle multiple prefix separators (for example the OCI image spec uses a : instead of a - in their digests).

About the reflection I don't think it'll have a big performance impact, another solution will be to define different hash types wrapping the hash.Hash interface. I tried this but it'll clash with func (s *Store) HashToKey(h hash.Hash) string { since it cannot be changed as it's needed by the acirenderer).

@s-urbaniak
Copy link
Contributor

@sgotti no problem, I think documenting the reasoning behind the reflection should be OK.

@lucab
Copy link
Member

lucab commented Jun 8, 2016

Visited for triaging. I'm not sure about the status of this and I know it will be tangled among other OCI rework, but I guess this may be given some priority at this point.

@s-urbaniak can you please shepherd this in in together with your other OCI stuff?

@sgotti I tentatively mile-stoned this for next 1.9. Feel free to provide us an estimated schedule on your side, and we'll try to plan this accordingly.

@s-urbaniak
Copy link
Contributor

ack 👍

On Wed, Jun 8, 2016 at 11:02 AM, Luca Bruno [email protected]
wrote:

Visited for triaging. I'm not sure about the status of this and I know it
will be tangled among other OCI rework, but I guess this may be given some
priority at this point.

@s-urbaniak https://github.com/s-urbaniak can you please shepherd this
in in together with your other OCI stuff?

@sgotti https://github.com/sgotti I tentatively mile-stoned this for
next 1.9. Feel free to provide us an estimated schedule on your side, and
we'll try to plan this accordingly.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2479 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAW8MLqd8GcJMNrYQm9Mw_kJHg5ToSSwks5qJoUPgaJpZM4IMot1
.

@sgotti
Copy link
Contributor Author

sgotti commented Jun 15, 2016

@lucab sorry for the delay. It was an attempt to generalize multiple hash algorithms for future OCI image spec. I'll prefer to sync with @s-urbaniak on the work on oci image spec implementation with rkt before going ahead with this (see also #2541)

@tmrts
Copy link
Contributor

tmrts commented Jun 21, 2016

Moving it to 1.11 milestone

@tmrts tmrts modified the milestones: v1.11.0, v1.9.0 Jun 21, 2016
@iaguis iaguis modified the milestones: v1+, v1.11.0 Jul 20, 2016
@iaguis
Copy link
Member

iaguis commented Jul 20, 2016

@sgotti or @s-urbaniak can you reassess?

@s-urbaniak
Copy link
Contributor

xref'ing issues #2925 #2541 and PRs #2953 #2919 as they all fall in the bucket "oci", tagged accordingly to "area/oci image"

@sgotti
Copy link
Contributor Author

sgotti commented Jul 21, 2016

The new store I'm working on needs a different but also simpler way to support multiple Hash algorithms. I think this could be closed.

@s-urbaniak
Copy link
Contributor

@sgotti as per request, closing this then

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants