search

crossplane-contrib / provider-upjet-gcp

Official GCP Provider for Crossplane by Upbound.
Apache License 2.0
64 stars 71 forks source link

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

Closed JonathanO closed 6 months ago

JonathanO commented 6 months ago

Is there an existing issue for this?

Affected Resource(s)

Resource MRs required to reproduce the bug

examples/compute/instance.yaml examples/cloudscheduler/job.yaml

Steps to Reproduce

Apply examples/compute/instance.yaml (or examples/cloudscheduler/job.yaml)

What happened?

External resource was not created, reconciliation fails with an error from terraform: observe failed: cannot compute the instance diff: failed to get *terraform.InstanceDiff: SetNew only operates on computed keys - zone is not one (For Job it's ... - region is not one)

Relevant Error Output Snippet

No response

Crossplane Version

1.15.0

Provider Version

1.0.0

Kubernetes Version

No response

Kubernetes Distribution

No response

Additional Info

The failing calls to SetNew appear to have been introduced in https://github.com/GoogleCloudPlatform/magic-modules/pull/8569

turkenf commented 6 months ago

Hi @JonathanO,

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

selfuryon commented 6 months ago

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:

ulucinar commented 6 months ago

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.