pulumi / pulumi-terraform-bridge

A library allowing Terraform providers to be bridged into Pulumi.
Apache License 2.0
191 stars 43 forks source link

Tfgen does not respect fully computed on nested object properties #2178

Open VenelinMartinov opened 1 month ago

VenelinMartinov commented 1 month ago

What happened?

In https://github.com/pulumi/pulumi-gcp/issues/2096 it was discovered that Computed + !Optional + !Required is not respected on nested object properties in tfgen.

The schemas is generated with the property as settable by the user:

  "gcp:compute/InstanceAttachedDisk:InstanceAttachedDisk": {
      "properties": {
          "deviceName": {
              "type": "string",
              "description": "Name with which the attached disk will be accessible\nunder `/dev/disk/by-id/google-*`\n"
          },
          "diskEncryptionKeyRaw": {
              "type": "string",
              "description": "A 256-bit [customer-supplied encryption key]\n(https://cloud.google.com/compute/docs/disks/customer-supplied-encryption),\nencoded in [RFC 4648 base64](https://tools.ietf.org/html/rfc4648#section-4)\nto encrypt this disk. Only one of `kms_key_self_link` and `disk_encryption_key_raw` may be set.\n",
              "secret": true
          },
          "diskEncryptionKeySha256": {
              "type": "string",
              "description": "The [RFC 4648 base64](https://tools.ietf.org/html/rfc4648#section-4)\nencoded SHA-256 hash of the [customer-supplied encryption key]\n(https://cloud.google.com/compute/docs/disks/customer-supplied-encryption) that protects this resource.\n"
          },
          "kmsKeySelfLink": {
              "type": "string",
              "description": "The self_link of the encryption key that is\nstored in Google Cloud KMS to encrypt this disk. Only one of `kms_key_self_link`\nand `disk_encryption_key_raw` may be set.\n"
          },
          "mode": {
              "type": "string",
              "description": "Either \"READ_ONLY\" or \"READ_WRITE\", defaults to \"READ_WRITE\"\nIf you have a persistent disk with data that you want to share\nbetween multiple instances, detach it from any read-write instances and\nattach it to one or more instances in read-only mode.\n"
          },
          "source": {
              "type": "string",
              "description": "The name or self_link of the disk to attach to this instance.\n"
          }
      },
      "type": "object",
      "required": [
          "source"
      ],
      "language": {
          "nodejs": {
              "requiredOutputs": [
                  "deviceName",
                  "diskEncryptionKeySha256",
                  "kmsKeySelfLink",
                  "source"
              ]
          }
      }
  },

Example

.

Output of pulumi about

.

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction. To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

t0yv0 commented 1 month ago

That's a good find, probably would be breaking to roll this out but in a good way (emphasize at compile time that programs are doing something wrong). There is a chance from what I remember that TF would panic or otherwise fail at runtime if a user attempts to set these. If this is true, we can fix it without worrying about breaking program compilation because the programs that would be affected are already broken at runtime, just need good changelog discipline.

VenelinMartinov commented 1 month ago

Yeah, TF does panic at runtime if the user tries to set a fully computed property, so this change should be entirely safe to make.

The tricky bit here is that the types are shared between inputs and outputs currently, which is how we ended up with the Computed properties in the input types.