etcdctl: add a new option --open-ended for unlimited range permission #7649

mitake · 2017-04-03T09:08:40Z

This commit adds a new option --open-ended to the command etcdctl role
grant-permission. If the option is passed, an open ended permission
will be granted to a role e.g. from start-key to any keys those are
larger than start-key.

Example:
$ ETCDCTL_API=3 bin/etcdctl --user root:p role grant r1 readwrite a b
$ ETCDCTL_API=3 bin/etcdctl --user root:p role grant --open-ended r1 readwrite c
$ ETCDCTL_API=3 bin/etcdctl --user root:p role get r1
Role r1
KV Read:
        [a, b) (prefix a)
        [c, <open ended>
KV Write:
        [a, b) (prefix a)
        [c, <open ended>

Note that a closed parenthesis doesn't follow the above
for indicating that the role has an open ended permission ("" is a valid range end).

Fixes #7468

heyitsanthony · 2017-04-03T16:56:29Z

e2e/ctl_v3_role_test.go

+	key       string
+	rangeEnd  string
+	prefix    bool
+	openEnded bool


just use rangeEnd == "\x00" to represent this instead of a special flag?

heyitsanthony · 2017-04-03T18:17:18Z

etcdctl/ctlv3/command/printer_simple.go

@@ -141,7 +141,11 @@ func (s *simplePrinter) RoleGet(role string, r v3.AuthRoleGetResponse) {
 	printRange := func(perm *v3.Permission) {
 		sKey := string(perm.Key)
 		sRangeEnd := string(perm.RangeEnd)
-		fmt.Printf("\t[%s, %s)", sKey, sRangeEnd)
+		if strings.Compare(sRangeEnd, "\xff") != 0 {


should this be sRangeEnd != "\x00"?

Yes, my understanding of Range interface was very wrong... any byte sequences (other than ascii strings) are allowed to be a key so 0xff cannot be the largest key. I'll follow your suggestions and use StringAffineComparable for the purpose.

Just checking against "\x00" here should be OK, no need for the adt/affine stuff. RangeEnd == "\x00" meaning >= is already part of the RPC wire protocol.

Sorry for my confusing writing. Yes, I'll just add the checking against "\x00" here. The idea of the affine stuff will be implemented in the perm cache side. My comment wasn't limited to the above line.

heyitsanthony · 2017-04-03T18:18:03Z

etcdctl/ctlv3/command/printer_simple.go

@@ -188,7 +192,11 @@ func (s *simplePrinter) RoleRevokePermission(role string, key string, end string
 		fmt.Printf("Permission of key %s is revoked from role %s\n", key, role)
 		return
 	}
-	fmt.Printf("Permission of range [%s, %s) is revoked from role %s\n", key, end, role)
+	if strings.Compare(end, "\xff") != 0 {


same here, check end is "\x00"?

heyitsanthony · 2017-04-03T18:21:18Z

etcdctl/ctlv3/command/role_command.go

@@ -83,16 +84,21 @@ func newRoleGrantPermissionCommand() *cobra.Command {
 	}

 	cmd.Flags().BoolVar(&grantPermissionPrefix, "prefix", false, "grant a prefix permission")
+	cmd.Flags().BoolVar(&openEnded, "open-ended", false, "grant an open ended range permission")


probably should be from-key to match etcdctl get flags

Yes, I'll use the name as the option.

heyitsanthony · 2017-04-03T18:22:37Z

etcdctl/ctlv3/command/role_command.go

 		rangeEnd = clientv3.GetPrefixRangeEnd(args[2])
+	} else if openEnded {
+		rangeEnd = "\xff"


rangeEnd = "\x00"?

heyitsanthony · 2017-04-03T18:22:46Z

etcdctl/ctlv3/command/role_command.go

@@ -190,6 +207,8 @@ func roleRevokePermissionCommandFunc(cmd *cobra.Command, args []string) {
 	rangeEnd := ""
 	if 3 <= len(args) {
 		rangeEnd = args[2]
+	} else if openEnded {
+		rangeEnd = "\xff"


rangeEnd = "\x00"?

heyitsanthony · 2017-04-03T18:23:10Z

etcdctl/ctlv3/command/role_command.go

 		Use:   "revoke-permission <role name> <key> [endkey]",
 		Short: "Revokes a key from a role",
 		Run:   roleRevokePermissionCommandFunc,
 	}
+
+	cmd.Flags().BoolVar(&openEnded, "open-ended", false, "revoke an open ended range permission")


from-key here too

heyitsanthony · 2017-04-03T18:25:33Z

e2e/ctl_v3_auth_test.go

+	// try the granted open ended permission
+	cx.user, cx.pass = "test-user", "pass"
+	for i := 0; i < 10; i++ {
+		key := fmt.Sprintf("z%d", i)


Can stress the >= range a bit more with \xff's:

key := "\xff" for j := 0; j < i; j++ { key += "\xff" }

mitake · 2017-04-04T08:02:08Z

@heyitsanthony updated based on your comments, especially for changing the option name (--open-ended -> --from-key) and following the wire RPC protocol (\xff -> \x00). Could you take a look?

This commit adds a new option --from-key to the command etcdctl role grant-permission. If the option is passed, an open ended permission will be granted to a role e.g. from start-key to any keys those are larger than start-key. Example: $ ETCDCTL_API=3 bin/etcdctl --user root:p role grant r1 readwrite a b $ ETCDCTL_API=3 bin/etcdctl --user root:p role grant --from-key r1 readwrite c $ ETCDCTL_API=3 bin/etcdctl --user root:p role get r1 Role r1 KV Read: [a, b) (prefix a) [c, <open ended> KV Write: [a, b) (prefix a) [c, <open ended> Note that a closed parenthesis doesn't follow the above <open ended> for indicating that the role has an open ended permission ("<open ended>" is a valid range end). Fixes etcd-io#7468

heyitsanthony · 2017-04-04T16:57:10Z

lgtm. Thanks!

mitake · 2017-04-05T02:03:49Z

@heyitsanthony thanks, I'm merging it

mitake requested a review from heyitsanthony April 3, 2017 09:08

heyitsanthony suggested changes Apr 3, 2017

View reviewed changes

mitake force-pushed the range-open-ended branch from 687afa8 to 70f5516 Compare April 4, 2017 08:00

mitake force-pushed the range-open-ended branch from 70f5516 to 33d5b20 Compare April 4, 2017 08:10

mitake added 2 commits April 4, 2017 17:28

e2e: add a test case for the --open-ended option

a204b14

mitake force-pushed the range-open-ended branch from 33d5b20 to a204b14 Compare April 4, 2017 08:29

heyitsanthony approved these changes Apr 4, 2017

View reviewed changes

gyuho added the kind/release-note label Apr 4, 2017

mitake merged commit 6335506 into etcd-io:master Apr 5, 2017

mitake deleted the range-open-ended branch April 5, 2017 02:03

gyuho removed the kind/need-release-note-v3.3 label Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcdctl: add a new option --open-ended for unlimited range permission #7649

etcdctl: add a new option --open-ended for unlimited range permission #7649

mitake commented Apr 3, 2017

heyitsanthony Apr 3, 2017

heyitsanthony Apr 3, 2017

mitake Apr 4, 2017

heyitsanthony Apr 4, 2017

mitake Apr 4, 2017

heyitsanthony Apr 3, 2017

heyitsanthony Apr 3, 2017

mitake Apr 4, 2017

heyitsanthony Apr 3, 2017

heyitsanthony Apr 3, 2017

heyitsanthony Apr 3, 2017

heyitsanthony Apr 3, 2017

mitake commented Apr 4, 2017

heyitsanthony commented Apr 4, 2017

mitake commented Apr 5, 2017

etcdctl: add a new option --open-ended for unlimited range permission #7649

etcdctl: add a new option --open-ended for unlimited range permission #7649

Conversation

mitake commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitake commented Apr 4, 2017

heyitsanthony commented Apr 4, 2017

mitake commented Apr 5, 2017