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

unknown StringOutput Project on NewProvider() causes potential unwanted deletion/modification of many resources #2539

Closed
pdemagny opened this issue Oct 21, 2024 · 5 comments
Assignees
Labels
kind/question Questions about existing features resolution/fixed This issue was fixed

Comments

@pdemagny
Copy link

pdemagny commented Oct 21, 2024

Describe what happened

After an upgrade of this provider and without any changes in my codebase (can't remember exactly but it was around may I think), I started having very weird previews:

I would get a lot of warnings:

warning: The provider for this resource has inputs that are not known during preview.
This preview may not correctly represent the changes that will be applied during an update.

And lots of values would end up unknown again:

"pdemagny-postgres-blobr-ai" => output<string>

And lots of occurences in the diff of:

                    - __defaults: []

And other randomly removed properties on resources ...

Sample program

I have this package that is overly complex for what it does, but it looks like this to maintain consistency with all our other packages in our codebase that all implements the Golang Functional Options Pattern.

import (
    "http://github.com/kr/pretty"
    googlecloud "http://github.com/pulumi/pulumi-gcp/sdk/v7/go/gcp"
    "github.com/pulumi/pulumi/sdk/v3/go/pulumi"
)

type (
    ResourceArgs struct {
        Name      string
        ProjectId pulumi.StringOutput
        Region    string
    }
    GcpProviderOptions func(*GcpProviderArgs)
    GcpProviderArgs    struct {
        Context         *pulumi.Context
        ResourceOptions []pulumi.ResourceOption
        Debug           bool
        ResourceArgs    ResourceArgs
    }
)

func WithContext(ctx *pulumi.Context) GcpProviderOptions {
    return func(p *GcpProviderArgs) { p.Context = ctx }
}

func WithResourceOptions(resourceOptions []pulumi.ResourceOption) GcpProviderOptions {
    return func(p *GcpProviderArgs) { p.ResourceOptions = resourceOptions }
}

func WithDebug(debug bool) GcpProviderOptions {
    return func(p *GcpProviderArgs) { p.Debug = debug }
}

func WithName(name string) GcpProviderOptions {
    return func(p *GcpProviderArgs) { p.ResourceArgs.Name = name }
}

func WithRegion(region string) GcpProviderOptions {
    return func(p *GcpProviderArgs) { p.ResourceArgs.Region = region }
}

func WithProjectId(projectId pulumi.StringOutput) GcpProviderOptions {
    return func(i *GcpProviderArgs) { i.ResourceArgs.ProjectId = projectId }
}

func NewProvider(opts ...GcpProviderOptions) *GcpProviderArgs {
    // Default values
    obj := GcpProviderArgs{
        Debug: false,
    }
    for _, opt := range opts {
        opt(&obj)
    }
    if obj.Debug {
        ctx := obj.Context
        ctx.Log.Info("gcp/provider.NewProvider:", nil)
        ctx.Log.Info(pretty.Sprint(&obj.ResourceArgs), nil)
    }
    return &obj
}

func (p *GcpProviderArgs) Create() (*googlecloud.Provider, error) {
    return googlecloud.NewProvider(p.Context, p.ResourceArgs.Name, &googlecloud.ProviderArgs{
        Region:  pulumi.String(p.ResourceArgs.Region),
        Project: p.ResourceArgs.ProjectId,
    })
}

As i said I can't find out what version it was, but a version of the Pulumi GCP Provider required me to add the Project option to the NewProvider() method.
But it doesn't seem to be a hard requirement, so when I create the provider like this:

gcpEuropeWest1Provider, err := gcpprovider.NewProvider(
	gcpprovider.WithContext(ctx),
	gcpprovider.WithDebug(true),
	gcpprovider.WithName("gcp-europewest1-provider"),
	gcpprovider.WithRegion(gcpRegion),
).Create()
if err != nil {
	return err
}

It causes warnings and ultimately errors and panic (same result with both pulumi cli & automation api):

Diagnostics:
  gcp:organizations:Folder (test-engineering):
    warning: The provider for this resource has inputs that are not known during preview.
    This preview may not correctly represent the changes that will be applied during an update.

  gcp:organizations:Project (test-prod-org1):
    warning: The provider for this resource has inputs that are not known during preview.
    This preview may not correctly represent the changes that will be applied during an update.

And it goes like this for all resources of this provider.

If I try to apply anyway:

panic: fatal: An assertion has failed: Update cannot be called if the configuration is unknown

When I remove only the Region instead of the Project, i get the expected:

error: rpc error: code = Unknown desc = expected a non-empty string: region was set to ``

Finally When I create the provider like this:

gcpEuropeWest1Provider, err := gcpprovider.NewProvider(
	gcpprovider.WithContext(ctx),
	gcpprovider.WithDebug(true),
	gcpprovider.WithName("gcp-europewest1-provider"),
	gcpprovider.WithRegion("europe-west1"),
	gcpprovider.WithProjectId(u.Ps("myproject").ToStringOutput()),
).Create()
if err != nil {
	return err
}

Everything works as expected !

I suspect that somehow with my code the Project ends up nil and it makes the provider loose his mind instead of just saying that it can't be empty (as it does when I don't provide the Region).

Log output

No response

Affected Resource(s)

  • The NewProvider() method of this Provider.
  • All resources of this provider when the bug is On.

Output of pulumi about

❯ pulumi about
CLI
Version 3.137.0
Go Version go1.23.2
Go Compiler gc

Host
OS ubuntu
Version 22.04
Arch x86_64

Backend
Name pulumi.com
URL https://app.pulumi.com/pdemagny-blobr
User pdemagny-blobr
Organizations pdemagny-blobr, blobr-io, blobr-ai
Token type personal

Pulumi locates its logs in /tmp by default

Additional context

This is related to support ticket n°5654.

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

@pdemagny pdemagny added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Oct 21, 2024
@VenelinMartinov
Copy link
Contributor

Hey @pdemagny. Sorry you've had trouble here. Looking at your code it looks like you are typing the ProjectId input to the Provider as pulumi.StringOutput. I don't believe that providers support output-type inputs, which is what the error message is trying to tell you.

Can you try changing that to:

type (
    ResourceArgs struct {
        Name      string
        ProjectId string
        Region    string
    }

That should work much better as that promises pulumi that the project id configuration will always be available - it can not be unknown.

@VenelinMartinov VenelinMartinov added awaiting-feedback Blocked on input from the author kind/question Questions about existing features and removed needs-triage Needs attention from the triage team kind/bug Some behavior is incorrect or out of spec labels Oct 23, 2024
@pdemagny
Copy link
Author

pdemagny commented Oct 25, 2024

When I modify my code with your suggestion, it demonstrates the expected behavior:

error: rpc error: code = Unknown desc = expected a non-empty string: project was set to ``

Thanks for the fix.
But it's still possible to pass a StringOutput that ends up potentialy unknown and risk breaking stuff, isn't there a way to prevent this ?

@pulumi-bot pulumi-bot added needs-triage Needs attention from the triage team and removed awaiting-feedback Blocked on input from the author labels Oct 25, 2024
@pdemagny pdemagny changed the title nil Project on NewProvider() causes potential unwanted deletion/modification of many resources unknown StringOutput Project on NewProvider() causes potential unwanted deletion/modification of many resources Oct 25, 2024
@VenelinMartinov
Copy link
Contributor

VenelinMartinov commented Oct 28, 2024

Hey @pdemagny. Do you think the warning is insufficient here? Would you prefer it if we provided a flag for this to become a hard error? Unfortunately we can not do this by default as some user programs do depend on this behaviour and we'd like to avoid breaking them.

@VenelinMartinov VenelinMartinov removed the needs-triage Needs attention from the triage team label Oct 28, 2024
@pdemagny
Copy link
Author

pdemagny commented Oct 29, 2024

Hi @VenelinMartinov. I think it would definitely be best if was an error instead of a warning, but not at the cost of a breaking change, and anyway that's not my place to tell 😅
In this case I think the warning could be enough, since it's gonna be a rare bug, but the warning could maybe be clearer somehow.

@VenelinMartinov VenelinMartinov added the resolution/fixed This issue was fixed label Oct 29, 2024
@VenelinMartinov VenelinMartinov self-assigned this Oct 29, 2024
@VenelinMartinov
Copy link
Contributor

Glad you managed to fix the issue on your end. Let me know if you run into any more troubles here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/question Questions about existing features resolution/fixed This issue was fixed
Projects
None yet
Development

No branches or pull requests

3 participants