Add support for nodeConfig in ApigeeEnvironment

[xuchenma]

This PR solves https://github.com/hashicorp/terraform-provider-google/issues/12218

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

• [X] 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).
• [X] Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
• [X] 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).
• [X] Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
• [X] Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

Add support for nodeConfig in ApigeeEnvironment

[modular-magician]

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @rileykarson, a repository maintainer, has been assigned to assist you and help review your changes.

:question: First time contributing? Click here for more details

---

Your assigned reviewer will help review your code by:

• Ensuring it's backwards compatible, covers common error cases, etc.
• Summarizing the change into a user-facing changelog note.
• Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

---

[modular-magician]

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

Diff report:

Terraform GA: Diff ( 2 files changed, 145 insertions(+)) Terraform Beta: Diff ( 3 files changed, 305 insertions(+)) TF Validator: Diff ( 3 files changed, 54 insertions(+), 3 deletions(-))

[xuchenma]

Hi Riley, this API isn't public yet (will be there later this week) so the tests won't pass for now. Meanwhile I think this PR supports the POST and GET calls, but not the PATCH call. Could you shed some light on that? The PATCH call is

curl -s -H "$AUTH" "https://apigee.googleapis.com/v1/organizations/$ORG_ID/environments/$ENVIRONMENT_NAME?updateMask=node_config" \
  -X PATCH \
  -H "Content-Type:application/json" \
  -d '{                                                           
     "nodeConfig": {
        "minNodeCount":"'"$MIN_NODE_COUNT"'",
        "minNodeCount":"'"$MAX_NODE_COUNT"'",
     }
  }'

Thanks!

[modular-magician]

Tests analytics

Total tests: 2123 Passed tests 1889 Skipped tests: 227 Failed tests: 7

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccComputeInstance_soleTenantNodeAffinities|TestAccActiveDirectoryPeering_activeDirectoryPeeringBasicExample|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccActiveDirectoryDomain_activeDirectoryDomainBasicExample|TestAccActiveDirectoryDomain_update|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample

[modular-magician]

Tests passed during RECORDING mode: TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[view] TestAccCGCSnippet_eventarcWorkflowsExample[view]

Tests failed during RECORDING mode: TestAccActiveDirectoryDomain_activeDirectoryDomainBasicExample[view] TestAccCloudRunService_cloudRunServiceStaticOutboundExample[view] TestAccComputeInstance_soleTenantNodeAffinities[view] TestAccActiveDirectoryPeering_activeDirectoryPeeringBasicExample[view] TestAccActiveDirectoryDomain_update[view]

Please fix these to complete your PR View the build log or the debug log for each test

[rileykarson]

Sorry, lost email on my side. I'm OOO for a bit, so picked you a new reviewer from the lottery.

[c2thorn]

Hi Riley, this API isn't public yet (will be there later this week) so the tests won't pass for now. Meanwhile I think this PR supports the POST and GET calls, but not the PATCH call. Could you shed some light on that? The PATCH call is

curl -s -H "$AUTH" "https://apigee.googleapis.com/v1/organizations/[](https://apigee.googleapis.com/v1/organizations/)$ORG_ID/environments/$ENVIRONMENT_NAME?updateMask=node_config" \
  -X PATCH \
  -H "Content-Type:application/json" \
  -d '{                                                           
     "nodeConfig": {
        "minNodeCount":"'"$MIN_NODE_COUNT"'",
        "minNodeCount":"'"$MAX_NODE_COUNT"'",
     }
  }'

Thanks!

It seems that this is the first updatable field introduced to the resource. We can specify PATCH via update_verb: :PATCH. see https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/products/apigee/api.yaml#L329

[xuchenma]

Hi Riley, this API isn't public yet (will be there later this week) so the tests won't pass for now. Meanwhile I think this PR supports the POST and GET calls, but not the PATCH call. Could you shed some light on that? The PATCH call is

curl -s -H "$AUTH" "https://apigee.googleapis.com/v1/organizations/[](https://apigee.googleapis.com/v1/organizations/)$ORG_ID/environments/$ENVIRONMENT_NAME?updateMask=node_config" \
  -X PATCH \
  -H "Content-Type:application/json" \
  -d '{                                                           
     "nodeConfig": {
        "minNodeCount":"'"$MIN_NODE_COUNT"'",
        "minNodeCount":"'"$MAX_NODE_COUNT"'",
     }
  }'

Thanks!

It seems that this is the first updatable field introduced to the resource. We can specify PATCH via update_verb: :PATCH. see https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/products/apigee/api.yaml#L329

The additional complication is that, actually there is already a PUT call that can be used to update all the other fields (except for this nodeConfig) of this resource (ref: https://cloud.google.com/apigee/docs/reference/apis/apigee/rest/v1/organizations.environments/update), and this new "nodeConfig" can only be updated via PATCH. If PUT is used to updated "nodeConfig", or PATCH is used to update any field other than "nodeConfig", the call will fail. I'm not sure how this situation can be represented in magic-modules..

[c2thorn]

The additional complication is that, actually there is already a PUT call that can be used to update all the other fields (except for this nodeConfig) of this resource (ref: cloud.google.com/apigee/docs/reference/apis/apigee/rest/v1/organizations.environments/update), and this new "nodeConfig" can only be updated via PATCH. If PUT is used to updated "nodeConfig", or PATCH is used to update any field other than "nodeConfig", the call will fail. I'm not sure how this situation can be represented in magic-modules..

I see. I believe you can set the update_verb property on the field itself, and we have logic to move it to a separate call

[xuchenma]

Thanks, I added update_verb and update_url to the field, and generated a local provider. It seems that a code block that starts with if d.HasChange("node_config") { is generated here https://github.com/hashicorp/terraform-provider-google/blob/main/google/resource_apigee_environment.go#L300 However, a PUT call was fired prior to this, at line https://github.com/hashicorp/terraform-provider-google/blob/main/google/resource_apigee_environment.go#L285.

When I tested it locally and updated the nodeConfig, it failed with a PUT call failure. But when I commented out the PUT call (above line 285 and a few related lines), the PATCH was fired properly and the update succeeded.

Is there a way to not fire the PUT call when only the nodeConfig has changed?

[c2thorn]

Thanks, I added update_verb and update_url to the field, and generated a local provider. It seems that a code block that starts with if d.HasChange("node_config") { is generated here https://github.com/hashicorp/terraform-provider-google/blob/main/google/resource_apigee_environment.go#L300 However, a PUT call was fired prior to this, at line https://github.com/hashicorp/terraform-provider-google/blob/main/google/resource_apigee_environment.go#L285.

When I tested it locally and updated the nodeConfig, it failed with a PUT call failure. But when I commented out the PUT call (above line 285 and a few related lines), the PATCH was fired properly and the update succeeded.

Is there a way to not fire the PUT call when only the nodeConfig has changed?

Apologies, that was not expected. I'll have to take a look myself to see how to best handle this situation as it's fairly rare. Would you mind pushing the changes you had in MM locally that ended up failing? So I can pull the change and test things out on my end?

[xuchenma]

Sure, pushed, thanks!

[modular-magician]

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

Diff report:

Terraform GA: Diff ( 2 files changed, 175 insertions(+)) Terraform Beta: Diff ( 3 files changed, 335 insertions(+)) TF Validator: Diff ( 3 files changed, 54 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2135 Passed tests 1898 Skipped tests: 228 Failed tests: 9

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeInstance_networkPerformanceConfig|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccCloudFunctions2Function_fullUpdate|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample

[modular-magician]

Tests passed during RECORDING mode: TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample[view] TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample[view] TestAccCloudFunctions2Function_fullUpdate[view] TestAccCGCSnippet_eventarcWorkflowsExample[view] TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[view]

Tests failed during RECORDING mode: TestAccComputeInstance_soleTenantNodeAffinities[view] TestAccComputeInstance_networkPerformanceConfig[view] TestAccComputeGlobalForwardingRule_internalLoadBalancing[view] TestAccCloudRunService_cloudRunServiceStaticOutboundExample[view]

Please fix these to complete your PR View the build log or the debug log for each test

[c2thorn]

@xuchenma I took a look at this. What I didn't realize is that any PUT body needs to have the nodeConfig field present because the lack of nodeConfig apparently implies you are removing it (which is an update). Which means it's required in the PUT body but not allowed to be updated.

This is an API constraint that I don't believe we've run into before for MMv1 generated resources. My initial impression is that this should be fixed in the API itself. I imagine TF is likely one of the top consumers of the API, and this constraint is not helpful. Surely this must be an implementation issue and not an intended design decision? I don't know if you have any insight into that.

Attempting to work around this in MMv1 generation will be relatively unsatisfactory. How we have the PR now excludes nodeConfig from the PUT request. There is no way currently in MMv1 to avoid the primary PUT update request. The only way I can think to workaround is to add update_encoder logic that adds the old nodeConfig value to the PUT request, something like (untested)

old, _ = d.GetChange("node_config")
obj["nodeConfig"] = old

This should allow the PUT to succeed leading to the actual PATCH request after, but then we are running an unnecessary non-op PUT request when only node_config has changed. This should be a relatively uncommon situation, but is still unsatisfactory... let me know what you think.

The only non-compromising way to implement this would be to convert the resource to be completely handwritten with very specific custom logic to deal with this.

[c2thorn]

example an update_encoder

https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/templates/terraform/update_encoder/pubsub_topic.erb

reference to the file in terraform.yaml: https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/products/pubsub/terraform.yaml#L74

[c2thorn]

I believe the logic within the update encoder file will look like

old, _ = d.GetChange("node_config")
obj["nodeConfig"] = old
return obj, nil

[modular-magician]

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

Diff report:

Terraform GA: Diff ( 2 files changed, 188 insertions(+), 5 deletions(-)) Terraform Beta: Diff ( 3 files changed, 348 insertions(+), 5 deletions(-)) TF Validator: Diff ( 3 files changed, 54 insertions(+), 3 deletions(-))

[xuchenma]

Thanks Cameron! I made the changes and it worked for me locally. Please check.

There are a few complications:

1. The PUT call is not an async call. The current implementation fails when any meaningful PUT is called, because it tries to check the "async" operation status based on operation_id, but the PUT call doesn't return one. This is a separate bug but I fixed it all together here, by checking async only for "create" and "delete" on ApigeeEnvironment. It would be better if we can add async status check also for the PATCH only.
2. The call resourceApigeeEnvironmentUpdateEncoder is generated before both PUT and PATCH. It should only update obj["nodeConfig"] for PUT, so I put a check at the top.
3. There are some type conversions that involve []interface{} and map[string]interface{}.
4. current_aggregate_node_count is output-only and need to be removed, before obj["nodeConfig"] can be served as input.

[modular-magician]

Tests analytics

Total tests: 2138 Passed tests 1891 Skipped tests: 229 Failed tests: 18

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_networkPerformanceConfig|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCloudFunctions2Function_fullUpdate|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccSqlDatabaseInstance_authNets|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccSqlDatabaseInstance_settings_upgrade|TestAccSqlDatabaseInstance_basic_with_user_labels|TestAccSqlDatabaseInstance_settings_deletionProtection|TestAccSqlDatabaseInstance_basicMSSQL|TestAccSqlDatabaseInstance_PointInTimeRecoveryEnabled|TestAccSqlDatabaseInstance_SqlServerAuditConfig

[modular-magician]

Tests passed during RECORDING mode: TestAccFirebaserulesRelease_BasicRelease[view] TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample[view] TestAccCloudFunctions2Function_fullUpdate[view] TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample[view] TestAccCGCSnippet_eventarcWorkflowsExample[view] TestAccSqlDatabaseInstance_authNets[view] TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRangeClone[view] TestAccSqlDatabaseInstance_settings_upgrade[view] TestAccSqlDatabaseInstance_basic_with_user_labels[view] TestAccSqlDatabaseInstance_settings_deletionProtection[view] TestAccSqlDatabaseInstance_basicMSSQL[view] TestAccSqlDatabaseInstance_PointInTimeRecoveryEnabled[view]

Tests failed during RECORDING mode: TestAccComputeInstance_networkPerformanceConfig[view] TestAccComputeGlobalForwardingRule_internalLoadBalancing[view] TestAccComputeInstance_soleTenantNodeAffinities[view] TestAccCloudRunService_cloudRunServiceStaticOutboundExample[view] TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[view] TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange[view] TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRangeReplica[view] TestAccSqlDatabaseInstance_SqlServerAuditConfig[view]

Please fix these to complete your PR View the build log or the debug log for each test

[modular-magician]

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

Diff report:

Terraform GA: Diff ( 3 files changed, 189 insertions(+), 5 deletions(-)) Terraform Beta: Diff ( 4 files changed, 524 insertions(+), 5 deletions(-)) TF Validator: Diff ( 3 files changed, 54 insertions(+), 3 deletions(-))

[xuchenma]

Thanks for the review! I added a test for nodeConfig update; in my local I can see the test is calling PATCH correctly. However as I mentioned in the above comment https://github.com/GoogleCloudPlatform/magic-modules/pull/6349#issuecomment-1220080526, PUT is a sync call, but PATCH is an async call. Because I removed the async check for update (otherwise PUT is broken), in the test, after calling PATCH, it immediately calls GET to verify the state, while the state has not changed yet. Is there a way to add async check for the PATCH only?

[modular-magician]

Tests analytics

Total tests: 2142 Passed tests 1901 Skipped tests: 230 Failed tests: 11

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccComputeInstance_networkPerformanceConfig|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccCloudFunctions2Function_fullUpdate|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCGCSnippet_eventarcWorkflowsExample

[modular-magician]

Tests passed during RECORDING mode: TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample[view] TestAccCloudFunctions2Function_fullUpdate[view] TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample[view] TestAccCGCSnippet_eventarcWorkflowsExample[view]

Tests failed during RECORDING mode: TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange[view] TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[view] TestAccSqlDatabaseInstance_SqlServerAuditConfig[view] TestAccComputeInstance_networkPerformanceConfig[view] TestAccComputeInstance_soleTenantNodeAffinities[view] TestAccCloudRunService_cloudRunServiceStaticOutboundExample[view] TestAccComputeGlobalForwardingRule_internalLoadBalancing[view]

Please fix these to complete your PR View the build log or the debug log for each test

[c2thorn]

@xuchenma I see, sorry for not catching that in your earlier message. we are really trying to stretch the MMv1 template with this logic here, and I am getting close to just recommending you to copy the generated files and convert the resource to handwritten so you can tweak the go code specifically.

However, there is another attempt I can think of. This is the resource template where we write the logic for fields with custom updates: https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/templates/terraform/resource.erb#L702 You'll notice there is no logic to try to do asynchronous calls. To try to shoehorn this logic into the template (without changing the template, as that could cause more disruption than just switching to handwritten), I think we can change the resource's update verb to PATCH and flip every other field to use a custom PUT verb. That way, PATCH gets the async logic, and PUT is handled separately. What do you think?

[c2thorn]

Hi @xuchenma, I have spoken with Riley and he is also open to moving this to a handwritten resource if needed. To be frank, I cannot be 100% confident that my previous comment would also not produce some sort of complication as I have been wrong up to this point.

I apologize for the amount of iterations this has caused you, but this really is behavior we have not run into otherwise. It's up to you if you want to explore my earlier suggestion, but know that converting to handwritten will give you full control of the go code and let you tweak the update function to exactly what is needed here.

You would not be starting from scratch as you could re-use the majority of the generated code as shown in https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-6349-old..auto-pr-6349 . There's some extra steps like manually registering the resource into https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/utils/provider.go.erb#L377

[xuchenma]

Thanks Cameron and Riley. I tried to change the update_verb of ApigeeEnvironment resource to PATCH just now, it seems to be working for nodeConfig. However, setting update_verb to PUT for other fields doesn't seem to generate another PUT codeblock in resource_apigee_environment.go, so there could be more complications.

But as I look deeper, I think with our current implementation for ApigeeEnvironment in mmv1, we can't really make a valid update call. All the fields except for "name" is annotated with "input: true" which results in ForceNew when its value changes, while making changes to "name" is not allowed in the API backend (we should add "input: true" to "name" too). So, effectively no update call can be made to ApigeeEnvironment. Although the API supports updating some of the fields like "properties", such field is not added to mmv1 yet.

I think we should go for handwritten resource when we need to support PUT updates to ApigeeEnvironment for new fields, but right now we can just set the update_verb of the resource as PATCH since it is not a breaking change. What do you think?

[c2thorn]

I think we should go for handwritten resource when we need to support PUT updates to ApigeeEnvironment for new fields, but right now we can just set the update_verb of the resource as PATCH since it is not a breaking change. What do you think?

That sounds reasonable for now. There's a chance the PUT update_verb will actually work correctly when we add an updatable field as well.