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

Allow Configuration Settings on a per-tenant basis #2233

Merged
merged 12 commits into from
Jun 28, 2023

Conversation

shaangill025
Copy link
Contributor

@swcurran
Copy link
Contributor

Thanks, @shaangill025 — let us know about the test failures and we’ll look at moving this forward.

@shaangill025 shaangill025 force-pushed the per_tenant_settings branch from 585ccc4 to ac47f88 Compare June 1, 2023 17:08
@shaangill025
Copy link
Contributor Author

@swcurran I have added the documentation, this PR is ready for review.

Multitenancy.md Outdated Show resolved Hide resolved
@swcurran
Copy link
Contributor

The goal was to get this into 0.8.2, but since the developer has not been available to work on this, it may not make it.

Copy link
Contributor

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

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

I did not test the multiledger portion of this, just the tenant extra settings.

This is probably up for discussion. Should this (tenant extra settings) be a plugin?

In any case, what is missing is the tenant managing their settings. Currently, the base wallet/admin is the one creating the settings and updating them. A tenant/sub-wallet cannot access the /multitenancy APIs. I think the current changes can stay, but we should have another API /settings (?) that requires a tenant/sub-wallet token and they can manage those settings.

Tagging @loneil so the Traction team can review and chime in.

Multitenancy.md Outdated Show resolved Hide resolved
Multitenancy.md Show resolved Hide resolved
aries_cloudagent/multitenant/admin/routes.py Show resolved Hide resolved
@shaangill025 shaangill025 force-pushed the per_tenant_settings branch from 2ff2c47 to daa33ed Compare June 21, 2023 23:59
@shaangill025
Copy link
Contributor Author

@usingtechnology The requested changes have been added.

@usingtechnology
Copy link
Contributor

usingtechnology commented Jun 22, 2023

@shaangill025 - I would expect the tenant /settings API to update/return the small set of values not ALL of the settings. I realize your logic will only update the given set, but the API itself should NOT return all the settings. There are definitely things in there a tenant should not know. Can we limit to the set you provided? Also, I don't think those changes are persistent over server restarts. It doesn't appear that we are reading or writing from the wallet record (where the extra settings are initially stored).

Also, sorry if I wasn't really clear. The config/arg_parse is pretty confusing. That secondary mapping I was thinking about would match what someone puts into config.yml, so it's generally the dash separated key. Like log-level, auto-ping-connection, auto-accept-invites.

Signed-off-by: Shaanjot Gill <[email protected]>
Signed-off-by: Shaanjot Gill <[email protected]>
Signed-off-by: Shaanjot Gill <[email protected]>
Signed-off-by: Shaanjot Gill <[email protected]>
Signed-off-by: Shaanjot Gill <[email protected]>
Signed-off-by: Shaanjot Gill <[email protected]>
Copy link
Contributor

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

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

Again, I haven't tested the ledger/indy_vdr changes, but the per tenant settings is almost ready to go. One simple change for the update API response.

aries_cloudagent/settings/routes.py Outdated Show resolved Hide resolved
Signed-off-by: Shaanjot Gill <[email protected]>
Copy link
Contributor

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

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

great work @shaangill025

Multitenancy.md Show resolved Hide resolved
aries_cloudagent/settings/routes.py Outdated Show resolved Hide resolved
Multitenancy.md Outdated Show resolved Hide resolved
Signed-off-by: Shaanjot Gill <[email protected]>
Signed-off-by: Shaanjot Gill <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

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.

Multitenancy: expose agent configuration settings on a per-tenant basis
6 participants