Skip to content

Commit

Permalink
usm: file registery: Introduce already activated callback
Browse files Browse the repository at this point in the history
To support the go-tls unique use-case, in which we need to run a callback everytime
a path id needs to be re-registered, we introduce a new callback option.

Currently, using a nop callback
  • Loading branch information
guyarb committed Aug 25, 2024
1 parent 4892289 commit 513fad5
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 14 deletions.
19 changes: 18 additions & 1 deletion pkg/network/usm/ebpf_gotls.go
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,9 @@ func (p *goTLSProgram) AttachPID(pid uint32) error {

// Check go process
probeList := make([]manager.ProbeIdentificationPair, 0)
return p.registry.Register(binPath, pid, registerCBCreator(p.manager, p.offsetsDataMap, &probeList, p.binAnalysisMetric, p.binNoSymbolsMetric), unregisterCBCreator(p.manager, &probeList, p.offsetsDataMap))
return p.registry.Register(binPath, pid, registerCBCreator(p.manager, p.offsetsDataMap, &probeList, p.binAnalysisMetric, p.binNoSymbolsMetric),
unregisterCBCreator(p.manager, &probeList, p.offsetsDataMap),
utils.NopCallback)
}

func registerCBCreator(mgr *manager.Manager, offsetsDataMap *ebpf.Map, probeIDs *[]manager.ProbeIdentificationPair, binAnalysisMetric, binNoSymbolsMetric *libtelemetry.Counter) func(path utils.FilePath) error {
Expand Down Expand Up @@ -383,6 +385,21 @@ func registerCBCreator(mgr *manager.Manager, offsetsDataMap *ebpf.Map, probeIDs
}
}

// alreadyCBCreator handles the case where a binary is already registered. In such a case the registry callback won't
// be called, so we need to add a mapping from the PID to the device/inode of the binary.
func alreadyCBCreator(pidToDeviceInodeMap *ebpf.Map) func(utils.FilePath) error {
return func(filePath utils.FilePath) error {
if filePath.PID == 0 {
return nil
}
return pidToDeviceInodeMap.Put(unsafe.Pointer(&filePath.PID), unsafe.Pointer(&gotls.TlsBinaryId{
Id_major: unix.Major(filePath.ID.Dev),
Id_minor: unix.Minor(filePath.ID.Dev),
Ino: filePath.ID.Inode,
}))
}
}

func (p *goTLSProgram) handleProcessExit(pid pid) {
_ = p.DetachPID(pid)
}
Expand Down
1 change: 1 addition & 0 deletions pkg/network/usm/istio.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ func (m *istioMonitor) AttachPID(pid uint32) error {
pid,
m.registerCB,
m.unregisterCB,
utils.NopCallback,
)
}

Expand Down
1 change: 1 addition & 0 deletions pkg/network/usm/nodejs.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ func (m *nodeJSMonitor) AttachPID(pid uint32) error {
pid,
m.registerCB,
m.unregisterCB,
utils.NopCallback,
)
}

Expand Down
6 changes: 3 additions & 3 deletions pkg/network/usm/sharedlibraries/watcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ func (w *Watcher) AttachPID(pid uint32) error {
// Iterate over the rule, and look for a match.
for _, r := range w.rules {
if r.Re.MatchString(path) {
if err := w.registry.Register(path, pid, r.RegisterCB, r.UnregisterCB); err != nil {
if err := w.registry.Register(path, pid, r.RegisterCB, r.UnregisterCB, utils.NopCallback); err != nil {
registerErrors = append(registerErrors, err)
} else {
successfulMatches = append(successfulMatches, path)
Expand Down Expand Up @@ -210,7 +210,7 @@ func (w *Watcher) Start() {
// Iterate over the rule, and look for a match.
for _, r := range w.rules {
if r.Re.MatchString(path) {
_ = w.registry.Register(path, uint32(pid), r.RegisterCB, r.UnregisterCB)
_ = w.registry.Register(path, uint32(pid), r.RegisterCB, r.UnregisterCB, utils.NopCallback)
break
}
}
Expand Down Expand Up @@ -267,7 +267,7 @@ func (w *Watcher) Start() {
for _, r := range w.rules {
if r.Re.Match(path) {
w.libMatches.Add(1)
_ = w.registry.Register(string(path), lib.Pid, r.RegisterCB, r.UnregisterCB)
_ = w.registry.Register(string(path), lib.Pid, r.RegisterCB, r.UnregisterCB, utils.NopCallback)
break
}
}
Expand Down
10 changes: 9 additions & 1 deletion pkg/network/usm/utils/file_registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ func NewFilePath(procRoot, namespacedPath string, pid uint32) (FilePath, error)

type callback func(FilePath) error

var (
// NopCallback is an empty callback implementation, useful for testing and code paths the does not need all callbacks.
NopCallback = callback(nil)
)

// NewFileRegistry creates a new `FileRegistry` instance
func NewFileRegistry(programName string) *FileRegistry {
blocklistByID, err := simplelru.NewLRU[PathIdentifier, string](2000, nil)
Expand Down Expand Up @@ -118,7 +123,7 @@ var (
// If no current registration exists for the given `PathIdentifier`, we execute
// its *activation* callback. Otherwise, we increment the reference counter for
// the existing registration if and only if `pid` is new;
func (r *FileRegistry) Register(namespacedPath string, pid uint32, activationCB, deactivationCB callback) error {
func (r *FileRegistry) Register(namespacedPath string, pid uint32, activationCB, deactivationCB, alreadyRegistered callback) error {
if activationCB == nil || deactivationCB == nil {
return errCallbackIsMissing
}
Expand Down Expand Up @@ -152,6 +157,9 @@ func (r *FileRegistry) Register(namespacedPath string, pid uint32, activationCB,
r.byPID[pid][pathID] = struct{}{}
}
r.telemetry.fileAlreadyRegistered.Add(1)
if alreadyRegistered != nil {
_ = alreadyRegistered(path)
}
return ErrPathIsAlreadyRegistered
}

Expand Down
24 changes: 15 additions & 9 deletions pkg/network/usm/utils/file_registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func TestRegister(t *testing.T) {
pid := uint32(cmd.Process.Pid)

r := newFileRegistry()
require.NoError(t, r.Register(path, pid, registerRecorder.Callback(), IgnoreCB))
require.NoError(t, r.Register(path, pid, registerRecorder.Callback(), IgnoreCB, NopCallback))

assert.Equal(t, 1, registerRecorder.CallsForPathID(pathID))
assert.Contains(t, r.GetRegisteredProcesses(), pid)
Expand All @@ -44,6 +44,9 @@ func TestMultiplePIDsSharingSameFile(t *testing.T) {
unregisterRecorder := new(CallbackRecorder)
unregisterCallback := unregisterRecorder.Callback()

alreadyRegisteredRecorder := new(CallbackRecorder)
alreadyRegisteredCallback := alreadyRegisteredRecorder.Callback()

r := newFileRegistry()
path, pathID := createTempTestFile(t, "foobar")

Expand All @@ -56,8 +59,8 @@ func TestMultiplePIDsSharingSameFile(t *testing.T) {
pid2 := uint32(cmd2.Process.Pid)

// Trying to register the same file twice from different PIDs
require.NoError(t, r.Register(path, pid1, registerCallback, unregisterCallback))
require.Equal(t, ErrPathIsAlreadyRegistered, r.Register(path, pid2, registerCallback, unregisterCallback))
require.NoError(t, r.Register(path, pid1, registerCallback, unregisterCallback, alreadyRegisteredCallback))
require.Equal(t, ErrPathIsAlreadyRegistered, r.Register(path, pid2, registerCallback, unregisterCallback, alreadyRegisteredCallback))

// Assert that the callback should execute only *once*
assert.Equal(t, 1, registerRecorder.CallsForPathID(pathID))
Expand All @@ -66,6 +69,9 @@ func TestMultiplePIDsSharingSameFile(t *testing.T) {
assert.Contains(t, r.GetRegisteredProcesses(), pid1)
assert.Contains(t, r.GetRegisteredProcesses(), pid2)

// Assert that the callback should execute only *once*
assert.Equal(t, 1, alreadyRegisteredRecorder.CallsForPathID(pathID))

// Assert that the first call to `Unregister` (from pid1) doesn't trigger
// the callback but removes pid1 from the list
require.NoError(t, r.Unregister(pid1))
Expand Down Expand Up @@ -99,8 +105,8 @@ func TestRepeatedRegistrationsFromSamePID(t *testing.T) {
require.NoError(t, err)
pid := uint32(cmd.Process.Pid)

require.NoError(t, r.Register(path, pid, registerCallback, unregisterCallback))
require.Equal(t, ErrPathIsAlreadyRegistered, r.Register(path, pid, registerCallback, unregisterCallback))
require.NoError(t, r.Register(path, pid, registerCallback, unregisterCallback, NopCallback))
require.Equal(t, ErrPathIsAlreadyRegistered, r.Register(path, pid, registerCallback, unregisterCallback, NopCallback))
require.NoError(t, r.Unregister(pid))

// Assert that despite multiple calls to `Register` from the same PID we
Expand All @@ -122,15 +128,15 @@ func TestFailedRegistration(t *testing.T) {
require.NoError(t, err)
pid := uint32(cmd.Process.Pid)

require.NoError(t, r.Register(path, pid, registerCallback, IgnoreCB))
require.NoError(t, r.Register(path, pid, registerCallback, IgnoreCB, NopCallback))

// First let's assert that the callback was executed once, but there are no
// registered processes because the registration should have failed
assert.Equal(t, 1, registerRecorder.CallsForPathID(pathID))
assert.Empty(t, r.GetRegisteredProcesses())

// Now let's try to register the same process again
require.Equal(t, errPathIsBlocked, r.Register(path, pid, registerCallback, IgnoreCB))
require.Equal(t, errPathIsBlocked, r.Register(path, pid, registerCallback, IgnoreCB, NopCallback))

// Assert that the number of callback executions hasn't changed for this pathID
// This is because we have block-listed this file
Expand All @@ -150,7 +156,7 @@ func TestFilePathInCallbackArgument(t *testing.T) {
pid := cmd.Process.Pid

r := newFileRegistry()
require.NoError(t, r.Register(path, uint32(pid), callback, callback))
require.NoError(t, r.Register(path, uint32(pid), callback, callback, NopCallback))

// Assert that the callback paths match the pattern <proc_root>/<pid>/root/<path>
expectedPath := filepath.Join(r.procRoot, strconv.Itoa(pid), "root", path)
Expand Down Expand Up @@ -182,7 +188,7 @@ func TestRelativeFilePathInCallbackArgument(t *testing.T) {
pid := cmd.Process.Pid

r := newFileRegistry()
require.NoError(t, r.Register(relpath, uint32(pid), callback, callback))
require.NoError(t, r.Register(relpath, uint32(pid), callback, callback, NopCallback))

// Assert that the callback paths match the pattern <proc_root>/<pid>/cwd/<path>.
// We need to avoid `filepath.Join` for the last component since using
Expand Down

0 comments on commit 513fad5

Please sign in to comment.