Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deprecate System role in EACL #616 #627

Merged
merged 6 commits into from
Sep 15, 2023
Merged

Deprecate System role in EACL #616 #627

merged 6 commits into from
Sep 15, 2023

Conversation

vvarg229
Copy link
Collaborator

@vvarg229 vvarg229 commented Sep 5, 2023 •
edited
Loading

Removed the System role in EACL and removed the tests related to this role. The System EACL role leads to a broken container and nothing else and will be removed in the issue nspcc-dev/neofs-node#2531

Tests result:
https://http.t5.fs.neo.org/86C4P6uJC7gb5n3KkwEGpXRfdczubXyRNW5N9KeJRW73/332-1693924040/index.html#

@cthulhu-rider
Copy link
Contributor

cthulhu-rider commented Sep 5, 2023 •
edited
Loading

i'm a bit out-of-context, but if

  • we just deprecate the role, we should continue to test it
  • we forbid it, then we should test that it's denied by the system (try and expect failure)

carpawell
carpawell reviewed Sep 5, 2023
View reviewed changes
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we add some tests about future system changes? I mean refusing system changes will be added in the neofs-cli and IR. Also, some --force may be added. So some issues about that?

@vvarg229
Copy link
Collaborator Author

vvarg229 commented Sep 6, 2023

  • we just deprecate the role, we should continue to test it
    These tests are the cause of cluster breakage, which affects other tests.
  • we forbid it, then we should test that it's denied by the system (try and expect failure)
    I agree, it's a good idea to add tests like this.
    @roman-khimov what do you think?

@vvarg229
Copy link
Collaborator Author

vvarg229 commented Sep 6, 2023

Will we add some tests about future system changes?
I think we should add tests for not being able to change System role using EACL. But it seems to me that this should be done after the issue nspcc-dev/neofs-node#2531 is resolved.

I mean refusing system changes will be added in the neofs-cli and IR. Also, some --force may be added. So some issues about that?
Let's discuss this in the nspcc-dev/neofs-node#2531 issue?

roman-khimov
roman-khimov requested changes Sep 6, 2023
View reviewed changes
pytest_tests/testsuites/acl/test_eacl.py Show resolved Hide resolved
pytest_tests/testsuites/acl/test_eacl.py Show resolved Hide resolved
pytest_tests/testsuites/acl/test_eacl.py Show resolved Hide resolved
pytest_tests/testsuites/acl/test_eacl.py Show resolved Hide resolved
pytest_tests/testsuites/acl/test_eacl.py Show resolved Hide resolved
pytest_tests/testsuites/acl/test_eacl.py Show resolved Hide resolved
pytest_tests/testsuites/acl/test_eacl.py Show resolved Hide resolved
pytest_tests/testsuites/acl/test_eacl.py Show resolved Hide resolved
pytest_tests/testsuites/acl/test_eacl.py Show resolved Hide resolved
@vvarg229 vvarg229 marked this pull request as draft September 7, 2023 17:07
@roman-khimov
Copy link
Member

When will this be updated?
https://http.fs.neo.org/HXSaMJXk2g8C14ht8HSi7BBaiYZ1HeWh2xnWPGQCg4H6/722-1694457866/index.html#suites/69b447004f3c682f67955ec900a09cf0/d419bcbd5433e713/

@vvarg229
helpers: Separate eACL grps responses
b3dd3e5 
Signed-off-by: Oleg Kulachenko <[email protected]>
@vvarg229
helpers: Add EACL_PROHIBITED_TO_MODIFY_SYSTEM_ACCESS grpc respons
3fce3e8 
Signed-off-by: Oleg Kulachenko <[email protected]>
@vvarg229
lib: Change hardcode string in the get_eacl function
15be7ce 
Signed-off-by: Oleg Kulachenko <[email protected]>
@vvarg229
acl: Add get ir and storage wallet
616baf0 
Added 2 new functions: get_ir_wallet and get_storage_wallet
This will allow to get rid of such terrible things in the test code as:
wallets.get_wallets_list()[:2]

Signed-off-by: Oleg Kulachenko <[email protected]>
@vvarg229
acl: Deprecate System role in eACL nspcc-dev#616
d0fee6b 
Removed changing the System role in EACL and changed the tests related
to this role.
The changing system eACL role leads to a broken container and
nothing else and will be removed in the issue
nspcc-dev/neofs-node#2531

Signed-off-by: Oleg Kulachenko <[email protected]>
@vvarg229 vvarg229 marked this pull request as ready for review September 14, 2023 18:13
roman-khimov
roman-khimov approved these changes Sep 14, 2023
View reviewed changes
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit too many commits than needed (at least 47be96f and 15be7ce can be squashed), but OK.

@roman-khimov roman-khimov merged commit c664e58 into nspcc-dev:master Sep 15, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov approved these changes

@MaxGelbakhiani MaxGelbakhiani Awaiting requested review from MaxGelbakhiani

@cthulhu-rider cthulhu-rider Awaiting requested review from cthulhu-rider cthulhu-rider is a code owner

@evgeniiz321 evgeniiz321 Awaiting requested review from evgeniiz321 evgeniiz321 is a code owner

@carpawell carpawell Awaiting requested review from carpawell carpawell is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vvarg229 @cthulhu-rider @roman-khimov @carpawell