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

Anoncreds - support for anoncreds and askar wallets concurrently #2807

Closed
ianco opened this issue Feb 21, 2024 · 16 comments
Closed

Anoncreds - support for anoncreds and askar wallets concurrently #2807

ianco opened this issue Feb 21, 2024 · 16 comments
Assignees
Labels
AnonCreds Ledger Agnostic AnonCreds

Comments

@ianco
Copy link
Contributor

ianco commented Feb 21, 2024

Support starting aca-py with both the credx and anoncreds libraries loaded, and support (in multitenancy) tenants with both askar and askar-anoncreds wallet types concurrently. (This is necessary to allow tenants in a multi- environment to upgrade their sub-wallets from askar to askar-anoncreds at their convenience. (Note that the upgrade function will be documented in a separate issue.)

When starting up aca-py, load both the credx and anoncreds libraries, and enable the API endpoints for both libraries (schema, credential definition and revocation). Ensure that the endpoints are under different url's. (Schema and cred def are currently under different url's - the "new" API's are under /anoncreds. For revocation, the new endpoints will need to be moved to the /anoncreds url, alongside the schema and cred def.

In each API endpoint that is duplicated, add a check to each endpoint to make sure it is being called with the correct wallet type (for example for the new anoncreds-related API's, if the wallet type is askar then return an error code.)

When in multi-tenancy mode, create new tenant wallets using the overall aca-py's configured wallet type. (For example if aca-py is started with --wallet-type askar then created tenant wallets with askar type, likewist with askar-anoncreds.)

When running in multi-tenant mode, ensure that the tenant wallet's wallet type is in scope for all processing related to that tenant (i.e. Admin API calls and processing for received agent-to-agent messages).

On aca-py startup, verify that the configured --wallet-type matches the actual database. If the wallet is askar and aca-py is started with --wallet-type askar-anoncreds, aca-py should shut down with an error message. Likewise if the wallet is askar-anoncreds and aca-py is started with --wallet-type askar. If there is a database upgrade in progress (askar to askar-anoncreds, see issue #2808) then aca-py should allow startup but the only endpoint available will be the upgrade endpoint. (Note that for a multi-tenant scenario, this just applies to the base wallet.)

@ianco ianco added the AnonCreds Ledger Agnostic AnonCreds label Feb 21, 2024
@WadeBarnes
Copy link
Contributor

WadeBarnes commented Feb 22, 2024

My vote is for a versioned API approach. IMO this will enable the API to remain consistent from a endpoint path perspective between versions where appropriate (such as with the schema, cred-def, and revocation endpoints) which will in-turn make migration from one API version to another easier for developers; consistent APIs with different parameters/functionality between versions.

I think this will also enable:

  • Setting a default API, in the case an API version is not explicitly defined by the caller. The default can be moved up as support for deprecated APIs is removed.
  • Making it easier to introduce new versions of existing APIs (aka introduce breaking changes), and deprecate unsupported API versions.
  • Easily eliminating deprecated API versions from subsequent releases.
  • The ability to provide deprecation warnings and migration recommendations to those calling deprecated APIs in a consistent manner.
  • The ability to easily load a single API version for a given tenant/client.

My thought is a given API version contains a full set of APIs including those that have not changed from the previous version, along with new APIs and those containing breaking changes. That way we don't run into the issue where unchanged APIs remain at the version they were introduced and we have to support old API versions.

New APIs can be introduced into the current API version since they don't impact existing API endpoints.

@WadeBarnes
Copy link
Contributor

When in multi-tenancy mode, create new tenant wallets using the overall aca-py's configured wallet type. (For example if aca-py is started with --wallet-type askar then created tenant wallets with askar type, likewist with askar-anoncreds.) NOTE this needs to be confirmed.

Agreed. This is a logical default for creating tenant wallets.

@swcurran
Copy link
Contributor

@WadeBarnes what are the rules you are suggesting for the versions changing?

  • When any new endpoint is added, or when any endpoint changes, all update to v2?
  • Every endpoint is versioned separately?

@WadeBarnes
Copy link
Contributor

  • When any new endpoint is added, or when any endpoint changes, all update to v2?
    New APIs can be introduced into the current API version since they don't impact existing API endpoints.
  • Every endpoint is versioned separately?
    They are versioned together as a set.

Details:

  • Only (breaking) changes to an existing endpoint trigger a new API version. i.e. Changes to the APIs that impact existing implementations or existing functionality, or the removal of an endpoint. Such as how the migration to askar-anoncreds impacts the Schema, Cred-Def, and Revocation functionality and endpoints.
  • New APIs (new functionality) can safely be added to an existing, the current, API version, since there are no changes to existing APIs, therefore existing implementations are not impacted. So, there is no reason to complicate things by introducing a new API version.
  • When a new API version is introduced, it exposes the complete API. Existing APIs that have not been changed, along with the ones that have. This allows the previous API version to be marked as deprecated and eventually removed. This also has a bit of a side affect for those using the default API version. Provided the endpoints they are using are not impacted by breaking changes or removed things just continue to work.

@swcurran
Copy link
Contributor

Thanks, Wade. That makes sense.

The downside of that approach is that when removing an API, everyone has to deploy a new controller, even if the endpoints they use have not changed. That will be worse when the motivation to bump the API version is required, but isolated to a small number of endpoints. For example, in the current case, no verifiers are affected by the schema/creddef/revreg, but in order to get rid of the “old code” related to those endpoints, every controller (including verifiers) will have to update their code to change to the “/v1” endpoints. Or we start being lienent on the rules (which is what I worry about) and we keep some “unversioned” endpoints (unchanged ones) and delete others. That makes fuzzy the definition of what is the “vX API”.

Obviously there is no right/wrong answer. So I’m not hard fast against, but my experience points to the “rename endpoints” and avoiding the “API vX” definition as an easier approach in the long run.

A couple of other reasons that I think make the ACA-Py scenario a little different:

  • We only publish ACA-Py, and it is deployed by many others. We can’t (for example) monitor the use of specific endpoints as we could in centralized system to know when to, for example, remove old endpoints.
  • plugins that add Admin API endpoints add to the fun. Should they be versioned as well? Presumably, they can’t be via the same numbering scheme.

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Feb 22, 2024

I don't think removing an API necessarily requires people to deploy a new controller. That's where the default version comes in. If the controller does not specify a version or the version the controller using does not exist any more, I see the call using the default API version. Properly versioned APIs don't impose the use of the /v* in the path. You can specify the API version in headers; OrgBook has this feature.

Further, there would be a sufficient amount of time from the point an API was deprecated to when it was removed from the code to allow clients to upgrade. The host of the service also has control over when and to what version of ACA-Py they upgrade, and are responsible for communicating this to they clients.

@swcurran
Copy link
Contributor

Ah…I was not aware of the API version in the header. Could you point me to how that works?

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Feb 22, 2024

Some examples in the code documentation:

@ianco
Copy link
Contributor Author

ianco commented Feb 22, 2024

The difference between orgbook api versioning and aca-py is that for orgbook the entire api is versioned, and for aca-py each protocol is versioned independently. So there's no "overall api version" - if we use the version parameters in the header then it needs to be targeted at each protocol individually. And since protocols can be loaded as plug-ins we have no overall control over what api's and versions that aca-py will be running.

The existing schema, cred def and revocation api's have no version info, so if we add the new version of the api's under a /v2 then we'll need to also move the existing api's to a /v1 url and build in handling for when a non-versioned api is called. (As Wade mentions we can handle this the same was as for orgbook however it will need to be implemented for each protocol and not for aca-py overall.)

@swcurran
Copy link
Contributor

Worse — some Admin API calls are not part of DIDComm protocols and so today they don’t have a version (e.g. the schema/creddef/revreg, the set tenant settings, etc.). So it will create confusion adding an “Admin API version” in conjunction with some endpoints already having a “DIDComm Protocol Versions”. The plugins add to the fun, as the full set of endpoints is not actually determined until the ACA-Py instance is deployed — a runtime determination.

My preference (not hard and fast :-) ) is back to not having an “API Version”.

@ianco
Copy link
Contributor Author

ianco commented Feb 23, 2024

I've updated the main description per the comments, @swcurran @WadeBarnes @jamshale

@jamshale jamshale self-assigned this Feb 27, 2024
@jamshale jamshale moved this to In Progress in CDT Enterprise Apps Feb 29, 2024
@jamshale
Copy link
Contributor

For:
On aca-py startup, verify that the configured --wallet-type matches the actual database. If the wallet is askar and aca-py is started with --wallet-type askar-anoncreds, aca-py should shut down with an error message. Likewise if the wallet is askar-anoncreds and aca-py is started with --wallet-type askar.

I'm trying to determine a way to tell if the existing wallet/DB is askar or askar-anonreds, because they both use Askar storage. Anyone know of a record type I can look for? Or another approach?

I was looking at #2384 but working with an encrypted DB, i'm not sure how I can use it. And that would only work for a DB/wallet that has a rev reg record.

@ianco
Copy link
Contributor Author

ianco commented Feb 29, 2024

I think we store a version number in the database, so we could store the storage type in the same way. If there is no storage type, then assume it's "askar". And then we could store a value for "anoncreds" (which we would store on creation of the wallet, or on upgrade from askar to anoncreds).

@swcurran
Copy link
Contributor

We do something similar in the upgrade.py that was updated here: https://github.com/hyperledger/aries-cloudagent-python/pull/2185/files

We are doing something similar, but instead of using the version number, we’re using the wallet type string.

And then we could store a value for "anoncreds" (which we would store on creation of the wallet, or on upgrade from askar to anoncreds).

Presumably we want to call it “askar-anoncreds” to match the —wallet-type option, right?

then assume it's "askar”.

Is there anything more we should do to prevent this from being run when the wallet-type is “indy”? Or is that already considered?

@jamshale
Copy link
Contributor

jamshale commented Mar 1, 2024

This ☝️ makes sense. But, I am a bit unsure about what to do with and indy wallet and config. I was thinking if the record is None and the first config it gets is either indy or askar that it will use that as the storage type? It would only cause problems if the first config it receives it the wrong type. Say it's an indy wallet and first config is askar, or vice versa.

If it gets askar-anoncreds config and a non empty DB then it will assume the storage type is askar and fail. If the DB is empty then it can startup and set the storage type to askar-anoncreds.

@ianco
Copy link
Contributor Author

ianco commented Mar 1, 2024

But, I am a bit unsure about what to do with and indy wallet and config

I think if you try to open an Indy wallet using Askar it will throw an exception.

I suggest we just document that people will need to upgrade Indy to Askar (if they are still running an Indy wallet) and then we don't need to do anything else.

@jamshale jamshale moved this from In Progress to In Review in CDT Enterprise Apps Mar 6, 2024
@jamshale jamshale moved this from In Review to Complete in CDT Enterprise Apps Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AnonCreds Ledger Agnostic AnonCreds
Projects
None yet
Development

No branches or pull requests

4 participants