Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go-maintainers missing permissions #982

Closed
Aneurysm9 opened this issue Feb 16, 2022 · 19 comments
Closed

go-maintainers missing permissions #982

Aneurysm9 opened this issue Feb 16, 2022 · 19 comments

Comments

@Aneurysm9
Copy link
Member

The @open-telemetry/go-maintainers team is missing admin permissions on the opentelemetry-go repo. We have had these permissions in the past and require them to appropriately maintain our repository.

Can we have the admin permissions restored and an explanation of who removed them, when, and for what reason?

@tigrannajaryan
Copy link
Member

I am not sure about the "who removed" part, but I think the policy currently says that maintainers do not normally have admin permissions: https://github.com/open-telemetry/community/blob/main/docs/how-to-configure-new-repository.md#permissions

I think this was also discussed once in the past (granting admin permissions to maintainers) but I can't remember the outcome of that decision.

@tigrannajaryan
Copy link
Member

Here, found the discussion: #616

@bogdandrutu
Copy link
Member

Based on the audit log this was done in Dec 2021.

Screen Shot 2022-02-16 at 10 45 58 AM

The reason is very simple, since as TC members we are responsible to respect the rules and maintain the organization, and the rules are here:

The team foo-maintainers has Maintain permissions for the repository.

There is no exception/request documented that go maintainers need special set of permissions.

@bogdandrutu
Copy link
Member

I also updated lots of other places in the past, unfortunately the audit log is deleted after 6months, or at least I could not find it.

Screen Shot 2022-02-16 at 10 50 12 AM

@bogdandrutu
Copy link
Member

Other TC members did too

Screen Shot 2022-02-16 at 10 51 17 AM

@dyladan
Copy link
Member

dyladan commented Feb 16, 2022

@Aneurysm9 what admin permission is required? I'm not sure if it is possible to grant more permissions to the maintain role or not but maybe that's an option?

edit: looks like its not possible

@bogdandrutu
Copy link
Member

@dyladan we discussed this in the GC, and I told you folks (2w ago if I remember correctly the time) that github allows us to define new "roles" with custom permissions, so we can define a "super-maintainer" role with more permissions if we identify that is needed. Also we can then update the community rules to give "super-maintainer" role to our maintainers.

@dyladan this issue is not a real concern issue, since they did not need these permissions. It is just @Aneurysm9 trying to denigrate my reputation one more time, see here, they did not actually realize that they miss any permission.

@Aneurysm9
Copy link
Member Author

Among the permissions we require is the ability to manage the CI checks required on protected branches. This is a permissions that was explicitly called out in the original issue discussing changing maintainer permissions. As it is a permission that we do not frequently exercise I had not noted the permissions change in the last two months. That does not change the fact that the permissions are required and appropriate.

@bogdandrutu I did not mention you at all and I do not appreciate you continuing to impute to me motivations that I do not have. Continued "insulting or derogatory comments, and personal ... attacks" will not be tolerated.

@bogdandrutu
Copy link
Member

In the issue linked:

It will give some additional permissions like setting up CI hooks

CI hooks are different than branch protection rules which you are referring to. I am not sure whether the right to change branch protection is or not required for the maintainers, but I would be happy to discuss that in the GC/TC meeting.

@SergeyKanzhelev
Copy link
Member

I definitely supportive to give as much permissions to repo maintainers as needed to control things independently. I listed a few reasons in the similar ticket #616 on why we went with this model https://github.com/open-telemetry/community/blob/main/docs/how-to-configure-new-repository.md#permissions. Also a note that unfortunately even "Maintain" permissions is not sufficient to allow managing CI checks.

So far we were solving the shortcomings of our tooling and GitHub permission model limitations by granting individuals permission (mostly temporary) and helping with configuration via the tickets in this repo. This is burdensome for both - maintainers and TC members.

We also discussed the need for dedicated GitHub admins group here: #923 (comment) that may be focused on fixing this and other tooling problems. However I am not aware of anybody volunteered to create and drive this group so far. Even for the large project as k8s with a lot of tooling, the group managing GH admin things is quite small.

All that said, this particular issue may be just a misunderstanding and I hope it can be resolved.

@Aneurysm9 is there any immediate issue that needs resolution wrt CI and branch protection set up?

@bogdandrutu it is a good idea to re-iterate on this again. Maybe we can request some CNCF support/budgeting to solve the mentioned issues and it will allow us to make maintainers Admins again. If this will be discussed at GC meeting, let me know if any input from me is needed.

@Aneurysm9
Copy link
Member Author

Without the ability to edit protected branches we do not have the ability to manage the CI checks that are required on protected branches. This is not immediately required, AFAIK, but will in the near future with the upcoming release of Go 1.18.

@SergeyKanzhelev
Copy link
Member

Without the ability to edit protected branches we do not have the ability to manage the CI checks that are required on protected branches. This is not immediately required, AFAIK, but will in the near future with the upcoming release of Go 1.18.

ok, let's keep the issue open. If this switch it planned soon, let me know if we need to make somebody as an admin for the repo for now.

I also have added it to the GC meeting agenda: https://docs.google.com/document/d/1-23Sf7-xZK3OL5Ogv2pK0NP9YotlSa0PKU9bvvtQwp8/edit# @bogdandrutu should I put your name as a topic owner?

This is a long standing issue and I hope we can resolve it one way or another. Thank you for bringing it up.

@dyladan
Copy link
Member

dyladan commented Feb 16, 2022

I would suggest to close this issue since the title, description, and early discussion were about go maintainers missing admin permission which is resolved by the fact that the documentation clearly states they don't have those permissions.

I would create a new issue requesting an additional permission for maintainers of a SIG.

@anuraaga
Copy link
Contributor

Not sure if this is the right issue for it but given there will be a GC session about it, just some notes.

Just confirming that this org is actually an enterprise cloud account, so has support for custom roles as it doesn't seem to be available to normal organizations.

Looking through the documented permissions (I don't have an enterprise account to verify the actual permissions UI)

https://docs.github.com/en/enterprise-cloud@latest/organizations/managing-peoples-access-to-your-organization-with-roles/managing-custom-repository-roles-for-an-organization#additional-permissions-for-custom-roles

While I may be misinterpreting some of the permission descriptions, what seem to be notable omissions are

  • Manage branch protection rules. Just updating the OS's or e.g. Java versions targeted by CI is enough to cause PRs to become unmergable without being able to quickly update this to match CI modifications
  • Secret management. Most credentials such as publishing credentials are owned by maintainers, they definitely need to have enough control to rotate credentials for example

On the bright side, "Delete an issue", which IIRC was a gap between Maintain and Admin, does seem to be there. From my reading, if the above two gaps could also be solved then a custom role could be a viable alternative to Admin but without it, it seems to be needed for reasonable maintenance of a repo.

@SergeyKanzhelev
Copy link
Member

Looked at custom roles. These are only permissions that are allowed beyond the "Maintain" role:

Delete an issue
Issue

Manage webhooks
Repository

Manage deploy keys
Repository

Delete code scanning alerts
Security

View secret scanning alerts
Security

Dismiss or reopen secret scanning alerts
Security

View Dependabot alerts
Security

Dismiss Dependabot alerts
Security

I created a custom role in the organization with all these permissions and tested it with @anuraaga. Unfortunately this role doesn't allow to do CI modification:

image

So custom roles would not work as a solution, at least for now.

@mtwo
Copy link
Member

mtwo commented Feb 17, 2022

We discussed this in the GC call today. Our general preference is to restrict permissions to the smallest necessary set, however given @SergeyKanzhelev's testing, this doesn't appear to be possible. As such, if this permission is truly needed by maintainers (the GC doesn't have the context to weigh in on this) then it seems to follow that they should have their permissions changed to admin.

@dyladan will follow up, and some TC folks should probably weigh in as well (they may have more stronger opinions than the GC on this topic).

@bogdandrutu
Copy link
Member

@anuraaga your examples are great, but would like to challenge a bit here:

Manage branch protection rules. Just updating the OS's or e.g. Java versions targeted by CI is enough to cause PRs to become unmergable without being able to quickly update this to match CI modifications

Changing the versions "targeted by CI" may be a significant change (sometimes even a breaking change) that may be better to require more thinking before doing it. I do agree that this is something that we should be able to do, but also it does not happen that often and may be something we can automate.

Secret management. Most credentials such as publishing credentials are owned by maintainers, they definitely need to have enough control to rotate credentials for example

This is definitely something that we need to fix. I believe secrets should be rotated/managed at org level and have org level accounts (shared with every TC/GC members). This is a single point of failure, the fact that if @anuraaga wants today can move away from the project and we cannot release any otel-java lib.

@anuraaga
Copy link
Contributor

but also it does not happen that often and may be something we can automate.

If there is some automation that isn't too complicated for updating the required checks that should be fine.

This is a single point of failure, the fact that if @anuraaga wants today can move away from the project and we cannot release any otel-java lib.

I agree if org secrets provided all the ones we need, there wouldn't be a need at the repo level. Not sure how practical it is to set that up - for reference, we have Maven, Gradle, AWS, and GPG secrets.

But want to clarify that in terms of java publishing it's not really a single point of failure since multiple maintainers have Maven access, so just a matter of swapping the secret. An org secret might have more risk of being a single point.

@trask
Copy link
Member

trask commented Dec 16, 2022

I think this issue can be closed now that #1311 is merged.

https://github.com/open-telemetry/community/blob/main/docs/how-to-configure-new-repository.md#collaborators-and-teams

If requested, foo-maintainers will be granted Admin permissions, and in return they must document any changes they make to the repository settings in a file named .github/repository-settings.md in their repository (other than temporarily
disabling "Do not allow bypassing the above settings", see branch protection rules below).

@open-telemetry/go-maintainers if you would still like Admin permissions, please open a new "Repo Maintenance" community issue requesting it.

@trask trask closed this as completed Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants