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.

Sign up for GitHub

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

Jump to bottom

feat: Fleet app operator permissions #1986

Merged
merged 33 commits into from
Jul 19, 2024
Merged

feat: Fleet app operator permissions #1986

merged 33 commits into from
Jul 19, 2024

Conversation

hosseingolestani
Copy link
Contributor

This PR introduces a Terraform module that bundles different permissions (IAM and RBAC Role Bindings) required for Fleet team management.

@hosseingolestani
added module for fleet app operator permissions
926c088
@hosseingolestani
some fixes to the module variables
59b1c46 
- added type string to the role variable
- changed role maps from variables to locals
@hosseingolestani
used make docker_generate_docs to correct module input/output documen…
d5c239b 
…tation
@hosseingolestani
added an empty-string default value for the user and group variables …
fded424 
…because only one of them is required
@hosseingolestani
added an example module to leverage fleet-app-operator-permissions
7f504ed
@hosseingolestani
changed app operator principal inputs from <user, group> to <app_oper…
8e77e74 
…ator_name, is_user_app_operator>

With this change, the admin explicitly specifies whether the app operator is a user or group. This can help to not take dependency to the value of the app operator name (i.e., previously whether user != '' or a group != '').
@hosseingolestani
added support for 'principal', 'principalSet, and 'serviceAccount' ap…
fd9b18e 
…p operators
@hosseingolestani
simplified the simple_fleet_app_operator_permissions module
90473bf 
- removed the app operator email input and use a service account created in the module instead
- removed the app operator role input and simply use the VIEW role
@hosseingolestani
simplified simple_fleet_app_operator_permissions a bit more by removi…
893360c 
…ng the app_operator_team input
@hosseingolestani
added basic go test for simple_fleet_app_operator_permissions
f427027
@hosseingolestani
fixed readme and issues raised by make docker_test_lint
2afd832
@hosseingolestani hosseingolestani requested review from ericyz, gtsorbo and a team as code owners July 3, 2024 18:12
@hosseingolestani hosseingolestani changed the title Fleet app operator permissions feat: Fleet app operator permissions Jul 3, 2024
@apeabody
Copy link
Contributor

apeabody commented Jul 3, 2024

/gcbrun

@apeabody apeabody self-assigned this Jul 3, 2024
@hosseingolestani
made fleet-app-operator-permissions source path relative in the examp…
67222db 
…le module This was changed by my local run of make docker_test_lint.
@apeabody
Copy link
Contributor

apeabody commented Jul 3, 2024

/gcbrun

@apeabody
Copy link
Contributor

apeabody commented Jul 3, 2024

From the CI:

        	Error:      	Received unexpected error:
        	            	FatalError{Underlying: error while running command: exit status 1; 
        	            	Error: No value for required variable
        	            	
        	            	  on variables.tf line 17:
        	            	  17: variable "project_id" {
        	            	
        	            	The root module input variable "project_id" is not set, and has no default
        	            	value. Use a -var or -var-file command line argument to provide a value for
        	            	this variable.}
        	Test:       	TestSimpleFleetAppOperatorPermissions

@apeabody
Copy link
Contributor

apeabody commented Jul 3, 2024

From the CI:

        	Error:      	Received unexpected error:
        	            	FatalError{Underlying: error while running command: exit status 1; 
        	            	Error: No value for required variable
        	            	
        	            	  on variables.tf line 17:
        	            	  17: variable "project_id" {
        	            	
        	            	The root module input variable "project_id" is not set, and has no default
        	            	value. Use a -var or -var-file command line argument to provide a value for
        	            	this variable.}
        	Test:       	TestSimpleFleetAppOperatorPermissions

When possible, it's suggest to avoid variables in examples (as you will need to provide a value from the integration test). For example, a end to end example could first create a born in fleet cluster, and then pass the fleet project from that.

@hosseingolestani
Copy link
Contributor Author

From the CI:

        	Error:      	Received unexpected error:
        	            	FatalError{Underlying: error while running command: exit status 1; 
        	            	Error: No value for required variable
        	            	
        	            	  on variables.tf line 17:
        	            	  17: variable "project_id" {
        	            	
        	            	The root module input variable "project_id" is not set, and has no default
        	            	value. Use a -var or -var-file command line argument to provide a value for
        	            	this variable.}
        	Test:       	TestSimpleFleetAppOperatorPermissions

When possible, it's suggest to avoid variables in examples (as you will need to provide a value from the integration test). For example, a end to end example could first create a born in fleet cluster, and then pass the fleet project from that.

I defined a new project for fleet testing purposes (though for now, I'm not creating a cluster in the example module). I added the test fixture to provide the project ID.

@apeabody
Copy link
Contributor

/gcbrun

hosseingolestani and others added 2 commits July 10, 2024 23:59
@hosseingolestani
made app operator an input to the example module and set it in the te…
4a4ee3e 
…st fixtures

This should hopefully make the app operator email available at apply time of the example module.
@hosseingolestani
Merge branch 'master' into fleet-app-operator-permissions
838e7de
@apeabody
Copy link
Contributor

/gcbrun

@hosseingolestani
put back app operator service account in simple_fleet_app_operator_pe…
51a2e1b 
…rmissions

I'm now building the service account email string explicitly and instead declaring depencency to the service account resource, so that for_each works without the need for applying the service account resource in advance.
@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

/gcbrun

@hosseingolestani
defined IAM bindings for gke-project-fleet
622b266 
This seems to be the correct way to grant the necessary permissions.
@hosseingolestani
Copy link
Contributor Author

/gcbrun

1 similar comment
@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor 

   simple_fleet_app_operator_permissions_test.go:54: 
        	Error Trace:	/workspace/test/integration/simple_fleet_app_operator_permissions/simple_fleet_app_operator_permissions_test.go:54
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:638
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/utils/stages.go:31
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
        	Error:      	Not equal: 
        	            	expected: false
        	            	actual  : true
        	Test:       	TestSimpleFleetAppOperatorPermissions
        	Messages:   	app operator's log view condition should be in the project IAM policy

hosseingolestani and others added 2 commits July 17, 2024 22:29
@hosseingolestani
broke down log view condition check in TestSimpleFleetAppOperatorPerm…
c1ffbee 
…issions

The condition is a bit long, and it's possibly included in two lines in the project IAM policy. I'm trying to see if checking subconditions separately works.
@hosseingolestani
Merge branch 'master' into fleet-app-operator-permissions
0f9e63d
@hosseingolestani
Copy link
Contributor Author

/gcbrun

apeabody
apeabody requested changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @hosseingolestani!

Just a few final nits and we can get this merged!

modules/fleet-app-operator-permissions/main.tf Outdated Show resolved Hide resolved
modules/fleet-app-operator-permissions/main.tf Outdated Show resolved Hide resolved
modules/fleet-app-operator-permissions/versions.tf Outdated Show resolved Hide resolved
test/fixtures/simple_fleet_app_operator_permissions/main.tf Outdated Show resolved Hide resolved
@apeabody
Copy link
Contributor

Spot checked the most recent CI test for TestSimpleFleetAppOperatorPermissions

@apeabody apeabody merged commit e0fd03a into terraform-google-modules:master Jul 19, 2024
4 checks passed
@release-please release-please bot mentioned this pull request Jul 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apeabody apeabody apeabody approved these changes

@ericyz ericyz Awaiting requested review from ericyz ericyz is a code owner

@gtsorbo gtsorbo Awaiting requested review from gtsorbo

Assignees

@apeabody apeabody

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hosseingolestani @apeabody