cli: Read ape rule from JSON file #989

Merged
fyrchik merged 4 commits from aarifullin/frostfs-node:feat/cli_parse_json into master 2024-02-20 07:42:30 +00:00
Member
  • Make add-rule command also read rules from JSON files or pass
    JSON as string.
  • Parse new statement any=true/false.
* Make add-rule command also read rules from JSON files or pass JSON as string. * Parse new statement any=true/false.
aarifullin requested review from storage-core-committers 2024-02-15 18:17:26 +00:00
aarifullin force-pushed feat/cli_parse_json from 41f7981607 to 88939c0a53 2024-02-15 18:17:27 +00:00 Compare
dstepanov-yadro approved these changes 2024-02-16 05:53:12 +00:00
fyrchik requested changes 2024-02-16 07:07:45 +00:00
@ -4,3 +4,3 @@
APE is a replacement for eACL. Each rule can restrict somehow access to the object/container or list of them.
Here is a simple representation for the rule:
`<status>[:status_detail] <action>... <condition>... <resource>...`
`<status>[:status_detail] <action>... [any=true/false] <condition>... <resource>...`
Owner

Is it related to the commit?

Is it related to the commit?
Author
Member

I have splitted in 3 commits. Check this out

I have splitted in 3 commits. Check this out
@ -50,2 +47,3 @@
chain.ID = apechain.ID(chainIDRaw)
serializedChain := chain.Bytes()
if ruleStmt, _ := cmd.Flags().GetStringArray(ruleFlag); len(ruleStmt) > 0 {
Owner

Why have you decided to go with 2 separate flags, second of which has 2 meanings?
See e.g. cmd/frostfs-cli/internal/common/token.go:ReadBinaryOrJSON() function: can we avoid the second parameter?

Why have you decided to go with 2 separate flags, second of which has 2 meanings? See e.g. `cmd/frostfs-cli/internal/common/token.go:ReadBinaryOrJSON()` function: can we avoid the second parameter?
Author
Member

First of all, I have made a mistake: the flag ruleJSON must be renamed to path. Rules are statements while path is a whole chain itself (id, conds, matchType etc.). Also, rules can be passed as an array of strings while path is the only one

First of all, I have made a mistake: the flag `ruleJSON` must be renamed to `path`. Rules are statements while `path` is a whole chain itself (id, conds, matchType etc.). Also, rules can be passed as an array of strings while `path` is the only one
Author
Member

I have borrowed the idea with ReadBinaryOrJSON but adopted it to ape chain parsing. But still I insist on separate flag --path. I expalined that above

I have borrowed the idea with `ReadBinaryOrJSON` but adopted it to ape chain parsing. But still I insist on separate flag `--path`. I expalined that above
@ -105,6 +108,16 @@ func parseRuleLexemes(r *apechain.Rule, lexemes []string) error {
var isObject *bool
for i, lexeme := range lexemes[1:] {
anyVal, anyErr := parseAny(lexeme)
Owner

It is a new feature, not related to JSON format acceptance, could you move it to a separate commit?
Also, why is it any=true/false and not e.g. any/all or even any and all by default?

It is a new feature, not related to JSON format acceptance, could you move it to a separate commit? Also, why is it `any=true/false` and not e.g. `any`/`all` or even `any` and `all` by default?
Author
Member

I have introduced any and all statement

I have introduced `any` and `all` statement
acid-ant approved these changes 2024-02-16 08:12:40 +00:00
aarifullin changed title from cli: Read ape rule from JSON file to cli: Read ape rule from JSON file 2024-02-16 11:02:21 +00:00
aarifullin changed title from cli: Read ape rule from JSON file to cli: Read ape rule from JSON file 2024-02-16 11:02:35 +00:00
aarifullin changed title from cli: Read ape rule from JSON file to cli: Read ape rule from JSON file 2024-02-16 11:02:44 +00:00
aarifullin changed title from cli: Read ape rule from JSON file to cli: Read ape rule from JSON file 2024-02-16 11:03:01 +00:00
aarifullin force-pushed feat/cli_parse_json from 2baa648245 to a5713ebda9 2024-02-16 11:16:06 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2024-02-16 11:30:00 +00:00
aarifullin requested review from acid-ant 2024-02-16 11:30:01 +00:00
aarifullin requested review from fyrchik 2024-02-16 11:30:03 +00:00
aarifullin force-pushed feat/cli_parse_json from a5713ebda9 to 1307794ccb 2024-02-16 13:59:33 +00:00 Compare
dstepanov-yadro approved these changes 2024-02-19 13:08:42 +00:00
fyrchik approved these changes 2024-02-19 13:18:59 +00:00
@ -60,0 +68,4 @@
if err != nil {
err = chain.UnmarshalJSON(data)
if err != nil {
return errors.New("invalid format")
Owner

What about passing context for both errors?

What about passing context for both errors?
Author
Member

I hope I got your point correctly - I've made the error more verbose adding %w
If you've been talking about adding cmd.Context(), then it wouldn't be correct because Parse* methods are placed in util package and also used by unit-tests

I hope I got your point correctly - I've made the error more verbose adding `%w` If you've been talking about adding `cmd.Context()`, then it wouldn't be correct because `Parse*` methods are placed in `util` package and also used by unit-tests
aarifullin force-pushed feat/cli_parse_json from 1307794ccb to 6f9911e915 2024-02-19 14:05:26 +00:00 Compare
aarifullin force-pushed feat/cli_parse_json from 6f9911e915 to d3a6e289be 2024-02-19 14:47:43 +00:00 Compare
Author
Member

Please, check the last commit out. I have cherry-picked it from #986

I will revert the commit there

Please, check the last commit out. I have cherry-picked it from [#986](https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/986) I will revert the commit there
fyrchik approved these changes 2024-02-19 19:36:25 +00:00
fyrchik merged commit 47d9ce71be into master 2024-02-20 07:42:30 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#989
No description provided.