Merge pull request #1181 from aryan9600/no-cross-ns-refs

Add flag to disable cross namespace refs to Custom Resources

pkg/controller/controller.go

@@ -49,24 +49,25 @@ const controllerAgentName = "flagger"
 
 // Controller is managing the canary objects and schedules canary deployments
 type Controller struct {
-	kubeClient       kubernetes.Interface
-	flaggerClient    clientset.Interface
-	flaggerInformers Informers
-	flaggerSynced    cache.InformerSynced
-	flaggerWindow    time.Duration
-	workqueue        workqueue.RateLimitingInterface
-	eventRecorder    record.EventRecorder
-	logger           *zap.SugaredLogger
-	canaries         *sync.Map
-	jobs             map[string]CanaryJob
-	recorder         metrics.Recorder
-	notifier         notifier.Interface
-	canaryFactory    *canary.Factory
-	routerFactory    *router.Factory
-	observerFactory  *observers.Factory
-	meshProvider     string
-	eventWebhook     string
-	clusterName      string
+	kubeClient           kubernetes.Interface
+	flaggerClient        clientset.Interface
+	flaggerInformers     Informers
+	flaggerSynced        cache.InformerSynced
+	flaggerWindow        time.Duration
+	workqueue            workqueue.RateLimitingInterface
+	eventRecorder        record.EventRecorder
+	logger               *zap.SugaredLogger
+	canaries             *sync.Map
+	jobs                 map[string]CanaryJob
+	recorder             metrics.Recorder
+	notifier             notifier.Interface
+	canaryFactory        *canary.Factory
+	routerFactory        *router.Factory
+	observerFactory      *observers.Factory
+	meshProvider         string
+	eventWebhook         string
+	clusterName          string
+	noCrossNamespaceRefs bool
 }
 
 type Informers struct {
@@ -89,6 +90,7 @@ func NewController(
 	version string,
 	eventWebhook string,
 	clusterName string,
+	noCrossNamespaceRefs bool,
 ) *Controller {
 	logger.Debug("Creating event broadcaster")
 	flaggerscheme.AddToScheme(scheme.Scheme)
@@ -103,24 +105,25 @@ func NewController(
 	recorder.SetInfo(version, meshProvider)
 
 	ctrl := &Controller{
-		kubeClient:       kubeClient,
-		flaggerClient:    flaggerClient,
-		flaggerInformers: flaggerInformers,
-		flaggerSynced:    flaggerInformers.CanaryInformer.Informer().HasSynced,
-		workqueue:        workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), controllerAgentName),
-		eventRecorder:    eventRecorder,
-		logger:           logger,
-		canaries:         new(sync.Map),
-		jobs:             map[string]CanaryJob{},
-		flaggerWindow:    flaggerWindow,
-		observerFactory:  observerFactory,
-		recorder:         recorder,
-		notifier:         notifier,
-		canaryFactory:    canaryFactory,
-		routerFactory:    routerFactory,
-		meshProvider:     meshProvider,
-		eventWebhook:     eventWebhook,
-		clusterName:      clusterName,
+		kubeClient:           kubeClient,
+		flaggerClient:        flaggerClient,
+		flaggerInformers:     flaggerInformers,
+		flaggerSynced:        flaggerInformers.CanaryInformer.Informer().HasSynced,
+		workqueue:            workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), controllerAgentName),
+		eventRecorder:        eventRecorder,
+		logger:               logger,
+		canaries:             new(sync.Map),
+		jobs:                 map[string]CanaryJob{},
+		flaggerWindow:        flaggerWindow,
+		observerFactory:      observerFactory,
+		recorder:             recorder,
+		notifier:             notifier,
+		canaryFactory:        canaryFactory,
+		routerFactory:        routerFactory,
+		meshProvider:         meshProvider,
+		eventWebhook:         eventWebhook,
+		clusterName:          clusterName,
+		noCrossNamespaceRefs: noCrossNamespaceRefs,
 	}
 
 	flaggerInformers.CanaryInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
@@ -250,6 +253,10 @@ func (c *Controller) syncHandler(key string) error {
 		return nil
 	}
 
+	if err := c.verifyCanary(cd); err != nil {
+		return fmt.Errorf("invalid canary spec: %s", err)
+	}
+
 	// Finalize if canary has been marked for deletion and revert is desired
 	if cd.Spec.RevertOnDeletion && cd.ObjectMeta.DeletionTimestamp != nil {
 		// If finalizers have been previously removed proceed
@@ -312,6 +319,34 @@ func (c *Controller) enqueue(obj interface{}) {
 	c.workqueue.AddRateLimited(key)
 }
 
+func (c *Controller) verifyCanary(canary *flaggerv1.Canary) error {
+	if c.noCrossNamespaceRefs {
+		if err := verifyNoCrossNamespaceRefs(canary); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func verifyNoCrossNamespaceRefs(canary *flaggerv1.Canary) error {
+	if canary.Spec.UpstreamRef != nil && canary.Spec.UpstreamRef.Namespace != canary.Namespace {
+		return fmt.Errorf("can't access gloo upstream %s.%s, cross-namespace references are blocked", canary.Spec.UpstreamRef.Name, canary.Spec.UpstreamRef.Namespace)
+	}
+	if canary.Spec.Analysis != nil {
+		for _, metric := range canary.Spec.Analysis.Metrics {
+			if metric.TemplateRef != nil && metric.TemplateRef.Namespace != canary.Namespace {
+				return fmt.Errorf("can't access metric template %s.%s, cross-namespace references are blocked", metric.TemplateRef.Name, metric.TemplateRef.Namespace)
+			}
+		}
+		for _, alert := range canary.Spec.Analysis.Alerts {
+			if alert.ProviderRef.Namespace != canary.Namespace {
+				return fmt.Errorf("can't access alert provider %s.%s, cross-namespace references are blocked", alert.ProviderRef.Name, alert.ProviderRef.Namespace)
+			}
+		}
+	}
+	return nil
+}
+
 func checkCustomResourceType(obj interface{}, logger *zap.SugaredLogger) (flaggerv1.Canary, bool) {
 	var roll *flaggerv1.Canary
 	var ok bool

pkg/controller/controller_test.go

@@ -0,0 +1,107 @@
+package controller
+
+import (
+	"testing"
+
+	flaggerv1 "github.com/fluxcd/flagger/pkg/apis/flagger/v1beta1"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestController_verifyCanary(t *testing.T) {
+	tests := []struct {
+		name    string
+		canary  flaggerv1.Canary
+		wantErr bool
+	}{
+		{
+			name: "Gloo upstream in a different namespace should return an error",
+			canary: flaggerv1.Canary{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cd-1",
+					Namespace: "default",
+				},
+				Spec: flaggerv1.CanarySpec{
+					UpstreamRef: &flaggerv1.CrossNamespaceObjectReference{
+						Name:      "upstream",
+						Namespace: "test",
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "Gloo upstream in the same namespace is allowed",
+			canary: flaggerv1.Canary{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cd-1",
+					Namespace: "default",
+				},
+				Spec: flaggerv1.CanarySpec{
+					UpstreamRef: &flaggerv1.CrossNamespaceObjectReference{
+						Name:      "upstream",
+						Namespace: "default",
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "MetricTemplate in a different namespace should return an error",
+			canary: flaggerv1.Canary{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cd-1",
+					Namespace: "default",
+				},
+				Spec: flaggerv1.CanarySpec{
+					Analysis: &flaggerv1.CanaryAnalysis{
+						Metrics: []flaggerv1.CanaryMetric{
+							{
+								TemplateRef: &flaggerv1.CrossNamespaceObjectReference{
+									Name:      "mt-1",
+									Namespace: "test",
+								},
+							},
+						},
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "AlertProvider in a different namespace should return an error",
+			canary: flaggerv1.Canary{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cd-1",
+					Namespace: "default",
+				},
+				Spec: flaggerv1.CanarySpec{
+					Analysis: &flaggerv1.CanaryAnalysis{
+						Alerts: []flaggerv1.CanaryAlert{
+							{
+								ProviderRef: flaggerv1.CrossNamespaceObjectReference{
+									Name:      "ap-1",
+									Namespace: "test",
+								},
+							},
+						},
+					},
+				},
+			},
+			wantErr: true,
+		},
+	}
+
+	ctrl := &Controller{
+		noCrossNamespaceRefs: true,
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			err := ctrl.verifyCanary(&test.canary)
+			if test.wantErr {
+				require.Error(t, err)
+			}
+		})
+	}
+}

pkg/controller/events.go

@@ -108,7 +108,7 @@ func (c *Controller) alert(canary *flaggerv1.Canary, message string, metadata bo
 
 		// determine alert provider namespace
 		providerNamespace := canary.GetNamespace()
-		if alert.ProviderRef.Namespace != "" {
+		if alert.ProviderRef.Namespace != canary.Namespace {
 			providerNamespace = alert.ProviderRef.Namespace
 		}
 

pkg/controller/scheduler_daemonset_fixture_test.go

@@ -261,7 +261,7 @@ func newDaemonSetTestCanary() *flaggerv1.Canary {
 			Name:      "podinfo",
 		},
 		Spec: flaggerv1.CanarySpec{
-			TargetRef: flaggerv1.CrossNamespaceObjectReference{
+			TargetRef: flaggerv1.LocalObjectReference{
 				Name:       "podinfo",
 				APIVersion: "apps/v1",
 				Kind:       "DaemonSet",
@@ -318,7 +318,7 @@ func newDaemonSetTestCanaryAB() *flaggerv1.Canary {
 			Name:      "podinfo",
 		},
 		Spec: flaggerv1.CanarySpec{
-			TargetRef: flaggerv1.CrossNamespaceObjectReference{
+			TargetRef: flaggerv1.LocalObjectReference{
 				Name:       "podinfo",
 				APIVersion: "apps/v1",
 				Kind:       "DaemonSet",

pkg/controller/scheduler_deployment_fixture_test.go

@@ -289,12 +289,12 @@ func newDeploymentTestCanary() *flaggerv1.Canary {
 			Name:      "podinfo",
 		},
 		Spec: flaggerv1.CanarySpec{
-			TargetRef: flaggerv1.CrossNamespaceObjectReference{
+			TargetRef: flaggerv1.LocalObjectReference{
 				Name:       "podinfo",
 				APIVersion: "apps/v1",
 				Kind:       "Deployment",
 			},
-			AutoscalerRef: &flaggerv1.CrossNamespaceObjectReference{
+			AutoscalerRef: &flaggerv1.LocalObjectReference{
 				Name:       "podinfo",
 				APIVersion: "autoscaling/v2beta2",
 				Kind:       "HorizontalPodAutoscaler",
@@ -351,12 +351,12 @@ func newDeploymentTestCanaryAB() *flaggerv1.Canary {
 			Name:      "podinfo",
 		},
 		Spec: flaggerv1.CanarySpec{
-			TargetRef: flaggerv1.CrossNamespaceObjectReference{
+			TargetRef: flaggerv1.LocalObjectReference{
 				Name:       "podinfo",
 				APIVersion: "apps/v1",
 				Kind:       "Deployment",
 			},
-			AutoscalerRef: &flaggerv1.CrossNamespaceObjectReference{
+			AutoscalerRef: &flaggerv1.LocalObjectReference{
 				Name:       "podinfo",
 				APIVersion: "autoscaling/v2beta2",
 				Kind:       "HorizontalPodAutoscaler",

pkg/controller/scheduler_metrics.go

@@ -54,7 +54,7 @@ func (c *Controller) checkMetricProviderAvailability(canary *flaggerv1.Canary) e
 
 		if metric.TemplateRef != nil {
 			namespace := canary.Namespace
-			if metric.TemplateRef.Namespace != "" {
+			if metric.TemplateRef.Namespace != canary.Namespace {
 				namespace = metric.TemplateRef.Namespace
 			}
 
@@ -238,7 +238,7 @@ func (c *Controller) runMetricChecks(canary *flaggerv1.Canary) bool {
 	for _, metric := range canary.GetAnalysis().Metrics {
 		if metric.TemplateRef != nil {
 			namespace := canary.Namespace
-			if metric.TemplateRef.Namespace != "" {
+			if metric.TemplateRef.Namespace != canary.Namespace {
 				namespace = metric.TemplateRef.Namespace
 			}
 

pkg/controller/scheduler_svc_test.go

@@ -123,7 +123,7 @@ func newTestServiceCanary() *flaggerv1.Canary {
 			Name:      "podinfo",
 		},
 		Spec: flaggerv1.CanarySpec{
-			TargetRef: flaggerv1.CrossNamespaceObjectReference{
+			TargetRef: flaggerv1.LocalObjectReference{
 				Name:       "podinfo",
 				APIVersion: "core/v1",
 				Kind:       "Service",
@@ -161,7 +161,7 @@ func newTestServiceCanaryMaxWeight() *flaggerv1.Canary {
 			Name:      "podinfo",
 		},
 		Spec: flaggerv1.CanarySpec{
-			TargetRef: flaggerv1.CrossNamespaceObjectReference{
+			TargetRef: flaggerv1.LocalObjectReference{
 				Name:       "podinfo",
 				APIVersion: "core/v1",
 				Kind:       "Service",
@@ -199,7 +199,7 @@ func newTestServiceCanaryWithWeightsHappyCase() *flaggerv1.Canary {
 			Name:      "podinfo",
 		},
 		Spec: flaggerv1.CanarySpec{
-			TargetRef: flaggerv1.CrossNamespaceObjectReference{
+			TargetRef: flaggerv1.LocalObjectReference{
 				Name:       "podinfo",
 				APIVersion: "core/v1",
 				Kind:       "Service",
@@ -236,7 +236,7 @@ func newTestServiceCanaryWithWeightsOverflow() *flaggerv1.Canary {
 			Name:      "podinfo",
 		},
 		Spec: flaggerv1.CanarySpec{
-			TargetRef: flaggerv1.CrossNamespaceObjectReference{
+			TargetRef: flaggerv1.LocalObjectReference{
 				Name:       "podinfo",
 				APIVersion: "core/v1",
 				Kind:       "Service",