pulumi / pulumi-yaml

YAML language provider for Pulumi
Apache License 2.0
39 stars 12 forks source link

YAML SDK does not honor schema-set Secrets #521

Closed guineveresaenger closed 10 months ago

guineveresaenger commented 11 months ago

What happened?

During the upgrade to pulumi-gcp to v7.0.0, we schematized a few new Outputs as Secret.

When running pulumi up on a YAML program, these Secrets were written to Outputs in plaintext.

This does not occur with the equivalent program in Typescript, or Go (which I verified.)

Example

name: dev
runtime: yaml
description: A minimal Google Cloud Pulumi YAML program
outputs:
  bucketName: ${my-bucket.url}
  pulumiAllLabels: ${my-bucket.pulumiLabels} # this should be Secret
  resourceSetLabels: ${my-bucket.labels} # this should be Secret
resources:
  # Create a GCP resource (Storage Bucket)
  my-bucket:
    properties:
      location: EU
      labels:
        good: morning
        bad: things
    type: gcp:storage:Bucket
    options:
      provider: ${gcp-provider}
  gcp-provider:
    type: pulumi:providers:gcp
    properties:
      defaultLabels:
        hello: goodbye
        new: defaultlabel
      project: ${project}

Output of pulumi preview

@guin:gcp-labels🦉 pulumi up
Previewing update (dev)

View in Browser (Ctrl+O): https://app.pulumi.com/guinevere-pulumi-corp/dev/dev/previews/905ab2de-9a79-459d-abf8-ebefbc9d0c36

     Type                     Name          Plan       
 +   pulumi:pulumi:Stack      dev-dev       create     
 +   ├─ pulumi:providers:gcp  gcp-provider  create     
 +   └─ gcp:storage:Bucket    my-bucket     create     

Outputs:
    bucketName       : output<string>
    pulumiAllLabels  : {
        bad  : "things"
        good : "morning"
        hello: "goodbye"
        new  : "defaultlabel"
    }
    resourceSetLabels: {
        bad : "things"
        good: "morning"
    }

Resources:
    + 3 to create

Do you want to perform this update?

Output of pulumi about

CLI          
Version      3.88.1
Go Version   go1.21.3
Go Compiler  gc

Plugins
NAME  VERSION
gcp   unknown (local plugin version)
yaml  unknown

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

justinvp commented 11 months ago

Offhand, I don't know if YAML ever supported this. It would have to get the schema from the provider to determine if an output for a resource was marked as a secret in the schema.

AaronFriel commented 10 months ago

There is shallow support in YAML for secrets and defaults on inputs, backed by schema, but I wasn't aware this was a feature of providers - outputs marked as secrets but not emitted as secret values.

I don't understand the issue though, maybe this belies my ignorance of another part of our SDKs though. It seems to me that the language host is on the wrong side of the language host <-> engine <-> provider RPC diagram to enforce this though.

If the provider returns a non-secret value that "should be" secret according to schema, how does the language host influence how the engine records the outputs in state?

AaronFriel commented 10 months ago

Ah, I knew something was off. This looks like a regression, introduced here:

cc @dixler @justinvp

https://github.com/pulumi/pulumi-yaml/commit/41c6b655e15da2f391d65aeca6b390703b34e274#diff-228d45f9d585e3feb8e035e1d905d4321203f645dd1a8be21da364c5699ad671R1576-R1599

Edit - only during preview?

AaronFriel commented 10 months ago

I think removing this:

https://github.com/pulumi/pulumi-yaml/blob/830ace173b9ecd4f50478c494493cac805a4ad58/pkg/pulumiyaml/run.go#L1594-L1617

And changing the behavior here will fix this:

https://github.com/pulumi/pulumi-yaml/blob/830ace173b9ecd4f50478c494493cac805a4ad58/pkg/pulumiyaml/run.go#L1641-L1648

I think the logic here is off, and I've only myself to blame - if the propertymap is missing a key rather than having an unknown property, x.ContainsUnknowns() will return false. Maybe the simplest fix is to have x.ContainsUnknowns() return true during previews?

That, or even more generally, property access should warn instead of error during preview and return unknowns on any evaluation error, letting the type checker catch invalid expressions. (edited)

iwahbe commented 10 months ago

There is shallow support in YAML for secrets and defaults on inputs, backed by schema, but I wasn't aware this was a feature of providers - outputs marked as secrets but not emitted as secret values.

I don't understand the issue though, maybe this belies my ignorance of another part of our SDKs though. It seems to me that the language host is on the wrong side of the language host <-> engine <-> provider RPC diagram to enforce this though.

If the provider returns a non-secret value that "should be" secret according to schema, how does the language host influence how the engine records the outputs in state?

The generated SDK makes the RegisterResource call with an augmented additionalSecretOutputs value to reflect schema secrets:

https://github.com/pulumi/pulumi-aws/blob/4d4bf52ce8bda75a5037d3be11a46f9fe868a14e/sdk/go/aws/waf/rule.go#L102-L105

YAML should do the same at runtime, but it seems like we are not doing so.

tgummerer commented 10 months ago

I don't think this is a regression actually. I just gave this a try with pulumi-yaml v1.1.0, which did not include https://github.com/pulumi/pulumi-yaml/commit/41c6b655e15da2f391d65aeca6b390703b34e274#diff-228d45f9d585e3feb8e035e1d905d4321203f645dd1a8be21da364c5699ad671R1576-R1599, and the same issue exists:

$ pulumi preview
Previewing update (dev)

View in Browser (Ctrl+O): https://app.pulumi.com/v-thomas-pulumi-corp/dev/dev/previews/904132e9-207f-4ffa-90c0-3a9bc4b600c5

     Type                     Name          Plan       Info
 +   pulumi:pulumi:Stack      dev-dev       create     1 warning
 +   ├─ pulumi:providers:gcp  gcp-provider  create     
 +   └─ gcp:storage:Bucket    my-bucket     create     

Diagnostics:
  pulumi:pulumi:Stack (dev-dev):
    warning: using pulumi-resource-gcp from $PATH at /home/tgummerer/work/pulumi-gcp/main/bin/pulumi-resource-gcp

Outputs:
    bucketName       : output<string>
    pulumiAllLabels  : {
        bad  : "things"
        good : "morning"
        hello: "goodbye"
        new  : "defaultlabel"
    }
    resourceSetLabels: {
        bad : "things"
        good: "morning"
    }

Resources:
    + 3 to create

Of course this is an issue that should still be fixed.

tgummerer commented 10 months ago

I believe https://github.com/pulumi/pulumi-yaml/pull/526 will fix this. I'm still looking into a test for that, and then it should be ready.

One doubt though, it looks like labels does not seem to be marked as secret by pulumi-gcp, only pulumiLabels is (and so is effectiveLabels). I tried in Go as well, and also go the same result, and looking around pulumi/pulumi-gcp that also seems to be the case. Am I missing something here?