[Bug]: Creation of compute Instances (and others) fails with `SetNew only operates on computed keys`

[JonathanO]

[Bug]: Creation of compute Instances (and others) fails with `SetNew only operates on computed keys`

[turkenf]

Hi @JonathanO,

Thank you for bringing up this issue, I can reproduce the issue with the provided information.

• For Instance.compute.gcp.upbound.io/v1beta1:

  - lastTransitionTime: "2024-03-11T21:07:55Z"
    message: 'observe failed: cannot compute the instance diff: failed to get *terraform.InstanceDiff:
      SetNew only operates on computed keys - zone is not one'
    reason: ReconcileError
    status: "False"
    type: Synced

• For Job.cloudscheduler.gcp.upbound.io/v1beta1:

  - lastTransitionTime: "2024-03-11T21:16:22Z"
    message: 'observe failed: cannot compute the instance diff: failed to get *terraform.InstanceDiff:
      SetNew only operates on computed keys - region is not one'
    reason: ReconcileError
    status: "False"
    type: Synced

logs:

2024-03-12T00:22:30+03:00	DEBUG	provider-gcp	Cannot observe external resource	{"controller": "managed/cloudscheduler.gcp.upbound.io/v1beta1, kind=job", "request": {"name":"job"}, "uid": "c35370b0-4ef1-41b6-8b00-647d803b8e13", "version": "3463", "external-name": "job", "error": "cannot compute the instance diff: failed to get *terraform.InstanceDiff: SetNew only operates on computed keys - region is not one", "errorVerbose": "SetNew only operates on computed keys - region is not one\nfailed to get *terraform.InstanceDiff\ngithub.com/crossplane/upjet/pkg/controller.(*terraformPluginSDKExternal).getResourceDataDiff\n\tgithub.com/crossplane/[email protected]/pkg/controller/external_tfpluginsdk.go:427\ngithub.com/crossplane/upjet/pkg/controller.(*terraformPluginSDKExternal).Observe\n\tgithub.com/crossplane/[email protected]/pkg/controller/external_tfpluginsdk.go:501\ngithub.com/crossplane/upjet/pkg/controller.(*terraformPluginSDKAsyncExternal).Observe\n\tgithub.com/crossplane/[email protected]/pkg/controller/external_async_tfpluginsdk.go:126\ngithub.com/crossplane/crossplane-runtime/pkg/reconciler/managed.(*Reconciler).Reconcile\n\tgithub.com/crossplane/[email protected]/pkg/reconciler/managed/reconciler.go:903\ngithub.com/crossplane/crossplane-runtime/pkg/ratelimiter.(*Reconciler).Reconcile\n\tgithub.com/crossplane/[email protected]/pkg/ratelimiter/reconciler.go:54\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile\n\tsigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:119\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\tsigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:316\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\tsigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\tsigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:227\nruntime.goexit\n\truntime/asm_arm64.s:1197\ncannot compute the instance diff\ngithub.com/crossplane/upjet/pkg/controller.(*terraformPluginSDKExternal).Observe\n\tgithub.com/crossplane/[email protected]/pkg/controller/external_tfpluginsdk.go:503\ngithub.com/crossplane/upjet/pkg/controller.(*terraformPluginSDKAsyncExternal).Observe\n\tgithub.com/crossplane/[email protected]/pkg/controller/external_async_tfpluginsdk.go:126\ngithub.com/crossplane/crossplane-runtime/pkg/reconciler/managed.(*Reconciler).Reconcile\n\tgithub.com/crossplane/[email protected]/pkg/reconciler/managed/reconciler.go:903\ngithub.com/crossplane/crossplane-runtime/pkg/ratelimiter.(*Reconciler).Reconcile\n\tgithub.com/crossplane/[email protected]/pkg/ratelimiter/reconciler.go:54\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile\n\tsigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:119\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\tsigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:316\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\tsigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\tsigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:227\nruntime.goexit\n\truntime/asm_arm64.s:1197"}
2024-03-12T00:22:30+03:00	DEBUG	events	cannot compute the instance diff: failed to get *terraform.InstanceDiff: SetNew only operates on computed keys - region is not one	{"type": "Warning", "object": {"kind":"Job","name":"job","uid":"c35370b0-4ef1-41b6-8b00-647d803b8e13","apiVersion":"cloudscheduler.gcp.upbound.io/v1beta1","resourceVersion":"3463"}, "reason": "CannotObserveExternalResource"}

[selfuryon]

For already created resources it's the same:

Events:                                                                                                                                                                                                          
  Type     Reason                         Age                 From                                                   Message                                                                                     
  ----     ------                         ----                ----                                                   -------                                                                                     
  Warning  CannotObserveExternalResource  27s (x36 over 30m)  managed/compute.gcp.upbound.io/v1beta1, kind=instance  cannot compute the instance diff: failed to get *terraform.InstanceDiff: SetNew only operate
s on computed keys - zone is not one    

I faced with the problem on these resources:

• instance.compute.gcp.upbound.io
• instancegroup.compute.gcp.upbound.io

[ulucinar]

We have investigated the root cause of this issue with @sergenyalcin. upjet's config.MarkAsRequired works by manipulating the native Terraform schema of a resource as follows:

func MarkAsRequired(sch *schema.Resource, fieldpaths ...string) {
	for _, fieldpath := range fieldpaths {
		if s := GetSchema(sch, fieldpath); s != nil {
			s.Computed = false
			s.Optional = false
		}
	}
}

Setting schema.Schema.Computed and schema.Schema.Optional forces the corresponding field in the generated CRD to be a required field because upjet's code generation pipelines check these two properties when deciding whether a field in the generated CRD should be a required field.

It turns out that interfering with the native schema has adverse affects here. The GCP project and zone defaulting functions introduced as CustomizeDiff functions for the Terraform resource make calls to the schema.ResourceDiff.SetNew, which in turn asserts the schema of the attribute being set so that it's a computed value.

However, MarkAsRequired's modification on the native schema breaks this check and results in the error message reported in this issue. These defaulting CustomizeDiff functions were not being used in the previous Terraform provider version we consumed (v4.77.0) before the v1.0.0 release bumped it to v5.19.0. So, these bugs silently creeped in with the v1.0.0 release, and are especially dangerous.

We had better get rid of all the Terraform schema modifications as they can potentially cause harmful interference with the Terraform runtime like this issue demonstrates. We have plans to remove the Terraform plugin SDK from upjet's code generation pipelines, which will enforce this. Till then, we must be very careful (and avoid as much as possible) modifying the Terraform schema for managed resource configuration.