auth/store: save consistentIndex to fix a data corruption bug #11652

tangcong · 2020-02-24T16:36:12Z

fix issue #11651

codecov-io · 2020-02-24T17:07:56Z

Codecov Report

Merging #11652 into master will decrease coverage by 0.42%.
The diff coverage is 78.84%.

@@            Coverage Diff             @@
##           master   #11652      +/-   ##
==========================================
- Coverage   66.66%   66.24%   -0.43%     
==========================================
  Files         401      402       +1     
  Lines       36629    36671      +42     
==========================================
- Hits        24420    24291     -129     
- Misses      10714    10862     +148     
- Partials     1495     1518      +23

Impacted Files	Coverage Δ
etcdserver/server.go	`75.79% <100%> (-0.16%)`	⬇️
mvcc/watchable_store.go	`86.47% <100%> (+2.24%)`	⬆️
etcdserver/backend.go	`56.25% <100%> (ø)`	⬆️
auth/store.go	`71.3% <68.75%> (-4.1%)`	⬇️
auth/metrics.go	`87.5% <87.5%> (ø)`
pkg/transport/timeout_conn.go	`60% <0%> (-40%)`	⬇️
client/keys.go	`60.3% <0%> (-28.15%)`	⬇️
auth/simple_token.go	`74.78% <0%> (-14.29%)`	⬇️
clientv3/ordering/kv.go	`72.05% <0%> (-13.24%)`	⬇️
proxy/grpcproxy/register.go	`72.5% <0%> (-10%)`	⬇️
... and 32 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 085b19b...7de3be9. Read the comment docs.

mitake

Thanks a lot for your very detailed investigation and bugfix!! I think the PR is almost ready to be merged after fixing some style in the commit logs.

The log of the 1st commit should be: *: fix auth revision corruption bug
The log of the 3rd commit should be: auth: add new metric 'etcd_debugging_auth_revision'
The 4th commit should be merged to the 1st one

auth/store.go

mitake · 2020-02-25T13:37:29Z

BTW I think lessor.Grant() (technically lease.persistTo()) would have the same problem of not updating the consistent_index. Replaying Grant multiple times wouldn't be harmful like the case of auth operations, but it might make the stats like leaseTotalTTLs and leaseGranted manipulated incorrectly. How do you think @jpbetz ?

^ on the second thought the counters aren't persisted so the only side effect caused by replaying would be granting again.

tangcong · 2020-02-25T14:43:52Z

@mitake thanks. i will optimize pr based on your suggestions.
do you have better advice that how to call mvcc.saveIndex function in auth/store? it is a private method in mvcc/store. it does not seem very good.
it is possible to cause data inconsistency when we restart etcd. it is better if we can backport this to 3.2, 3.3 and 3.4, as well as changelog note.
i think it is necessary to improve code testability about auth revision. there is no e2e test about auth revision because client can not get auth revision. can we submit another pr to add auth revision field in StatusResponse?
how do you think? @mitake @jingyih @gyuho

tangcong · 2020-02-25T16:03:30Z

Thanks a lot for your very detailed investigation and bugfix!! I think the PR is almost ready to be merged after fixing some style in the commit logs.

The log of the 1st commit should be: *: fix auth revision corruption bug

The log of the 3rd commit should be: auth: add new metric 'etcd_debugging_auth_revision'

The 4th commit should be merged to the 1st one

done, PTAL.

codecov-io · 2020-02-25T16:49:39Z

Codecov Report

Merging #11652 into master will decrease coverage by 0.66%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master   #11652      +/-   ##
==========================================
- Coverage   66.66%      66%   -0.67%     
==========================================
  Files         401      402       +1     
  Lines       36629    36672      +43     
==========================================
- Hits        24420    24204     -216     
- Misses      10714    10966     +252     
- Partials     1495     1502       +7

Impacted Files	Coverage Δ
etcdserver/server.go	`75.34% <100%> (-0.61%)`	⬇️
mvcc/watchable_store.go	`84.34% <100%> (+0.11%)`	⬆️
etcdserver/backend.go	`56.25% <100%> (ø)`	⬆️
auth/store.go	`55.05% <60.6%> (-20.35%)`	⬇️
auth/metrics.go	`87.5% <87.5%> (ø)`
client/members.go	`51.61% <0%> (-33.88%)`	⬇️
pkg/transport/timeout_conn.go	`80% <0%> (-20%)`	⬇️
proxy/httpproxy/director.go	`52.43% <0%> (-15.86%)`	⬇️
proxy/grpcproxy/register.go	`72.5% <0%> (-10%)`	⬇️
... and 25 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 085b19b...f14d2a0. Read the comment docs.

jingyih

Thanks a lot for digging into this issue and fixing it!

jingyih · 2020-02-25T18:04:37Z

mvcc/watchable_store.go

@@ -69,11 +70,11 @@ type watchableStore struct {
 // cancel operations.
 type cancelFunc func()

-func New(lg *zap.Logger, b backend.Backend, le lease.Lessor, ig ConsistentIndexGetter, cfg StoreConfig) ConsistentWatchableKV {
-	return newWatchableStore(lg, b, le, ig, cfg)
+func New(lg *zap.Logger, b backend.Backend, le lease.Lessor, as auth.AuthStore, ig ConsistentIndexGetter, cfg StoreConfig) ConsistentWatchableKV {


I think logically we want to pass ConsistentIndexGetter to NewAuthStore(), so that the auth store can get consistent index from etcdserver, and writes it to backend meta bucket at the end of applying a raft entry (such as adding a user, deleting a user, etc). Not sure if this means a lot of extra complexity in terms of implementation.

thanks. passing ConsistentIndexGetter to NewAuthStore will cause the saveIndex funtion to repeat.it looks a little ugly and violates the principle of function reuse. this is what we did in the first version.

in the second version, we define a public method SaveIndex in mvcc/store and define a interface ConsistentIndexSyncer which has a SaveIndex method that implemented by mvcc/store.SaveIndex. then, we want to pass ConsistentIndexSyncer interface to NewAuthStore().
What you see is the third version. After weighing the pros and cons, we choose the third version.

Thanks for trying multiple implementations, and I understand maybe the current implementation makes the most sense coding wise. However here is my push back to the current implementation: auth store's correctness should not depend on passing the store pointer to mvcc. Auth store should be relatively independent of mvcc, although they share the same backend.
cc @mitake

In my opinion, consistentIndex is a global concept. It has nothing to do with mvcc. It is better to separate it from mvcc/store and encapsulate it in a separate package. When mvcc, lessor, auth and other modules need to prevent repeated execution of commands, call consistentIndex package Save methods to store. What do you think of this solution? but I'm not sure how much complexity this approach will cause. which solution do you prefer? it is a cricital bug for us,so we take a simple and safe approach to fix it.
thanks. @jingyih @mitake

Totally agree with you! @tangcong

Got it. For short term we can get this merged and backported. Could you add a TODO for encapsulating consistentindex into a separate package?

done. @jingyih

mitake · 2020-02-26T15:05:06Z

@tangcong I think the ideal way of updating consistent index would be writing data through mvcc, not directly access to backend. e.g. storeTxnWrite.End() calls saveIndex() internally. But your change works for the issue so merging this first would be fine.

mitake

LGTM! defer to @jingyih

jingyih

Thanks again for fixing this! I added few comments. LGTM after fix.

auth/metrics.go

auth/store.go

tangcong · 2020-02-29T03:35:47Z

Thanks again for fixing this! I added few comments. LGTM after fix.

done. thanks.

…io#11735

…io#11735,etcd-io#11659

#11670-#11710-origin-release-3.3 Automated cherry pick of #11652 #11670 #11710

#11670-#11710-origin-release-3.4 Automated cherry pick of #11652 #11670 #11710

CHANGELOG: update for #11699,#11670,#11652,#11735,#11659

tangcong requested review from mitake and jpbetz February 24, 2020 16:44

tangcong force-pushed the fix-auth-store-corruption-bug branch from d97c8b0 to 7de3be9 Compare February 25, 2020 03:52

tangcong requested review from gyuho and jingyih and removed request for jpbetz February 25, 2020 03:59

mitake suggested changes Feb 25, 2020

View reviewed changes

mitake reviewed Feb 25, 2020

View reviewed changes

auth/store.go Outdated Show resolved Hide resolved

tangcong force-pushed the fix-auth-store-corruption-bug branch from 7de3be9 to fc44b73 Compare February 25, 2020 15:56

jingyih reviewed Feb 25, 2020

View reviewed changes

tangcong force-pushed the fix-auth-store-corruption-bug branch from fc44b73 to beb8d39 Compare February 26, 2020 02:34

mitake approved these changes Feb 26, 2020

View reviewed changes

wswcfan mentioned this pull request Feb 26, 2020

etcdserver: add auth revision to AuthStatus to improve observability and testability #11659

Merged

tangcong force-pushed the fix-auth-store-corruption-bug branch from beb8d39 to 88b580d Compare February 27, 2020 17:41

jingyih approved these changes Feb 28, 2020

View reviewed changes

auth/metrics.go Show resolved Hide resolved

auth/store.go Outdated Show resolved Hide resolved

auth/store.go Outdated Show resolved Hide resolved

auth/store.go Show resolved Hide resolved

tangcong force-pushed the fix-auth-store-corruption-bug branch 2 times, most recently from 6ae860f to 2c99a83 Compare February 29, 2020 02:43

tangcong and others added 3 commits February 29, 2020 13:31

*: fix auth revision corruption bug

06ad533

tools/etcd-dump-db: add auth decoder, optimize print format

08a8b80

auth: add new metric 'etcd_debugging_auth_revision'

f14d2a0

tangcong force-pushed the fix-auth-store-corruption-bug branch from 2c99a83 to f14d2a0 Compare February 29, 2020 05:32

jingyih merged commit cb63341 into etcd-io:master Mar 1, 2020

jingyih mentioned this pull request Mar 1, 2020

a data corruption bug in all etcd3 version when authentication is enabled #11651

Closed

tangcong mentioned this pull request Mar 9, 2020

Automated cherry pick of #11652 #11670 #11686

Closed

tangcong mentioned this pull request Apr 5, 2020

Automated cherry pick of #11652 #11670 #11699 #11710 #11748

Closed

tangcong added a commit to tangcong/etcd that referenced this pull request Apr 5, 2020

changelog: update for etcd-io#11699,etcd-io#11670,etcd-io#11652,etcd-…

c28807f

…io#11735

tangcong added a commit to tangcong/etcd that referenced this pull request Apr 5, 2020

changelog: update for etcd-io#11699,etcd-io#11670,etcd-io#11652,etcd-…

0ab10a1

…io#11735

tangcong added a commit to tangcong/etcd that referenced this pull request Apr 5, 2020

CHANGELOG: update for etcd-io#11699,etcd-io#11670,etcd-io#11652,etcd-…

b93ba91

…io#11735

tangcong added a commit to tangcong/etcd that referenced this pull request Apr 5, 2020

CHANGELOG: update for etcd-io#11699,etcd-io#11670,etcd-io#11652,etcd-…

75c3e12

…io#11735,etcd-io#11659

This was referenced Apr 6, 2020

Automated cherry pick of #11652 #11670 #11710 origin release 3.4 #11751

Closed

Automated cherry pick of #11652 #11670 #11710 #11752

Merged

Automated cherry pick of #11652 #11670 #11710 #11753

Merged

tangcong added a commit to tangcong/etcd that referenced this pull request Apr 6, 2020

CHANGELOG: update for etcd-io#11699,etcd-io#11670,etcd-io#11652,etcd-…

b2ebf6e

…io#11735,etcd-io#11659

jingyih added a commit that referenced this pull request Apr 10, 2020

Merge pull request #11753 from tangcong/automated-cherry-pick-of-#11652-

7e20b9f

#11670-#11710-origin-release-3.3 Automated cherry pick of #11652 #11670 #11710

jingyih added a commit that referenced this pull request Apr 10, 2020

Merge pull request #11752 from tangcong/automated-cherry-pick-of-#11652-

f1eca4e

#11670-#11710-origin-release-3.4 Automated cherry pick of #11652 #11670 #11710

jingyih added a commit that referenced this pull request Apr 10, 2020

Merge pull request #11750 from tangcong/changelog

c8d3e99

CHANGELOG: update for #11699,#11670,#11652,#11735,#11659

jingyih mentioned this pull request May 19, 2020

Update to etcd 3.4.9 in Kubernetes 1.19 kubernetes/kubernetes#91266

Closed

4 tasks

warmchang mentioned this pull request Jun 7, 2020

Add embedded etcd support k3s-io/k3s#1770

Merged

tangcong deleted the fix-auth-store-corruption-bug branch February 26, 2021 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auth/store: save consistentIndex to fix a data corruption bug #11652

auth/store: save consistentIndex to fix a data corruption bug #11652

tangcong commented Feb 24, 2020

codecov-io commented Feb 24, 2020 •

edited

Loading

mitake left a comment

mitake commented Feb 25, 2020 •

edited

Loading

tangcong commented Feb 25, 2020 •

edited

Loading

tangcong commented Feb 25, 2020

codecov-io commented Feb 25, 2020 •

edited

Loading

jingyih left a comment

jingyih Feb 25, 2020

tangcong Feb 25, 2020

tangcong Feb 25, 2020

jingyih Feb 26, 2020

tangcong Feb 27, 2020 •

edited

Loading

jingyih Feb 27, 2020

jingyih Feb 27, 2020

tangcong Feb 27, 2020

mitake commented Feb 26, 2020

mitake left a comment

jingyih left a comment

tangcong commented Feb 29, 2020

auth/store: save consistentIndex to fix a data corruption bug #11652

auth/store: save consistentIndex to fix a data corruption bug #11652

Conversation

tangcong commented Feb 24, 2020

codecov-io commented Feb 24, 2020 • edited Loading

Codecov Report

mitake left a comment

Choose a reason for hiding this comment

mitake commented Feb 25, 2020 • edited Loading

tangcong commented Feb 25, 2020 • edited Loading

tangcong commented Feb 25, 2020

codecov-io commented Feb 25, 2020 • edited Loading

Codecov Report

jingyih left a comment

Choose a reason for hiding this comment

jingyih Feb 25, 2020

Choose a reason for hiding this comment

tangcong Feb 25, 2020

Choose a reason for hiding this comment

tangcong Feb 25, 2020

Choose a reason for hiding this comment

jingyih Feb 26, 2020

Choose a reason for hiding this comment

tangcong Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

jingyih Feb 27, 2020

Choose a reason for hiding this comment

jingyih Feb 27, 2020

Choose a reason for hiding this comment

tangcong Feb 27, 2020

Choose a reason for hiding this comment

mitake commented Feb 26, 2020

mitake left a comment

Choose a reason for hiding this comment

jingyih left a comment

Choose a reason for hiding this comment

tangcong commented Feb 29, 2020

codecov-io commented Feb 24, 2020 •

edited

Loading

mitake commented Feb 25, 2020 •

edited

Loading

tangcong commented Feb 25, 2020 •

edited

Loading

codecov-io commented Feb 25, 2020 •

edited

Loading

tangcong Feb 27, 2020 •

edited

Loading