TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 100 Pull requests 4 Projects Releases 1 Activity Actions

adm: Make NewLocalActor receive accout name #1463

Merged
dstepanov-yadro merged 1 commit from aarifullin/frostfs-node:fix/adm_local_actor into master 2024-11-08 13:57:52 +00:00
Conversation 7 Commits 1 Files changed 3 +10 -6
aarifullin commented 2024-10-30 14:41:17 +00:00
Member
Copy link
  • Some RPC-clients for contracts require different wallet account types.
    Since, Policy contract gets consensus accounts while NNS gets
    committee accounts.

Close #1393

* Some RPC-clients for contracts require different wallet account types. Since, `Policy` contract gets `consensus` accounts while `NNS` gets `committee` accounts. Close #1393
👍 1
aarifullin added 1 commit 2024-10-30 14:41:18 +00:00
[#XX] adm: Make NewLocalActor receive accout name
Some checks failed
DCO action / DCO (pull_request) Failing after 2m17s
Details
Tests and linters / Run gofumpt (pull_request) Failing after 2m12s
Details
Vulncheck / Vulncheck (pull_request) Successful in 2m17s
Details
Pre-commit hooks / Pre-commit (pull_request) Successful in 2m53s
Details
Build / Build Components (pull_request) Successful in 3m15s
Details
Tests and linters / gopls check (pull_request) Successful in 3m20s
Details
Tests and linters / Staticcheck (pull_request) Successful in 3m46s
Details
Tests and linters / Lint (pull_request) Successful in 4m17s
Details
Tests and linters / Tests (pull_request) Successful in 7m12s
Details
Tests and linters / Tests with -race (pull_request) Successful in 7m13s
Details
78188afee6 
* Some RPC-clients for contracts require different wallet account types.
  Since, `Policy` contract gets `consensus` accounts while `NNS` gets
  `committee` accounts.

Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>
aarifullin force-pushed fix/adm_local_actor from 78188afee6 to 883b6327e3 2024-10-30 14:42:06 +00:00 Compare
fyrchik commented 2024-10-30 15:26:42 +00:00
Owner
Copy link

Are you trying to solve #1393? I think the problem could be with the incorrect signature on the contract side.

Are you trying to solve #1393? I think the problem could be with the incorrect signature on the contract side.
potyarkin commented 2024-10-31 08:59:27 +00:00
Member
Copy link

Are you trying to solve #1393? I think the problem could be with the incorrect signature on the contract side.

In #1393 I've outlined that the fix could be either in frostfs-adm or in the contract. @alexvanin thinks we should fix frostfs-adm and not the contract (discussion in private chat)

> Are you trying to solve #1393? I think the problem could be with the incorrect signature on the contract side. In #1393 I've outlined that the fix could be either in `frostfs-adm` or in the contract. @alexvanin thinks we should fix `frostfs-adm` and not the contract (discussion in [private chat](https://chat.yadro.com/yadro/pl/7b7pmaqae3bqim339bwk4c6a3w))
👍 1
fyrchik requested changes 2024-10-31 11:36:05 +00:00
Dismissed
fyrchik left a comment
Owner
Copy link

By itself this PR just introduces complexity for no reason. Could you solve #1393 so that we can review everything together?

By itself this PR just introduces complexity for no reason. Could you solve #1393 so that we can review everything together?
cmd/frostfs-adm/internal/modules/morph/helper/actor.go Outdated
@ -49,11 +58,13 @@ func NewLocalActor(cmd *cobra.Command, c actor.RPCActor) (*LocalActor, error) {
return nil, err
}
} else {
commonCmd.ExitOnErr(cmd, "%w", checkAccountName(accName))
fyrchik commented 2024-10-31 11:35:06 +00:00
Owner
Copy link

Why do we have this check? GetWalletAccount already returns an error.
Especially given that this accName is given statically.

Why do we have this check? `GetWalletAccount` already returns an error. Especially given that this `accName` is given statically.
aarifullin commented 2024-11-08 11:30:01 +00:00
Author
Member
Copy link

Good point. Fixed

Good point. Fixed
fyrchik marked this conversation as resolved
cmd/frostfs-adm/internal/modules/morph/helper/actor.go Outdated
@ -54,3 +65,3 @@
for _, w := range wallets {
acc, err := GetWalletAccount(w, constants.CommitteeAccountName)
acc, err := GetWalletAccount(w, accName) //constants.CommitteeAccountName)
fyrchik commented 2024-10-31 11:34:25 +00:00
Owner
Copy link

Error message should be changed as well.

Error message should be changed as well.
aarifullin commented 2024-11-08 11:29:54 +00:00
Author
Member
Copy link

Fixed

Fixed
fyrchik marked this conversation as resolved
potyarkin commented 2024-10-31 14:01:15 +00:00
Member
Copy link

🎉 I've just checked and this PR does solve the problem described in #1393:

  • I've reproduced the error on clean cluster with frostfs-adm v0.43.2
  • Everything worked correctly with frostfs-adm from this PR
🎉 I've just checked and this PR does solve the problem described in #1393: - I've reproduced the error on clean cluster with frostfs-adm v0.43.2 - Everything worked correctly with frostfs-adm from this PR
aarifullin commented 2024-10-31 14:24:48 +00:00
Author
Member
Copy link

By itself this PR just introduces complexity for no reason. Could you solve #1393 so that we can review everything together?

Initially, I've created this PR rather to agree with @potyarkin to build frostfs-adm and check if it can change Policy contract state with these signatures. So, that's why I left it with WIP prefix :) So, it seems it works out.

For now, let's discuss the solution

I think the problem could be with the incorrect signature on the contract side.

Policy contract has got this:

func checkAuthorization(ctx storage.Context) {
	admin := getAdmin(ctx)
	if admin != nil && runtime.CheckWitness(admin) {
		return
	}
	if runtime.CheckWitness(common.AlphabetAddress()) {
		return
	}

	panic(ErrNotAuthorized)
}

So, I can ask you (because you're an author of this check) to understand the problem's context:

  1. Did you actually mean common.AlphabetAddress() or policy contract state can be changed with a transaction signed by common.CommitteeAddress()?
  2. What if we allow both common.CommitteeAddress() and common.AlphabetAddress() signers to change the state? Like proxy contract does

If it actually means invocation can be verified only with common.AlphabetAddress() signers, then we need to select consensus accounts from wallets like this PR does

> By itself this PR just introduces complexity for no reason. Could you solve #1393 so that we can review everything together? Initially, I've created this PR rather to agree with @potyarkin to build `frostfs-adm` and check if it can change `Policy` contract state with these signatures. So, that's why I left it with `WIP` prefix :) So, it seems it works out. For now, let's discuss the solution > I think the problem could be with the incorrect signature on the contract side. `Policy` contract has got this: ```go func checkAuthorization(ctx storage.Context) { admin := getAdmin(ctx) if admin != nil && runtime.CheckWitness(admin) { return } if runtime.CheckWitness(common.AlphabetAddress()) { return } panic(ErrNotAuthorized) } ``` So, I can ask you (because you're an author of this check) to understand the problem's context: 1. Did you actually mean `common.AlphabetAddress()` or policy contract state can be changed with a transaction signed by `common.CommitteeAddress()`? 2. What if we allow both `common.CommitteeAddress()` and `common.AlphabetAddress()` signers to change the state? Like `proxy` contract does If it actually means invocation can be verified only with `common.AlphabetAddress()` signers, then we need to select `consensus` accounts from wallets like this PR does
potyarkin commented 2024-10-31 14:33:04 +00:00
Member
Copy link
  1. What if we allow both common.CommitteeAddress() and common.AlphabetAddress() signers to change the state? Like proxy contract does

For what it's worth, this is the workaround I've been using for my cluster. Thought of it as a kludge. I don't quite understand the mechanics of policy contract though; a clarification from @fyrchik would be very welcome.

> 2. What if we allow both `common.CommitteeAddress()` and `common.AlphabetAddress()` signers to change the state? Like `proxy` contract does For what it's worth, this is the workaround [I've been using](https://git.frostfs.info/TrueCloudLab/frostfs-infra/issues/109#issuecomment-52991) for my cluster. Thought of it as a kludge. I don't quite understand the mechanics of policy contract though; a clarification from @fyrchik would be very welcome.
👍 1
fyrchik commented 2024-11-05 08:58:48 +00:00
Owner
Copy link

It's what we use in other contracts for data-path related operations, so you are right, AlphabetAddress() seems right there.

It's what we use in other contracts for data-path related operations, so you are right, `AlphabetAddress()` seems right there.
aarifullin force-pushed fix/adm_local_actor from 883b6327e3 to e631c30309 2024-11-08 11:28:59 +00:00 Compare
aarifullin requested review from storage-core-committers 2024-11-08 11:30:12 +00:00
aarifullin changed title from WIP: adm: Make NewLocalActor receive accout name to adm: Make NewLocalActor receive accout name 2024-11-08 11:30:16 +00:00
aarifullin requested review from storage-core-developers 2024-11-08 11:30:17 +00:00
aarifullin commented 2024-11-08 11:34:43 +00:00
Author
Member
Copy link

Are you trying to solve #1393? I think the problem could be with the incorrect signature on the contract side.

Let summarize our discussion in the PR:

  • If I correctly understand, then the verification by AlphabetAddress() is correct according to your comment above. So, frostfs-adm should be changed by this reason
  • frostfs-adm should be able to choose accounts from wallets by a contract requirements: nns requires to sign with CommitteeAccountName while policy expects for ConsensusAccountName. I left a comment for NewLocalActor to specify how to choose accounts before actor creation
> Are you trying to solve #1393? I think the problem could be with the incorrect signature on the contract side. Let summarize our discussion in the PR: - If I correctly understand, then the verification by `AlphabetAddress()` is correct according to your comment above. So, `frostfs-adm` should be changed by this reason - `frostfs-adm` should be able to choose accounts from wallets by a contract requirements: `nns` requires to sign with `CommitteeAccountName` while `policy` expects for `ConsensusAccountName`. I left a comment for `NewLocalActor` to specify how to choose accounts before actor creation
fyrchik approved these changes 2024-11-08 12:18:27 +00:00
acid-ant approved these changes 2024-11-08 12:38:21 +00:00
dstepanov-yadro approved these changes 2024-11-08 13:57:41 +00:00
dstepanov-yadro merged commit d336f2d487 into master 2024-11-08 13:57:52 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fyrchik
acid-ant
dstepanov-yadro
TrueCloudLab/storage-core-developers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

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
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
5 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#1463
No description provided.
Delete branch "aarifullin/frostfs-node:fix/adm_local_actor"

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?