Update TODO list, rename some files, fix fentry

Signed-off-by: Andre Fredette <[email protected]>

TODO.md

@@ -1,12 +1,113 @@
+# Intro
+
 The code has support for XDP, TCX, and Fentry programs in a BpfApplication.
 
 It's written so that Dave's load/attach split code should drop in pretty easily,
 but it's not using it yet.  I'm simulating the attachments by reloading the code
 for each attachment (like we do today).
 
+# Observation/Question
+Fentry and Fexit programs break the mold.
+
+Fentry and Fexit programs need to be loaded separately for each attach point, so
+the user must specify the BPF function name and attach point together for each
+attachment.  The user can then attach or detach that program later, but if the
+user wants to attach the same Fentry/Fexit program to a different attach point,
+the program must be loaded again with the new attach point.
+
+For other program types, the user can load a program, and then attach or detach
+the program to/from multiple attach points at any time after it has been loaded.
+
+Some differences that result from these requirements:
+- Each Fentry/Fexit attach point results in a unique bpf program ID (even if
+  they all use the same bpf function)
+- For other types, a given bpf program can have one bpf program ID (assigned
+  when it's loaded), plus multiple attach IDs (assigened when it is attached)
+- We don't need an attach ID for Fentry/Fexit programs.
+
+We need to do one of the following:
+- Not support the user modifying Fentry/Fexit attach points after the initial
+  BpfApplication load.
+- Load the program if they add an attach point (which would result in an
+  independent set of global data), and unload the program if they remove an
+  attachment.
+
+Yaml options:
+
+**Option 1:**  The current design uses a map indexed by the bpffunction name, so
+we can only list a given bpffunction name once followed by a list of attach
+points as shown below.  This represents Fentry/Fexit programs the same way as
+others, but they would need to behave differently as outlined above.
+
+```yaml
+programs:
+  tcx_stats:
+    type: TCX
+    tcx:
+      attach_points:
+        - interfaceselector:
+            primarynodeinterface: true
+          priority: 500
+          direction: ingress
+        - interfaceselector:
+            interfaces:
+              - eth1
+          priority: 100
+          direction: egress
+  test_fentry:
+    type: Fentry
+    fentry:
+      attach_points:
+        - function_name: do_unlinkat
+          attach: true
+        - function_name: tcp_connect
+          attach: false
+```
+
+**Options 2:**  Use a slice, and allow the same Fentry/Fexit functions to be
+included multiple times. The is more like the bpfman api, but potentially more
+cumbersome for Fentry/Fexit programs.
+
+```yaml
+  programs:
+    - type: TCX
+      tcx:
+        bpffunctionname: tcx_stats
+        attach_points:
+          - interfaceselector:
+              primarynodeinterface: true
+            priority: 500
+            direction: ingress
+          - interfaceselector:
+              interfaces:
+                - eth0
+            priority: 100
+            direction: egress
+            containers:
+              namespace: bpfman
+              pods:
+                matchLabels:
+                  name: bpfman-daemon
+              containernames:
+                - bpfman
+                - bpfman-agent
+    - type: Fentry
+      fentry:
+        bpffunctionname: tcx_stats
+        function_name: do_unlinkat
+        attach: true
+    - type: Fentry
+      fentry:
+        bpffunctionname: tcx_stats
+        function_name: tcp_connect
+        attach: true
+```
+
+# New Code
+
 The new code is mainly in these directories:
 
-Updated & working APIs:
+## Updated & working APIs:
 
 - apis/v1alpha1/fentryProgram_types.go
 - apis/v1alpha1/xdpProgram_types.go
@@ -16,27 +117,30 @@ Updated & working APIs:
 
 Note: the rest are partially updated.
 
-New Agent:
+## New Agent:
 
 - controllers/app-agent
 
-New Operator:
+## New Operator:
 
 - controllers/app-operator
 
 Note: I left the old bpfman-agent and bpfman-operator code unchanged (except as
 needed due to CRD changed).  It should work, but it's not being initialized when
 we run the operator.
 
-Testing:
+# Testing:
 
 - Unit tests for the agent and the operator
 - The following working samples:
     - config/samples/bpfman.io_v1alpha1_bpfapplication.yaml (XDP & TCX)
     - config/samples/bpfman.io_v1alpha1_fentry_bpfapplication.yaml (Fentry)
 
-TODO:
+# TODO:
+
+(In no particular order.)
 
+- Implement Fentry/Fexit solution.
 - Integrate with the new bpfman code with load/attach split (of course)
 - Create a bpf.o file with all the application types for both cluster and
   namespace scoped BpfApplicaitons.

config/samples/bpfman.io_v1alpha1_bpfapplication.yaml

@@ -9,7 +9,7 @@ spec:
   nodeselector: {}
   bytecode:
     image:
-      url: quay.io/bpfman-bytecode/go-app-counter:latest
+      url: quay.io/bpfman-bytecode/app-test:latest
   programs:
     # - type: Kprobe
     #   kprobe:
@@ -34,7 +34,7 @@ spec:
     tcx_stats:
       type: TCX
       tcx:
-        bpffunctionname: tcx_stats
+        bpffunctionname: tcx_next
         attach_points:
           - interfaceselector:
               primarynodeinterface: true
@@ -58,7 +58,7 @@ spec:
     xdp_stats:
       type: XDP
       xdp:
-        bpffunctionname: xdp_stats
+        bpffunctionname: xdp_pass
         attach_points:
           - interfaceselector:
               primarynodeinterface: true
@@ -73,4 +73,10 @@ spec:
                   name: bpfman-daemon
               containernames:
                 - bpfman
-                - bpfman-agent
+                - bpfman-agent
+    test_fentry:
+      type: Fentry
+      fentry:
+        bpffunctionname: fentry_test
+        function_name: do_unlinkat
+        attach: true    

controllers/app-agent/fentry-program.go → controllers/app-agent/cl-fentry-program.go

@@ -47,7 +47,7 @@ func (r *FentryProgramReconciler) getProgId() *uint32 {
 }
 
 func (r *FentryProgramReconciler) getProgType() internal.ProgramType {
-	return internal.Tc
+	return internal.Tracing
 }
 
 func (r *FentryProgramReconciler) getNode() *v1.Node {
@@ -74,8 +74,8 @@ func (r *FentryProgramReconciler) setAttachId(id *uint32) {
 	r.currentAttachPoint.AttachId = id
 }
 
-func (r *FentryProgramReconciler) setAttachStatus(status bpfmaniov1alpha1.BpfProgramConditionType) {
-	r.currentAttachPoint.AttachStatus = status
+func (r *FentryProgramReconciler) setAttachStatus(status bpfmaniov1alpha1.BpfProgramConditionType) bool {
+	return updateSimpleStatus(&r.currentAttachPoint.AttachStatus, status)
 }
 
 func (r *FentryProgramReconciler) getAttachStatus() bpfmaniov1alpha1.BpfProgramConditionType {
@@ -186,7 +186,7 @@ func (r *FentryProgramReconciler) processAttachInfo(ctx context.Context, mapOwne
 		"mapOwnerStatus", mapOwnerStatus)
 
 	// Get existing ebpf state from bpfman.
-	loadedBpfPrograms, err := bpfmanagentinternal.ListBpfmanPrograms(ctx, r.BpfmanClient, internal.Tc)
+	loadedBpfPrograms, err := bpfmanagentinternal.ListBpfmanPrograms(ctx, r.BpfmanClient, r.getProgType())
 	if err != nil {
 		r.Logger.Error(err, "failed to list loaded bpfman programs")
 		updateSimpleStatus(&r.currentProgramState.ProgramAttachStatus, bpfmaniov1alpha1.BpfProgCondAttachError)

controllers/app-agent/tcx-program.go → controllers/app-agent/cl-tcx-program.go

@@ -73,8 +73,8 @@ func (r *TcxProgramReconciler) setAttachId(id *uint32) {
 	r.currentAttachPoint.AttachId = id
 }
 
-func (r *TcxProgramReconciler) setAttachStatus(status bpfmaniov1alpha1.BpfProgramConditionType) {
-	r.currentAttachPoint.AttachStatus = status
+func (r *TcxProgramReconciler) setAttachStatus(status bpfmaniov1alpha1.BpfProgramConditionType) bool {
+	return updateSimpleStatus(&r.currentAttachPoint.AttachStatus, status)
 }
 
 func (r *TcxProgramReconciler) getAttachStatus() bpfmaniov1alpha1.BpfProgramConditionType {

controllers/app-agent/xdp-program.go → controllers/app-agent/cl-xdp-program.go

@@ -73,8 +73,8 @@ func (r *XdpProgramReconciler) setAttachId(id *uint32) {
 	r.currentAttachPoint.AttachId = id
 }
 
-func (r *XdpProgramReconciler) setAttachStatus(status bpfmaniov1alpha1.BpfProgramConditionType) {
-	r.currentAttachPoint.AttachStatus = status
+func (r *XdpProgramReconciler) setAttachStatus(status bpfmaniov1alpha1.BpfProgramConditionType) bool {
+	return updateSimpleStatus(&r.currentAttachPoint.AttachStatus, status)
 }
 
 func (r *XdpProgramReconciler) getAttachStatus() bpfmaniov1alpha1.BpfProgramConditionType {

controllers/app-agent/common.go

@@ -108,7 +108,7 @@ type ProgramReconciler interface {
 	getUUID() string
 	getAttachId() *uint32
 	setAttachId(id *uint32)
-	setAttachStatus(status bpfmaniov1alpha1.BpfProgramConditionType)
+	setAttachStatus(status bpfmaniov1alpha1.BpfProgramConditionType) bool
 	getAttachStatus() bpfmaniov1alpha1.BpfProgramConditionType
 }
 
@@ -447,68 +447,85 @@ func (r *ReconcilerCommon) reconcileBpfAttachment(
 			// Confirm it's in the correct state.
 			loadRequest, err := rec.getLoadRequest(mapOwnerStatus.mapOwnerId)
 			if err != nil {
-				rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondBytecodeSelectorError)
-				return false, err
-			}
-			isSame, reasons := bpfmanagentinternal.DoesProgExist(loadedBpfProgram, loadRequest)
-			if !isSame {
-				r.Logger.Info("Attachment is in wrong state, detach and re-attach", "reason", reasons, "Attach ID", rec.getAttachId())
-				if err := bpfmanagentinternal.UnloadBpfmanProgram(ctx, r.BpfmanClient, *rec.getAttachId()); err != nil {
-					rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondNotUnloaded)
-					r.Logger.Error(err, "Failed to detach eBPF Program")
-					return false, err
-				}
-
-				r.Logger.Info("Calling bpfman to attach eBPF Program")
-				attachId, err := bpfmanagentinternal.LoadBpfmanProgram(ctx, r.BpfmanClient, loadRequest)
-				if err != nil {
-					rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondNotLoaded)
-					r.Logger.Error(err, "Failed to attach eBPF Program")
+				r.Logger.Error(err, "Failed to get LoadRequest")
+				if rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondBytecodeSelectorError) {
 					return false, err
 				}
-				rec.setAttachId(attachId)
-				rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondAttached)
 			} else {
-				// Attachment exists and bpfProgram K8s Object is up to date
-				r.Logger.V(1).Info("Attachment is in correct state.  Nothing to do in bpfman")
-				rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondAttached)
+				r.Logger.Info("LoadRequest", "loadRequest", loadRequest)
+				isSame, reasons := bpfmanagentinternal.DoesProgExist(loadedBpfProgram, loadRequest)
+				if !isSame {
+					r.Logger.Info("Attachment is in wrong state, detach and re-attach", "reason", reasons, "Attach ID", rec.getAttachId())
+					err := bpfmanagentinternal.UnloadBpfmanProgram(ctx, r.BpfmanClient, *rec.getAttachId())
+					if err != nil {
+						r.Logger.Error(err, "Failed to detach eBPF Program")
+						if rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondNotUnloaded) {
+							return false, err
+						}
+					} else {
+						r.Logger.Info("Calling bpfman to attach eBPF Program")
+						attachId, err := bpfmanagentinternal.LoadBpfmanProgram(ctx, r.BpfmanClient, loadRequest)
+						if err != nil {
+							r.Logger.Error(err, "Failed to attach eBPF Program")
+							if rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondNotLoaded) {
+								return false, err
+
+							}
+						} else {
+							rec.setAttachId(attachId)
+							rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondAttached)
+						}
+					}
+				} else {
+					// Attachment exists and bpfProgram K8s Object is up to date
+					r.Logger.V(1).Info("Attachment is in correct state.  Nothing to do in bpfman")
+					rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondAttached)
+				}
 			}
 		case false:
 			// The program should be attached, but it isn't.
 			r.Logger.Info("Program is not attached, calling getLoadRequest()")
 			loadRequest, err := rec.getLoadRequest(mapOwnerStatus.mapOwnerId)
 			if err != nil {
-				rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondBytecodeSelectorError)
-				return false, err
-			}
-			r.Logger.Info("Calling bpfman to attach eBPF Program on node")
-			attachId, err := bpfmanagentinternal.LoadBpfmanProgram(ctx, r.BpfmanClient, loadRequest)
-			if err != nil {
-				rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondNotLoaded)
-				r.Logger.Error(err, "Failed to attach eBPF Program")
-				return false, err
+				r.Logger.Error(err, "Failed to get LoadRequest")
+				if rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondBytecodeSelectorError) {
+					return false, err
+				}
+			} else {
+				r.Logger.Info("LoadRequest", "loadRequest", loadRequest)
+				r.Logger.Info("Calling bpfman to attach eBPF Program on node")
+				attachId, err := bpfmanagentinternal.LoadBpfmanProgram(ctx, r.BpfmanClient, loadRequest)
+				if err != nil {
+					r.Logger.Error(err, "Failed to attach eBPF Program")
+					if rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondNotLoaded) {
+						return false, err
+					}
+				} else {
+					rec.setAttachId(attachId)
+					rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondAttached)
+				}
 			}
-			rec.setAttachId(attachId)
-			rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondAttached)
 		}
 	case false:
 		switch isAttached {
 		case true:
 			// The program is attached but it shouldn't be attached.  Unload it.
 			r.Logger.Info("Calling bpfman to detach eBPF Program", "Attach ID", rec.getAttachId())
 			if err := bpfmanagentinternal.UnloadBpfmanProgram(ctx, r.BpfmanClient, *rec.getAttachId()); err != nil {
-				rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondNotUnloaded)
 				r.Logger.Error(err, "Failed to detach eBPF Program")
-				return false, err
+				if rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondNotUnloaded) {
+					return false, err
+				}
+			} else {
+				rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondNotAttached)
 			}
-			rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondNotAttached)
 		case false:
 			// The program shouldn't be attached and it isn't.
 			rec.setAttachStatus(bpfmaniov1alpha1.BpfProgCondNotAttached)
 		}
 	}
-	// The BPF program was successfully reconciled.
 
+	// The BPF program was successfully reconciled.
 	remove := !rec.shouldAttach() && rec.getAttachStatus() == bpfmaniov1alpha1.BpfProgCondNotAttached
 	return remove, nil
 }