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: ensure RoleGrantPermission is compatible with older versions #11710

Merged
merged 1 commit into from
Mar 25, 2020

Conversation

tangcong
Copy link
Contributor

@tangcong tangcong commented Mar 22, 2020

we learn lessons from issue #11689, if a feature that may update auth-revision or revision is not compatible with older versions, it is possible to cause data inconsistency. so we have to ensure RoleGrantPermission is compatible with older versions, just revert it to old versions.

@mitake @jingyih

@codecov-io
Copy link

codecov-io commented Mar 22, 2020

Codecov Report

Merging #11710 into master will increase coverage by 0.40%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11710      +/-   ##
==========================================
+ Coverage   66.02%   66.43%   +0.40%     
==========================================
  Files         403      403              
  Lines       36731    36727       -4     
==========================================
+ Hits        24252    24399     +147     
+ Misses      10976    10833     -143     
+ Partials     1503     1495       -8     
Impacted Files Coverage Δ
auth/store.go 78.24% <ø> (+26.08%) ⬆️
client/client.go 49.34% <0.00%> (-34.65%) ⬇️
clientv3/namespace/watch.go 87.87% <0.00%> (-6.07%) ⬇️
proxy/grpcproxy/watcher.go 85.71% <0.00%> (-4.09%) ⬇️
etcdserver/v3_server.go 72.92% <0.00%> (-1.53%) ⬇️
clientv3/client.go 74.05% <0.00%> (-0.59%) ⬇️
etcdserver/server.go 75.64% <0.00%> (-0.54%) ⬇️
etcdmain/main.go 0.00% <0.00%> (ø)
raft/raft.go 90.97% <0.00%> (+0.63%) ⬆️
clientv3/leasing/kv.go 90.36% <0.00%> (+0.66%) ⬆️
... and 17 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 0eee733...d70600f. Read the comment docs.

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! Defer to @jingyih

@mitake
Copy link
Contributor

mitake commented Mar 22, 2020

We should have a process for listing up every update which introduces changes of the etcd state machine in the documents. Also we should have a guide for operators for handling such changes (e.g. take snapshot then restore it to a new cluster might be helpful). It's a challenging problem for a system based on replicated state machine...

@jingyih
Copy link
Contributor

jingyih commented Mar 23, 2020

If we revert the change, do we still need the test TestRoleGrantPermissionRevision?

@jingyih
Copy link
Contributor

jingyih commented Mar 23, 2020

Totally agree we need a formal guidance on rolling out behavior changes (including bug fixes) in state machine apply.

@tangcong
Copy link
Contributor Author

tangcong commented Mar 24, 2020

If we revert the change, do we still need the test TestRoleGrantPermissionRevision?

it can increase test coverage, so i keep it.

@jingyih
Copy link
Contributor

jingyih commented Mar 24, 2020

IIUC, the old test verifies that the duplicate RoleGrantPermission request will not be applied by the server. For consistency, we are reverting that behavior change. Now the test says duplicate RoleGrantPermission request will be applied by server - this is not the intended behavior and we probably want to correct it in the future (with proper migration process). Therefore we probably do not want to have a test verifying this new behavior. If you really want to keep it, add a comment or TODO explaining this is not the intended behavior?

@tangcong
Copy link
Contributor Author

IIUC, the old test verifies that the duplicate RoleGrantPermission request will not be applied by the server. For consistency, we are reverting that behavior change. Now the test says duplicate RoleGrantPermission request will be applied by server - this is not the intended behavior and we probably want to correct it in the future (with proper migration process). Therefore we probably do not want to have a test verifying this new behavior. If you really want to keep it, add a comment or TODO explaining this is not the intended behavior?

ok, i have cleaned up it.

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.

LGTM. 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