releng/tickets

Fork 2

Fix incorrectly created epel10 branch for bluechi package #12457

New issue

Open

opened 2024-11-13 14:26:37 +00:00 by markkemel · 14 comments

markkemel commented

2024-11-13 14:26:37 +00:00

Describe the issue

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

* Describe the issue 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

carlwgeorge commented

2024-11-13 18:00:18 +00:00

Member

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

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

markkemel commented

2024-11-13 20:21:16 +00:00

Author

@carlwgeorge
I have successfully run the build.
Thanks a lot!

@carlwgeorge I have successfully run the build. Thanks a lot!

phsmoura commented

2024-11-13 20:26:08 +00:00

Metadata Update from @phsmoura:

Issue tagged with: medium-gain, medium-trouble

**Metadata Update from @phsmoura**: - Issue tagged with: medium-gain, medium-trouble

dherrera commented

2024-11-14 15:21:25 +00:00

Contributor

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'

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' ```

dherrera commented

2024-11-19 20:00:47 +00:00

Contributor

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

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

kevin commented

2024-11-19 21:40:47 +00:00

Owner

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: 1. we need the scm-requests-processor to be exempted, so it can... make the branches. 2. 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?

dherrera commented

2024-11-19 22:44:07 +00:00

Contributor

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.

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?

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 two possible issues with that: > > 1. 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. > 2. 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? > 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

zlopez commented

2024-11-20 09:43:38 +00:00

Member

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.

> The two possible issues with that: > > 1. 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.

churchyard commented

2024-11-20 11:21:05 +00:00

Member

When this happens, should we instruct packagers to retry with fedpkg request-branch --no-git-branch?

When this happens, should we instruct packagers to retry with `fedpkg request-branch --no-git-branch`?

kevin commented

2024-11-22 19:25:35 +00:00

Owner

When this happens, should we instruct packagers to retry with fedpkg request-branch --no-git-branch?

I think that might work.

note that we need to fix the message around that entire flow:

fedpkg still mentions that it updates pdc, but not git if you pass that
scm commits processor toddler says:

                "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.

> When this happens, should we instruct packagers to retry with `fedpkg request-branch --no-git-branch`? I think that might work. note that we need to fix the message around that entire flow: * fedpkg still mentions that it updates pdc, but not git if you pass that * scm commits processor toddler says: ``` "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.

dherrera commented

2024-11-25 18:33:53 +00:00

Contributor

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

> > The two possible issues with that: > > > > 1. 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

carlwgeorge commented

2024-11-26 05:59:13 +00:00

Member

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

As I understand it, without PDC we are now relying on two data points:

Is there a non-archive bodhi release with that branch?
Does the dist-git repo have a deadpackage file?

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.

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). As I understand it, without PDC we are now relying on two data points: - Is there a non-archive bodhi release with that branch? - Does the dist-git repo have a deadpackage file? 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.

dherrera commented

2024-11-26 18:37:33 +00:00

Contributor

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.

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

carlwgeorge commented

2024-11-27 05:28:58 +00:00

Member

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.

Me and @dherrera were chatting about this, and he found that forgejo has a feature called [repository flags](https://codeberg.org/forgejo/forgejo/pulls/2079). 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.