Non-kube network observer installation - #2532
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds non-Kubernetes Network Observer installation and uninstallation, including container and Prometheus configuration, systemd services, namespace cleanup integration, image updates, and new ChangesNetwork Observer deployment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant SystemCommand
participant Installer
participant ContainerRuntime
participant Systemd
Operator->>SystemCommand: run system network-observer
SystemCommand->>Installer: create and validate installer
SystemCommand->>Installer: install stack
Installer->>ContainerRuntime: pull, create, and start containers
Installer->>Systemd: create, enable, and start services
Installer-->>SystemCommand: return access URL
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5b694b6 to
8da7426
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
internal/nonkube/network-observer/systemd.go (2)
47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
--restart alwaysin container run commands.All three templates specify
--restart alwaysin thedocker run/podman runcommand while also using systemdRestart=always. On container exit, both the container runtime and systemd will attempt restarts, which can race. TheExecStartPrestop/rm mitigates this but adds unnecessary churn. Let systemd own the lifecycle and drop--restartfrom the container command.Also applies to: 79-79, 113-113
295-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse structured logging instead of
fmt.Printffor warnings.
RemoveServicesusesfmt.Printffor a warning message while the rest of the codebase usesslog. Consider injecting a logger intoSystemdServiceManageror accepting one as a parameter.internal/nonkube/network-observer/installer.go (2)
143-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winContainers are created and started directly, then immediately recreated by systemd.
installContainer()pulls, creates, and starts each container. ThencreateSystemdServices()starts systemd units whoseExecStartPrestops and removes those same containers beforeExecStartruns new ones. The direct container creation is redundant and causes unnecessary churn. If the intent is validation, consider only pulling images or creating containers without starting them.
409-409: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse structured logger instead of
fmt.Printf.
installContainerusesfmt.Printffor the "Pulled image" message while the rest of the installer usesslog.internal/cmd/skupper/system/kube/system_network-observer.go (1)
13-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused fields
namespace,user,passwordflagged by linter.These fields mirror the nonkube struct but are never set or read in the kube stub. Consider removing them or adding
//nolint:unusedcomments to keep linter output clean.Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba6e9269-549e-423f-b915-392d3f6b7123
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
go.modinternal/cmd/skupper/common/flags.gointernal/cmd/skupper/system/kube/system_network-observer.gointernal/cmd/skupper/system/nonkube/system_network-observer.gointernal/cmd/skupper/system/nonkube/system_network-observer_test.gointernal/cmd/skupper/system/system.gointernal/cmd/skupper/system/system_test.gointernal/images/image_utils.gointernal/images/images.gointernal/nonkube/network-observer/certificates.gointernal/nonkube/network-observer/config_templates.gointernal/nonkube/network-observer/containers.gointernal/nonkube/network-observer/installer.gointernal/nonkube/network-observer/password.gointernal/nonkube/network-observer/password_test.gointernal/nonkube/network-observer/systemd.go
…nstead of having it harcoded
…ed platform is podman
fgiorgetti
left a comment
fgiorgetti
left a comment
There was a problem hiding this comment.
@nluaces can we have an option to disable basic auth at all?
Eventually configure the host to be bound as well?
@fgiorgetti I was planning to remove basic auth, nginx and the generated certs like Christian suggested. Does it work for you? |
Yes @nluaces ! Thank you. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/nonkube/network-observer/installer.go (1)
134-151: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winBroken Authentication (CWE-306): Missing Authentication for Critical Function
Reachability: External
Protect the network-observer HTTP endpoint bound by the default installation
The nonkube installer starts the observer container with
-listen=:{{NetobsPort}}insystemd.goand returns anhttp://localhost:...URL ininstaller.go, so any local user accessing that address can reach the observer API without credentials. Add authentication for the observer endpoint and require TLS when the port cannot be guaranteed loopback-only; make binding to other interfaces opt-in.internal/nonkube/network-observer/systemd.go (1)
251-265: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop and disable every generated unit before deleting its files.
CreateServices()enables the main, Prometheus, and app units, butRemoveServices()only callsstopAndDisableService()for the main unit. Add the same unit not-found handling and iterate over all three units so the Prometheus and app enablement symlinks are removed and their containers are stopped before deleting the unit files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9801bc60-8b74-476b-bb4b-c0fb6a443253
📒 Files selected for processing (13)
internal/cmd/skupper/common/flags.gointernal/cmd/skupper/system/nonkube/system_network-observer.gointernal/cmd/skupper/system/nonkube/system_network-observer_test.gointernal/cmd/skupper/system/nonkube/system_uninstall.gointernal/cmd/skupper/system/nonkube/system_uninstall_test.gointernal/cmd/skupper/system/system.gointernal/cmd/skupper/system/system_test.gointernal/images/image_utils.gointernal/images/images.gointernal/nonkube/network-observer/config_templates.gointernal/nonkube/network-observer/containers.gointernal/nonkube/network-observer/installer.gointernal/nonkube/network-observer/systemd.go
💤 Files with no reviewable changes (5)
- internal/nonkube/network-observer/config_templates.go
- internal/images/images.go
- internal/cmd/skupper/common/flags.go
- internal/images/image_utils.go
- internal/cmd/skupper/system/system_test.go
3 participants
resolves #2489
New command:
Notes:
8443,9090,8080, and9000; like the kubernetes version. If any of them is not available it will be assigned the next one that is free.Summary by CodeRabbit
New Features
--uninstallsupport to remove Network Observer services and data by namespace.Updates
Limitations