Switch from Pagure to Forge for blocker review discussions #296
Labels
No labels
Closed As
Duplicate
Closed As
Fixed
Closed As
Invalid
discussions
easyfix
enhancement
task
ai-review-please
Backlog Status
Needs Review
Backlog Status
Ready
chore
documentation
points
01
points
02
points
03
points
05
points
08
points
13
pr2jira
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Sprint Status
Blocked
Sprint Status
Done
Sprint Status
In Progress
Sprint Status
Review
Sprint Status
To Do
Technical Debt
Work Item
Bug
Work Item
Epic
Work Item
Spike
Work Item
Task
Work Item
User Story
No milestone
No project
3 participants
Notifications
Due date
No due date set.
Depends on
#299 Blockerbot for Forge
quality/blockerbugs
#13250 Quality Blockerbugs app secret storage update
infra/tickets
#3195 quality: Update blockerbugs app env variables
infra/ansible
Reference
quality/blockerbugs#296
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Currently we support only Pagure for doing async blocker review discussions. For production, we use the blocker-review repo.
Since we're migrating from Pagure to Forge, we would need a change in blockerbugs that would drop Pagure support and introduce Forge support for blocker discussions.
This means:
a) implementing reading, creating and editing a Forge ticket through its API
b) figuring out how to get notified when a Forge ticket is updated (in Pagure, we do this through a webhook)
c) figuring out authentication with Forge, and testing for privileged members
d) fixing ticket formatting if needed
Don't forget that most of the changes can be tested through staging instances - https://stg.pagure.io , https://bugzilla.stage.redhat.com and https://forge.stg.fedoraproject.org .
Implementation checklist:
fix/296-blocker-review-voting-on-forgeFORGEJO_prefix inconfig.py, there is also newDISCUSSION_SYSTEMoption, which needs to be changed from "pagure" to "forgejo"FORGEJO_ADMIN_TEAM(unfortunate name, needs refactoring :) )Some thoughts during my quick inspection today:
Also, let's create a PR so that I can put some direct comments on particular lines?
I was just playing it safe and keeping backwards compatibility with Pagure just in case. If that is not really required we can remove Pagure specific code for sure.
Let's create PR after possible Actions upgrade is evaluated (#298) since that would change required code considerably.
I wouldn't delay this with that ticket. It might take a long time to evaluate it and it might be a complete rewrite of the process. We can be much quicker if we finalize this and push to production, because you have it almost all written already, as it seems. We can consider #298 as a possible future improvement.
In our today's team call, we agreed that it makes no sense to carry forward Pagure integration as well. Jaroslav, could you please adjust the PR to drop Pagure support? It should make some code paths simpler. Thanks!
Ack, will do.
TODO: In Forgejo 15.0 access tokens can now be limited to just specific repositories. It might be a good idea to limit the blockerbot access just to the blocker-review repo. (Once Forgejo 15.0 is deployed in Fedora).
the staging deployment should be working and ready for testing now.
We've hit a snag with checking admin vote commands privileges (AGREED, REVOTE). Previously, we checked membership in a particular Pagure group, and those members could issue admin vote commands. We tried to replicate the same approach in Forge, where we have two Quality teams - Owners (admin access to the organization and all our repos), and Members (admin access to all our repos). The idea is that Quality Members can issue admin vote commands (in the blocker-review repo).
At the same time, I'm trying to have the bot (blockerbot) just minimal access itself (both interactive and API), so that an attacker can't misuse it much if it gains access to it.
But it's not easy to have both at the same time. Current constraints:
Our current options (that we're aware of):
A) Give blockerbot Quality Members membership, use one token with rw:issues and read:org. This is the easiest, but least safe. No more code changes needed, but both the account and the API token can be misused widely.
B) Give blockerbot Quality Members membership, use two tokens, one with read:org, and the second with rw:issues just for the blocker-review repo. This is a bit safer, the API tokens can't be misused (outside of the blocker-review repo). The full account hijack still allows everything.
C) Create a specific new Quality team just for blockerbot, with very restricted access. We just need blockerbot to be a part of Quality, it doesn't need to have write permissions anywhere (just blocker-review repo is enough). We can have a special-tailored team just for it. This would be ideal, but it's not self-service. We would need to ask Fedora Infra to set up two new FAS groups (one is the primary source of truth, the second just clones the members and is used for Forge - I have no idea why they do it this way, but they do), create the Forge group, and set up the connections. Because we might have some needs for future adjustments, I feel a bit bad for bothering the Infra team every time.
D) Don't give blockerbot Quality membership, check global Quality membership for admin vote command privileges. If we don't require to check a particular Quality team, but we rely just on arbitrary Quality membership (i.e. any team, as long as it's inside Quality), we don't need blockerbot to be a part of Quality, all the troubles from above are gone. The downside is, obviously, that anyone inside Quality can issue admin vote commands. Currently that's not a problem at all, because Members is used exactly for that, and there's no other team (except Owners). But if we wanted, in the future, create some other team, with limited access, for some specific group of people, all of those would be able to issue admin vote commands as well. Please note that I'm currently not aware of any specific use case we might want to achieve by having another team (if we want to give elevated access to a specific repo to a specific person, that can be done inside that very repo through Collaborators feature, it doesn't need handling through org teams). So perhaps this is just a theoretical concern. But if we decide to go this way, it will require code changes if we ever encounter this situation and want to create another team.
E) Check FAS directly. Instead of fighting Forge and its tokens and memberships, we can query a FAS group directly to see its members. The Forge teams are directly mapped from FAS anyway. But this would require a new code (with some form of authentication, because FAS account/group info can't be accessed publicly), and we don't know how difficult this can be.
F) Keep the admin list defined in Blockerbugs directly. Similarly to Testdays, in the admin interface we could define a list of FAS users eligible for admin vote commands. It's quite a bit of work, and increases maintenance, though.
There might be more approaches that didn't occur to us. Do you see any other?
@adamwill @jgroman Do you have any preference for these options?
For me, the technically best is C), but it complicates the setup and creates more work for Infra. I'd love if this (and any future changes) were in our hands instead. So I might suggest D), because it feels good enough for now, and should be a very easy change. E) might be also easy, we just don't know.
In ideal conditions I would also prefer C (dedicated team) but as not to burden infra team with extra work I'd say let's do D (just query Quality org public members).
BTW: I suspect E (querying FAS) would require Kerberos auth.
I'd also prefer D or E. We've had extremely loose permissions for relatively unimportant things before - like the blockerbugs admin interface having no auth on it at all, and anonymous editing in the wiki "Test Results" space - and it has not been a problem. Any abuse of membership would be pretty visible, too (anyone subscribed to the ticket would see the inappropriate action).
I guess we could also note that the admin commands in blockerbugs don't currently really mean much...and the 'real' status change action - which is to put a string in the Whiteboard field in Bugzilla - is also loosely-constrained. There are lots and lots of people who can set the Whiteboard field on any bug in BZ, and of course the user who reported any blocker can also set it. So far I can't think of a single case of it being done maliciously.
We went with D) and it seems to be working.
@adamwill @psklenar @lruzicka
Folks, the (hopefully) final version is now deployed in staging. Can you please test it a bit before we move it to production?
Blockerbugs: https://qa.stg.fedoraproject.org/blockerbugs/
Bugs are retrieved from: https://bugzilla.stage.redhat.com/
Review tickets: https://forge.stg.fedoraproject.org/quality/blocker-review/issues
The latest version in stg bugzilla is 40, so use F40BetaBlocker and similar keywords when proposing any bugs. Don't worry, all emails are disabled in staging, you can do completely what you want there. If you use the Propose button in BBA, it gets added to the UI immediately, you don't need to wait for the 30 minute refresh.
Please try to propose some bugs and vote on them in the tickets, use AGREED and REVOTE etc.
If no one can see any issue, we can promote this to production. Thanks!
@adamwill Any opinion whether we should migrate all the old 2000 voting tickets from https://pagure.io/fedora-qa/blocker-review , when setting up our production Forge blocker-review repo?
Thanks!
mmm...I think we probably should give it a shot. There's valuable historic info in there. It's good to have a record of why decisions were made. If it turns out to be a real pain we could maybe live without it, though.
Alright, I'll try. (I added an implementation checklist to the first post, so that everyone can keep track of the progress).
BBA version 1.15 tagged and deployed to production.
Publicly announced here:
Infra ansible cleanup ticket:
infra/ansible#3324
I believe we can finally consider this done, yay 🎉️
This has been through many many milestone, might deserve even more story points. Changing and closing.
I guess we can open a new ticket for testing in staging and then deploying to prod?
It's already deployed to production 😆️ See the checklist in the first comment.