CI/GHA: test against all minor supported libseccomp versions #70

kolyshkin · 2021-09-13T15:47:13Z

In short:

bump minimally required libseccomp to 2.3.1 (was 2.2.0);
CI/GHA: test against all major supported libseccomp releases (2.3.x to 2.5.x);
fix all the bugs against non-latest releases (quite a few).

Addresses: #65 (comment)

Please review commit by commit.

drakenclimber · 2021-09-13T21:15:25Z

Thanks @kolyshkin. A matrix seemed like a good way to meet everyone's testing needs; I'm glad you added it.

Reviewed-by: Tom Hromatka <tom.hromatka@oracle.com>

.github/workflows/test.yml

kolyshkin · 2021-09-16T17:11:42Z

Looking more into libseccomp-golang, it seems that it will benefit from testing against older supported libseccomp releases, not just the latest. Currently the minimum supported version is 2.2.0, let's try to add it.

kolyshkin · 2021-09-16T20:18:21Z

Nice -- added libseccomp 2.3.x and 2.4.x revealed a bug in TestNotif -- it checks the API level but do not check libseccomp version :)

Will fix later today, changing to DRAFT for now.

kolyshkin · 2021-09-24T02:33:38Z

@drakenclimber @pcmoore PTAL.

If sending patches to the ML is preferable, please let me know (currently, CONTRIBUTING.md says otherwise).

drakenclimber · 2021-09-24T12:51:14Z

If sending patches to the ML is preferable, please let me know (currently, CONTRIBUTING.md says otherwise).

I personally prefer github pull requests. But I wear many hats and unfortunately libseccomp-golang isn't one of them this week. The Linux Plumbers Conference is currently going on all week.

pcmoore · 2021-09-24T19:44:08Z

If sending patches to the ML is preferable, please let me know (currently, CONTRIBUTING.md says otherwise).

I personally prefer github pull requests. But I wear many hats and unfortunately libseccomp-golang isn't one of them this week. The Linux Plumbers Conference is currently going on all week.

+1 to what @drakenclimber already said. This has been a particularly difficult week on my end due to some personal issues, LPC, and a handful of high-priority and high-visibility SELinux kernel issues flaring up. I'm working my way through the libseccomp-golang PRs right now ...

pcmoore · 2021-09-24T20:36:24Z

.github/workflows/test.yml

@@ -16,7 +16,7 @@ jobs:
      fail-fast: false
      matrix:
        go-version: [1.15.x, 1.16.x, 1.17.x]
-        libseccomp: ["v2.5.2", "HEAD"]
+        libseccomp: ["v2.2.1", "v2.2.3", "v2.3.3", "v2.4.3", "v2.5.2", "HEAD"]


This is interesting as we no longer support v2.2.x, v2.3.x, or v2.4.x upstream anymore. I guess we can let this slide for no so long as there are no major issues, but as soon as something significant crops up with these older releases we should probably just drop them.

I'm merely following what's in there already (except for dropping 2.2.0 support).

If you want, I can rework this to drop 2.2.x support, too, as this is only used by very old (but still supported) Ubuntu 14.04.
Dropping 2.3 and especially 2.4 is more problematic though, as this is what many long-term supported distros ship (Debian, RHEL, SLES).

I think it makes total sense to drop 2.2 and 2.3 (and maybe 2.4) after the release is cut, but it's up to you to decide.

pcmoore · 2021-09-24T20:57:00Z

Thanks again for the patches @kolyshkin, but I have a few general comments, in no particular order:

I dislike combining unrelated patches into a single PR simply because while working on item A you happened to stumble across item B. I get it, it's extremely tempting to just do everything in one PR, but it is a bit of a mess to review. I'd much rather you see things where you post one PR, get it merged, and then post the other; or post both PRs at the same time with a comment that PR A is dependent on PR b (or something similar).
Your patch prefixes are not consistent, which I guess is somewhat understandable given the repo and what I'm used to seeing with golang. When I say not consistent I'm talking about mixing function names, filenames, wildcards, etc. I very much prefer using patch subject prefixes that are either a filename or a subsystem name, e.g. "tests: test all the things" or "seccomp_internal: bump the minimum supported version to X.Y..Z"; they are less transient than function names. Please don't ever use wildcards in the subject prefix.
Be careful when you talk about "supported versions" in the commit descriptions and code comments as there is ambiguity around who is providing support. For example, there are distros which are still providing user support but are shipping libseccomp v2.3 (possibly older) and we/upstream don't currently support releases below v2.5.

My apologies if all of the above seem a bit trivial and nit-picky, but as you are starting to contribute more - thank you! - you're going to be subjected to a bit more "tough love" if you will :) Please leave this PR as-is, I'll fix these things up when I hear back from @drakenclimber (below), but it is something to keep in mind for the future.

@drakenclimber, this PR has changed quite a bit since you last looked at it, does your Reviewed-by tag still stand?

kolyshkin · 2021-09-24T21:59:14Z

I dislike combining unrelated patches into a single PR simply because while working on item A you happened to stumble across item B. I get it, it's extremely tempting to just do everything in one PR, but it is a bit of a mess to review. I'd much rather you see things where you post one PR, get it merged, and then post the other; or post both PRs at the same time with a comment that PR A is dependent on PR b (or something similar).

If you got an impression that I piled up everything I can change into this single PR, let me assure you this is not so. It all started with "let's add libseccomp versions that this package seems to support to CI", and the rest was added to make the CI green.

The only non-really-related stuff is "seccomp_internal.go: simplify version checks" and "test: execInSubprocess: simplify", and I will move it out of this commit.

Be careful when you talk about "supported versions" in the commit descriptions and code comments

Will reword.

My apologies if all of the above seem a bit trivial and nit-picky

Definitely no need to apologize, any constructive feedback (and "tough love") is very welcome and truly appreciated.

Please leave this PR as-is.

No problem, I'd rather fix it in place.

Currently this package requires libseccomp < 2.2.0, refusing to build otherwise, and contains a few kludges specifically targeting libseccomp 2.2.0. Let's require version 2.3.1 or greater, and remove the kludges for older versions. While at it, reword the error message to remove the word "supported". Checking for libseccomp versions shipped with various old (but still supported) releases, here is what I found out: * Ubuntu 14.04 "Trusty Tahr": 2.1.1 (unsupported by this pkg), with 2.2.3 available in backports repo [1] * Debian "Stretch" (aka oldoldstable): 2.3.1 [2] * RHEL/CentOS 7: 2.3.1 [3] * SLES 15 SP1: 2.4.3 [4] * openSUSE Leap 15.2: 2.4.1 [4] * Alpine 3.11: 2.4.2 [5] * Arch, Gentoo: 2.5.x [1] https://launchpad.net/ubuntu/+source/libseccomp [2] https://packages.debian.org/search?keywords=libseccomp [3] https://rpmfind.net/linux/rpm2html/search.php?query=libseccomp [4] https://software.opensuse.org/package/libseccomp [5] https://pkgs.alpinelinux.org/packages?name=libseccomp&branch=v3.11 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Otherwise the LD_PRELOAD, set in a CI job, is not being used, leading to distro libseccomp version being used instead of the one we prepared. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Instead of testing against whatever version of libseccomp the distro (Ubuntu 20.04 in this case) provides, do compile and test against the latest released version, 2.5.2. While at it, it's relatively easy to enable testing against git tip as well, so implement it as well. All we have to do is to change the version from 0.0.0 to some value that is >= current version. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Functions seccomp_api_get(3) and seccomp_api_set(3) appeared in libseccomp 2.4.0, not 2.3.3 like the code assumed. This fixes TestGetAPILevel failure with libseccomp 2.3.3. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

While testing this package against a recent kernel and older libseccomp library (2.2.x to 2.4.x) it appears that every time we perform an API level check, libseccomp version check is also needed. Modify checkAPI to also call checkVersion. Unify both functions, as well as VersionError, to use minor, major, micro triad everywhere. While at it: - fix checkAPI and checkVersion docs; - simplify their tests, add more test cases. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

TestNotif should check not only for API level >= 6, but also for libseccomp >= 2.5.0. Use notifSupported() to fix. Same applies to TestNotifUnsupported. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

NOTE that libseccomp < 2.5.x is no longer officially supported upstream, but this library still works with 2.3.x and 2.4.x, so we test against those. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This is to ensure we're testing against the expected libseccomp version (as prepared by CI, which sets the _EXPECTED_LIBSECCOMP_VERSION var). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2021-09-29T20:36:22Z

Rebased on top of the current main

kolyshkin · 2021-10-01T01:21:09Z

@drakenclimber @pcmoore PTAL 🙏🏻

pcmoore · 2021-10-01T12:43:42Z

Patience please. This week, and last week for that matter, have been busy with conferences as well as the usual stuff. On a personal note, the late summer, early fall weather has been really nice where I live too so I've been trying to enjoy that outside of work.

We see the activity on this, and appreciate it, but please be patient and allow us the time to review it.

drakenclimber · 2021-10-04T21:55:46Z

Thanks, @kolyshkin. The most recent changes look good to me.

Acked-by: Tom Hromatka <tom.hromatka@oracle.com>

pcmoore · 2021-10-08T19:13:44Z

Merged at HEAD 6a935f5, thanks @kolyshkin!

kolyshkin force-pushed the test-against-2.5.2 branch from 0e3567c to 59c82f9 Compare September 13, 2021 15:49

kolyshkin changed the title ~~[WIP] CI/GHA: test against libseccomp 2.5.2 and HEAD~~ CI/GHA: test against libseccomp 2.5.2 and HEAD Sep 13, 2021

drakenclimber requested review from drakenclimber and pcmoore September 13, 2021 21:07

drakenclimber added this to the v0.9.2 milestone Sep 13, 2021

drakenclimber added the enhancement label Sep 13, 2021

drakenclimber mentioned this pull request Sep 13, 2021

RFE: add release process document #71

Closed

drakenclimber approved these changes Sep 13, 2021

View reviewed changes

pcmoore reviewed Sep 13, 2021

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

kolyshkin force-pushed the test-against-2.5.2 branch from 59c82f9 to 5bcd998 Compare September 14, 2021 00:01

kolyshkin force-pushed the test-against-2.5.2 branch from 5bcd998 to c985b4b Compare September 16, 2021 20:06

kolyshkin changed the title ~~CI/GHA: test against libseccomp 2.5.2 and HEAD~~ CI/GHA: test against various libseccomp versions Sep 16, 2021

kolyshkin force-pushed the test-against-2.5.2 branch from c985b4b to 6df53ab Compare September 16, 2021 20:09

kolyshkin marked this pull request as draft September 16, 2021 20:17

kolyshkin mentioned this pull request Sep 17, 2021

RFE: misc nits #73

Closed

kolyshkin force-pushed the test-against-2.5.2 branch 5 times, most recently from 3038dc9 to e247b99 Compare September 17, 2021 20:11

kolyshkin changed the title ~~CI/GHA: test against various libseccomp versions~~ CI/GHA: test against all minor supported libseccomp versions Sep 17, 2021

kolyshkin force-pushed the test-against-2.5.2 branch 6 times, most recently from 3e739ba to e421558 Compare September 17, 2021 21:40

kolyshkin force-pushed the test-against-2.5.2 branch from 43c6868 to 591efe2 Compare September 18, 2021 19:17

kolyshkin mentioned this pull request Sep 18, 2021

RFE: support for SCMP_FLTATR_CTL_OPTIMIZE, SCMP_FLTATR_API_SYSRAWRC #75

Closed

kolyshkin requested a review from pcmoore September 20, 2021 20:44

pcmoore reviewed Sep 24, 2021

View reviewed changes

pcmoore added the priority/medium label Sep 24, 2021

kolyshkin force-pushed the test-against-2.5.2 branch 3 times, most recently from 126079c to 08d3474 Compare September 25, 2021 00:42

kolyshkin added 8 commits September 29, 2021 13:35

test: execInSubprocess: pass current env

2710d8c

Otherwise the LD_PRELOAD, set in a CI job, is not being used, leading to distro libseccomp version being used instead of the one we prepared. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

test: fix APILevelIsSupported

d1ff2ae

Functions seccomp_api_get(3) and seccomp_api_set(3) appeared in libseccomp 2.4.0, not 2.3.3 like the code assumed. This fixes TestGetAPILevel failure with libseccomp 2.3.3. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

test: fix TestNotif*

ce7ceff

TestNotif should check not only for API level >= 6, but also for libseccomp >= 2.5.0. Use notifSupported() to fix. Same applies to TestNotifUnsupported. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

CI/GHA: add more libseccomp versions to the matrix

70107f6

NOTE that libseccomp < 2.5.x is no longer officially supported upstream, but this library still works with 2.3.x and 2.4.x, so we test against those. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

test: add TestExpectedSeccompVersion

6f6ba25

This is to ensure we're testing against the expected libseccomp version (as prepared by CI, which sets the _EXPECTED_LIBSECCOMP_VERSION var). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the test-against-2.5.2 branch from 08d3474 to 6f6ba25 Compare September 29, 2021 20:36

drakenclimber approved these changes Oct 5, 2021

View reviewed changes

kolyshkin requested a review from pcmoore October 7, 2021 00:24

pcmoore closed this Oct 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI/GHA: test against all minor supported libseccomp versions #70

CI/GHA: test against all minor supported libseccomp versions #70

kolyshkin commented Sep 13, 2021 •

edited

drakenclimber commented Sep 13, 2021

kolyshkin commented Sep 16, 2021

kolyshkin commented Sep 16, 2021

kolyshkin commented Sep 24, 2021

drakenclimber commented Sep 24, 2021

pcmoore commented Sep 24, 2021

pcmoore Sep 24, 2021

kolyshkin Sep 24, 2021

pcmoore commented Sep 24, 2021

kolyshkin commented Sep 24, 2021

kolyshkin commented Sep 29, 2021

kolyshkin commented Oct 1, 2021

pcmoore commented Oct 1, 2021

drakenclimber commented Oct 4, 2021

pcmoore commented Oct 8, 2021

CI/GHA: test against all minor supported libseccomp versions #70

CI/GHA: test against all minor supported libseccomp versions #70

Conversation

kolyshkin commented Sep 13, 2021 • edited

drakenclimber commented Sep 13, 2021

kolyshkin commented Sep 16, 2021

kolyshkin commented Sep 16, 2021

kolyshkin commented Sep 24, 2021

drakenclimber commented Sep 24, 2021

pcmoore commented Sep 24, 2021

pcmoore Sep 24, 2021

Choose a reason for hiding this comment

kolyshkin Sep 24, 2021

Choose a reason for hiding this comment

pcmoore commented Sep 24, 2021

kolyshkin commented Sep 24, 2021

kolyshkin commented Sep 29, 2021

kolyshkin commented Oct 1, 2021

pcmoore commented Oct 1, 2021

drakenclimber commented Oct 4, 2021

pcmoore commented Oct 8, 2021

kolyshkin commented Sep 13, 2021 •

edited