Using discovery client to properly check for cluster-scoped resources #1319

zen-dog · 2020-01-30T14:15:39Z

Summary:

proper applying and re-applying of cluster-scoped resources in the apply task
ownership is NOT set for cluster-scoped resources to the Instance

Note: the second change has implications, because we would previously set ownerReference of all resources (even cluster-scoped ones like CRDs) to the Instance and though k8s GC docs clearly state that "Cross-namespace owner references are disallowed by design" it would still garbage collect them when the Instance was uninstalled. This is not the case anymore. Operators must utilize the cleanup plan to remove cluster-scoped resources.

Fixes #1288 #1265

Summary: - [x] proper applying and re-applying of cluster-scoped resources in the apply task - [x] ownership is **NOT** set for cluster-scoped resources to the `Instance` **Note**: the second change has implications, because we would previously set `ownerReference` of **all** resources (even cluster-scoped ones like CRDs) to the `Instance` and though k8s [GC docs](https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/) clearly state that _"Cross-namespace owner references are disallowed by design"_ it would still garbage collect them when the Instance was uninstalled. This is not the case anymore. Operators must utilize the `cleanup` plan to remove cluster-scoped resources. Fixes #1288 #1265

zen-dog · 2020-01-30T14:16:41Z

pkg/engine/helpers/object_key.go

@@ -0,0 +1,62 @@
+package helpers


I had to extract these methods into a separate package because of circular dependencies 😞

zen-dog · 2020-01-30T14:17:32Z

pkg/engine/renderer/enhancer.go

+			// 1) Namespace-scoped dependents can only specify owners in the same namespace, and owners that are cluster-scoped.
+			// 2) Cluster-scoped dependents can only specify cluster-scoped owners, but not namespace-scoped owners.
+			// More: https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/
+			if isNamespaced {


Here, we don't set ownerReference for cluster-scoped resources anymore.

zen-dog · 2020-01-30T14:23:14Z

cmd/manager/main.go

@@ -113,10 +114,17 @@ func main() {
 	}

 	log.Print("Setting up instance controller")
+	discoveryClient, err := utils.GetDiscoveryClient(mgr)


Lots of line changes deal with the fact that we wire the discoverClient through the workflow engine 🤷‍♂

zen-dog · 2020-01-30T14:24:29Z

pkg/test/utils/kubernetes.go

@@ -687,8 +691,34 @@ func FakeDiscoveryClient() discovery.DiscoveryInterface {
 				{
 					GroupVersion: corev1.SchemeGroupVersion.String(),
 					APIResources: []metav1.APIResource{
-						{Name: "pods", Namespaced: true, Kind: "Pod"},
-						{Name: "namespaces", Namespaced: false, Kind: "Namespace"},
+						{Name: "pod", Namespaced: true, Kind: "Pod"},


Since fake discovery client is now needed in many of the workflow package tests, I extended it with the types we use in tests.

zen-dog · 2020-01-30T15:57:42Z

Note, this PR does not cache the []APIResource list returned by the discovery client. This will be either a followup PR or (if simple enough) I might push it here later.

kensipe

This is similar to some work / thoughts on the matter. I have 1 thought for consideration and 1 reason to reject this...
consideration: We are now passing 2 Clients. would it make sense to have a struct of Client which is a struct of different Client types? I'm good either way.

reason for rejection:
the use of package names utils, util or helpers is strongly discouraged. I was hoping to get rid of the ones we have... we shouldn't add to that list of tech debt IMO.

kensipe · 2020-01-30T16:39:26Z

This is not the case anymore. Operators must utilize the cleanup plan to remove cluster-scoped resources.

We need this in our docs... good design choice IMO! 👏

zen-dog · 2020-01-30T19:07:59Z

consideration: We are now passing 2 Clients. would it make sense to have a struct of Client which is a struct of different Client types? I'm good either way.

They're two completely different interfaces. Conflating both would add to confusion IMHO

the use of package names utils, util or helpers is strongly discouraged. I was hoping to get rid of the ones we have... we shouldn't add to that list of tech debt IMO.

I couldn't agree more! I think I lost half an hour of my life trying to find a better name for it and...still failed 😆 It has to be either a new package or an existing one (but not task, enhancer or controller) that should hold the object_key.go because of circular dependencies. What do you suggest?

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 · 2020-01-31T11:47:25Z

@kensipe I've renamed helpers to resources, as it handles k8s resources. That sounds reasonable to my ears, if we're going to use the methods there from outside the engine package we can move it to a different place, but I think it's good there for now

Regarding the Clients: I think the way it is right now is ok, but with the next client we need we should consider a struct :)

Renamed the package as requested ;)

…#1319) Summary: - [x] proper applying and re-applying of cluster-scoped resources in the apply task - [x] ownership is **NOT** set for cluster-scoped resources to the `Instance` **Note**: the second change has implications, because we would previously set `ownerReference` of **all** resources (even cluster-scoped ones like CRDs) to the `Instance` and though k8s [GC docs](https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/) clearly state that _"Cross-namespace owner references are disallowed by design"_ it would still garbage collect them when the Instance was uninstalled. This is not the case anymore. Operators must utilize the `cleanup` plan to remove cluster-scoped resources. Fixes #1288 #1265 Co-authored-by: Andreas Neumann <aneumann@mesosphere.com> Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

…#1319) Summary: - [x] proper applying and re-applying of cluster-scoped resources in the apply task - [x] ownership is **NOT** set for cluster-scoped resources to the `Instance` **Note**: the second change has implications, because we would previously set `ownerReference` of **all** resources (even cluster-scoped ones like CRDs) to the `Instance` and though k8s [GC docs](https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/) clearly state that _"Cross-namespace owner references are disallowed by design"_ it would still garbage collect them when the Instance was uninstalled. This is not the case anymore. Operators must utilize the `cleanup` plan to remove cluster-scoped resources. Fixes #1288 #1265 Co-authored-by: Andreas Neumann <aneumann@mesosphere.com> Signed-off-by: Thomas Runyon <runyontr@gmail.com>

Summary: previously any error returned by the `DefaultEnhancer` was treated as fatal. It made sense because it would act deterministically on a prepared set of resources. However, since #1319 enhancer also uses discovery interface to determine whether or not given resource is namespaced. These API requests may fail and must be retried, hence the introduction of a transient error. Fixes #1413 Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

… errors (#1427) Summary: previously any error returned by the `Enhancer` was treated as fatal. It made sense because it would act deterministically on a prepared set of resources. However, since #1319 enhancer also uses the discovery interface to determine whether or not the given resource is namespaced. These API requests may fail and must be retried. With this fix, `DefaultEnhancer` clearly marks certain errors as fatal. Fixes #1413 Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog requested review from kensipe, alenkacz and runyontr January 30, 2020 14:15

zen-dog requested review from gerred, jbarrick-mesosphere and nfnt as code owners January 30, 2020 14:15

zen-dog commented Jan 30, 2020

View reviewed changes

Removed debug logging

7e76667

zen-dog commented Jan 30, 2020

View reviewed changes

kensipe previously requested changes Jan 30, 2020

View reviewed changes

Renamed helpers package to 'resource'

822d64f

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Merge branch 'master' into ad/cluster-scoped-resources

8e55c96

ANeumann82 approved these changes Feb 11, 2020

View reviewed changes

zen-dog merged commit ff85649 into master Feb 11, 2020

zen-dog deleted the ad/cluster-scoped-resources branch February 11, 2020 12:53

zen-dog mentioned this pull request Mar 17, 2020

Enhancer and HealthUtil now differentiate between fatal and transient errors #1427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using discovery client to properly check for cluster-scoped resources #1319

Using discovery client to properly check for cluster-scoped resources #1319

zen-dog commented Jan 30, 2020

zen-dog Jan 30, 2020

zen-dog Jan 30, 2020

zen-dog Jan 30, 2020

zen-dog Jan 30, 2020 •

edited

Loading

zen-dog commented Jan 30, 2020

kensipe left a comment •

edited

Loading

kensipe commented Jan 30, 2020

zen-dog commented Jan 30, 2020 •

edited

Loading

ANeumann82 commented Jan 31, 2020 •

edited

Loading

Using discovery client to properly check for cluster-scoped resources #1319

Using discovery client to properly check for cluster-scoped resources #1319

Conversation

zen-dog commented Jan 30, 2020

zen-dog Jan 30, 2020

Choose a reason for hiding this comment

zen-dog Jan 30, 2020

Choose a reason for hiding this comment

zen-dog Jan 30, 2020

Choose a reason for hiding this comment

zen-dog Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

zen-dog commented Jan 30, 2020

kensipe left a comment • edited Loading

Choose a reason for hiding this comment

kensipe commented Jan 30, 2020

zen-dog commented Jan 30, 2020 • edited Loading

ANeumann82 commented Jan 31, 2020 • edited Loading

zen-dog Jan 30, 2020 •

edited

Loading

kensipe left a comment •

edited

Loading

zen-dog commented Jan 30, 2020 •

edited

Loading

ANeumann82 commented Jan 31, 2020 •

edited

Loading