Remove all "static" initializations

They are also flawed: we should not assume they are created once for all when flowcollector is created. E.g. of how it could go wrong: 1. an old version of an operator is installed, with a flowcollector 2. an upgrade is done, which introduces a new "static" resource => that new resource would not be created, since the FlowCollector object was already present hence did not trigger these "static init" things
netobserv · Mar 15, 2023 · 34f0fbb · 34f0fbb
1 parent 34b628a
commit 34f0fbb
Show file tree

Hide file tree

Showing 6 changed files with 40 additions and 84 deletions.
diff --git a/controllers/consoleplugin/consoleplugin_reconciler.go b/controllers/consoleplugin/consoleplugin_reconciler.go
@@ -64,18 +64,9 @@ func NewReconciler(cl reconcilers.ClientHelper, ns, prevNS, imageName string, av
 	return CPReconciler{ClientHelper: cl, nobjMngr: nobjMngr, owned: owned, image: imageName, availableAPIs: availableAPIs}
 }
 
-// InitStaticResources inits some "static" / one-shot resources, usually not subject to reconciliation
-func (r *CPReconciler) InitStaticResources(ctx context.Context) error {
-	cr := buildClusterRole()
-	return r.ReconcileClusterRole(ctx, cr)
-}
-
-// PrepareNamespaceChange cleans up old namespace and restore the relevant "static" resources
-func (r *CPReconciler) PrepareNamespaceChange(ctx context.Context) error {
-	// Switching namespace => delete everything in the previous namespace
+// CleanupNamespace cleans up old namespace
+func (r *CPReconciler) CleanupNamespace(ctx context.Context) {
 	r.nobjMngr.CleanupPreviousNamespace(ctx)
-	cr := buildClusterRole()
-	return r.ReconcileClusterRole(ctx, cr)
 }
 
 // Reconcile is the reconciler entry point to reconcile the current plugin state with the desired configuration
@@ -147,6 +138,11 @@ func (r *CPReconciler) reconcilePermissions(ctx context.Context, builder *builde
 		return r.CreateOwned(ctx, builder.serviceAccount())
 	} // update not needed for now
 
+	cr := buildClusterRole()
+	if err := r.ReconcileClusterRole(ctx, cr); err != nil {
+		return err
+	}
+
 	desired := builder.clusterRoleBinding()
 	if err := r.ReconcileClusterRoleBinding(ctx, desired); err != nil {
 		return err

diff --git a/controllers/flowcollector_controller.go b/controllers/flowcollector_controller.go
@@ -202,31 +202,12 @@ func (r *FlowCollectorReconciler) handleNamespaceChanged(
 	cpReconciler *consoleplugin.CPReconciler,
 ) error {
 	log := log.FromContext(ctx)
-	if oldNS == "" {
-		// First install: create one-shot resources
-		log.Info("FlowCollector first install: creating initial resources")
-		err := flpReconciler.InitStaticResources(ctx)
-		if err != nil {
-			return err
-		}
-		if r.availableAPIs.HasConsolePlugin() {
-			err := cpReconciler.InitStaticResources(ctx)
-			if err != nil {
-				return err
-			}
-		}
-	} else {
+	if oldNS != "" {
 		// Namespace updated, clean up previous namespace
-		log.Info("FlowCollector namespace change detected: cleaning up previous namespace and preparing next one", "old namespace", oldNS, "new namepace", newNS)
-		err := flpReconciler.PrepareNamespaceChange(ctx)
-		if err != nil {
-			return err
-		}
+		log.Info("FlowCollector namespace change detected: cleaning up previous namespace", "old namespace", oldNS, "new namepace", newNS)
+		flpReconciler.CleanupNamespace(ctx)
 		if r.availableAPIs.HasConsolePlugin() {
-			err := cpReconciler.PrepareNamespaceChange(ctx)
-			if err != nil {
-				return err
-			}
+			cpReconciler.CleanupNamespace(ctx)
 		}
 	}
 

diff --git a/controllers/flowlogspipeline/flp_ingest_reconciler.go b/controllers/flowlogspipeline/flp_ingest_reconciler.go
@@ -67,18 +67,9 @@ func (r *flpIngesterReconciler) context(ctx context.Context) context.Context {
 	return log.IntoContext(ctx, l)
 }
 
-// initStaticResources inits some "static" / one-shot resources, usually not subject to reconciliation
-func (r *flpIngesterReconciler) initStaticResources(ctx context.Context) error {
-	cr := buildClusterRoleIngester(r.useOpenShiftSCC)
-	return r.ReconcileClusterRole(ctx, cr)
-}
-
-// PrepareNamespaceChange cleans up old namespace and restore the relevant "static" resources
-func (r *flpIngesterReconciler) prepareNamespaceChange(ctx context.Context) error {
-	// Switching namespace => delete everything in the previous namespace
+// cleanupNamespace cleans up old namespace
+func (r *flpIngesterReconciler) cleanupNamespace(ctx context.Context) {
 	r.nobjMngr.CleanupPreviousNamespace(ctx)
-	cr := buildClusterRoleIngester(r.useOpenShiftSCC)
-	return r.ReconcileClusterRole(ctx, cr)
 }
 
 func (r *flpIngesterReconciler) reconcile(ctx context.Context, desired *flowslatest.FlowCollector) error {
@@ -176,6 +167,11 @@ func (r *flpIngesterReconciler) reconcilePermissions(ctx context.Context, builde
 		return r.CreateOwned(ctx, builder.serviceAccount())
 	} // We only configure name, update is not needed for now
 
+	cr := buildClusterRoleIngester(r.useOpenShiftSCC)
+	if err := r.ReconcileClusterRole(ctx, cr); err != nil {
+		return err
+	}
+
 	desired := builder.clusterRoleBinding()
 	if err := r.ClientHelper.ReconcileClusterRoleBinding(ctx, desired); err != nil {
 		return err

diff --git a/controllers/flowlogspipeline/flp_monolith_reconciler.go b/controllers/flowlogspipeline/flp_monolith_reconciler.go
@@ -70,17 +70,9 @@ func (r *flpMonolithReconciler) context(ctx context.Context) context.Context {
 	return log.IntoContext(ctx, l)
 }
 
-// initStaticResources inits some "static" / one-shot resources, usually not subject to reconciliation
-func (r *flpMonolithReconciler) initStaticResources(ctx context.Context) error {
-	// Nothing to do here: monolith FLP uses cluster roles defined by Transformer and Ingester reconcilers
-	return nil
-}
-
-// prepareNamespaceChange cleans up old namespace and restore the relevant "static" resources
-func (r *flpMonolithReconciler) prepareNamespaceChange(ctx context.Context) error {
-	// Switching namespace => delete everything in the previous namespace
+// cleanupNamespace cleans up old namespace
+func (r *flpMonolithReconciler) cleanupNamespace(ctx context.Context) {
 	r.nobjMngr.CleanupPreviousNamespace(ctx)
-	return nil
 }
 
 func (r *flpMonolithReconciler) reconcile(ctx context.Context, desired *flowslatest.FlowCollector) error {
@@ -183,6 +175,14 @@ func (r *flpMonolithReconciler) reconcilePermissions(ctx context.Context, builde
 		return r.CreateOwned(ctx, builder.serviceAccount())
 	} // We only configure name, update is not needed for now
 
+	cr := buildClusterRoleIngester(r.useOpenShiftSCC)
+	if err := r.ReconcileClusterRole(ctx, cr); err != nil {
+		return err
+	}
+	cr = buildClusterRoleTransformer()
+	if err := r.ReconcileClusterRole(ctx, cr); err != nil {
+		return err
+	}
 	// Monolith uses ingester + transformer cluster roles
 	for _, kind := range []ConfKind{ConfKafkaIngester, ConfKafkaTransformer} {
 		desired := builder.clusterRoleBinding(kind)

diff --git a/controllers/flowlogspipeline/flp_reconciler.go b/controllers/flowlogspipeline/flp_reconciler.go
@@ -3,6 +3,7 @@ package flowlogspipeline
 import (
 	"context"
 	"fmt"
+
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -25,8 +26,7 @@ const contextReconcilerName = "FLP kind"
 
 type singleReconciler interface {
 	context(ctx context.Context) context.Context
-	initStaticResources(ctx context.Context) error
-	prepareNamespaceChange(ctx context.Context) error
+	cleanupNamespace(ctx context.Context)
 	reconcile(ctx context.Context, desired *flowslatest.FlowCollector) error
 }
 
@@ -60,24 +60,11 @@ func NewReconciler(ctx context.Context, cl reconcilers.ClientHelper, ns, prevNS,
 	}
 }
 
-// InitStaticResources inits some "static" / one-shot resources, usually not subject to reconciliation
-func (r *FLPReconciler) InitStaticResources(ctx context.Context) error {
+// CleanupNamespace cleans up old namespace
+func (r *FLPReconciler) CleanupNamespace(ctx context.Context) {
 	for _, sr := range r.reconcilers {
-		if err := sr.initStaticResources(sr.context(ctx)); err != nil {
-			return err
-		}
+		sr.cleanupNamespace(sr.context(ctx))
 	}
-	return nil
-}
-
-// PrepareNamespaceChange cleans up old namespace and restore the relevant "static" resources
-func (r *FLPReconciler) PrepareNamespaceChange(ctx context.Context) error {
-	for _, sr := range r.reconcilers {
-		if err := sr.prepareNamespaceChange(sr.context(ctx)); err != nil {
-			return err
-		}
-	}
-	return nil
 }
 
 func validateDesired(desired *flpSpec) error {

diff --git a/controllers/flowlogspipeline/flp_transfo_reconciler.go b/controllers/flowlogspipeline/flp_transfo_reconciler.go
@@ -71,18 +71,9 @@ func (r *flpTransformerReconciler) context(ctx context.Context) context.Context
 	return log.IntoContext(ctx, l)
 }
 
-// initStaticResources inits some "static" / one-shot resources, usually not subject to reconciliation
-func (r *flpTransformerReconciler) initStaticResources(ctx context.Context) error {
-	cr := buildClusterRoleTransformer()
-	return r.ReconcileClusterRole(ctx, cr)
-}
-
-// prepareNamespaceChange cleans up old namespace and restore the relevant "static" resources
-func (r *flpTransformerReconciler) prepareNamespaceChange(ctx context.Context) error {
-	// Switching namespace => delete everything in the previous namespace
+// cleanupNamespace cleans up old namespace
+func (r *flpTransformerReconciler) cleanupNamespace(ctx context.Context) {
 	r.nobjMngr.CleanupPreviousNamespace(ctx)
-	cr := buildClusterRoleTransformer()
-	return r.ReconcileClusterRole(ctx, cr)
 }
 
 func (r *flpTransformerReconciler) reconcile(ctx context.Context, desired *flowslatest.FlowCollector) error {
@@ -207,6 +198,11 @@ func (r *flpTransformerReconciler) reconcilePermissions(ctx context.Context, bui
 		return r.CreateOwned(ctx, builder.serviceAccount())
 	} // We only configure name, update is not needed for now
 
+	cr := buildClusterRoleTransformer()
+	if err := r.ReconcileClusterRole(ctx, cr); err != nil {
+		return err
+	}
+
 	desired := builder.clusterRoleBinding()
 	if err := r.ReconcileClusterRoleBinding(ctx, desired); err != nil {
 		return err