chore(metrics-operator): refactor fetching resouce namespaces during …

…analysis (#2105)

Signed-off-by: odubajDT <[email protected]>

metrics-operator/api/v1alpha3/common.go

@@ -6,3 +6,15 @@ type ObjectReference struct {
 	// Namespace defines the namespace of the referenced object
 	Namespace string `json:"namespace,omitempty"`
 }
+
+func (o *ObjectReference) IsNamespaceSet() bool {
+	return o.Namespace != ""
+}
+
+func (o *ObjectReference) GetNamespace(defaultNamespace string) string {
+	if o.IsNamespaceSet() {
+		return o.Namespace
+	}
+
+	return defaultNamespace
+}

metrics-operator/api/v1alpha3/common_test.go

@@ -0,0 +1,27 @@
+package v1alpha3
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestObjectReference_IsNamespaceSet(t *testing.T) {
+	o := ObjectReference{}
+
+	require.False(t, o.IsNamespaceSet())
+
+	o.Namespace = "ns"
+
+	require.True(t, o.IsNamespaceSet())
+}
+
+func TestObjectReference_GetNamespace(t *testing.T) {
+	o := ObjectReference{}
+
+	require.Equal(t, "default", o.GetNamespace("default"))
+
+	o.Namespace = "ns"
+
+	require.Equal(t, "ns", o.GetNamespace("default"))
+}

metrics-operator/controllers/analysis/controller.go

@@ -41,7 +41,6 @@ type AnalysisReconciler struct {
 	Scheme     *runtime.Scheme
 	Log        logr.Logger
 	MaxWorkers int //maybe 2 or 4 as def
-	Namespace  string
 	NewWorkersPoolFactory
 	common.IAnalysisEvaluator
 }
@@ -73,14 +72,12 @@ func (a *AnalysisReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 	}
 
 	//find AnalysisDefinition to have the collection of Objectives
+	analysisDefNamespace := analysis.Spec.AnalysisDefinition.GetNamespace(analysis.Namespace)
 	analysisDef := &metricsapi.AnalysisDefinition{}
-	if analysis.Spec.AnalysisDefinition.Namespace == "" {
-		analysis.Spec.AnalysisDefinition.Namespace = a.Namespace
-	}
 	err := a.Client.Get(ctx,
 		types.NamespacedName{
 			Name:      analysis.Spec.AnalysisDefinition.Name,
-			Namespace: analysis.Spec.AnalysisDefinition.Namespace},
+			Namespace: analysisDefNamespace},
 		analysisDef,
 	)
 
@@ -89,7 +86,7 @@ func (a *AnalysisReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 			a.Log.Info(
 				fmt.Sprintf("AnalysisDefinition '%s' in namespace '%s' not found, requeue",
 					analysis.Spec.AnalysisDefinition.Name,
-					analysis.Spec.AnalysisDefinition.Namespace),
+					analysisDefNamespace),
 			)
 			return ctrl.Result{Requeue: true, RequeueAfter: 10 * time.Second}, nil
 		}
@@ -107,7 +104,7 @@ func (a *AnalysisReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 	}
 
 	//create multiple workers handling the Objectives
-	childCtx, wp := a.NewWorkersPoolFactory(ctx, analysis, todo, a.MaxWorkers, a.Client, a.Log, a.Namespace)
+	childCtx, wp := a.NewWorkersPoolFactory(ctx, analysis, todo, a.MaxWorkers, a.Client, a.Log, analysisDefNamespace)
 
 	res, err := wp.DispatchAndCollect(childCtx)
 	if err != nil {

metrics-operator/controllers/analysis/controller_test.go

@@ -25,6 +25,54 @@ func TestAnalysisReconciler_Reconcile_BasicControlLoop(t *testing.T) {
 
 	analysis, analysisDef, template, _ := getTestCRDs()
 
+	analysis2 := metricsapi.Analysis{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-analysis",
+			Namespace: "default",
+		},
+		Spec: metricsapi.AnalysisSpec{
+			Timeframe: metricsapi.Timeframe{
+				From: metav1.Time{
+					Time: time.Now(),
+				},
+				To: metav1.Time{
+					Time: time.Now(),
+				},
+			},
+			Args: map[string]string{
+				"good": "good",
+				"dot":  ".",
+			},
+			AnalysisDefinition: metricsapi.ObjectReference{
+				Name:      "my-analysis-def",
+				Namespace: "default2",
+			},
+		},
+	}
+
+	analysisDef2 := metricsapi.AnalysisDefinition{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-analysis-def",
+			Namespace: "default2",
+		},
+		Spec: metricsapi.AnalysisDefinitionSpec{
+			Objectives: []metricsapi.Objective{
+				{
+					AnalysisValueTemplateRef: metricsapi.ObjectReference{
+						Name:      "my-template",
+						Namespace: "default",
+					},
+					Weight:       1,
+					KeyObjective: false,
+				},
+			},
+			TotalScore: metricsapi.TotalScore{
+				PassPercentage:    0,
+				WarningPercentage: 0,
+			},
+		},
+	}
+
 	tests := []struct {
 		name    string
 		client  client.Client
@@ -55,6 +103,13 @@ func TestAnalysisReconciler_Reconcile_BasicControlLoop(t *testing.T) {
 			wantErr: false,
 			status:  &metricsapi.AnalysisStatus{Raw: "{\"objectiveResults\":null,\"totalScore\":0,\"maximumScore\":0,\"pass\":true,\"warning\":false}", Pass: true},
 			res:     metricstypes.AnalysisResult{Pass: true},
+		}, {
+			name:    "succeeded - analysis in different namespace, status updated",
+			client:  fake2.NewClient(&analysis2, &analysisDef2, &template),
+			want:    controllerruntime.Result{},
+			wantErr: false,
+			status:  &metricsapi.AnalysisStatus{Raw: "{\"objectiveResults\":null,\"totalScore\":0,\"maximumScore\":0,\"pass\":true,\"warning\":false}", Pass: true},
+			res:     metricstypes.AnalysisResult{Pass: true},
 		},
 	}
 

metrics-operator/controllers/analysis/provider_selector.go

@@ -54,44 +54,38 @@ func (ps ProvidersPool) DispatchToProviders(ctx context.Context, id int) {
 		default:
 			ps.log.Info("worker", "id:", id, "started  job:", j.AnalysisValueTemplateRef.Name)
 			templ := &metricsapi.AnalysisValueTemplate{}
-			if j.AnalysisValueTemplateRef.Namespace == "" {
-				j.AnalysisValueTemplateRef.Namespace = ps.Namespace
-			}
 			err := ps.Client.Get(ctx,
 				types.NamespacedName{
 					Name:      j.AnalysisValueTemplateRef.Name,
-					Namespace: j.AnalysisValueTemplateRef.Namespace},
+					Namespace: j.AnalysisValueTemplateRef.GetNamespace(ps.Namespace)},
 				templ,
 			)
 
 			if err != nil {
-				ps.log.Error(err, "Failed to get the correct Provider")
+				ps.log.Error(err, "Failed to get AnalysisValueTemplate")
 				ps.results <- metricsapi.ProviderResult{Objective: j.AnalysisValueTemplateRef, ErrMsg: err.Error()}
 				ps.cancel()
 				return
 			}
 
 			providerRef := &metricsapi.KeptnMetricsProvider{}
-			if templ.Spec.Provider.Namespace == "" {
-				templ.Spec.Provider.Namespace = ps.Namespace
-			}
 			err = ps.Client.Get(ctx,
 				types.NamespacedName{
 					Name:      templ.Spec.Provider.Name,
-					Namespace: templ.Spec.Provider.Namespace},
+					Namespace: templ.Spec.Provider.GetNamespace(ps.Namespace)},
 				providerRef,
 			)
 
 			if err != nil {
-				ps.log.Error(err, "Failed to get Provider")
+				ps.log.Error(err, "Failed to get KeptnMetricsProvider")
 				ps.results <- metricsapi.ProviderResult{Objective: j.AnalysisValueTemplateRef, ErrMsg: err.Error()}
 				ps.cancel()
 				return
 			}
 
 			templatedQuery, err := generateQuery(templ.Spec.Query, ps.Analysis.Spec.Args)
 			if err != nil {
-				ps.log.Error(err, "Failed to substitute args in templ")
+				ps.log.Error(err, "Failed to substitute args in AnalysisValueTemplate")
 				ps.results <- metricsapi.ProviderResult{Objective: j.AnalysisValueTemplateRef, ErrMsg: err.Error()}
 				ps.cancel()
 				return

metrics-operator/controllers/analysis/provider_selector_test.go

@@ -11,6 +11,7 @@ import (
 	metricstypes "github.com/keptn/lifecycle-toolkit/metrics-operator/controllers/common/analysis/types"
 	fake2 "github.com/keptn/lifecycle-toolkit/metrics-operator/controllers/common/fake"
 	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -75,28 +76,109 @@ func TestProvidersPool(t *testing.T) {
 
 	analysis, analysisDef, template, provider := getTestCRDs()
 
+	provider2 := metricsapi.KeptnMetricsProvider{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-provider",
+			Namespace: "default2",
+		},
+		Spec: metricsapi.KeptnMetricsProviderSpec{
+			Type:         "prometheus",
+			TargetServer: "localhost:2000",
+		},
+	}
+
+	template2 := metricsapi.AnalysisValueTemplate{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-template",
+			Namespace: "default",
+		},
+		Spec: metricsapi.AnalysisValueTemplateSpec{
+			Provider: metricsapi.ObjectReference{
+				Name: "my-provider",
+			},
+			Query: "this is a {{.good}} query{{.dot}}",
+		},
+	}
+
+	analysisDef2 := metricsapi.AnalysisDefinition{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-analysis-def",
+			Namespace: "default",
+		},
+		Spec: metricsapi.AnalysisDefinitionSpec{
+			Objectives: []metricsapi.Objective{
+				{
+					AnalysisValueTemplateRef: metricsapi.ObjectReference{
+						Name: "my-template",
+					},
+					Weight:       1,
+					KeyObjective: false,
+				},
+			},
+			TotalScore: metricsapi.TotalScore{
+				PassPercentage:    0,
+				WarningPercentage: 0,
+			},
+		},
+	}
+
+	template3 := metricsapi.AnalysisValueTemplate{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-template",
+			Namespace: "default2",
+		},
+		Spec: metricsapi.AnalysisValueTemplateSpec{
+			Provider: metricsapi.ObjectReference{
+				Name:      "my-provider",
+				Namespace: "default",
+			},
+			Query: "this is a {{.good}} query{{.dot}}",
+		},
+	}
+
 	provider.Spec.Type = "mock-provider"
+	provider2.Spec.Type = "mock-provider"
 
 	testCases := []struct {
 		name           string
 		expectedErr    string
 		mockClient     client.Client
+		analysisDef    metricsapi.AnalysisDefinition
 		providerResult *metricstypes.ProviderRequest
 	}{
 
 		{
 			name:        "MissingTemplate",
 			expectedErr: "analysisvaluetemplates.metrics.keptn.sh \"my-template\" not found",
+			analysisDef: analysisDef,
 			mockClient:  fake2.NewClient(&analysis, &analysisDef),
 		},
 		{
 			name:        "MissingProvider",
 			expectedErr: "keptnmetricsproviders.metrics.keptn.sh \"my-provider\" not found",
+			analysisDef: analysisDef,
 			mockClient:  fake2.NewClient(&analysis, &analysisDef, &template),
 		},
 		{
-			name:       "Success",
-			mockClient: fake2.NewClient(&analysis, &analysisDef, &template, &provider),
+			name:        "Success",
+			mockClient:  fake2.NewClient(&analysis, &analysisDef, &template, &provider),
+			analysisDef: analysisDef,
+			providerResult: &metricstypes.ProviderRequest{
+				Query: "this is a good query.",
+			},
+		},
+		{
+			name:        "Success - provider in same namespace",
+			mockClient:  fake2.NewClient(&analysis, &analysisDef, &template2, &provider2),
+			analysisDef: analysisDef,
+			providerResult: &metricstypes.ProviderRequest{
+				Query: "this is a good query.",
+			},
+		},
+		{
+			name:        "Success - analysisValueTemplate in same namespace",
+			mockClient:  fake2.NewClient(&analysis, &analysisDef2, &template3, &provider),
+			analysisDef: analysisDef2,
 			providerResult: &metricstypes.ProviderRequest{
 				Query: "this is a good query.",
 			},
@@ -118,9 +200,9 @@ func TestProvidersPool(t *testing.T) {
 				IObjectivesEvaluator: mockEvaluator,
 				Client:               tc.mockClient,
 				log:                  mockLogger,
-				Namespace:            "default",
+				Namespace:            "default2",
 				Objectives: map[int][]metricsapi.Objective{
-					1: analysisDef.Spec.Objectives,
+					1: tc.analysisDef.Spec.Objectives,
 				},
 				Analysis: &analysis,
 				results:  resultChan,

metrics-operator/controllers/common/analysis/objective_evaluator.go

@@ -63,7 +63,7 @@ func getValueFromMap(values map[string]v1alpha3.ProviderResult, key string) (flo
 }
 
 func ComputeKey(obj v1alpha3.ObjectReference) string {
-	if obj.Namespace == "" {
+	if !obj.IsNamespaceSet() {
 		return obj.Name
 	}
 	return obj.Name + "-" + obj.Namespace

metrics-operator/main.go

@@ -194,7 +194,6 @@ func main() {
 			Scheme:                mgr.GetScheme(),
 			Log:                   analysisLogger.V(env.AnalysisControllerLogLevel),
 			MaxWorkers:            2,
-			Namespace:             env.PodNamespace,
 			NewWorkersPoolFactory: analysiscontroller.NewWorkersPool,
 			IAnalysisEvaluator:    &analysisEval,
 		}).SetupWithManager(mgr); err != nil {

test/integration/analysis-controller/00-teststep-template.yaml

@@ -2,8 +2,6 @@ apiVersion: kuttl.dev/v1beta1
 kind: TestStep
 commands:
   - script: |
-      envsubst < mock-server.yaml  | kubectl apply -f -
-  #  substitutes current time and namespace, making sure they are changed to env var first
-  # to prevent bad files in case of a test interrupt
+      kubectl apply -f mock-server.yaml -n $NAMESPACE
   - script: |
-      envsubst < install.yaml | kubectl apply -f -
+      envsubst < install.yaml | kubectl apply -f - -n $NAMESPACE