Crash when deploying change to minInstances on v1 function in us-west1 due to undefined tier

dominics commented 7 months ago

[REQUIRED] Environment info

firebase-tools: 13.7.1 Platform: macOS / Linux

[REQUIRED] Test case

firebase deploy --only functions --project my-project --debug

However, you must also have a function with endpoint=gcfv1, region=us-west1

[REQUIRED] Steps to reproduce

Try to ~change~ reduce to zero the minInstances of the gcfv1 function in us-west1 and deploy

[REQUIRED] Expected behavior

The CLI shows a pricing warning about the minimum instances

[REQUIRED] Actual behavior

Crash with:

[2024-04-10T10:47:37.720Z] TypeError: Cannot read properties of undefined (reading 'ram')
at Object.monthlyMinInstanceCost (/proj/node_modules/firebase-tools/lib/deploy/functions/pricing.js:147:38)
at /proj/node_modules/firebase-tools/lib/deploy/functions/prompts.js:139:32
at Array.some (<anonymous>)
at Object.someEndpoint (/proj/node_modules/firebase-tools/lib/deploy/functions/backend.js:263:38)
at promptForMinInstances (/proj/node_modules/firebase-tools/lib/deploy/functions/prompts.js:124:35)
at prepare (/proj/node_modules/firebase-tools/lib/deploy/functions/prepare.js:177:47)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async chain (/proj/node_modules/firebase-tools/lib/deploy/index.js:38:9)
at async deploy (/proj/node_modules/firebase-tools/lib/deploy/index.js:95:5)

Error: An unexpected error has occurred.

This is because at line 146 of pricing.js, tier is undefined (because at line 142, the V1_REGION_TO_TIER[endpoint.region] index fails)
This is in turn because V1_REGION_TO_TIER earlier in the file does not contain an entry for us-west1 (only us-west2 through us-west4, and despite V2_REGION_TO_TIER containing it)

Notes

Is this (v1 function in us-west1) an unsupported configuration?

aalej commented 7 months ago

Hey @dominics, thanks for reaching out and sharing your observations. Looking through our docs, it seems like us-west1 is a supported region.

I’m currently working on reproducing this, however, I haven’t encountered the same error message you shared. Could you provide a code snippet of the function you’re trying to deploy to help us accurately replicate the issue?

dominics commented 7 months ago

Hi @aalej, sure thing! Thanks for taking a look

The function is configured (and already deployed) like this:

import * as functions from "firebase-functions";

export const myAppFunction = functions
    .runWith({
        memory: "4GB",
        timeoutSeconds: 540,
        vpcConnector: "myapp-vpc-connector",
        vpcConnectorEgressSettings: "PRIVATE_RANGES_ONLY",
        minInstances: 1,
    })
    .region("us-west1")
    .firestore.document("documents/{documentId}/comments/{commentId}")
    .onCreate(myAppFunc);

(and I'm trying to decrease the minInstances back to 0 - but tried RESET_VALUE too)

Sorry, but I just realized my original stack trace and line numbers I referred to were misleading, because I copied them after I added some logging around the relevant lines in pricing.js. The actual failure happens at line 143/144 (not 147), where we try to dereference undefined when looking up usage["gcfv1"][tier].ram, because tier became undefined at line 142. This bit:

You can see that us-west1 is not present in this table: https://github.com/firebase/firebase-tools/blob/3dce5d821c87d5ec64d9c0556d4678a3566d15f0/src/deploy/functions/pricing.ts#L14-L35

If I log the relevant parts of endpoint from the loop in monthlyMinInstanceCost(), around here it looks like this:

{
    platform: 'gcfv1',
    id: 'myAppFunction',
    project: 'my-project',
    region: 'us-west1',
    // [...]
    minInstances: 1,
    maxInstances: 3000,
    // [...]
}

which is why I thought the problem was the lookup of us-west1 in V1_REGION_TO_TIER.

Edit: Late discovery important for reproduction: there is a guard in canCalculateMinInstanceCost() that would otherwise stop the crash (if (!V1_REGION_TO_TIER[endpoint.region]) { return false; }), but it's therefore important that I am setting the minInstances to zero, a falsy value, because it will make the guard early-exit with true instead of false:

https://github.com/firebase/firebase-tools/blob/3dce5d821c87d5ec64d9c0556d4678a3566d15f0/src/deploy/functions/pricing.ts#L134-L137

aalej commented 7 months ago

Hey @dominics, thanks for the clarifications and additional details. That was a lot of info! I am now able to reproduce the issue. It does look like reducing the minInstances to 0 causes the error. I’ll raise this to our engineering team so they can take a look.

Also, it looks like you may have identified the cause of the issue here - if you’re open to the idea, feel free to create a PR for this!

colerogers commented 7 months ago

@dominics Wow thanks for the detailed writeup and location for the fix! I've merged in the change to check specifically for undefined and null so the falsy 0 should not cause a deployment failure. The fix should be out in the next release of the repo. Thanks again for your help!

firebase / firebase-tools