<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 11, 2017 at 2:23 PM, Jeff Ortel <span dir="ltr"><<a href="mailto:jortel@redhat.com" target="_blank">jortel@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On 07/11/2017 12:27 PM, Dennis Kliban wrote:<br>
<span class="gmail-">> On Tue, Jul 11, 2017 at 1:20 PM, Brian Bouterse <<a href="mailto:bbouters@redhat.com">bbouters@redhat.com</a> <mailto:<a href="mailto:bbouters@redhat.com">bbouters@redhat.com</a>>> wrote:<br>
><br>
>     We should not raise a validation exception because due to semver we can't stop raising that exception<br>
>     later. Specifically if we want to ever allow double checksum being specified in the future.<br>
><br>
>     For the MVP, I think the choices are: only respect sha256 and ignore the rest OR do as many standard<br>
>     validators as they specify. I'm ok with either of these with a slight preference for validating as many of<br>
>     them as are specified would be the best. Since we're already handling all of the data at save time,<br>
>     pushing it through additional digest validators will cost a bit of cpu but not much additional I/O. Also<br>
>     if the user asked us to do that validation then having it demand some additional resources is OK. Also<br>
>     having feature parity with the changesets is good. The changesets can validate all standard digest<br>
>     validators so having uploads do the same would be consistent.<br>
><br>
><br>
> I am reversing what I had previously said, and I agree that we should not raise an exception if a user<br>
> provides more than one checksum at upload time. My current implementation checks every checksum specified by<br>
> the user.<br>
><br>
><br>
<br>
</span>I don't think I understand the purpose of an upload user specifying more than 1 algorithm/digest for the<br>
platform to validate the integrity of an uploaded file.  Why would the user specify an algorithm/digest they<br>
don't trust?<br></blockquote><div><br></div><div>There isn't a good reason for a user to specify multiple digests especially since sha512 is a significantly stronger statistical claim than others we support. If a user did want to do it (even because they could) I don't think we should put in effort to stop them. I was thinking if they ask us to validate 3 types, we can just do that blindly and reject if any of them fail. Also we should be pre-computing all digests.<br><br></div><div>I think both sync and upload should pre-compute all the checksums on each the Artifact fields [0]. This was a feature we had to add later for pulp_rpm for Pulp2. We've had use cases where plugin code wants to publish different publishes with different checksums, especially in cases where repositories are consumed by EL5 systems which don't support sha256 which was the default checksum. With that in mind, I believe both Artifact upload and ChangeSets should be pre-computing all checksums automatically. @jortel, do the ChangeSets do that currently?<br><br>[0]: <a href="https://github.com/pulp/pulp/blob/3.0-dev/platform/pulpcore/app/models/content.py#L104-L109">https://github.com/pulp/pulp/blob/3.0-dev/platform/pulpcore/app/models/content.py#L104-L109</a><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-"><br>
><br>
><br>
>     On Mon, Jul 10, 2017 at 5:11 PM, Jeff Ortel <<a href="mailto:jortel@redhat.com">jortel@redhat.com</a> <mailto:<a href="mailto:jortel@redhat.com">jortel@redhat.com</a>>> wrote:<br>
><br>
><br>
><br>
>         On 07/10/2017 02:36 PM, Dennis Kliban wrote:<br>
>         > On Mon, Jul 10, 2017 at 3:26 PM, Michael Hrivnak <<a href="mailto:mhrivnak@redhat.com">mhrivnak@redhat.com</a> <mailto:<a href="mailto:mhrivnak@redhat.com">mhrivnak@redhat.com</a>><br>
</span>>         <mailto:<a href="mailto:mhrivnak@redhat.com">mhrivnak@redhat.com</a> <mailto:<a href="mailto:mhrivnak@redhat.com">mhrivnak@redhat.com</a>>>> wrote:<br>
>         ><br>
>         ><br>
>         ><br>
>         >     On Mon, Jul 10, 2017 at 3:06 PM, Dennis Kliban <<a href="mailto:dkliban@redhat.com">dkliban@redhat.com</a> <mailto:<a href="mailto:dkliban@redhat.com">dkliban@redhat.com</a>> <mailto:<a href="mailto:dkliban@redhat.com">dkliban@redhat.com</a><br>
<div><div class="gmail-h5">>         <mailto:<a href="mailto:dkliban@redhat.com">dkliban@redhat.com</a>>>> wrote:<br>
>         ><br>
>         >         The upload API for Artifacts is going to allow users to specify the artifact size and a digest. The<br>
>         >         Artifact model currently supports  'md5', 'sha1', 'sha224', 'sha256', 'sha384', and 'sha512' digests.<br>
>         ><br>
>         >         Do we want to let users specify more than one digest per upload? e.g. md5 and sha256?<br>
>         ><br>
>         ><br>
>         >     There may be no harm in this, but it would add complexity to the verification and not add much value. I'd<br>
>         >     stick with just one unless there's a compelling reason for multiple.<br>
>         ><br>
>         ><br>
>         > I agree. The API is going to raise a validation exception when more than 1 digest is provided.<br>
><br>
>         +1<br>
><br>
>         ><br>
>         ><br>
>         ><br>
>         ><br>
>         ><br>
>         >         Do we want to store all 6 digests for each Artifact?<br>
>         ><br>
>         ><br>
>         >     The expensive part of calculating the digests is reading the file. As long as you're already reading the<br>
>         >     entire file, which we will during verification, you may as well stuff the bits through multiple hashers<br>
>         >     (digesters?) and get all the digests. Pulp 2 has a function that does this:<br>
>         ><br>
>         >     <a href="https://github.com/pulp/pulp/blob/2.13-release/server/pulp/server/util.py#L327-L353" rel="noreferrer" target="_blank">https://github.com/pulp/pulp/<wbr>blob/2.13-release/server/pulp/<wbr>server/util.py#L327-L353</a><br>
>         <<a href="https://github.com/pulp/pulp/blob/2.13-release/server/pulp/server/util.py#L327-L353" rel="noreferrer" target="_blank">https://github.com/pulp/pulp/<wbr>blob/2.13-release/server/pulp/<wbr>server/util.py#L327-L353</a>><br>
>         >     <<a href="https://github.com/pulp/pulp/blob/2.13-release/server/pulp/server/util.py#L327-L353" rel="noreferrer" target="_blank">https://github.com/pulp/pulp/<wbr>blob/2.13-release/server/pulp/<wbr>server/util.py#L327-L353</a><br>
>         <<a href="https://github.com/pulp/pulp/blob/2.13-release/server/pulp/server/util.py#L327-L353" rel="noreferrer" target="_blank">https://github.com/pulp/pulp/<wbr>blob/2.13-release/server/pulp/<wbr>server/util.py#L327-L353</a>>><br>
>         ><br>
>         >     But we can't always guarantee that we'll have all the checksums available, for at least two reasons. 1) If<br>
>         >     in the future if we want to use yet another algorithm, we probably won't want to run a migration that<br>
>         >     re-reads every file and calculates the additional digest. 2) For on-demand content, we don't have it<br>
>         >     locally, so we can't calculate any additional checksums until it gets fetched.<br>
>         ><br>
>         >     So this may be one of those times where we use a good-ole-fashioned getter method that returns the<br>
>         >     requested digest if it's on the artifact, calculates it if not, or raises an exception if the value isn't<br>
>         >     available and can't be calculated.<br>
>         ><br>
>         ><br>
>         > For uploaded Artifacts, all of the digests will be calculated as the file is being processed during the<br>
>         > upload. So I don't think calculating all of them should incur significantly more cost than just one. The code<br>
>         > snippet from Pulp 2 looks similar to what I am doing.<br>
><br>
>         This functionality should be a method on the Artifact and not a util function somewhere.<br>
><br>
>         ><br>
>         > I haven't given much thought to the getter, but your idea sounds fine to me.<br>
>         > Thanks,<br>
>         > Dennis<br>
>         ><br>
>         ><br>
>         ><br>
>         ><br>
>         >     --<br>
>         ><br>
>         >     Michael Hrivnak<br>
>         ><br>
>         >     Principal Software Engineer, RHCE<br>
>         ><br>
>         >     Red Hat<br>
>         ><br>
>         ><br>
>         ><br>
>         ><br>
>         > ______________________________<wbr>_________________<br>
>         > Pulp-dev mailing list<br>
</div></div>>         > <a href="mailto:Pulp-dev@redhat.com">Pulp-dev@redhat.com</a> <mailto:<a href="mailto:Pulp-dev@redhat.com">Pulp-dev@redhat.com</a>><br>
>         > <a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/<wbr>mailman/listinfo/pulp-dev</a> <<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/<wbr>mailman/listinfo/pulp-dev</a>><br>
>         ><br>
><br>
><br>
>         ______________________________<wbr>_________________<br>
>         Pulp-dev mailing list<br>
>         <a href="mailto:Pulp-dev@redhat.com">Pulp-dev@redhat.com</a> <mailto:<a href="mailto:Pulp-dev@redhat.com">Pulp-dev@redhat.com</a>><br>
>         <a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/<wbr>mailman/listinfo/pulp-dev</a> <<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/<wbr>mailman/listinfo/pulp-dev</a>><br>
><br>
><br>
><br>
>     ______________________________<wbr>_________________<br>
>     Pulp-dev mailing list<br>
>     <a href="mailto:Pulp-dev@redhat.com">Pulp-dev@redhat.com</a> <mailto:<a href="mailto:Pulp-dev@redhat.com">Pulp-dev@redhat.com</a>><br>
>     <a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/<wbr>mailman/listinfo/pulp-dev</a> <<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/<wbr>mailman/listinfo/pulp-dev</a>><br>
><br>
><br>
<br>
</blockquote></div><br></div></div>