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

Fix panics during preview when metadata is a computed value #572

Merged
merged 2 commits into from
May 29, 2019

Conversation

ellismg
Copy link
Contributor

@ellismg ellismg commented May 23, 2019

During preview, an object's metadata bag may be computed (or be known
but contain values which are computed). This could happen, for
example, by using apply to take an output property from a yet to be
created resource and use it to build part of an object's metadata,
like we saw in #559.

In these cases, we incorrectly panic while attempting to extract out
the metadata.lables or metadata.annotations members of the metadata
object, when trying to set an annotation or label.

To fix this, we now treat requests to set annotations or labels as
no-ops if the metadata object is computed (or the label or annotation
values inside the metadata object are computed). This allows preview
to continue, as expected. During a real update, we will not have
computed values and so we will be able to correctly set the labels as
we expected.

Fixes #559

During preview, an object's metadata bag may be computed (or be known
but contain values which are computed). This could happen, for
example, by using `apply` to take an output property from a yet to be
created resource and use it to build part of an object's metadata,
like we saw in #559.

In these cases, we incorrectly panic while attempting to extract out
the metadata.lables or metadata.annotations members of the metadata
object, when trying to set an annotation or label.

To fix this, we now treat requests to set annotations or labels as
no-ops if the metadata object is computed (or the label or annotation
values inside the metadata object are computed). This allows preview
to continue, as expected. During a real update, we will not have
computed values and so we will be able to correctly set the labels as
we expected.

Fixes #559
Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes here look right to me - but would love @lblackstone or @hausdorff to take a look as well.

I will say that the rest of the code in these codepaths looks pretty brittle in a bunch of other ways (strong assumptions about the schema of these resource maps, which are strictly untyped and so cannot assume anything about them).

@ellismg
Copy link
Contributor Author

ellismg commented May 24, 2019

I will say that the rest of the code in these codepaths looks pretty brittle in a bunch of other ways (strong assumptions about the schema of these resource maps, which are strictly untyped and so cannot assume anything about them).

To address the above, @hausdorff @lblackstone, do you think it is reasonable to just implement logic in Check to ensure that the metadata variable always has some sort of decentish shape (is either not present, computed, or a property map) and similar to checks for labels and annotations?

@hausdorff
Copy link
Contributor

I don't think I understand what we mean when we talk about strong assumptions of the resource schemas here. That .metadata exists and has a specific type? I'm not sure what it accomplishes specifically to validate more about metadata than we already do?

My main concern is that I think we will find ourselves in this position again, but I'm not sure how to protect against it.

@lblackstone
Copy link
Member

I revised the tests a bit, but changes LGTM. Thanks @ellismg!

@lblackstone lblackstone merged commit eb593f2 into master May 29, 2019
@pulumi-bot pulumi-bot deleted the ellismg/fix-559 branch May 29, 2019 18:30
@ellismg
Copy link
Contributor Author

ellismg commented May 29, 2019

I don't think I understand what we mean when we talk about strong assumptions of the resource schemas here. That .metadata exists and has a specific type? I'm not sure what it accomplishes specifically to validate more about metadata than we already do?

Specifically, it would address issues like the following:

const cfgmap = new k8s.core.v1.ConfigMap("map", {
    data: {
        "foo": "bar",
        "baz": "qzr",
    },
    metadata: <any>"not-a-map",
}, {provider});

This will cause the provider to panic, because it assumes that /if/ the metadata property is not computed then it must be a map:

    panic: interface conversion: interface {} is string, not map[string]interface {}
    goroutine 55 [running]:
    github.com/pulumi/pulumi-kubernetes/pkg/metadata.SetAnnotation(...)
    	/home/matell/go/src/github.com/pulumi/pulumi-kubernetes/pkg/metadata/annotations.go:58
    github.com/pulumi/pulumi-kubernetes/pkg/metadata.SetAnnotationTrue(0xc0001360f8, 0x172259a, 0x14)
    	/home/matell/go/src/github.com/pulumi/pulumi-kubernetes/pkg/metadata/annotations.go:76 +0x21c
    github.com/pulumi/pulumi-kubernetes/pkg/metadata.AdoptOldNameIfUnnamed(0xc0001360f8, 0xc000119690)
    	/home/matell/go/src/github.com/pulumi/pulumi-kubernetes/pkg/metadata/naming.go:46 +0x18f

I think ideally we would have "check" fail here eagerly, since we know for this special property it has to be a computed value or a property map, even without consulting the API Spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic creating ConfigMap.
4 participants