search

pulumi / pulumi-gcp

A Google Cloud Platform (GCP) Pulumi resource package, providing multi-language access to GCP
Apache License 2.0
185 stars 52 forks source link

Missing configuration leads to poor errors #129

Closed joeduffy closed 9 months ago

joeduffy commented 5 years ago

If I neglect to set my project, region, or zone configuration, I get an unhelpful error message:

error: Plan apply failed: project: required field is not set

Given that this will happen almost guaranteed for every new user of Pulumi on GCP, we should try to make this experience better. IIRC, we have some trick that we already use in the AWS package to hook errors like this and prettify them before displaying to the end user.

joeduffy commented 5 years ago

I believe @ellismg and @jen20 looked into this and concluded it isn't as easy as copying what we do in the AWS package, since you can set these on a per-resource basis.

This would be a very unfortunate conclusion. Here is a common experience with basic GCP programs, annotated with some of my own commentary/questions:

@ellismg I know you had feedback on the GCP getting started flow. I'm actually wondering whether this was a problem for others on the team, or whether our tutorials make it so you don't stumble here. (I'm always stumbling, but maybe I'm not paying close enough attention.) Thoughts?

Also /cc @swgillespie as I know improving errors is/was a passion of his, and I'm curious if he's ever looked into this sort of "error rewriting" we might need to do on the fly in order to improve these messages.

ellismg commented 5 years ago

@ellismg I know you had feedback on the GCP getting started flow. I'm actually wondering whether this was a problem for others on the team, or whether our tutorials make it so you don't stumble here. (I'm always stumbling, but maybe I'm not paying close enough attention.) Thoughts?

I did not specifically run into the project error, because I used pulumi new to create a new project using the gcp-typescript template and that flow prompted me for a project, so it gcp:project was always defined. I did hit the region/zone issue later when trying to add a serverless handler to my project, but the Cannot determine region: set in this resource, or set provider-level 'region' or 'zone'. text was sufficient enough for me to know what to do.

Regarding this:

Question: Why doesn't this use my default gcloud project? If I run gcloud info, it prints out the very project I want to use...

I have had the same question about the aws provider, where I have to set a region for every project, even though my default configuration for the AWS SDK on my machine says I want to use us-west-2.

I do wonder if the right path forward here is to further augment the configuration that would be passed to the tf provider after terraform has expanded its defaults but before we actually configure it. In that case, we could try to include information from other sources on the machine.

I'm curious if he's ever looked into this sort of "error rewriting" we might need to do on the fly in order to improve these messages.

Would it be possible to add some hooks in the bridge such that we could rewrite errors coming from the CRUD methods on the bridged provider before returning them? We could at least string sniff for stuff then and produce slightly nicer messages.

lukehoban commented 5 years ago

@mikhailshilkov Did your recent improvements address this case?

mikhailshilkov commented 5 years ago

@lukehoban I believe you are referring to the open PR https://github.com/pulumi/pulumi-terraform/pull/414

This issue is NOT fixed by that PR (as of its current state).

Update: I've figured out the config basics, so removed some questions. Working further...

There are several things on the way:

  1. Azure location is a property of the resource with a default value pointing to the config value. I believe GCP project has a different configuration - one can't set it in the resource properties directly.

  2. The GCP failure comes from a call to Create while Azure failure comes from a call to Check. I'd need to wrap the returned error after the call to p.tf.Apply too. That's doable after I understand how (1) should work. Can we make the preview fail too?

  3. Small but unexpected thing: the property name is reported with quotes for Azure "location": required field is not set and without them for GCP project: required field is not set. So, I'd have to adjust the regex. Any idea about the reason?

mikhailshilkov commented 5 years ago

To answer my own questions, these errors come from the specific implementation of the Terraform Google provider: project error, region error, zone error.

I can add mappings from those hard-coded messages to our more descriptive errors pointing to pulumi config commands.

Do you think that would be enough here?

lukehoban commented 5 years ago

I see - this is indeed subtly different than https://github.com/pulumi/pulumi-terraform/pull/414.

Honestly a shame that upstream doesn't implement these requirements in Check - might be worth even contributing an upstream PR to move these checks forward into Check. At that point, we could almost use the existing mechanism to solve for this.

I can add mappings from those hard-coded messages to our more descriptive errors pointing to pulumi config commands. Do you think that would be enough here?

That does sound a little more unfortunate. In part, I'm not sure the errors come back in as structured of a way from Create, and also a shame to just have to do a blind grep for these messages in all errors from Create (at least in the GCP provider).

But not sure there is going to be any better option unfortunately unless upstream improves here. (Notably, all the problems noted in this thread are also problems for terraform users - so this really is a problem ideally solved in the upstream provider - whereas https://github.com/pulumi/pulumi-terraform/pull/414 addresses a problem we introduced in the Pulumi provider).

mikhailshilkov commented 5 years ago

@lukehoban I started by creating https://github.com/terraform-providers/terraform-provider-google/issues/4071 Let me know if it's enough priority to give a try for a PR.

mikhailshilkov commented 5 years ago

Terraform said the upstream change is not feasible, so we are back to the next-best option of parsing Create errors...

lukehoban commented 5 years ago

I honestly think at this point we should leave things as is. This really is an issue in the upstream provider, and if the upstream provider is unwilling to fix it, it's not clear we should be resorting to trying to clean up their error message post-hoc.

pulumi/pulumi-terraform#414 was quite different - as it was addressing a case where we had introduced a new behaviour in the Pulumi projection and so needed to massage error messages to be correct for Pulumi.

redbaron commented 5 years ago

we've been hit by it and it was confusing. I realise that upstream is not going to fix it,but it doesn't mean Pulumi can't do it's part. One way would be to do merging with ambient defaults on Pulumi side and always pass project to TF explicitly or error before even calling TF is project is not set.

lukehoban commented 5 years ago

One way would be to do merging with ambient defaults on Pulumi side and always pass project to TF explicitly or error before even calling TF is project is not set.

Could you share more details on how you would see this happening? How would this merging avoid the error message? (That error happens when the user fails to provide the project in either stack config or resource inputs).

jaxxstorm commented 4 years ago

I actually just ran into this, and until I found this issue it wasn't at all clear what the issue way.

One thing that was very helpful with the AWS provider is that when a required field is missing, like the aws:region configuration option, you get a much more helpful error:

aws:s3:Bucket (bucket):
    error: 1 error occurred:
        * missing required configuration key "aws:region": The region where AWS operations will take place. Examples are us-east-1, us-west-2, etc.
    Set a value using the command `pulumi config set aws:region <value>`.

Can we at least make the error message clearer, or have the previous comments indicated why that isn't possible?

t0yv0 commented 11 months ago

Bumping into this at the end of 2023. Trying examples/topic. This still needs work. Without specifying anything I now get:

Diagnostics:
  pulumi:pulumi:Stack (serverless-topic-gcptopic):
    warning: unable to detect a global setting for GCP Project;
    Pulumi will rely on per-resource settings for this operation.
    Set the GCP Project by using:
        `pulumi config set gcp:project <project>`
    warning: unable to detect a global setting for GCP Project;
    Pulumi will rely on per-resource settings for this operation.
    Set the GCP Project by using:
        `pulumi config set gcp:project <project>`
    error: preview failed

gcp:cloudfunctions:Function (test-published):
  error: Preview failed: diffing urn:pulumi:gcptopic::serverless-topic::gcp:pubsub/topic:Topic$gcp:cloudfunctions:CallbackFunction$gcp:cloudfunctions/function:Function::test-published: 2 errors occurred:
      * Failed to retrieve project, pid: , err: project: required field is not set
      * Failed to retrieve region, pid: , err: region: required field is not set

The first message is good but it's bad that it is duplicated. @VenelinMartinov deduplicated this message in pulumi-aws https://github.com/pulumi/pulumi-aws/pull/2949 and it sounds like this needs to be confirmed/replicated for all tier 1 providers at least.

The errors on gcp:cloudfunctions:Function are very disappointing.

Continuing with this.

pulumi config set gcp:project pulumi-development

Now this:

  gcp:cloudfunctions:Function (test-published):
    error: Preview failed: diffing urn:pulumi:gcptopic::serverless-topic::gcp:pubsub/topic:Topic$gcp:cloudfunctions:CallbackFunction$gcp:cloudfunctions/function:Function::test-published: 1 error occurred:
        * Failed to retrieve region, pid: , err: region: required field is not set

Setting the region incorrectly doesn't inform of the correct values:

pulumi config set gcp:region westus

Looks like no validation is done until doing HTTP calls:

  gcp:cloudfunctions:Function (test-published):
    error: 1 error occurred:
        * googleapi: Error 403: Permission denied on 'locations/westus' (or it may not exist)., forbidden
CLI          
Version      3.97.0
Go Version   go1.21.4
Go Compiler  gc

Plugins
NAME    VERSION
gcp     7.4.0
nodejs  unknown

Host     
OS       darwin
Version  14.1.1
Arch     x86_64

This project is written in nodejs: executable='/Users/t0yv0/bin/node' version='v18.18.2'

Current Stack: t0yv0/serverless-topic/gcptopic

TYPE                                   URN
pulumi:pulumi:Stack                    urn:pulumi:gcptopic::serverless-topic::pulumi:pulumi:Stack::serverless-topic-gcptopic
pulumi:providers:gcp                   urn:pulumi:gcptopic::serverless-topic::pulumi:providers:gcp::default_7_4_0
gcp:pubsub/topic:Topic                 urn:pulumi:gcptopic::serverless-topic::gcp:pubsub/topic:Topic::test
gcp:cloudfunctions:CallbackFunction    urn:pulumi:gcptopic::serverless-topic::gcp:pubsub/topic:Topic$gcp:cloudfunctions:CallbackFunction::test-published
gcp:storage/bucket:Bucket              urn:pulumi:gcptopic::serverless-topic::gcp:pubsub/topic:Topic$gcp:cloudfunctions:CallbackFunction$gcp:storage/bucket:Bucket::test-published
gcp:storage/bucketObject:BucketObject  urn:pulumi:gcptopic::serverless-topic::gcp:pubsub/topic:Topic$gcp:cloudfunctions:CallbackFunction$gcp:storage/bucketObject:BucketObject::test-published

Found no pending operations associated with gcptopic

Backend        
Name           pulumi.com
URL            https://app.pulumi.com/t0yv0
User           t0yv0
Organizations  t0yv0, pulumi
Token type     personal

Dependencies:
NAME            VERSION
@types/node     8.10.66
@pulumi/gcp     7.4.0
@pulumi/pulumi  3.99.0

Pulumi locates its logs in /var/folders/gk/cchgxh512m72f_dmkcc3d09h0000gp/T/com.apple.shortcuts.mac-helper// by default
VenelinMartinov commented 10 months ago

I think most of the originally reported problems are no longer true. GCP now reports missing config as in @t0yv0's comment above.

https://github.com/pulumi/pulumi-gcp/pull/1552 should address the duplicate warning about the lack of global project.

I am also working on adding validation for the region config in https://github.com/pulumi/pulumi-gcp/pull/1554

t0yv0 commented 9 months ago

Reopening this because we had to revert because of a regression introduced by region validation. https://github.com/pulumi/pulumi-gcp/issues/1641 - this needs a little bit of rework to carefully account for Workload Identity Federation / OIDC style of authentication, not failing region validation when get region permission is unset as the user mentions in 1641.

iwahbe commented 9 months ago

I really liked the suggestion by https://github.com/pulumi/pulumi-gcp/issues/1641: Keep the get region call as a happy path for error messages, but don't let it introduce a failure.