Fix panics in applyTags when tags are unknown #2776

t0yv0 · 2023-09-01T20:02:19Z

We detected an issue in pulumi-eks test suite against the latest v6.0.3 release. This theoretically could affects users that set resource and or provider tags to unknown Outputs, although I tried and failed to reproduce outside of the pulumi-eks test.

The fix for #1655 included reimplementing tag merging (computing effective tags from the set of provider-level tags and resource-level tags) on Pulumi side as a callback to the Check Pulumi gRPC method. This works against resource.PropertyValue model and did not fully account for receiving a Computed when tags are unknown, resulting in a panic in the eks test.

The fix puts some combinators in place to shield resource.PropertyValue transformations from observing Unknowns, Computeds, and first-class Outputs. Note that this is a bit forward looking since this is a bridged provider, and as such on current version of tf-bridge it opts into a simplified version of the wire protocol where for example first-class Output values should not be passed to the bridge code. So it's not strictly the simplest fix, but hopefully this can be useful down the line.

I'd like to consider upstreaming propertyvalue.TransformErr (should it be called Walk in Go?) and similar combinators to pulumi/pulumi at some point if they prove useful.

github-actions · 2023-09-01T20:13:45Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

provider/tests/provider_test.go

iwahbe · 2023-09-01T21:59:27Z

provider/tags.go

+	}
+	// If there are 0 tags, delete the tags entry rather than sending an empty map. The unknown
+	// case is quirky though, prefer to send the unknown marker out rather than deleting it.
+	if !hasTags && !allTags.ContainsUnknowns() {


I think this is equally correct, and simpler to comprehend. It allows us to remove hasTags.

Suggested change

if !hasTags && !allTags.ContainsUnknowns() {

if allTags.IsNil() && !allTags.ContainsUnknowns() {

Ah but it also can be an Output{Element: nil, Dependencies}, among other things.

Well you're right. IsNil() is the only case we truly want to elide. The condition should just be a nil-check

iwahbe · 2023-09-01T22:00:24Z

provider/tags_test.go

+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"pgregory.net/rapid"


iwahbe · 2023-09-01T22:03:11Z

provider/pvutil.go

+// Computed, or Output values. All the metadata bits about these is floated to top-level and
+// re-applied to the value the user code receives out of composePropertyValue.


I think this is fine. It means that tags: { "foo": "bar", "secret": [secret] } will come out as [secret]. I doubt that this will be a problem in practice. I don't want to block merging on it.

Hmm you're right. We're losing precision. Unlike what happens in the restricted CheckConfigure, there are true nested secrets here. That's bad especially when merging is not even involved, like empty provider tags + secret resource tags. Hm.. I may do a quick fix to this by keeping a track of secret string hashes and re-secreting matching in the output.

t0yv0 · 2023-09-02T01:19:57Z

Verifying secret handling for tags at integration level, it's not what you'd expect.

First off all if you pass secrets, the provider unwraps the secrets and stores them in tagsAll property. So the protections are defeated.

Second if you start from a program that does not have secrets, and you mark a resource input such as tags secret, you don't get an update plan..... I think we need to trace this down as this is more generic than tags. It might be an issue with providers per se.

Third. Secrets never reach the code edited here.

import * as pulumi from "@pulumi/pulumi";
import * as aws from "@pulumi/aws";
import * as awsx from "@pulumi/awsx";

const b2 = new aws.s3.Bucket("my-bucket-40", {}, {});

// Create an AWS provider for the us-east-1 region.
let useast1 = new aws.Provider("useast1", {
    region: "us-east-1",
    defaultTags: {tags: {"this": b2.arn }},
});


// Create an AWS resource (S3 Bucket)
const bucket = new aws.s3.Bucket(
    "my-bucket",
    {tags: {"that": pulumi.secret(b2.arn)}},
    {provider: useast1});

// Export the name of the bucket
export const bucketName = bucket.id;

// interesting doe adding a <secret> wrapper not update ?

Gives

{
  "method": "/pulumirpc.ResourceProvider/Check",
  "request": {
    "urn": "urn:pulumi:dev::repro-aws-tagpanic::aws:s3/bucket:Bucket::my-bucket",
    "olds": {},
    "news": {
      "tags": {
        "that": "arn:aws:s3:::my-bucket-40-f07f095"
      }
    },
    "randomSeed": "25PggOkN2OdDhrlXlBtn2O4buWU6YkMyRY0F9JfeExw="
  },
  "response": {
    "inputs": {
      "__defaults": [
        "acl",
        "bucket",
        "forceDestroy"
      ],
      "acl": "private",
      "bucket": "my-bucket-d35e552",
      "forceDestroy": false,
      "tags": {
        "__defaults": [],
        "that": "arn:aws:s3:::my-bucket-40-f07f095",
        "this": "arn:aws:s3:::my-bucket-40-f07f095"
      }
    }
  },
  "metadata": {
    "kind": "resource",
    "mode": "client",
    "name": "aws"
  }
}

But in the state we get:

                    "tags": {
                        "that": {
                            "4dabf18193072939515e22adb298388d": "1b47061264138c4ac30d75fd1eb44270",
                            "ciphertext": "AAABAM8fGIsN3ZQv0HSv4nfSh0oCujcIdKGLU6lEgZHtfFIvmXRZt4tCv8XlUfAtJZSaYOTaMZHKm2TgBuBSiraF7w=="
                        },
                        "this": "arn:aws:s3:::my-bucket-40-f07f095"
                    },
                    "tagsAll": {
                        "that": "arn:aws:s3:::my-bucket-40-f07f095",
                        "this": "arn:aws:s3:::my-bucket-40-f07f095"
                    },

Squinting at this, the engine must have applied some secrets handling so the provider did not even get exposed to it.

t0yv0 · 2023-09-02T01:49:45Z

OK. I poked around some more. First-class outputs or nested computed values or secrets never reach this code, they never reach Check.. This is all the idiosyncratic behavior of the bridge.

The only place is CheckConfig.

The repro is:

const b2 = new aws.s3.Bucket("my-bucket-40", {}, {});

// Create an AWS provider for the us-east-1 region.
let useast1 = new aws.Provider("useast1", {
    region: "us-east-1",
    defaultTags: {tags: {"this": b2.id }},
});

IN this case we get the unknown sentinel.

{
  "method": "/pulumirpc.ResourceProvider/CheckConfig",
  "request": {
    "urn": "urn:pulumi:dev::repro-aws-tagpanic::pulumi:providers:aws::useast1",
    "olds": {},
    "news": {
      "defaultTags": "04da6b54-80e4-46f7-96ec-b56ff0331ba9",
      "region": "us-east-1",

Note that the "this" structure is destroyed already, it's one big unknown. Unfortunately, PreCheckCallback gets this down because it recalls the map from CheckConfig and gets sent it.. So this is where we get the unknown from. This is the only realistic case that can happen given how current bridge + pulumi CLI work.

t0yv0 · 2023-09-02T02:17:03Z

Simplified based on the findings, PTAL.

iwahbe

LGTM

t0yv0 added 2 commits September 1, 2023 13:32

Build a rapid model of possible combinations

c972223

Fixes and more tests

953fed7

Add gRPC level test; fix unk propagation

d5b0692

t0yv0 commented Sep 1, 2023

View reviewed changes

provider/tests/provider_test.go Outdated Show resolved Hide resolved

More test cases

24dde92

t0yv0 marked this pull request as ready for review September 1, 2023 21:45

t0yv0 requested review from a team and iwahbe September 1, 2023 21:45

t0yv0 changed the title ~~Fix panics in apply tags~~ Fix panics in applyTags when tags are unknown Sep 1, 2023

t0yv0 requested a review from thomas11 September 1, 2023 21:47

iwahbe reviewed Sep 1, 2023

View reviewed changes

Move the test so it is tested by CI

38fca1d

Simplify based on findings

ae5f67e

t0yv0 requested a review from iwahbe September 2, 2023 02:16

iwahbe approved these changes Sep 2, 2023

View reviewed changes

t0yv0 merged commit e035d7c into master Sep 2, 2023
15 checks passed

t0yv0 deleted the t0yv0/fix-apply-tags-panics branch September 2, 2023 03:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panics in applyTags when tags are unknown #2776

Fix panics in applyTags when tags are unknown #2776

t0yv0 commented Sep 1, 2023 •

edited

Loading

github-actions bot commented Sep 1, 2023

iwahbe Sep 1, 2023

t0yv0 Sep 1, 2023

t0yv0 Sep 1, 2023

iwahbe Sep 1, 2023

iwahbe Sep 1, 2023

t0yv0 Sep 1, 2023

t0yv0 commented Sep 2, 2023

t0yv0 commented Sep 2, 2023

t0yv0 commented Sep 2, 2023

iwahbe left a comment

	if !hasTags && !allTags.ContainsUnknowns() {
	if allTags.IsNil() && !allTags.ContainsUnknowns() {

		// Computed, or Output values. All the metadata bits about these is floated to top-level and
		// re-applied to the value the user code receives out of composePropertyValue.

Fix panics in applyTags when tags are unknown #2776

Fix panics in applyTags when tags are unknown #2776

Conversation

t0yv0 commented Sep 1, 2023 • edited Loading

github-actions bot commented Sep 1, 2023

Does the PR have any schema changes?

Does the PR have any schema changes?

iwahbe Sep 1, 2023

Choose a reason for hiding this comment

t0yv0 Sep 1, 2023

Choose a reason for hiding this comment

t0yv0 Sep 1, 2023

Choose a reason for hiding this comment

iwahbe Sep 1, 2023

Choose a reason for hiding this comment

iwahbe Sep 1, 2023

Choose a reason for hiding this comment

t0yv0 Sep 1, 2023

Choose a reason for hiding this comment

t0yv0 commented Sep 2, 2023

t0yv0 commented Sep 2, 2023

t0yv0 commented Sep 2, 2023

iwahbe left a comment

Choose a reason for hiding this comment

t0yv0 commented Sep 1, 2023 •

edited

Loading