feat: Fleet app operator permissions #1986

hosseingolestani · 2024-07-03T18:12:44Z

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

- added type string to the role variable - changed role maps from variables to locals

…tation

…because only one of them is required

…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 != '').

…p operators

- 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

…ng the app_operator_team input

apeabody · 2024-07-03T18:34:41Z

/gcbrun

…le module This was changed by my local run of make docker_test_lint.

apeabody · 2024-07-03T19:22:32Z

/gcbrun

apeabody · 2024-07-03T20:18:23Z

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 · 2024-07-03T20:25:40Z

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.

apeabody · 2024-07-03T21:16:14Z

modules/fleet-app-operator-permissions/variables.tf

+ * limitations under the License.
+ */
+
+variable "project_id" {


Given the heavy use of project_id, I suggest clarifying:

Suggested change

variable "project_id" {

variable "fleet_project_id" {

apeabody · 2024-07-03T21:26:54Z

modules/fleet-app-operator-permissions/variables.tf

+  type        = string
+}
+
+variable "is_user_app_operator" {


Rather than introducing an additional variable, I'd suggest we try to simplify the user experience. Looks like we required user:person@google.com or group:people@google.com for app_operator_name, this variable is_user_app_operator could be eliminated?

Could even add app_operator_name variable validation to ensure the string starts with a valid prefix: "user:", "group:", "principal:", "principalSet:". Then use a regex or similar to determine local.is_user_app_operator.

I'm now using the users and groups variables, by which the lists of user and group app operators can be provided to have the specified role.

One thing about the format is that a user/group is in fact an RBAC subject. The corresponding IAM principal is created accordingly, e.g., for user person@google.com, the IAM principal would be user:person@google.com. I defined these variables in RBAC style (rather than IAM style) to be consistent with how the same thing is done with gcloud. So as for validation, we don't have a set of acceptable prefixes with this approach.

apeabody · 2024-07-03T21:27:46Z

modules/fleet-app-operator-permissions/variables.tf

+  description = "The principal role for the Fleet Scope (`VIEW`/`EDIT`/`ADMIN`)."
+  type        = string
+  validation {
+    condition     = var.role == "VIEW" || var.role == "EDIT" || var.role == "ADMIN"


nit: probably could use contains(list, value)

apeabody · 2024-07-03T21:28:30Z

modules/fleet-app-operator-permissions/outputs.tf

+  value       = var.project_id
+}
+
+output "wait" {


…lists

…ions and example modules

… tests

…sion project ID

hosseingolestani · 2024-07-10T18:37:39Z

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 · 2024-07-10T20:00:45Z

/gcbrun

…st fixtures This should hopefully make the app operator email available at apply time of the example module.

apeabody · 2024-07-12T17:19:32Z

/gcbrun

…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 · 2024-07-12T20:13:26Z

/gcbrun

…d iam service account

apeabody · 2024-07-12T22:16:14Z

/gcbrun

This seems to be the correct way to grant the necessary permissions.

hosseingolestani · 2024-07-13T00:01:12Z

/gcbrun

apeabody · 2024-07-15T16:23:49Z

/gcbrun

apeabody · 2024-07-15T17:33:00Z

   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/blueprint-test@v0.16.0/pkg/tft/terraform.go:638
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.16.0/pkg/tft/terraform.go:670
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.16.0/pkg/utils/stages.go:31
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.16.0/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

…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 · 2024-07-17T22:39:00Z

/gcbrun

…ions to just looking for log buckets in the project IAM policy

hosseingolestani added 11 commits May 28, 2024 20:18

added module for fleet app operator permissions

926c088

some fixes to the module variables

59b1c46

- added type string to the role variable - changed role maps from variables to locals

used make docker_generate_docs to correct module input/output documen…

d5c239b

…tation

added an empty-string default value for the user and group variables …

fded424

…because only one of them is required

added an example module to leverage fleet-app-operator-permissions

7f504ed

added support for 'principal', 'principalSet, and 'serviceAccount' ap…

fd9b18e

…p operators

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

simplified simple_fleet_app_operator_permissions a bit more by removi…

893360c

…ng the app_operator_team input

added basic go test for simple_fleet_app_operator_permissions

f427027

fixed readme and issues raised by make docker_test_lint

2afd832

hosseingolestani requested review from ericyz, gtsorbo and a team as code owners July 3, 2024 18:12

hosseingolestani changed the title ~~Fleet app operator permissions~~ Jul 3, 2024

Merge branch 'master' into fleet-app-operator-permissions

e4f9f05

apeabody self-assigned this Jul 3, 2024

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 reviewed Jul 3, 2024

View reviewed changes

modules/fleet-app-operator-permissions/outputs.tf

value = var.project_id

}

output "wait" {

Copy link

Contributor

apeabody Jul 3, 2024

Choose a reason for hiding this comment

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

nice! :)

hosseingolestani added 4 commits July 9, 2024 23:38

changed module variables so that users and groups can be provided as …

fcfae5a

…lists

changed module variable project_id to fleet_project_id

6d9b72b

simplified validation of the role module variable

f7f6809

corrected READMEs regarding how to use the fleet-app-operator-permiss…

9a654cf

…ions and example modules

hosseingolestani and others added 4 commits July 10, 2024 17:07

added config for a new project intended to be used for fleet-specific…

1922a50

… tests

added test fixture for simple_fleet_app_operator_permissions to provi…

2015fa8

…sion project ID

small variable and validation fixes for fleet-app-operator-permissions

7d08847

Merge branch 'master' into fleet-app-operator-permissions

7cdfcf0

hosseingolestani and others added 2 commits July 10, 2024 23:59

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.

Merge branch 'master' into fleet-app-operator-permissions

838e7de

added roles to gke-project-fleet to permit creation of fleet scope an…

4bc10fb

…d iam service account

defined IAM bindings for gke-project-fleet

622b266

This seems to be the correct way to grant the necessary permissions.

hosseingolestani and others added 2 commits July 17, 2024 22:29

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.

Merge branch 'master' into fleet-app-operator-permissions

0f9e63d

hosseingolestani added 2 commits July 18, 2024 20:50

empty commit to run the tests

3940b0a

simplified log view access check in TestSimpleFleetAppOperatorPermiss…

458cdf6

…ions to just looking for log buckets in the project IAM policy

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Fleet app operator permissions #1986

feat: Fleet app operator permissions #1986

hosseingolestani commented Jul 3, 2024

apeabody commented Jul 3, 2024

apeabody commented Jul 3, 2024

apeabody commented Jul 3, 2024

apeabody commented Jul 3, 2024

apeabody Jul 3, 2024

hosseingolestani Jul 10, 2024

apeabody Jul 3, 2024 •

edited

Loading

hosseingolestani Jul 10, 2024

apeabody Jul 3, 2024

hosseingolestani Jul 10, 2024

apeabody Jul 3, 2024

hosseingolestani commented Jul 10, 2024

apeabody commented Jul 10, 2024

apeabody commented Jul 12, 2024

apeabody commented Jul 12, 2024

apeabody commented Jul 12, 2024

hosseingolestani commented Jul 13, 2024

apeabody commented Jul 15, 2024

apeabody commented Jul 15, 2024

hosseingolestani commented Jul 17, 2024

feat: Fleet app operator permissions #1986

Are you sure you want to change the base?

feat: Fleet app operator permissions #1986

Conversation

hosseingolestani commented Jul 3, 2024

apeabody commented Jul 3, 2024

apeabody commented Jul 3, 2024

apeabody commented Jul 3, 2024

apeabody commented Jul 3, 2024

apeabody Jul 3, 2024

Choose a reason for hiding this comment

hosseingolestani Jul 10, 2024

Choose a reason for hiding this comment

apeabody Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

hosseingolestani Jul 10, 2024

Choose a reason for hiding this comment

apeabody Jul 3, 2024

Choose a reason for hiding this comment

hosseingolestani Jul 10, 2024

Choose a reason for hiding this comment

apeabody Jul 3, 2024

Choose a reason for hiding this comment

hosseingolestani commented Jul 10, 2024

apeabody commented Jul 10, 2024

apeabody commented Jul 12, 2024

apeabody commented Jul 12, 2024

apeabody commented Jul 12, 2024

hosseingolestani commented Jul 13, 2024

apeabody commented Jul 15, 2024

apeabody commented Jul 15, 2024

hosseingolestani commented Jul 17, 2024

apeabody Jul 3, 2024 •

edited

Loading