Merge pull request #114 from kluctl/fix-watch

codablock · web-flow · commit e9da8b674f7d · 2024-07-19T12:51:31.000+02:00
Fix permissions errors when creating watches
diff --git a/controllers/objecttemplate_controller.go b/controllers/objecttemplate_controller.go
@@ -32,6 +32,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/log"
@@ -193,11 +194,11 @@ func (r *ObjectTemplateReconciler) doReconcile(ctx context.Context, rt *template
 	newObjects := map[templatesv1alpha1.ObjectRef]struct{}{}
 	for _, me := range rt.Spec.Matrix {
 		if me.Object != nil {
-			newObjects[me.Object.Ref] = struct{}{}
-			err = wt.addWatchForObject(ctx, me.Object.Ref)
+			refWithNs, err := wt.addWatchForObject(ctx, me.Object.Ref)
 			if err != nil {
 				return err
 			}
+			newObjects[refWithNs] = struct{}{}
 		}
 	}
 	wt.removeDeletedWatches(ctx, newObjects)
@@ -234,11 +235,11 @@ func (r *ObjectTemplateReconciler) doReconcile(ctx context.Context, rt *template
 	}
 
 	for _, x := range allResources {
-		rm, err := r.Client.RESTMapper().RESTMapping(x.GroupVersionKind().GroupKind(), x.GroupVersionKind().Version)
+		isNs, err := apiutil.IsGVKNamespaced(x.GroupVersionKind(), objClient.RESTMapper())
 		if err != nil {
 			return err
 		}
-		if rm.Scope.Name() == apimeta.RESTScopeNameNamespace && x.GetNamespace() == "" {
+		if isNs && x.GetNamespace() == "" {
 			x.SetNamespace(rt.Namespace)
 		}
 	}
diff --git a/controllers/objecttemplate_controller_test.go b/controllers/objecttemplate_controller_test.go
@@ -153,10 +153,33 @@ var _ = Describe("ObjectTemplate controller", func() {
 		})
 		It("Should succeed after the SA is being created", func() {
 			createServiceAccount("non-existent", ns)
-			createRoleWithBinding("non-existent", ns, []string{"configmaps"})
+			createRoleWithBinding("non-existent", ns, []string{"configmaps", "secrets"})
 			triggerReconcile(key)
 			waitUntiReconciled(key, timeout)
-			assertAppliedConfigMaps(key, cmKey)
+			t2 := getObjectTemplate(key)
+			c := getReadyCondition(t2.GetConditions())
+			Expect(c.Status).To(Equal(metav1.ConditionTrue))
+		})
+		It("Should still succeed if the matrix input has no namespace", func() {
+			Expect(k8sClient.Create(ctx, buildTestSecret("m2", ns, map[string]string{
+				"k3": "3",
+			}))).To(Succeed())
+			updateObjectTemplate(key, func(t *templatesv1alpha1.ObjectTemplate) {
+				// we test the existing matrix entry to be changed
+				t.Spec.Matrix[0].Object.Ref.Namespace = ""
+				// and a new one being added
+				t.Spec.Matrix = append(t.Spec.Matrix, buildMatrixObjectEntry("m2", "m2", "", "Secret", "", false))
+				t.Spec.Templates[0].Object = buildTestConfigMap(cmKey.Name, cmKey.Namespace, map[string]string{
+					"k1": `{{ matrix.m1.data.k1 + matrix.m1.data.k2 + matrix.m2.data.k3 }}`,
+				})
+			})
+			waitUntiReconciled(key, timeout)
+			t2 := getObjectTemplate(key)
+			c := getReadyCondition(t2.GetConditions())
+			Expect(c.Status).To(Equal(metav1.ConditionTrue))
+			assertConfigMapData(cmKey, map[string]string{
+				"k1": "MQ==Mg==Mw==", // three base64 encoded strings got added
+			})
 		})
 		It("Should cleanup", func() {
 			Expect(k8sClient.Delete(ctx, t)).To(Succeed())
diff --git a/controllers/texttemplate_controller.go b/controllers/texttemplate_controller.go
@@ -129,29 +129,25 @@ func (r *TextTemplateReconciler) doReconcile(ctx context.Context, tt *templatesv
 	wt.setClient(ctx, objClient, tt.Spec.ServiceAccountName)
 	newObjects := map[templatesv1alpha1.ObjectRef]struct{}{}
 	if tt.Spec.TemplateRef != nil && tt.Spec.TemplateRef.ConfigMap != nil {
-		ns := tt.Spec.TemplateRef.ConfigMap.Namespace
-		if ns == "" {
-			ns = tt.Namespace
-		}
 		objRef := templatesv1alpha1.ObjectRef{
 			APIVersion: "v1",
 			Kind:       "ConfigMap",
-			Namespace:  ns,
+			Namespace:  tt.Spec.TemplateRef.ConfigMap.Namespace,
 			Name:       tt.Spec.TemplateRef.ConfigMap.Name,
 		}
-		err = wt.addWatchForObject(ctx, objRef)
+		refWithNs, err := wt.addWatchForObject(ctx, objRef)
 		if err != nil {
 			return err
 		}
-		newObjects[objRef] = struct{}{}
+		newObjects[refWithNs] = struct{}{}
 	}
 	for _, me := range tt.Spec.Inputs {
 		if me.Object != nil {
-			err = wt.addWatchForObject(ctx, me.Object.Ref)
+			refWithNs, err := wt.addWatchForObject(ctx, me.Object.Ref)
 			if err != nil {
 				return err
 			}
-			newObjects[me.Object.Ref] = struct{}{}
+			newObjects[refWithNs] = struct{}{}
 		}
 	}
 	wt.removeDeletedWatches(ctx, newObjects)
diff --git a/controllers/texttemplate_controller_test.go b/controllers/texttemplate_controller_test.go
@@ -103,6 +103,15 @@ var _ = Describe("TextTemplate controller", func() {
 			Expect(c.Status).To(Equal(metav1.ConditionTrue))
 			assertTextTemplateResult(key, "v1")
 		})
+		It("Should still succeed when the input object has no namespace", func() {
+			updateTextTemplate(key, func(t *templatesv1alpha1.TextTemplate) {
+				t.Spec.Inputs[0].Object.Ref.Namespace = ""
+			})
+			waitUntiTextTemplateReconciled(key, timeout)
+			t2 := getTextTemplate(key)
+			c := getReadyCondition(t2.GetConditions())
+			Expect(c.Status).To(Equal(metav1.ConditionTrue))
+		})
 		It("Should respect the jsonPath", func() {
 			updateTextTemplate(key, func(t *templatesv1alpha1.TextTemplate) {
 				p := ".data"
diff --git a/controllers/watches_util.go b/controllers/watches_util.go
@@ -12,6 +12,7 @@ import (
 	"k8s.io/apimachinery/pkg/watch"
 	"net/http"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
@@ -87,24 +88,33 @@ func (wt *watchesForTemplate) setClient(ctx context.Context, objClient client.Wi
 	wt.client = objClient
 }
 
-func (wt *watchesForTemplate) addWatchForObject(ctx context.Context, objectRef templatesv1alpha1.ObjectRef) error {
+func (wt *watchesForTemplate) addWatchForObject(ctx context.Context, objectRef templatesv1alpha1.ObjectRef) (templatesv1alpha1.ObjectRef, error) {
 	logger := log.FromContext(ctx)
 
+	gvk, err := objectRef.GroupVersionKind()
+	if err != nil {
+		return objectRef, err
+	}
+
+	// if namespace is not set, default to the template's namespace
+	if objectRef.Namespace == "" {
+		if isNs, err := apiutil.IsGVKNamespaced(gvk, wt.client.RESTMapper()); err != nil {
+			return objectRef, fmt.Errorf("failed to determine if object is namespaced: %w", err)
+		} else if isNs {
+			objectRef.Namespace = wt.templateKey.Namespace
+		}
+	}
+
 	wt.mutex.Lock()
 	defer wt.mutex.Unlock()
 
 	w := wt.watches[objectRef]
 	if w != nil {
-		return nil
+		return objectRef, nil
 	}
 
 	logger.V(1).Info("Starting watch for object", "templateKey", wt.templateKey, "objectRef", objectRef)
 
-	gvk, err := objectRef.GroupVersionKind()
-	if err != nil {
-		return err
-	}
-
 	// this is a single-object watch that does NOT require global watch permissions!
 	var dummy unstructured.UnstructuredList
 	dummy.SetGroupVersionKind(gvk)
@@ -120,7 +130,7 @@ func (wt *watchesForTemplate) addWatchForObject(ctx context.Context, objectRef t
 				err = fmt.Errorf("watch for %s \"%s\" is forbidden: %w", gvk.Kind, objectRef.Name, err)
 			}
 		}
-		return err
+		return objectRef, err
 	}
 	wt.watches[objectRef] = w
 
@@ -132,7 +142,7 @@ func (wt *watchesForTemplate) addWatchForObject(ctx context.Context, objectRef t
 		}
 	}()
 
-	return nil
+	return objectRef, nil
 }
 
 func (wt *watchesForTemplate) removeDeletedWatches(ctx context.Context, newRefs map[templatesv1alpha1.ObjectRef]struct{}) {

Original file line number	Diff line number	Diff line change
`@@ -32,6 +32,7 @@ import (`
`32`	`32`	`ctrl "sigs.k8s.io/controller-runtime"`
`33`	`33`	`"sigs.k8s.io/controller-runtime/pkg/builder"`
`34`	`34`	`"sigs.k8s.io/controller-runtime/pkg/client"`
	`35`	`+ "sigs.k8s.io/controller-runtime/pkg/client/apiutil"`
`35`	`36`	`"sigs.k8s.io/controller-runtime/pkg/controller"`
`36`	`37`	`"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"`
`37`	`38`	`"sigs.k8s.io/controller-runtime/pkg/log"`
`@@ -193,11 +194,11 @@ func (r ObjectTemplateReconciler) doReconcile(ctx context.Context, rt template`
`193`	`194`	`newObjects := map[templatesv1alpha1.ObjectRef]struct{}{}`
`194`	`195`	`for _, me := range rt.Spec.Matrix {`
`195`	`196`	`if me.Object != nil {`
`196`		`- newObjects[me.Object.Ref] = struct{}{}`
`197`		`- err = wt.addWatchForObject(ctx, me.Object.Ref)`
	`197`	`+ refWithNs, err := wt.addWatchForObject(ctx, me.Object.Ref)`
`198`	`198`	`if err != nil {`
`199`	`199`	`return err`
`200`	`200`	`}`
	`201`	`+ newObjects[refWithNs] = struct{}{}`
`201`	`202`	`}`
`202`	`203`	`}`
`203`	`204`	`wt.removeDeletedWatches(ctx, newObjects)`
`@@ -234,11 +235,11 @@ func (r ObjectTemplateReconciler) doReconcile(ctx context.Context, rt template`
`234`	`235`	`}`
`235`	`236`
`236`	`237`	`for _, x := range allResources {`
`237`		`- rm, err := r.Client.RESTMapper().RESTMapping(x.GroupVersionKind().GroupKind(), x.GroupVersionKind().Version)`
	`238`	`+ isNs, err := apiutil.IsGVKNamespaced(x.GroupVersionKind(), objClient.RESTMapper())`
`238`	`239`	`if err != nil {`
`239`	`240`	`return err`
`240`	`241`	`}`
`241`		`- if rm.Scope.Name() == apimeta.RESTScopeNameNamespace && x.GetNamespace() == "" {`
	`242`	`+ if isNs && x.GetNamespace() == "" {`
`242`	`243`	`x.SetNamespace(rt.Namespace)`
`243`	`244`	`}`
`244`	`245`	`}`