Refactor to remove old reconcilers + tests refactoring

- Removing the "subReconcilers" in FLP controller, as FLP is now only deployed with Kafka ... - ... but still keep a disctinct pseudo-reconciler "in-process", which has FLP logic but is triggered from Agent controller - Big tests refactoring, started due to the reconciler removal and went further by factorizing some patterns like checking that a list of objects are deleted/created/owned/etc.
netobserv · Mar 11, 2024 · 5881bc4 · 5881bc4
1 parent e743bbe
commit 5881bc4
Show file tree

Hide file tree

Showing 39 changed files with 947 additions and 1,835 deletions.
diff --git a/apis/flowcollector/v1beta1/flowcollector_types.go b/apis/flowcollector/v1beta1/flowcollector_types.go
@@ -400,9 +400,7 @@ type FlowCollectorFLP struct {
 	//+kubebuilder:validation:Minimum=1025
 	//+kubebuilder:validation:Maximum=65535
 	//+kubebuilder:default:=2055
-	// Port of the flow collector (host port).
-	// By convention, some values are forbidden. It must be greater than 1024 and different from
-	// 4500, 4789 and 6081.
+	// [Deprecated (*)] Port of the flow collector (host port). It is not used anymore and will be removed in a future version.
 	Port int32 `json:"port,omitempty"`
 
 	//+kubebuilder:validation:Minimum=1

diff --git a/apis/flowcollector/v1beta2/flowcollector_types.go b/apis/flowcollector/v1beta2/flowcollector_types.go
@@ -892,9 +892,7 @@ type AdvancedProcessorConfig struct {
 	//+kubebuilder:validation:Maximum=65535
 	//+kubebuilder:default:=2055
 	//+optional
-	// Port of the flow collector (host port).
-	// By convention, some values are forbidden. It must be greater than 1024 and different from
-	// 4500, 4789 and 6081.
+	// [Deprecated (*)] Port of the flow collector (host port). It is not used anymore and will be removed in a future version.
 	Port *int32 `json:"port,omitempty"`
 
 	//+kubebuilder:validation:Minimum=1

diff --git a/bundle/manifests/flows.netobserv.io_flowcollectors.yaml b/bundle/manifests/flows.netobserv.io_flowcollectors.yaml
@@ -2674,9 +2674,9 @@ spec:
                     type: boolean
                   port:
                     default: 2055
-                    description: Port of the flow collector (host port). By convention,
-                      some values are forbidden. It must be greater than 1024 and
-                      different from 4500, 4789 and 6081.
+                    description: '[Deprecated (*)] Port of the flow collector (host
+                      port). It is not used anymore and will be removed in a future
+                      version.'
                     format: int32
                     maximum: 65535
                     minimum: 1025
@@ -4968,9 +4968,9 @@ spec:
                         type: integer
                       port:
                         default: 2055
-                        description: Port of the flow collector (host port). By convention,
-                          some values are forbidden. It must be greater than 1024
-                          and different from 4500, 4789 and 6081.
+                        description: '[Deprecated (*)] Port of the flow collector
+                          (host port). It is not used anymore and will be removed
+                          in a future version.'
                         format: int32
                         maximum: 65535
                         minimum: 1025

diff --git a/bundle/manifests/netobserv-operator.clusterserviceversion.yaml b/bundle/manifests/netobserv-operator.clusterserviceversion.yaml
@@ -1041,14 +1041,6 @@ spec:
           - list
           - update
           - watch
-        - apiGroups:
-          - security.openshift.io
-          resourceNames:
-          - hostnetwork
-          resources:
-          - securitycontextconstraints
-          verbs:
-          - use
         - apiGroups:
           - authentication.k8s.io
           resources:

diff --git a/config/crd/bases/flows.netobserv.io_flowcollectors.yaml b/config/crd/bases/flows.netobserv.io_flowcollectors.yaml
@@ -2661,9 +2661,9 @@ spec:
                     type: boolean
                   port:
                     default: 2055
-                    description: Port of the flow collector (host port). By convention,
-                      some values are forbidden. It must be greater than 1024 and
-                      different from 4500, 4789 and 6081.
+                    description: '[Deprecated (*)] Port of the flow collector (host
+                      port). It is not used anymore and will be removed in a future
+                      version.'
                     format: int32
                     maximum: 65535
                     minimum: 1025
@@ -4955,9 +4955,9 @@ spec:
                         type: integer
                       port:
                         default: 2055
-                        description: Port of the flow collector (host port). By convention,
-                          some values are forbidden. It must be greater than 1024
-                          and different from 4500, 4789 and 6081.
+                        description: '[Deprecated (*)] Port of the flow collector
+                          (host port). It is not used anymore and will be removed
+                          in a future version.'
                         format: int32
                         maximum: 65535
                         minimum: 1025

diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
@@ -199,11 +199,3 @@ rules:
   - list
   - update
   - watch
-- apiGroups:
-  - security.openshift.io
-  resourceNames:
-  - hostnetwork
-  resources:
-  - securitycontextconstraints
-  verbs:
-  - use
diff --git a/controllers/consoleplugin/consoleplugin_objects.go b/controllers/consoleplugin/consoleplugin_objects.go
@@ -9,7 +9,7 @@ import (
 
 	osv1alpha1 "github.com/openshift/api/console/v1alpha1"
 	monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
-	"gopkg.in/yaml.v2"
+	"gopkg.in/yaml.v3"
 	appsv1 "k8s.io/api/apps/v1"
 	ascv2 "k8s.io/api/autoscaling/v2"
 	corev1 "k8s.io/api/core/v1"

diff --git a/controllers/consoleplugin/consoleplugin_test.go b/controllers/consoleplugin/consoleplugin_test.go
@@ -330,57 +330,6 @@ func TestConfigMapContent(t *testing.T) {
 	assert.Equal(config.Frontend.Deduper.Merge, false)
 }
 
-func TestConfigMapError(t *testing.T) {
-	assert := assert.New(t)
-
-	agentSpec := flowslatest.FlowCollectorAgent{
-		Type: "eBPF",
-		EBPF: flowslatest.FlowCollectorEBPF{
-			Sampling: ptr.To(int32(1)),
-			Advanced: &flowslatest.AdvancedAgentConfig{
-				Env: map[string]string{
-					"DEDUPER_JUST_MARK": "invalid",
-				},
-			},
-		},
-	}
-	lokiSpec := flowslatest.FlowCollectorLoki{}
-	loki := helper.NewLokiConfig(&lokiSpec, "any")
-
-	// spec with invalid flag
-	spec := flowslatest.FlowCollectorSpec{
-		Agent:         agentSpec,
-		ConsolePlugin: getPluginConfig(),
-		Loki:          lokiSpec,
-	}
-	builder := newBuilder(testNamespace, testImage, &spec, &loki)
-	cm, _, err := builder.configMap()
-	assert.Nil(cm)
-	assert.NotNil(err)
-
-	// update to valid flags
-	agentSpec.EBPF.Advanced.Env = map[string]string{
-		"DEDUPER_JUST_MARK": "false",
-		"DEDUPER_MERGE":     "true",
-	}
-	spec = flowslatest.FlowCollectorSpec{
-		Agent:         agentSpec,
-		ConsolePlugin: getPluginConfig(),
-		Loki:          lokiSpec,
-	}
-	builder = newBuilder(testNamespace, testImage, &spec, &loki)
-	cm, _, err = builder.configMap()
-	assert.NotNil(cm)
-	assert.Nil(err)
-
-	// parse output config and check expected values
-	var config config.PluginConfig
-	err = yaml.Unmarshal([]byte(cm.Data["config.yaml"]), &config)
-	assert.Nil(err)
-	assert.Equal(config.Frontend.Deduper.Mark, false)
-	assert.Equal(config.Frontend.Deduper.Merge, true)
-}
-
 func TestServiceUpdateCheck(t *testing.T) {
 	assert := assert.New(t)
 	old := getServiceSpecs()

diff --git a/controllers/ebpf/agent_controller.go b/controllers/ebpf/agent_controller.go
@@ -33,8 +33,6 @@ const (
 	envExcludeInterfaces          = "EXCLUDE_INTERFACES"
 	envInterfaces                 = "INTERFACES"
 	envAgentIP                    = "AGENT_IP"
-	envFlowsTargetHost            = "FLOWS_TARGET_HOST"
-	envFlowsTargetPort            = "FLOWS_TARGET_PORT"
 	envSampling                   = "SAMPLING"
 	envExport                     = "EXPORT"
 	envKafkaBrokers               = "KAFKA_BROKERS"
@@ -143,13 +141,10 @@ func (c *AgentController) Reconcile(ctx context.Context, target *flowslatest.Flo
 		return fmt.Errorf("reconciling permissions: %w", err)
 	}
 
-	var inprocFLPInfo *flp.InProcessInfo
-	if helper.UseMergedAgentFLP(&target.Spec) {
-		// Direct-FLP mode
-		inprocFLPInfo, err = flp.ReconcileInProcess(ctx, c.Instance, target)
-		if err != nil {
-			return fmt.Errorf("reconciling in-process FLP: %w", err)
-		}
+	// Direct-FLP mode
+	inprocFLPInfo, err := flp.ReconcileInProcess(ctx, c.Instance, target, constants.EBPFAgentName)
+	if err != nil {
+		return fmt.Errorf("reconciling in-process FLP: %w", err)
 	}
 
 	desired, err := c.desired(ctx, target, inprocFLPInfo, rlog)
@@ -364,7 +359,6 @@ func (c *AgentController) envConfig(ctx context.Context, coll *flowslatest.FlowC
 			)
 		}
 	} else {
-		debugConfig := helper.GetAdvancedProcessorConfig(coll.Spec.Processor.Advanced)
 		config = append(config,
 			corev1.EnvVar{
 				Name:  envExport,
@@ -374,19 +368,6 @@ func (c *AgentController) envConfig(ctx context.Context, coll *flowslatest.FlowC
 				Name:  envFLPConfig,
 				Value: inprocFLPInfo.JSONConfig,
 			},
-			corev1.EnvVar{
-				Name: envFlowsTargetHost,
-				ValueFrom: &corev1.EnvVarSource{
-					FieldRef: &corev1.ObjectFieldSelector{
-						APIVersion: "v1",
-						FieldPath:  "status.hostIP",
-					},
-				},
-			},
-			corev1.EnvVar{
-				Name:  envFlowsTargetPort,
-				Value: strconv.Itoa(int(*debugConfig.Port)),
-			},
 		)
 	}
 	return config, nil

diff --git a/controllers/ebpf/internal/permissions/permissions.go b/controllers/ebpf/internal/permissions/permissions.go
@@ -74,23 +74,23 @@ func (c *Reconciler) reconcileNamespace(ctx context.Context) error {
 			},
 		},
 	}
-	if actual == nil && desired != nil {
+	if actual == nil {
 		rlog.Info("creating namespace")
 		return c.CreateOwned(ctx, desired)
 	}
-	if actual != nil && desired != nil {
-		// We noticed that audit labels are automatically removed
-		// in some configurations of K8s, so to avoid an infinite update loop, we just ignore
-		// it (if the user removes it manually, it's at their own risk)
-		if !helper.IsSubSet(actual.ObjectMeta.Labels,
-			map[string]string{
-				"app":                                constants.OperatorName,
-				"pod-security.kubernetes.io/enforce": "privileged",
-			}) {
-			rlog.Info("updating namespace")
-			return c.UpdateIfOwned(ctx, actual, desired)
-		}
+
+	// We noticed that audit labels are automatically removed
+	// in some configurations of K8s, so to avoid an infinite update loop, we just ignore
+	// it (if the user removes it manually, it's at their own risk)
+	if !helper.IsSubSet(actual.ObjectMeta.Labels,
+		map[string]string{
+			"app":                                constants.OperatorName,
+			"pod-security.kubernetes.io/enforce": "privileged",
+		}) {
+		rlog.Info("updating namespace")
+		return c.UpdateIfOwned(ctx, actual, desired)
 	}
+
 	rlog.Info("namespace is already reconciled. Doing nothing")
 	return nil
 }

diff --git a/controllers/flowcollector_controller_certificates_test.go b/controllers/flowcollector_controller_certificates_test.go
@@ -13,8 +13,6 @@ import (
 
 	flowslatest "github.com/netobserv/network-observability-operator/apis/flowcollector/v1beta2"
 	"github.com/netobserv/network-observability-operator/controllers/constants"
-	. "github.com/netobserv/network-observability-operator/controllers/controllerstest"
-	"github.com/netobserv/network-observability-operator/controllers/flp"
 	"github.com/netobserv/network-observability-operator/pkg/test"
 	"github.com/netobserv/network-observability-operator/pkg/watchers"
 )
@@ -33,7 +31,7 @@ func flowCollectorCertificatesSpecs() {
 		Name: "cluster",
 	}
 	flpKey := types.NamespacedName{
-		Name:      constants.FLPName + flp.FlpConfSuffix[flp.ConfKafkaTransformer],
+		Name:      constants.FLPName,
 		Namespace: operatorNamespace,
 	}
 	pluginKey := types.NamespacedName{
@@ -471,30 +469,11 @@ func flowCollectorCertificatesSpecs() {
 
 	Context("Checking CR ownership", func() {
 		It("Should be garbage collected", func() {
-			// Retrieve CR to get its UID
-			By("Getting the CR")
-			flowCR := getCR(crKey)
-
-			By("Expecting flowlogs-pipeline deployment to be garbage collected")
-			Eventually(func() interface{} {
-				d := appsv1.Deployment{}
-				_ = k8sClient.Get(ctx, flpKey, &d)
-				return &d
-			}, timeout, interval).Should(BeGarbageCollectedBy(flowCR))
-
-			By("Expecting agent daemonset to be garbage collected")
-			Eventually(func() interface{} {
-				d := appsv1.DaemonSet{}
-				_ = k8sClient.Get(ctx, agentKey, &d)
-				return &d
-			}, timeout, interval).Should(BeGarbageCollectedBy(flowCR))
-
-			By("Expecting console plugin deployment to be garbage collected")
-			Eventually(func() interface{} {
-				d := appsv1.Deployment{}
-				_ = k8sClient.Get(ctx, pluginKey, &d)
-				return &d
-			}, timeout, interval).Should(BeGarbageCollectedBy(flowCR))
+			expectOwnership(operatorNamespace,
+				test.Deployment(flpKey.Name),
+				test.Deployment(pluginKey.Name),
+			)
+			expectOwnership(agentKey.Namespace, test.DaemonSet(agentKey.Name))
 		})
 	})