Learn more about these different git repos.
Other Git URLs
No commits found
Fixes: https://pagure.io/koji/issue/2665
rebased onto 34ef79a0be8d6719ec78f755effcd7f0720b36eb
logger.warning("File: %s has been deleted", path) maybe?
logger.warning("File: %s has been deleted", path)
Deleting all data silently for the rpm when sigkey is None might be ok for API deleteRPMSig, but it might be too tolerant for a CLI command? I think it is better to add an option like --all in this case
sigkey is None
deleteRPMSig
--all
rebased onto 3f4d76cf1abdc5c145695d188ccf877c757db7bd
@julian8628 fixed
:thumbsup:
A few design things:
Deleting data is a big deal in Koji, and use of this call is not expected to be routine. It should only be needed when there is a mistake. As such, I don't think that the tag permission is enough here. For a start, let's make this admin only.
tag
Similarly, I'm concerned about having the API default to deleting all signatures. Perhaps a separate option, or a different sentinel value than None.
None
...to be continued...
This call is necessarily constructing a delete query, because we don't have a special mechanism for that like we do with SELECT and UPDATE (because there are so few DELETEs).
When constructing a query like this, we must be extremely vigilant. This query does not use a safe method of passing in parameters.
clauses = ["rpm_id=%(id)i" % rinfo]
This is using (unsafe) python string expansion, rather than the safer expansion built into the db library. Granted, in this case the value comes from a get_rpm call, however it is still unwise to use string expansion if we don't have to. It makes it that much easier for a future bug to turn into a security issue.
get_rpm
This is a rather critical point, and anyone reviewing changes that involve queries like this needs to consider such things in the review.
Minor things:
I'm not sure it makes sense to warn on missing signed copies, as this is a common and expected case. Perhaps just .info()? I guess it does make sense to warn on missing cached signature headers, however.
.info()
Wording:
"There is nothing for delete" -> "{rpm} has no matching signatures to delete"
"Delete all RPMs related to specified RPM" -> "Delete all signed copies for specified RPM"
"Sigkey or --all option must be specified" -> "Either --sigkey or --all options must be given"
"Only one of options sigkey or all must be specified" -> "Conflicting options specified"
It seems like there is duplication in the path removal. We're both saving the koji.pathinfo.sighdr and koji.pathinfo.signed paths as we generate them, but later we are constructing the same paths, data/sigcache/X and data/signed/X, manually. I believe this can be simplified.
koji.pathinfo.sighdr
koji.pathinfo.signed
data/sigcache/X
data/signed/X
At the end, we are trying to remove the top level sigcache, signed, and data directories. If they are empty. I get the desire for cleanup, but I think this is unnecessary and could error in a race with a parallel import-sig. We can probably just leave this out for simplicity.
sigcache
signed
data
We might want to reconsider the case where we fail to delete and re-raise. File operations are not transactional like the db operations, so when this happens, we might have an rpm with signatures still listed in the db, but no longer cached on disk. That is a data corruption scenario, so we should probably warn about that a little louder.
I'm not sure what the best thing here is, but we should at least provide more information, probably a scarier exception message, plus perhaps additional error logging for the admins.
It occurs to me that we also don't have an api call for the simpler+safer action of deleting just signed copies. The prune-signed-copies command relies on rw fs access for this.
prune-signed-copies
It might be nice to separate out the deletion of the signed copies, and make this a separate api call (that one could be governed by the sign perm). This call could call out to that one first, and this would have the helpful effect of putting the least impactful file deletions first. This way, for at least some I/O failures, we'd avoid creating a mismatch between rpmsigs and the sigcache files because we'd error on a signed copy first.
We could also (not necessarily required here) eventually adjust the prune-signed-copies command to use this call, allowing it to be run on systems without rw access to the fs.
rebased onto d6d81fae2b503ecfe96c03b8f8e853f60b65be70
@mikem all your comments are fixed, instead of last comment. I guess, it should be better to create new ticket for last comment and work on it in separate issue. What do you think?
rebased onto fc17803b44cb15b3f2209182812f3f7b0464526a
rebased onto f7b4ab2e43b5c50a56b774ed87a5d7c6c621a813
rebased onto ca38247b1ce6f0cf7756f370b6d3dc7335a73507
rebased onto 6c7e8778b64ea5abb3c895f6b024f4df5a6539da
The name all is a python builtin. We should use another name for the option. Perhaps all_sigs?
all
all_sigs
I don't know if the api call needs to duplicate the sanity checks of the cli. Perhaps this would be simpler:
if all_sigs: sigkey = None elif not sigkey raise koji.GenericError("No signature specified")
Returning the paths is interesting, but I don't think we want to do that right now. Paths are tricky data, since the path on the hub may not be meaningful on the client. It's probably best to have this function simply not have a return.
The error handling doesn't seem quite right. When a hub call errors, Koji rolls back the db transaction. So, if an error is raised midway in this function, we're still going to have the signatures listed in the db, but we may have already deleted the sigcache files for them.
The error message under if not rpm_query_result needs to be adjusted. Also, that variable should probably be named nvra.
if not rpm_query_result
I don't think I mentioned this last time, but this PR doesn't really include most unit test coverage for the new hub function. The only case is a trivial early failure case.
rebased onto e05b9a8686998059e8478fd4ca1e2b447842446f
rebased onto 9b65c0290b350d10002eb1122bfa704ff75418f0
rebased onto 0d86e82cd12233077719bab1bcc09cca58f2265b
you can use
with tempfile.TemporaryDirectory() as temp_dir: ....
here
this line could be removed if you use with above
with
when an error happens, you could only move the files have been moved to tmp_dir back. No need to move all files back.
tmp_dir
Does it look a typo? should be list_tmps
list_tmps
rebased onto e7ec5f478e604fb3a32801a29e799cbd651b73ec
rebased onto 239f7077a10c72f0501cf50da83c3a6e5147e923
rebased onto a58bf02a39b18c9f6a1cbf5c2ff8d8550754f560
rebased onto d10299214706949ccf6162f519b5f63c3436ccc2
rebased onto 8f19263898ba82eee2cfe372afdfe005394d47a3
rebased onto 4731a023600e2ed839dcd94778172c2e7200ad56
Maybe be a bit more readable? ``nvra = "%(name)s-%(version)s-%(release)s-%(arch)" % rinfo
logger.error("xxx: %s", argument)
dst needs to be a dir here - otherwise it raise FileNotFoundError.
FileNotFoundError
rebased onto bd03c8c780dfe54d61075dc1c7264a4af58185a4
rebased onto 16f80c7641e0fd3409d5938b762eb5c05d478ecd
rebased onto 12035fadc410675e6bf5f92ecb5aafa3e2c0138e
rebased onto 5fb5e99d12e89af6a6e2cabd5efd3f335a1256f8
rebased onto 5d44b33e6190f51354408c54c64847ba734d93bc
rebased onto 6f07d42b56e3cda6ba74ff1828e102791c7b4937
rebased onto 822b435bce9dc59b09ab1401e4caab4bf53f6440
all fixed
@mikem Current implementation tries to delete empty arch and signature directories if they are empty. I'm hesitating if it is a balanced level of import race condition risk or if we should leave all directories in place (It could be a lot of additional directories if distro is e.g. revoking some compromised sigkey - anyway in such case it will be probably double-checked by some manual script).
arch
signature
The tempdir approach feels wrong here. While we may want to eventually look into some sort of acidfs approach for our file operations, a fragmentary implementation for one call doesn't make sense to me. Furthermore, this has us (most likely) moving (potentially very large) files back and forth across file systems when we don't have to.
I don't think that we can fully avoid leaving things inconsistent in the case of an I/O error and I didn't mean to suggest that we should, or could, make the fs operations transactional. I merely wanted us to:
Current implementation tries to delete empty arch and signature directories if they are empty.
I commented on similar behavior in a previous version of the PR. I think it's fine to try to remove the first layer of basedirs (i.e. the directories whose name is the signature key), but that second layer (sigcache and signed) doesn't really need to be dealt with here given the race potential (e.g. this call happening while another call is importing a different signature).
Metadata Update from @tkopecek: - Pull-request tagged with: testing-ready
Metadata Update from @tkopecek: - Pull-request untagged with: testing-ready
rebased onto cbf2fad7ab594671acca258c59a0d1e18fc4149b
Fixed
rebased onto 26744d4
Small bug with path recreation. I've updated it in #2962
Final updated (and tested) version in #2965.
Pull-Request has been closed by jcupova
Fixes: https://pagure.io/koji/issue/2665