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

REQUEST: Repository maintenance on open-telemetry/opentelemetry.io #2234

Open
svrnm opened this issue Jul 23, 2024 · 21 comments
Open

REQUEST: Repository maintenance on open-telemetry/opentelemetry.io #2234

svrnm opened this issue Jul 23, 2024 · 21 comments
Assignees
Labels
area/repo-maintenance Maintenance of repos in the open-telemetry org

Comments

@svrnm
Copy link
Member

svrnm commented Jul 23, 2024

Affected Repository

https://github.com/open-telemetry/opentelemetry.io

Requested changes

Upgrade the opentelemetry bot PAT (OPENTELEMETRYBOT_GITHUB_TOKEN) to have permissions to assign issues

Purpose

Since CODEOWNERS requires teams to have write permissions on a repository, there are currently lots of groups with that permission on the otel.io repo, to reduce that requirement, we want to roll out @dyladan's componentowner workflow. To make this workflow work with github teams (like open-telemetry/docs-approvers) the workflow needs a PTA that has the permission to assign an issue. The normal GITHUB_SECRET lacks the visibility into the org for that.

See https://github.com/dyladan/component-owners?tab=readme-ov-file#using-own-access-token for more details on required settings

Expected Duration

permanently

Repository Maintainers

@open-telemetry/docs-maintainers

@svrnm svrnm added the area/repo-maintenance Maintenance of repos in the open-telemetry org label Jul 23, 2024
@jack-berg
Copy link
Member

There are multiple PATs associated with that account. Based on this doc, I assume the one we're talking about is "OpenTelemetry GitHub Org Secret".

Screenshot 2024-07-25 at 9 50 42 AM

According to this document, anyone with write access to a repository can assign issues and pull requests.

Does this mean we need to assign the bot the write:repo_hook scope? Does assigning generic write capabilities expose any vulnerabilities based on the usages of the bot?

Screenshot 2024-07-25 at 9 50 56 AM

@jack-berg jack-berg self-assigned this Jul 25, 2024
@svrnm
Copy link
Member Author

svrnm commented Jul 25, 2024

@jack-berg thanks for looking into this, the current issue is that the bot can not identify the groups, I guess it is admin:org > read:org?

More automation has that permission, can that key be assigned to opentelemetry.io repo as a secret?

@jack-berg
Copy link
Member

Yes that seems reasonable. Can you open a PR to update the OpenTelemetry Bot asset doc, indicating that it has this new scope and a brief explanation of what use cases it enables?

@svrnm
Copy link
Member Author

svrnm commented Jul 25, 2024

Yes that seems reasonable. Can you open a PR to update the OpenTelemetry Bot asset doc, indicating that it has this new scope and a brief explanation of what use cases it enables?

Will do!

svrnm added a commit to svrnm/community that referenced this issue Jul 25, 2024
@svrnm
Copy link
Member Author

svrnm commented Jul 25, 2024

After trying out a few variations of that, the issue boils down to the problem that @opentelemetrybot does not have the right permissions on the opentelemetry.io repository to request code reviews. Only if at least triage permissions are assigned to the bot, the workflow runs successfully. However this goes against the following statement in assets.md:

“Important: You do not need to (and should not) give this account any permissions to any OpenTelemetry repository.”

https://github.com/open-telemetry/community/blob/main/assets.md#opentelemetry-bot

@svrnm
Copy link
Member Author

svrnm commented Jul 25, 2024

The PR that introduced that note on not assigning permissions to the bot:

#1285 (comment)

By using component owners and not CODEOWNERS we would reduce the number of members with write permissions significantly, and replace it with the bot having triage permissions. I think that this would be better.

cc @arminru @trask

@trask
Copy link
Member

trask commented Jul 25, 2024

“Important: You do not need to (and should not) give this account any permissions to any OpenTelemetry repository.”

https://github.com/open-telemetry/community/blob/main/assets.md#opentelemetry-bot

I think the idea at the time was that we would give out individual fine-grained @opentelemetrybot tokens for anything that needed write access to a repo

@svrnm
Copy link
Member Author

svrnm commented Jul 25, 2024

“Important: You do not need to (and should not) give this account any permissions to any OpenTelemetry repository.”
main/assets.md#opentelemetry-bot

I think the idea at the time was that we would give out individual fine-grained @opentelemetrybot tokens for anything that needed write access to a repo

Thanks for clarification. In this particular use case we would not even need write permissions, triage would be fine, would we have any concerns with that?

Looking through the permissions I don't see anything I would be concerned about:

https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/managing-repository-roles/repository-roles-for-an-organization#permissions-for-each-role

@svrnm
Copy link
Member Author

svrnm commented Jul 30, 2024

@arminru @trask based on my comment above, any concerns? 👍 👎

@trask
Copy link
Member

trask commented Jul 30, 2024

from the perspective of least-privilege, we could create a new opentelemetrybot PAT just for the website repo, and grant permissions to the website repo only for that particular PAT, e.g.

image

@jack-berg
Copy link
Member

I've created a new fine-grained PAT for opentelemetry.io and set it as a secret for the repository under key OPENTELEMETRYBOT_OPENTELEMETRY_IO_PAT.

The PAT has read and write access to issues and pull requests, and expires 7/30/2025 (one year is the longest TTL allowed).

Screenshot 2024-07-30 at 4 11 45 PM

@svrnm
Copy link
Member Author

svrnm commented Jul 31, 2024

Error: Resource not accessible by personal access token - https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request

with the new PAT

@jack-berg
Copy link
Member

I've generated a new token and verified it works. The issue was that the token's resource owner was opentelemetrybot instead of the open-telemetry organization. This meant that the token only had access to the bot's forks of opentelemetry repos. See screenshot for a successful PR review request:

Screenshot 2024-07-31 at 1 13 05 PM

@svrnm
Copy link
Member Author

svrnm commented Jul 31, 2024

It works also in the workflow, I just tested it 🎉

One last question: the key will expire in 12 months from now? How do we make sure that we reset the value early?

@trask
Copy link
Member

trask commented Aug 1, 2024

It works also in the workflow, I just tested it 🎉

@svrnm It looks like @opentelemetrybot still has triage access to the website repo.

Can you remove that access and see if it still works? Thanks!

One last question: the key will expire in 12 months from now? How do we make sure that we reset the value early?

Can you open a new issue for this, and we can add it to the Infrastructure SIG project board? I think @austinlparker was right about possibly needing some automation (in this case reminders?) around opentelemetrybot token maintenance.

@svrnm
Copy link
Member Author

svrnm commented Aug 1, 2024

It works also in the workflow, I just tested it 🎉

@svrnm It looks like @opentelemetrybot still has triage access to the website repo.

Can you remove that access and see if it still works? Thanks!

I thought I had removed it before :-/

without these permissions we are back to the error:

Error: Resource not accessible by personal access token - https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request

I will keep the permissions set to have the workflow working right now, later when we are all available we can remove them once again and get it working without it.

@jack-berg
Copy link
Member

One last question: the key will expire in 12 months from now? How do we make sure that we reset the value early?

Not sure. Low tech way is to set a reminder and followup.

@jack-berg
Copy link
Member

@svrnm marking this as resolved - feel free to re-open if there is more to do.

@svrnm svrnm reopened this Aug 9, 2024
@svrnm
Copy link
Member Author

svrnm commented Aug 9, 2024

@jack-berg the opentelemetrybot still has "triage" permissions on the opentelemetry.io repository, without I get an error. If we can leave it like that, I am fine, otherwise we need to figure out what's missing for that token

@trask
Copy link
Member

trask commented Aug 12, 2024

Error: Resource not accessible by personal access token - https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request

I wonder if the token needs read:org permission in order to reference the @open-telemetry teams that are used as reviewers?

I've run into a semi-related issue before:

https://github.com/open-telemetry/opentelemetry-java-contrib/blob/d9ab85f943513f01330de809ecf33208013d60b2/.github/workflows/reusable-create-collector-contrib-pull-request.yml#L82-L89

@svrnm
Copy link
Member Author

svrnm commented Aug 13, 2024

I wonder if the token needs read:org permission in order to reference the https://github.com/open-telemetry teams that are used as reviewers?

Maybe? Probably we need to try. Can we (@trask @jack-berg and I) find some time in early September to sit together for ~1hr and work on this in sync? It works right now with the triager permissions and I will be out of office for a while soon, so if we fix that I would like to not have another long interruption.

@austinlparker austinlparker moved this to Repo Tasks in GitHub Maintenance Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/repo-maintenance Maintenance of repos in the open-telemetry org
Projects
Status: Repo Tasks
Development

No branches or pull requests

3 participants