add iam workload identity federation#4071
add iam workload identity federation#4071rileykarson merged 11 commits intoGoogleCloudPlatform:masterfrom
Conversation
be9866b to
05402fa
Compare
|
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. Thanks for your contribution! A human will be with you soon. @rileykarson, please review this PR or find an appropriate assignee. |
products/iam/api.yaml
Outdated
There was a problem hiding this comment.
The {{id}} field will also need to appear as a field under properties (or parameters, but being in that list instead doesn't actually do anything).
You can then add url_param_only: true to indicate that it's only used in the URL. See
magic-modules/products/servicedirectory/api.yaml
Lines 50 to 57 in 6c2be6a
I'd also recommend avoiding the id name, just because Terraform treats id as a reserved field name. workloadIdentityPoolId from https://cloud.google.com/iam/docs/reference/rest/v1beta/projects.locations.workloadIdentityPools/create seems appropriate.
products/iam/api.yaml
Outdated
There was a problem hiding this comment.
Interesting- Magic Modules assumes that subsequent APIs are a rough superset of each other, but this v1beta API doesn't contain the other handful of IAM resources at all. We may need to represent this as a separate entry in the products/ folder, and override it in terraform.yaml to name it correctly.
You don't need that yet- I want to see what changes the generator makes, exactly.
There was a problem hiding this comment.
Ah, yep, we'll want to add this as a new products/ entry. MM assumes that each version in an API is a rough superset of more stable versions (ga < beta < alpha < eap) because it uses a single API endpoint for all resources under a product. IAM exposes some beta-only resources at a beta endpoint but doesn't also have the GA resources there.
You'll want to create a new products/ folder- probably iambeta- and create an api.yaml and terraform.yaml there. The name (line 21 in this file) should also likely be IAMBeta. Inside terraform.yaml, you can override the default resource name using https://github.com/GoogleCloudPlatform/magic-modules/blob/master/overrides/terraform/resource_override.rb#L34-L38 so that the resources are google_iam_* not google_iam_beta_*
This should result in a second IAM basepath/endpoint getting registered, which points to the beta endpoint.
products/iam/api.yaml
Outdated
There was a problem hiding this comment.
Would you mind breaking this resource into a followup PR afterwards? In my experience it tends to be a lot easier if we're iterating on one resource at a time, especially because a lot of comments that apply to one resource will apply to the second.
There was a problem hiding this comment.
I've removed WorkloadIdentityPoolProvider for now and we can add that in a subsequent PR
05402fa to
7e4e4ba
Compare
|
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 68 insertions(+), 16 deletions(-)) |
|
I added a first attempt at a test, but can't figure out why this is failing when running the terraform-provider-google-beta tests: This is weird as I can see that |
|
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 75 insertions(+), 23 deletions(-)) |
rileykarson
left a comment
There was a problem hiding this comment.
Sorry for the delay in getting back!
The error you're getting is because the tests are being run in the GA provider and not the google-beta provider. The GA google provider doesn't support beta features.
You'll want to supply -v beta to MM to generate into the terraform-google-provider-beta repo.
third_party/terraform/tests/resource_iam_workload_identity_pool_test.go
Outdated
Show resolved
Hide resolved
products/iam/api.yaml
Outdated
There was a problem hiding this comment.
Ah, yep, we'll want to add this as a new products/ entry. MM assumes that each version in an API is a rough superset of more stable versions (ga < beta < alpha < eap) because it uses a single API endpoint for all resources under a product. IAM exposes some beta-only resources at a beta endpoint but doesn't also have the GA resources there.
You'll want to create a new products/ folder- probably iambeta- and create an api.yaml and terraform.yaml there. The name (line 21 in this file) should also likely be IAMBeta. Inside terraform.yaml, you can override the default resource name using https://github.com/GoogleCloudPlatform/magic-modules/blob/master/overrides/terraform/resource_override.rb#L34-L38 so that the resources are google_iam_* not google_iam_beta_*
This should result in a second IAM basepath/endpoint getting registered, which points to the beta endpoint.
b09bf11 to
698a885
Compare
|
I've moved the new resources to But I can see the new Add the google-beta provider to the example also doesn't work: gives Is there another beta-only resource with a test I can copy from? |
1 similar comment
|
Most changes to identity-pools (and providers) return Operations that we need to poll for. I can copy that from other similar resources, but would like to get the test running before attempting so. With a (failing) test it's much easier for me to iterate on attempts to get it all working. |
rileykarson
left a comment
There was a problem hiding this comment.
Are you still encountering the same issue with the branch as it is now? I received the following, which indicates the rest ran successfully:
------- Stdout: -------
=== RUN TestAccIAMBetaWorkloadIdentityPool_example
=== PAUSE TestAccIAMBetaWorkloadIdentityPool_example
=== CONT TestAccIAMBetaWorkloadIdentityPool_example
TestAccIAMBetaWorkloadIdentityPool_example: provider_test.go:266: Step 1/1 error: Error running pre-apply refresh: 2020/10/14 21:29:32 [DEBUG] Using modified User-Agent: Terraform/0.12.29 HashiCorp-terraform-exec/0.10.0
Error: Missing required argument
on config516010655/terraform_plugin_test.tf line 13, in resource "google_iam_workload_identity_pool" "my_pool":
13: resource "google_iam_workload_identity_pool" "my_pool" {
The argument "workload_identity_pool_id" is required, but no definition was
found.
--- FAIL: TestAccIAMBetaWorkloadIdentityPool_example (0.39s)
FAIL
One thing you could try is checking out https://github.com/modular-magician/terraform-provider-google-beta/tree/auto-pr-4071, the code generated by our CI by your current changes (and the code I ran), and then running MM on top of that locally.
|
Nevermind, I forgot to upgrade terraform to the latest version. Now upgrade to v0.13.4 and the test runs (and fails as expected). While I continu to work on this PR I'll mark it as draft until it is complete |
so we first focus on a single new resource
as this is no auto-generated
698a885 to
9e09387
Compare
|
I have added some tests and made some changes to the code to ensure all tests succeed. I think this is ready for review by @rileykarson |
|
Should I also make sure a terraform data source is generated? (if so, what are the basic steps to get a data source - are they all handcoded or somehow generated?) |
rileykarson
left a comment
There was a problem hiding this comment.
LGTM!
If you're interested in adding a datasource, #4111 is a good template for what adding one looks like. They're handwritten, but based off the existing resource code. I'd suggest doing that in a separate PR (including from the one adding WorkloadIdentityPoolProvider) for ease of review.
This is my attempt to add the new Google Workload Identity Federation resources.
fixes hashicorp/terraform-provider-google#7455
But I run into the issue that the IAM resources seem to be handwritten. Now that I add
products/iam/terraform.yamlall the resources in that file get generated to terraform, including things like service_account and role.What would be the best way to leave the handwritten iam resources alone, but still be able to add the new iam-beta resources for workload identity federation?
If this PR is for Terraform, I acknowledge that I have:
make testandmake lintto ensure it passes unit and linter tests.