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

fix(organization): use correct role for bindings #1621

Conversation

gustavovalverde
Copy link
Contributor

@gustavovalverde gustavovalverde commented Aug 24, 2023

Previously, setting a binding was using the binding key as the the role, which does not necessarily have the correct format.

Causing errors like:

│ Error: Error applying IAM policy for organization "***REDACTED***": Error setting IAM policy for organization "***REDACTED***": googleapi: Error 400: The role name must be in the form "roles/{role}", "organizations/{organization_id}/roles/{role}", or "projects/{project_id}/roles/{role}"., badRequest
│
│   with module.organization.google_organization_iam_binding.bindings["sa_resman_delegated_iam"],
│   on ../../../modules/organization/iam.tf line 51, in resource "google_organization_iam_binding" "bindings":
│   51: resource "google_organization_iam_binding" "bindings" {

Previously, setting a binding was using the binding `key` as the the role, which does not necessarily have the correct format.

Causing errors like:

```
│ Error: Error applying IAM policy for organization "***REDACTED***": Error setting IAM policy for organization "***REDACTED***": googleapi: Error 400: The role name must be in the form "roles/{role}", "organizations/{organization_id}/roles/{role}", or "projects/{project_id}/roles/{role}"., badRequest
│
│   with module.organization.google_organization_iam_binding.bindings["sa_resman_delegated_iam"],
│   on ../../../modules/organization/iam.tf line 51, in resource "google_organization_iam_binding" "bindings":
│   51: resource "google_organization_iam_binding" "bindings" {
```
@ludoo
Copy link
Collaborator

ludoo commented Aug 24, 2023

This changes how the variable works on a single place (the same interface is used across all modules), for no good reason. Can you provide a rationale for doing this?

@juliocc
Copy link
Collaborator

juliocc commented Aug 24, 2023

@ludoo this is related to this discussion related to FAST custom roles. I think the problem is with FAST and not with this module

@ludoo
Copy link
Collaborator

ludoo commented Aug 24, 2023

Well, changing the variable type is not a good solution. 😀

@ludoo
Copy link
Collaborator

ludoo commented Aug 24, 2023

@ludoo this is related to this discussion related to FAST custom roles. I think the problem is with FAST and not with this module

let's close this PR and figure out what's happening with FAST, BTW I'm using the new IAM code and don't have the above issue

@ludoo ludoo closed this Aug 24, 2023
@gustavovalverde
Copy link
Contributor Author

For FAST this seemed like the right approach based on how the the bindings were defined there (which seems correct). But I certainly can't scope the implications on other modules, even though it solved the problem.

@ludoo
Copy link
Collaborator

ludoo commented Aug 24, 2023

Discussed with Juliio, and you surfaced an error in FAST which was introduced in the last IAM refactor. The role should be the key here

image

but it cannot be as that would trigger an error in the internal loop since the role is custom and its id is dynamic.

Julio came up with a workaround which is similar to the one we use in the project and service account modules' outputs. Alternatively, we will just go back to using a resource for that IAM binding in FAST like before.

Thanks for surfacing this, a fix should land shortly. :)

@ludoo
Copy link
Collaborator

ludoo commented Aug 24, 2023

As a quick workaround, you could use iam_bindings_additive for that, and it would work (replace members with member and make sure to merge it with the other bindings).

@juliocc
Copy link
Collaborator

juliocc commented Aug 24, 2023

So we actually already have the fix in place (i.e. to build the role id statically).

Need to finish a couple of things first but I'll send a fix ASAP. As a separate issue, we need to update the iam_bindings interface to allow multiple conditions on the same role

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants