Skip to content
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

Investigate TF-inexpressible resource config with unknowns #2032

Closed
VenelinMartinov opened this issue May 28, 2024 · 9 comments · Fixed by #2061
Closed

Investigate TF-inexpressible resource config with unknowns #2032

VenelinMartinov opened this issue May 28, 2024 · 9 comments · Fixed by #2061
Assignees
Labels
kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Milestone

Comments

@VenelinMartinov
Copy link
Contributor

What happened?

In TF, blocks can't be nil or unknown, as there is no way to specify that in HCL.

On the pulumi side we map blocks to either flat or collection parameters, depending on MaxItemsOne. These can then be specified as unknown in pulumi.

This is "unsupported" on the TF side so we should investigate if it works correctly and if there is a way to prevent that.

I suspect this isn't too big of a problem in the wild as nothing should output the block type but there might be uses with apply which output an unknown block

Example

resource myres "example" {
  block {
    val = "val"
  }
}

the block parameter of myres can not be specified as an unknown value here.

In pulumi this maps to either:

myRes("example", block=[MyResBlockArgs(val="val")])

or

myRes("example", block=MyResBlockArgs(val="val"))

both of which might be unknown, perhaps with out.apply(lambda x: MyResBlockArgs(val=x)

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).

@VenelinMartinov VenelinMartinov added needs-triage Needs attention from the triage team kind/bug Some behavior is incorrect or out of spec and removed needs-triage Needs attention from the triage team labels May 28, 2024
@VenelinMartinov VenelinMartinov changed the title Investigate TF-inexpressible resource config Investigate TF-inexpressible resource config with unkowns May 28, 2024
@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Jun 5, 2024

Investigated in #2054:

a few interesting cases:

TestPlanResourceChangeUnknowns/unknown_for_set_block_prop - here the whole set block is unknown and is returned back as a nil object.

TestPlanResourceChangeUnknowns/unknown_for_set_block_prop_collection - the whole collection here is unknown and is returned as a set with a nil object element, similar to above.

TestPlanResourceChangeUnknowns/unknown_for_set_prop - here the value is a set attribute with an unknown element but the value is returned as as an unknown set.

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Jun 5, 2024

For the first two cases it looks like the mistranslation happens in

return cty2hcty(objchange.ProposedNew(schema, priorC, configC)), nil

Proposed new returns a nil object instead of an unknown - I hacked it and the test now correctly returns an unknown for the test.

digging deeper it looks like it's objchange.ProposedNew which gets it wrong - it returns a "set_block_prop":cty.SetVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"prop":cty.NullVal(cty.String)})}) instead of a "set_block_prop":cty.SetVal([]cty.Value{cty.UnknownVal(cty.Object(map[string]cty.Type{"prop":cty.String}))}),

@VenelinMartinov VenelinMartinov self-assigned this Jun 5, 2024
@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Jun 5, 2024

I tried to repro TestPlanResourceChangeUnknowns/unknown_for_set_block_prop_collection in a program, as that seems to be a different root case than _prop.

Program:

"""A Google Cloud Python Pulumi program"""

import pulumi
import pulumi_gcp as gcp
import pulumi_random as random

random_string = random.RandomString(
    "random-string", length=8, special=True, upper=True, number=False
)

obj_arr = random_string.result.apply(
    lambda x: [gcp.storage.BucketCorArgs(origins=[x], methods=["GET"])]
)

# Create a GCP resource (Storage Bucket)
bucket = gcp.storage.Bucket("my-bucket", location="US", cors=obj_arr)

# Export the DNS name of the bucket
pulumi.export("bucket_name", bucket.url)

GRPC logs:

{
    "method": "/pulumirpc.ResourceProvider/Check",
    "request": {
        "urn": "urn:pulumi:dev::gcp_unknowns::gcp:storage/bucket:Bucket::my-bucket",
        "olds": {},
        "news": {
            "cors": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
            "location": "US"
        },
        "randomSeed": "ROwbJVHmCcN8pilnAHgl2qU626UEO6pWtcnBFUc63uY="
    },
    "response": {
        "inputs": {
            "__defaults": [
                "forceDestroy",
                "name",
                "storageClass"
            ],
            "cors": [
                {}
            ],
            "forceDestroy": false,
            "location": "US",
            "name": "my-bucket-8326b9a",
            "storageClass": "STANDARD"
        }
    },
    "metadata": {
        "kind": "resource",
        "mode": "client",
        "name": "gcp"
    }
},
{
    "method": "/pulumirpc.ResourceProvider/Create",
    "request": {
        "urn": "urn:pulumi:dev::gcp_unknowns::gcp:storage/bucket:Bucket::my-bucket",
        "properties": {
            "__defaults": [
                "forceDestroy",
                "name",
                "storageClass"
            ],
            "cors": [
                {}
            ],
            "forceDestroy": false,
            "location": "US",
            "name": "my-bucket-8326b9a",
            "storageClass": "STANDARD"
        },
        "preview": true
    },
    "response": {
        "properties": {
            "__meta": "{\"_new_extra_shim\":{\"location\":\"US\"},\"e2bfb730-ecaa-11e6-8f88-34363bc7c4c0\":{\"create\":600000000000,\"read\":240000000000,\"update\":240000000000}}",
            "autoclass": null,
            "cors": [
                {
                    "maxAgeSeconds": null,
                    "methods": null,
                    "origins": null,
                    "responseHeaders": null
                }
            ],
            "customPlacementConfig": null,
            "defaultEventBasedHold": null,
            "effectiveLabels": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
            "enableObjectRetention": null,
            "encryption": null,
            "forceDestroy": false,
            "id": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
            "labels": null,
            "lifecycleRules": [],
            "location": "US",
            "logging": null,
            "name": "my-bucket-8326b9a",
            "project": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
            "projectNumber": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
            "publicAccessPrevention": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
            "pulumiLabels": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
            "requesterPays": null,
            "retentionPolicy": null,
            "rpo": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
            "selfLink": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
            "softDeletePolicy": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
            "storageClass": "STANDARD",
            "uniformBucketLevelAccess": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
            "url": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
            "versioning": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
            "website": "04da6b54-80e4-46f7-96ec-b56ff0331ba9"
        }
    },
    "metadata": {
        "kind": "resource",
        "mode": "client",
        "name": "gcp"
    }
}

We seem to discard the unknown collection of objects in check, so I couldn't repro the test case - it fails much earlier.

@VenelinMartinov
Copy link
Contributor Author

I'm investigating a fix for the _prop case in https://github.com/pulumi/pulumi-terraform-bridge/pull/2060/files

I've also added a repro for Check discarding the unknown collection of objects.

@VenelinMartinov
Copy link
Contributor Author

Ok, the "unknown array of obj" -> array of single empty obj is happening in makeTerraformUnknown:
It sets unknown for any required properties and misses the rest.

I think we can actually do better here - TF supports dynamic blocks, which AFAIK support unknown values, hence TF supports unknowns inside collections.

This seems to then get promoted to an unknown collection as evidenced by the _block_prop tests.

So in the case of unknown collections we could generate a collection of unknown instead of collection of empty.

@VenelinMartinov
Copy link
Contributor Author

Testing the last hypothesis in #2061

@t0yv0
Copy link
Member

t0yv0 commented Jun 5, 2024

ProposedNew is copied directly from TF though maybe an older version compared to opentofu so I strongly suspect editing it is going to take us further away from TF behavior, at least worth checking to x-compare.

makeTerraformUnknown is interesting it looks like a heuristic to deal with exactly this case and indeed maybe it can be refined. Of course for cases where makeTerraformUnknown kicks in we can't -compare because it's not expressible in TF.

@mikhailshilkov mikhailshilkov changed the title Investigate TF-inexpressible resource config with unkowns Investigate TF-inexpressible resource config with unknowns Jun 10, 2024
@VenelinMartinov
Copy link
Contributor Author

The original hypothesis here that in TF blocks can't be unknown is false, since TF supports dynamic blocks, which are indeed unknown during plan.

We did find some valuable improvements in our handling of unknowns in #2061 and this issue was repurposed for that.

@pulumi-bot
Copy link
Contributor

This issue has been addressed in PR #2061 and shipped in release v3.88.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants