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

auth_command: create etcdctl auth status command #11536

Merged
merged 1 commit into from
Feb 6, 2020

Conversation

tarcinil
Copy link
Contributor

This changes have started at etcdctl under auth.go, and make changes to stub out everything down into the internal raft. Made changes to the .proto files and regenerated them so that the local version would build successfully.

fixes #11516

@tarcinil tarcinil requested a review from xiang90 January 17, 2020 02:32
@codecov-io
Copy link

codecov-io commented Jan 17, 2020

Codecov Report

Merging #11536 into master will decrease coverage by 0.58%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11536      +/-   ##
==========================================
- Coverage   64.25%   63.66%   -0.59%     
==========================================
  Files         403      403              
  Lines       38091    38116      +25     
==========================================
- Hits        24475    24268     -207     
- Misses      11979    12204     +225     
- Partials     1637     1644       +7
Impacted Files Coverage Δ
etcdserver/api/v3rpc/auth.go 43.67% <0%> (-2.67%) ⬇️
proxy/grpcproxy/auth.go 32.07% <0%> (-1.93%) ⬇️
proxy/grpcproxy/adapter/auth_client_adapter.go 33.33% <0%> (-1.97%) ⬇️
etcdserver/v3_server.go 70.61% <0%> (-2.9%) ⬇️
clientv3/auth.go 48.75% <0%> (-4.5%) ⬇️
etcdserver/apply.go 77.04% <0%> (-0.6%) ⬇️
clientv3/retry.go 68.42% <0%> (-1.23%) ⬇️
etcdserver/apply_auth.go 55.72% <0%> (-0.87%) ⬇️
etcdctl/ctlv3/command/auth_command.go 38.98% <40%> (+0.34%) ⬆️
client/client.go 53.26% <0%> (-11.77%) ⬇️
... and 28 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 574ee42...98c7290. Read the comment docs.

@tarcinil tarcinil force-pushed the fix/11516/etcdctl-auth-status branch from d060265 to 2ce580e Compare January 19, 2020 00:34
@tarcinil
Copy link
Contributor Author

tarcinil commented Jan 19, 2020

I have worked further on what I believe is a mostly working command. I don't have a good idea on how to remove the &{} stuff from the output. Any additional input you could give would be helpful.

Thanks.

$ ./bin/etcdctl auth status            
Authentication Status:  &{false}

$ ./bin/etcdctl auth enable                
Authentication Enabled

$ ./bin/etcdctl auth status --user root    
Password: 
Authentication Status:  &{true}

EDIT: I have figured it out. It no longer has a &{}.

@tarcinil tarcinil marked this pull request as ready for review January 19, 2020 01:06
@tarcinil tarcinil changed the title WIP: *: stubbing out AuthStatus command *: stubbing out AuthStatus command Jan 19, 2020
@mitake
Copy link
Contributor

mitake commented Jan 20, 2020

@tarcinil thanks for the PR! I'd like to review it. But before that could you reorganize your commits with below rules for easier review?

  • our typical commit log is formatted like this: package name: description of your change
  • refactoring commits e.g. gofmt error fix shouldn't be an independent commit, could you squash them to suitable commits?

@tarcinil tarcinil force-pushed the fix/11516/etcdctl-auth-status branch from 131758a to fed0a0f Compare January 20, 2020 14:13
@tarcinil
Copy link
Contributor Author

@mitake Let me know if that is the change to the commit that you had in mind.

@tarcinil tarcinil changed the title *: stubbing out AuthStatus command auth_command: create etcdctl auth status command Jan 20, 2020
@tarcinil tarcinil closed this Jan 21, 2020
@tarcinil tarcinil reopened this Jan 21, 2020
@tarcinil
Copy link
Contributor Author

Forcing Travis Build as the test that failed didn't have to do with my work and they passed before squash. Brittle test?

Copy link
Contributor

@mitake mitake left a comment

Choose a reason for hiding this comment

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

Could you squash 1st, 2nd, 4th and 5th commits into a single commit? The commit log should be *: add a new API and command for checking auth status.
Also you can drop 3rd commit because authStore.IsAuthEnabled() can be reused for the purpose and the method already has its tests.

Thanks a lot for your PR!

auth/store.go Outdated Show resolved Hide resolved
etcdserver/apply.go Outdated Show resolved Hide resolved
@tarcinil tarcinil removed the request for review from xiang90 January 27, 2020 21:34
@tarcinil tarcinil force-pushed the fix/11516/etcdctl-auth-status branch from fed0a0f to deb850b Compare January 29, 2020 01:14
@tarcinil tarcinil requested a review from mitake January 29, 2020 01:21
Copy link
Contributor

@mitake mitake left a comment

Choose a reason for hiding this comment

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

AuthStatus() isn't required. After fixing it I think this PR can be merged. Could you cross check @xiang90 @gyuho @jingyih ?

auth/store.go Outdated Show resolved Hide resolved
auth/store_test.go Outdated Show resolved Hide resolved
@tarcinil tarcinil force-pushed the fix/11516/etcdctl-auth-status branch 3 times, most recently from 935b0fe to 98c7290 Compare February 4, 2020 01:58
Copy link
Contributor

@mitake mitake left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot @tarcinil !
Could you cross check this PR @xiang90 @gyuho @jingyih ?

Copy link
Contributor

@jingyih jingyih left a comment

Choose a reason for hiding this comment

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

Overall looks good. Thanks! We also need to highlight this change in CHANGELOG-3.5.

etcdctl/ctlv3/command/auth_command.go Outdated Show resolved Hide resolved
etcdserver/etcdserverpb/rpc.proto Outdated Show resolved Hide resolved
@xiang90
Copy link
Contributor

xiang90 commented Feb 5, 2020

lgtm

@xiang90
Copy link
Contributor

xiang90 commented Feb 5, 2020

@tarcinil can you add this to the changelog?

@jingyih
Copy link
Contributor

jingyih commented Feb 5, 2020

LGTM. Thanks!

@tarcinil
Copy link
Contributor Author

tarcinil commented Feb 5, 2020

I will look at doing the changelog tonight.

This changes have started at etcdctl under auth.go, and make changes to stub out everything down into the internal raft.  Made changes to the .proto files and regenerated them so that the local version would build successfully.
@tarcinil tarcinil force-pushed the fix/11516/etcdctl-auth-status branch from cdf9368 to 3d74793 Compare February 5, 2020 23:44
@tarcinil
Copy link
Contributor Author

tarcinil commented Feb 5, 2020

@xiang90 Changelog has been added. I created an entry for etcdctl and API as it technically covers both.

@xiang90
Copy link
Contributor

xiang90 commented Feb 6, 2020

lgtm

@xiang90 xiang90 merged commit 071e70c into etcd-io:master Feb 6, 2020
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.

CLI command: etcdctl auth status
5 participants