policy: Introduce ListTargets method for Policy contract #79

aarifullin · 2024-02-09T14:31:09Z

aarifullin commented

2024-02-09 14:31:09 +00:00

Introduce a new method ListTargets that lists targets by kind.
Slightly fix key mapping - also concatenate kind to prefix.
Write unit-tests.
Regenerate rpcclient.

Close #78

* Introduce a new method ListTargets that lists targets by kind. * Slightly fix key mapping - also concatenate kind to prefix. * Write unit-tests. * Regenerate rpcclient. Close #78

aarifullin requested review from storage-core-committers 2024-02-09 14:41:25 +00:00

aarifullin requested review from storage-core-developers 2024-02-09 14:41:25 +00:00

aarifullin requested review from storage-services-committers 2024-02-09 14:41:39 +00:00

aarifullin requested review from storage-services-developers 2024-02-09 14:41:39 +00:00

aarifullin reviewed 2024-02-09 14:50:37 +00:00

policy/policy_contract.go Outdated

					
				@ -216,2 +220,4 @@

					return storage.Find(ctx, keyPrefix, storage.ValuesOnly)

				}

				// ListTargets lists targets for which rules are defined by kind.

aarifullin commented

2024-02-09 14:50:37 +00:00

@fyrchik Since we have decided a container/namespace will get default policy with deny, we should consider a migration for already created containers. len(targets) == len(containers) + len(namespaces). That means listing containers may be a problem if the container number is huge. But we can't use iterators - we need iterators with filters, because we should check if at least one chain is defined for a name

@fyrchik Since we have decided a container/namespace will get default policy with `deny`, we should consider a migration for already created containers. `len(targets) == len(containers) + len(namespaces)`. That means listing containers may be a problem if the container number is huge. But we can't use iterators - we need iterators with filters, because we should check if at least one chain is defined for a name

fyrchik commented

2024-02-12 07:04:34 +00:00

Why do we need any filters? If a key exist, there must be some chain.

aarifullin commented

2024-02-12 10:49:12 +00:00

If a key exist, there must be some chain.

This cannot be always true. If a chain existed for a while and then removed, then the mapping is left anyway (if want to remove the mapping we need to impose remove methods to check if no chains are defined for a target. Also, this is helpful because a new chain can be added to a target again).
So, briefly, even the mapping is found it does not mean a chain is defined for a target

> If a key exist, there must be some chain. This cannot be _always_ true. If a chain existed for a while and then removed, then the mapping is left anyway (if want to remove the mapping we need to impose remove methods to check if no chains are defined for a target. Also, this is helpful because a new chain can be added to a target again). So, briefly, even the mapping is found it does not mean a chain is defined for a target

dstepanov-yadro commented

2024-02-13 11:35:49 +00:00

Is it possible to add batchSize and last as arguments? Then you can use something like this:

for k, _ := c.Seek(prm.NextPageToken); k != nil; k, _ = c.Next() {

Is it possible to add `batchSize` and `last` as arguments? Then you can use something like this: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/3a41858a0f2b02f426d37abe9d9a39ca1835436f/pkg/local_object_storage/pilorama/boltdb.go#L1224

aarifullin commented

2024-02-14 16:18:12 +00:00

I suppose such result "paging" is possible only if some isolation can be supported to not get range invalidated. @fyrchik WDYT, is it possible in contracts?

fyrchik commented

2024-02-15 07:52:56 +00:00

Sessions are the only way to do paging correctly.

👍 1

fyrchik commented

2024-02-15 07:54:17 +00:00

we need to impose remove methods to check if no chains are defined for a target

TBH I do not see a problem here, besides counter issues.
To make possible number reuse we can even store rule count=0 instead of removal

>we need to impose remove methods to check if no chains are defined for a target TBH I do not see a problem here, besides counter issues. To make possible number reuse we can even store rule count=0 instead of removal

aarifullin commented

2024-02-16 15:12:51 +00:00

I hope I properly got your idea. I introduced active rules count for a target. That helps to just check counter instead listing chains

dstepanov-yadro approved these changes 2024-02-15 10:50:27 +00:00

dkirillov approved these changes 2024-02-15 11:58:28 +00:00

acid-ant approved these changes 2024-02-15 12:14:35 +00:00

aarifullin changed title from ~~policy: Introduce ListTargets method for Policy contract~~ to WIP: policy: Introduce ListTargets method for Policy contract

2024-02-16 13:18:40 +00:00

aarifullin force-pushed feat/78-list_targets from dfddd11f71 to 31ec39fbcc

2024-02-16 13:19:41 +00:00

Compare

aarifullin force-pushed feat/78-list_targets from 31ec39fbcc to 1d4deffb7d

2024-02-16 15:11:18 +00:00

Compare

aarifullin changed title from ~~WIP: policy: Introduce ListTargets method for Policy contract~~ to policy: Introduce ListTargets method for Policy contract

2024-02-16 15:11:44 +00:00

aarifullin requested review from dstepanov-yadro 2024-02-16 15:13:13 +00:00

aarifullin requested review from dkirillov 2024-02-16 15:13:14 +00:00

aarifullin requested review from acid-ant 2024-02-16 15:13:14 +00:00

aarifullin commented

2024-02-19 12:07:18 +00:00

Cannot be updated due to some gitea bug :( will be reopened

aarifullin closed this pull request

2024-02-19 12:07:18 +00:00

aarifullin referenced this pull request

2024-02-19 12:08:38 +00:00

[#78] policy: Introduce ListTargets method for Policy contract #80