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

Kubernetes is vulnerable to stale reads, violating critical pod safety guarantees #59848

Open
smarterclayton opened this issue Feb 14, 2018 · 67 comments

Comments

@smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Feb 14, 2018

When we added resourceVersion=0 to reflectors, we didn't properly reason about its impact on nodes. Its current behavior can cause two nodes to run a pod with the same name at the same time when using multiple api servers, which violates the pod safety guarantees on the cluster. Because a read serviced by the watch cache can be arbitrarily delayed, a client that connects to that api server can read an arbitrarily old history. We explicitly use quorum reads against etcd to prevent this.

Scenario:

  1. T1: StatefulSet controller creates pod-0 (uid 1) which is scheduled to node-1
  2. T2: pod-0 is deleted as part of a rolling upgrade
  3. node-1 sees that pod-0 is deleted and cleans it up, then deletes the pod in the api
  4. The StatefulSet controller creates a second pod pod-0 (uid 2) which is assigned to node-2
  5. node-2 sees that pod-0 has been scheduled to it and starts pod-0
  6. The kubelet on node-1 crashes and restarts, then performs an initial list of pods scheduled to it against an API server in an HA setup (more than one API server) that is partitioned from the master (watch cache is arbitrarily delayed). The watch cache returns a list of pods from before T2
  7. node-1 fills its local cache with a list of pods from before T2
  8. node-1 starts pod-0 (uid 1) and node-2 is already running pod-0 (uid 2).

This violates pod safety. Since we support HA api servers, we cannot use resourceVersion=0 from reflectors on the node, and probably should not use it on the masters. We can only safely use resourceVersion=0 after we have retrieved at least one list, and only if we verify that resourceVersion is in the future.

@kubernetes/sig-apps-bugs @kubernetes/sig-api-machinery-bugs @kubernetes/sig-scalability-bugs This is a fairly serious issue that can lead to cluster identity guarantees being lost, which means clustered software cannot run safely if it has assumed the pod safety guarantee prevents two pods with the same name running on the cluster at the same time. The user impact is likely data loss of critical data.

This is also something that could happen for controllers - during a controller lease failover the next leader could be working from a very old cache and undo recent work done.

No matter what, the first list of a component with a clean state that must preserve "happens-before" must perform a live quorum read against etcd to fill their cache. That can only be done by omitting resourceVersion=0.

Fixes:

  1. Disable resourceVersion=0 from being used in reflector list, only use when known safe
  2. Disable resourceVersion=0 for first list, and optionally use resourceVersion=0 for subsequent calls if we know the previous resourceVersion is after our current version (assumes resource version monotonicity)
  3. Disable resourceVersion=0 for the first list from a reflector, then send resourceVersion= on subsequent list calls (which causes the watch cache to wait until the resource version shows up).
  4. Perform live reads from Kubelet on all new pods coming in to verify they still exist

1 is a pretty significant performance regression, but is the most correct and safest option (just like we enabled quorum reads everywhere). 2 is more complex, and there are a few people trying to remove the monotonicity guarantees from resource version, but would retain most of the performance benefits of using this in the reflector. 3 is probably less complex than 2, but i'm not positive it actually works. 4 is hideous and won't fix other usage.

Probably needs to be backported to 1.6.

@smarterclayton smarterclayton added this to the v1.10 milestone Feb 14, 2018
@smarterclayton smarterclayton changed the title Use of resourceVersion=0 in reflectors for initial sync breaks pod safety when more than one master Use of resourceVersion=0 in reflectors for initial sync breaks pod safety when more than one api server Feb 14, 2018
@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

@smarterclayton smarterclayton commented Feb 14, 2018

Note that we added this to improve large cluster performance, so we are almost certainly going to regress to some degree (how much has been mitigated by other improvements in the last year is uncertain)

@roycaihw

This comment has been minimized.

Copy link
Member

@roycaihw roycaihw commented Feb 15, 2018

/sub

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

@smarterclayton smarterclayton commented Feb 17, 2018

Disabling resourceVersion=0 for lists when more than one master is present is an option as well.

As Jordan noted the optimization here is impactful because we avoid having to fetch many full pod lists from etcd when we only return a subset to each node. It’s possible we could require all resourceVersion=0 calls to acquire a read lease on etcd which bounds the delay, but doesn’t guarantee happens before if the cache is delayed. If the watch returned a freshness guarantee we could synchronize on that as well.

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

@smarterclayton smarterclayton commented Feb 17, 2018

We can do a synthetic write to the range and wait until it is observed by the cache, then service the rv=0. The logical equivalent is a serializable read on etcd for the range, but we need to know the highest observed RV on the range.

We can accomplish that by executing a range request with min create and mod revisions equal to the latest observed revision, and then performing the watch cache list at the highest RV on the range.

So:

  1. All reflectors need to preserve a “happens before” guarantee when fetching for general sanity - use of rv=0 breaks that
  2. rv=0 is a client visible optimization, but it’s not really necessary except for clients that can observe arbitrarily delayed history safely
  3. Not many clients can observe arbitrarily delayed history safely
  4. We should stop trearing rv=0 specially

We can mitigate the performance impact by ensuring that an initial list from the watch cache preserves happens before. We can safely serve historical lists (rv=N) at any time. The watch cache already handles rv=N mostly correctly.

So the proposed change here is:

  1. Clients remove rv=0 and we stop honoring it
  2. Verify the watch cache correctly waits for rv=N queries
  3. We can serve a list or get from watch cache iff we perform a serializable read against etcd and retrieve the highest create/mod revisions
  4. We can make that query efficient by using min_create/mod_revision on the etcd list call
  5. Clients currently sending rv=0 where perf is critical (node) should start using the last observed resource version, which allows the watch cache to serve correctly.

That resolves this issue.

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

@smarterclayton smarterclayton commented Feb 17, 2018

Serializable read over the range, that is.

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

@smarterclayton smarterclayton commented Feb 17, 2018

Also:

  1. The next time someone adds a cache at any layer, we need to have a good review process that catches this. Probably a checklist we add into api review and an audit of any existing caches.
@jberkus

This comment has been minimized.

Copy link

@jberkus jberkus commented Feb 21, 2018

if this MUST be resolved for 1.10, please add status/approved-for-milestone to it before Code Freeze. Thanks!

@jdumars

This comment has been minimized.

Copy link
Member

@jdumars jdumars commented Feb 23, 2018

Hi Clayton, could you go ahead and add "approved-for-milestone" label to this, as well as status (in progress)? That will help it stay in the milestone if this is a 1.10 blocker. Thanks!

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

@smarterclayton smarterclayton commented Feb 26, 2018

Caveat on the label - still trying to identify how we fix this - but no matter what because this is a critical data integrity issues we'll end up back porting this several releases. Will try to get a better estimate of timeframe soon.

@kow3ns kow3ns added this to Backlog in Workloads Feb 26, 2018
@jberkus

This comment has been minimized.

Copy link

@jberkus jberkus commented Feb 26, 2018

ooops, fixing labels.

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

@smarterclayton smarterclayton commented Feb 27, 2018

This can happen on a singleton master using an HA etcd cluster:

  1. apiserver is talking to member 0 in the cluster
  2. apiserver crashes and watch cache performs its initial list at RV=10
  3. kubelet deletes pod at RV=11
  4. a new pod is created at RV=12 on a different node
  5. kubelet crashes
  6. apiserver becomes partitioned from the etcd majority but is able to reestablish a watch from the partitioned member at RV=10
  7. kubelet contacts the apiserver and sees RV=10, launches the pod
@smarterclayton smarterclayton changed the title Use of resourceVersion=0 in reflectors for initial sync breaks pod safety when more than one api server Kubernetes is vulnerable to stale reads for critical pod safety guarantees Feb 28, 2018
@smarterclayton smarterclayton changed the title Kubernetes is vulnerable to stale reads for critical pod safety guarantees Kubernetes is vulnerable to stale reads, violating critical pod safety guarantees Feb 28, 2018
@wojtek-t

This comment has been minimized.

Copy link
Member

@wojtek-t wojtek-t commented Mar 1, 2018

So the proposed change here is:

  1. Clients remove rv=0 and we stop honoring it
  2. Verify the watch cache correctly waits for rv=N queries
  3. We can serve a list or get from watch cache iff we perform a serializable read against etcd and retrieve the highest create/mod revisions
  4. We can make that query efficient by using min_create/mod_revision on the etcd list call
  5. Clients currently sending rv=0 where perf is critical (node) should start using the last observed resource version, which allows the watch cache to serve correctly.

Why do we need (3) and (4)?
If we verify that rv=N works fine (and it should), why can't we just start setting rv= instead.
If we just do that there are two options:

  1. that RV is already in watch cache, and then we can safely serve from watch cache
  2. that RV is not yet in watch cache (watch cache is delayed) and then we will wait for some time and if it won't become fresh enough within some threshold we will reject with some error IIRC.

In the second case, reflector may retry without setting any RV for consistent read from etcd (that should be pretty rare so it should be fine from performance point of view).

@smarterclayton What an I missing?

@wojtek-t

This comment has been minimized.

Copy link
Member

@wojtek-t wojtek-t commented Mar 1, 2018

@jdumars

This comment has been minimized.

Copy link
Member

@jdumars jdumars commented Mar 1, 2018

Please stay in close contact with the release team on this.

@yastij

This comment has been minimized.

Copy link
Member

@yastij yastij commented Oct 25, 2018

any news on this @jpbetz ?

@jpbetz

This comment has been minimized.

Copy link
Contributor

@jpbetz jpbetz commented Oct 29, 2018

@yastij See the above linked design doc. Our main discussion/activity has been there. We'll be sending out PRs for the initial set of changes shortly.

@dims

This comment has been minimized.

Copy link
Member

@dims dims commented Jul 12, 2019

/priority important-soon
/milestone v1.16

(to match the priority and milestone in the PR)

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Jul 12, 2019
@makoscafee

This comment has been minimized.

Copy link
Member

@makoscafee makoscafee commented Aug 23, 2019

@jpbetz @dims Hello! I'm bug triage shadow for the 1.16 release cycle and considering this issue is tagged for 1.16, but not updated for a long time, I'd like to check its status. The code freeze is starting on August 29th (about 1.5 weeks from now), which means that there should be a PR ready (and merged) until then.

Do we still target this issue to be fixed for 1.16?

@justaugustus

This comment has been minimized.

Copy link
Member

@justaugustus justaugustus commented Aug 28, 2019

/remove-priority critical-urgent
/milestone clear

@k8s-ci-robot k8s-ci-robot removed this from the v1.16 milestone Aug 28, 2019
@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

@smarterclayton smarterclayton commented Aug 28, 2019

@jpbetz can you please update the status of this? I believe etcd mechanisms are in place now?

/priority critical-urgent

This is still the most severe possible known vulnerability in Kubernetes safety guarantees.

@lavalamp

This comment has been minimized.

Copy link
Member

@lavalamp lavalamp commented Aug 28, 2019

Joe is OOO; I was under the impression that this was mostly or completely done, but I could be wrong.

@wojtek-t

This comment has been minimized.

Copy link
Member

@wojtek-t wojtek-t commented Aug 28, 2019

@jberkus

This comment has been minimized.

Copy link

@jberkus jberkus commented Aug 28, 2019

Generally "critical-urgent" means "needs to be, and will be, fixed in 3-10 days", and, importantly, "Kubernetes cannot be released unless we fix this first".

Given that we have had 5 releases since this issue was first opened, that's manifestly not true.

@jpbetz

This comment has been minimized.

Copy link
Contributor

@jpbetz jpbetz commented Sep 17, 2019

@smarterclayton

Apologies. This got stalled out. We had split this into two sub-problems:

  • Clarifying how resourceVersion semantics currently behave and proposing some adjustments to make them consistent (both across operations, and when the watch cache is enabled/disabled) (ref). Once we have this resolved we can fix #67998.
  • Improving the reflector API so that clients have the primitives they need to avoid stale reads and providing clear guidance to clients must do to avoid stale reads (ref)

I can circle back on the resourceVersion semantics this week. If we can get closure on that then we can work on updating reflectors to be consistent across restarts and then talk more with sig-node about options for fixing the node problem.

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

@smarterclayton smarterclayton commented Sep 17, 2019

@jpbetz

This comment has been minimized.

Copy link
Contributor

@jpbetz jpbetz commented Sep 19, 2019

#82862 and #72170 are ready for review.

@markyjackson-taulia

This comment has been minimized.

Copy link

@markyjackson-taulia markyjackson-taulia commented Oct 26, 2019

@jpbetz Bug triage for 1.17 here with a gentle reminder that code freeze for this release is on November 14. Is this issue still intended for 1.17?

@markyjackson-taulia

This comment has been minimized.

Copy link

@markyjackson-taulia markyjackson-taulia commented Nov 5, 2019

@jpbetz A kind reminder that code freeze for this release is on November 14. Is this issue still intended for 1.17? If no traction by weeks end we will have to move this to milestone 1.18

@jpbetz

This comment has been minimized.

Copy link
Contributor

@jpbetz jpbetz commented Nov 5, 2019

@markyjackson-taulia Trying to get #83520 in this week. It solves the "relist" problem that contributes to this issue but not the "restart" issue, so this issue will definitely not be fully resolved in 1.17. Targeting milestone 1.18 sounds good for now, for remaining work.

@markyjackson-taulia

This comment has been minimized.

Copy link

@markyjackson-taulia markyjackson-taulia commented Nov 5, 2019

@jpbetz ok, I am going to move to 1.18 milestone
/milestone v1.18

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.17, v1.18 Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.