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.

Sign up for GitHub

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

Simplify secret:bulk api via script settings #4179

Merged
merged 1 commit into from
Oct 30, 2023
+274 −143
Merged

Simplify secret:bulk api via script settings #4179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .changeset/funny-rice-prove.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
273 changes: 178 additions & 95 deletions packages/wrangler/src/__tests__/secret.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
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";
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";
Expand Down Expand Up @@ -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)));
}
)
);
Expand All @@ -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 () => {
Expand All @@ -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)));
}
)
);
Expand Down Expand Up @@ -633,78 +636,43 @@ 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`);
}
)
);

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
"
`);
Expand All @@ -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`);
}
)
);
Expand All @@ -748,24 +723,132 @@ 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(`
"[31mX [41;31m[[41;97mERROR[41;31m][0m [1muploading secret for key: secret-name-1:[0m
"[31mX [41;31m[[41;97mERROR[41;31m][0m [1m🚨 2 secrets failed to upload[0m
request to
[4mhttps://api.cloudflare.com/client/v4/accounts/some-account-id/workers/scripts/script-name/secrets[0m
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<FormData>;
}
(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<FormData> {
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;
}
Loading