Implement pterodactyl 292 changes #158
Conversation
Add the same change as pterodactyl/wings#292 This adds configuration for the `machone-id` file that is required by hytale Creates and manages machine-id files on a per-server basis Adds code to remove machine-id files when a server is deleted as well. It also adds a group file for use along with the passwd file Updated config for passwd Updated mounts to not set default except for the the correct default.
📝 WalkthroughWalkthroughRestructures passwd configuration into nested structs and adds ConfigurePasswd(); introduces MachineID support (config, creation at server start, mount, and async cleanup on server deletion); adjusts mounts to include /etc/group and /etc/machine-id and moves passwd generation into a new function. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Router
participant Server
participant Config
participant FS as Filesystem
User->>Router: Request server start
Router->>Server: Start server
Server->>Config: Get System.MachineID settings
alt MachineID enabled
Server->>FS: write <server-id-without-dashes> -> <config.MachineID.Directory>/<server-id>
FS-->>Server: write OK / error
end
Server->>Config: Get passwd settings
alt Passwd enabled
Config->>FS: ConfigurePasswd() writes passwd & group files into configured directory
FS-->>Config: write OK / error
end
Server->>ContainerRuntime: Create container with mounts (/etc/passwd, /etc/group, /etc/machine-id)
ContainerRuntime-->>Server: container started
Note over Router,FS: On server deletion
Router->>FS: remove server files
Router->>FS: async remove machine-id file (spawned goroutine)
FS-->>Router: removal OK / warning logged on failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)server/server.go (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting 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 |
|
Just submitting this before I go to sleep. Will build and test on my end before I convert to a full commit. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Moved machine-id generation code outside of server create only called during initial server creation Create machine-id file for servers that already exists if the file is missing. Makes sure tempdir is created on wings start
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/root.go`:
- Around line 142-144: The call to config.ConfigurePasswd() logs a fatal error
with log.WithField(...).Fatal but omits the subsequent return used elsewhere for
consistency; after the existing log.WithField("error", err).Fatal("failed to
create passwd files for pelican") line, add a return to match the other error
handlers and keep control flow consistent (locate the call to
config.ConfigurePasswd and the log.WithField(...).Fatal invocation to insert the
return).
♻️ Duplicate comments (1)
config/config.go (1)
639-662: Directory creation logic is correct; static analysis concern is low-risk.The static analysis tool flagged these paths as "uncontrolled data used in path expression." While technically the paths come from configuration, they are admin-controlled via the config file (
/etc/pelican/config.yml), not from external user input. The risk is acceptable given the administrative nature of this configuration.However, consider adding basic path validation (e.g., ensuring paths are absolute and don't contain
..) as a defense-in-depth measure, especially forMachineID.Directorywhich will later be joined with server IDs.
🧹 Nitpick comments (3)
config/config.go (1)
666-688: ConfigurePasswd implementation is functional.The function correctly generates passwd and group files when enabled. A few observations:
File permissions (0644): Appropriate for passwd/group files since they need to be world-readable inside containers.
Named return with bare returns: While valid Go, consider using explicit returns for clarity, especially in error paths.
The static analysis warnings about path expressions are duplicates of past review comments - these paths are admin-controlled configuration values.
♻️ Optional: Explicit returns for clarity
func ConfigurePasswd() (err error) { if !_config.System.User.Passwd.Enable { - return + return nil } // ... file writes ... - return + return nil }router/router_server.go (1)
288-294: Consider checking if machine-id is enabled and ignoring "not found" errors.The cleanup goroutine runs unconditionally, but if
MachineID.Enableis false, the file won't exist. Additionally, if the file was already removed or never created, logging a warning is unnecessary noise.♻️ Proposed improvement
// remove hanging machine-id file for the server when removing go func(s *server.Server) { - if err := os.Remove(filepath.Join(config.Get().System.MachineID.Directory, s.ID())); err != nil { + if !config.Get().System.MachineID.Enable { + return + } + if err := os.Remove(filepath.Join(config.Get().System.MachineID.Directory, s.ID())); err != nil && !os.IsNotExist(err) { log.WithFields(log.Fields{"server_id": s.ID(), "error": err}). Warn("failed to remove machine-id file for server") } }(s)server/mounts.go (1)
33-59: Mount configuration looks correct.The implementation properly:
- Adds
/etc/passwdand/etc/groupmounts whenPasswd.Enableis true- Adds per-server
/etc/machine-idmount whenMachineID.Enableis true- Sets all new mounts as read-only (appropriate security posture)
- Removes
Default: truefrom passwd mount (correct - only/home/containershould be default)One consideration: if the source files don't exist (e.g.,
CreateMachineIDfailed earlier), the container will fail to start. This is probably the desired behavior, but worth noting.♻️ Optional: Cache config to reduce repeated calls
func (s *Server) Mounts() []environment.Mount { + cfg := config.Get() m := []environment.Mount{ { Default: true, Target: "/home/container", Source: s.Filesystem().Path(), ReadOnly: false, }, } - if config.Get().System.User.Passwd.Enable { + if cfg.System.User.Passwd.Enable { passwdMount := environment.Mount{ Target: "/etc/passwd", - Source: filepath.Join(config.Get().System.User.Passwd.Directory, "passwd"), + Source: filepath.Join(cfg.System.User.Passwd.Directory, "passwd"), ReadOnly: true, } // ... similar changes for other config.Get() calls
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/root.goconfig/config.gorouter/router_server.goserver/mounts.goserver/power.goserver/server.go
🧰 Additional context used
🧬 Code graph analysis (4)
server/power.go (2)
config/config.go (1)
Get(440-448)system/system.go (1)
System(60-67)
cmd/root.go (1)
config/config.go (1)
ConfigurePasswd(668-688)
server/mounts.go (3)
config/config.go (1)
Get(440-448)system/system.go (1)
System(60-67)environment/settings.go (1)
Mount(14-32)
server/server.go (1)
config/config.go (1)
Get(440-448)
🪛 GitHub Check: CodeQL
server/server.go
[failure] 334-334: Uncontrolled data used in path expression
This path depends on a user-provided value.
config/config.go
[failure] 640-640: Uncontrolled data used in path expression
This path depends on a user-provided value.
🔇 Additional comments (4)
config/config.go (1)
189-205: Configuration structure looks good.The restructuring of
Passwdfrom a boolean to a struct withEnableandDirectory, and the addition ofMachineIDwith similar fields, provides flexible configuration. The defaults (/etc/pelicanfor passwd,/etc/pelican/machine-idfor machine-id) are sensible.One minor observation: the
MachineID.Directoryfield is namedDirectorybut the comment on line 203 says "FilePath is the full path", which is slightly inconsistent. Consider updating the comment to match the actual field name.router/router_server.go (1)
283-286: Formatting change only - LGTM.server/power.go (1)
217-222: Machine-id creation in preflight looks correct.The placement ensures the machine-id file exists before the server container starts. The error is properly propagated, which will prevent the server from starting if machine-id creation fails. The
CreateMachineIDimplementation usesos.WriteFile(), which is idempotent—it creates the file if missing and overwrites it if it already exists, so repeated calls on restart pose no issues.server/server.go (1)
315-321: LGTM!The conditional machine-id creation is correctly placed after ensuring the data directory exists and before creating the environment. Error propagation is handled properly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
removes the append where not needed
|
This PR has been working flawlessly in my environment so far. All resources are created without any issues, and the machine ID is detected correctly as expected. The implementation feels clean and reliable, and I haven’t encountered any regressions or unexpected behavior. Great work on this improvement — it’s a solid enhancement to Wings. |
|
Multiple members of the community have been running the version that compiled with this PR. I am going to squash and merge the code. |
* Implement pterodactyl security fixes * Implement pterodactyl 292 changes (#158) * Implement pterodactyl 292 changes Add the same change as pterodactyl/wings#292 This adds configuration for the `machone-id` file that is required by hytale Creates and manages machine-id files on a per-server basis Adds code to remove machine-id files when a server is deleted as well. It also adds a group file for use along with the passwd file Updated config for passwd Updated mounts to not set default except for the the correct default. * Update machine-id generation Moved machine-id generation code outside of server create only called during initial server creation Create machine-id file for servers that already exists if the file is missing. Makes sure tempdir is created on wings start * remove append removes the append where not needed * Implement pterodactyl security fixes
2 participants
Add the same change as pterodactyl/wings#292
This adds configuration for the
machone-idfile that is required by hytaleCreates and manages machine-id files on a per-server basis
Adds code to remove machine-id files when a server is deleted as well.
Updated config for passwd that also adds a group file for use along with the passwd file
Updated mounts to not set default except for the the correct default.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.