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.

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

Enhance the root permission, when root role exist, it always return rootPerm. #13006

Merged
merged 1 commit into from
May 24, 2021

Conversation

horizonzy
Copy link
Contributor

After #12979.
We need make the server-side return "".."\x00" always when asked about the root.

$ etcdctl role grant-permission root readwrite foo.
Role root updated
$ etcdctl role get root
Role root
KV Read:
        foo
KV Write:
        foo

The root role should be

Role root
KV Read:
        [, <open ended>
KV Write:
        [, <open ended>

@@ -200,7 +200,7 @@ func (s *simplePrinter) RoleGet(role string, r v3.AuthRoleGetResponse) {
} else {
fmt.Printf("\t[%s, <open ended>", sKey)
}
if v3.GetPrefixRangeEnd(sKey) == sRangeEnd {
if v3.GetPrefixRangeEnd(sKey) == sRangeEnd && len(sKey) > 0 {
Copy link
Contributor Author

@horizonzy horizonzy May 19, 2021

Choose a reason for hiding this comment

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

When sKey is empty, GetPrefixRangeEnd will return []byte{0}, it will be same with rootPerm rangeEnd []byte{0}.
And when sKey is empty, it shouldn't show prefix.
So modify it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity: What's wrong with showing 'empty' prefix. Prefixes are IMHO easier to understand than ranges.

Copy link
Contributor Author

@horizonzy horizonzy May 20, 2021

Choose a reason for hiding this comment

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

it will show:


Role root
KV Read:
        [, <open ended> (prefix )
KV Write:
        [, <open ended> (prefix )

It's not the expected when role is root

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, the empty prefix is no sense.

@codecov-commenter
Copy link

Codecov Report

Merging #13006 (5b2c038) into main (5f60e0d) will decrease coverage by 11.06%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main   #13006       +/-   ##
===========================================
- Coverage   63.41%   52.34%   -11.07%     
===========================================
  Files         439      426       -13     
  Lines       34119    33688      -431     
===========================================
- Hits        21637    17635     -4002     
- Misses      10435    14219     +3784     
+ Partials     2047     1834      -213     
Flag Coverage Δ
all 52.34% <83.33%> (-11.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
etcdctl/ctlv3/command/printer_simple.go 72.64% <0.00%> (+2.83%) ⬆️
server/auth/store.go 80.64% <100.00%> (+7.16%) ⬆️
client/v3/txn.go 0.00% <0.00%> (-100.00%) ⬇️
raft/quorum/quorum.go 0.00% <0.00%> (-100.00%) ⬇️
raft/tracker/state.go 0.00% <0.00%> (-100.00%) ⬇️
client/v3/compact_op.go 0.00% <0.00%> (-100.00%) ⬇️
client/v3/ordering/util.go 0.00% <0.00%> (-100.00%) ⬇️
client/v3/namespace/util.go 0.00% <0.00%> (-100.00%) ⬇️
client/pkg/v3/tlsutil/cipher_suites.go 0.00% <0.00%> (-100.00%) ⬇️
client/pkg/v3/transport/sockopt_unix.go 0.00% <0.00%> (-100.00%) ⬇️
... and 273 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f60e0d...5b2c038. Read the comment docs.

server/auth/store.go Outdated Show resolved Hide resolved
server/auth/store_test.go Outdated Show resolved Hide resolved
@horizonzy horizonzy force-pushed the fix-auth-server branch 2 times, most recently from 992805f to a162a62 Compare May 20, 2021 14:37
…ootPerm.

etcdctl role grant-permission root readwrite foo.
see etcdctl role get root output.
Before:
Role root
KV Read:
        foo
KV Write:
        foo
After:
Role root
KV Read:
        [, <open ended>
KV Write:
        [, <open ended>
@xiang90 xiang90 merged commit 64b01a7 into etcd-io:main May 24, 2021
@xiang90
Copy link
Contributor

xiang90 commented May 24, 2021

@horizonzy

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants