adm/ape: Adopt policy reader #1607
No reviewers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#1607
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "dkirillov/frostfs-node:feature/adopt_policy_reader"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Embed https://git.frostfs.info/dkirillov/policy-reader
tool to 'frostfs-adm morph ape' command
Signed-off-by: Denis Kirillov d.kirillov@yadro.com
603c6b2fa7
toef84808576
WIP: [#XX] adm/ape: Adopt policy readerto WIP: [#1607] adm/ape: Adopt policy readerWIP: [#1607] adm/ape: Adopt policy readerto [#1607] adm/ape: Adopt policy readeref84808576
to4706a15743
@ -0,0 +18,4 @@
)
var listContainerCmd = &cobra.Command{
Use: "list-container",
What is the difference with
list-rule-chains
command?It seems this runs listing under
chains
subcommandape chains list-container
(although it can be justape chains list
and flags can define targets) instead ofape list-rule-chains
.This command is aimed to list all container related chains. Not only for
container
target but also fornamespace
target (in which container is location)@ -0,0 +52,4 @@
commonCmd.ExitOnErr(cmd, "can't parse chain-name: %w", err)
endpoint := viper.GetString(commonflags.EndpointFlag)
namespace := viper.GetString(namespaceFlag)
Other ape-related commands have
root
to empty string replacement:if name == "root" {
@ -0,0 +13,4 @@
maxPrintable = 127
)
func PrintChains(cmd *cobra.Command, list []stackitem.Item, decodeChain, decodeID bool) error {
In which case is it expected that
frostfs-adm
user will not want to decode ID and chain?When data in policy contract isn't actually chain. For
i
kind we store aws policy json (that isn't compatible with ape chain format)@ -0,0 +7,4 @@
)
var Cmd = &cobra.Command{
Use: "low-level",
What about
raw
?I thought about it but for some reason I decided that you wouldn't like it
@dkirillov, look, I believe I can figure out your intentions, but you should take in account that we can't leave such aliases for subcommands.
Currently
morph ape
also defines such commands to manage/read chainsThis can't be accepted without refactoring. Is it possible to implement
policy-reader
features adding functionality to the currentfrostfs-adm morph ape
subcommands?@ -0,0 +20,4 @@
}
const (
policyHashFlag = "policy-hash"
We don't need to pass it. See
These commands aimed to debug
policy
contract in any environment. In some environment this contract isn't registered in NNSOkay. You haven't marked the flag as required but it's not processed when it's empty either.
Could we use searching within NNS as default behavior if its value is empty?
@ -0,0 +31,4 @@
func initListChainNamesCmd() {
listChainNamesCmd.Flags().StringP(commonflags.EndpointFlag, commonflags.EndpointFlagShort, "", commonflags.EndpointFlagDesc)
listChainNamesCmd.Flags().String(kindFlag, "n", "Target kind (1-byte) to list (n(namespace)/c(container)/g(group)/u(user)/i(iam))")
Consider reusing flag names from common package, please!
Actually, I tried. But it leads to huge unwanted refactor in APE because target-type be parsed to this. This
TargetType
is widely used inRequestTarget
and function like this.But for
i
kind (IAM = 'i'
) we store aws json policy and this data MUST NOT be used in
IsAllowed
APE function because it isn't valid APE chain.So I don't this that it's good idea change APE and provide user way for incorrect using APE library with
i
kind (target-type)Okay. Let me explain my point of view and, probably, we'll find a compromise.
These types are not bound to
frostfs-node
. It implied you can manage chains on the different protocol levels (ingress
,s3
etc.), but it seemss3-gw
doesn't use the interface yet (or it' not adapted ?)s3-gw
manages IAM chains onS3
levelIsAllowed
will never lists3
chains, onlyingress
ape-manager
are created oningress
level.i
won't affect its workSo, ideally,
policy-engine
could introducei
type that could be listed on S3 level using policy contract interface. So, it doesn't look like a huge refactoringAnyway, I just asked to use flag names and description. Parsing to target hasn't been on point :))
#1607 (comment)
To the current subcommands? I believe it isn't possible (without a huge kludge).
4706a15743
to98eeed2e74
@ -0,0 +12,4 @@
apeCmd "git.frostfs.info/TrueCloudLab/frostfs-node/cmd/internal/common/ape"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container"
cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/ns"
I would like to avoid using this package in this repo.
Ideally all node <-> neo-go communication is done via autogenerated code or helpers from the
frostfs-contract
.We already have multiple commands using different mechanisms (actors vs common client from the frostfsid), let's not add yet another one.
Specifically, I would like to make this a thing #1035, so connection handling should be decoupled from the argument parsing.
@ -0,0 +19,4 @@
var listContainerCmd = &cobra.Command{
Use: "list-container",
Short: "List container related (namespace) policies",
I would use different naming:
namespace
somewhere in the command name and--container
as an optional argument (aka filter).Current implementation looks too specific.
Indeed it is. This command is aimed to get all container related policies. In
container
target and innamespace
target.The similar command for user that get all user related (affected) policies (from
namespace
,group
,user
targets)I'm not sure if using "
--container
as optional argument (aka filter)" is appropriate here@ -0,0 +24,4 @@
Example: `chains list-container -r http://localhost:40332 list --container 7h7NcXcF6k6b1yidqEHc1jkyXUm1MfUDrrTuHAefhiDe
chains list-container -r http://localhost:40332 --policy-hash 81c1a41d09e08087a4b679418b12be5d3ab15742 list --container 7h7NcXcF6k6b1yidqEHc1jkyXUm1MfUDrrTuHAefhiDe --namespace test`,
PreRun: func(cmd *cobra.Command, _ []string) {
_ = viper.BindPFlag(containerFlag, cmd.Flags().Lookup(containerFlag))
We use
BindPFlag
to allow providing command values via configuration.I doubt it is useful for any of these 4 flags.
(
containerFlag
is required, to this bind seems definitely useless).@ -0,0 +72,4 @@
res, err = commonclient.ReadIteratorItems(inv, 100, policyHash, methodIteratorChainsByPrefix, big.NewInt(int64(policycontract.Container)), cnrID.String(), string(chainName))
commonCmd.ExitOnErr(cmd, "can't read iterator: %w", err)
cmd.Printf("container policies: %d\n", len(res))
Why do we have
%s
for namespace message, but no CID for this one?@ -0,0 +77,4 @@
}
// ResolveContainerID determine container id by resolving NNS name.
func ResolveContainerID(endpoint, namespace, containerName string) (cid.ID, error) {
It is used only once and in this package, why is it public?
Why have you decided to move it to
helper
package instead of making it private?To make it similar to
NNSResolveHash
. Probably it can be helpful in some other placesProbably can, probably not.
To me it complicates things now.
@ -0,0 +128,4 @@
return ffsid.GetSubjectExtended(subj.PrimaryKey.GetScriptHash())
}
func parseChainName(service string) (apechain.Name, error) {
To my complete surprise, there is an old and undone issue: https://github.com/spf13/pflag/issues/236 which could be helpful here.
@ -0,0 +141,4 @@
return "", errUnknownServiceType
}
func printSubject(cmd *cobra.Command, subj *ffsidclient.SubjectExtended) {
It accepts
SubjectExtended
but doesn't printAdditionalKeys
. It this intended?Probably this was done (in
policy-reader
tool) before additional keys starts to be useful to see@ -0,0 +25,4 @@
)
func init() {
Cmd.PersistentFlags().String(policyHashFlag, "policy.frostfs", "NNS name or script hash of policy contract")
Every other command may resolve contract hash from NNS.
frostfs-adm
may resolve any domain too.Why this argument exists?
#1607 (comment)
@ -0,0 +70,4 @@
return nil, fmt.Errorf("invalid type: %s", typ)
}
return big.NewInt(int64(typ[0])), nil
This is very error-prone: we connect some constants from contracts with CLI interface in the frostfs-adm.
If something changes, we won't notice.
Can we make this an implicit switch that uses some exported constants from the
policy
contract?And, to be fair, full multi-byte names or even boolean flags look better IMO.
I consider this command as low-level one, so I would like to be able to invoke ListTarget using any valid one-byte parameter (the same way as I can do this by
neo-go contract testinvokefuntion
)But can we provide e.g.
\x00
?Update parsing to be able provide just number
@ -8,3 +8,3 @@
TargetNameFlagDesc = "Resource name in APE resource name format"
TargetTypeFlag = "target-type"
TargetTypeFlagDesc = "Resource type(container/namespace)"
TargetTypeFlagDesc = "Resource type(container/namespace/group/user)"
This should be in a separate commit.
98eeed2e74
to4022349fbe
@ -0,0 +9,4 @@
var Cmd = &cobra.Command{
Use: "raw",
Short: "FrostFS policy contract raw reader",
Long: "Helps reading policy information from contact in FrostFS network",
raw
is subcommand andLong
won't be showed in the prompt. You can leaveShort
only@ -0,0 +81,4 @@
commonCmd.ExitOnErr(cmd, "can't get NNS contract state: %w", err)
policyHashStr, _ := cmd.Flags().GetString(policyHashFlag)
policyHash, err := helper.NNSResolveHash(inv, nnsCs.Hash, policyHashStr)
Please, let this flag be optional (see this)
This is already optional, If I understood you correctly. I have default
policy.frostfs
value for this flagCmd.PersistentFlags().String(policyHashFlag, "policy.frostfs", "NNS name or script hash of policy contract")
Ah, yeah. Correct!
4022349fbe
toa1154801d1
[#1607] adm/ape: Adopt policy readerto adm/ape: Adopt policy reader@ -0,0 +158,4 @@
for i, key := range subj.AdditionalKeys {
additionalKeys[i] = hex.EncodeToString(key.Bytes())
}
cmd.Printf("\tadditional keys: %v\n", additionalKeys)
We print all other arrays line-by-line, and here we use
%v
, why so?Because we don't need extra formatting in this case
I mean, why we don't need it?
Having keys line-by-line is easier to read and will correspond to the formatting of other array values.
a1154801d1
toa6f300ea17
a6f300ea17
toee4c32ad6e
We definitely should have this thing in our cli
@ -0,0 +87,4 @@
nnsCs, err := helper.GetContractByID(management.NewReader(inv), 1)
commonCmd.ExitOnErr(cmd, "can't get NNS contract state: %w", err)
policyHashStr, _ := cmd.Flags().GetString(policyHashFlag)
Could you explain, why this code works?
We get policyhash, which is hex-encoded contract hash we would like to use.
But instead of decoding it we use
NNSResolveHash
wherepolicyHashStr
is provided as a domain and resolved.Secondly, regardless of how it works, if the user provides explicit policy hash, we should not care about NNS at all.
What am I missing here?
Oh, It seems I've lost decoding during refactor. I'll fix it
err != nil
for some reason. Why? We expect the hash, if it is invalid command should fail.s, _ := cmd.Flags().GetString(addrAdminFlag)
We skip an error there because it cannot possibly happen (the flag is defined and all flags can be used as string).
The error you skip (in the
if
below this line) happens during parsing.If parsing failed then we should try to resolve it in NNS, because it can have format like
policy.frostfs
Then it should be a
--policy-domain
and not--policy-hash
.However, I think
policy-hash
is enough, if we want to use a custom domain, we might as well resolve it separately.ee4c32ad6e
to05517ae305
New commits pushed, approval review dismissed automatically according to repository settings
05517ae305
to10574f8ff0
@ -0,0 +187,4 @@
}
frostfsidHashStr, _ := cmd.Flags().GetString(frostfsidHashFlag)
frostfsidHash, err := util.Uint160DecodeStringLE(policyHashStr)
Is it really
policyHashStr
?View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.