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!: fold access-api into upload-api #194

Merged
merged 63 commits into from
Jul 18, 2023
Merged

Conversation

travis
Copy link
Member

@travis travis commented May 5, 2023

Get @web3-storage/access-api handlers running in the upload-api service.

This introduces two new DynamoDB tables to store "delegations" and "provisions". NB: I have a related PR in #200 that uses "consumers" and "subscriptions" tables rather than "provisions".

This also adds a new Lambda to handle the email verification flow - I've brought some HTML utilities over from @web3-storage/access-api for this purpose and wired the handlers up to the same path they were at in the old service /validate-email.

This is the part 2 of a larger project (started in storacha/w3up#790) to integrate the access service into the w3infra upload service, which will let us move operational details out of w3up, retire D1, consolidate our operations around AWS Lambdas, deprecate now-unused functionality like the voucher flows and generally streamline our UCAN handlers and the infrastructure that supports them.

TODO

@seed-deploy seed-deploy bot temporarily deployed to pr194 May 5, 2023 20:07 Inactive
@travis travis marked this pull request as draft May 5, 2023 20:18
@seed-deploy
Copy link

seed-deploy bot commented May 5, 2023

View stack outputs
  • pr194-w3infra-CarparkStack

    Name Value
    BucketName carpark-pr194-0
    Region us-east-2
  • pr194-w3infra-SatnavStack

    Name Value
    BucketName satnav-pr194-0
    Region us-east-2
  • pr194-w3infra-UploadApiStack

    Name Value
    ApiEndpoint https://hp29y2yke9.execute-api.us-east-2.amazonaws.com
    CustomDomain https://pr194.up.web3.storage
  • pr194-w3infra-BusStack

  • pr194-w3infra-ReplicatorStack

  • pr194-w3infra-UcanInvocationStack

  • pr194-w3infra-UploadDbStack

Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

As a direct port of what we have in d1 this looks all right. That said, I think taking bit more time to avoid another migration down the line might be in our benefit, from that perspective I have following notes:

  • We need two tables as opposed to currently 1 provisions table, which

    That way when you first login web3.storage can delegate an account a subscription that account can later add a consumer (space) to.

    1. Tracks subscriptions as in provider <-> customer relationship which is where provider could impose any constraints it wishes on the customer.
    2. Tracks subscription consumer provider <-> consumer <-> subscription relationship.
  • We need to support different actors to perform following queries

    • provider:
      • find my customers (accounts that have subscriptions)
      • find my consumers (spaces subscribed)
      • cancel subscription
      • block customer
      • stats per customer / consumer / subscription
    • space
      • what are my providers / subscriptions
      • unsubscribe
      • stats / limits
    • customer (account)
      • cancel subscription
      • what are my subscriptions
      • stats per subscriptions
  • we need to make provider capable of imposing whatever constraints it wants on accounts.

    • I propose accomplishing this via additional order field with uniqueness constraint. This way provider can impose all the constraints it wants by encoding them into the field generation.
      • e.g. web3.storage can derive order from the account email address.
      • nft.storage could choose alternative that would fit it's goals.

upload-api/access-types.ts Outdated Show resolved Hide resolved
upload-api/access-types.ts Outdated Show resolved Hide resolved
upload-api/access-types.ts Outdated Show resolved Hide resolved
upload-api/access-types.ts Outdated Show resolved Hide resolved
upload-api/types.ts Outdated Show resolved Hide resolved
upload-api/access-types.ts Outdated Show resolved Hide resolved
upload-api/buckets/delegations-store.js Show resolved Hide resolved
upload-api/tables/delegations.js Outdated Show resolved Hide resolved
upload-api/tables/index.js Outdated Show resolved Hide resolved
upload-api/tables/provisions.js Outdated Show resolved Hide resolved
travis added 5 commits May 9, 2023 13:45
This is a first step toward storacha/w3up#711 as documented in https://github.com/web3-storage/w3up/pull/744/files?short_path=4e012d9#diff-4e012d97ece45b455806fabbe1f93ce11af56213b9137463a427e1c5d4d8bda6

This is an implementation spike of storing delegations in DynamoDB rather than D1.

Because we already use DynamoDB in the upload-api, it made sense to reuse the configuration and testing infrastructure we've already built there. I copied a number of interfaces from https://github.com/web3-storage/w3up/tree/main/packages/access-api to get this done, and will migrate back to them if that ends up making sense.

As this implies, the path forward for this is somewhat unclear and I need feedback and consensus from the team to keep this moving.

We have a couple options to move forward:

1) Port the code in this PR over to the `access-api` repository and run it inside a cloudflare worker. This will require us to port some of the configuration and testing utilities in this repository to that repository and will leave us with a situation where compute in Cloudflare is reading and writing to DynamoDB in AWS. It will also require us to change the S3 logic in this PR to operate against R2, but that may Just Work thanks to theoretical API compatibility.

This will also result in AWS-related logic being split between this repository and w3up, and we will probably want to extract common utilities to a separate package at some point in the future.

2) Port the delegations-related service endpoints in `w3up/access-api` over to this repository and get them running in an AWS Lambda. In this scenario we could either use S3 as implemented in this PR or continue to store delegations in R2.

This option would move us in the direction of abstracting all operational details out of the w3up repository, following the pattern already used by w3up's `upload-api` package, which is imported by `upload-api` in this package.

In this option I think it probably makes sense to migrate existing delegations from R2 to S3, but could be wrong on that front.

My conversations with @Bengo suggested that the team may be mostly aligned behind option (2), especially because it seems like it moves us in the direction of isolating operational details of our services to this repository and turning the w3ui `*-api` packages into abstract implementations of the service providers we implement.

Thoughts?
implement a test that represents my current best understanding of how provisions should work for now.
travis added a commit to storacha/w3up that referenced this pull request May 15, 2023
travis added a commit to storacha/w3up that referenced this pull request May 16, 2023
@Gozala doesn't like them and made a good argument against this here: storacha/w3infra#194 (comment)
- add necessary context to run new providers
- add validate-email GET and POST handlers as a new Lambda
@travis travis changed the title initial implementations of DynamoDB-based delegation and provision storage services feat!: fold access-api into upload-api May 17, 2023
@travis travis requested a review from olizilla May 17, 2023 17:43
@seed-deploy
Copy link

seed-deploy bot commented Jun 26, 2023

Stack outputs updated
  • pr194-w3infra-CarparkStack

    Name Value
    BucketName carpark-pr194-0
    Region us-east-2
  • pr194-w3infra-SatnavStack

    Name Value
    BucketName satnav-pr194-0
    Region us-east-2
  • pr194-w3infra-UploadApiStack

    Name Value
    ApiEndpoint https://uv7re3yazj.execute-api.us-east-2.amazonaws.com
    CustomDomain https://pr194.up.web3.storage
  • pr194-w3infra-BusStack

  • pr194-w3infra-ReplicatorStack

  • pr194-w3infra-UcanInvocationStack

  • pr194-w3infra-UploadDbStack

I'll reintroduce these in a separate branch
Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

This LGTM! Thanks for the great work here @travis

As we talked, let's get the migration code here and tested in staging only next week. So that we don't have main branch blocked until then

To run:

```
# first ensure you are logged in to wrangler with `wrangler login`
STAGE=pr194 npm run d1-dynamo-migration
```

---------

Co-authored-by: Irakli Gozalishvili <[email protected]>
@seed-deploy seed-deploy bot temporarily deployed to pr194 July 13, 2023 18:12 Inactive
@seed-deploy seed-deploy bot temporarily deployed to pr194 July 13, 2023 18:36 Inactive
@travis travis requested a review from vasco-santos July 13, 2023 18:52
@seed-deploy seed-deploy bot temporarily deployed to pr194 July 13, 2023 23:53 Inactive
grab data from D1 and use it to verify that all the bits got moved
correctly

during the course of this I found a bug in the migration script - whew!
thanks for the suggestion @vasco-santos

also add a simple utility to list the emails of folks who have
registered for w3up
emails don't seem to be making it through within 30 seconds
tools/d1-migration/add-to-dynamo.js Show resolved Hide resolved
Comment on lines +27 to +48
* Abstraction layer to handle operations on Store Table.
*
* @param {string} region
* @param {string} tableName
* @param {object} [options]
* @param {string} [options.endpoint]
*/
export function createSubscriptionTable (region, tableName, options = {}) {
const dynamoDb = new DynamoDBClient({
region,
endpoint: options.endpoint,
})

return useSubscriptionTable(dynamoDb, tableName)
}

/**
* @param {DynamoDBClient} dynamoDb
* @param {string} tableName
* @returns {SubscriptionTable}
*/
export function useSubscriptionTable (dynamoDb, tableName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not want to block this PR, but I still dislike amount of incidental complexity we have for every single table which I have described a solution for in another PR, that I think applies here just as well

storacha/w3filecoin-infra#26 (comment)

Perhaps we could remove some of this complexity later as a cleanup.

Copy link
Member Author

Choose a reason for hiding this comment

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

I hear you on the incidental complexity, but to be honest this boilerplate doesn't bother me much - having one initializer per table that takes a generic client and another that takes a set of options to set up a client in a default way feels pretty normal and good to me.

That said I'm definitely not opposed to putting together a cleaner/easier-to-use abstraction for this. From my read of the comment thread you linked, @vasco-santos identified some additional complexity that isn't accounted for by the solution you propose.

If you're up for it, I think it would be a good idea for you to do a quick spike (including integrating it into tests and other places where we use these initializers - migration scripts, etc) of what you're thinking on a single table and then have the three of us do a (high bandwidth maybe?) review of the solution to make sure it fits all our needs and is strictly better than what we have right now. I'd be more than happy to take that spike and run with it, updating our usage across the board. I'd also be down to do the spike, but would definitely need to sync with you and Vasco to summarize and dig into the thinking on this thread so far.

All that said, I appreciate you looking out for these abstractions! I'm adding yet another table so I do think investing in this will help us keep things readable and usable.

@seed-deploy seed-deploy bot temporarily deployed to pr194 July 17, 2023 22:55 Inactive
@travis travis merged commit f2a7a11 into main Jul 18, 2023
1 check passed
@travis travis deleted the feat/dynamo-delegations-provisions branch July 18, 2023 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants