new(proposals): API versioning for user/kernel boundary #71
Conversation
|
@gnosek: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnosek The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/area proposals |
|
@deepskyblue86: changing LGTM is restricted to collaborators In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Grzegorz Nosek <[email protected]> Add a link to https://semver.org/ Co-authored-by: Angelo Puglisi <[email protected]>
gnosek
force-pushed
the
driver-versioning-proposal
branch
from
c5a1e04 to
d07f535
Compare
proposals/20210818-driver-semver.md
Outdated
| 4. Remove consumer name and version from the libscap build process | ||
| * The probe should always be named `scap_probe` and use the API version to identify | ||
| itself |
There was a problem hiding this comment.
Removing the consumer name from the build process is the only part of this proposal I'm not convinced of. 🤔
I understand that it is convenient to use a canonical name (e.g., scap since we shouldn't use probe as per our glossary and I would avoid the underscore too). However, I would let the implementers free to change the name if they want.
So why not just change the default value to scap?
Other than that, I agree with removing the version from the build system.
There was a problem hiding this comment.
This proposal addresses a lot of usability and maintenance issues for API consumers. I like the idea of a default/canonical name (scap).
I lean towards a parametric build, where the consumer name can be passed to override a default consumer name.
There was a problem hiding this comment.
I'm fine with leaving an option to specify the module name as an internal I-know-what-I'm-doing hack but IMO it shouldn't be the default way of building. I'll try to update the wording to better express the idea.
There was a problem hiding this comment.
This is a nice proposal that is really needed to support libsinsp and libscap as separate entities from their consumers. The probe should definitely come with a different consumer agnostic name, and that name and version should be used in the pre-built probes that are downloadable to make maintenance easier.
As for whether the name should be parameterized, I don't have a strong opinion. I think most consumers are going to want to take advantage of the pre-built probes they can download; however, there may be cases where two different consumers want to have two different probes. As such, I think probably making the name configurable but defaulted, wouldn't add a lot of complexity to the build process, but give flexibility to change for those who really want too.
Finally, one thing that I think might go really well with this proposal would be to:
1.) move a version of the falco-driver-loader and docker-entrypoint.sh script into the libraries, as these are key scripts for any consumer using the probe.
There was a problem hiding this comment.
Moving falco-driver-loader into this repo would be a good idea, but it wouldn't really be a standalone script then (we don't want to hardcode the Falco driver repo for all consumers).
Maybe it would work to make two scripts: one to build the driver and another one to download a prebuilt driver (with the logic to determine the config hash etc.). Consumers could then wrap these two however they want and provide the prebuilt drivers from wherever they want.
Not sure about the docker-entrypoint script. There will have to be some per-consumer logic (at the very minimum, we're starting different processes in the end) so maybe it would work as a shell library to be sourced into the consumers' entry point scripts.
There was a problem hiding this comment.
Yes, it doesn't have to be exactly that falco-driver-loader file, but something that does something similar. Either builds the ebpf/kernel probe from source or downloads from a pre-built scap probe location - that could be configurable if a consumer wants to use its own pre-built probe repo.
All the docker-entrypoint.sh does is mount the kernel header/src directories to /host/ in the container and then calls the aforementioned driver loader. Note there is nothing consumer dependent inside the docker-entrypoint.sh that I am aware of. The consumer binary is passed in to the script as a parameter:
When porting the sysflow collector over from the sysdig libs to the falco libs, I still needed to link to the falco project proper for three main reasons:
1.) Copy over the falco-driver-loader file to automatically build or download the prebuilt probes.
2.) Copy over the docker-entrypoint.sh to setup the environment so the probe could be built properly and the driver loader called,
3.) the falco-libs patch file which renamed SYSDIG_HOST_ROOT to HOST_ROOT and .sysdig to .falco
Consumers could wrap this core functionality with whatever added features they wanted. Just a thought.
There was a problem hiding this comment.
Will have to think about it, but I think it could live as a separate PR/proposal, independent of this one.
I like the idea, I'd just like to avoid scope creep
There was a problem hiding this comment.
3.) the falco-libs patch file which renamed SYSDIG_HOST_ROOT to HOST_ROOT and .sysdig to .falco
This point and the defaulted canonical name should be addressed by #41 (N.B. the PR still use falco as default, but I will be happy to change it to scap once this proposal has been approved)
Regarding other points, I agree with @gnosek : another independent proposal would be better.
Just my 2 cents
Signed-off-by: Grzegorz Nosek <[email protected]> Co-authored-by: Leonardo Grasso <[email protected]>
|
If we rename the driver, we can start at 1.0.0. Otherwise we need to start with a version higher than all known consumers (I proposed 32 in the original PR, did anybody go higher than that?). We'll probably have to phase out PROBE_VERSION some time after this proposal gets implemented so that consumers can switch to the new scheme on their own schedule. Maybe just make it optional. |
Signed-off-by: Grzegorz Nosek <[email protected]>
|
@gnosek I do really like the proposed approach, however I'm thinking whether this should come along with a proper test suite (e.g. to reduce risk of unintentional BC introduction), WDYT? |
|
@zuc: Do I think having decent test coverage is a good idea? Yes. |
|
@gnosek to better frame my thinking: if this goes through and we end up "centralizing" drivers, relevance of a proper test suite changes dramatically IMO |
We used the libs in our sysflow collector, but the collector pretended to be sysdig, and more recently falco, for versioning so we could take advantage of the precompiled probes. I imagine most other consumers would do something similar to take advantage of that feature. |
Absolutely, that's why I'm excited about the proposal so I stop having to do it. My comment more was in response to your comment about versioning number. Just to say, I think you are fairly safe setting a version number that is higher than falco/sysdig but lower than 1.0.0 if you want. |
|
@terylt I believe starting at 1.0.0 makes sense (the API has been stable for years now with minor changes). We're about to release 12.0.0, so we can't just replace the product version with the API version in the prebuilt module names but I hope we'll be able to just change the module name (to Or we start at MAJOR_VERSION=32. 🤷 |
@gnosek The probe has been around for a number of years already so starting at 1.0.0 makes sense. |
|
@leogr what are the next steps now? |
The proposal SGTM already 👍 That being said, before giving my review approval, I want to wait for another couple of days to be sure a sufficient discussion has taken place and give other @falcosecurity/maintainers the time to take a look. I'd advise presenting this proposal in the next community call too. |
|
LGTM label has been added. Git tree hash: b0124dde89f0ab90872cd05382923ef19efba061
|
What type of PR is this?
/kind design
Any specific area of the project related to this PR?
/area proposals
What this PR does / why we need it:
This proposal introduces semver-compatible version checks for the user/kernel boundary,
i.e. between the kernel driver/eBPF probe and the userspace components.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: