Garbage collector behavior on invalid ownerReferences for existing uids across namespaces and across kinds is non-deterministic #65200

caesarxuchao · 2018-06-18T21:54:35Z

If an object with a given uid is already in the garbage collector uid map, a child object created with an ownerReference pointing to that uid is not treated as having a non-existent parent, even if:

the ownerReference refers to a cluster-scoped kind and no cluster-scoped kind with that name exists
the child is namespaced, and the ownerReference refers to a namespaced kind and no namespace-scoped kind with that name exists in the same namespace
the child is cluster-scoped, and the ownerReference refers to a namespaced kind (since the namespace of the parent is ambiguous, it should be treated as not existing)

Original description follows

Forked from #63386 (comment).

Garbage collector should work for three cases: (a) cluster-scoped owner with namespaced dependents, (b) namespaced owner and namespaced dependents that are in the same namespace, and (c) cluster-scoped owner with cluster-scoped dependents.

Garbage collector should NOT work for the other two cases: (c) namespaced owner with cluster-scoped dependents. (d) owner and dependents that are in different namespaces. Today, GC sometimes work in these two cases. It's a bug for two reasons:

This weakens our security guarantees. Users might exploit this bug to grant other users delete permissions.
Users might rely on the unsupported behavior.

We can add extra checks in the GC controller to make it never work for case (c) and (d).

cc @lavalamp @liggitt @deads2k

tpepper · 2018-06-20T23:58:32Z

The discussion in #63386 is unclear to me. While that is 1.11 milestone material, is the content of this issue here a lower priority and deferred to the next milestone and perhaps later cherry-picking? Given the delay, I'd like to assume it is not critical-urgent, but rather than assume...

@kubernetes/sig-api-machinery-bugs can folks comment?

liggitt · 2018-06-21T00:17:28Z

This is longstanding and not a 1.11 blocker.

wenjiaswe · 2018-06-25T20:27:37Z

/cc @janetkuo @roycaihw

fejta-bot · 2018-09-23T20:37:16Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

caesarxuchao · 2018-09-24T16:57:35Z

/remove-lifecycle stale
/lifecycle frozen

roytman · 2019-09-05T11:31:10Z

I work with K8s 1.13 and observed that GC works when

(d) owner and dependents that are in different namespaces

If required, I can provide a simple test example.

liggitt · 2019-09-05T12:21:00Z

I work with K8s 1.13 and observed that GC works when

(d) owner and dependents that are in different namespaces

It does not work reliably. If you set up a relationship like that, then restart the controller manager, the dependent will usually be deleted (because the uid-indexed owner cache is not populated yet)

roytman · 2019-09-05T12:28:00Z

I mean that in my case the dependent is deleted without restart the controller manager. It is quite reproducible.

liggitt · 2019-09-05T12:32:13Z

I mean that in my case the dependent is deleted without restart the controller manager. It is quite reproducible.

That is the expected/desired behavior, but is racy, so sometimes the cross-namespace relationship does not result in deletion until restart. Because it is racy, environmental factors like load and performance (both of the API server and controller manager) can affect results.

roytman · 2019-09-05T12:37:23Z

I see, thanks

liggitt · 2019-10-17T18:03:23Z

xref #72192

tedyu · 2019-10-18T15:03:01Z

In GarbageCollector#attemptToDeleteItem :

		for _, dep := range deps {
			if dep.isDeletingDependents() {
...
				if _, err := gc.patch(item, patch, gc.unblockOwnerReferencesJSONMergePatch); err != nil {
					return err
				}
				break

I think we should continue with other dependents performing the unblocking (instead of breaking out of the loop).

liggitt · 2020-08-05T14:39:40Z

I'm working on a fix for this for the 1.20 timeframe

sayanchowdhury · 2020-10-24T11:14:58Z

👋🏽 Hey @liggitt ! I'm from the Bug Triage team. This issue has not been updated for a while, so I'd like to check on the status. The code freeze is starting November 12th (about 3 weeks from now) and while there is still plenty of time, we want to ensure that each PR has a chance to be merged on time.

As the issue is tagged for 1.20, is it still planned for this release?

liggitt · 2020-10-26T13:18:59Z

Yes, work on this is in progress for 1.20

liggitt · 2020-11-17T21:20:35Z

Fixed in 1.20 by #92743

A k8s bug (kubernetes/kubernetes#65200) may cause the k8s garbage collection to delete undesired resources in case users manually copy an operator-managed secret to another namespace. To avoid that situation, this commit ensures no ownerRef is set on a subset of managed secrets users are susceptible to copy around: - the elastic user password secret - elasticsearch public transport certs - elasticsearch, kibana, enterprise search, apm server public http certs Existing ownerReferences set with earlier ECK versions will be removed when reconciled. Since they do not have an ownerRef anymore, those secrets are not automatically deleted when the Elasticsearch resource is deleted. To work around that situation, the secret reconciliation logic adds an additional set of labels to the reconciled secrets that don't have an ownerRef specified. These labels reference the "soft" owner ("soft" as in handled through some custom code and not through k8s builtin garbage collection logic). Once a controller receives a deletion event for the resource it manages, it will automatically remove the soft-owned secrets. This is done as best-effort. Secrets will remain orphan if: - the operator is not running when the owner is deleted - an error happens while deleting the soft-owned secrets

…#3992) * Don't set an ownerRef on secrets users are susceptible to copy around A k8s bug (kubernetes/kubernetes#65200) may cause the k8s garbage collection to delete undesired resources in case users manually copy an operator-managed secret to another namespace. To avoid that situation, this commit ensures no ownerRef is set on a subset of managed secrets users are susceptible to copy around: - the elastic user password secret - elasticsearch public transport certs - elasticsearch, kibana, enterprise search, apm server public http certs Existing ownerReferences set with earlier ECK versions will be removed when reconciled. Since they do not have an ownerRef anymore, those secrets are not automatically deleted when the Elasticsearch resource is deleted. To work around that situation, the secret reconciliation logic adds an additional set of labels to the reconciled secrets that don't have an ownerRef specified. These labels reference the "soft" owner ("soft" as in handled through some custom code and not through k8s builtin garbage collection logic). Once a controller receives a deletion event for the resource it manages, it will automatically remove the soft-owned secrets. This is done as best-effort. Secrets will remain orphan if: - the operator is not running when the owner is deleted - an error happens while deleting the soft-owned secrets * Error out the reconciliation on garbage collection errors * Improvements from PR review * Label soft-owned secrets with the owner Kind * Reuse Kind constants instead of hardcoded string * Fix existing unit tests * Garbage collect orphan soft-owned secrets on operator startup * Fix linter warnings * Add unit tests * Cover soft owned secrets gc in e2e tests * Fix linter warning (typo) * Improvements from PR review * Trigger reconciliations on soft-owned secrets events * Fix linter stuff

…elastic#3992) * Don't set an ownerRef on secrets users are susceptible to copy around A k8s bug (kubernetes/kubernetes#65200) may cause the k8s garbage collection to delete undesired resources in case users manually copy an operator-managed secret to another namespace. To avoid that situation, this commit ensures no ownerRef is set on a subset of managed secrets users are susceptible to copy around: - the elastic user password secret - elasticsearch public transport certs - elasticsearch, kibana, enterprise search, apm server public http certs Existing ownerReferences set with earlier ECK versions will be removed when reconciled. Since they do not have an ownerRef anymore, those secrets are not automatically deleted when the Elasticsearch resource is deleted. To work around that situation, the secret reconciliation logic adds an additional set of labels to the reconciled secrets that don't have an ownerRef specified. These labels reference the "soft" owner ("soft" as in handled through some custom code and not through k8s builtin garbage collection logic). Once a controller receives a deletion event for the resource it manages, it will automatically remove the soft-owned secrets. This is done as best-effort. Secrets will remain orphan if: - the operator is not running when the owner is deleted - an error happens while deleting the soft-owned secrets * Error out the reconciliation on garbage collection errors * Improvements from PR review * Label soft-owned secrets with the owner Kind * Reuse Kind constants instead of hardcoded string * Fix existing unit tests * Garbage collect orphan soft-owned secrets on operator startup * Fix linter warnings * Add unit tests * Cover soft owned secrets gc in e2e tests * Fix linter warning (typo) * Improvements from PR review * Trigger reconciliations on soft-owned secrets events * Fix linter stuff

…#3992) (#4008) * Don't set an ownerRef on secrets users are susceptible to copy around A k8s bug (kubernetes/kubernetes#65200) may cause the k8s garbage collection to delete undesired resources in case users manually copy an operator-managed secret to another namespace. To avoid that situation, this commit ensures no ownerRef is set on a subset of managed secrets users are susceptible to copy around: - the elastic user password secret - elasticsearch public transport certs - elasticsearch, kibana, enterprise search, apm server public http certs Existing ownerReferences set with earlier ECK versions will be removed when reconciled. Since they do not have an ownerRef anymore, those secrets are not automatically deleted when the Elasticsearch resource is deleted. To work around that situation, the secret reconciliation logic adds an additional set of labels to the reconciled secrets that don't have an ownerRef specified. These labels reference the "soft" owner ("soft" as in handled through some custom code and not through k8s builtin garbage collection logic). Once a controller receives a deletion event for the resource it manages, it will automatically remove the soft-owned secrets. This is done as best-effort. Secrets will remain orphan if: - the operator is not running when the owner is deleted - an error happens while deleting the soft-owned secrets * Error out the reconciliation on garbage collection errors * Improvements from PR review * Label soft-owned secrets with the owner Kind * Reuse Kind constants instead of hardcoded string * Fix existing unit tests * Garbage collect orphan soft-owned secrets on operator startup * Fix linter warnings * Add unit tests * Cover soft owned secrets gc in e2e tests * Fix linter warning (typo) * Improvements from PR review * Trigger reconciliations on soft-owned secrets events * Fix linter stuff

caesarxuchao added kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jun 18, 2018

caesarxuchao mentioned this issue Jun 18, 2018

Let the garbage collector use json merge patch when SMP is not supported #63386

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 23, 2018

k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 24, 2018

ironcladlou mentioned this issue Sep 25, 2018

Handle delete events openshift/cluster-ingress-operator#29

Merged

caesarxuchao mentioned this issue Oct 24, 2018

Delete node lease if the corresponding node is deleted #70034

Merged

stepanstipl mentioned this issue May 24, 2019

GPII-3903: Fix garbage-collector cross-namespace issue gpii-ops/gpii-infra#409

Merged

liggitt added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Jun 12, 2019

roytman mentioned this issue Sep 9, 2019

Removing a Composable object, doesn't remove its created objects composable-operator/composable#19

Closed

liggitt added this to the v1.17 milestone Oct 6, 2019

liggitt mentioned this issue Oct 17, 2019

gc's behavior is not consistent when restarting kube-controller-manager #72192

Closed

liggitt changed the title ~~Garbage collector needs extra check to prevent cross-namespace cascading deletion~~ Garbage collector behavior on ownerReferences for existing uids across namespaces and across kinds is non-deterministic Oct 17, 2019

liggitt added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Oct 17, 2019

liggitt self-assigned this Oct 17, 2019

liggitt changed the title ~~Garbage collector behavior on ownerReferences for existing uids across namespaces and across kinds is non-deterministic~~ Garbage collector behavior on invalid ownerReferences for existing uids across namespaces and across kinds is non-deterministic Oct 17, 2019

k8s-ci-robot added this to the v1.20 milestone Jul 15, 2020

adrian555 mentioned this issue Aug 28, 2020

Operator reconciling and redeploying the kfdef without known reason kubeflow/kfctl#393

Open

mrajashree mentioned this issue Sep 2, 2020

Create an operator to backup and restore rancher rancher/backup-restore-operator#8

Closed

9 tasks

Madhu-1 mentioned this issue Sep 8, 2020

The rook-ceph-csi-config cm disappeared after host reboot rook/rook#6162

Closed

This was referenced Sep 28, 2020

Bug 1880926: Fix owner references for elasticsearch CR openshift/elasticsearch-operator#498

Merged

Bug 1880926: Fix owner references for ClusterLogging CR openshift/cluster-logging-operator#713

Merged

travis-minke-sap mentioned this issue Oct 27, 2020

kne trigger dispatcher deployment disappearing kyma-project/kyma#9758

Closed

mrobson mentioned this issue Oct 28, 2020

Trident objects removed by Kube Garbage Collector NetApp/trident#474

Closed

travis-minke-sap mentioned this issue Oct 29, 2020

Distributed KafkaChannel Uses Cross-Namespace OwnerReferences knative-extensions/eventing-kafka#152

Closed

liggitt mentioned this issue Nov 17, 2020

Fix GC uid races and handling of conflicting ownerReferences #92743

Merged

12 tasks

k8s-ci-robot closed this as completed in #92743 Nov 17, 2020

tumido mentioned this issue Nov 24, 2020

Ownerreferences no longer added to resources deployed through operator kubeflow/kfctl#460

Open

barkbay mentioned this issue Nov 26, 2020

File realm may not always be automatically reloaded elastic/cloud-on-k8s#3982

Open

sebgl mentioned this issue Nov 26, 2020

Work around Kubernetes bug causing garbage collection across namespaces to happen when ownerReferences are set elastic/cloud-on-k8s#3986

Closed

sebgl mentioned this issue Nov 30, 2020

Don't set an ownerRef on secrets users are susceptible to copy around elastic/cloud-on-k8s#3992

Merged

4 tasks

jrivers96 mentioned this issue Dec 16, 2020

[One of the Kafka Cluster re-created/re-deployed abnormally..Need Help] ... strimzi/strimzi-kafka-operator#3577

Closed

liggitt mentioned this issue Jan 27, 2021

kube-controller-manager: Garbage Collector is removing Resources with owner reference to Custom Resource #98471

Closed

baluchicken mentioned this issue Mar 30, 2021

Remove ownerref from cluster scoped clusterissuer banzaicloud/koperator#579

Merged

4 tasks

andylibrian mentioned this issue May 27, 2021

Gloo discovery creates invalid ownerReferences in upstream solo-io/gloo#4790

Open

liggitt mentioned this issue Jun 30, 2021

kubernetes should support cross namespace ownerrefrence #100577

Closed

astefanutti mentioned this issue Jul 29, 2021

Fix #2530: fix type and do not add cross-namespace owner references apache/camel-k#2535

Merged

mszostok mentioned this issue Aug 3, 2021

Periodic cluster integration tests fail on testing that proper policy was injected capactio/capact#420

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Garbage collector behavior on invalid ownerReferences for existing uids across namespaces and across kinds is non-deterministic #65200

Garbage collector behavior on invalid ownerReferences for existing uids across namespaces and across kinds is non-deterministic #65200

caesarxuchao commented Jun 18, 2018 •

edited by liggitt

tpepper commented Jun 20, 2018

liggitt commented Jun 21, 2018

wenjiaswe commented Jun 25, 2018

fejta-bot commented Sep 23, 2018

caesarxuchao commented Sep 24, 2018

roytman commented Sep 5, 2019

liggitt commented Sep 5, 2019

roytman commented Sep 5, 2019

liggitt commented Sep 5, 2019

roytman commented Sep 5, 2019

liggitt commented Oct 17, 2019

tedyu commented Oct 18, 2019

liggitt commented Aug 5, 2020

sayanchowdhury commented Oct 24, 2020

liggitt commented Oct 26, 2020

liggitt commented Nov 17, 2020

Garbage collector behavior on invalid ownerReferences for existing uids across namespaces and across kinds is non-deterministic #65200

Garbage collector behavior on invalid ownerReferences for existing uids across namespaces and across kinds is non-deterministic #65200

Comments

caesarxuchao commented Jun 18, 2018 • edited by liggitt

Original description follows

tpepper commented Jun 20, 2018

liggitt commented Jun 21, 2018

wenjiaswe commented Jun 25, 2018

fejta-bot commented Sep 23, 2018

caesarxuchao commented Sep 24, 2018

roytman commented Sep 5, 2019

liggitt commented Sep 5, 2019

roytman commented Sep 5, 2019

liggitt commented Sep 5, 2019

roytman commented Sep 5, 2019

liggitt commented Oct 17, 2019

tedyu commented Oct 18, 2019

liggitt commented Aug 5, 2020

sayanchowdhury commented Oct 24, 2020

liggitt commented Oct 26, 2020

liggitt commented Nov 17, 2020

caesarxuchao commented Jun 18, 2018 •

edited by liggitt