Enhance getRole output when the role is 'root'. #12979

horizonzy · 2021-05-16T13:23:49Z

codecov-commenter · 2021-05-16T13:41:46Z

Codecov Report

Merging #12979 (e723d83) into main (9501e8e) will increase coverage by 2.35%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main   #12979      +/-   ##
==========================================
+ Coverage   46.10%   48.46%   +2.35%     
==========================================
  Files         409      409              
  Lines       33636    33700      +64     
==========================================
+ Hits        15509    16333     +824     
+ Misses      16119    15309     -810     
- Partials     2008     2058      +50

Flag	Coverage Δ
all	`48.46% <0.00%> (+2.35%)`	⬆️

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

Impacted Files	Coverage Δ
server/auth/store.go	`72.27% <0.00%> (-0.24%)`	⬇️
pkg/ioutil/reader.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/report/report.go	`0.00% <0.00%> (-95.58%)`	⬇️
pkg/report/timeseries.go	`0.00% <0.00%> (-86.77%)`	⬇️
pkg/expect/expect.go	`0.00% <0.00%> (-80.56%)`	⬇️
pkg/stringutil/rand.go	`0.00% <0.00%> (-78.58%)`	⬇️
pkg/flags/unique_strings.go	`0.00% <0.00%> (-75.00%)`	⬇️
pkg/wait/wait_time.go	`29.41% <0.00%> (-70.59%)`	⬇️
pkg/report/weighted.go	`0.00% <0.00%> (-69.77%)`	⬇️
pkg/flags/unique_urls.go	`0.00% <0.00%> (-69.24%)`	⬇️
... and 96 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 9501e8e...e723d83. Read the comment docs.

tangcong

when the role is root, the output of auth v2 seems to be no problem. can you modify auth v3 on the server by referring to the way of auth v2? thanks.
https://github.com/etcd-io/etcd/blob/main/server/etcdserver/api/v2auth/auth.go#L49-L67

etcdctl/ctlv2/command/role_commands.go

horizonzy · 2021-05-16T14:56:27Z

when the role is root, the output of auth v2 seems to be no problem. can you modify auth v3 on the server by referring to the way of auth v2? thanks.
https://github.com/etcd-io/etcd/blob/main/server/etcdserver/api/v2auth/auth.go#L49-L67

Hi, The code already push, I'm not good at it, so I didn't konw the logic is right or not, need your guidance. Thanks.

etcdctl/ctlv3/command/printer_simple.go

server/etcdserver/v3_server.go

server/go.sum

tangcong

thanks. LGTM.

xiang90 · 2021-05-16T17:24:41Z

@horizonzy

Do you have an example output before/after this change? Better if you can write a test for it.

horizonzy · 2021-05-17T02:12:03Z

The response:

$ etcdctl role get root
Role root
KV Read:
	[*, <open ended>
KV Write:
	[*, <open ended>

horizonzy · 2021-05-17T02:12:10Z

@horizonzy

Do you have an example output before/after this change? Better if you can write a test for it.

done.

server/auth/store.go

xiang90 · 2021-05-17T06:15:04Z

LGTM. If tests all pass, please get all commits squashed into one commit. Then it should be good to merge.

Thank you!

horizonzy · 2021-05-17T06:54:58Z

LGTM. If tests all pass, please get all commits squashed into one commit. Then it should be good to merge.

Thank you!

done.

server/auth/store_test.go

ptabor

Please add information to CHANGELOG-3.5
Please cleanup the commit message to describe what was the behavior before and after the change

horizonzy · 2021-05-17T14:46:37Z

Please add information to CHANGELOG-3.5

Please cleanup the commit message to describe what was the behavior before and after the change

done.

hexfusion

overall LGTM defer to @ptabor or @mitake .

ptabor · 2021-05-17T16:06:55Z

I have no experience with permission model and the auth.proto is poorly documented, so forgive my silly questions:

message Permission {
  enum Type {
    READ = 0;
    WRITE = 1;
    READWRITE = 2;
  }
  Type permType = 1;

  bytes key = 2;
  bytes range_end = 3;
}

Does it mean that the permission is granted for: [key..range_end)

where 'range_end' can have special value of:

etcd/client/v3/op.go

Line 29 in 932d42b

var noPrefixEnd = []byte{0}

Why do we need to introduce an additional representation of full range ('*') instead of: '' .. '\0' ?

horizonzy · 2021-05-17T16:16:30Z

I have no experience with permission model and the auth.proto is purely documented, so forgive my silly questions:
message Permission {
  enum Type {
    READ = 0;
    WRITE = 1;
    READWRITE = 2;
  }
  Type permType = 1;

  bytes key = 2;
  bytes range_end = 3;
}
Does it mean that the permission is granted for: [key..range_end)

where 'range_end' can have special value of:

etcd/client/v3/op.go

Line 29 in 932d42b

var noPrefixEnd = []byte{0}

Why do we need to introduce an additional representation of full range ('*') instead of: '' .. '\0' ?

Hi, In my view, * means all data, just like mysql select * from xxx.

xiang90 · 2021-05-17T16:56:36Z

@ptabor

One thing we can do is to reuse the range_end thing to donate range_start. Then all keys will look like <open_end>, <open_end>.

Alternatively, we can just make * to donate all (get rid of the <open_end> in the all key case). So it will look just like *.

ptabor · 2021-05-17T18:38:17Z

I

@ptabor

One thing we can do is to reuse the range_end thing to donate range_start. Then all keys will look like <open_end>, <open_end>.

Alternatively, we can just make * to donate all (get rid of the <open_end> in the all key case). So it will look just like *.

I don't understand. Could you, please, define current/desired semantic of fields in the proto:

etcd/api/authpb/auth.proto

Lines 25 to 40 in e73facb

    
           message Permission { 
        
             enum Type { 
        
               READ = 0; 
        
               WRITE = 1; 
        
               READWRITE = 2; 
        
             } 
        
             Type permType = 1; 
        
             bytes key = 2; 
        
             bytes range_end = 3; 
        
           } 
        
           // Role is a single entry in the bucket authRoles 
        
           message Role { 
        
             bytes name = 1;

would key be only used for prefix scan ?

xiang90 · 2021-05-17T19:39:27Z

@ptabor

I think this change has nothing to do with the actual server side proto. It is just a aesthetics improvement for better command line tool display.

Without the change, the perm list for root shows nothing since it is a special case. After this change, it will show something (either *, open_end for all keys, or open_end, open_end for all keys or just * for all keys or anything we prefer...).

xiang90 · 2021-05-17T19:46:54Z

We can probably use noPrefixEnd. Then at etcdctl side, we can translate it to something more visually meaningful. Then it can be more consistent with the server side processing logic as well.

ptabor · 2021-05-18T10:00:35Z

@ptabor

I think this change has nothing to do with the actual server side proto. It is just a aesthetics improvement for better command line tool display.

For me its a change of server-side implementation and not of etcdctl printer.

mitake · 2021-05-18T13:42:55Z

@ptabor Although we can grant permissions to root role, the permissions are ignored because the role is special and it allows users to access every key. The purpose of this PR is ignoring these unused permissions and just print wildcard.
It would be valuable to update the server side and make both server and client consistent as @xiang90 suggests.

ptabor · 2021-05-18T13:55:56Z

@mitake How do we express in the data-model/proto that not-root user has access to any key ?
Why do we need different language in the wire-format to express these to states for root and not-root

I would not have any concerns if this was a change in etcdctl printer's, but it actually changes the server's wire-format.

mitake · 2021-05-18T13:59:51Z

@ptabor oops sorry I missed the latest version of this PR... yeah it changes the server side, let me take a look again. The initial version of this PR just changed etcdctl.

mitake · 2021-05-18T14:19:04Z

server/auth/store.go

@@ -624,6 +627,10 @@ func (as *authStore) UserRevokeRole(r *pb.AuthUserRevokeRoleRequest) (*pb.AuthUs
 }

 func (as *authStore) RoleGet(r *pb.AuthRoleGetRequest) (*pb.AuthRoleGetResponse, error) {
+	if rootRole == r.Role {


I feel this approach is a little bit risky because the result returned by RoleGet() might be used by other components of etcd server (it should be avoided of course). Also it changes the meaning of the fields. Although v2 employs the similar approach, I prefer the initial approach which only changes etcdctl. Note that v2's auth is based on path and v3's auth is based on prefix, so they are quite different.
How do you think @tangcong ?

Actually * is a valid etcd key name.

sounds reasonable. we need to be consistent with the existing rules, so we cannot use * on the server-side. How about we use noPrefixEnd? If this PR only changes etcdctl, it looks a little tricky, and there are other languages/sdk that will be called through API.

horizonzy · 2021-05-18T15:11:02Z

At now, we can grant permission to root role, when we get root role, it will output what we grant. In fact, whatever we grant permission to root role, it still can operate all datas.

xiang90 · 2021-05-18T15:37:17Z

@ptabor

For me its a change of server-side implementation and not of etcdctl printer.

You are right. I did not give the latest version a closer enough look.

@horizonzy

Hi, basically there two separate things:

change the output of etcdctl of root role. For this part, I think either * or open_end makes sense. Human will understand it. But I prefer open_end more since * might confuse users that the role only has permission on a single * key.
change the return value of getRole of the root role. For this part, we need to be consistent with the existing rules and use noPrefixEnd (I assume it is noPrefixEnd. It should be something we use to represent starting from the very first key ). We cannot use * as the server side return since it means the permission is only on the single key * or the range starting with key *.

horizonzy · 2021-05-18T16:06:59Z

change the output of etcdctl of root role. For this part, I think either * or open_end makes sense. Human will understand it. But I prefer open_end more since * might confuse users that the role only has permission on a single * key.

Yep, it indeed confuse users. We just modify output of etcdctl get role root.
The output:

$ etcdctl role get root
Role root
KV Read:
	<open ended>
KV Write:
	<open ended>

How about it?

change the return value of getRole of the root role. For this part, we need to be consistent with the existing rules and use noPrefixEnd (I assume it is noPrefixEnd. It should be something we use to represent starting from the very first key ). We cannot use * as the server side return since it means the permission is only on the single key * or the range starting with key *.

At server, we can grant permission to root role some permission like other role. In server, the permission data also will be save, but it not works. I think it should be solved. And now the root role need be add by hand, now that the root role permission can't be change, maybe we can add it defaultly.

xiang90 · 2021-05-18T16:24:11Z

@horizonzy Maybe we should just focus on 1) in this PR. Just fixing the output of etcdctl. We can get the 2) part (server) done later in another PR>

horizonzy · 2021-05-18T16:26:12Z

@horizonzy Maybe we should just focus on 1) in this PR. Just fixing the output of etcdctl. We can get the 2) part (server) done later in another PR>

fine, just modify the output at etcdctl.

CHANGELOG-3.5.md

etcdctl/ctlv3/command/printer_simple.go

ptabor

test, pls.

horizonzy · 2021-05-19T07:03:22Z

Output:

$ etcdctl role get root      
{"level":"warn","ts":"2021-05-19T14:54:45.762+0800","caller":"clientv3/retry_interceptor.go:62","msg":"retrying of unary invoker failed","target":"endpoint://client-04ea1642-b55b-4757-a48b-b0a5edf3b7fa/127.0.0.1:2379","attempt":0,"error":"rpc error: code = FailedPrecondition desc = etcdserver: role name not found"}
Error: etcdserver: role name not found

$ ./bin/etcdctl role get root
{"level":"warn","ts":"2021-05-19T14:54:52.254+0800","logger":"etcd-client","caller":"v3/retry_interceptor.go:62","msg":"retrying of unary invoker failed","target":"etcd-endpoints://0xc0000b6700/#initially=[127.0.0.1:2379]","attempt":0,"error":"rpc error: code = FailedPrecondition desc = etcdserver: role name not found"}
Error: etcdserver: role name not found

$ etcdctl role add root
Role root created

$ etcdctl role get root
Role root
KV Read:
KV Write:

$ ./bin/etcdctl role get root
Role root
KV Read:
        [, <open ended>
KV Write:
        [, <open ended>

$ etcdctl role grant-permission root readwrite foo
Role root updated

$ etcdctl role get root                           
Role root
KV Read:
        foo
KV Write:
        foo

$ ./bin/etcdctl role get root
Role root
KV Read:
        foo
KV Write:
        foo

ptabor · 2021-05-19T07:23:36Z

Thank you. It looks good to me.

Please add a test to https://github.com/etcd-io/etcd/blob/main/tests/e2e/ctl_v3_role_test.go

As follow up we should make the server-side return "".."\x00" always when asked about the root (in a separate PR).
This will solve the problem of the KV Read: foo outputs

ptabor

Thank you.

tests/e2e/ctl_v3_role_test.go

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

mitake · 2021-05-19T14:39:58Z

LGTM, merged, thanks!

tangcong suggested changes May 16, 2021

View reviewed changes

mitake reviewed May 16, 2021

View reviewed changes

etcdctl/ctlv2/command/role_commands.go Outdated Show resolved Hide resolved

tangcong reviewed May 16, 2021

View reviewed changes

etcdctl/ctlv3/command/printer_simple.go Outdated Show resolved Hide resolved

tangcong reviewed May 16, 2021

View reviewed changes

server/etcdserver/v3_server.go Outdated Show resolved Hide resolved

tangcong reviewed May 16, 2021

View reviewed changes

server/go.sum Outdated Show resolved Hide resolved

horizonzy force-pushed the enhance-role-output branch from ee298fc to 77a73d4 Compare May 16, 2021 15:27

tangcong approved these changes May 16, 2021

View reviewed changes

xiang90 reviewed May 17, 2021

View reviewed changes

server/auth/store.go Outdated Show resolved Hide resolved

horizonzy force-pushed the enhance-role-output branch 2 times, most recently from 1ce84ec to aa4341f Compare May 17, 2021 06:52

ptabor reviewed May 17, 2021

View reviewed changes

server/auth/store_test.go Outdated Show resolved Hide resolved

ptabor suggested changes May 17, 2021

View reviewed changes

horizonzy force-pushed the enhance-role-output branch from aa4341f to 69a51ab Compare May 17, 2021 14:45

lilic mentioned this pull request May 17, 2021

3.5 release next steps: Code freeze Monday 5/17 #12970

Closed

4 tasks

hexfusion approved these changes May 17, 2021

View reviewed changes

mitake reviewed May 18, 2021

View reviewed changes

horizonzy force-pushed the enhance-role-output branch from 69a51ab to 0a10619 Compare May 18, 2021 16:39

ptabor reviewed May 18, 2021

View reviewed changes

CHANGELOG-3.5.md Outdated Show resolved Hide resolved

ptabor reviewed May 18, 2021

View reviewed changes

etcdctl/ctlv3/command/printer_simple.go Outdated Show resolved Hide resolved

ptabor suggested changes May 18, 2021

View reviewed changes

horizonzy force-pushed the enhance-role-output branch 2 times, most recently from 6d23bf1 to 77e837d Compare May 18, 2021 18:45

This comment has been minimized.

Sign in to view

horizonzy force-pushed the enhance-role-output branch from 77e837d to 208a6f9 Compare May 19, 2021 07:04

horizonzy force-pushed the enhance-role-output branch from 208a6f9 to 123a4cf Compare May 19, 2021 08:02

ptabor approved these changes May 19, 2021

View reviewed changes

tests/e2e/ctl_v3_role_test.go Show resolved Hide resolved

horizonzy force-pushed the enhance-role-output branch from 123a4cf to b45167a Compare May 19, 2021 08:21

enhance GetRole output when role is 'root' and the response perm is nil.

6ab56fc

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

horizonzy force-pushed the enhance-role-output branch from b45167a to 6ab56fc Compare May 19, 2021 09:36

mitake merged commit 5f60e0d into etcd-io:main May 19, 2021

horizonzy mentioned this pull request May 19, 2021

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

Merged

Enhance getRole output when the role is 'root'. #12979

Enhance getRole output when the role is 'root'. #12979

Conversation

horizonzy commented May 16, 2021

codecov-commenter commented May 16, 2021 • edited Loading

Codecov Report

tangcong left a comment

Choose a reason for hiding this comment

horizonzy commented May 16, 2021

tangcong left a comment

Choose a reason for hiding this comment

xiang90 commented May 16, 2021

horizonzy commented May 17, 2021

horizonzy commented May 17, 2021

xiang90 commented May 17, 2021

horizonzy commented May 17, 2021

ptabor left a comment

Choose a reason for hiding this comment

horizonzy commented May 17, 2021 • edited Loading

hexfusion left a comment

Choose a reason for hiding this comment

ptabor commented May 17, 2021 • edited Loading

horizonzy commented May 17, 2021

xiang90 commented May 17, 2021

ptabor commented May 17, 2021

xiang90 commented May 17, 2021

xiang90 commented May 17, 2021

ptabor commented May 18, 2021

mitake commented May 18, 2021 • edited Loading

ptabor commented May 18, 2021

mitake commented May 18, 2021 • edited Loading

mitake May 18, 2021

Choose a reason for hiding this comment

mitake May 18, 2021

Choose a reason for hiding this comment

tangcong May 18, 2021

Choose a reason for hiding this comment

horizonzy commented May 18, 2021

xiang90 commented May 18, 2021 • edited Loading

horizonzy commented May 18, 2021

xiang90 commented May 18, 2021

horizonzy commented May 18, 2021

ptabor left a comment

Choose a reason for hiding this comment

This comment has been minimized.

horizonzy commented May 19, 2021

ptabor commented May 19, 2021

ptabor left a comment

Choose a reason for hiding this comment

mitake commented May 19, 2021

codecov-commenter commented May 16, 2021 •

edited

Loading

horizonzy commented May 17, 2021 •

edited

Loading

ptabor commented May 17, 2021 •

edited

Loading

mitake commented May 18, 2021 •

edited

Loading

mitake commented May 18, 2021 •

edited

Loading

xiang90 commented May 18, 2021 •

edited

Loading