Skip to content

feat: Add namespace isolation support for proxy processes#7

Merged
muchzill4 merged 35 commits into
arm:mainfrom
Luke-Parkin:network-namespaces
Nov 3, 2025
Merged

feat: Add namespace isolation support for proxy processes#7
muchzill4 merged 35 commits into
arm:mainfrom
Luke-Parkin:network-namespaces

Conversation

@Luke-Parkin

@Luke-Parkin Luke-Parkin commented Oct 7, 2025

Copy link
Copy Markdown
Member

This PR resolves the issue where the proxy process was being spawned in the namespace as defined by the OCI bundle.

Namespacing can only be effectively delivered in two ways:

  1. If the kernel has control groups v1- Root is required.
  2. If the kernel has control groups v2, using User groups is possible.

However control groups v2 support is not guaranteed (especially in embedded linux distros). Plus, since our proxy process doesn't actually do anything that would require namespacing, it seems overkill to implement cgroups v2 and v1.

For context, runc rewrites the OCI bundle to ignore and flags that wouldn't be supported in rootless mode, if running in rootless mode.

We have elected to do essentially the same, however instead of rewriting the bundle, if our effective user ID is not root, we just don't change the namespace (so it's created into that of the host.)

What this means for the user, is that if they are running in root; everything works as expected. Their proxy process is namespaced as told to by the engine. If they are in rootless, we ignore the namespacing settings.

Changes

  • Create proxy in the correct namespace if root, defined by the bundle.
  • Add two E2E tests
    • One of these requires root, the other one mandates no root. As the rest of the tests require no root, I elected to add a separate test to the actions that runs the root test in sudo mode.

Checklist

  • 🤖 This change is covered by tests as required.
  • 🤹 All required manual testing has been performed.
  • 📖 All documentation updates are complete.

Signed-off-by: luke <luke.parkin@arm.com>
Signed-off-by: luke <luke.parkin@arm.com>
@Luke-Parkin Luke-Parkin changed the title Network namespaces feat: Network namespaces Oct 7, 2025
Signed-off-by: luke <luke.parkin@arm.com>
Signed-off-by: luke <luke.parkin@arm.com>
@Luke-Parkin Luke-Parkin force-pushed the network-namespaces branch 2 times, most recently from 646e34d to 7d06bff Compare October 7, 2025 16:21
The options here were either to override the EUID in the code, or require the E2E tests to be run one as sudo, and one as not sudo.
The latter was chosen as the changes to the code for tests were somewhat heavy.

Add sudo test run to the actions.

Signed-off-by: luke <luke.parkin@arm.com>
Signed-off-by: luke <luke.parkin@arm.com>
@Luke-Parkin Luke-Parkin marked this pull request as ready for review October 7, 2025 17:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements network namespace support by creating proxy processes in the namespace as defined by the bundle specification. The changes enable namespace isolation for containers while maintaining compatibility for non-root execution.

  • Adds namespace clone flag mapping and processing logic
  • Modifies proxy process creation to accept and use namespace specifications
  • Includes comprehensive test coverage for both root and non-root scenarios

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
internal/runtime/create.go Updates proxy process creation call to pass spec parameter
internal/proxy/proxy.go Implements namespace flag processing and clone flags for proxy creation
e2e/runtime_test.go Adds test coverage for namespace inheritance and non-root behavior
docs/DEVELOPMENT.md Documents how to run namespace-specific tests requiring sudo
.github/workflows/ci.yml Adds CI workflow step for root namespace tests

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread internal/proxy/proxy.go Outdated
Comment thread e2e/runtime_test.go Outdated
Signed-off-by: luke <luke.parkin@arm.com>
Comment thread .github/workflows/ci.yml Outdated
Comment thread docs/DEVELOPMENT.md Outdated
Comment thread e2e/runtime_test.go
Comment thread e2e/runtime_test.go
Comment thread internal/proxy/proxy.go Outdated
Comment thread internal/proxy/proxy.go Outdated

@yejseo01 yejseo01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Had a look, and had some refactoring suggestions

Comment thread e2e/runtime_test.go Outdated
Comment thread internal/proxy/proxy.go Outdated
Comment thread .github/workflows/ci.yml Outdated
Signed-off-by: luke <luke.parkin@arm.com>
Signed-off-by: luke <luke.parkin@arm.com>
Comment thread internal/proxy/namespaces_test.go Outdated
Comment thread internal/proxy/namespaces.go
Comment thread e2e/limavm/limavm.go Outdated
Comment thread docs/DEVELOPMENT.md
Comment thread e2e/runtime_test.go Outdated
Comment thread e2e/runtime_test.go Outdated
@Luke-Parkin

Copy link
Copy Markdown
Member Author

#7 (comment)

Assuming this one is outdated?

@Luke-Parkin Luke-Parkin requested a review from muchzill4 October 31, 2025 15:48
Signed-off-by: luke <luke.parkin@arm.com>
@Luke-Parkin

Luke-Parkin commented Oct 31, 2025

Copy link
Copy Markdown
Member Author
    --- PASS: TestRuntime/basic_container_lifecycle (0.42s)
    --- PASS: TestRuntime/errors_when_requested_remoteproc_name_doesn't_exist (0.04s)
    --- PASS: TestRuntime/killing_process_by_pid_stops_the_running_container (0.28s)
    --- PASS: TestRuntime/writes_pid_to_file_specified_by_--pid-file (0.10s)
    --- PASS: TestRuntime/proxy_process_creates_new_process (0.77s)
        --- PASS: TestRuntime/proxy_process_creates_new_process/in_requested_namespace_when_root (0.39s)
        --- PASS: TestRuntime/proxy_process_creates_new_process/in_user's_namespace_when_not_root (0.38s)
=== RUN   TestPodman

What does everyone think about this? Bit messy? @muchzill4

@Luke-Parkin

Copy link
Copy Markdown
Member Author

Also- I'm running golangci-lint on v2.6.0 locally and it's not picking up and linting issues :/

@muchzill4

Copy link
Copy Markdown
Contributor

What does everyone think about this? Bit messy? @muchzill4

I think it's fine. Tweaked the test descriptions a bit:

--- PASS: TestRuntime (99.31s)
    --- PASS: TestRuntime/proxy_process_namespacing (0.77s)
        --- PASS: TestRuntime/proxy_process_namespacing/creates_process_in_requested_namespace_when_root (0.39s)
        --- PASS: TestRuntime/proxy_process_namespacing/creates_process_in_user's_namespace_when_not_root (0.38s)
=== RUN   TestPodman

Signed-off-by: luke <luke.parkin@arm.com>
Signed-off-by: luke <luke.parkin@arm.com>
muchzill4 and others added 3 commits October 31, 2025 18:02
refactor: Reuse `Runnable` abstraction for `sudo`
Signed-off-by: luke <luke.parkin@arm.com>
muchzill4
muchzill4 previously approved these changes Oct 31, 2025

@muchzill4 muchzill4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚢 🇮🇹

Signed-off-by: luke <luke.parkin@arm.com>
Signed-off-by: Luke Parkin <65398091+Luke-Parkin@users.noreply.github.com>
@Luke-Parkin Luke-Parkin requested a review from muchzill4 November 2, 2025 17:33
@muchzill4 muchzill4 changed the title feat: Network namespaces feat: Add namespace isolation support for proxy processes Nov 3, 2025
muchzill4
muchzill4 previously approved these changes Nov 3, 2025
Comment thread e2e/runtime_via_podman_test.go Outdated
Signed-off-by: Bartek Mucha <muchzill4@gmail.com>
@muchzill4 muchzill4 merged commit e414dfb into arm:main Nov 3, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants