From 30ed9fb75c8ea76ea478a05a1869055b44025ea3 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Fri, 29 Apr 2022 13:51:58 +0530 Subject: [PATCH] verify canary spec before syncing Signed-off-by: Sanskar Jaiswal --- docs/gitbook/faq.md | 4 +- pkg/controller/controller.go | 32 +++++++ pkg/controller/controller_test.go | 107 +++++++++++++++++++++++ pkg/controller/events.go | 5 -- pkg/controller/scheduler_metrics.go | 7 -- pkg/controller/scheduler_metrics_test.go | 4 - 6 files changed, 141 insertions(+), 18 deletions(-) create mode 100644 pkg/controller/controller_test.go diff --git a/docs/gitbook/faq.md b/docs/gitbook/faq.md index 67879e90..4526e246 100644 --- a/docs/gitbook/faq.md +++ b/docs/gitbook/faq.md @@ -74,8 +74,8 @@ to finish, and disable it afterward (`skipAnalysis: false`). #### How to disable cross namespace references? -Flagger by default can access resources across namespaces (`AlertProivder` and `MetricProvider`). If you're in a multi-tenant enviornemnt -and wish to disable this, you can do so through the `no-cross-namespace-refs` flag. +Flagger by default can access resources across namespaces (`AlertProivder`, `MetricProvider` and Gloo `Upsteream`). +If you're in a multi-tenant environment and wish to disable this, you can do so through the `no-cross-namespace-refs` flag. ``` flagger \ diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index d4c97579..50fb63e2 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -253,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 @@ -315,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 diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go new file mode 100644 index 00000000..8e107d04 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/pkg/controller/events.go b/pkg/controller/events.go index a09011c7..0a83be56 100644 --- a/pkg/controller/events.go +++ b/pkg/controller/events.go @@ -109,11 +109,6 @@ func (c *Controller) alert(canary *flaggerv1.Canary, message string, metadata bo // determine alert provider namespace providerNamespace := canary.GetNamespace() if alert.ProviderRef.Namespace != canary.Namespace { - if c.noCrossNamespaceRefs { - c.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). - Errorf("can't access alert provider ref %s.%s, cross-namespace references are blocked", alert.ProviderRef.Name, alert.ProviderRef.Namespace) - return - } providerNamespace = alert.ProviderRef.Namespace } diff --git a/pkg/controller/scheduler_metrics.go b/pkg/controller/scheduler_metrics.go index 705e88a5..f0a7f2d7 100644 --- a/pkg/controller/scheduler_metrics.go +++ b/pkg/controller/scheduler_metrics.go @@ -55,9 +55,6 @@ func (c *Controller) checkMetricProviderAvailability(canary *flaggerv1.Canary) e if metric.TemplateRef != nil { namespace := canary.Namespace if metric.TemplateRef.Namespace != canary.Namespace { - if c.noCrossNamespaceRefs { - return fmt.Errorf("can't access metric template ref %s.%s, cross-namespace references are blocked", metric.TemplateRef.Name, metric.TemplateRef.Namespace) - } namespace = metric.TemplateRef.Namespace } @@ -242,10 +239,6 @@ func (c *Controller) runMetricChecks(canary *flaggerv1.Canary) bool { if metric.TemplateRef != nil { namespace := canary.Namespace if metric.TemplateRef.Namespace != canary.Namespace { - if c.noCrossNamespaceRefs { - c.recordEventErrorf(canary, "Metric template %s.%s error: cross-namespace references are blocked", metric.TemplateRef.Name, metric.TemplateRef.Namespace) - return false - } namespace = metric.TemplateRef.Namespace } diff --git a/pkg/controller/scheduler_metrics_test.go b/pkg/controller/scheduler_metrics_test.go index 163f5509..81ec50cc 100644 --- a/pkg/controller/scheduler_metrics_test.go +++ b/pkg/controller/scheduler_metrics_test.go @@ -65,9 +65,5 @@ func TestController_checkMetricProviderAvailability(t *testing.T) { Namespace: "default", } require.NoError(t, ctrl.checkMetricProviderAvailability(canary)) - - ctrl.noCrossNamespaceRefs = true - canary.Namespace = "test" - require.Error(t, ctrl.checkMetricProviderAvailability(canary)) }) }