oci/cas/cas/dir: Drop supported-algorithm check in blobPath

[wking]

I don't see a point to having a local preference distinct from the go-digest maintainer's suggestion.

I've dropped the supported check in dir's blobPath, because:

• PutBlob is calling Digester before it calls blobPath, and Digester will have already panicked on an unavailable algorithm.
• GetBlob will no longer error on usupported algorithms, but it should be up to the GetBlob consumer performing the validation to decide if the algorithm is supported or not. I don't see why dir needs to have an opinion on this.
• DeleteBlob will no longer error on unsupported algorithms. But if the caller asked us to delete a blob, I think we should delete it regardless of whether we can hash the blob contents.

Spun off from here.

[wking]

The Travis failure looks like a network hiccup. I expect this PR will pass if you kick the tests again.

[cyphar]

I'm not really sure about using digest.Canonical (and whether it solves an actual problem). I would much prefer that we support arbitrary hashing algorithms rather than switching from SHA-256 to SHA-256 in the form of "Canonical" (maybe that makes sense for the default? I'm not sure, because I might want to change it in the future irrespective of what Steven thinks the best algorithm is). At the moment, the only two explicitly supported hashes are SHA-256 and SHA-512.

[...] but it should be up to the GetBlob consumer performing the validation to decide if the algorithm is supported or not. I don't see why dir needs to have an opinion on this. [...]
[...] But if the caller asked us to delete a blob, I think we should delete it regardless of whether we can hash the blob contents. [...]

Yeah, this is because umoci doesn't have support for different types of blob hashes. The main reason for this is because I wasn't sure how exactly the UI should look for choosing a hash, and also there are some other validation questions that come up as well in that discussion. But as I said above, I would prefer if we solved the entire "mixed hashes" issue in one go.

[wking]

On Wed, Oct 18, 2017 at 04:24:12PM -0700, Aleksa Sarai wrote: I'm not sure, because I might want to change it in the future irrespective of what Steven thinks the best algorithm is).

In that case you may want to have a distinct-from-go-digest opinion. But you vendor go-digest. Maybe you want this change, and you can always drag BlobAlgorithm back out if go-digest does something crazy.

Yeah, this is because `umoci` doesn't have support for different types of blob hashes. The main reason for this is because I wasn't sure how exactly the UI should look for choosing a hash…

Get and delete don't seem like they'd need algo-choosing UIs. Put might support that; see wking/casengine@b1ec59028 for one potential API (although you'd have to decide how to expose that downstream).

[cyphar]

@wking

Get and delete don't seem like they'd need algo-choosing UIs. Put
might support that; see wking/casengine@b1ec590 for one potential
API (although you'd have to decide how to expose that downstream).

Without having support in Put for choosing hashes (and code in mutate that makes a decision on whether to use the previous hash, our default, or something a user picked) then the current way umoci will handle mixed hashes doesn't feel quite right -- which is what I was referring to. This /could/ be done by having a Configure() method in the cas interface which lets you set things like that, or by having some metadata stored in the net.Context...

[wking]

On Tue, Oct 31, 2017 at 01:13:40AM +0000, Aleksa Sarai wrote: > Get and delete don't seem like they'd need algo-choosing UIs. Put > might support that; see ***@***.*** for one potential > API (although you'd have to decide how to expose that downstream). Without having support in `Put` for choosing hashes (and code in `mutate` that makes a decision on whether to use the previous hash, our default, or something a user picked) then the current way `umoci` will handle mixed hashes doesn't feel quite right…

I still don't see a major difference between a locally-defined constant and a vendored constant, and filling in a configurable algo-choosing framework is more than I want to bite off at the moment. So I've rerolled this PR with 0c33ca1 → c1fab82 to include only the blobPath change.

[cyphar]

@wking I'm not sure you understood what I was saying. What I was saying is that currently we error out if we have an image that has a "mixed hashes" setup (more than one hash in use, or using a hash that we don't use by default). I agree with you that this is not correct, and needs to be fixed.

However, the fix cannot just be removing the error condition -- now umoci will act quite weirdly when interacting with "mixed hash" images. All Puts will use our built-in default (whether that's go-digest.Canonical or cas.BlobAlgorithm doesn't really matter here). But if we're replacing a manifest in the middle of DescriptorPath then we're going to end up changing all of the algorithms used up to the root. If a user has explicilty used a different hashing algorithm we've overridden their decision.

[wking]

If a user has explicilty used a different hashing algorithm we've overridden their decision.

Good point. But I think "the new blobs we push use our preferred algo, even though you may have preferred a different one" is still friendlier than erroring out with "sorry, we don't deal with that algorithm". Adding configurable put algos can happen in follow-up work.

[cyphar]

This is still something we should do, but this PR is quite stale and as I mentioned sigh three years ago this would require some more work to make sense.