Add foreign key constraint here, please.

mutable field shouldn't be the default value (replace with None and set it in function instead) + checksum_types would be better here if we expect list

typo

More informative would be something like "There is no rpm {nvrea} signed with {sigkey}". I think that "No cached signature" is a bit misleading.

There is no need to error here - just return with noop?

We want to compute checksum for signed rpm itself, not for its content (it is not even possible to get rpm file data this way)

It works, but for consistency with other records it could be in one line rpm_id INTEGER NOT NULL REFERENCES rpminfo(id)

These can be reverted to original state. There is no change in the method, it is just pulled out of original space (listRPMFiles), same for get_rpm_file

Is there any reason to publish this method via API?

This is not necessary - we want to have default types in config file, not code.

pull it from config

any iterable is ok, so at least (list, tuple)

sighash is not used for anything, it could be replaced with no columns and countOnly.

same nvra could be used in previous error message when there is only rpm_id

checksum_types are now strings. In the db are integers, unify usage (either convert to ints now or expects ints on input. sigkey is completely missing in clauses. Furthermore, next step checks only equivalent sets. In case some of hashes already exist it will fail during the insert.

It could be a bit easier without list encapsulation
chsum_list = {chsum: getattr(hashlib, chsum)() for chsum in checksum_type}

chunk name is used in similar context throughout the code

@tkopecek all fixed

+
CREATE INDEX rpm_checksum_rpm_id ON rpm_checksum(id);
UNIQUE(rpm_id, sigkey, checksum_type)

quotes should not be there + typo in name

checksum=chsum.hexdigest()

mutable list can't be in default values (use None) instead + config says that default is md5+sha256, not sha256

typo

typo

checksum_type_int = [koji.CHECKSUM_TYPES[chsum] for chsum in checksum_types]

There could be subset available.

you should call write_signed_rpm instead of following code. It would create signed/* data, so it could be computed. Otherwise it would be failing in most cases as GC will delete those copies.

typo

generally "s/RPMDefaultCHecksums/RPMDefaultChecksums/g"

s/repo/rpm

It should be renamed everywhere as it is always list -> missing_checksum_types

isinstance(checksum_types, (list, tuple))

if chtype not in koji.CHECKSUM_TYPES

it could be simplifed via set()

koji.CHECKSUM_TYPES[func]

s/friandly/friendly

drop this cycle ^

There are a few places we need to clear this data:

• delete_build
• reset_build
• delete_rpm_sig

The rpm_checksum table doesn't currently have a primary key. It would make sense to use (rpm_id, sigkey, checksum_type)

The exception for unsigned rpms seems unnecessary and inconsistent. The call is not looking for a specific key, but for any key. If there are no keys, then a {} result is accurate.

The koji get* calls are generally tolerant unless they are explicitly passed a strict arg. This one should be too (not that I'm suggesting that we add a strict mode here).

I had hoped for more efficient use of IO in the write_signed_rpm hook. We already loop over the new files contents in koji.splice_rpm_sighdr. We could add a callback there to enable the checksum. This is admittedly more complex, but without it we're re-reading every signed copy after writing it. I thought I'd made this more explicit, but looking back I see I only alluded to a few places.

Apologies for the delay. There's a bit more complexity here than I expected.

1) splice changes

The splice_rpm_sighdr callback changes are not quite what I envisioned, and I don't think we need to expose so much specific data to that function. I made some changes here that are more along the lines of what I was thinking.

https://pagure.io/fork/mikem/koji/commits/pr3628updates

The first commit there, "simplify splice_rpm_sighdr changes", is kind of what I had in mind initially. The second commit takes things a bit further and bears some explanation (see below).

Also, note that the previous callback changes were not capturing the full file contents, but only the part after the signature. This would result in incorrect sums. This is fixed in the above changes.

2) unauthenticated writes

The getRPMChecksums export is an unauthenticated call that can potentially write out signed copies to disc. I don't think we should allow this so freely. The writeSignedRPM call requires special access.

There are a couple ways we might address this. One is to calculate the checksums without writing the copy. This is what led to my second commit above ("generator for reading spliced rpm signatures"). Splitting that logic out of the original splice function should make it fairly easy to generate a checksum for a signed rpm without actually performing the writes.

Alternately, we could simply fail when we have neither cached sums or signed copies. Which leads me to...

3) strict option

Above I write that I was not suggesting adding a strict option for getRPMChecksums. There might be reason to, but I think it bears more thought. However, I do not think that the case where an rpm has no signatures should ever result in an error. In that case, there's a definitive answer to the query and that answer is {}.

That said, the situation above suggests a situation where a strict option might make sense -- whether to raise an error when we have the signature, but neither a cached checksum nor a signed copy. In the non-strict case we'd report the missing checksums as None. This would make it easier for the client to distinguish which sum is missing.

4) Some smaller things.

The variable named chsum_list is not a list, which is unnecessarily confusing.

Seems a little odd to use BulkInsert for such a small number of inserts.

The create_rpm_checksum function has signature *kwargs, which seems unnecessarily generic. We only call it twice, and with the same sort of parameters each time.

The create_rpm_checksum function modifies mutable parameters that are passed to it. This sort of hidden side effect can cause issues that are very hard to debug.

The getRPMChecksums function is appended at the end of the exports class, but it would probably make more sense to have it near the other exports that relate to rpm signatures. E.g. somewhere near writeSignedRPM.

With rewriting generator to IOStream it could be more usable? We can unify behaviour to deal with it as with signed copy in all places except writing the signed copy (based on https://gist.github.com/mechanical-snail/7688353, not clear what is the license there). Not tested.

def spliced_sig_reader(path, sighdr, bufsize=8192):
    class Stream(io.RawIOBase):
        def __init__(self, path, sighdr):
            self.path = path
            self.sighdr = sighdr
            self.buf = None
            self.gen = self.generator()
        def generator(self):
            (start, size) = find_rpm_sighdr(self.path)
            with open(path, 'rb') as fo:
                # the part before the signature
                yield fo.read(start)
                # the spliced signature
                yield sighdr
                # skip original signature
                fo.seek(size, 1)
                # the part after the signature
                while True:
                    buf = fo.read(bufsize)
                    if not buf:
                        break
                    yield buf
        def readable(self):
            return True
        def readinto(self, b):
            try:
                expected_buf_size = len(b)
                data = self.buf or next(self.gen)
                output = data[:expected_buf_size]
                self.buf = data[expected_buf_size:]
                b[:len(output)] = output
                return len(output)
            except StopIteration:
                return 0    # indicate EOF
    return io.BufferedReader(Stream(path, sighdr), buffer_size=bufsize)

We should avoid os.path.join in the hub code for safety reasons. We have koji.joinpath for this now.

The BufferedReader implementation is neat, but the class declaration should be moved out of the function (and given a clearer name). Embedding the class definition like this creates a new class each time the function is called.

With the code updates that automatically splice and sum when the signed copy is missing, the strict option doesn't really have much meaning separate from (anti-)cacheonly. Better and simpler to just drop the strict option.

The builddir calculation is incorrect.

        builddir = koji.pathinfo.build(rpm_info)

The rpm info cannot stand in for the build info. The NVR could be completely different.

I have addressed some of these on my branch. I've also added some unit tests for the splicing code.
https://pagure.io/fork/mikem/koji/commits/pr3628updates

self.checksums[name] = checksum.hexdigest()

Having this function alter the structure of a key instance variable seems fragile and unnecessary. This could could instead return an altered copy and calling code could use the result of that call rather than accessing checksums.

def spliced_sig_reader(path, sighdr, bufsize=8192):
    """A generator that yields the contents of an rpm with signature spliced in"""

minor, but the docstring here is now inaccurate. Should be something like: "returns a file-like object whose contents have the new signature spliced in"
Sorry, meant to note that before.

Your commit message only mentions the class move, but the other changes are also notable.

Oh! One more thing.

In create_rpm_checksum we filter out the checksums that we already have in the db. We should probably take the opportunity to check that the values we're ignoring actually match what is in the db. A mismatch here is a "should not happen" situation, so we should raise an error.

Sorry, I keep finding more small things :)

The deepcopy in create_rpm_checksum is not needed at this point, since the data will be a name:digest dictionary. A simple .copy() should suffice.

The error in create_rpm_checksum should indicate the rpm id, sigkey, and checksum type so that this can be debugged if it ever happens.

The checksum_types keyword arg for write_signed_rpm is never used in the current version of the PR. I think we can drop it and just use the default.

Ok, I think we're done

:thumbsup:

Thanks for all the revisions!

Commit 6f068f16 fixes this pull-request