feature: add gzip support for static files #143
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces gzip compression support for static file serving in a plugin system. A new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Plugin
participant FileSystem
participant Compressor
Client->>Plugin: Request static file
Plugin->>Client: Check Accept-Encoding header
alt Gzip supported
Plugin->>FileSystem: Check for existing gzip file
alt Gzip file not exists
Plugin->>Compressor: Compress file
Compressor->>FileSystem: Save compressed file
end
Plugin->>Client: Serve gzip file
else Gzip not supported
Plugin->>Client: Serve original file
end
Poem
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 (2)
config.go (1)
34-35: Add documentation for the Gzip field.Please add a comment explaining the purpose and behavior of the Gzip configuration option.
+ // Gzip enables compression of static files using gzip. + // When enabled, the plugin will create and serve .gz files for supported content. Gzip bool `mapstructure:"gzip"`plugin.go (1)
290-307: Add modification time check and disk space handling.The compressed file cache should be invalidated when the original file changes, and disk space issues should be handled gracefully.
func (s *Plugin) compressContent(originalFilePath string, originalFile http.File) (http.File, error) { compressedFilePath := originalFilePath + ".gz" - if _, err := os.Stat(compressedFilePath); os.IsNotExist(err) { + // Check if compressed file exists and is up to date + compressedInfo, err := os.Stat(s.cfg.Dir + compressedFilePath) + originalInfo, err := originalFile.Stat() + if err != nil || os.IsNotExist(err) || compressedInfo.ModTime().Before(originalInfo.ModTime()) { + // Check available disk space + var stat unix.Statfs_t + if err := unix.Statfs(s.cfg.Dir, &stat); err == nil { + // Require at least 100MB free + if stat.Bavail*uint64(stat.Bsize) < 100*1024*1024 { + s.log.Error("insufficient disk space for compression") + return originalFile, nil + } + } + if err := s.createCompressedFile(originalFile, compressedFilePath); err != nil { s.log.Debug("Error creating compressed file: ", zap.Error(err)) - return nil, err + // Fall back to serving uncompressed on error + return originalFile, nil } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config.go(1 hunks)plugin.go(5 hunks)
🔇 Additional comments (1)
plugin.go (1)
58-58: LGTM!The gzip field is properly initialized from the configuration.
Also applies to: 90-90
plugin.go
Outdated
| RootPluginName = "http" | ||
| ) | ||
|
|
||
| var cacheMutex sync.Mutex |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move cacheMutex to Plugin struct.
The global mutex could cause issues with testing and multiple plugin instances. Consider moving it to the Plugin struct.
-var cacheMutex sync.Mutex
type Plugin struct {
// ... other fields ...
+ cacheMutex sync.Mutex
gzip bool
}Committable suggestion skipped: line range outside the PR's diff.
plugin.go
Outdated
| if s.gzip && r.Header.Get("Accept-Encoding") == "gzip" { | ||
| s.log.Debug("gzip compression requested") | ||
| cf, err := s.compressContent(fp, f) | ||
| if err != nil { | ||
| s.log.Error("failed to compress content", zap.Error(err)) | ||
| http.Error(w, "Internal Server Error", http.StatusInternalServerError) | ||
| return | ||
| } | ||
| err = f.Close() | ||
| if err != nil { | ||
| s.log.Error("failed to close file", zap.Error(err)) | ||
| } | ||
| f = cf | ||
| // No new defer to close needed for cf. f is already closed, previous defer exist | ||
| w.Header().Set("Content-Encoding", "gzip") | ||
| } |
There was a problem hiding this comment.
Improve gzip compression handling.
Two issues need to be addressed:
- The Accept-Encoding check is too strict. Browsers can send multiple encodings.
- Missing check for already compressed content types (e.g., images, videos).
Apply this diff to fix the issues:
- if s.gzip && r.Header.Get("Accept-Encoding") == "gzip" {
+ if s.gzip && strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") {
+ // Skip compression for already compressed formats
+ contentType := http.DetectContentType(make([]byte, 512))
+ if strings.Contains(contentType, "image/") ||
+ strings.Contains(contentType, "video/") ||
+ strings.Contains(contentType, "audio/") {
+ s.log.Debug("skipping compression for already compressed content",
+ zap.String("type", contentType))
+ http.ServeContent(w, r, finfo.Name(), finfo.ModTime(), f)
+ return
+ }Committable suggestion skipped: line range outside the PR's diff.
plugin.go
Outdated
| func (s *Plugin) createCompressedFile(originalFile http.File, compressedPath string) error { | ||
| cacheMutex.Lock() | ||
| defer cacheMutex.Unlock() | ||
|
|
||
| // We check again to avoid a race between threads. | ||
| if _, err := os.Stat(compressedPath); err == nil { | ||
| return nil | ||
| } | ||
| compressedFile, err := os.Create(s.cfg.Dir + compressedPath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer func() { | ||
| err := compressedFile.Close() | ||
| if err != nil { | ||
| s.log.Error("failed to close file", zap.Error(err)) | ||
| } | ||
| }() | ||
|
|
||
| gz := gzip.NewWriter(compressedFile) | ||
| defer func() { | ||
| err := gz.Close() | ||
| if err != nil { | ||
| s.log.Error("failed to close file", zap.Error(err)) | ||
| } | ||
| }() | ||
|
|
||
| _, err = io.Copy(gz, originalFile) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Address security and resource management concerns.
Several issues need attention:
- No cleanup of partially written files on error
- Missing maximum file size check
- Potential path traversal vulnerability
Apply these improvements:
func (s *Plugin) createCompressedFile(originalFile http.File, compressedPath string) error {
+ // Prevent path traversal
+ if !strings.HasPrefix(compressedPath, "/") {
+ compressedPath = "/" + compressedPath
+ }
+ cleanPath := path.Clean(compressedPath)
+ if !strings.HasPrefix(cleanPath, "/") {
+ return fmt.Errorf("invalid path")
+ }
+
cacheMutex.Lock()
defer cacheMutex.Unlock()
- compressedFile, err := os.Create(s.cfg.Dir + compressedPath)
+ tempFile, err := os.CreateTemp(s.cfg.Dir, "gzip-*.tmp")
if err != nil {
return err
}
+ tempName := tempFile.Name()
defer func() {
- err := compressedFile.Close()
+ err := tempFile.Close()
if err != nil {
s.log.Error("failed to close file", zap.Error(err))
}
+ if err != nil {
+ _ = os.Remove(tempName)
+ }
}()
- gz := gzip.NewWriter(compressedFile)
+ // Limit the size of compressed files
+ maxSize := int64(10 * 1024 * 1024) // 10MB
+ limitedReader := io.LimitReader(originalFile, maxSize)
+
+ gz := gzip.NewWriter(tempFile)
defer func() {
err := gz.Close()
if err != nil {
s.log.Error("failed to close file", zap.Error(err))
}
}()
- _, err = io.Copy(gz, originalFile)
- return err
+ _, err = io.Copy(gz, limitedReader)
+ if err != nil {
+ return err
+ }
+
+ // Atomically rename the temp file to the final compressed file
+ return os.Rename(tempName, s.cfg.Dir+cleanPath)
}📝 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.
| func (s *Plugin) createCompressedFile(originalFile http.File, compressedPath string) error { | |
| cacheMutex.Lock() | |
| defer cacheMutex.Unlock() | |
| // We check again to avoid a race between threads. | |
| if _, err := os.Stat(compressedPath); err == nil { | |
| return nil | |
| } | |
| compressedFile, err := os.Create(s.cfg.Dir + compressedPath) | |
| if err != nil { | |
| return err | |
| } | |
| defer func() { | |
| err := compressedFile.Close() | |
| if err != nil { | |
| s.log.Error("failed to close file", zap.Error(err)) | |
| } | |
| }() | |
| gz := gzip.NewWriter(compressedFile) | |
| defer func() { | |
| err := gz.Close() | |
| if err != nil { | |
| s.log.Error("failed to close file", zap.Error(err)) | |
| } | |
| }() | |
| _, err = io.Copy(gz, originalFile) | |
| return err | |
| } | |
| func (s *Plugin) createCompressedFile(originalFile http.File, compressedPath string) error { | |
| // Prevent path traversal | |
| if !strings.HasPrefix(compressedPath, "/") { | |
| compressedPath = "/" + compressedPath | |
| } | |
| cleanPath := path.Clean(compressedPath) | |
| if !strings.HasPrefix(cleanPath, "/") { | |
| return fmt.Errorf("invalid path") | |
| } | |
| cacheMutex.Lock() | |
| defer cacheMutex.Unlock() | |
| // We check again to avoid a race between threads. | |
| if _, err := os.Stat(compressedPath); err == nil { | |
| return nil | |
| } | |
| tempFile, err := os.CreateTemp(s.cfg.Dir, "gzip-*.tmp") | |
| if err != nil { | |
| return err | |
| } | |
| tempName := tempFile.Name() | |
| defer func() { | |
| err := tempFile.Close() | |
| if err != nil { | |
| s.log.Error("failed to close file", zap.Error(err)) | |
| } | |
| if err != nil { | |
| _ = os.Remove(tempName) | |
| } | |
| }() | |
| // Limit the size of compressed files | |
| maxSize := int64(10 * 1024 * 1024) // 10MB | |
| limitedReader := io.LimitReader(originalFile, maxSize) | |
| gz := gzip.NewWriter(tempFile) | |
| defer func() { | |
| err := gz.Close() | |
| if err != nil { | |
| s.log.Error("failed to close file", zap.Error(err)) | |
| } | |
| }() | |
| _, err = io.Copy(gz, limitedReader) | |
| if err != nil { | |
| return err | |
| } | |
| // Atomically rename the temp file to the final compressed file | |
| return os.Rename(tempName, s.cfg.Dir+cleanPath) | |
| } |
rustatian
changed the title
plugin.go
Outdated
| var cacheMutex sync.Mutex | ||
|
|
There was a problem hiding this comment.
No global variables (this is a bad practice in Go) allowed
plugin.go
Outdated
| if err != nil { | ||
| s.log.Error("failed to close file", zap.Error(err)) | ||
| } | ||
| f = cf |
There was a problem hiding this comment.
I'm not sure what is that. You're closing a file and then reassign another file to the closed one to be closed by the initial defer?
plugin.go
Outdated
| } | ||
|
|
||
| func (s *Plugin) createCompressedFile(originalFile http.File, compressedPath string) error { | ||
| cacheMutex.Lock() |
plugin.go
Outdated
| // opentelemetry | ||
| prop propagation.TextMapPropagator | ||
|
|
||
| gzip bool |
There was a problem hiding this comment.
option without explanation comment. Also I think this is a bad name for the option, because what a gzip=true should do in the gzip middleware?
plugin.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| defer func() { |
There was a problem hiding this comment.
Too many unnecessarry defers. There're no errors returning by another function calls, to overload a stack with 2 defers in a row.
plugin.go
Outdated
| }() | ||
|
|
||
| gz := gzip.NewWriter(compressedFile) | ||
| defer func() { |
|
Hey @siamskoi 👋🏻 A few additional notes:
|
Introduce `gzipEnabled` and `gzipMaxFileSize` for fine-grained gzip compression settings. Added logic to skip gzip for already compressed content and enforce file size limits. Improved thread safety and path validation in file compression.
Introduce `gzipEnabled` and `gzipMaxFileSize` for fine-grained gzip compression settings. Added logic to skip gzip for already compressed content and enforce file size limits. Improved thread safety and path validation in file compression.
Replaced custom gzip logic with `github.com/klauspost/compress` to simplify compression handling and improve maintainability. Removed unused attributes and methods related to custom gzip implementation, such as max file size and custom file compression logic. This change streamlines the code and leverages the capabilities of an external, well-maintained library.
| Response map[string]string `mapstructure:"response"` | ||
|
|
||
| // GzipEnabled determines if gzip compression is enabled for serving static files. | ||
| GzipEnabled bool `mapstructure:"gzip_enabled"` |
There was a problem hiding this comment.
I'm not sure this is a good name for the option. From my POV option name should be self-explanatory.
I think that we may remove that option at all and check the gzip header only.
| if s.gzipEnabled && strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") { | ||
| s.log.Debug("gzip compression requested") | ||
| // Skip compression for already compressed formats | ||
| contentType := http.DetectContentType(make([]byte, 512)) |
There was a problem hiding this comment.
Are you detecting the content-type for the empty slice of bytes?
2 participants
Reason for This PR
Static file compression functionality is required. Fileserver is not suitable for this, as it requires nginx or apache for its use. With nginx or apache, it makes no sense to use the RR functionality for working with static files.
Description of Changes
The functionality of compressing static files using gzip has been added, the corresponding header is added to the response, a .gz file is created to reduce the load on the CPU for re-compressing files. Added a
gzipsetting to enable this functionality.curl -I -X GET --header "Accept-Encoding: gzip" http://127.0.0.1:8080/style.cssAnswer for small files:
And answer for big files:
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Bug Fixes
Performance