refactor: merge ndns and nproxy with ncdns and ncproxy#1420
Open
0xle0ne wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the proxy and DNS architecture by folding the legacy nproxy (nginx) and ndns (dnsmasq) runtime responsibilities into the controllers (ncproxy, ncdns), and updates dev/CI/build tooling accordingly (including a Rust toolchain bump).
Changes:
- Remove legacy
nproxy/ndnsservices/images from dev + CI flows and documentation; keep their source assets as legacy references. - Make
ncproxymanage an embedded nginx process (start/reload/monitor) and ship nginx in thencproxyruntime image. - Make
ncdnsmanage an embedded dnsmasq process (start/reload/monitor) and ship dnsmasq in thencdnsruntime image; bump Rust toolchain to 1.95.0 across builds.
Reviewed changes
Copilot reviewed 23 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/docker-compose.yaml | Removes legacy nproxy/ndns from test compose setup. |
| Statefile.yml | Switches dev Statefile to use ncproxy/ncdns only (host networking, state dirs). |
| scripts/install_dev_image.sh | Builds the unified nanocl-dev image instead of building/pulling legacy images. |
| scripts/dev.Dockerfile | Rebuilds dev image on Debian bookworm and installs nginx/dnsmasq tooling for embedded runtimes. |
| scripts/build_images.sh | Skips publishing legacy nproxy/ndns images by default. |
| rust-toolchain.toml | Bumps Rust toolchain to 1.95.0. |
| README.md | Updates service list and routing/DNS references to ncproxy/ncdns. |
| installer.yml | Removes legacy nproxy/ndns cargoes; configures ncproxy/ncdns host networking/state env. |
| docker-compose.yaml | Removes legacy services; runs ncproxy/ncdns with host networking and state dirs. |
| CONTRIBUTING.md | Marks nproxy/ndns as legacy runtime asset sources. |
| bin/ncvpnkit/Dockerfile | Updates Rust base image to 1.95.0. |
| bin/ncproxy/src/utils/nginx.rs | Replaces docker-exec-based nginx control with local process management + monitoring. |
| bin/ncproxy/src/subsystem/init.rs | Starts/configures nginx during ncproxy initialization. |
| bin/ncproxy/src/models/system.rs | Refactors event emitter to trigger reloads via state_dir instead of docker client exec. |
| bin/ncproxy/src/main.rs | Spawns nginx monitor alongside the HTTP server. |
| bin/ncproxy/Dockerfile | Changes runtime image from scratch to Ubuntu w/ nginx; embeds legacy HTML assets. |
| bin/ncdns/src/utils.rs | Updates reload path to call dnsmasq reload; test helper now starts dnsmasq. |
| bin/ncdns/src/services/rule.rs | Switches rule application/removal to reload dnsmasq via the new API. |
| bin/ncdns/src/main.rs | Starts dnsmasq + monitor as part of controller startup. |
| bin/ncdns/src/dnsmasq.rs | Implements dnsmasq lifecycle management (pidfile, start/reload, monitor). |
| bin/ncdns/Dockerfile | Changes runtime image from scratch to Alpine w/ dnsmasq. |
| bin/nanocld/Dockerfile | Updates Rust base image to 1.95.0. |
| bin/nanocl/Dockerfile | Updates Rust base image to 1.95.0. |
| .github/workflows/tests.yml | Builds nanocl-dev image for CI instead of legacy images. |
| .github/workflows/publish_stable_image.yml | Skips publishing legacy nproxy/ndns images. |
| .github/workflows/publish_nightly_image.yml | Skips publishing legacy nproxy/ndns images. |
| .github/workflows/draft_stable_nanocl.yml | Updates Rust container image to 1.95.0. |
| .github/workflows/draft_nightly_nanocl.yml | Updates Rust container image to 1.95.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+32
to
+33
| crate::utils::nginx::ensure_conf(&state).await?; | ||
| crate::utils::nginx::ensure_started(&state.store.dir).await?; |
Comment on lines
19
to
25
| async fn run(cli: &Cli) -> IoResult<()> { | ||
| // Spawn a new thread to listen events from nanocld | ||
| let dnsmasq = Dnsmasq::new(&cli.state_dir) | ||
| .with_dns(cli.dns.clone()) | ||
| .ensure()?; | ||
| dnsmasq.start().await?; | ||
| #[allow(unused)] |
Comment on lines
168
to
+175
| let dnsmasq = dnsmasq::Dnsmasq::new("/tmp/dnsmasq"); | ||
| dnsmasq | ||
| .ensure() | ||
| .expect("Expect to setup minimal dnsmasq config"); | ||
| dnsmasq | ||
| .start() | ||
| .await | ||
| .expect("Expect to start dnsmasq for tests"); |
Member
Author
There was a problem hiding this comment.
You realize that there is a pid file write and the start function doesn't restart if the pid file exists right?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)