feat(pipelines): auto-inject pipeline-declared capabilities; xcover requests CAP_SYS_ADMIN - #2607
feat(pipelines): auto-inject pipeline-declared capabilities; xcover requests CAP_SYS_ADMIN#2607AnmolVirdi wants to merge 10 commits into
Conversation
Signed-off-by: Anmol Virdi <anmol.virdi@chainguard.dev>
Signed-off-by: Anmol Virdi <anmol.virdi@chainguard.dev>
Signed-off-by: Anmol Virdi <anmol.virdi@chainguard.dev>
# Conflicts: # pkg/build/compile.go
toabctl
left a comment
There was a problem hiding this comment.
Nice — the mechanism is the right shape and mirrors needs.packages. Two asks:
Don't merge test-pipeline caps into the build container. Build.Compile merges caps from test: pipelines into Configuration.Capabilities, which feeds the build runner (build.go:1048) — but melange build never executes test pipelines, so uses: xcover/profile under test: grants CAP_SYS_ADMIN to the container running upstream configure/make for nothing. Fix: keep the c.Capabilities merge, delete the two tc.Capabilities merges. Test.Compile is already correct — xcover keeps working under melange test.
Warn on add/drop conflicts. If one source adds a cap and another drops it, melange emits both and each runner arbitrates differently (under bubblewrap the drop silently wins — flag order; docker/qemu decide downstream). Please warn during compile when the merged Add/Drop sets intersect.
Smaller:
- Log gathered caps in
gatherDeps(Info, with pipeline id) — packages are logged, sandbox widening should be too. - Validate cap names at compile time; a typo currently fails at
bwrap --cap-addruntime. docs/BUILD-PROCESS.mddescribesneeds:as packages-only.- Nit:
mergeCapabilitiesskips dedup whensrcis empty, contradicting its comment.
maxgio92
left a comment
There was a problem hiding this comment.
The mechanism looks right and mirrors needs.packages nicely; my main concern is scoping, details inline. These are meant to add to toabctl's review, not repeat it.
Just a detail (non-blocking): the new Needs block in schema.json matches generator output, but a full go generate at this head also emits the apko runtime_keyring definitions, so a future clean regen will produce an unrelated-looking diff.
Signed-off-by: Anmol Virdi <anmol.virdi@chainguard.dev>
|
@toabctl, thanks for the suggestions. I've address all concerns:
On compile-time cap-name validation, I left it out of this PR on purpose: there's no capability list in our deps to check against, so it would mean hardcoding and maintaining the kernel set (which drifts as caps like |
|
@AnmolVirdi thanks for b61c13e, the build-side fix and the negative test look good. One scoping gap remains under melange test: subpackage test caps still merge into the shared t.Configuration.Capabilities, so one subpackage's CAP_SYS_ADMIN reaches every sibling test container. Scoping them per-test Compiled into the corresponding buildWorkspaceConfig call (container.Config already has a Capabilities field) would close it, the way te.Packages is scoped today. |
Signed-off-by: Anmol Virdi <anmol.virdi@chainguard.dev>
|
Thanks for the follow-up review folks. I've update the PR.
|
maxgio92
left a comment
There was a problem hiding this comment.
Thanks Anmol! The per-test capability scoping is exactly right, and the new compile_test.go cases pin it well. 🚀
toabctl
left a comment
There was a problem hiding this comment.
Nice feature — declaring capabilities next to the pipeline that needs them is the right shape, and I like that test caps are scoped per-test container rather than unioned into the build runner.
I verified the shipped path works end to end: with uses: xcover/profile, CAP_SYS_ADMIN reaches both the build runner (Build.Compile) and each test runner including subpackage tests (Test.Compile), after going through ParseConfiguration. So the xcover fix itself is good.
The problem is the other two entry points the PR documents. ParseConfiguration rebuilds pipelines and tests field-by-field (config.go:1785, config.go:1799, unconditionally — not gated on whether any substitutions exist), and replaceNeeds/replaceTest don't copy the new fields. Parsing this manifest:
capabilities:
add: [CAP_TOP_LEVEL]
pipeline:
- needs:
packages: [wget]
capabilities:
add: [CAP_SYS_ADMIN]
runs: echo hi
test:
capabilities:
add: [CAP_TEST_LEVEL]
pipeline:
- needs:
capabilities:
add: [CAP_TEST_PIPELINE]
runs: echo testgives:
top-level capabilities: {Add:[CAP_TOP_LEVEL] Drop:[]} <- survives
pipeline[0].Needs: &{Packages:[wget] Capabilities:{Add:[]}}
test.Capabilities: {Add:[] Drop:[]}
test.pipeline[0].Needs: &{Packages:[] Capabilities:{Add:[]}}
needs.packages survives, every capability is gone. Same for subpackages[].pipeline[].needs.capabilities and subpackages[].test.capabilities. No error, no warning — KnownFields(true) accepts the keys because the fields exist, they're just dropped afterwards.
Two consequences:
- The inline form that
docs/BUILD-PROCESS.mdintroduces the feature with is a no-op, as istest.capabilitiesfromschema.json/schema.cue. - The doc line "Names are checked while the pipeline is compiled, so a misspelled
CAP_*fails the build rather than the container" doesn't hold for the inline form —needs.capabilities.add: [CAP_TOTALLY_NOT_REAL]parses clean andCompilereturnsnil, because the value is discarded beforeknownRunnerCapabilitiesvalidation ever sees it.
No regression risk (both fields are new in this PR), so this is "documented field is dead on arrival" rather than "something broke." Still worth fixing here — shipping the docs without the fix is worse than shipping neither.
The reason CI is green on this is that TestCompileCapabilities builds config.Configuration literals in Go and never goes through ParseConfiguration, so it only exercises the one path that happens to work. A parse-based case would have caught all of it.
Separately, worth considering: these replaceX functions returning a freshly-constructed struct means every future field added to Needs/Test/Pipeline is silently dropped until someone remembers to update them. out := *in followed by overriding just the fields that need substitution fails safe instead.
|
@toabctl thanks, all three are fixed in a80853b. Added parse-based coverage: On your last point: |
|
The check failure |
Melange Pull Request Template
Functional Changes
Summary
This PR adds the ability for a pipeline to declare the Linux capabilities it requires, so that those capabilities are automatically merged into the runner whenever the pipeline is used. It also wires this mechanism into the built-in
xcover/profilepipeline, which now automatically requestsCAP_SYS_ADMINbecause xcover attaches BPF uprobes to the profiled interpreter.Motivation
Previously, capabilities could only be declared at the top-level
capabilitiesblock of a melange manifest, which meant every package author using xcover had to remember to manually addCAP_SYS_ADMINor the profiler would fail at runtime. This change moves that requirement into the pipeline definition itself, following the same pattern already used byneeds.packages, so the capability is granted transparently and cannot be forgotten.Changes
Capabilitiesfield to theNeedsstruct inpkg/config/config.go, allowing a pipeline to declare capabilities vianeeds.capabilities.add/needs.capabilities.drop.Compiledstruct andgatherDepsinpkg/build/compile.goto accumulate capabilities declared by pipelines as they are walked.mergeCapabilitieshelper and wired it into every build and test compile path so gathered capabilities are merged into the runner configuration.needs.capabilities.add: [CAP_SYS_ADMIN]onpkg/build/pipelines/xcover/profile.yaml.pkg/config/schema.jsonand updatedpkg/config/schema.cueto reflect the new field.Behavior
Manifest-declared capabilities and pipeline-declared capabilities are combined as a deduplicated union rather than one overwriting the other. If a manifest declares
CAP_NET_ADMINand a test usesxcover/profile, the runner receives bothCAP_NET_ADMINandCAP_SYS_ADMIN, and duplicates across the two sources are collapsed to a single entry. BecauseConfiguration.Capabilitiesis shared by both the build and test runners, the union applies to whichever runner executes the pipeline; bubblewrap cannot scope a capability to an individual pipeline step.Testing
Added
TestCompileCapabilitiesinpkg/build/compile_test.go, which covers both theTest.CompileandBuild.Compilepaths and asserts that manifest and pipeline capabilities are merged into a deduplicated union without overwriting each other.go build ./...succeeds, and the fullpkg/buildandpkg/configtest suites pass.