backend.ai icon indicating copy to clipboard operation
backend.ai copied to clipboard

feat: implement atomic vfolder host permission

Open fregataa opened this issue 2 years ago • 2 comments

ref: https://github.com/lablup/backend.ai/issues/753#issuecomment-1260603825

  1. Migrate domains.allowed_vfolder_hosts, groups.allowed_vfolder_hosts, keypair_resource_policies.allowed_vfolder_hosts from ARRAY of string to JSONB

etc)

# Before
domains.allowed_vfolder_hosts = ["storage1"]
domains.allowed_vfolder_hosts = ["storage2"]

# After
domains.allowed_vfolder_hosts = {"storage1": ["mount", "create-vfolder"]}
domains.allowed_vfolder_hosts = {"storage2": ["*"]}
  1. Impl PermissionListColumn which can be used by other permission spec columns.

fregataa avatar Oct 13 '22 15:10 fregataa

Please set PR status to "Ready for review" when you've done the final touches.

kyujin-cho avatar Oct 14 '22 07:10 kyujin-cho

@yomybaby @lizable @Sujin-Kim1 There should be update in frontend since allowed_vfolder_hosts field of Domain, Group,KeyPairResourcePolicy graphene and db schema will change in this PR. There is an alternative, just letting allowed_vfolder_hosts as a legacy field and adding allowed_vfolder_host_map JSON field.

Please give me your opinion.

fregataa avatar Oct 19 '22 08:10 fregataa

IMO, there are a few requests written in the comments and also one more thing to be fixed before merging this PR. TL;DR: We need to change the code in here(src/ai/backend/manager/model/gql.py#1428)

https://github.com/lablup/backend.ai/blob/40c04807d9b03040218b39ead81262ff483e79f2/src/ai/backend/manager/models/gql.py#L1415

to this mutation_cls = getattr(Mutations, info.field_name).type Considering the overall key allowed_vfolder_hosts are located in several policies (such as domains, groups(project groups, and keypair_resource_policies), I think It's quite crucial to show the overview of access in those policies easily. In light of this, It's not surprising that users may want features for updating those access controls at once. Therefore, I decided to chain mutations so that It reduces(not completely, but some) repetitive requests to manager server. To make chained mutations, it needs aliasing. Unfortunately, In current code, It regards aliasing string as one of the variables in Mutations class, which would definitely throw an error. I checked many graphql requests and It doesn't affect any mutations at all. It would be appreciated you to apply the new code described above.

It's better to handle this in as a separate issue(#879), which is already opened by @yomybaby .

lizable avatar Nov 15 '22 02:11 lizable

@fregataa Could you merge the diff from main branch?

lizable avatar Nov 16 '22 01:11 lizable