#2946 Add delete-rpm-sig CLI and deleteRPMSig hub call
Closed 2 years ago by jcupova. Opened 2 years ago by jcupova.
jcupova/koji issue-2665  into  master

No commits found

rebased onto 34ef79a0be8d6719ec78f755effcd7f0720b36eb

2 years ago

logger.warning("File: %s has been deleted", path) maybe?

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

rebased onto 3f4d76cf1abdc5c145695d188ccf877c757db7bd

2 years ago

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.

Similarly, I'm concerned about having the API default to deleting all signatures. Perhaps a separate option, or a different sentinel value than 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.

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.

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.

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.

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.

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

2 years ago

@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

2 years ago

rebased onto f7b4ab2e43b5c50a56b774ed87a5d7c6c621a813

2 years ago

rebased onto ca38247b1ce6f0cf7756f370b6d3dc7335a73507

2 years ago

rebased onto 6c7e8778b64ea5abb3c895f6b024f4df5a6539da

2 years ago

The name all is a python builtin. We should use another name for the option. Perhaps 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.

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

2 years ago

rebased onto 9b65c0290b350d10002eb1122bfa704ff75418f0

2 years ago

rebased onto 0d86e82cd12233077719bab1bcc09cca58f2265b

2 years ago

you can use

with tempfile.TemporaryDirectory() as temp_dir:
    ....

here

this line could be removed if you use with above

when an error happens, you could only move the files have been moved to tmp_dir back. No need to move all files back.

Does it look a typo? should be list_tmps

rebased onto e7ec5f478e604fb3a32801a29e799cbd651b73ec

2 years ago

rebased onto 239f7077a10c72f0501cf50da83c3a6e5147e923

2 years ago

rebased onto a58bf02a39b18c9f6a1cbf5c2ff8d8550754f560

2 years ago

rebased onto d10299214706949ccf6162f519b5f63c3436ccc2

2 years ago

rebased onto 8f19263898ba82eee2cfe372afdfe005394d47a3

2 years ago

rebased onto 4731a023600e2ed839dcd94778172c2e7200ad56

2 years ago

Maybe be a bit more readable? ``nvra = "%(name)s-%(version)s-%(release)s-%(arch)" % rinfo

  • Just for consistency - shouldn't it be called 'remove-sig' in CLI? We don't have anything with delete prefix.
  • logger calls should use logger.error("xxx: %s", argument) syntax instead of string interpolation

dst needs to be a dir here - otherwise it raise FileNotFoundError.

rebased onto bd03c8c780dfe54d61075dc1c7264a4af58185a4

2 years ago

rebased onto 16f80c7641e0fd3409d5938b762eb5c05d478ecd

2 years ago

rebased onto 12035fadc410675e6bf5f92ecb5aafa3e2c0138e

2 years ago

rebased onto 5fb5e99d12e89af6a6e2cabd5efd3f335a1256f8

2 years ago

rebased onto 5d44b33e6190f51354408c54c64847ba734d93bc

2 years ago

rebased onto 6f07d42b56e3cda6ba74ff1828e102791c7b4937

2 years ago

rebased onto 822b435bce9dc59b09ab1401e4caab4bf53f6440

2 years ago

@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).

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:

  • minimize the chances of problematic inconsistency (e.g. by handling all the signed copies first)
  • clearly indicate to the admin when there is an inconsistency

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

2 years ago

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-ready

2 years ago

rebased onto cbf2fad7ab594671acca258c59a0d1e18fc4149b

2 years ago

rebased onto 26744d4

2 years ago

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-ready

2 years ago

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

2 years ago
Metadata