Replace darkhttpd with cozystack-assets-server #596
Conversation
dosubot
bot
added
the
size:M
WalkthroughThis pull request introduces a new Go-based assets server to replace the existing Changes
Sequence DiagramsequenceDiagram
participant Client
participant AssetsServer
participant FileSystem
Client->>AssetsServer: HTTP Request
AssetsServer->>FileSystem: Resolve File Path
FileSystem-->>AssetsServer: Return File
AssetsServer->>Client: Serve File
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/installer/templates/cozystack.yaml (1)
70-75: Add container security context and resource limits.Consider adding:
- Security context to run as non-root
- Resource limits and requests
- name: assets image: "{{ .Values.cozystack.image }}" + securityContext: + runAsNonRoot: true + runAsUser: 1000 + readOnlyRootFilesystem: true + resources: + limits: + cpu: "0.5" + memory: "128Mi" + requests: + cpu: "0.1" + memory: "64Mi" command: - /usr/bin/cozystack-assets-server - "-dir=/cozystack/assets" - "-address=:8123"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/cozystack-assets-server/main.go(1 hunks)packages/core/installer/images/cozystack/Dockerfile(2 hunks)packages/core/installer/templates/cozystack.yaml(1 hunks)
🔇 Additional comments (3)
cmd/cozystack-assets-server/main.go (1)
3-8: LGTM! Clean and minimal imports.Using only standard library packages keeps the implementation simple and reduces external dependencies.
packages/core/installer/images/cozystack/Dockerfile (2)
24-24: LGTM! Build flags are secure and optimize binary size.The build flags look good:
-extldflags "-static": Ensures static linking-w: Disables DWARF generation-s: Disables symbol table
39-39: LGTM! Binary is correctly placed in the final image.The built server binary is correctly copied to
/usr/bin/in the final image.
| func main() { | ||
| addr := flag.String("address", ":8123", "Address to listen on") | ||
| dir := flag.String("dir", "/cozystack/assets", "Directory to serve files from") | ||
| flag.Parse() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for command-line flags.
Consider validating:
- Address format validity
- Directory existence and permissions before starting the server
func main() {
addr := flag.String("address", ":8123", "Address to listen on")
dir := flag.String("dir", "/cozystack/assets", "Directory to serve files from")
flag.Parse()
+
+ // Validate address format
+ host, port, err := net.SplitHostPort(*addr)
+ if err != nil {
+ log.Fatalf("Invalid address format %s: %v", *addr, err)
+ }
+
+ // Validate directory
+ if _, err := os.Stat(*dir); os.IsNotExist(err) {
+ log.Fatalf("Directory %s does not exist", *dir)
+ }Committable suggestion skipped: line range outside the PR's diff.
| absDir, err := filepath.Abs(*dir) | ||
| if err != nil { | ||
| log.Fatalf("Error getting absolute path for %s: %v", *dir, err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security measures for file serving.
Consider adding:
- Path traversal protection
- Explicit permission checks
absDir, err := filepath.Abs(*dir)
if err != nil {
log.Fatalf("Error getting absolute path for %s: %v", *dir, err)
}
+
+ // Ensure the directory has appropriate permissions
+ info, err := os.Stat(absDir)
+ if err != nil {
+ log.Fatalf("Error accessing directory %s: %v", absDir, err)
+ }
+ if !info.IsDir() {
+ log.Fatalf("%s is not a directory", absDir)
+ }
+ if info.Mode().Perm()&0444 == 0 {
+ log.Fatalf("Directory %s is not readable", absDir)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| absDir, err := filepath.Abs(*dir) | |
| if err != nil { | |
| log.Fatalf("Error getting absolute path for %s: %v", *dir, err) | |
| } | |
| absDir, err := filepath.Abs(*dir) | |
| if err != nil { | |
| log.Fatalf("Error getting absolute path for %s: %v", *dir, err) | |
| } | |
| // Ensure the directory has appropriate permissions | |
| info, err := os.Stat(absDir) | |
| if err != nil { | |
| log.Fatalf("Error accessing directory %s: %v", absDir, err) | |
| } | |
| if !info.IsDir() { | |
| log.Fatalf("%s is not a directory", absDir) | |
| } | |
| if info.Mode().Perm()&0444 == 0 { | |
| log.Fatalf("Directory %s is not readable", absDir) | |
| } |
| fs := http.FileServer(http.Dir(absDir)) | ||
| http.Handle("/", fs) | ||
|
|
||
| log.Printf("Server starting on %s, serving directory %s", *addr, absDir) | ||
|
|
||
| err = http.ListenAndServe(*addr, nil) | ||
| if err != nil { | ||
| log.Fatalf("Server failed to start: %v", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance server with production-ready features.
Consider adding:
- Graceful shutdown
- Basic security headers
- Custom 404 handling
+ // Create a wrapped file server with security headers
+ wrappedFs := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ w.Header().Set("X-Content-Type-Options", "nosniff")
+ w.Header().Set("X-Frame-Options", "DENY")
+ w.Header().Set("X-XSS-Protection", "1; mode=block")
+ fs.ServeHTTP(w, r)
+ })
+
- http.Handle("/", fs)
+ http.Handle("/", wrappedFs)
log.Printf("Server starting on %s, serving directory %s", *addr, absDir)
- err = http.ListenAndServe(*addr, nil)
- if err != nil {
- log.Fatalf("Server failed to start: %v", err)
- }
+ srv := &http.Server{
+ Addr: *addr,
+ Handler: nil,
+ }
+
+ // Graceful shutdown
+ go func() {
+ sigChan := make(chan os.Signal, 1)
+ signal.Notify(sigChan, os.Interrupt, syscall.SIGTERM)
+ <-sigChan
+ log.Println("Shutting down server...")
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ if err := srv.Shutdown(ctx); err != nil {
+ log.Printf("Server shutdown error: %v", err)
+ }
+ }()
+
+ if err := srv.ListenAndServe(); err != http.ErrServerClosed {
+ log.Fatalf("Server failed to start: %v", err)
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/core/installer/images/cozystack/Dockerfile (1)
24-24: Consider multi-stage build optimization.The build flags look good for size optimization. Consider adding CGO_ENABLED=0 for a fully static binary.
-RUN go build -o /cozystack-assets-server -ldflags '-extldflags "-static" -w -s' ./cmd/cozystack-assets-server +RUN CGO_ENABLED=0 go build -o /cozystack-assets-server -ldflags '-w -s' ./cmd/cozystack-assets-server
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/cozystack-assets-server/main.go(1 hunks)packages/core/installer/images/cozystack/Dockerfile(2 hunks)packages/core/installer/templates/cozystack.yaml(1 hunks)
| import ( | ||
| "flag" | ||
| "log" | ||
| "net/http" | ||
| "path/filepath" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding security-related imports.
The current implementation could benefit from additional security measures. Consider importing:
"compress/gzip"for compression"context"for graceful shutdown"os/signal"for signal handling
| fs := http.FileServer(http.Dir(absDir)) | ||
| http.Handle("/", fs) |
There was a problem hiding this comment.
Enhance security and performance of the file server.
The current implementation lacks several important features:
- Security:
- No path traversal protection
- No rate limiting
- No access logging
- Performance:
- No caching headers
- No compression support
Consider implementing a custom FileServer wrapper:
type secureFileServer struct {
root http.FileSystem
handler http.Handler
}
func (sfs *secureFileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
// Add security headers
w.Header().Set("X-Content-Type-Options", "nosniff")
// Add caching headers
w.Header().Set("Cache-Control", "public, max-age=3600")
// Basic rate limiting (consider using a proper rate limiter)
// Access logging
log.Printf("%s - %s %s %s", r.RemoteAddr, r.Method, r.URL.Path, r.UserAgent())
sfs.handler.ServeHTTP(w, r)
}
// Usage:
fs := &secureFileServer{
root: http.Dir(absDir),
handler: http.FileServer(http.Dir(absDir)),
}
http.Handle("/", fs)| log.Printf("Server starting on %s, serving directory %s", *addr, absDir) | ||
|
|
||
| err = http.ListenAndServe(*addr, nil) | ||
| if err != nil { | ||
| log.Fatalf("Server failed to start: %v", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement graceful shutdown.
The server lacks graceful shutdown capabilities, which could lead to connection interruptions during updates or restarts.
Add graceful shutdown support:
func main() {
// ... existing flag parsing code ...
srv := &http.Server{
Addr: *addr,
Handler: fs,
}
// Graceful shutdown
go func() {
sigChan := make(chan os.Signal, 1)
signal.Notify(sigChan, syscall.SIGTERM, syscall.SIGINT)
<-sigChan
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
if err := srv.Shutdown(ctx); err != nil {
log.Printf("HTTP server shutdown error: %v", err)
}
}()
log.Printf("Server starting on %s, serving directory %s", *addr, absDir)
if err := srv.ListenAndServe(); err != http.ErrServerClosed {
log.Fatalf("Server failed to start: %v", err)
}
}| FROM alpine:3.21 | ||
|
|
||
| RUN apk add --no-cache make darkhttpd | ||
| RUN apk add --no-cache make | ||
| RUN apk add helm kubectl --repository=https://dl-cdn.alpinelinux.org/alpine/edge/community | ||
|
|
||
| COPY scripts /cozystack/scripts | ||
| COPY --from=builder /src/packages/core /cozystack/packages/core | ||
| COPY --from=builder /src/packages/system /cozystack/packages/system | ||
| COPY --from=builder /src/_out/repos /cozystack/assets/repos | ||
| COPY --from=builder /src/_out/logos /cozystack/assets/logos | ||
| COPY --from=builder /cozystack-assets-server /usr/bin/cozystack-assets-server |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider running as non-root user.
For better security, consider running the server as a non-root user.
Add before the ENTRYPOINT:
RUN adduser -D -H -s /sbin/nologin cozystack
USER cozystack| - name: assets | ||
| image: "{{ .Values.cozystack.image }}" | ||
| command: | ||
| - /usr/bin/darkhttpd | ||
| - /cozystack/assets | ||
| - --port | ||
| - "8123" | ||
| - /usr/bin/cozystack-assets-server | ||
| - "-dir=/cozystack/assets" | ||
| - "-address=:8123" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add container security context and resource constraints.
The assets container configuration could be enhanced with security context and resource management:
- Security:
- Add non-root security context
- Add read-only root filesystem
- Resource Management:
- Add resource limits/requests
- Add liveness/readiness probes
Apply these changes:
- name: assets
image: "{{ .Values.cozystack.image }}"
command:
- /usr/bin/cozystack-assets-server
- "-dir=/cozystack/assets"
- "-address=:8123"
+ securityContext:
+ runAsNonRoot: true
+ runAsUser: 1000
+ readOnlyRootFilesystem: true
+ resources:
+ limits:
+ cpu: 200m
+ memory: 128Mi
+ requests:
+ cpu: 100m
+ memory: 64Mi
+ livenessProbe:
+ httpGet:
+ path: /
+ port: 8123
+ initialDelaySeconds: 5
+ periodSeconds: 10
+ readinessProbe:
+ httpGet:
+ path: /
+ port: 8123
+ initialDelaySeconds: 5
+ periodSeconds: 10📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: assets | |
| image: "{{ .Values.cozystack.image }}" | |
| command: | |
| - /usr/bin/darkhttpd | |
| - /cozystack/assets | |
| - --port | |
| - "8123" | |
| - /usr/bin/cozystack-assets-server | |
| - "-dir=/cozystack/assets" | |
| - "-address=:8123" | |
| - name: assets | |
| image: "{{ .Values.cozystack.image }}" | |
| command: | |
| - /usr/bin/cozystack-assets-server | |
| - "-dir=/cozystack/assets" | |
| - "-address=:8123" | |
| securityContext: | |
| runAsNonRoot: true | |
| runAsUser: 1000 | |
| readOnlyRootFilesystem: true | |
| resources: | |
| limits: | |
| cpu: 200m | |
| memory: 128Mi | |
| requests: | |
| cpu: 100m | |
| memory: 64Mi | |
| livenessProbe: | |
| httpGet: | |
| path: / | |
| port: 8123 | |
| initialDelaySeconds: 5 | |
| periodSeconds: 10 | |
| readinessProbe: | |
| httpGet: | |
| path: / | |
| port: 8123 | |
| initialDelaySeconds: 5 | |
| periodSeconds: 10 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
cmd/cozystack-assets-server/main.go (1)
15-18: Add directory existence check.The server should verify that the directory exists and is accessible before starting.
absDir, err := filepath.Abs(*dir) if err != nil { log.Fatalf("Error getting absolute path for %s: %v", *dir, err) } + + if _, err := os.Stat(absDir); os.IsNotExist(err) { + log.Fatalf("Directory %s does not exist", absDir) + }packages/core/installer/images/cozystack/Dockerfile (1)
24-24: Consider using Go's semantic version tags.The build command looks good with appropriate static linking and size optimization. Consider adding a version tag to the binary during build.
-RUN go build -o /cozystack-assets-server -ldflags '-extldflags "-static" -w -s' ./cmd/cozystack-assets-server +RUN go build -o /cozystack-assets-server -ldflags '-X main.version=v0.1.0 -extldflags "-static" -w -s' ./cmd/cozystack-assets-serverThen add version flag support in the Go code:
var version string flag.StringVar(&version, "version", version, "Show version information")packages/core/installer/templates/cozystack.yaml (1)
70-75: Add health checks.Consider adding liveness and readiness probes to ensure proper container health monitoring.
command: - /usr/bin/cozystack-assets-server - "-dir=/cozystack/assets" - "-address=:8123" + livenessProbe: + httpGet: + path: / + port: 8123 + initialDelaySeconds: 5 + periodSeconds: 10 + readinessProbe: + httpGet: + path: / + port: 8123 + initialDelaySeconds: 5 + periodSeconds: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/cozystack-assets-server/main.go(1 hunks)packages/core/installer/images/cozystack/Dockerfile(2 hunks)packages/core/installer/templates/cozystack.yaml(1 hunks)
| fs := http.FileServer(http.Dir(absDir)) | ||
| http.Handle("/", fs) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security headers and rate limiting.
The current implementation exposes files without essential security headers or rate limiting. Consider:
- Adding security headers (X-Content-Type-Options, X-Frame-Options, etc.)
- Implementing rate limiting to prevent DoS attacks
Here's a suggested implementation using middleware:
- fs := http.FileServer(http.Dir(absDir))
- http.Handle("/", fs)
+ fs := http.FileServer(http.Dir(absDir))
+ handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ w.Header().Set("X-Content-Type-Options", "nosniff")
+ w.Header().Set("X-Frame-Options", "DENY")
+ w.Header().Set("X-XSS-Protection", "1; mode=block")
+ fs.ServeHTTP(w, r)
+ })
+ http.Handle("/", handler)Consider adding rate limiting using a package like golang.org/x/time/rate.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fs := http.FileServer(http.Dir(absDir)) | |
| http.Handle("/", fs) | |
| fs := http.FileServer(http.Dir(absDir)) | |
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| w.Header().Set("X-Content-Type-Options", "nosniff") | |
| w.Header().Set("X-Frame-Options", "DENY") | |
| w.Header().Set("X-XSS-Protection", "1; mode=block") | |
| fs.ServeHTTP(w, r) | |
| }) | |
| http.Handle("/", handler) |
| err = http.ListenAndServe(*addr, nil) | ||
| if err != nil { | ||
| log.Fatalf("Server failed to start: %v", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add graceful shutdown handling.
The server lacks graceful shutdown handling, which could lead to connection interruptions during updates or termination.
Add context and signal handling:
+ ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
+ defer stop()
+
+ server := &http.Server{
+ Addr: *addr,
+ Handler: nil,
+ }
+
+ go func() {
+ <-ctx.Done()
+ shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ if err := server.Shutdown(shutdownCtx); err != nil {
+ log.Printf("Server shutdown error: %v", err)
+ }
+ }()
+
- err = http.ListenAndServe(*addr, nil)
+ err = server.ListenAndServe()
if err != nil && err != http.ErrServerClosed {
log.Fatalf("Server failed to start: %v", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = http.ListenAndServe(*addr, nil) | |
| if err != nil { | |
| log.Fatalf("Server failed to start: %v", err) | |
| } | |
| ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) | |
| defer stop() | |
| server := &http.Server{ | |
| Addr: *addr, | |
| Handler: nil, | |
| } | |
| go func() { | |
| <-ctx.Done() | |
| shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| if err := server.Shutdown(shutdownCtx); err != nil { | |
| log.Printf("Server shutdown error: %v", err) | |
| } | |
| }() | |
| err = server.ListenAndServe() | |
| if err != nil && err != http.ErrServerClosed { | |
| log.Fatalf("Server failed to start: %v", err) | |
| } |
| - name: assets | ||
| image: "{{ .Values.cozystack.image }}" | ||
| command: | ||
| - /usr/bin/darkhttpd | ||
| - /cozystack/assets | ||
| - --port | ||
| - "8123" | ||
| - /usr/bin/cozystack-assets-server | ||
| - "-dir=/cozystack/assets" | ||
| - "-address=:8123" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add resource limits and security context.
The assets container should have resource limits and a security context defined for better resource management and security.
- name: assets
image: "{{ .Values.cozystack.image }}"
+ resources:
+ limits:
+ cpu: "200m"
+ memory: "256Mi"
+ requests:
+ cpu: "100m"
+ memory: "128Mi"
+ securityContext:
+ readOnlyRootFilesystem: true
+ runAsNonRoot: true
+ runAsUser: 65534
+ allowPrivilegeEscalation: false
command:
- /usr/bin/cozystack-assets-server
- "-dir=/cozystack/assets"
- "-address=:8123"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: assets | |
| image: "{{ .Values.cozystack.image }}" | |
| command: | |
| - /usr/bin/darkhttpd | |
| - /cozystack/assets | |
| - --port | |
| - "8123" | |
| - /usr/bin/cozystack-assets-server | |
| - "-dir=/cozystack/assets" | |
| - "-address=:8123" | |
| - name: assets | |
| image: "{{ .Values.cozystack.image }}" | |
| resources: | |
| limits: | |
| cpu: "200m" | |
| memory: "256Mi" | |
| requests: | |
| cpu: "100m" | |
| memory: "128Mi" | |
| securityContext: | |
| readOnlyRootFilesystem: true | |
| runAsNonRoot: true | |
| runAsUser: 65534 | |
| allowPrivilegeEscalation: false | |
| command: | |
| - /usr/bin/cozystack-assets-server | |
| - "-dir=/cozystack/assets" | |
| - "-address=:8123" |
Summary by CodeRabbit
New Features
darkhttpdwith a custom Go-based file serverImprovements
Infrastructure