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

feat: Use OAuth flow to generate R2 tokens for Pipelines #7534

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

cmackenzie1
Copy link
Contributor

@cmackenzie1 cmackenzie1 commented Dec 12, 2024

Fixes https://jira.cfdata.org/browse/PIPE-159.

This commit changes the generateR2Tokens flow which will direct the user to the web browser to perform a OAuth flow to grant the Workers Pipelines client the ability to generate R2 tokens on behalf of the user. This will only run if the user does not provide the credentials as CLI parameters.

Due to requiring user interactivity, and reliance on the callbacks, there is no easy way to support a "headless" mode for wrangler pipelines create (or update) unless the user provides the tokens as arguments. The same applies for testing this flow, which can only be done manually at this time.


  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because: new behaviour can only be tested manually (see above)
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because:

@cmackenzie1 cmackenzie1 requested a review from a team as a code owner December 12, 2024 18:24
Copy link

changeset-bot bot commented Dec 12, 2024

🦋 Changeset detected

Latest commit: 92842a6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
wrangler Patch
@cloudflare/vitest-pool-workers Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@cmackenzie1
Copy link
Contributor Author

@penalosa here is a variant of what we discussed surrounding the OAuth flow. We instead are hosting the Worker doing the token exchange instead of making wrangler a trusted client. I'll definitely need some help/guidance around the testing aspect of this PR.

@cmackenzie1
Copy link
Contributor Author

fyi: @oliy @hhoughgg

Copy link
Contributor

github-actions bot commented Dec 12, 2024

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12638040461/npm-package-wrangler-7534

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7534/npm-package-wrangler-7534

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12638040461/npm-package-wrangler-7534 dev path/to/script.js
Additional artifacts:
wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12638040461/npm-package-cloudflare-workers-bindings-extension-7534 -O ./cloudflare-workers-bindings-extension.0.0.0-vf2ed8e9ff.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-vf2ed8e9ff.vsix
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12638040461/npm-package-create-cloudflare-7534 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12638040461/npm-package-cloudflare-kv-asset-handler-7534
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12638040461/npm-package-miniflare-7534
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12638040461/npm-package-cloudflare-pages-shared-7534
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12638040461/npm-package-cloudflare-unenv-preset-7534
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12638040461/npm-package-cloudflare-vitest-pool-workers-7534
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12638040461/npm-package-cloudflare-workers-editor-shared-7534
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12638040461/npm-package-cloudflare-workers-shared-7534
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12638040461/npm-package-cloudflare-workflows-shared-7534

Note that these links will no longer work once the GitHub Actions artifact expires.


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241218.0
workerd 1.20241230.0 1.20241230.0
workerd --version 1.20241230.0 2024-12-30

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@cmackenzie1 cmackenzie1 force-pushed the cole/pipelines-auto-r2-tokens branch from 6f27584 to c11f0f4 Compare December 16, 2024 18:31
@lrapoport-cf lrapoport-cf added the caretaking Priority for caretaking label Dec 18, 2024
Copy link
Contributor

@andyjessop andyjessop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmackenzie1 thanks very much for this! I've got just a few comments about code organisation and documentation.

packages/wrangler/src/pipelines/client.ts Outdated Show resolved Hide resolved
packages/wrangler/src/pipelines/client.ts Show resolved Hide resolved
packages/wrangler/src/pipelines/client.ts Outdated Show resolved Hide resolved
@cmackenzie1 cmackenzie1 force-pushed the cole/pipelines-auto-r2-tokens branch from 522332c to 08b2507 Compare December 18, 2024 21:10
@emily-shen emily-shen added the e2e Run e2e tests on a PR label Jan 2, 2025
Copy link
Contributor

@emily-shen emily-shen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it not possible to test this with mocks like the existing ones for wrangler login, which also depend on user interactivity and callbacks. Or have you considered that already and decided it would be mocking so much that there's not too much point in the first place?

@emily-shen emily-shen removed the e2e Run e2e tests on a PR label Jan 2, 2025
This commit changes the generateR2Tokens flow which will direct the user
to the web browser to perform a OAuth flow to grant the Workers
Pipelines client the ability to generate R2 tokens on behalf of the
user. This will only run if the user does not provide the credentials as
CLI parameters.

Due to requiring user interactivity, and reliance on the callbacks,
there is no easy way to support a "headless" mode for `wrangler pipelines
create` (or `update`) unless the user provides the tokens as arguments.
The same applies for testing this flow, which can only be done manually
at this time.
After creating an R2 token, there is a slight delay before if can be
used. Previously we would sleep for some amount of time, but this method
is really sensitive to latency.

Instead, use the S3 SDK and try using the token until we exhaust all
attempts, or we finally succeed in using it. Each failure results in a
constant backoff of 1 second.

This commit does add the dependency `@aws-sdk/client-s3`.
@cmackenzie1 cmackenzie1 force-pushed the cole/pipelines-auto-r2-tokens branch from 5ad62f8 to c63dbb0 Compare January 2, 2025 21:40
emily-shen and others added 2 commits January 3, 2025 10:30
This uses the promise based version of `setTimeout` from NodeJS and
registers the AbortController to handle cancellation signal. The http
server `.close()` method is also registered to the abort controller for
cleanup as `controller.abort()` is always called before returning the
result.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
caretaking Priority for caretaking
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

4 participants