-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
google_compute_instance_template.advanced_machine_features erroneously marked as computed #9436
google_compute_instance_template.advanced_machine_features erroneously marked as computed #9436
Comments
@karlkfi tested the api behavior.
It is true, any changes needed for |
@edwardmedia: Grabbing this bug, since I'm onduty & have been talking w/ @karlkfi about it offline as well. The error caused by KCC does not occur when using Terraform, I'll keep the bug open for a bit in case we decide to amend the behaviour of the field (although it's likely a breaking change to do so) |
Yes, but I don't think this is desirable behavior. Example 1Removing values that aren't default should trigger re-create.
to
This SHOULD cause an update/recreate, because it's not the default in the API and has explicit meaning. But it actually ignores the change.
Example 2Removing the default values should be a noop.
to
This SHOULD be a noop, because it's equivalent to the default. But it's actually a noop because removal of
Example 3Adding the default values should be a noop.
to
This SHOULD be a noop, because it's equivalent to the default. But it triggers a recreate.
Example 4Adding the default values should be a noop.
to
This SHOULD be a noop, because it's equivalent to the default. But it triggers a recreate.
FixRemoving I'm not sure why Computed was set on |
The tf i used for the above tests was just this. Change the project and you can test yourself.
|
Karl, I initially tried those permutations for Has that bug in 4849 been fixed yet? |
GoogleCloudPlatform/magic-modules#4849 looks like it also specifies I suggest changing it and adding more tests to validate the behavior works as desired for these use cases. |
Oh, I see. You reported a bug in the docs/api not agreeing about the field being mutable. I don't have an snwer for that, but I can try to follow up. The issue in this ticket is about unexpected behavior in the TF provider from your impl in #9363 Example 1 is the worst offender. It ignores a change when it should cause an update/recreate. The other examples here are minor issues for TF users, because the template itself is immutable, but have caused another issue in Config Connector, which depends on the TF provider code. |
Speaking about changing the instance template schema: Even though the impact is likely to be minor, removing the @karlkfi: Those are all well-known behaviours of marking a field Note that Config Connector uses a private (unsupported) interface to interact with the provider, and that issues for Config Connector are not always issues for Terraform, such as this case. The configuration of the field(s) in #9363 are valid for a Terraform field and Config Connector should handle them correctly. In fact, it's entirely possible that there is a configuration of the field that requires |
Do we have an example where any of these fields are computed by the API, for either instances or instance templates? Checking the responses from the API (using I didn't test every permutation, but here are a few examples. I got the same results from both instances and instance templates. Below, we see that for instances,
Below, we see that for instance templates,
Below, we see that for instances
Below, we see that for instance
Below, we see that for instance templates
|
@slevenick: marking |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Community Note
modular-magician
user, it is either in the process of being autogenerated, or is planned to be autogenerated soon. If an issue is assigned to a user, that user is claiming responsibility for the issue. If an issue is assigned tohashibot
, a community member has claimed the issue already.Terraform Version
terraform-provider-google v3.73.0
This version JUST added the
advanced_machine_features
field. So no other versions are affected yet.Affected Resource(s)
Terraform Configuration Files
Example used to test KCC (does not specify AdvancedMachineFeatures, because it was only just added) :
Error Output
This error is from the Config Connector integration tests after pulling in the new terraform-provider-google v3.73.0 code as dependency.
However, no changes were made to the advancedMachineFeatures field, because it's not even being specified by the existing integration tests (new field).
Expected Behavior
The test should pass, because the advancedMachineFeatures field is not being specified by the client nor changed.
Actual Behavior
The test fails, with the error:
cannot make changes to immutable field(s): [advancedMachineFeatures.#]
.Terraform Provider Google would probably always trigger an unnecessary re-create as long as
advanced_machine_features
is empty and an update was performed.TODO: Validate the actual error behavior when using TF without KCC.
Probable Root Cause
The
advanced_machine_features
field is marked asComputed: true
andForceNew: true
. So when calculating the diff, if the new value is empty, the diff setsRequiresNew: true
, which triggers aImmutableFieldsMutationError
error from Config Connector, even if nothing changed.https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/schema.go#L394
References
advanced_machine_features
to GCE Instance Templates #9363The text was updated successfully, but these errors were encountered: