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

feat: updated the role for CF Gen 2 #88

Merged
merged 6 commits into from
Apr 11, 2024
Merged

feat: updated the role for CF Gen 2 #88

merged 6 commits into from
Apr 11, 2024

Conversation

prabhu34
Copy link
Collaborator

@prabhu34 prabhu34 commented Jan 23, 2024

  • updated the role from cloudfunctions.invoker to run.invoker and from cloudfunctions.developer to run.developer.

#71
#87

@prabhu34 prabhu34 requested review from gtsorbo and a team as code owners January 23, 2024 08:08
@prabhu34
Copy link
Collaborator Author

@amandakarina Can you or your team revisit the roles used in the secure module?

@bruno561
Copy link

@prabhu34 I tried to use the modifications from your PR locally, but it is generating an error when applying.
NOTE: Ignore the use of terragrunt.

module.cloud_functions2.google_cloudfunctions2_function_iam_member.invokers["allUsers"]: Creating...
╷
│ Error: Error applying IAM policy for cloudfunctions2 function "projects/xxx/locations/us-east4/functions/function-terragrunt": Error setting IAM policy for cloudfunctions2 function "projects/xxx/locations/us-east4/functions/function-terragrunt": googleapi: Error 400: Invalid argument: 'An invalid argument was specified. Please check the fields and try again.'
│ 
│   with module.cloud_functions2.google_cloudfunctions2_function_iam_member.invokers["allUsers"],
│   on module/main.tf line 131, in resource "google_cloudfunctions2_function_iam_member" "invokers":131: resource "google_cloudfunctions2_function_iam_member" "invokers" ***
│ 
╵
time=2024-01-28T16:25:53Z level=error msg=Module /home/runner/work/terraform-modules/terraform-modules/terraform/cloudfunctions2 has finished with an error: 1 error occurred:
	* exit status 1

 prefix=[/home/runner/work/terraform-modules/terraform-modules/terraform/cloudfunctions2] 
time=2024-01-28T16:25:53Z level=error msg=1 error occurred:
	* exit status 1

@bruno561
Copy link

I found the error. I made a PR similar to this with the necessary changes to not generate the mentioned error.
PR: #90

@prabhu34
Copy link
Collaborator Author

I found the error. I made a PR similar to this with the necessary changes to not generate the mentioned error. PR: #90

Thank you! I have added the support for both these roles here. Hence closing #90.

@bruno561
Copy link

@prabhu34 Is it still necessary to create the google_cloudfunctions2_function_iam_member resources? Whereas google_cloud_run_service_iam_member will now be used.

@prabhu34
Copy link
Collaborator Author

@prabhu34 Is it still necessary to create the google_cloudfunctions2_function_iam_member resources? Whereas google_cloud_run_service_iam_member will now be used.

It would gradually go off in future versions. But for now this is to support the existing usage of the role bindings.

@g-awmalik
Copy link

/gcbrun

@g-awmalik
Copy link

g-awmalik commented Apr 3, 2024

@bharathkkb - I think this can be approved/merged while the CI is failing and we figure out the root cause.

Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

LGTM, just a question

@@ -153,3 +153,31 @@ resource "google_cloudfunctions2_function_iam_member" "developers" {
google_cloudfunctions2_function.function
]
}

// IAM for invoking HTTP functions (roles/run.invoker)
resource "google_cloud_run_service_iam_member" "invokers" {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to grant both google_cloudfunctions2_function_iam_member and google_cloud_run_service_iam_member?

Copy link
Collaborator Author

@prabhu34 prabhu34 Apr 5, 2024

Choose a reason for hiding this comment

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

@bharathkkb Not necessarily. I thought this could be affecting existing users and left both of them. Previous comment.

@daniel-cit
Copy link
Collaborator

@g-awmalik @bharathkkb @apeabody

PR chore: add retry to secure cloud function test should make the build process stable enough.

A further improvement can be added by increasing the possibles values for the access level create in the test in this other fix!: replace random_id with random_string to increase number of possible access levels .

It can be used after we got a new release of the terraform-google-cloud-run repository.

This should fix most of the build errors base in the last 50 failing build (VPC-SC propagation and VPC-SC name collision)

@apeabody apeabody enabled auto-merge (squash) April 10, 2024 23:46
@apeabody apeabody merged commit 33e9efa into main Apr 11, 2024
6 checks passed
@apeabody apeabody deleted the feat/gen2-role branch April 11, 2024 00:52
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.

None yet

6 participants