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

Remove pl-deploy-bot from the org #66

Merged
merged 8 commits into from
Sep 11, 2024

Conversation

rjan90
Copy link
Contributor

@rjan90 rjan90 commented Sep 3, 2024

Summary

When reviewing the Lotus-Infra repository, which FilOz now maintains, we encountered the pl-deploy-bot user. We believe this bot was originally created as part of a GitOps contract between Protocol Labs and Weaveworks.

We've determined that the bot is no longer used in the lotus-infra repository. For security reasons, we recommend removing the pl-deploy-bot user from the organization entirely, and are opening this PR to propose this change and get feedback. If anyone is aware of any current uses for this bot within the organization, please let us know.

Reviewer's Checklist

  • It is clear where the request is coming from (if unsure, ask)
  • All the automated checks passed
  • The YAML changes reflect the summary of the request
  • The Terraform plan posted as a comment reflects the summary of the request

Remove pl-deploy-bot
@rjan90 rjan90 requested a review from a team as a code owner September 3, 2024 13:18
Copy link
Member

@BigLep BigLep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I can merge in a day if no one else chimes in.

github/filecoin-project.yml Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Sep 3, 2024

The following access changes will be introduced as a result of applying the plan:

Access Changes
User pl-deploy-bot:
  - will lose push permission to sentinel-infra
  - will lose admin permission to wge-post-deployment-template-renderer

@BigLep
Copy link
Member

BigLep commented Sep 6, 2024

@galargh : can you help educate me on how to handle this?

From https://github.com/filecoin-project/github-mgmt/actions/runs/10743694671/job/29805452133?pr=66

Error: Instance cannot be destroyed

on resources.tf line 1:
1: resource "github_membership" "this" {

Resource github_membership.this["pl-deploy-bot"] has
lifecycle.prevent_destroy set, but the plan calls for this resource to be
destroyed. To avoid this error and continue with the plan, either disable
lifecycle.prevent_destroy or reduce the scope of the plan using the -target
flag.

I'm not seeing anything in https://github.com/orgs/filecoin-project/people/pl-deploy-bot that would prevent this.

My fallback is to remove the user from the UI and then merge this PR.

@BigLep
Copy link
Member

BigLep commented Sep 6, 2024

Oh, it looks like we prevent member deletes per https://github.com/filecoin-project/github-mgmt/blob/master/terraform/resources.tf#L9

@galargh : is that intentional? What's the recommended way forward?

@galargh
Copy link
Contributor

galargh commented Sep 11, 2024

Oh, it looks like we prevent member deletes per master/terraform/resources.tf#L9

@galargh : is that intentional? What's the recommended way forward?

This is intentional. This is because org member removals are hard to revert. To re-invite someone, they have to accept the invitation. This is a security measure.

There are 2 intended ways forward for this.

  1. Temporarily remove the protection. As in: 1. Remove the protection. 2. Remove the org member via github as code. 3. Reinstate the protection.
  2. Discuss removal of the member in a PR but apply the change manually through the UI. The order being: 1. Discussion. 2. Manual removal. 3. Run sync in github as code.

Copy link
Contributor

Before merge, verify that all the following plans are correct. They will be applied as-is after the merge.

Terraform plans

filecoin-project

Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
  - destroy

Terraform will perform the following actions:

  # github_repository_collaborator.this["sentinel-infra:pl-deploy-bot"] will be destroyed
  # (because key ["sentinel-infra:pl-deploy-bot"] is not in for_each map)
  - resource "github_repository_collaborator" "this" {
      - id         = "sentinel-infra:pl-deploy-bot" -> null
      - permission = "push" -> null
      - repository = "sentinel-infra" -> null
      - username   = "pl-deploy-bot" -> null
    }

  # github_repository_collaborator.this["wge-post-deployment-template-renderer:pl-deploy-bot"] will be destroyed
  # (because key ["wge-post-deployment-template-renderer:pl-deploy-bot"] is not in for_each map)
  - resource "github_repository_collaborator" "this" {
      - id         = "wge-post-deployment-template-renderer:pl-deploy-bot" -> null
      - permission = "admin" -> null
      - repository = "wge-post-deployment-template-renderer" -> null
      - username   = "pl-deploy-bot" -> null
    }

Plan: 0 to add, 0 to change, 2 to destroy.

@BigLep
Copy link
Member

BigLep commented Sep 11, 2024

Oh, it looks like we prevent member deletes per master/terraform/resources.tf#L9
@galargh : is that intentional? What's the recommended way forward?

This is intentional. This is because org member removals are hard to revert. To re-invite someone, they have to accept the invitation. This is a security measure.

There are 2 intended ways forward for this.

  1. Temporarily remove the protection. As in: 1. Remove the protection. 2. Remove the org member via github as code. 3. Reinstate the protection.
  2. Discuss removal of the member in a PR but apply the change manually through the UI. The order being: 1. Discussion. 2. Manual removal. 3. Run sync in github as code.

Ack, got it. Here is my plan:

  • I added a comment next to the user to delete saying they'll be manually deleted and referencing this PR
  • I'm then going to merge this PR. That way the context for this change shows up in the commit log.
  • Then I'll go delete in the UI
  • Paste back screenshot of deletion
  • Write a readme for how to delete a user

@BigLep BigLep merged commit 2635a26 into filecoin-project:master Sep 11, 2024
6 checks passed
@BigLep
Copy link
Member

BigLep commented Sep 11, 2024

@BigLep BigLep mentioned this pull request Sep 11, 2024
4 tasks
@BigLep
Copy link
Member

BigLep commented Sep 11, 2024

Docs for how to remove a member: #72

Here is the sync workflow run to update now that the user has been removed: https://github.com/filecoin-project/github-mgmt/actions/runs/10816816463

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

Successfully merging this pull request may close these issues.

3 participants