etcdserver: do not allow creating empty role

[spzala]

Like user, we should not allow creating empty role.

Related #10905

[jingyih]

Might be a good chance to add more description to the APIs? For example:

etcd/auth/store.go

Lines 114 to 115 in 828225c

	// UserAdd adds a new user
	UserAdd(r *pb.AuthUserAddRequest) (*pb.AuthUserAddResponse, error)

etcd/auth/store.go

Lines 132 to 133 in 828225c

	// RoleAdd adds a new role
	RoleAdd(r *pb.AuthRoleAddRequest) (*pb.AuthRoleAddResponse, error)

[spzala]

Might be a good chance to add more description to the APIs? For example:

etcd/auth/store.go

Lines 114 to 115 in 828225c

	// UserAdd adds a new user
	UserAdd(r *pb.AuthUserAddRequest) (*pb.AuthUserAddResponse, error)

etcd/auth/store.go

Lines 132 to 133 in 828225c

	// RoleAdd adds a new role
	RoleAdd(r *pb.AuthRoleAddRequest) (*pb.AuthRoleAddResponse, error)

Thanks @jingyih I updated the commit, does that look OK?

[jingyih]

Thanks @jingyih I updated the commit, does that look OK?

Thanks for adding more description.

I understand the existing pattern is to check for empty user name during apply. Have we considered moving the check prior to sending request to Raft, so the request fails faster?
cc @mitake

[spzala]

Thanks @jingyih I updated the commit, does that look OK?

Thanks for adding more description.

I understand the existing pattern is to check for empty user name during apply. Have we considered moving the check prior to sending request to Raft, so the request fails faster?
cc @mitake

@jingyih np and thank you! I initially and naturally thought of doing so i.e. to return error in etcdctl role_command and clientv3/auth but as you noticed I have tried following how user name is being handled currently. Let's wait on comment from @mitake

[jingyih]

Sounds good.

On the server side this can also be done earlier:

etcd/etcdserver/api/v3rpc/auth.go

Line 56 in fe86a78

func (as *AuthServer) RoleAdd(ctx context.Context, r *pb.AuthRoleAddRequest) (*pb.AuthRoleAddResponse, error) {

[spzala]

OK, that sounds great. Thanks @jingyih

[mitake]

@spzala @jingyih thanks for doing this! I think the error checking should be in the state machine layer as implemented in this PR because it is more defensive than checking in the API layer. Even if something in the API layer is broken by the future change, the state machine layer can deny it. I think it is safer.
Also operations of role creation doesn't happen so much (unlike reading/writing keys). The overhead passing the log entry to the Raft layer wouldn't be a problem.
However it can potentially change the behavior of existing clusters which have empty roles (although such a situation isn't good). Probably writing it to the changelog would be user friendly, I think.

[spzala]

@mitake hello, thank you so much for the quick comment!! Agree, I don't see a real good reason to create empty role besides playing with it or accident. For courtesy writing to changelog sounds good too. I will defer to @jingyih for a decision.

[jingyih]

@mitake Thanks for the explanation. It makes sense to me.

@spzala I agree we should document the change. What is the current behavior when user try to create a role with empty name? Reading from the bug report, it looks like server will crash, and client will get an error with code = Unavailable. With this PR, client will get an error with code = InvalidArgument. Is my understanding correct?

[spzala]

@jingyih yup, you understood it correct! I guess this will go in the CHANGELOG-3.4, should I update it under this PR or as a follow up once this gets merged? Thanks!

[jingyih]

I think either is fine. Maybe simply push a commit to update the change log?

[jingyih]

lgtm after nits.

[jingyih]

remove the leading empty line?

[spzala]

Agree. Thanks!

[spzala]

Thanks @jingyih I have also updated the changelog!

[spzala]

/cc @xiang90

[codecov-io]

Codecov Report

Merging #10907 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #10907      +/-   ##
==========================================
+ Coverage   63.25%   63.31%   +0.05%     
==========================================
  Files         400      400              
  Lines       37687    37689       +2     
==========================================
+ Hits        23839    23861      +22     
+ Misses      12252    12232      -20     
  Partials     1596     1596

Impacted Files	Coverage Δ
etcdserver/api/v3rpc/util.go	51.61% <ø> (ø)	⬆️
etcdserver/api/v3rpc/rpctypes/error.go	90.47% <ø> (ø)	⬆️
auth/store.go	45.59% <100%> (+0.13%)	⬆️
client/client.go	68.3% <0%> (-5.23%)	⬇️
clientv3/leasing/cache.go	87.22% <0%> (-4.45%)	⬇️
clientv3/auth.go	59.74% <0%> (-2.6%)	⬇️
clientv3/balancer/balancer.go	84.12% <0%> (-2.39%)	⬇️
clientv3/leasing/kv.go	89.03% <0%> (-2.33%)	⬇️
clientv3/op.go	72.5% <0%> (-1.25%)	⬇️
clientv3/balancer/grpc1.7-health.go	58.43% <0%> (-0.88%)	⬇️
... and 15 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 d137fa9...1cef112. Read the comment docs.

[spzala]

@jingyih there was a merge conflict on changelog which I have fixed now. Please take a look now. It will be great if we can merge this one after your final review, considering we may have few more updates on changelog due to 3.4 release, to avoid any further conflicts :-). Thanks!

[jingyih]

lgtm. Thanks for fixing this.

[spzala]

@jingyih yrw, thank you so much for the quick review!!