apache
GCP Access Control
This PR has the purpose of implementing a GCP resource access control using a system managed through Git and Terraform
The idea is to have 5 roles:
- Beam Viewer -> GCP Viewer permissions based on the services used by Beam and excluding secretmanager permissions.
- Beam Commiter -> GCP Viewer permissions based on the services used by Beam and excluding secretmanager permissions. Its like Beam Viewer but for people actively contributing code.
- Beam Infra Manager -> GCP Editor permissions based on the services used by Beam Administrators without destructive permissions.
- Beam Admin -> Permissions similar to GCP Editor based on the services used by Beam, but with destructive capabilities.
- Beam Owner -> This is currently a placeholder; use the GCP Owner role directly if needed.
New contributors need to add their user and desired role to the users.yml file and receive the assigned access once the PR is merged using terraform and github actions.
This would help with the administrative workflow of managing permissions and force the use of a simple set of roles.
Open for any feedback :)
Right now the users.yml file is mapped to the things we have right now but the idea would be to migrate them to the new roles set.
Assigning reviewers:
R: @Abacn added as fallback since no labels match configuration
Note: If you would like to opt out of this review, comment assign to next reviewer.
Available commands:
stop reviewer notifications- opt out of the automated review toolingremind me after tests pass- tag the comment author after tests passwaiting on author- shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:
R: @Abacn added as fallback since no labels match configuration
Available commands:
stop reviewer notifications- opt out of the automated review toolingremind me after tests pass- tag the comment author after tests passwaiting on author- shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
Added base roles on the custom roles generation script. Now the roles can have multiple base roles and not just viewer and editor, to add more permissions.
Also added some permissions roles to a new commiter role, the idea is to use it when you need resource access as a user but not as editor
@damccorm - @ksobrenat32 has added the capability to select google roles, which should help have a backwards-compatible migration.
I think this still results in a huge diff between what we have today and what we're migrating to - https://github.com/apache/beam/blob/b26ecbadb35c2876b50fa29ddd49b1470163ed9d/infra/apache-beam-testing.permission-differences.yaml is quite large.
I think we should be targeting exact 1:1 permission parity for a migration like this. If we want to update/prune permissions later, that seems fine, but I don't think we should do that as part of this.
I think this still results in a huge diff between what we have today and what we're migrating to - https://github.com/apache/beam/blob/b26ecbadb35c2876b50fa29ddd49b1470163ed9d/infra/apache-beam-testing.permission-differences.yaml is quite large.
I think we should be targeting exact 1:1 permission parity for a migration like this. If we want to update/prune permissions later, that seems fine, but I don't think we should do that as part of this.
the file is large because it adds a lot of permissions, but it only removes one. We should target to remove zero, right? so this way the migration is backwards compatible.
@ksobrenat32 can we investigate why dcrhodes is losing that one permission?
I think this still results in a huge diff between what we have today and what we're migrating to - https://github.com/apache/beam/blob/b26ecbadb35c2876b50fa29ddd49b1470163ed9d/infra/apache-beam-testing.permission-differences.yaml is quite large. I think we should be targeting exact 1:1 permission parity for a migration like this. If we want to update/prune permissions later, that seems fine, but I don't think we should do that as part of this.
the file is large because it adds a lot of permissions, but it only removes one. We should target to remove zero, right? so this way the migration is backwards compatible.
@ksobrenat32 can we investigate why dcrhodes is losing that one permission?
I think we should target adding 0 permissions as well
The idea is to start with the original permissions, which is a 1:1 copy to the permissions on the gcp protect.
Then move to the migrated permissions, which are the ones based on the custom roles. This migration can be made gradually based on the permissions each user has.
The migration plan is explained at the end of the readme
I think this still results in a huge diff between what we have today and what we're migrating to - https://github.com/apache/beam/blob/b26ecbadb35c2876b50fa29ddd49b1470163ed9d/infra/apache-beam-testing.permission-differences.yaml is quite large.
I think we should be targeting exact 1:1 permission parity for a migration like this. If we want to update/prune permissions later, that seems fine, but I don't think we should do that as part of this.
the file is large because it adds a lot of permissions, but it only removes one. We should target to remove zero, right? so this way the migration is backwards compatible.
@ksobrenat32 can we investigate why dcrhodes is losing that one permission?
I think we should target adding 0 permissions as well
The idea is to start with the original permissions, which is a 1:1 copy to the permissions on the gcp protect.
Then move to the migrated permissions, which are the ones based on the custom roles. This migration can be made gradually based on the permissions each user has.
The migration plan is explained at the end of the readme
I think this still results in a huge diff between what we have today and what we're migrating to - https://github.com/apache/beam/blob/b26ecbadb35c2876b50fa29ddd49b1470163ed9d/infra/apache-beam-testing.permission-differences.yaml is quite large.
I think we should be targeting exact 1:1 permission parity for a migration like this. If we want to update/prune permissions later, that seems fine, but I don't think we should do that as part of this.
the file is large because it adds a lot of permissions, but it only removes one. We should target to remove zero, right? so this way the migration is backwards compatible.
@ksobrenat32 can we investigate why dcrhodes is losing that one permission?
I think we should target adding 0 permissions as well
Ok, looking at that, users.yml should match apache-beam-testing.original-roles.yaml, then right?
If there are 2 distinct parts to this migration could we split this into those pieces? I think migrating the initial set of roles to terraform management is reasonable, but I do not think we should bundle that with changing permissions (including at the review stage)
The idea is to start with the original permissions, which is a 1:1 copy to the permissions on the gcp protect. Then move to the migrated permissions, which are the ones based on the custom roles. This migration can be made gradually based on the permissions each user has. The migration plan is explained at the end of the readme
I think this still results in a huge diff between what we have today and what we're migrating to - https://github.com/apache/beam/blob/b26ecbadb35c2876b50fa29ddd49b1470163ed9d/infra/apache-beam-testing.permission-differences.yaml is quite large.
I think we should be targeting exact 1:1 permission parity for a migration like this. If we want to update/prune permissions later, that seems fine, but I don't think we should do that as part of this.
the file is large because it adds a lot of permissions, but it only removes one. We should target to remove zero, right? so this way the migration is backwards compatible.
@ksobrenat32 can we investigate why dcrhodes is losing that one permission?
I think we should target adding 0 permissions as well
Ok, looking at that,
users.ymlshould matchapache-beam-testing.original-roles.yaml, then right?If there are 2 distinct parts to this migration could we split this into those pieces? I think migrating the initial set of roles to terraform management is reasonable, but I do not think we should bundle that with changing permissions (including at the review stage)
I have done just that. Created a smaller PR that has only the 1:1 migration at #35701
This PR now follows #35701. Now that user roles are generated through a yaml file, the objective is to migrate most users to custom roles that cover most of beam's GCP usage.
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:
R: @Abacn added as fallback since no labels match configuration
Available commands:
stop reviewer notifications- opt out of the automated review toolingremind me after tests pass- tag the comment author after tests passwaiting on author- shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:
R: @Abacn added as fallback since no labels match configuration
Available commands:
stop reviewer notifications- opt out of the automated review toolingremind me after tests pass- tag the comment author after tests passwaiting on author- shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
okay - the contents of this PR do not perform the migration. They are the utilities necessary to perform it. These look good. My proposal for the migration is to do a small subset of people each time in a new PR. I can work with @ksobrenat32 to make these changes.
The utilities LGTM and I think we can proceed to merge. The actual changes will come afterwards.