Hi, I was working on building a new version for bluechi, and have accidentally incorrectly created an epel10 branch (checkout -b and push instead of fedpkg request-branch). Now fedpkg request-branch returns "Requested branch "epel10" already exists. Skipping the operation.", and of course I can't delete the remote branch.
Can you help me with that? Also, maybe it would make sense to fool-proof that and reject new branch pushes of that sort?
Thanks
The main thing you're missing from doing request-branch is getting the package in the allowlist for the tag. I've manually added it so that you can build.
❯ koji list-pkgs --package bluechi --tag epel10.0 Package Tag Extra Arches Owner ----------------------- ----------------------- ---------------- --------------- bluechi epel10.0 pingou
You're right that this should be rejected, and it's configured to do so, but it doesn't seem to be working correctly.
https://pagure.io/fedora-infra/ansible/blob/main/f/roles/distgit/pagure/templates/pagure_shared.cfg#_98-106
@carlwgeorge I have successfully run the build. Thanks a lot!
Metadata Update from @phsmoura: - Issue tagged with: medium-gain, medium-trouble
Ok, I was able to reproduce this with another package. locally I get this message list
git push --set-upstream origin epel10 Total 0 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0) remote: ERROR: ['git', 'ls-tree', 'epel10', '--name-only', '--', 'dead.package'] =-- 128 remote: remote: fatal: Not a valid object name epel10 remote: remote: ERROR: ['git', 'ls-tree', 'epel10', '--name-only', '--', 'dead.package'] =-- 128 remote: remote: fatal: Not a valid object name epel10 remote: remote: Emitting a message to the fedora-messaging message bus. remote: * Publishing information for 14 commits remote: - to fedora-message remote: 2024-11-14 15:04:50,656 [WARNING] pagure.lib.notify: pagure is about to send a message that has no schemas: pagure.git.branch.creation remote: Sending to redis to log activity and send commit notification emails To ssh://pkgs.fedoraproject.org/rpms/angband * [new branch] epel10 -> epel10 branch 'epel10' set up to track 'origin/epel10'.
Which is weird, since if you try to send a epel10-test branch, it get's correctly processed by ACL
git push --set-upstream origin epel10-test Total 0 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0) remote: Unspecified ref refs/heads/epel10-test is blocked remote: Denied push for ref 'refs/heads/epel10-test' for user 'dherrera' remote: All changes have been rejected To ssh://pkgs.fedoraproject.org/rpms/angband ! [remote rejected] epel10-test -> epel10-test (pre-receive hook declined) error: failed to push some refs to 'ssh://pkgs.fedoraproject.org/rpms/angband'
After further inspection and @carlwgeorge 's help, we noticed where ACL fails to stop the branch from being created.
We were expecting for it to be stopped since EPEL branches are already being blocked by the UNSPECIFIED_BLACKLIST_RES [0], but before that, it checks if the branch is supported [1]. That check verifies that the release is created on bohdi and that the branch isn't retired [2]. The problem is that when it tries do check for retirement[3], the check fails because the branch doesn't exists (hence the ERROR message on the previous post) and marks the request as supported [4]. Since the requests is marked as supported will skip the UNSPECIFIED_BLACKLIST_RES check [5], so it ends up being accepted.
I was thinking that this could be fixed if we make that is_supported branch to return None if the branch doesn't exist. That way it will pass the supported verification, but will be evaluated against the unspecified blacklist check.
[0] https://pagure.io/fedora-infra/ansible/blob/main/f/roles/distgit/pagure/templates/pagure_shared.cfg#_98-106 [1] https://pagure.io/pagure-dist-git/blob/master/f/dist_git_auth.py#_274 [2] https://pagure.io/pagure-dist-git/blob/master/f/dist_git_auth.py#_84-141 [3] https://pagure.io/pagure-dist-git/blob/master/f/dist_git_auth.py#_140 [4] https://pagure.io/pagure-dist-git/blob/master/f/dist_git_auth.py#_78-82 [5] https://pagure.io/pagure-dist-git/blob/master/f/dist_git_auth.py#_294
[6] https://pagure.io/pagure-dist-git/blob/master/f/dist_git_auth.py#_274 [7] https://pagure.io/pagure-dist-git/blob/master/f/dist_git_auth.py#_284 [8] https://pagure.io/pagure-dist-git/blob/master/f/dist_git_auth.py#_296
The two possible issues with that:
we need the scm-requests-processor to be exempted, so it can... make the branches.
There's a 'don't do any commits for me' flag to scm-processor. Does that just push a empty branch? or does that do nothing and expect the user to push whatever they want to the branch?
The two possible issues with that: we need the scm-requests-processor to be exempted, so it can... make the branches.
There are already some exceptions in place, one is for users from the "relenggroup" group[0][1], the other is for operations marked as internal [2]. From what I checked, I don't think that the worker complies with any of the two, but I might be wrong.
It doesn't requests the branch if that flag is up [3], it just requests the repository, which should come with an empty rawhide branch.
[0] https://pagure.io/pagure-dist-git/blob/master/f/dist_git_auth.py#_252-258 [1] https://pagure.io/fedora-infra/ansible/blob/main/f/roles/distgit/pagure/templates/pagure_shared.cfg#_78-79 [2] https://pagure.io/pagure-dist-git/blob/master/f/dist_git_auth.py#_192-197
[3] https://pagure.io/fedora-infra/toddlers/blob/main/f/toddlers/utils/pagure.py#_388
The scm-request-processor is just using admin token with create_branch ACL. That token is generated for releng-bot user and that user is already in RelEng group.
create_branch
releng-bot
RelEng
I think that in this case everything should be already OK.
When this happens, should we instruct packagers to retry with fedpkg request-branch --no-git-branch?
fedpkg request-branch --no-git-branch
I think that might work.
note that we need to fix the message around that entire flow:
"Pagure is still processing " "the request, but in about 10 minutes, you may create the " "branch in Pagure using git."
which I suppose is not wrong, but if the branch already exists you don't need to create it.
The two possible issues with that: we need the scm-requests-processor to be exempted, so it can... make the branches. The scm-request-processor is just using admin token with create_branch ACL. That token is generated for releng-bot user and that user is already in RelEng group. I think that in this case everything should be already OK.
Sent a PR to pagure-dist-git to implement what I described https://pagure.io/pagure-dist-git/pull-request/168
I think this comment from @pingou in another issue helps clarify things.
In pkgdb/pdc days, this regex was combined with PDC status, as in, it would block you on two sides: from pushing non-existing or EOL branch (ie: push to an epel4 branch after epel4 had been EOL'd, or block you from pushing an epel42 branch before epel42 existed) and, iirc, it would also check the status of the branch for that package and allow/deny pushing to it Now the question is: do we still have a per package, per branch state saved somewhere? If not, could it be that we're only relying on the branch status (as in: does it exist in bodhi?)? If so, that would explain why anyone can push a epel* branch as long as that epel release exists and is active in bodhi (or wherever it's stored).
In pkgdb/pdc days, this regex was combined with PDC status, as in, it would block you on two sides: from pushing non-existing or EOL branch (ie: push to an epel4 branch after epel4 had been EOL'd, or block you from pushing an epel42 branch before epel42 existed) and, iirc, it would also check the status of the branch for that package and allow/deny pushing to it
Now the question is: do we still have a per package, per branch state saved somewhere? If not, could it be that we're only relying on the branch status (as in: does it exist in bodhi?)? If so, that would explain why anyone can push a epel* branch as long as that epel release exists and is active in bodhi (or wherever it's stored).
As I understand it, without PDC we are now relying on two data points:
Using just these two data points, we can't determine if there a branch that is active but hasn't been requested yet, which was the scenario that happened at the start of this ticket. There is no PDC to check for per-package+per-branch active status. I don't think this will be a problem for Fedora, where every non-retired package should have a branch for the current releases, but it is a problem for EPEL.
The question is, is this a problem that needs to be solved? As far as I can tell, the only thing missing when a packager pushes to the branch without a request is that the package doesn't get added to the allowlist for the koji build tag. This is straightforward to fix, as I did earlier in this issue. Hopefully it's a rare occurrence. I think packagers will realize if they don't follow the documented process then they'll be blocked creating a build until releng intervenes. Even if we did implement some kind of solution, it would probably be short lived with the upcoming migration to a new git forge.
Another thing that is missing is that pagure doesn't check if a package is allowed to have an EPEL branch. This check is only made when you request a branch... It's not this specific issue, but it's a consequence of the same problem.
But yeah, my proposal would disable users from pushing their own branch history. Even if it would stop branches for being created in cases where they shouldn't, now I'm seeing that it might be way to overkill and would trample workflows on which maintainers relay on :/.
My only worry is that, even if we delay this to be solved by the new gitforge, there will still exist the need to check on an external source of truth to see if EPEL branches can be created.
Me and @dherrera were chatting about this, and he found that forgejo has a feature called repository flags. If forgejo is selected to be the new Fedora distgit, perhaps we could use that to store which branches are authorized. Each repo would start with a flag like valid_branches/rawhide, and then if a packager requests a new branch, a new flag such as valid_branches/epel10 could be added. We'd still have to figure out how to deny pushing to branches that don't have a corresponding flag, but it would be a starting point.
valid_branches/rawhide
valid_branches/epel10
Log in to comment on this ticket.