List and watch by namespace (#114)

Make sure that index and list operations respect namespaced resources --------- Co-authored-by: Mariano Uvalle <[email protected]>
Azure · Apr 30, 2024 · 64d7d96 · 64d7d96
1 parent 141b2f7
commit 64d7d96
Show file tree

Hide file tree

Showing 7 changed files with 133 additions and 56 deletions.
diff --git a/internal/controllers/aggregation/symphony.go b/internal/controllers/aggregation/symphony.go
@@ -37,7 +37,7 @@ func (c *symphonyController) Reconcile(ctx context.Context, req ctrl.Request) (c
 	logger = logger.WithValues("symphonyName", symph.Name, "symphonyNamespace", symph.Namespace)
 
 	existing := &apiv1.CompositionList{}
-	err = c.client.List(ctx, existing, client.MatchingFields{
+	err = c.client.List(ctx, existing, client.InNamespace(symph.Namespace), client.MatchingFields{
 		manager.IdxCompositionsBySymphony: symph.Name,
 	})
 	if err != nil {

diff --git a/internal/controllers/reconciliation/symphony_test.go b/internal/controllers/reconciliation/symphony_test.go
@@ -11,6 +11,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/util/retry"
+	ctrlcache "sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	apiv1 "github.com/Azure/eno/api/v1"
@@ -27,9 +28,12 @@ func TestSymphonyIntegration(t *testing.T) {
 	corev1.SchemeBuilder.AddToScheme(scheme)
 
 	ctx := testutil.NewContext(t)
-	mgr := testutil.NewManager(t)
+	mgr := testutil.NewManager(t, testutil.WithCompositionNamespace(ctrlcache.AllNamespaces))
 	upstream := mgr.GetClient()
 
+	// Create test namespace.
+	require.NoError(t, upstream.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test"}}))
+
 	// Register supporting controllers
 	require.NoError(t, rollout.NewController(mgr.Manager, time.Millisecond))
 	require.NoError(t, synthesis.NewStatusController(mgr.Manager))
@@ -74,9 +78,44 @@ func TestSymphonyIntegration(t *testing.T) {
 	symph.Spec.Variations = []apiv1.Variation{{Synthesizer: apiv1.SynthesizerRef{Name: syn.Name}}}
 	require.NoError(t, upstream.Create(ctx, symph))
 
+	// TODO: Extract the boilerplate for this test and create a dedicated
+	// scenario for ns isolation.
+	// Creating a second symphony with the same name in a separate namespace
+	// to ensure we handle namespace isolation correctly.
+	symph2 := &apiv1.Symphony{}
+	symph2.Name = "test-comp"
+	symph2.Namespace = "test"
+	symph2.Spec.Variations = []apiv1.Variation{{Synthesizer: apiv1.SynthesizerRef{Name: syn.Name}}}
+	require.NoError(t, upstream.Create(ctx, symph2))
+
 	testutil.Eventually(t, func() bool {
 		upstream.Get(ctx, client.ObjectKeyFromObject(symph), symph)
-		return symph.Status.Reconciled != nil && symph.Status.ObservedGeneration == symph.Generation
+		if symph.Status.Reconciled == nil || symph.Status.ObservedGeneration != symph.Generation {
+			return false
+		}
+
+		comps := &apiv1.CompositionList{}
+		upstream.List(ctx, comps, client.InNamespace(symph.Namespace))
+		return len(comps.Items) == 1
+	})
+
+	testutil.Eventually(t, func() bool {
+		upstream.Get(ctx, client.ObjectKeyFromObject(symph2), symph2)
+		if symph.Status.Reconciled == nil || symph.Status.ObservedGeneration != symph.Generation {
+			return false
+		}
+
+		comps := &apiv1.CompositionList{}
+		upstream.List(ctx, comps, client.InNamespace(symph2.Namespace))
+		return len(comps.Items) == 1
+	})
+
+	// Delet one of the symphonies
+	require.NoError(t, upstream.Delete(ctx, symph2))
+	testutil.Eventually(t, func() bool {
+		comps := &apiv1.CompositionList{}
+		upstream.List(ctx, comps)
+		return len(comps.Items) == 1
 	})
 
 	// Add another variation

diff --git a/internal/controllers/replication/symphony.go b/internal/controllers/replication/symphony.go
@@ -41,7 +41,7 @@ func (c *symphonyController) Reconcile(ctx context.Context, req ctrl.Request) (c
 	ctx = logr.NewContext(ctx, logger)
 
 	existing := &apiv1.CompositionList{}
-	err = c.client.List(ctx, existing, client.MatchingFields{
+	err = c.client.List(ctx, existing, client.InNamespace(symph.Namespace), client.MatchingFields{
 		manager.IdxCompositionsBySymphony: symph.Name,
 	})
 	if err != nil {

diff --git a/internal/controllers/synthesis/lifecycle.go b/internal/controllers/synthesis/lifecycle.go
@@ -70,7 +70,7 @@ func (c *podLifecycleController) Reconcile(ctx context.Context, req ctrl.Request
 	// Delete any unnecessary pods
 	pods := &corev1.PodList{}
 	err = c.client.List(ctx, pods, client.InNamespace(c.config.PodNamespace), client.MatchingFields{
-		manager.IdxPodsByComposition: comp.Name,
+		manager.IdxPodsByComposition: manager.PodByCompIdxValueFromComp(comp),
 	})
 	if err != nil {
 		return ctrl.Result{}, fmt.Errorf("listing pods: %w", err)

diff --git a/internal/manager/indices.go b/internal/manager/indices.go
@@ -0,0 +1,66 @@
+package manager
+
+import (
+	"context"
+
+	"github.com/go-logr/logr"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+)
+
+const (
+	IdxPodsByComposition           = ".podsByComposition"
+	IdxCompositionsBySynthesizer   = ".spec.synthesizer"
+	IdxCompositionsBySymphony      = ".compositionsBySymphony"
+	IdxResourceSlicesByComposition = ".resourceSlicesByComposition"
+
+	CompositionNameLabelKey      = "eno.azure.io/composition-name"
+	CompositionNamespaceLabelKey = "eno.azure.io/composition-namespace"
+)
+
+func PodReferencesComposition(pod *corev1.Pod) bool {
+	labels := pod.GetLabels()
+	if labels == nil || labels[CompositionNameLabelKey] == "" || labels[CompositionNamespaceLabelKey] == "" {
+		return false
+	}
+	return true
+}
+
+func PodToCompMapFunc(ctx context.Context, obj client.Object) []reconcile.Request {
+	pod, ok := obj.(*corev1.Pod)
+	if !ok {
+		logr.FromContextOrDiscard(ctx).V(0).Info("unexpected type given to podToCompMapFunc")
+		return nil
+	}
+	if !PodReferencesComposition(pod) {
+		return nil
+	}
+	return []reconcile.Request{{
+		NamespacedName: types.NamespacedName{
+			Name:      pod.GetLabels()[CompositionNameLabelKey],
+			Namespace: pod.GetLabels()[CompositionNamespaceLabelKey],
+		},
+	}}
+}
+
+func PodByCompIdxValueFromPod(po client.Object) string {
+	labels := po.GetLabels()
+	return labels[CompositionNameLabelKey] + "/" + labels[CompositionNamespaceLabelKey]
+}
+
+func PodByCompIdxValueFromComp(comp client.Object) string {
+	return comp.GetName() + "/" + comp.GetNamespace()
+}
+
+func indexController() client.IndexerFunc {
+	return func(o client.Object) []string {
+		owner := metav1.GetControllerOf(o)
+		if owner == nil {
+			return nil
+		}
+		return []string{owner.Name}
+	}
+}
diff --git a/internal/manager/manager.go b/internal/manager/manager.go
@@ -10,7 +10,6 @@ import (
 
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -40,16 +39,8 @@ func init() {
 // - The resource slices cached by the informer do not have the configured manifests since they are held by the reconstitution cache anyway
 
 const (
-	IdxPodsByComposition           = ".podsByComposition"
-	IdxCompositionsBySynthesizer   = ".spec.synthesizer"
-	IdxCompositionsBySymphony      = ".compositionsBySymphony"
-	IdxResourceSlicesByComposition = ".resourceSlicesByComposition"
-
 	ManagerLabelKey   = "app.kubernetes.io/managed-by"
 	ManagerLabelValue = "eno"
-
-	CompositionNameLabelKey      = "eno.azure.io/composition-name"
-	CompositionNamespaceLabelKey = "eno.azure.io/composition-namespace"
 )
 
 func init() {
@@ -117,12 +108,19 @@ func newMgr(logger logr.Logger, opts *Options, isController, isReconciler bool)
 	}
 
 	if isReconciler {
-		mgrOpts.Cache.ByObject[&apiv1.Composition{}] = cache.ByObject{
-			Namespaces: map[string]cache.Config{
-				opts.CompositionNamespace: {
-					LabelSelector: opts.CompositionSelector,
+		// TODO: Dedicated test for composition namespace isolation.
+		if opts.CompositionNamespace != cache.AllNamespaces {
+			mgrOpts.Cache.ByObject[&apiv1.Composition{}] = cache.ByObject{
+				Namespaces: map[string]cache.Config{
+					opts.CompositionNamespace: {
+						LabelSelector: opts.CompositionSelector,
+					},
 				},
-			},
+			}
+		} else {
+			mgrOpts.Cache.ByObject[&apiv1.Composition{}] = cache.ByObject{
+				Label: opts.CompositionSelector,
+			}
 		}
 
 		yespls := true
@@ -148,7 +146,7 @@ func newMgr(logger logr.Logger, opts *Options, isController, isReconciler bool)
 
 	if isController {
 		err = mgr.GetFieldIndexer().IndexField(context.Background(), &corev1.Pod{}, IdxPodsByComposition, func(o client.Object) []string {
-			return []string{o.GetLabels()[CompositionNameLabelKey]}
+			return []string{PodByCompIdxValueFromPod(o)}
 		})
 		if err != nil {
 			return nil, err
@@ -193,7 +191,7 @@ func NewLogConstructor(mgr ctrl.Manager, controllerName string) func(*reconcile.
 func NewCompositionToResourceSliceHandler(cli client.Client) handler.EventHandler {
 	apply := func(ctx context.Context, rli workqueue.RateLimitingInterface, obj client.Object) {
 		list := &apiv1.ResourceSliceList{}
-		err := cli.List(ctx, list, client.MatchingFields{
+		err := cli.List(ctx, list, client.InNamespace(obj.GetNamespace()), client.MatchingFields{
 			IdxResourceSlicesByComposition: obj.GetName(),
 		})
 		if err != nil {
@@ -216,38 +214,3 @@ func NewCompositionToResourceSliceHandler(cli client.Client) handler.EventHandle
 		},
 	}
 }
-
-func PodReferencesComposition(pod *corev1.Pod) bool {
-	labels := pod.GetLabels()
-	if labels == nil || labels[CompositionNameLabelKey] == "" || labels[CompositionNamespaceLabelKey] == "" {
-		return false
-	}
-	return true
-}
-
-func PodToCompMapFunc(ctx context.Context, obj client.Object) []reconcile.Request {
-	pod, ok := obj.(*corev1.Pod)
-	if !ok {
-		logr.FromContextOrDiscard(ctx).V(0).Info("unexpected type given to podToCompMapFunc")
-		return nil
-	}
-	if !PodReferencesComposition(pod) {
-		return nil
-	}
-	return []reconcile.Request{{
-		NamespacedName: types.NamespacedName{
-			Name:      pod.GetLabels()[CompositionNameLabelKey],
-			Namespace: pod.GetLabels()[CompositionNamespaceLabelKey],
-		},
-	}}
-}
-
-func indexController() client.IndexerFunc {
-	return func(o client.Object) []string {
-		owner := metav1.GetControllerOf(o)
-		if owner == nil {
-			return nil
-		}
-		return []string{owner.Name}
-	}
-}
diff --git a/internal/testutil/testutil.go b/internal/testutil/testutil.go
@@ -20,6 +20,7 @@ import (
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apimachinery/pkg/util/yaml"
@@ -106,6 +107,12 @@ func WithPodNamespace(ns string) TestManagerOption {
 	})
 }
 
+func WithCompositionNamespace(ns string) TestManagerOption {
+	return TestManagerOption(func(o *manager.Options) {
+		o.CompositionNamespace = ns
+	})
+}
+
 // NewManager starts one or two envtest environments depending on the env.
 // This should work seamlessly when run locally assuming binaries have been fetched with setup-envtest.
 // In CI the second environment is used to compatibility test against a matrix of k8s versions.
@@ -150,6 +157,8 @@ func NewManager(t *testing.T, testOpts ...TestManagerOption) *Manager {
 		HealthProbeAddr:         "127.0.0.1:0",
 		MetricsAddr:             "127.0.0.1:0",
 		SynthesizerPodNamespace: "default",
+		CompositionNamespace:    "default",
+		CompositionSelector:     labels.Everything(),
 	}
 	for _, o := range testOpts {
 		o.apply(options)