Add support IAM policy for the Environment of Apigee X

[xuchenma]

This PR fixes hashicorp/terraform-provider-google#9767 by adding IAM support to ApigeeEnvironment.
It also changed the ApigeeEnvironment resource from requiring "org_id = organizations/<org_name>" to "org_name = <org_name>".
It also refactored some tests for ApigeeOrganization and ApigeeEnvironment. Previously there were separated sets of tests, some written solely for docs (skipped testing) and some solely for testing (skipped docs). This PR used the same test for docs and testing wherever possible. For cases where it has to be separated tests, it minimized the difference between them.

If this PR is for Terraform, I acknowledge that I have:

• Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
• Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
• Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
• Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
• Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

apigee: Added IAM support for `google_apigee_environment`.

apigee: Replaced `org_id` with `org_name` for `google_apigee_environment`.

[modular-magician]

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.

@melinath, please review this PR or find an appropriate assignee.

[xuchenma]

I ran the following acceptance tests in my local:
TestAccApigeeOrganization_apigeeOrganizationCloudBasicExample
TestAccApigeeOrganization_apigeeOrganizationCloudFullTestExample (in beta)
TestAccApigeeEnvironment_apigeeEnvironmentBasicExample
TestAccApigeeEnvironmentIamPolicyGenerated
TestAccApigeeEnvironmentIamBindingGenerated
TestAccApigeeEnvironmentIamMemberGenerated

5 out of the 6 passed, except for "TestAccApigeeEnvironmentIamPolicyGenerated".
The cause seemed to be that, the payload for the POST call to "/v1/organizations/<apigee-org>/environments/<apigee-env>:setIamPolicy" did not include the etag field.
It gave 400 with error message: "Validation error: Etag must be set during SetPolicy and PurgePolicy calls. You can obtain current etag value for this resource by making a GetPolicy call first."
However during "TestAccApigeeEnvironmentIamBindingGenerated" and "TestAccApigeeEnvironmentIamMemberGenerated", the POST calls were made with etag in the payload.
I'm not sure what I can do to fix this.

[melinath]

The change from org_id -> org_name would be a breaking change, so we can't accept it. All existing configs need to keep working without being changed. Putting org_id and org_name side by side would likely be acceptable as long as it won't break existing configs.

However, unless it's absolutely necessary to include org_name changes, this PR should be limited to just adding support for IAM policy to Apigee X Environments.

I'm not sure offhand what to do about the etag fields. I can look into it a bit if you're not able to figure it out. But I wanted to cover the big issue first.

[xuchenma]

Thank you for the reply, Stephen!
I made a few more commits. Right now the api.yaml only has minimal code changes to support IAM.

I ran the following tests in my local:
TestAccApigeeOrganization_apigeeOrganizationCloudBasicExample => PASSED
TestAccApigeeOrganization_apigeeOrganizationCloudFullTestExample (in beta) => PASSED
TestAccApigeeEnvironment_apigeeEnvironmentBasicExample => PASSED
TestAccApigeeEnvironmentIamPolicyGenerated => FAILED, policy cannot be POST'ed for the same "etag" reason as in my previous comment.
TestAccApigeeEnvironmentIamBindingGenerated => FAILED, policy can be POST'ed, but GET afterwards fetched the wrong URL endpoint. It should fetch v1/organizations/<org_name>/environments/<env_name>; however it fetches v1/<org_name>/environments/<env_name> (missed the "organizations/" part).
TestAccApigeeEnvironmentIamMemberGenerated => FAILED for the same reason, missed the "organizations/" part.

Because I only made minimal changes to api.yaml, I'm not sure what I'm missing here. Please advise. Thanks!

[melinath]

You'll probably need to add back the separate test files. The reason we have them is that you can only have one apigee org per project. So, we need to set up separate unique projects for each test, each with their own apigee org (and then enable the relevant apis, etc.)

[xuchenma]

I see. I reverted all changes I made to test files. However I'm not able to run these tests in my local at the moment.. I'm working with my teams to get the permissions to create projects with billings.
Please let me know if there's anything else I can do! Thanks!

[melinath]

It looks like this is resulting in invalid generated test code:

google/iam_apigee_environment_generated_test.go:43:24: Sprintf format %s reads arg #1, but call has 0 args
google/iam_apigee_environment_generated_test.go:53:24: Sprintf format %s reads arg #1, but call has 0 args
google/iam_apigee_environment_generated_test.go:81:24: Sprintf format %s reads arg #1, but call has 0 args
google/iam_apigee_environment_generated_test.go:108:24: Sprintf format %s reads arg #1, but call has 0 args
google/iam_apigee_environment_generated_test.go:117:24: Sprintf format %s reads arg #1, but call has 0 args

[xuchenma]

I couldn't run the existing test cases in my local, so I made the fix to the test files in the previous commit.. Now that those are reverted, can you look into the errors and we collaborate in this PR?

[melinath]

@xuchenma the issue here seems to be that in order to support the automatic generation of IAM tests, you need to add a primary_resource_name to the example in terraform.yaml - here's an example of what that looks like:

magic-modules/mmv1/products/bigquery/terraform.yaml

Line 360 in 55d2caa

primary_resource_name: "fmt.Sprintf(\"tf_test_dataset_id%s\", context[\"random_suffix\"]), fmt.Sprintf(\"tf_test_table_id%s\", context[\"random_suffix\"])"

The lines that are currently failing to compile in your generated code look like this: https://github.com/modular-magician/terraform-provider-google/compare/auto-pr-5270-old..auto-pr-5270#diff-6f00cf9b869826b379f694c599e4c65cf5a78211f5123adbf1b8cf4711373c2fR43 (You can see this code using the links in the comment by the modular magician, or look at your locally-generated code.)

Here's the corresponding line for a working IAM test: https://github.com/hashicorp/terraform-provider-google/blob/d312ba579b0017146eaca06203a2ac7ce29b3999/google/iam_bigquery_table_generated_test.go#L102

In case it helps, here's the line in the template file used to generate IAM tests:

magic-modules/mmv1/templates/terraform/examples/base_configs/iam_test_file.go.erb

Line 82 in 55d2caa

ImportStateId: fmt.Sprintf("<%= import_url -%> <%= object.iam_policy.allowed_iam_role -%>"<% unless import_qualifiers.empty? -%>, <% end -%><%= import_qualifiers.join(', ') -%>, <%= example.primary_resource_name -%>),

Please make sure the generated code compiles for you locally - for example, by ensuring that make lint and make test run successfully. Then I'll be able to help you debug any remaining issues.

[xuchenma]

Thanks, I see. I've added "primary_resource_name", make test passes in my local.
However make lint fails with message make: golangci-lint: No such file or directory.

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 821 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 821 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 3 files changed, 304 insertions(+))
TF Validator: Diff ( 3 files changed, 304 insertions(+))

[modular-magician]

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudbuildWorkerPool_withNetwork|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeFirewallPolicyRule_update|TestAccComputeInstanceFromMachineImage_basic|TestAccComputeInstanceFromMachineImage_overrideMetadataDotStartupScript|TestAccComputeInstanceFromMachineImage_diffProject|TestAccComputeOrganizationSecurityPolicyRule_organizationSecurityPolicyRuleUpdateExample|TestAccComputeOrganizationSecurityPolicy_organizationSecurityPolicyUpdateExample|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerAnalysisOccurrence_multipleSignatures|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccContainerNodePool_withWorkloadIdentityConfig|TestAccContainerNodePool_withGPU|TestAccContainerNodePool_ephemeralStorageConfig|TestAccSqlDatabaseInstance_withPrivateNetwork|TestAccSqlUser_postgresIAM You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=214023

[melinath]

It looks like the reason that etag is present for the member/binding resources and not for the policy resources is because member/binding need to do a get request to merge the existing IAM policy with their changes - which gets them an etag as a side effect. The policy resource overrides whatever's there so it doesn't do a get request first (and so doesn't get an etag). I think we expect this kind of request to go through; this API must be slightly different from standard.

[melinath]

The issues with the member/binding resources have to do with the import URL. I'm not entirely sure what the fix will be; I need to look into it some more.

[melinath]

This should resolve the issues with member/binding.

[melinath]

This % will indicate that the org_id can contain slashes, which should resolve the problem. (Currently the import_format regex excludes the "organization/" at the start of the org id.)

Suggested change

	import_format: ["{{org_id}}/environments/{{name}}", "{{name}}"]
	import_format: ["{{%org_id}}/environments/{{name}}", "{{name}}"]

[xuchenma]

Thanks! I just committed the suggested change and rebased.

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 820 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 820 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 3 files changed, 304 insertions(+))
TF Validator: Diff ( 3 files changed, 304 insertions(+))

[melinath]

/gcbrun

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 820 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 820 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 3 files changed, 304 insertions(+))
TF Validator: Diff ( 3 files changed, 304 insertions(+))

[melinath]

run of just new tests: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstream/216659

[melinath]

It looks like this is causing issues; it is causing things like this to be generated:

			{
				ResourceName:      "google_apigee_environment_iam_binding.foo",
				ImportStateId:     fmt.Sprintf("%s/environments/%s roles/viewer", getTestProjectFromEnv(), fmt.Sprintf("tf-test-apigee-env%s", context["random_suffix"])),
				ImportState:       true,
				ImportStateVerify: true,
			},

This results in the import id being set to something like my-project/environments/tf-test-apigee-envasdfklj but it should be organizations/tf-testasdfklj/environments/tf-testasdfklj. I think you could resolve the issue with something like this:

Suggested change

	primary_resource_name: "getTestProjectFromEnv(), fmt.Sprintf(\"tf-test-apigee-env%s\", context[\"random_suffix\"])"
	primary_resource_name: "fmt.Sprintf(\"organizations/tf-test%s\", context[\"random_suffix\"]), fmt.Sprintf(\"tf-test%s\", context[\"random_suffix\"])"

[melinath]

Let me know if that helps the tests pass for you locally!

[xuchenma]

Thanks, done! I'm still working with my team to setup the permission for me to run tests locally. We can try and see if the tests pass in your system.

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 820 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 820 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 3 files changed, 304 insertions(+))
TF Validator: Diff ( 3 files changed, 304 insertions(+))

[melinath]

Run of the IAM tests: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstream/217239

[melinath]

It looks like the binding and member tests are now passing, so all that's left is the policy test.

[melinath]

/gcbrun

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 820 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 820 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 3 files changed, 304 insertions(+))
TF Validator: Diff ( 3 files changed, 304 insertions(+))

[modular-magician]

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccAssuredWorkloadsWorkload_BasicHandWritten|TestAccAssuredWorkloadsWorkload_FullHandWritten|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeForwardingRule_update|TestAccComputeInstanceFromMachineImage_basic|TestAccComputeInstanceFromMachineImage_overrideMetadataDotStartupScript|TestAccComputeInstanceFromMachineImage_diffProject|TestAccComputeRegionHealthCheck_tcp_update|TestAccComputeRegionHealthCheck_typeTransition|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccComputeServiceAttachment_serviceAttachmentBasicExampleUpdate|TestAccDataprocWorkflowTemplate_basic|TestAccEventarcTrigger_BasicHandWritten|TestAccMonitoringMonitoredProject_BasicMonitoredProject|TestAccOrgPolicyPolicy_EnforcePolicy|TestAccOrgPolicyPolicy_FolderPolicy|TestAccOrgPolicyPolicy_OrganizationPolicy|TestAccOrgPolicyPolicy_ProjectPolicy|TestAccPrivatecaCertificateTemplate_BasicCertificateTemplate|TestAccSqlUser_postgresIAM|TestAccTags You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=221386

[melinath]

Run of just the IAM tests: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstream/221390

[melinath]

LGTM