Skip to content

Conversation

ExceptionalHandler
Copy link
Contributor

Description

This PR lists sensor side changes to support CreateProcess and ExitProcess events.
The program loader needs to load the entire collection of native Windows ebps program image using cilium/ebpf library.
The specs for a native Windows bpf program is not available as it is not in ELF format
This changes the order in which maps are loaded - collection is loaded first which loads maps and programs automatically.

Changelog

Tetragon on Windows supports Create Process and Exit process events via bpf programs  specific to Windows.

@ExceptionalHandler ExceptionalHandler added the release-note/major This PR introduces major new functionality label Apr 2, 2025
@ExceptionalHandler ExceptionalHandler requested a review from a team as a code owner April 2, 2025 21:26
@jrfastab
Copy link
Contributor

jrfastab commented Apr 3, 2025

OK fix up the linter. Are there any specfic changes on _linux and multi os loader side that need extra review or are we mostly moving code around? Just looking for a hint on how to review here. Can you add a bit more description to the PR title to let us know if (a) this is just code refactor to get _windows and shared or if we have logic changes on _linux side. Most important is not to introduce regressions here.

@ExceptionalHandler
Copy link
Contributor Author

Diff1
Diff2
The Load function is moved back to load.go, so a comparison from few versions ago might be easy to understand. I have attached a couple screenshots to help assess the difference.

Code is moved around and logical changes are done to assist compilation with code re-use.
Otherwise almost all most Linux code is same.

Copy link
Contributor

@olsajiri olsajiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, left some comment for the loader, thanks

return windows.GUID{Data1: data1, Data2: data2, Data3: data3, Data4: data4}
}

func WinAttachStub(_ *ebpf.Collection, _ *ebpf.CollectionSpec,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be local, also maybe more decriptive name like windowsAttachNotSupp ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding Not Supported to a function name seems little off. I think the word 'stub' should communicate the meaning.

}
}

func LoadTracepointProgram(bpfDir string, load *Program, maps []*Map, verbose int) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should not be in generic code, let's have both linux and windows version
and windows version would look like:

func LoadTracepointProgram(bpfDir string, load *Program, maps []*Map, verbose int) error {
  return windowsAttachNotSupp
}

I think it's more straight than overloading attach functions.. also it's conveying the message what attachments are supported on each os

ditto for all the other linux specific function Load*Program

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made this change.

Copy link

netlify bot commented Apr 4, 2025

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit e9bfe12
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/67f3f05c2dca840008fa3091
😎 Deploy Preview https://deploy-preview-3578--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

return winAttachStub
}

func TracepointAttach(load *Program, bpfDir string) AttachFunc {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we get away with removing this one and others, like suggested in here 16b67c7

I think we don't need those attach functions in windows code,
but I can't check and it's possible I'm missing something

it might be helpful to have target that would build windows go code on linux,
just to check if we broken something with our change

Copy link
Contributor Author

@ExceptionalHandler ExceptionalHandler Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need these as stub functions for compilation. Windows code will not compile without these stub functions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I can remove some but not all stub functions. Let me change..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed some more unused functions

…anges

This PR lists sensor side changes to support CreateProcess and ExitProcess events.
The program loader needs to load the entire collection of native Windows ebps program image using cilium/ebpf library.
The specs for a native Windows bpf program is not available as it is not in ELF format
This changes the order in which maps are loaded - collection is loaded first which loads maps and programs automatically.

Signed-off-by: Anadi Anadi<[email protected]>
@ExceptionalHandler ExceptionalHandler merged commit e786762 into cilium:main Apr 7, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/major This PR introduces major new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants