Don't set an ownerRef on secrets users are susceptible to copy around #3992

sebgl · 2020-11-30T14:26:55Z

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.

Additionally, garbage collection runs at operator startup to cover the case where
owners were deleted while the operator was down.

TODO:

Unit tests
E2E test
GC secrets on operator startup to cover the case where the operator is down during the owner deletion
Major bug to fix: the soft owner reference needs to include the object Kind, otherwise we may end up mismatching e.g. Elasticsearch stuff with Kibana stuff if they have the same name.

Fixes #3986.

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

barkbay

I just gave it a try, lgtm so far. I left a few nits and one question about the use of the UID instead of the object name.

pkg/controller/common/reconciler/secret.go

barkbay · 2020-12-01T08:49:21Z

pkg/controller/common/reconciler/secret.go

+	// - we remove the existing owner
+	// - we set additional labels to perform garbage collection on owner deletion (best-effort)
+	expected.Labels[SoftOwnerNamespaceLabel] = theoreticalOwner.GetNamespace()
+	expected.Labels[SoftOwnerNameLabel] = theoreticalOwner.GetName()


I'm wondering if it makes sense to rely on the owner's UID, and not on the object name, to be consistent with what is done by the K8S GC ? It would require for the object to have been stored first but I think it is a fair assumption ?

Suggested change

expected.Labels[SoftOwnerNameLabel] = theoreticalOwner.GetName()

expected.Labels[SoftOwnerUID] = string(theoreticalOwner.GetUID())

Hm I think the problem is that the garbage collection part of the code only knows about the deleted resource name + namespace (as part of the reconcile.Request). We don't know anything about the parent ID since it doesn't exist anymore? (Maybe I'm misunderstanding what you mean).

pkg/controller/elasticsearch/elasticsearch_controller.go

sebgl · 2020-12-01T13:42:06Z

Major bug to fix: the soft owner reference needs to include the object Kind, otherwise we may end up mismatching e.g. Elasticsearch stuff with Kibana stuff if they have the same name.

cmd/manager/main.go

pkg/apis/apm/v1/apmserver_types.go

sebgl · 2020-12-01T15:39:01Z

Current status: this PR is missing unit tests, but otherwise I don't expect any code change.

pebrc

LGTM, did a few simple tests that all seemed to work. Quirk: If you delete your es cluster while the operator is not running and recreate another cluster with the same name it adopts the left over resources from the previous incarnation which means password stays the same etc.

cmd/manager/main.go

cmd/manager/main_test.go

pkg/controller/common/certificates/reconcile.go

pkg/controller/common/reconciler/secret.go

sebgl · 2020-12-03T08:17:03Z

run full pr build

sebgl · 2020-12-03T11:21:32Z

We're missing a watch on soft-owned secrets to automatically trigger reconciliations on any change (e.g. deleting the elastic user secret).
I'll add another commit.

pebrc · 2020-12-03T11:38:34Z

We're missing a watch on soft-owned secrets to automatically trigger reconciliations on any change (e.g. deleting the elastic user secret).
I'll add another commit.

Good catch! I totally forgot that we are basing our watches on the owner reference :-(

pebrc

LGTM, so happy we have these e2e tests...

pkg/controller/common/watches/secrets_test.go

pebrc · 2020-12-03T14:21:56Z

Jenkins test this please

My bad still not fixed #3617

sebgl · 2020-12-03T15:12:08Z

run full pr build

…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

jberger · 2022-04-28T22:10:38Z

I guess I understand why this is done however it does mean that those generated resources do not show up in tools like Argo. When lots of resources are generated being visible from tooling is helpful so as to understand what has even been generated. Would there be any appetite to revisit this?

sebgl added >bug Something isn't working v1.3.1 labels Nov 30, 2020

sebgl mentioned this pull request Nov 30, 2020

Work around Kubernetes bug causing garbage collection across namespaces to happen when ownerReferences are set #3986

Closed

Error out the reconciliation on garbage collection errors

dd481f9

sebgl changed the title ~~Don't set an ownerRef on secrets users are susceptible to copy around~~ (wip) Don't set an ownerRef on secrets users are susceptible to copy around Nov 30, 2020

barkbay reviewed Dec 1, 2020

View reviewed changes

Improvements from PR review

8a0cc2a

sebgl added 4 commits December 1, 2020 15:55

Label soft-owned secrets with the owner Kind

5cb2add

Reuse Kind constants instead of hardcoded string

72964ad

Fix existing unit tests

a24f7d3

Garbage collect orphan soft-owned secrets on operator startup

86197dd

sebgl commented Dec 1, 2020

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

pkg/apis/apm/v1/apmserver_types.go Show resolved Hide resolved

Fix linter warnings

d1fb62c

sebgl added 2 commits December 2, 2020 15:18

Add unit tests

536407f

Cover soft owned secrets gc in e2e tests

78caaaa

sebgl changed the title ~~(wip) Don't set an ownerRef on secrets users are susceptible to copy around~~ Don't set an ownerRef on secrets users are susceptible to copy around Dec 2, 2020

Fix linter warning (typo)

c68ed2c

pebrc approved these changes Dec 2, 2020

View reviewed changes

Improvements from PR review

0c3f325

Trigger reconciliations on soft-owned secrets events

fb573f1

sebgl requested a review from pebrc December 3, 2020 12:01

pebrc approved these changes Dec 3, 2020

View reviewed changes

pkg/controller/common/watches/secrets_test.go Show resolved Hide resolved

pkg/controller/common/watches/secrets_test.go Outdated Show resolved Hide resolved

Fix linter stuff

4b514da

sebgl merged commit 34a1209 into elastic:master Dec 4, 2020

sebgl mentioned this pull request Dec 4, 2020

[1.3.1 Backport] Don't set an ownerRef on secrets users are susceptible to copy around… #4008

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't set an ownerRef on secrets users are susceptible to copy around #3992

Don't set an ownerRef on secrets users are susceptible to copy around #3992

sebgl commented Nov 30, 2020 •

edited

Loading

barkbay left a comment

barkbay Dec 1, 2020

sebgl Dec 1, 2020

sebgl commented Dec 1, 2020 •

edited

Loading

sebgl commented Dec 1, 2020

pebrc left a comment

sebgl commented Dec 3, 2020

sebgl commented Dec 3, 2020

pebrc commented Dec 3, 2020

pebrc left a comment

pebrc commented Dec 3, 2020

sebgl commented Dec 3, 2020

jberger commented Apr 28, 2022

	expected.Labels[SoftOwnerNameLabel] = theoreticalOwner.GetName()
	expected.Labels[SoftOwnerUID] = string(theoreticalOwner.GetUID())

Don't set an ownerRef on secrets users are susceptible to copy around #3992

Don't set an ownerRef on secrets users are susceptible to copy around #3992

Conversation

sebgl commented Nov 30, 2020 • edited Loading

barkbay left a comment

Choose a reason for hiding this comment

barkbay Dec 1, 2020

Choose a reason for hiding this comment

sebgl Dec 1, 2020

Choose a reason for hiding this comment

sebgl commented Dec 1, 2020 • edited Loading

sebgl commented Dec 1, 2020

pebrc left a comment

Choose a reason for hiding this comment

sebgl commented Dec 3, 2020

sebgl commented Dec 3, 2020

pebrc commented Dec 3, 2020

pebrc left a comment

Choose a reason for hiding this comment

pebrc commented Dec 3, 2020

sebgl commented Dec 3, 2020

jberger commented Apr 28, 2022

sebgl commented Nov 30, 2020 •

edited

Loading

sebgl commented Dec 1, 2020 •

edited

Loading