features: Make Main Package Feature Tests Public

[nathanjsweet]

Move all feature teats in the main package to a new internal directory, "internal/features", which can be used by the main package and also mirrored in the "features" package for public use.

[nathanjsweet]

I'm sure we could eventually move all other feature tests to this new "internal/features" directory and mirror them in the "features" directory, but I think this is a good start for now.

[rgo3]

Thank you for putting this PR together! I want to share some concerns I have about expanding the features package's public API surface.

The features package was originally designed to closely mirror bpftool feature probes, with very limited exceptions. I'm hesitant about adding these new APIs since I haven't seen strong evidence of demand for them. Keeping these probes unexported gives us the flexibility to adapt if kernel changes affect the underlying functionality.

I've also noticed that the proposed internal package approach would create challenges - specifically with feature tests in other (sub)packages like link where we'd run into cyclic import issues due to main package dependencies.

For handling feature testing needs, here are the approaches I'd recommend (in order of preference):

1. Consider using a "better to ask for forgiveness than permission" pattern - rather than probing first, try calling the desired API directly and handle any returned errors. Our internal feature tests exist primarily so the APIs themselves can return ErrNotSupported when a kernel feature isn't available.

2. If the above pattern doesn't work for your use case, copying the specific internal feature test you need would likely be simpler than expanding the features package API.

3. If neither of these approaches meets your needs, I'd be happy to discuss which specific APIs you need and explore why they might belong in the features package. We can continue the discussion here or start a thread in the ebpf-go-dev channel on the OSS slack.

Let me know what you think.

[nathanjsweet]

The features package was originally designed to closely mirror bpftool feature probes, with very limited exceptions. I'm hesitant about adding these new APIs since I haven't seen strong evidence of demand for them. Keeping these probes unexported gives us the flexibility to adapt if kernel changes affect the underlying functionality.

The guarantee I would want the features package to make is that it is simply the same checks that the main library methods in the package are using to test feature viability. The underlying kernel stability does not matter to an adequate definition of stability for the package. Consider this PR demand for the feature.

I've also noticed that the proposed internal package approach would create challenges - specifically with feature tests in other (sub)packages like link where we'd run into cyclic import issues due to main package dependencies.

If you notice how this PR is laid out it is solving the exact same problem for the top level package (including tests that consumed parts of the package it was in). internal/features (which depends on nothing) runs the actual tests, the other packages consume it, and features exposes it.

Consider using a "better to ask for forgiveness than permission" pattern - rather than probing first, try calling the desired API directly and handle any returned errors. Our internal feature tests exist primarily so the APIs themselves can return ErrNotSupported when a kernel feature isn't available.

It won't work for my use case.

If the above pattern doesn't work for your use case, copying the specific internal feature test you need would likely be simpler than expanding the features package API.

I could do this, but I would not maintain it as well as this library could.

If neither of these approaches meets your needs, I'd be happy to discuss which specific APIs you need and explore why they might belong in the features package. We can continue the discussion here or start a thread in the ebpf-go-dev channel on the OSS slack.

This library is a good place for this logic to live for public consumption, because it must keep the checks up to date anyways for itself. Stability is a balance between literal API stability and coherence. Sorry for reiterating this, but it's essential. Literal API stability in the features package is less critical than coherence with the rest of the library. Users of this library can easily handle an upgrade that breaks compilation. Maintaining logic that may never break but becomes out of date is a lot harder.

[rgo3]

Consider this PR demand for the feature.

To me, just adding 14 new public APIs without a specific use-case isn't evidence of demand for a feature. It's not my intention to sound pedantic here, so for reference I would like to refer to the original issue that led to the features package: #235. Already then, one of the main concerns was how we would decide which feature probes would go into this new package, since we've always aimed for a minimal public API surface. And as already stated, preferably users will call APIs directly and rely on errors returned from the internally executed feature tests. Merging this PR would completely move against that decision and set a precedent I'm not comfortable with given that we've rejected similar proposals in the past. (cc @ti-mo in case that stance has changed)

internal/features (which depends on nothing) runs the actual tests, the other packages consume it, and features exposes it.

Maybe I didn't express myself well enough here. My concern is that this approach won't be as clean for feature tests that don't live in the main package and depend on the main package. E.g.

ebpf/link/syscalls.go

Line 167 in fd1fe1b

var haveNetkit = internal.NewFeatureTest("netkit", func() error {

, which we recently rejected as a publicly exposed feature test, see #1602. Merging your PR would set precedent to revisit such cases.

It won't work for my use case

Could you elaborate on why it won't work and what exactly that use-case is? Given your specifics, maybe we can find some middle ground.