Refactor APE-related commands #1501

Merged
fyrchik merged 8 commits from aarifullin/frostfs-node:feat/refactor_ape_cli into master 2024-11-20 07:58:36 +00:00
2 changed files with 144 additions and 0 deletions
Showing only changes of commit 07c96414d0 - Show all commits

View file

@ -1,13 +1,43 @@
package ape package ape
import ( import (
"encoding/hex"
"errors"
"fmt" "fmt"
"strconv" "strconv"
"strings"
commonCmd "git.frostfs.info/TrueCloudLab/frostfs-node/cmd/internal/common"
apeutil "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/util/ape"
cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id"
apechain "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain" apechain "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain"
"git.frostfs.info/TrueCloudLab/policy-engine/pkg/engine"
"github.com/nspcc-dev/neo-go/cli/input"
"github.com/spf13/cobra" "github.com/spf13/cobra"
) )
const (
defaultNamespace = "root"
namespaceTarget = "namespace"
containerTarget = "container"
userTarget = "user"
groupTarget = "group"
Ingress = "ingress"
S3 = "s3"
)
var mChainName = map[string]apechain.Name{
Ingress: apechain.Ingress,
S3: apechain.S3,
}
var (
errSettingDefaultValueWasDeclined = errors.New("setting default value was declined")
errUnknownTargetType = errors.New("unknown target type")
errUnsupportedChainName = errors.New("unsupported chain name")
)
// PrintHumanReadableAPEChain print APE chain rules. // PrintHumanReadableAPEChain print APE chain rules.
func PrintHumanReadableAPEChain(cmd *cobra.Command, chain *apechain.Chain) { func PrintHumanReadableAPEChain(cmd *cobra.Command, chain *apechain.Chain) {
cmd.Println("Chain ID: " + string(chain.ID)) cmd.Println("Chain ID: " + string(chain.ID))
@ -39,3 +69,98 @@ func PrintHumanReadableAPEChain(cmd *cobra.Command, chain *apechain.Chain) {
} }
} }
} }
// ParseTarget handles target parsing of an APE chain.
func ParseTarget(cmd *cobra.Command) engine.Target {
typ := ParseTargetType(cmd)
name, _ := cmd.Flags().GetString(TargetNameFlag)
switch typ {

This looks like a bad idea: you have taken two different functions and created a new one with a bool flag which switches the behaviour. The amount of different code between these functions is comparable to their size. And they were not that long before.
Let's leave them as they were.

This looks like a bad idea: you have taken two different functions and created a new one with a bool flag which switches the behaviour. The amount of different code between these functions is comparable to their size. And they were not that long before. Let's leave them as they were.

Now I am wondering: it's fine, if we omit this flag but leave prompt anyway. Prompt is used in frostfs-cli. frostfs-adm also correctly receives and processes --target-name root, but ignores empty input (--target-name "" is taken for fine input). I think that won't be wrong if frostfs-adm will also output this prompt. AFAIR, autotests use only --target-name root

Now I am wondering: it's fine, if we omit this flag but leave prompt anyway. Prompt is used in `frostfs-cli`. `frostfs-adm` also correctly receives and processes `--target-name root`, but ignores empty input (`--target-name ""` is taken for fine input). I think that won't be wrong if `frostfs-adm` will also output this prompt. AFAIR, autotests use only `--target-name root`
case engine.Namespace:
if name == "" {
ln, err := input.ReadLine(fmt.Sprintf("Target name is not set. Confirm to use %s namespace (n|Y)> ", defaultNamespace))
commonCmd.ExitOnErr(cmd, "read line error: %w", err)
ln = strings.ToLower(ln)
if len(ln) > 0 && (ln[0] == 'n') {
commonCmd.ExitOnErr(cmd, "read namespace error: %w", errSettingDefaultValueWasDeclined)
}
name = defaultNamespace
}
return engine.NamespaceTarget(name)
case engine.Container:
var cnr cid.ID
commonCmd.ExitOnErr(cmd, "can't decode container ID: %w", cnr.DecodeString(name))
return engine.ContainerTarget(name)
case engine.User:
return engine.UserTarget(name)
case engine.Group:
return engine.GroupTarget(name)
default:
commonCmd.ExitOnErr(cmd, "read target type error: %w", errUnknownTargetType)
}
return engine.Target{}
fyrchik marked this conversation as resolved Outdated

Here too. I would also move it to default to communicate that the switch is exhaustive.

Here too. I would also move it to `default` to communicate that the switch is exhaustive.

fixed

fixed
}
// ParseTargetType handles target type parsing of an APE chain.
func ParseTargetType(cmd *cobra.Command) engine.TargetType {
typ, _ := cmd.Flags().GetString(TargetTypeFlag)
switch typ {
case namespaceTarget:
return engine.Namespace
case containerTarget:
return engine.Container
case userTarget:
return engine.User
case groupTarget:
return engine.Group
}
commonCmd.ExitOnErr(cmd, "parse target type error: %w", errUnknownTargetType)
return engine.TargetType(0)
fyrchik marked this conversation as resolved
Review

I think it is more idiomatic to panic with panic("unreachable").

I think it is more idiomatic to panic with `panic("unreachable")`.
Review

fixed

fixed
}
// ParseChainID handles the parsing of APE-chain identifier.
// For some subcommands, chain ID is optional as an input parameter and should be generated by
// the service instead.
func ParseChainID(cmd *cobra.Command) (id apechain.ID) {

This is very dirty: you use some obscure cobra internal in order to be able to use a single function from 2 places. Just copy the function, no problem.
And again, it questions the benefit of this refactoring.

This is very dirty: you use some obscure cobra internal in order to be able to use a single function from 2 places. Just copy the function, no problem. And again, it questions the benefit of this refactoring.

Just copy the function, no problem

Okay. Also, as flags are already marked Required, we don't need this extra checks at all - user will receive error from subcommands anyway

> Just copy the function, no problem Okay. Also, as flags are already marked **Required**, we don't need this extra checks at all - user will receive error from subcommands anyway
chainID, _ := cmd.Flags().GetString(ChainIDFlag)
id = apechain.ID(chainID)
hexEncoded, _ := cmd.Flags().GetBool(ChainIDHexFlag)
if !hexEncoded {
return
}
chainIDRaw, err := hex.DecodeString(chainID)
commonCmd.ExitOnErr(cmd, "can't decode chain ID as hex: %w", err)
id = apechain.ID(chainIDRaw)
return
}
// ParseChain parses an APE chain which can be provided either as a rule statement
// or loaded from a binary/JSON file path.
func ParseChain(cmd *cobra.Command) *apechain.Chain {
chain := new(apechain.Chain)
chain.ID = ParseChainID(cmd)
if rules, _ := cmd.Flags().GetStringArray(RuleFlag); len(rules) > 0 {
commonCmd.ExitOnErr(cmd, "parser error: %w", apeutil.ParseAPEChain(chain, rules))
} else if encPath, _ := cmd.Flags().GetString(PathFlag); encPath != "" {
commonCmd.ExitOnErr(cmd, "decode binary or json error: %w", apeutil.ParseAPEChainBinaryOrJSON(chain, encPath))
} else {
commonCmd.ExitOnErr(cmd, "parser error", errors.New("rule is not passed"))
}
cmd.Println("Parsed chain:")
PrintHumanReadableAPEChain(cmd, chain)
return chain
}
// ParseChainName parses chain name: the place in the request lifecycle where policy is applied.
func ParseChainName(cmd *cobra.Command) apechain.Name {
chainName, _ := cmd.Flags().GetString(ChainNameFlag)
apeChainName, ok := mChainName[strings.ToLower(chainName)]
if !ok {
commonCmd.ExitOnErr(cmd, "", errUnsupportedChainName)
}
return apeChainName
}

View file

@ -0,0 +1,19 @@
package ape
const (
RuleFlag = "rule"
RuleFlagDesc = "Rule statement"
PathFlag = "path"
PathFlagDesc = "Path to encoded chain in JSON or binary format"
TargetNameFlag = "target-name"
TargetNameFlagDesc = "Resource name in APE resource name format"
TargetTypeFlag = "target-type"
TargetTypeFlagDesc = "Resource type(container/namespace)"
ChainIDFlag = "chain-id"
ChainIDFlagDesc = "Chain id"
ChainIDHexFlag = "chain-id-hex"
ChainIDHexFlagDesc = "Flag to parse chain ID as hex"
ChainNameFlag = "chain-name"
ChainNameFlagDesc = "Chain name(ingress|s3)"
AllFlag = "all"
)