Skip to content
This repository was archived by the owner on Nov 27, 2024. It is now read-only.

Fix folder_iam logic to use the correct updater#764

Merged
melinath merged 4 commits into
mainfrom
fix_folder_iam
Jun 16, 2022
Merged

Fix folder_iam logic to use the correct updater#764
melinath merged 4 commits into
mainfrom
fix_folder_iam

Conversation

@roaks3

@roaks3 roaks3 commented Jun 14, 2022

Copy link
Copy Markdown
Contributor

This change fixes the google_folder_iam_member resource by using the correct updater in our folder_iam logic. It also introduces tests for the google_folder_iam_member to prevent regressions.

Fixes #708
b/233071873

@roaks3 roaks3 requested review from a team and iyabchen and removed request for a team June 14, 2022 14:43
@roaks3

roaks3 commented Jun 15, 2022

Copy link
Copy Markdown
Contributor Author

@melinath I'm not sure if you have a better idea of what's going wrong, but my latest attempt at getting these tests working was to introduce a new folder, and I was expecting the test to work like the storage bucket from this example: #218. However, I'm still getting errors when they run. My suspicion is that I'm doing something wrong with the placeholders, or perhaps folders are somehow handled differently than storage buckets.

@roaks3

roaks3 commented Jun 15, 2022

Copy link
Copy Markdown
Contributor Author

Ahh, after seeing the unit test failure already, now I'm wondering if the issue here is that google_folder itself is not a supported resource (where google_storage_bucket is, from the other example)

@melinath melinath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

google_folder isn't supported, but that won't have any impact on whether the related iam resource can be validated, so you should be good from that perspective!

It looks like the fatal error you're getting is:

converting TF resource to CAI: getting resource ancestry or parent failed: folder id not found in terraform data

That's coming from ancestrymanager.go - basically it means that the folder id is (known after apply). The workaround is to "hardcode" the folder id as a known value using the {{.FolderID}} variable.

It looks like we do this currently for folder-level organization policies, so you may be able to base your work off of that:

resource "google_folder_organization_policy" "serial_port_policy" {

@roaks3

roaks3 commented Jun 15, 2022

Copy link
Copy Markdown
Contributor Author

@melinath if you take a look at the initial commit in this PR, I had tried using the {{.FolderID}}, but ran into the other issue we had talked about where there are other permissions on the folder already. Here is the integration test error from that commit: https://pantheon.corp.google.com/cloud-build/builds/82d19c48-b300-43bc-b494-28c78c2aed43?project=cloud-foundation-cicd.

I'm happy to shift toward a solution using the folder provided by the user, but can you provide any insight on how to get around the existing permissions problem?

@melinath

Copy link
Copy Markdown
Member

@roaks3 ah, gotcha! Thanks for letting me know. I'll look into that and see if we already have a workaround.

@melinath

Copy link
Copy Markdown
Member

Okay - so, it looks like for iam binding and iam member, we use special comparison functions to only check the parts we care about:

{name: "example_project_iam_binding", compareConvertOutput: compareMergedIamBindingOutput},

For iam_policy, it shouldn't have any issues because it's fully authoritative.

Once you get to the magic modules side - I believe that if you add the folder tests as an explicit test in https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/validator/tests/source/cli_test.go.erb, it should automatically exclude them from the autogenerated list.

Relatedly: As long as you're working on this, could you add tests for iam binding and iam policy as well to make sure there aren't any issues we're missing there?

@roaks3

roaks3 commented Jun 15, 2022

Copy link
Copy Markdown
Contributor Author

Awesome, thank you! I'll try that out, and yes, happy to add the other tests 🙂

@roaks3

roaks3 commented Jun 16, 2022

Copy link
Copy Markdown
Contributor Author

Will address those additional tests with a separate ticket: b/236256186

@melinath melinath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fail to parse terraform 1.1.9 plan with google_folder_iam_member in a new folder

2 participants