diff --git a/.changeset/funny-rice-prove.md b/.changeset/funny-rice-prove.md new file mode 100644 index 000000000000..df4d955989d4 --- /dev/null +++ b/.changeset/funny-rice-prove.md @@ -0,0 +1,11 @@ +--- +"wrangler": minor +--- + +Simplify secret:bulk api via script settings + +Firing PUTs to the secret api in parallel has never been a great solution - each request independently needs to lock the script, so running in parallel is at best just as bad as running serially. + +Luckily, we have the script settings PATCH api now, which can update the settings for a script (including secret bindings) at once, which means we don't need any parallelization. However this api doesn't work with a partial list of bindings, so we have to fetch the current bindings and merge in with the new secrets before PATCHing. We can however just omit the value of the binding (i.e. only provide the name and type) which instructs the config service to inherit the existing value, which simplifies this as well. Note that we don't use the bindings in your current wrangler.toml, as you could be in a draft state, and it makes sense as a user that a bulk secrets update won't update anything else. Instead, we use script settings api again to fetch the current state of your bindings. + +This simplified implementation means the operation can only fail or succeed, rather than succeeding in updating some secrets but failing for others. In order to not introduce breaking changes for logging output, the language around "${x} secrets were updated" or "${x} secrets failed" is kept, even if it doesn't make much sense anymore. diff --git a/packages/wrangler/src/__tests__/secret.test.ts b/packages/wrangler/src/__tests__/secret.test.ts index 496b42905516..4e7c5c6d38f7 100644 --- a/packages/wrangler/src/__tests__/secret.test.ts +++ b/packages/wrangler/src/__tests__/secret.test.ts @@ -1,8 +1,10 @@ +import { Blob } from "node:buffer"; import * as fs from "node:fs"; import { writeFileSync } from "node:fs"; import readline from "node:readline"; import * as TOML from "@iarna/toml"; -import { rest } from "msw"; +import { MockedRequest, rest, type RestRequest } from "msw"; +import { FormData } from "undici"; import { mockAccountId, mockApiToken } from "./helpers/mock-account-id"; import { mockConsoleMethods } from "./helpers/mock-console"; import { mockConfirm, mockPrompt, clearDialogs } from "./helpers/mock-dialogs"; @@ -10,6 +12,7 @@ import { useMockIsTTY } from "./helpers/mock-istty"; import { mockGetMembershipsFail } from "./helpers/mock-oauth-flow"; import { useMockStdin } from "./helpers/mock-stdin"; import { msw } from "./helpers/msw"; +import { FileReaderSync } from "./helpers/msw/read-file-sync"; import { runInTempDir } from "./helpers/run-in-tmp"; import { runWrangler } from "./helpers/run-wrangler"; import type { Interface } from "node:readline"; @@ -543,22 +546,23 @@ describe("wrangler secret", () => { }) as unknown as Interface ); - let counter = 0; msw.use( - rest.put( - `*/accounts/:accountId/workers/scripts/:scriptName/secrets`, + rest.get( + `*/accounts/:accountId/workers/scripts/:scriptName/settings`, (req, res, ctx) => { expect(req.params.accountId).toEqual("some-account-id"); - counter++; - return res( - ctx.json( - createFetchResult({ - name: `secret-name-${counter}`, - type: "secret_text", - }) - ) - ); + return res(ctx.json(createFetchResult({ bindings: [] }))); + } + ) + ); + msw.use( + rest.patch( + `*/accounts/:accountId/workers/scripts/:scriptName/settings`, + (req, res, ctx) => { + expect(req.params.accountId).toEqual("some-account-id"); + + return res(ctx.json(createFetchResult(null))); } ) ); @@ -573,7 +577,6 @@ describe("wrangler secret", () => { ✨ 2 secrets successfully uploaded" `); expect(std.err).toMatchInlineSnapshot(`""`); - expect(counter).toEqual(2); }); it("should create secret:bulk", async () => { @@ -585,23 +588,23 @@ describe("wrangler secret", () => { }) ); - // User counter to pass different secrets to the request mock - let counter = 0; msw.use( - rest.put( - `*/accounts/:accountId/workers/scripts/:scriptName/secrets`, + rest.get( + `*/accounts/:accountId/workers/scripts/:scriptName/settings`, (req, res, ctx) => { expect(req.params.accountId).toEqual("some-account-id"); - counter++; - return res( - ctx.json( - createFetchResult({ - name: `secret-name-${counter}`, - type: "secret_text", - }) - ) - ); + return res(ctx.json(createFetchResult({ bindings: [] }))); + } + ) + ); + msw.use( + rest.patch( + `*/accounts/:accountId/workers/scripts/:scriptName/settings`, + (req, res, ctx) => { + expect(req.params.accountId).toEqual("some-account-id"); + + return res(ctx.json(createFetchResult(null))); } ) ); @@ -633,27 +636,23 @@ describe("wrangler secret", () => { }) ); - // User counter to pass different secrets to the request mock - let counter = 0; msw.use( - rest.put( - `*/accounts/:accountId/workers/scripts/:scriptName/secrets`, + rest.get( + `*/accounts/:accountId/workers/scripts/:scriptName/settings`, (req, res, ctx) => { expect(req.params.accountId).toEqual("some-account-id"); - counter++; - if (counter % 2 === 0) { - return res( - ctx.json( - createFetchResult({ - name: `secret-name-${counter}`, - type: "secret_text", - }) - ) - ); - } else { - return res.networkError(`Failed to create secret ${counter}`); - } + return res(ctx.json(createFetchResult({ bindings: [] }))); + } + ) + ); + msw.use( + rest.patch( + `*/accounts/:accountId/workers/scripts/:scriptName/settings`, + (req, res) => { + expect(req.params.accountId).toEqual("some-account-id"); + + return res.networkError(`Failed to create secret`); } ) ); @@ -661,50 +660,19 @@ describe("wrangler secret", () => { await expect(async () => { await runWrangler("secret:bulk ./secret.json --name script-name"); }).rejects.toThrowErrorMatchingInlineSnapshot( - `"🚨 4 secrets failed to upload"` + `"🚨 7 secrets failed to upload"` ); expect(std.out).toMatchInlineSnapshot(` "🌀 Creating the secrets for the Worker \\"script-name\\" - ✨ Successfully created secret for key: secret-name-2 - ✨ Successfully created secret for key: secret-name-4 - ✨ Successfully created secret for key: secret-name-6 Finished processing secrets JSON file: - ✨ 3 secrets successfully uploaded + ✨ 0 secrets successfully uploaded If you think this is a bug then please create an issue at https://github.com/cloudflare/workers-sdk/issues/new/choose" `); expect(std.err).toMatchInlineSnapshot(` - "X [ERROR] uploading secret for key: secret-name-1: - - request to - https://api.cloudflare.com/client/v4/accounts/some-account-id/workers/scripts/script-name/secrets - failed, reason: Failed to create secret 1 - - - X [ERROR] uploading secret for key: secret-name-3: - - request to - https://api.cloudflare.com/client/v4/accounts/some-account-id/workers/scripts/script-name/secrets - failed, reason: Failed to create secret 3 - - - X [ERROR] uploading secret for key: secret-name-5: - - request to - https://api.cloudflare.com/client/v4/accounts/some-account-id/workers/scripts/script-name/secrets - failed, reason: Failed to create secret 5 - - - X [ERROR] uploading secret for key: secret-name-7: - - request to - https://api.cloudflare.com/client/v4/accounts/some-account-id/workers/scripts/script-name/secrets - failed, reason: Failed to create secret 7 - - - X [ERROR] 🚨 4 secrets failed to upload + "X [ERROR] 🚨 7 secrets failed to upload " `); @@ -719,16 +687,23 @@ describe("wrangler secret", () => { }) ); - // User counter to pass different secrets to the request mock - let counter = 0; msw.use( - rest.put( - `*/accounts/:accountId/workers/scripts/:scriptName/secrets`, + rest.get( + `*/accounts/:accountId/workers/scripts/:scriptName/settings`, + (req, res, ctx) => { + expect(req.params.accountId).toEqual("some-account-id"); + + return res(ctx.json(createFetchResult({ bindings: [] }))); + } + ) + ); + msw.use( + rest.patch( + `*/accounts/:accountId/workers/scripts/:scriptName/settings`, (req, res) => { expect(req.params.accountId).toEqual("some-account-id"); - counter++; - return res.networkError(`Failed to create secret ${counter}`); + return res.networkError(`Failed to create secret`); } ) ); @@ -748,24 +723,130 @@ describe("wrangler secret", () => { If you think this is a bug then please create an issue at https://github.com/cloudflare/workers-sdk/issues/new/choose" `); expect(std.err).toMatchInlineSnapshot(` - "X [ERROR] uploading secret for key: secret-name-1: + "X [ERROR] 🚨 2 secrets failed to upload - request to - https://api.cloudflare.com/client/v4/accounts/some-account-id/workers/scripts/script-name/secrets - failed, reason: Failed to create secret 1 + " + `); + }); + it("should merge existing bindings and secrets when patching", async () => { + writeFileSync( + "secret.json", + JSON.stringify({ + "secret-name-2": "secret_text", + "secret-name-3": "secret_text", + }) + ); + + msw.use( + rest.get( + `*/accounts/:accountId/workers/scripts/:scriptName/settings`, + (req, res, ctx) => { + expect(req.params.accountId).toEqual("some-account-id"); + + return res( + ctx.json( + createFetchResult({ + bindings: [ + { + type: "plain_text", + name: "env_var", + text: "the content", + }, + { + type: "json", + name: "another_var", + json: { some: "stuff" }, + }, + { type: "secret_text", name: "secret-name-1" }, + { type: "secret_text", name: "secret-name-2" }, + ], + }) + ) + ); + } + ) + ); + msw.use( + rest.patch( + `*/accounts/:accountId/workers/scripts/:scriptName/settings`, + async (req, res, ctx) => { + expect(req.params.accountId).toEqual("some-account-id"); - X [ERROR] uploading secret for key: secret-name-2: + const formBody = await ( + req as MockedRequest as RestRequestWithFormData + ).formData(); + const settings = formBody.get("settings"); + expect(settings).not.toBeNull(); + expect(JSON.parse(formBody.get("settings") as string)).toMatchObject({ + bindings: [ + { type: "plain_text", name: "env_var" }, + { type: "json", name: "another_var" }, + { type: "secret_text", name: "secret-name-1" }, + { + type: "secret_text", + name: "secret-name-2", + text: "secret_text", + }, + { + type: "secret_text", + name: "secret-name-3", + text: "secret_text", + }, + ], + }); - request to - https://api.cloudflare.com/client/v4/accounts/some-account-id/workers/scripts/script-name/secrets - failed, reason: Failed to create secret 2 + return res(ctx.json(createFetchResult(null))); + } + ) + ); + await runWrangler("secret:bulk ./secret.json --name script-name"); - X [ERROR] 🚨 2 secrets failed to upload + expect(std.out).toMatchInlineSnapshot(` + "🌀 Creating the secrets for the Worker \\"script-name\\" + ✨ Successfully created secret for key: secret-name-2 + ✨ Successfully created secret for key: secret-name-3 - " - `); + Finished processing secrets JSON file: + ✨ 2 secrets successfully uploaded" + `); + expect(std.err).toMatchInlineSnapshot(`""`); }); }); }); + +FormData.prototype.toString = mockFormDataToString; +export interface RestRequestWithFormData extends MockedRequest, RestRequest { + formData(): Promise; +} +(MockedRequest.prototype as RestRequestWithFormData).formData = + mockFormDataFromString; + +function mockFormDataToString(this: FormData) { + const entries = []; + for (const [key, value] of this.entries()) { + if (value instanceof Blob) { + const reader = new FileReaderSync(); + reader.readAsText(value); + const result = reader.result; + entries.push([key, result]); + } else { + entries.push([key, value]); + } + } + return JSON.stringify({ + __formdata: entries, + }); +} + +async function mockFormDataFromString(this: MockedRequest): Promise { + const { __formdata } = await this.json(); + expect(__formdata).toBeInstanceOf(Array); + + const form = new FormData(); + for (const [key, value] of __formdata) { + form.set(key, value); + } + return form; +} diff --git a/packages/wrangler/src/secret/index.ts b/packages/wrangler/src/secret/index.ts index 6b1d74a624b9..65b97133ebb9 100644 --- a/packages/wrangler/src/secret/index.ts +++ b/packages/wrangler/src/secret/index.ts @@ -1,8 +1,12 @@ import path from "node:path"; import readline from "node:readline"; +import { FormData } from "undici"; import { fetchResult } from "../cfetch"; import { readConfig } from "../config"; -import { createWorkerUploadForm } from "../deployment-bundle/create-worker-upload-form"; +import { + type WorkerMetadataBinding, + createWorkerUploadForm, +} from "../deployment-bundle/create-worker-upload-form"; import { confirm, prompt } from "../dialogs"; import { getLegacyScriptName, @@ -20,6 +24,19 @@ import type { StrictYargsOptionsToInterface, } from "../yargs-types"; +type SecretBindingUpload = { + type: "secret_text"; + name: string; + text: string; +}; + +type InheritBindingUpload = { + type: (WorkerMetadataBinding | SecretBindingRedacted)["type"]; + name: string; +}; + +type SecretBindingRedacted = Omit; + function isMissingWorkerError(e: unknown): e is { code: 10007 } { return ( typeof e === "object" && @@ -356,7 +373,7 @@ export const secretBulkHandler = async (secretBulkArgs: SecretBulkArgs) => { }` ); - let content; + let content: Record; if (secretBulkArgs.json) { const jsonFilePath = path.resolve(secretBulkArgs.json); content = parseJSON>( @@ -380,58 +397,78 @@ export const secretBulkHandler = async (secretBulkArgs: SecretBulkArgs) => { return logger.error(`🚨 No content found in JSON file or piped input.`); } - function submitSecrets(key: string, value: string) { + function getSettings() { const url = !secretBulkArgs.env || isLegacyEnv(config) - ? `/accounts/${accountId}/workers/scripts/${scriptName}/secrets` - : `/accounts/${accountId}/workers/services/${scriptName}/environments/${secretBulkArgs.env}/secrets`; + ? `/accounts/${accountId}/workers/scripts/${scriptName}/settings` + : `/accounts/${accountId}/workers/services/${scriptName}/environments/${secretBulkArgs.env}/settings`; + + return fetchResult<{ + bindings: Array; + }>(url); + } + + function putBindingsSettings( + bindings: Array + ) { + const url = + !secretBulkArgs.env || isLegacyEnv(config) + ? `/accounts/${accountId}/workers/scripts/${scriptName}/settings` + : `/accounts/${accountId}/workers/services/${scriptName}/environments/${secretBulkArgs.env}/settings`; + + const data = new FormData(); + data.set("settings", JSON.stringify({ bindings })); return fetchResult(url, { - method: "PUT", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ - name: key, - text: value, - type: "secret_text", - }), + method: "PATCH", + body: data, }); } - // Until we have a bulk route for secrets, we need to make a request for each key/value pair - const bulkOutcomes = await Promise.all( - Object.entries(content).map(async ([key, value]) => { - try { - await submitSecrets(key, value); - logger.log(`✨ Successfully created secret for key: ${key}`); - return true; - } catch (e) { - if (e instanceof Error) { - logger.error( - `uploading secret for key: ${key}: - ${e.message}` - ); - if (isMissingWorkerError(e)) { - // create a draft worker and try again - await createDraftWorker({ - config, - args: secretBulkArgs, - accountId, - scriptName, - }); - await submitSecrets(key, value); - // TODO: delete the draft worker if this failed too? - } - return false; - } - } - }) - ); - const successes = bulkOutcomes.filter((outcome) => outcome).length; - const failures = bulkOutcomes.length - successes; - logger.log(""); - logger.log("Finished processing secrets JSON file:"); - logger.log(`✨ ${successes} secrets successfully uploaded`); - if (failures > 0) { - throw new Error(`🚨 ${failures} secrets failed to upload`); + let existingBindings: Array; + try { + const settings = await getSettings(); + existingBindings = settings.bindings; + } catch (e) { + if (isMissingWorkerError(e)) { + // create a draft worker before patching + await createDraftWorker({ + config, + args: secretBulkArgs, + accountId, + scriptName, + }); + existingBindings = []; + } else { + throw e; + } + } + const inheritBindings = existingBindings.filter((binding) => { + return binding.type !== "secret_text" || !content[binding.name]; + }); + const upsertBindings: Array = Object.entries( + content + ).map(([key, value]) => { + return { + type: "secret_text", + name: key, + text: value, + }; + }); + try { + await putBindingsSettings(inheritBindings.concat(upsertBindings)); + for (const upsertedBinding of upsertBindings) { + logger.log( + `✨ Successfully created secret for key: ${upsertedBinding.name}` + ); + } + logger.log(""); + logger.log("Finished processing secrets JSON file:"); + logger.log(`✨ ${upsertBindings.length} secrets successfully uploaded`); + } catch (err) { + logger.log(""); + logger.log("Finished processing secrets JSON file:"); + logger.log(`✨ 0 secrets successfully uploaded`); + throw new Error(`🚨 ${upsertBindings.length} secrets failed to upload`); } };