From ae7fa147f7c90925a379d790ab0bf1040d805bb2 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 12 Nov 2025 15:29:57 +0000 Subject: [PATCH 01/14] third party config support --- internal/config/config.go | 43 ++++ internal/config/config_test.go | 7 + internal/config/defaults.go | 3 + internal/config/flags.go | 6 + internal/config/types.go | 39 ++-- internal/file/file_manager_service.go | 303 ++++++++++++++++++++++--- internal/file/file_service_operator.go | 17 ++ internal/model/config.go | 4 + internal/model/file.go | 1 + 9 files changed, 376 insertions(+), 47 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 038b0db78..a90d92af1 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -123,6 +123,7 @@ func ResolveConfig() (*Config, error) { Features: viperInstance.GetStringSlice(FeaturesKey), Labels: resolveLabels(), LibDir: viperInstance.GetString(LibDirPathKey), + ExternalDataSource: resolveExternalDataSource(), } defaultCollector(collector, config) @@ -434,6 +435,7 @@ func registerFlags() { registerCollectorFlags(fs) registerClientFlags(fs) registerDataPlaneFlags(fs) + registerExternalDataSourceFlags(fs) fs.SetNormalizeFunc(normalizeFunc) @@ -448,6 +450,24 @@ func registerFlags() { }) } +func registerExternalDataSourceFlags(fs *flag.FlagSet) { + fs.String( + ExternalDataSourceProxyUrlKey, + DefExternalDataSourceProxyUrl, + "Url to the proxy service to fetch the external file.", + ) + fs.StringSlice( + ExternalDataSourceAllowDomainsKey, + []string{}, + "List of allowed domains for external data sources.", + ) + fs.Int64( + ExternalDataSourceMaxBytesKey, + DefExternalDataSourceMaxBytes, + "Maximum size in bytes for external data sources.", + ) +} + func registerDataPlaneFlags(fs *flag.FlagSet) { fs.Duration( NginxReloadMonitoringPeriodKey, @@ -1506,3 +1526,26 @@ func areCommandServerProxyTLSSettingsSet() bool { viperInstance.IsSet(CommandServerProxyTLSSkipVerifyKey) || viperInstance.IsSet(CommandServerProxyTLSServerNameKey) } + +func resolveExternalDataSource() *ExternalDataSource { + proxyURLStruct := ProxyURL{ + URL: viperInstance.GetString(ExternalDataSourceProxyUrlKey), + } + externalDataSource := &ExternalDataSource{ + ProxyURL: proxyURLStruct, + AllowedDomains: viperInstance.GetStringSlice(ExternalDataSourceAllowDomainsKey), + MaxBytes: viperInstance.GetInt64(ExternalDataSourceMaxBytesKey), + } + + // Validate domains + if len(externalDataSource.AllowedDomains) > 0 { + for _, domain := range externalDataSource.AllowedDomains { + if strings.ContainsAny(domain, "/\\ ") || domain == "" { + slog.Error("domain is not specified in allowed_domains") + return nil + } + } + } + + return externalDataSource +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index cdbbcaf47..fd9071656 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1325,6 +1325,13 @@ func createConfig() *Config { config.FeatureCertificates, config.FeatureFileWatcher, config.FeatureMetrics, config.FeatureAPIAction, config.FeatureLogsNap, }, + ExternalDataSource: &ExternalDataSource{ + ProxyURL: ProxyURL{ + URL: "", + }, + AllowedDomains: nil, + MaxBytes: 0, + }, } } diff --git a/internal/config/defaults.go b/internal/config/defaults.go index 615c7bc8b..a9ef34c6d 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -111,6 +111,9 @@ const ( // File defaults DefLibDir = "/var/lib/nginx-agent" + + DefExternalDataSourceProxyUrl = "" + DefExternalDataSourceMaxBytes = 100 * 1024 * 1024 ) func DefaultFeatures() []string { diff --git a/internal/config/flags.go b/internal/config/flags.go index 697c2906e..1bd4b5a10 100644 --- a/internal/config/flags.go +++ b/internal/config/flags.go @@ -25,6 +25,7 @@ const ( InstanceHealthWatcherMonitoringFrequencyKey = "watchers_instance_health_watcher_monitoring_frequency" FileWatcherKey = "watchers_file_watcher" LibDirPathKey = "lib_dir" + ExternalDataSourceRootKey = "external_data_source" ) var ( @@ -138,6 +139,11 @@ var ( FileWatcherMonitoringFrequencyKey = pre(FileWatcherKey) + "monitoring_frequency" NginxExcludeFilesKey = pre(FileWatcherKey) + "exclude_files" + + ExternalDataSourceProxyKey = pre(ExternalDataSourceRootKey) + "proxy" + ExternalDataSourceProxyUrlKey = pre(ExternalDataSourceProxyKey) + "url" + ExternalDataSourceMaxBytesKey = pre(ExternalDataSourceRootKey) + "max_bytes" + ExternalDataSourceAllowDomainsKey = pre(ExternalDataSourceRootKey) + "allowed_domains" ) func pre(prefixes ...string) string { diff --git a/internal/config/types.go b/internal/config/types.go index fe3bc1773..e8dc88a4f 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -36,20 +36,21 @@ func parseServerType(str string) (ServerType, bool) { type ( Config struct { - Command *Command `yaml:"command" mapstructure:"command"` - AuxiliaryCommand *Command `yaml:"auxiliary_command" mapstructure:"auxiliary_command"` - Log *Log `yaml:"log" mapstructure:"log"` - DataPlaneConfig *DataPlaneConfig `yaml:"data_plane_config" mapstructure:"data_plane_config"` - Client *Client `yaml:"client" mapstructure:"client"` - Collector *Collector `yaml:"collector" mapstructure:"collector"` - Watchers *Watchers `yaml:"watchers" mapstructure:"watchers"` - Labels map[string]any `yaml:"labels" mapstructure:"labels"` - Version string `yaml:"-"` - Path string `yaml:"-"` - UUID string `yaml:"-"` - LibDir string `yaml:"-"` - AllowedDirectories []string `yaml:"allowed_directories" mapstructure:"allowed_directories"` - Features []string `yaml:"features" mapstructure:"features"` + Watchers *Watchers `yaml:"watchers" mapstructure:"watchers"` + Labels map[string]any `yaml:"labels" mapstructure:"labels"` + Log *Log `yaml:"log" mapstructure:"log"` + DataPlaneConfig *DataPlaneConfig `yaml:"data_plane_config" mapstructure:"data_plane_config"` + Client *Client `yaml:"client" mapstructure:"client"` + Collector *Collector `yaml:"collector" mapstructure:"collector"` + AuxiliaryCommand *Command `yaml:"auxiliary_command" mapstructure:"auxiliary_command"` + ExternalDataSource *ExternalDataSource `yaml:"external_data_source" mapstructure:"external_data_source"` + Command *Command `yaml:"command" mapstructure:"command"` + Path string `yaml:"-"` + Version string `yaml:"-"` + LibDir string `yaml:"-"` + UUID string `yaml:"-"` + AllowedDirectories []string `yaml:"allowed_directories" mapstructure:"allowed_directories"` + Features []string `yaml:"features" mapstructure:"features"` } Log struct { @@ -352,6 +353,16 @@ type ( Token string `yaml:"token,omitempty" mapstructure:"token"` Timeout time.Duration `yaml:"timeout" mapstructure:"timeout"` } + + ProxyURL struct { + URL string `yaml:"url" mapstructure:"url"` + } + + ExternalDataSource struct { + ProxyURL ProxyURL `yaml:"proxy" mapstructure:"proxy"` + AllowedDomains []string `yaml:"allowed_domains" mapstructure:"allowed_domains"` + MaxBytes int64 `yaml:"max_bytes" mapstructure:"max_bytes"` + } ) func (col *Collector) Validate(allowedDirectories []string) error { diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index 6c7ed4afb..5ada5f443 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -11,11 +11,16 @@ import ( "encoding/json" "errors" "fmt" + "io" "log/slog" + "net/http" + "net/url" "os" "path/filepath" "strconv" + "strings" "sync" + "time" "google.golang.org/grpc" @@ -39,6 +44,13 @@ const ( executePerm = 0o111 ) +const fileDownloadTimeout = 60 * time.Second + +type DownloadHeaders struct { + ETag string + LastModified string +} + type ( fileOperator interface { Write(ctx context.Context, fileContent []byte, fileName, filePermissions string) error @@ -73,6 +85,7 @@ type ( ) error SetIsConnected(isConnected bool) RenameFile(ctx context.Context, hash, fileName, tempDir string) error + RenameExternalFile(ctx context.Context, fileName, tempDir string) error UpdateClient(ctx context.Context, fileServiceClient mpi.FileServiceClient) } @@ -103,26 +116,28 @@ type FileManagerService struct { // map of files and the actions performed on them during config apply fileActions map[string]*model.FileCache // key is file path // map of the files currently on disk, used to determine the file action during config apply - currentFilesOnDisk map[string]*mpi.File // key is file path - previousManifestFiles map[string]*model.ManifestFile - manifestFilePath string - rollbackManifest bool - filesMutex sync.RWMutex + currentFilesOnDisk map[string]*mpi.File // key is file path + previousManifestFiles map[string]*model.ManifestFile + newExternalFileHeaders map[string]DownloadHeaders + manifestFilePath string + rollbackManifest bool + filesMutex sync.RWMutex } func NewFileManagerService(fileServiceClient mpi.FileServiceClient, agentConfig *config.Config, manifestLock *sync.RWMutex, ) *FileManagerService { return &FileManagerService{ - agentConfig: agentConfig, - fileOperator: NewFileOperator(manifestLock), - fileServiceOperator: NewFileServiceOperator(agentConfig, fileServiceClient, manifestLock), - fileActions: make(map[string]*model.FileCache), - currentFilesOnDisk: make(map[string]*mpi.File), - previousManifestFiles: make(map[string]*model.ManifestFile), - rollbackManifest: true, - manifestFilePath: agentConfig.LibDir + "/manifest.json", - manifestLock: manifestLock, + agentConfig: agentConfig, + fileOperator: NewFileOperator(manifestLock), + fileServiceOperator: NewFileServiceOperator(agentConfig, fileServiceClient, manifestLock), + fileActions: make(map[string]*model.FileCache), + currentFilesOnDisk: make(map[string]*mpi.File), + previousManifestFiles: make(map[string]*model.ManifestFile), + newExternalFileHeaders: make(map[string]DownloadHeaders), + rollbackManifest: true, + manifestFilePath: agentConfig.LibDir + "/manifest.json", + manifestLock: manifestLock, } } @@ -232,7 +247,7 @@ func (fms *FileManagerService) Rollback(ctx context.Context, instanceID string) delete(fms.currentFilesOnDisk, fileAction.File.GetFileMeta().GetName()) continue - case model.Delete, model.Update: + case model.Delete, model.Update, model.ExternalFile: content, err := fms.restoreFiles(fileAction) if err != nil { return err @@ -387,13 +402,16 @@ func (fms *FileManagerService) DetermineFileActions( modifiedFile.Action = model.Add fileDiff[fileName] = modifiedFile - continue // if file currently exists and file hash is different, file has been updated // copy contents, set file action } else if ok && modifiedFile.File.GetFileMeta().GetHash() != currentFile.GetFileMeta().GetHash() { modifiedFile.Action = model.Update fileDiff[fileName] = modifiedFile } + if modifiedFile.File.GetExternalDataSource() != nil || currentFile.GetExternalDataSource() != nil { + modifiedFile.Action = model.ExternalFile + fileDiff[fileName] = modifiedFile + } } return fileDiff, nil @@ -571,33 +589,40 @@ func (fms *FileManagerService) executeFileActions(ctx context.Context) (actionEr func (fms *FileManagerService) downloadUpdatedFilesToTempLocation(ctx context.Context) (updateError error) { for _, fileAction := range fms.fileActions { - if fileAction.Action == model.Add || fileAction.Action == model.Update { - tempFilePath := tempFilePath(fileAction.File.GetFileMeta().GetName()) + tempFilePath := tempFilePath(fileAction.File.GetFileMeta().GetName()) + switch fileAction.Action { + case model.ExternalFile: + updateError = fms.handleExternalFileDownload(ctx, fileAction, tempFilePath) + case model.Add, model.Update: slog.DebugContext( ctx, "Downloading file to temp location", "file", tempFilePath, ) + updateError = fms.fileUpdate(ctx, fileAction.File, tempFilePath) + case model.Delete, model.Unchanged: + continue + } - updateErr := fms.fileUpdate(ctx, fileAction.File, tempFilePath) - if updateErr != nil { - updateError = updateErr - break - } + if updateError != nil { + return updateError } } - return updateError + return nil } func (fms *FileManagerService) moveOrDeleteFiles(ctx context.Context, actionError error) error { actionsLoop: for _, fileAction := range fms.fileActions { + var err error + fileMeta := fileAction.File.GetFileMeta() + tempFilePath := tempFilePath(fileAction.File.GetFileMeta().GetName()) switch fileAction.Action { case model.Delete: slog.DebugContext(ctx, "Deleting file", "file", fileAction.File.GetFileMeta().GetName()) - if err := os.Remove(fileAction.File.GetFileMeta().GetName()); err != nil && !os.IsNotExist(err) { + if err = os.Remove(fileAction.File.GetFileMeta().GetName()); err != nil && !os.IsNotExist(err) { actionError = fmt.Errorf("error deleting file: %s error: %w", fileAction.File.GetFileMeta().GetName(), err) @@ -606,17 +631,16 @@ actionsLoop: continue case model.Add, model.Update: - fileMeta := fileAction.File.GetFileMeta() - tempFilePath := tempFilePath(fileAction.File.GetFileMeta().GetName()) - err := fms.fileServiceOperator.RenameFile(ctx, fileMeta.GetHash(), tempFilePath, fileMeta.GetName()) - if err != nil { - actionError = err - - break actionsLoop - } + err = fms.fileServiceOperator.RenameFile(ctx, fileMeta.GetHash(), tempFilePath, fileMeta.GetName()) + case model.ExternalFile: + err = fms.fileServiceOperator.RenameExternalFile(ctx, tempFilePath, fileMeta.GetName()) case model.Unchanged: slog.DebugContext(ctx, "File unchanged") } + if err != nil { + actionError = err + break actionsLoop + } } return actionError @@ -772,3 +796,216 @@ func tempBackupFilePath(fileName string) string { tempFileName := "." + filepath.Base(fileName) + ".agent.backup" return filepath.Join(filepath.Dir(fileName), tempFileName) } + +func (fms *FileManagerService) handleExternalFileDownload(ctx context.Context, fileAction *model.FileCache, + tempFilePath string, +) error { + location := fileAction.File.GetExternalDataSource().GetLocation() + slog.InfoContext(ctx, "Downloading external file from", "location", location) + + var contentToWrite []byte + var downloadErr, updateError error + var headers DownloadHeaders + + contentToWrite, headers, downloadErr = fms.downloadFileContent(ctx, fileAction.File) + + if downloadErr != nil { + updateError = fmt.Errorf("failed to download file %s from %s: %w", + fileAction.File.GetFileMeta().GetName(), location, downloadErr) + + return updateError + } + + if contentToWrite == nil { + slog.DebugContext(ctx, "External file unchanged (304), skipping disk write.", + "file", fileAction.File.GetFileMeta().GetName()) + return nil + } + + fileName := fileAction.File.GetFileMeta().GetName() + fms.newExternalFileHeaders[fileName] = headers + + updateErr := fms.writeContentToTempFile(ctx, contentToWrite, tempFilePath) + + return updateErr +} + +func (fms *FileManagerService) writeContentToTempFile( + ctx context.Context, + content []byte, + path string, +) error { + writeErr := fms.fileOperator.Write( + ctx, + content, + path, + "0600", + ) + + if writeErr != nil { + return fmt.Errorf("failed to write downloaded content to temp file %s: %w", path, writeErr) + } + + return nil +} + +// downloadFileContent performs an HTTP GET request to the given URL and returns the file content as a byte slice. +func (fms *FileManagerService) downloadFileContent( + ctx context.Context, + file *mpi.File, +) (content []byte, headers DownloadHeaders, err error) { + fileName := file.GetFileMeta().GetName() + downloadURL := file.GetExternalDataSource().GetLocation() + externalConfig := fms.agentConfig.ExternalDataSource + + if !isDomainAllowed(downloadURL, externalConfig.AllowedDomains) { + return nil, DownloadHeaders{}, fmt.Errorf("download URL %s is not in the allowed domains list", downloadURL) + } + + httpClient, err := fms.setupHTTPClient(ctx, externalConfig.ProxyURL.URL) + if err != nil { + return nil, DownloadHeaders{}, err + } + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, downloadURL, nil) + if err != nil { + return nil, DownloadHeaders{}, fmt.Errorf("failed to create request for %s: %w", downloadURL, err) + } + + if externalConfig.ProxyURL.URL != "" { + fms.addConditionalHeaders(ctx, req, fileName) + } else { + slog.DebugContext(ctx, "No proxy configured; sending plain HTTP request without caching headers.") + } + + resp, err := httpClient.Do(req) + if err != nil { + return nil, DownloadHeaders{}, fmt.Errorf("failed to execute download request for %s: %w", downloadURL, err) + } + defer resp.Body.Close() + + switch resp.StatusCode { + case http.StatusOK: + headers.ETag = resp.Header.Get("ETag") + headers.LastModified = resp.Header.Get("Last-Modified") + case http.StatusNotModified: + slog.InfoContext(ctx, "File content unchanged (304 Not Modified)", "file_name", fileName) + return nil, DownloadHeaders{}, nil + default: + return nil, DownloadHeaders{}, fmt.Errorf("download failed with status code %d", resp.StatusCode) + } + + reader := io.Reader(resp.Body) + if fms.agentConfig.ExternalDataSource.MaxBytes > 0 { + reader = io.LimitReader(resp.Body, fms.agentConfig.ExternalDataSource.MaxBytes) + } + + content, err = io.ReadAll(reader) + if err != nil { + return nil, DownloadHeaders{}, fmt.Errorf("failed to read content from response body: %w", err) + } + + slog.InfoContext(ctx, "Successfully downloaded file content", "file_name", fileName, "size", len(content)) + + return content, headers, nil +} + +func isDomainAllowed(downloadURL string, allowedDomains []string) bool { + u, err := url.Parse(downloadURL) + if err != nil { + slog.Debug("Failed to parse download URL for domain check", "url", downloadURL, "error", err) + return false + } + + hostname := u.Hostname() + if hostname == "" { + return false + } + + for _, pattern := range allowedDomains { + if pattern == "" { + continue + } + + if pattern == hostname { + return true + } + + if isWildcardMatch(hostname, pattern) { + return true + } + } + + return false +} + +func (fms *FileManagerService) setupHTTPClient(ctx context.Context, proxyURLString string) (*http.Client, error) { + var transport *http.Transport + + if proxyURLString != "" { + proxyURL, err := url.Parse(proxyURLString) + if err != nil { + return nil, fmt.Errorf("invalid proxy URL configured: %w", err) + } + slog.DebugContext(ctx, "Configuring HTTP client to use proxy", "proxy_url", proxyURLString) + transport = &http.Transport{ + Proxy: http.ProxyURL(proxyURL), + } + } else { + slog.DebugContext(ctx, "Configuring HTTP client for direct connection (no proxy)") + transport = &http.Transport{ + Proxy: nil, + } + } + + httpClient := &http.Client{ + Transport: transport, + Timeout: fileDownloadTimeout, + } + + return httpClient, nil +} + +func (fms *FileManagerService) addConditionalHeaders(ctx context.Context, req *http.Request, fileName string) { + slog.DebugContext(ctx, "Proxy configured; adding headers to GET request.") + + manifestFiles, _, manifestFileErr := fms.manifestFile() + + if manifestFileErr != nil && !errors.Is(manifestFileErr, os.ErrNotExist) { + slog.WarnContext(ctx, "Error reading manifest file for headers", "error", manifestFileErr) + } + + manifestFile, ok := manifestFiles[fileName] + + if ok && manifestFile != nil && manifestFile.ManifestFileMeta != nil { + fileMeta := manifestFile.ManifestFileMeta + + if fileMeta.ETag != "" { + req.Header.Set("If-None-Match", fileMeta.ETag) + } + if fileMeta.LastModified != "" { + req.Header.Set("If-Modified-Since", fileMeta.LastModified) + } + } else { + slog.DebugContext(ctx, "File not found in manifest or missing metadata; skipping conditional headers.", + "file", fileName) + } +} + +func isWildcardMatch(hostname, pattern string) bool { + if !strings.HasPrefix(pattern, "*.") { + return false + } + + baseDomain := pattern[2:] + if strings.HasSuffix(hostname, baseDomain) { + // Check to ensure it's a true subdomain match (e.g., must have a '.' + // before baseDomain unless it IS the baseDomain) + // This handles cases like preventing 'foo.com' matching '*.oo.com' + if hostname == baseDomain || hostname[len(hostname)-len(baseDomain)-1] == '.' { + return true + } + } + + return false +} diff --git a/internal/file/file_service_operator.go b/internal/file/file_service_operator.go index 19211c600..339c98eec 100644 --- a/internal/file/file_service_operator.go +++ b/internal/file/file_service_operator.go @@ -276,6 +276,23 @@ func (fso *FileServiceOperator) UpdateFile( return fso.sendUpdateFileStream(ctx, fileToUpdate, fso.agentConfig.Client.Grpc.FileChunkSize) } +func (fso *FileServiceOperator) RenameExternalFile( + ctx context.Context, source, desination string, +) error { + slog.DebugContext(ctx, fmt.Sprintf("Moving file %s to %s (no hash validation)", source, desination)) + + if err := os.MkdirAll(filepath.Dir(desination), dirPerm); err != nil { + return fmt.Errorf("failed to create directories for %s: %w", desination, err) + } + + moveErr := os.Rename(source, desination) + if moveErr != nil { + return fmt.Errorf("failed to move file: %w", moveErr) + } + + return nil +} + // renameFile, renames (moves) file from tempDir to new location to update file. func (fso *FileServiceOperator) RenameFile( ctx context.Context, hash, source, desination string, diff --git a/internal/model/config.go b/internal/model/config.go index 78c764ce0..98cc6705c 100644 --- a/internal/model/config.go +++ b/internal/model/config.go @@ -47,6 +47,10 @@ type ManifestFileMeta struct { Referenced bool `json:"referenced"` // File is not managed by the agent Unmanaged bool `json:"unmanaged"` + // ETag of the 3rd Party external file + ETag string `json:"etag"` + // Last modified time of the 3rd Party external file + LastModified string `json:"last_modified"` } type ConfigApplyMessage struct { Error error diff --git a/internal/model/file.go b/internal/model/file.go index fc6c5baca..671a4ff85 100644 --- a/internal/model/file.go +++ b/internal/model/file.go @@ -19,4 +19,5 @@ const ( Update Delete Unchanged + ExternalFile ) From 14f1190b612f12ffb177d17e69371ec91a72d6cc Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 12 Nov 2025 15:43:35 +0000 Subject: [PATCH 02/14] lint fix --- internal/file/file_manager_service.go | 1 + internal/model/config.go | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index 5ada5f443..8615b444a 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -819,6 +819,7 @@ func (fms *FileManagerService) handleExternalFileDownload(ctx context.Context, f if contentToWrite == nil { slog.DebugContext(ctx, "External file unchanged (304), skipping disk write.", "file", fileAction.File.GetFileMeta().GetName()) + return nil } diff --git a/internal/model/config.go b/internal/model/config.go index 98cc6705c..5c5feb11a 100644 --- a/internal/model/config.go +++ b/internal/model/config.go @@ -41,16 +41,16 @@ type ManifestFileMeta struct { Name string `json:"name"` // The hash of the file contents sha256, hex encoded Hash string `json:"hash"` + // ETag of the 3rd Party external file + ETag string `json:"etag"` + // Last modified time of the 3rd Party external file + LastModified string `json:"last_modified"` // The size of the file in bytes Size int64 `json:"size"` // File referenced in the NGINX config Referenced bool `json:"referenced"` // File is not managed by the agent Unmanaged bool `json:"unmanaged"` - // ETag of the 3rd Party external file - ETag string `json:"etag"` - // Last modified time of the 3rd Party external file - LastModified string `json:"last_modified"` } type ConfigApplyMessage struct { Error error From d31b2df6146872881599c0e4d6b64a70bdcbda0b Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Thu, 13 Nov 2025 12:12:30 +0000 Subject: [PATCH 03/14] Added a generic file timeout --- internal/config/config.go | 6 ++++++ internal/config/defaults.go | 2 ++ internal/config/flags.go | 1 + internal/config/types.go | 7 ++++--- internal/file/file_manager_service.go | 5 +---- 5 files changed, 14 insertions(+), 7 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 80d201180..1ba2c5d1e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -648,6 +648,11 @@ func registerClientFlags(fs *flag.FlagSet) { DefMaxFileSize, "Max file size in bytes.", ) + fs.Duration( + ClientFileDownloadTimeoutKey, + DefClientFileDownloadTimeout, + "Timeout value in seconds, to downloading file for config apply.", + ) } func registerCommandFlags(fs *flag.FlagSet) { @@ -1133,6 +1138,7 @@ func resolveClient() *Client { RandomizationFactor: viperInstance.GetFloat64(ClientBackoffRandomizationFactorKey), Multiplier: viperInstance.GetFloat64(ClientBackoffMultiplierKey), }, + FileDownloadTimeout: viperInstance.GetDuration(ClientFileDownloadTimeoutKey), } } diff --git a/internal/config/defaults.go b/internal/config/defaults.go index 6035ec4fc..545e37a24 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -81,6 +81,8 @@ const ( DefBackoffMaxInterval = 20 * time.Second DefBackoffMaxElapsedTime = 1 * time.Minute + DefClientFileDownloadTimeout = 60 * time.Second + // Watcher defaults DefInstanceWatcherMonitoringFrequency = 5 * time.Second DefInstanceHealthWatcherMonitoringFrequency = 5 * time.Second diff --git a/internal/config/flags.go b/internal/config/flags.go index 21517cb62..953ce7f0d 100644 --- a/internal/config/flags.go +++ b/internal/config/flags.go @@ -47,6 +47,7 @@ var ( ClientBackoffMaxElapsedTimeKey = pre(ClientRootKey) + "backoff_max_elapsed_time" ClientBackoffRandomizationFactorKey = pre(ClientRootKey) + "backoff_randomization_factor" ClientBackoffMultiplierKey = pre(ClientRootKey) + "backoff_multiplier" + ClientFileDownloadTimeoutKey = pre(ClientRootKey) + "file_download_timeout" CollectorConfigPathKey = pre(CollectorRootKey) + "config_path" CollectorAdditionalConfigPathsKey = pre(CollectorRootKey) + "additional_config_paths" diff --git a/internal/config/types.go b/internal/config/types.go index 39488e38d..6aff4cd78 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -75,9 +75,10 @@ type ( } Client struct { - HTTP *HTTP `yaml:"http" mapstructure:"http"` - Grpc *GRPC `yaml:"grpc" mapstructure:"grpc"` - Backoff *BackOff `yaml:"backoff" mapstructure:"backoff"` + HTTP *HTTP `yaml:"http" mapstructure:"http"` + Grpc *GRPC `yaml:"grpc" mapstructure:"grpc"` + Backoff *BackOff `yaml:"backoff" mapstructure:"backoff"` + FileDownloadTimeout time.Duration `yaml:"file_download_timeout" mapstructure:"file_download_timeout"` } HTTP struct { diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index 8615b444a..e60c96d2c 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -20,7 +20,6 @@ import ( "strconv" "strings" "sync" - "time" "google.golang.org/grpc" @@ -44,8 +43,6 @@ const ( executePerm = 0o111 ) -const fileDownloadTimeout = 60 * time.Second - type DownloadHeaders struct { ETag string LastModified string @@ -961,7 +958,7 @@ func (fms *FileManagerService) setupHTTPClient(ctx context.Context, proxyURLStri httpClient := &http.Client{ Transport: transport, - Timeout: fileDownloadTimeout, + Timeout: fms.agentConfig.Client.FileDownloadTimeout, } return httpClient, nil From 501e08f665995ae054708bf3da386650e7d065a1 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Thu, 13 Nov 2025 15:48:00 +0000 Subject: [PATCH 04/14] added generic timeout to create connection --- internal/file/file_service_operator.go | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/internal/file/file_service_operator.go b/internal/file/file_service_operator.go index 339c98eec..8dba2acf7 100644 --- a/internal/file/file_service_operator.go +++ b/internal/file/file_service_operator.go @@ -79,7 +79,10 @@ func (fso *FileServiceOperator) File( defer backoffCancel() getFile := func() (*mpi.GetFileResponse, error) { - return fso.fileServiceClient.GetFile(ctx, &mpi.GetFileRequest{ + grpcCtx, cancel := context.WithTimeout(ctx, fso.agentConfig.Client.FileDownloadTimeout) + defer cancel() + + return fso.fileServiceClient.GetFile(grpcCtx, &mpi.GetFileRequest{ MessageMeta: &mpi.MessageMeta{ MessageId: id.GenerateMessageID(), CorrelationId: logger.CorrelationID(ctx), @@ -225,7 +228,10 @@ func (fso *FileServiceOperator) ChunkedFile( ) error { slog.DebugContext(ctx, "Getting chunked file", "file", file.GetFileMeta().GetName()) - stream, err := fso.fileServiceClient.GetFileStream(ctx, &mpi.GetFileRequest{ + grpcCtx, cancel := context.WithTimeout(ctx, fso.agentConfig.Client.FileDownloadTimeout) + defer cancel() + + stream, err := fso.fileServiceClient.GetFileStream(grpcCtx, &mpi.GetFileRequest{ MessageMeta: &mpi.MessageMeta{ MessageId: id.GenerateMessageID(), CorrelationId: logger.CorrelationID(ctx), @@ -388,12 +394,15 @@ func (fso *FileServiceOperator) sendUpdateFileRequest( return nil, errors.New("CreateConnection rpc has not being called yet") } - response, updateError := fso.fileServiceClient.UpdateFile(ctx, request) + grpcCtx, cancel := context.WithTimeout(ctx, fso.agentConfig.Client.FileDownloadTimeout) + defer cancel() + + response, updateError := fso.fileServiceClient.UpdateFile(grpcCtx, request) validatedError := internalgrpc.ValidateGrpcError(updateError) if validatedError != nil { - slog.ErrorContext(ctx, "Failed to send update file", "error", validatedError) + slog.ErrorContext(grpcCtx, "Failed to send update file", "error", validatedError) return nil, validatedError } @@ -423,7 +432,10 @@ func (fso *FileServiceOperator) sendUpdateFileStream( return errors.New("file chunk size must be greater than zero") } - updateFileStreamClient, err := fso.fileServiceClient.UpdateFileStream(ctx) + grpcCtx, cancel := context.WithTimeout(ctx, fso.agentConfig.Client.FileDownloadTimeout) + defer cancel() + + updateFileStreamClient, err := fso.fileServiceClient.UpdateFileStream(grpcCtx) if err != nil { return err } From b7eaef745ab1c482fdce3d21357a008692fba4ca Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 19 Nov 2025 07:39:21 +0000 Subject: [PATCH 05/14] added uT --- internal/config/config.go | 25 ++++++++---- internal/config/config_test.go | 70 ++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 8 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index c5fcb0900..26c6ed81d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1597,15 +1597,24 @@ func resolveExternalDataSource() *ExternalDataSource { MaxBytes: viperInstance.GetInt64(ExternalDataSourceMaxBytesKey), } - // Validate domains - if len(externalDataSource.AllowedDomains) > 0 { - for _, domain := range externalDataSource.AllowedDomains { - if strings.ContainsAny(domain, "/\\ ") || domain == "" { - slog.Error("domain is not specified in allowed_domains") - return nil - } - } + if err := validateAllowedDomains(externalDataSource.AllowedDomains); err != nil { + return nil } return externalDataSource } + +func validateAllowedDomains(domains []string) error { + if len(domains) == 0 { + return nil + } + + for _, domain := range domains { + if strings.ContainsAny(domain, "/\\ ") || domain == "" { + slog.Error("domain is not specified in allowed_domains") + return errors.New("invalid domain found in allowed_domains") + } + } + + return nil +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 55534cd41..b35818ebe 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1576,3 +1576,73 @@ func TestValidateLabel(t *testing.T) { }) } } + +func TestValidateAllowedDomains(t *testing.T) { + tests := []struct { + name string + domains []string + wantErr bool + }{ + { + name: "Test 1: Success: Empty slice", + domains: []string{}, + wantErr: false, + }, + { + name: "Test 2: Success: Nil slice", + domains: nil, + wantErr: false, + }, + { + name: "Test 3: Success: Valid domains", + domains: []string{"example.com", "api.nginx.com", "sub.domain.io"}, + wantErr: false, + }, + { + name: "Test 4: Failure: Domain contains space", + domains: []string{"valid.com", "bad domain.com"}, + wantErr: true, + }, + { + name: "Test 5: Failure: Empty string domain", + domains: []string{"valid.com", ""}, + wantErr: true, + }, + { + name: "Test 6: Failure: Domain contains forward slash /", + domains: []string{"domain.com/path"}, + wantErr: true, + }, + { + name: "Test 7: Failure: Domain contains backward slash \\", + domains: []string{"domain.com\\path"}, + wantErr: true, + }, + { + name: "Test 8: Failure: Mixed valid and invalid (first is invalid)", + domains: []string{" only.com", "good.com"}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var logBuffer bytes.Buffer + logHandler := slog.NewTextHandler(&logBuffer, &slog.HandlerOptions{Level: slog.LevelError}) + + originalLogger := slog.Default() + slog.SetDefault(slog.New(logHandler)) + defer slog.SetDefault(originalLogger) + + actualErr := validateAllowedDomains(tt.domains) + + if tt.wantErr { + require.Error(t, actualErr, "Expected an error but got nil.") + assert.Contains(t, logBuffer.String(), "domain is not specified in allowed_domains", + "Expected the error log message to be present in the output.") + } else { + assert.NoError(t, actualErr, "Did not expect an error but got one: %v", actualErr) + } + }) + } +} From a83e45b9841b152e7e5fc6e7fd7f46702d5f9de3 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 19 Nov 2025 09:54:14 +0000 Subject: [PATCH 06/14] Added UT --- internal/file/file_manager_service_test.go | 176 +++++++++++++++++++++ 1 file changed, 176 insertions(+) diff --git a/internal/file/file_manager_service_test.go b/internal/file/file_manager_service_test.go index e42128063..4052a89d5 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -10,11 +10,15 @@ import ( "encoding/json" "errors" "fmt" + "net/http" + "net/http/httptest" + "net/url" "os" "path/filepath" "sync" "testing" + "github.com/nginx/agent/v3/internal/config" "github.com/nginx/agent/v3/internal/model" "github.com/nginx/agent/v3/pkg/files" @@ -1173,3 +1177,175 @@ rQHX6DP4w6IwZY8JB8LS }) } } + +func TestFileManagerService_DetermineFileActions_ExternalFile(t *testing.T) { + ctx := context.Background() + tempDir := t.TempDir() + fileName := filepath.Join(tempDir, "external.conf") + + modifiedFiles := map[string]*model.FileCache{ + fileName: { + File: &mpi.File{ + FileMeta: &mpi.FileMeta{ + Name: fileName, + }, + ExternalDataSource: &mpi.ExternalDataSource{Location: "http://example.com/file"}, + }, + }, + } + + fakeFileServiceClient := &v1fakes.FakeFileServiceClient{} + fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig(), &sync.RWMutex{}) + fileManagerService.agentConfig.AllowedDirectories = []string{tempDir} + + diff, err := fileManagerService.DetermineFileActions(ctx, map[string]*mpi.File{}, modifiedFiles) + require.NoError(t, err) + + fc, ok := diff[fileName] + require.True(t, ok, "expected file to be present in diff") + assert.Equal(t, model.ExternalFile, fc.Action) +} + +func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { + type tc struct { + name string + handler http.HandlerFunc + allowedDomains []string + maxBytes int + expectError bool + expectErrContains string + expectTempFile bool + expectContent []byte + expectHeaderETag string + expectHeaderLastMod string + } + + tests := []tc{ + { + name: "Success", + handler: func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("ETag", "test-etag") + w.Header().Set("Last-Modified", "Mon, 02 Jan 2006 15:04:05 MST") + w.WriteHeader(200) + _, _ = w.Write([]byte("external file content")) + }, + allowedDomains: nil, // will be set per test from ts + maxBytes: 0, + expectError: false, + expectTempFile: true, + expectContent: []byte("external file content"), + expectHeaderETag: "test-etag", + expectHeaderLastMod: "Mon, 02 Jan 2006 15:04:05 MST", + }, + { + name: "NotModified", + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotModified) + }, + allowedDomains: nil, + maxBytes: 0, + expectError: false, + expectTempFile: false, + expectContent: nil, + expectHeaderETag: "", + expectHeaderLastMod: "", + }, + { + name: "NotAllowedDomain", + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + _, _ = w.Write([]byte("external file content")) + }, + allowedDomains: []string{"not-the-host"}, + maxBytes: 0, + expectError: true, + expectErrContains: "not in the allowed domains", + expectTempFile: false, + }, + { + name: "NotFound", + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + }, + allowedDomains: nil, + maxBytes: 0, + expectError: true, + expectErrContains: "status code 404", + expectTempFile: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := context.Background() + tempDir := t.TempDir() + fileName := filepath.Join(tempDir, "external.conf") + + ts := httptest.NewServer(http.HandlerFunc(test.handler)) + defer ts.Close() + + u, err := url.Parse(ts.URL) + require.NoError(t, err) + host := u.Hostname() + + fakeFileServiceClient := &v1fakes.FakeFileServiceClient{} + fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig(), &sync.RWMutex{}) + + // If the test provided allowedDomains, use it; otherwise allow this test server's host + if test.allowedDomains == nil { + fileManagerService.agentConfig.ExternalDataSource = &config.ExternalDataSource{ + ProxyURL: config.ProxyURL{URL: ""}, + AllowedDomains: []string{host}, + MaxBytes: int64(test.maxBytes), + } + } else { + fileManagerService.agentConfig.ExternalDataSource = &config.ExternalDataSource{ + ProxyURL: config.ProxyURL{URL: ""}, + AllowedDomains: test.allowedDomains, + MaxBytes: int64(test.maxBytes), + } + } + + fileManagerService.fileActions = map[string]*model.FileCache{ + fileName: { + File: &mpi.File{ + FileMeta: &mpi.FileMeta{Name: fileName}, + ExternalDataSource: &mpi.ExternalDataSource{Location: ts.URL}, + }, + Action: model.ExternalFile, + }, + } + + err = fileManagerService.downloadUpdatedFilesToTempLocation(ctx) + + if test.expectError { + require.Error(t, err) + if test.expectErrContains != "" { + assert.Contains(t, err.Error(), test.expectErrContains) + } + // ensure no temp file left + _, statErr := os.Stat(tempFilePath(fileName)) + assert.True(t, os.IsNotExist(statErr)) + return + } + + require.NoError(t, err) + + if test.expectTempFile { + b, readErr := os.ReadFile(tempFilePath(fileName)) + require.NoError(t, readErr) + assert.Equal(t, test.expectContent, b) + + h, ok := fileManagerService.newExternalFileHeaders[fileName] + require.True(t, ok) + assert.Equal(t, test.expectHeaderETag, h.ETag) + assert.Equal(t, test.expectHeaderLastMod, h.LastModified) + + _ = os.Remove(tempFilePath(fileName)) + } else { + _, statErr := os.Stat(tempFilePath(fileName)) + assert.True(t, os.IsNotExist(statErr)) + } + }) + } +} From 529c8393ebecdf83b1f91acb90f65aa3ede39644 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 19 Nov 2025 10:35:37 +0000 Subject: [PATCH 07/14] fix lint issues --- internal/file/file_manager_service_test.go | 25 ++++++++++++---------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/internal/file/file_manager_service_test.go b/internal/file/file_manager_service_test.go index 4052a89d5..ca14b2f5d 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -17,6 +17,7 @@ import ( "path/filepath" "sync" "testing" + "time" "github.com/nginx/agent/v3/internal/config" "github.com/nginx/agent/v3/internal/model" @@ -1198,7 +1199,7 @@ func TestFileManagerService_DetermineFileActions_ExternalFile(t *testing.T) { fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig(), &sync.RWMutex{}) fileManagerService.agentConfig.AllowedDirectories = []string{tempDir} - diff, err := fileManagerService.DetermineFileActions(ctx, map[string]*mpi.File{}, modifiedFiles) + diff, err := fileManagerService.DetermineFileActions(ctx, make(map[string]*mpi.File), modifiedFiles) require.NoError(t, err) fc, ok := diff[fileName] @@ -1206,18 +1207,19 @@ func TestFileManagerService_DetermineFileActions_ExternalFile(t *testing.T) { assert.Equal(t, model.ExternalFile, fc.Action) } +//nolint:gocognit,revive,govet // cognitive complexity is 25 func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { type tc struct { + allowedDomains []string + expectContent []byte name string + expectHeaderETag string + expectHeaderLastMod string + expectErrContains string handler http.HandlerFunc - allowedDomains []string maxBytes int expectError bool - expectErrContains string expectTempFile bool - expectContent []byte - expectHeaderETag string - expectHeaderLastMod string } tests := []tc{ @@ -1225,8 +1227,8 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { name: "Success", handler: func(w http.ResponseWriter, r *http.Request) { w.Header().Set("ETag", "test-etag") - w.Header().Set("Last-Modified", "Mon, 02 Jan 2006 15:04:05 MST") - w.WriteHeader(200) + w.Header().Set("Last-Modified", time.RFC1123) + w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte("external file content")) }, allowedDomains: nil, // will be set per test from ts @@ -1235,7 +1237,7 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { expectTempFile: true, expectContent: []byte("external file content"), expectHeaderETag: "test-etag", - expectHeaderLastMod: "Mon, 02 Jan 2006 15:04:05 MST", + expectHeaderLastMod: time.RFC1123, }, { name: "NotModified", @@ -1253,7 +1255,7 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { { name: "NotAllowedDomain", handler: func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(200) + w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte("external file content")) }, allowedDomains: []string{"not-the-host"}, @@ -1281,7 +1283,7 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { tempDir := t.TempDir() fileName := filepath.Join(tempDir, "external.conf") - ts := httptest.NewServer(http.HandlerFunc(test.handler)) + ts := httptest.NewServer(test.handler) defer ts.Close() u, err := url.Parse(ts.URL) @@ -1326,6 +1328,7 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { // ensure no temp file left _, statErr := os.Stat(tempFilePath(fileName)) assert.True(t, os.IsNotExist(statErr)) + return } From 2a51d2268a29b8c2c52d754c7c31f3992b700c5e Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 19 Nov 2025 12:51:39 +0000 Subject: [PATCH 08/14] Added UT for Proxy URL --- internal/file/file_manager_service_test.go | 85 ++++++++++++++++----- internal/file/file_service_operator_test.go | 83 ++++++++++++++++++++ 2 files changed, 149 insertions(+), 19 deletions(-) diff --git a/internal/file/file_manager_service_test.go b/internal/file/file_manager_service_test.go index ca14b2f5d..9efd7ed97 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -1207,7 +1207,7 @@ func TestFileManagerService_DetermineFileActions_ExternalFile(t *testing.T) { assert.Equal(t, model.ExternalFile, fc.Action) } -//nolint:gocognit,revive,govet // cognitive complexity is 25 +//nolint:gocognit,revive,govet // cognitive complexity is 22 func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { type tc struct { allowedDomains []string @@ -1224,14 +1224,14 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { tests := []tc{ { - name: "Success", + name: "Test 1: Success", handler: func(w http.ResponseWriter, r *http.Request) { w.Header().Set("ETag", "test-etag") w.Header().Set("Last-Modified", time.RFC1123) w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte("external file content")) }, - allowedDomains: nil, // will be set per test from ts + allowedDomains: nil, maxBytes: 0, expectError: false, expectTempFile: true, @@ -1240,7 +1240,7 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { expectHeaderLastMod: time.RFC1123, }, { - name: "NotModified", + name: "Test 2: NotModified", handler: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotModified) }, @@ -1253,7 +1253,7 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { expectHeaderLastMod: "", }, { - name: "NotAllowedDomain", + name: "Test 3: NotAllowedDomain", handler: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte("external file content")) @@ -1265,7 +1265,7 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { expectTempFile: false, }, { - name: "NotFound", + name: "Test 4: NotFound", handler: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotFound) }, @@ -1275,6 +1275,32 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { expectErrContains: "status code 404", expectTempFile: false, }, + { + name: "Test 5: ProxyWithConditionalHeaders", + handler: func(w http.ResponseWriter, r *http.Request) { + // verify conditional headers from manifest are added + if r.Header.Get("If-None-Match") != "manifest-test-etag" { + http.Error(w, "missing If-None-Match", http.StatusBadRequest) + return + } + if r.Header.Get("If-Modified-Since") != time.RFC1123 { + http.Error(w, "missing If-Modified-Since", http.StatusBadRequest) + return + } + w.Header().Set("ETag", "resp-etag") + w.Header().Set("Last-Modified", time.RFC1123) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("external file via proxy")) + }, + allowedDomains: nil, + maxBytes: 0, + expectError: false, + expectTempFile: true, + expectContent: []byte("external file via proxy"), + expectHeaderETag: "resp-etag", + expectHeaderLastMod: time.RFC1123, + expectErrContains: "", + }, } for _, test := range tests { @@ -1293,21 +1319,43 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { fakeFileServiceClient := &v1fakes.FakeFileServiceClient{} fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig(), &sync.RWMutex{}) - // If the test provided allowedDomains, use it; otherwise allow this test server's host - if test.allowedDomains == nil { - fileManagerService.agentConfig.ExternalDataSource = &config.ExternalDataSource{ - ProxyURL: config.ProxyURL{URL: ""}, - AllowedDomains: []string{host}, - MaxBytes: int64(test.maxBytes), - } - } else { - fileManagerService.agentConfig.ExternalDataSource = &config.ExternalDataSource{ - ProxyURL: config.ProxyURL{URL: ""}, - AllowedDomains: test.allowedDomains, - MaxBytes: int64(test.maxBytes), + eds := &config.ExternalDataSource{ + ProxyURL: config.ProxyURL{URL: ""}, + AllowedDomains: []string{host}, + MaxBytes: int64(test.maxBytes), + } + + if test.allowedDomains != nil { + eds.AllowedDomains = test.allowedDomains + } + + if test.name == "Test 5: ProxyWithConditionalHeaders" { + manifestFiles := map[string]*model.ManifestFile{ + fileName: { + ManifestFileMeta: &model.ManifestFileMeta{ + Name: fileName, + ETag: "manifest-test-etag", + LastModified: time.RFC1123, + }, + }, } + manifestJSON, mErr := json.MarshalIndent(manifestFiles, "", " ") + require.NoError(t, mErr) + + manifestFile, mErr := os.CreateTemp(tempDir, "manifest.json") + require.NoError(t, mErr) + _, mErr = manifestFile.Write(manifestJSON) + require.NoError(t, mErr) + _ = manifestFile.Close() + + fileManagerService.agentConfig.LibDir = tempDir + fileManagerService.manifestFilePath = manifestFile.Name() + + eds.ProxyURL = config.ProxyURL{URL: ts.URL} } + fileManagerService.agentConfig.ExternalDataSource = eds + fileManagerService.fileActions = map[string]*model.FileCache{ fileName: { File: &mpi.File{ @@ -1325,7 +1373,6 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { if test.expectErrContains != "" { assert.Contains(t, err.Error(), test.expectErrContains) } - // ensure no temp file left _, statErr := os.Stat(tempFilePath(fileName)) assert.True(t, os.IsNotExist(statErr)) diff --git a/internal/file/file_service_operator_test.go b/internal/file/file_service_operator_test.go index f8206e145..f176dbe2a 100644 --- a/internal/file/file_service_operator_test.go +++ b/internal/file/file_service_operator_test.go @@ -187,3 +187,86 @@ func TestFileManagerService_UpdateFile_LargeFile(t *testing.T) { helpers.RemoveFileWithErrorCheck(t, testFile.Name()) } + +func TestFileServiceOperator_RenameExternalFile(t *testing.T) { + tests := []struct { + prepare func(t *testing.T) (src, dst string) + name string + wantErrMsg string + wantErr bool + }{ + { + name: "Test 1: success", + prepare: func(t *testing.T) (string, string) { + t.Helper() + tmp := t.TempDir() + src := filepath.Join(tmp, "src.txt") + dst := filepath.Join(tmp, "subdir", "dest.txt") + content := []byte("hello world") + require.NoError(t, os.WriteFile(src, content, 0o600)) + + return src, dst + }, + wantErr: false, + }, + { + name: "Test 2: mkdirall_fail", + prepare: func(t *testing.T) (string, string) { + t.Helper() + tmp := t.TempDir() + parentFile := filepath.Join(tmp, "not_a_dir") + require.NoError(t, os.WriteFile(parentFile, []byte("block"), 0o600)) + dst := filepath.Join(parentFile, "dest.txt") + src := filepath.Join(tmp, "src.txt") + require.NoError(t, os.WriteFile(src, []byte("content"), 0o600)) + + return src, dst + }, + wantErr: true, + wantErrMsg: "failed to create directories for", + }, + { + name: "Test 3: rename_fail", + prepare: func(t *testing.T) (string, string) { + t.Helper() + tmp := t.TempDir() + src := filepath.Join(tmp, "does_not_exist.txt") + dst := filepath.Join(tmp, "subdir", "dest.txt") + + return src, dst + }, + wantErr: true, + wantErrMsg: "failed to move file", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + fso := NewFileServiceOperator(types.AgentConfig(), nil, &sync.RWMutex{}) + + src, dst := tc.prepare(t) + + err := fso.RenameExternalFile(ctx, src, dst) + if tc.wantErr { + require.Error(t, err) + if tc.wantErrMsg != "" { + require.Contains(t, err.Error(), tc.wantErrMsg) + } + + return + } + + require.NoError(t, err) + + dstContent, readErr := os.ReadFile(dst) + require.NoError(t, readErr) + if tc.name == "success" { + require.Equal(t, []byte("hello world"), dstContent) + } + + _, statErr := os.Stat(src) + require.True(t, os.IsNotExist(statErr)) + }) + } +} From f22834fad1c9d41333e9ee974e0938619efb8c9d Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 19 Nov 2025 15:58:04 +0000 Subject: [PATCH 09/14] increasing UT coverage --- internal/file/file_manager_service_test.go | 138 +++++++++++++++++++++ 1 file changed, 138 insertions(+) diff --git a/internal/file/file_manager_service_test.go b/internal/file/file_manager_service_test.go index 9efd7ed97..b53aa3885 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -15,6 +15,7 @@ import ( "net/url" "os" "path/filepath" + "strings" "sync" "testing" "time" @@ -34,6 +35,39 @@ import ( "github.com/stretchr/testify/require" ) +type fakeFSO struct { + renameSrc, renameDst string + renameExternalCalled bool +} + +func (f *fakeFSO) File(ctx context.Context, file *mpi.File, tempFilePath, expectedHash string) error { + return nil +} + +func (f *fakeFSO) UpdateOverview(ctx context.Context, instanceID string, filesToUpdate []*mpi.File, configPath string, + iteration int, +) error { + return nil +} + +func (f *fakeFSO) ChunkedFile(ctx context.Context, file *mpi.File, tempFilePath, expectedHash string) error { + return nil +} +func (f *fakeFSO) IsConnected() bool { return true } +func (f *fakeFSO) UpdateFile(ctx context.Context, instanceID string, fileToUpdate *mpi.File) error { + return nil +} +func (f *fakeFSO) SetIsConnected(isConnected bool) {} +func (f *fakeFSO) RenameFile(ctx context.Context, hash, fileName, tempDir string) error { return nil } +func (f *fakeFSO) RenameExternalFile(ctx context.Context, fileName, tempDir string) error { + f.renameExternalCalled = true + f.renameSrc = fileName + f.renameDst = tempDir + + return nil +} +func (f *fakeFSO) UpdateClient(ctx context.Context, fileServiceClient mpi.FileServiceClient) {} + func TestFileManagerService_ConfigApply_Add(t *testing.T) { ctx := context.Background() tempDir := t.TempDir() @@ -1399,3 +1433,107 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { }) } } + +func TestMoveOrDeleteFiles_ExternalFileRenameCalled(t *testing.T) { + ctx := context.Background() + fms := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{}) + + fake := &fakeFSO{} + fms.fileServiceOperator = fake + + fileName := filepath.Join(t.TempDir(), "ext.conf") + fms.fileActions = map[string]*model.FileCache{ + fileName: { + File: &mpi.File{FileMeta: &mpi.FileMeta{Name: fileName}}, + Action: model.ExternalFile, + }, + } + + tempPath := tempFilePath(fileName) + reqDir := filepath.Dir(tempPath) + require.NoError(t, os.MkdirAll(reqDir, 0o755)) + require.NoError(t, os.WriteFile(tempPath, []byte("data"), 0o600)) + + err := fms.moveOrDeleteFiles(ctx, nil) + require.NoError(t, err) + assert.True(t, fake.renameExternalCalled) + assert.Equal(t, tempPath, fake.renameSrc) + assert.Equal(t, fileName, fake.renameDst) +} + +func TestDownloadFileContent_MaxBytesLimit(t *testing.T) { + ctx := context.Background() + fms := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{}) + + // test server returns 10 bytes, we set MaxBytes to 4 and expect only 4 bytes returned + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("ETag", "etag-1") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("0123456789")) + })) + defer ts.Close() + + u, err := url.Parse(ts.URL) + require.NoError(t, err) + + fms.agentConfig.ExternalDataSource = &config.ExternalDataSource{ + AllowedDomains: []string{u.Hostname()}, + MaxBytes: 4, + } + + fileName := filepath.Join(t.TempDir(), "external.conf") + file := &mpi.File{ + FileMeta: &mpi.FileMeta{Name: fileName}, + ExternalDataSource: &mpi.ExternalDataSource{Location: ts.URL}, + } + + content, headers, err := fms.downloadFileContent(ctx, file) + require.NoError(t, err) + assert.Len(t, content, 4) + assert.Equal(t, "etag-1", headers.ETag) +} + +func TestDownloadFileContent_InvalidProxyURL(t *testing.T) { + ctx := context.Background() + fms := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{}) + + downURL := "http://example.com/file" + fms.agentConfig.ExternalDataSource = &config.ExternalDataSource{ + AllowedDomains: []string{"example.com"}, + ProxyURL: config.ProxyURL{URL: "http://:"}, + } + + file := &mpi.File{ + FileMeta: &mpi.FileMeta{Name: "/tmp/file"}, + ExternalDataSource: &mpi.ExternalDataSource{Location: downURL}, + } + + _, _, err := fms.downloadFileContent(ctx, file) + require.Error(t, err) + if !strings.Contains(err.Error(), "invalid proxy URL configured") && + !strings.Contains(err.Error(), "failed to execute download request") && + !strings.Contains(err.Error(), "proxyconnect") { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestIsDomainAllowed_EdgeCases(t *testing.T) { + ok := isDomainAllowed("http://%", []string{"example.com"}) + assert.False(t, ok) + + ok = isDomainAllowed("http://", []string{"example.com"}) + assert.False(t, ok) + + ok = isDomainAllowed("http://example.com/path", []string{""}) + assert.False(t, ok) + + ok = isDomainAllowed("http://example.com/path", []string{"example.com"}) + assert.True(t, ok) + + ok = isDomainAllowed("http://sub.example.com/path", []string{"*.example.com"}) + assert.True(t, ok) + + assert.True(t, isWildcardMatch("example.com", "*.example.com")) + assert.True(t, isWildcardMatch("sub.example.com", "*.example.com")) + assert.False(t, isWildcardMatch("badexample.com", "*.example.com")) +} From 4a2b917ff1431aeb71f191ff838a4a4b51694c36 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Thu, 20 Nov 2025 11:59:25 +0000 Subject: [PATCH 10/14] updated Mock management to test external file config apply --- internal/file/file_manager_service.go | 2 ++ test/mock/grpc/mock_management_command_service.go | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index 07f462378..bbbd6b30e 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -842,6 +842,8 @@ func (fms *FileManagerService) handleExternalFileDownload(ctx context.Context, f slog.DebugContext(ctx, "External file unchanged (304), skipping disk write.", "file", fileAction.File.GetFileMeta().GetName()) + fileAction.Action = model.Unchanged + return nil } diff --git a/test/mock/grpc/mock_management_command_service.go b/test/mock/grpc/mock_management_command_service.go index f68c4c7cd..b1badb443 100644 --- a/test/mock/grpc/mock_management_command_service.go +++ b/test/mock/grpc/mock_management_command_service.go @@ -577,7 +577,8 @@ func processConfigApplyRequestBody(c *gin.Context, initialFiles []*mpi.File) ([] } else { newFile := &mpi.File{ FileMeta: &mpi.FileMeta{ - Name: ed.FilePath, + Name: ed.FilePath, + Permissions: "0644", }, ExternalDataSource: &mpi.ExternalDataSource{ Location: ed.Location, From b7759e1afc4d81eb1affd566e638260777de9ddd Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Mon, 24 Nov 2025 15:59:04 +0000 Subject: [PATCH 11/14] PR review comments addressed --- internal/config/config.go | 13 ++- internal/config/config_test.go | 8 +- internal/config/defaults.go | 1 + internal/config/testdata/nginx-agent.conf | 8 ++ internal/file/file_manager_service.go | 93 ++++++++++++---------- internal/file/file_manager_service_test.go | 2 +- 6 files changed, 75 insertions(+), 50 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 26c6ed81d..d765bdcf0 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -50,6 +50,10 @@ const ( regexLabelPattern = "^[a-zA-Z0-9]([a-zA-Z0-9-_]{0,254}[a-zA-Z0-9])?$" ) +var domainRegex = regexp.MustCompile( + `^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$`, +) + var viperInstance = viper.NewWithOptions(viper.KeyDelimiter(KeyDelimiter)) func RegisterRunner(r func(cmd *cobra.Command, args []string)) { @@ -495,7 +499,7 @@ func registerExternalDataSourceFlags(fs *flag.FlagSet) { fs.String( ExternalDataSourceProxyUrlKey, DefExternalDataSourceProxyUrl, - "Url to the proxy service to fetch the external file.", + "Url to the proxy service for fetching external files.", ) fs.StringSlice( ExternalDataSourceAllowDomainsKey, @@ -651,7 +655,7 @@ func registerClientFlags(fs *flag.FlagSet) { fs.Duration( ClientFileDownloadTimeoutKey, DefClientFileDownloadTimeout, - "Timeout value in seconds, to downloading file for config apply.", + "Timeout value in seconds, for downloading a file during a config apply.", ) fs.Int( @@ -1610,8 +1614,9 @@ func validateAllowedDomains(domains []string) error { } for _, domain := range domains { - if strings.ContainsAny(domain, "/\\ ") || domain == "" { - slog.Error("domain is not specified in allowed_domains") + // Validating syntax using the RFC-compliant regex + if !domainRegex.MatchString(domain) || domain == "" { + slog.Error("domain specified in allowed_domains is invalid", "domain", domain) return errors.New("invalid domain found in allowed_domains") } } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index b35818ebe..8fd3dd1fe 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1427,10 +1427,10 @@ func createConfig() *Config { }, ExternalDataSource: &ExternalDataSource{ ProxyURL: ProxyURL{ - URL: "", + URL: "http://proxy.example.com", }, - AllowedDomains: nil, - MaxBytes: 0, + AllowedDomains: []string{"example.com", "api.example.com"}, + MaxBytes: 1048576, }, } } @@ -1638,7 +1638,7 @@ func TestValidateAllowedDomains(t *testing.T) { if tt.wantErr { require.Error(t, actualErr, "Expected an error but got nil.") - assert.Contains(t, logBuffer.String(), "domain is not specified in allowed_domains", + assert.Contains(t, logBuffer.String(), "domain specified in allowed_domains is invalid", "Expected the error log message to be present in the output.") } else { assert.NoError(t, actualErr, "Did not expect an error but got one: %v", actualErr) diff --git a/internal/config/defaults.go b/internal/config/defaults.go index ed2cdbe90..a9141d067 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -118,6 +118,7 @@ const ( DefLibDir = "/var/lib/nginx-agent" DefExternalDataSourceProxyUrl = "" + // Default allow external data sources up to 100 MB DefExternalDataSourceMaxBytes = 100 * 1024 * 1024 ) diff --git a/internal/config/testdata/nginx-agent.conf b/internal/config/testdata/nginx-agent.conf index 2ac87b9ee..d7892c4c6 100644 --- a/internal/config/testdata/nginx-agent.conf +++ b/internal/config/testdata/nginx-agent.conf @@ -183,3 +183,11 @@ collector: log: level: "INFO" path: "/var/log/nginx-agent/opentelemetry-collector-agent.log" + +external_data_source: + proxy: + url: "http://proxy.example.com" + allowed_domains: + - example.com + - api.example.com + max_bytes: 1048576 diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index bbbd6b30e..b75506747 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -44,7 +44,7 @@ const ( executePerm = 0o111 ) -type DownloadHeaders struct { +type DownloadHeader struct { ETag string LastModified string } @@ -114,28 +114,28 @@ type FileManagerService struct { // map of files and the actions performed on them during config apply fileActions map[string]*model.FileCache // key is file path // map of the files currently on disk, used to determine the file action during config apply - currentFilesOnDisk map[string]*mpi.File // key is file path - previousManifestFiles map[string]*model.ManifestFile - newExternalFileHeaders map[string]DownloadHeaders - manifestFilePath string - rollbackManifest bool - filesMutex sync.RWMutex + currentFilesOnDisk map[string]*mpi.File // key is file path + previousManifestFiles map[string]*model.ManifestFile + externalFileHeaders map[string]DownloadHeader + manifestFilePath string + rollbackManifest bool + filesMutex sync.RWMutex } func NewFileManagerService(fileServiceClient mpi.FileServiceClient, agentConfig *config.Config, manifestLock *sync.RWMutex, ) *FileManagerService { return &FileManagerService{ - agentConfig: agentConfig, - fileOperator: NewFileOperator(manifestLock), - fileServiceOperator: NewFileServiceOperator(agentConfig, fileServiceClient, manifestLock), - fileActions: make(map[string]*model.FileCache), - currentFilesOnDisk: make(map[string]*mpi.File), - previousManifestFiles: make(map[string]*model.ManifestFile), - newExternalFileHeaders: make(map[string]DownloadHeaders), - rollbackManifest: true, - manifestFilePath: agentConfig.LibDir + "/manifest.json", - manifestLock: manifestLock, + agentConfig: agentConfig, + fileOperator: NewFileOperator(manifestLock), + fileServiceOperator: NewFileServiceOperator(agentConfig, fileServiceClient, manifestLock), + fileActions: make(map[string]*model.FileCache), + currentFilesOnDisk: make(map[string]*mpi.File), + previousManifestFiles: make(map[string]*model.ManifestFile), + externalFileHeaders: make(map[string]DownloadHeader), + rollbackManifest: true, + manifestFilePath: agentConfig.LibDir + "/manifest.json", + manifestLock: manifestLock, } } @@ -402,22 +402,30 @@ func (fms *FileManagerService) DetermineFileActions( slog.DebugContext(ctx, "Skipping unmanaged file updates", "file_name", fileName) continue } + + // If either the modified file or the current file is an external data source, + // treat this as an ExternalFile and skip the regular Add/Update checks. This + // ensures external files are downloaded/validated every single time. + if modifiedFile.File.GetExternalDataSource() != nil || (ok && currentFile.GetExternalDataSource() != nil) { + modifiedFile.Action = model.ExternalFile + fileDiff[fileName] = modifiedFile + + continue + } + // if file doesn't exist in the current files, file has been added // set file action if _, statErr := os.Stat(fileName); errors.Is(statErr, os.ErrNotExist) { modifiedFile.Action = model.Add fileDiff[fileName] = modifiedFile + continue // if file currently exists and file hash is different, file has been updated // copy contents, set file action } else if ok && modifiedFile.File.GetFileMeta().GetHash() != currentFile.GetFileMeta().GetHash() { modifiedFile.Action = model.Update fileDiff[fileName] = modifiedFile } - if modifiedFile.File.GetExternalDataSource() != nil || currentFile.GetExternalDataSource() != nil { - modifiedFile.Action = model.ExternalFile - fileDiff[fileName] = modifiedFile - } } return fileDiff, nil @@ -615,7 +623,7 @@ func (fms *FileManagerService) downloadUpdatedFilesToTempLocation(ctx context.Co switch fileAction.Action { case model.ExternalFile: - return fms.handleExternalFileDownload(errGroupCtx, fileAction, tempFilePath) + return fms.downloadExternalFile(errGroupCtx, fileAction, tempFilePath) case model.Add, model.Update: slog.DebugContext( errGroupCtx, @@ -819,15 +827,17 @@ func tempBackupFilePath(fileName string) string { return filepath.Join(filepath.Dir(fileName), tempFileName) } -func (fms *FileManagerService) handleExternalFileDownload(ctx context.Context, fileAction *model.FileCache, - tempFilePath string, +func (fms *FileManagerService) downloadExternalFile(ctx context.Context, fileAction *model.FileCache, + filePath string, ) error { location := fileAction.File.GetExternalDataSource().GetLocation() + permission := fileAction.File.GetFileMeta().GetPermissions() + slog.InfoContext(ctx, "Downloading external file from", "location", location) var contentToWrite []byte var downloadErr, updateError error - var headers DownloadHeaders + var headers DownloadHeader contentToWrite, headers, downloadErr = fms.downloadFileContent(ctx, fileAction.File) @@ -848,9 +858,9 @@ func (fms *FileManagerService) handleExternalFileDownload(ctx context.Context, f } fileName := fileAction.File.GetFileMeta().GetName() - fms.newExternalFileHeaders[fileName] = headers + fms.externalFileHeaders[fileName] = headers - updateErr := fms.writeContentToTempFile(ctx, contentToWrite, tempFilePath) + updateErr := fms.writeContentToTempFile(ctx, contentToWrite, filePath, permission) return updateErr } @@ -859,12 +869,13 @@ func (fms *FileManagerService) writeContentToTempFile( ctx context.Context, content []byte, path string, + permission string, ) error { writeErr := fms.fileOperator.Write( ctx, content, path, - "0600", + permission, ) if writeErr != nil { @@ -878,23 +889,23 @@ func (fms *FileManagerService) writeContentToTempFile( func (fms *FileManagerService) downloadFileContent( ctx context.Context, file *mpi.File, -) (content []byte, headers DownloadHeaders, err error) { +) (content []byte, headers DownloadHeader, err error) { fileName := file.GetFileMeta().GetName() downloadURL := file.GetExternalDataSource().GetLocation() externalConfig := fms.agentConfig.ExternalDataSource if !isDomainAllowed(downloadURL, externalConfig.AllowedDomains) { - return nil, DownloadHeaders{}, fmt.Errorf("download URL %s is not in the allowed domains list", downloadURL) + return nil, DownloadHeader{}, fmt.Errorf("download URL %s is not in the allowed domains list", downloadURL) } httpClient, err := fms.setupHTTPClient(ctx, externalConfig.ProxyURL.URL) if err != nil { - return nil, DownloadHeaders{}, err + return nil, DownloadHeader{}, err } req, err := http.NewRequestWithContext(ctx, http.MethodGet, downloadURL, nil) if err != nil { - return nil, DownloadHeaders{}, fmt.Errorf("failed to create request for %s: %w", downloadURL, err) + return nil, DownloadHeader{}, fmt.Errorf("failed to create request for %s: %w", downloadURL, err) } if externalConfig.ProxyURL.URL != "" { @@ -905,7 +916,7 @@ func (fms *FileManagerService) downloadFileContent( resp, err := httpClient.Do(req) if err != nil { - return nil, DownloadHeaders{}, fmt.Errorf("failed to execute download request for %s: %w", downloadURL, err) + return nil, DownloadHeader{}, fmt.Errorf("failed to execute download request for %s: %w", downloadURL, err) } defer resp.Body.Close() @@ -914,10 +925,10 @@ func (fms *FileManagerService) downloadFileContent( headers.ETag = resp.Header.Get("ETag") headers.LastModified = resp.Header.Get("Last-Modified") case http.StatusNotModified: - slog.InfoContext(ctx, "File content unchanged (304 Not Modified)", "file_name", fileName) - return nil, DownloadHeaders{}, nil + slog.DebugContext(ctx, "File content unchanged (304 Not Modified)", "file_name", fileName) + return nil, DownloadHeader{}, nil default: - return nil, DownloadHeaders{}, fmt.Errorf("download failed with status code %d", resp.StatusCode) + return nil, DownloadHeader{}, fmt.Errorf("download failed with status code %d", resp.StatusCode) } reader := io.Reader(resp.Body) @@ -927,7 +938,7 @@ func (fms *FileManagerService) downloadFileContent( content, err = io.ReadAll(reader) if err != nil { - return nil, DownloadHeaders{}, fmt.Errorf("failed to read content from response body: %w", err) + return nil, DownloadHeader{}, fmt.Errorf("failed to read content from response body: %w", err) } slog.InfoContext(ctx, "Successfully downloaded file content", "file_name", fileName, "size", len(content)) @@ -947,16 +958,16 @@ func isDomainAllowed(downloadURL string, allowedDomains []string) bool { return false } - for _, pattern := range allowedDomains { - if pattern == "" { + for _, domain := range allowedDomains { + if domain == "" { continue } - if pattern == hostname { + if domain == hostname { return true } - if isWildcardMatch(hostname, pattern) { + if isWildcardMatch(hostname, domain) { return true } } diff --git a/internal/file/file_manager_service_test.go b/internal/file/file_manager_service_test.go index b53aa3885..f67318db2 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -1420,7 +1420,7 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { require.NoError(t, readErr) assert.Equal(t, test.expectContent, b) - h, ok := fileManagerService.newExternalFileHeaders[fileName] + h, ok := fileManagerService.externalFileHeaders[fileName] require.True(t, ok) assert.Equal(t, test.expectHeaderETag, h.ETag) assert.Equal(t, test.expectHeaderLastMod, h.LastModified) From bd60017e469b2d40ba295c7316892cbb7069490f Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Mon, 24 Nov 2025 17:09:52 +0000 Subject: [PATCH 12/14] worked on PR comments --- internal/file/file_manager_service.go | 7 +- internal/file/file_manager_service_test.go | 59 +- .../fake_file_service_operator_interface.go | 664 ++++++++++++++++++ 3 files changed, 687 insertions(+), 43 deletions(-) create mode 100644 internal/file/filefakes/fake_file_service_operator_interface.go diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index b75506747..0cc28e029 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -37,6 +37,9 @@ import ( //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6@v6.8.1 -generate //counterfeiter:generate . fileManagerServiceInterface +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6@v6.8.1 -generate +//counterfeiter:generate . fileServiceOperatorInterface + const ( maxAttempts = 5 dirPerm = 0o755 @@ -967,7 +970,7 @@ func isDomainAllowed(downloadURL string, allowedDomains []string) bool { return true } - if isWildcardMatch(hostname, domain) { + if isMatchesWildcardDomain(hostname, domain) { return true } } @@ -1028,7 +1031,7 @@ func (fms *FileManagerService) addConditionalHeaders(ctx context.Context, req *h } } -func isWildcardMatch(hostname, pattern string) bool { +func isMatchesWildcardDomain(hostname, pattern string) bool { if !strings.HasPrefix(pattern, "*.") { return false } diff --git a/internal/file/file_manager_service_test.go b/internal/file/file_manager_service_test.go index f67318db2..89d05a4f1 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -21,6 +21,7 @@ import ( "time" "github.com/nginx/agent/v3/internal/config" + "github.com/nginx/agent/v3/internal/file/filefakes" "github.com/nginx/agent/v3/internal/model" "github.com/nginx/agent/v3/pkg/files" @@ -35,39 +36,6 @@ import ( "github.com/stretchr/testify/require" ) -type fakeFSO struct { - renameSrc, renameDst string - renameExternalCalled bool -} - -func (f *fakeFSO) File(ctx context.Context, file *mpi.File, tempFilePath, expectedHash string) error { - return nil -} - -func (f *fakeFSO) UpdateOverview(ctx context.Context, instanceID string, filesToUpdate []*mpi.File, configPath string, - iteration int, -) error { - return nil -} - -func (f *fakeFSO) ChunkedFile(ctx context.Context, file *mpi.File, tempFilePath, expectedHash string) error { - return nil -} -func (f *fakeFSO) IsConnected() bool { return true } -func (f *fakeFSO) UpdateFile(ctx context.Context, instanceID string, fileToUpdate *mpi.File) error { - return nil -} -func (f *fakeFSO) SetIsConnected(isConnected bool) {} -func (f *fakeFSO) RenameFile(ctx context.Context, hash, fileName, tempDir string) error { return nil } -func (f *fakeFSO) RenameExternalFile(ctx context.Context, fileName, tempDir string) error { - f.renameExternalCalled = true - f.renameSrc = fileName - f.renameDst = tempDir - - return nil -} -func (f *fakeFSO) UpdateClient(ctx context.Context, fileServiceClient mpi.FileServiceClient) {} - func TestFileManagerService_ConfigApply_Add(t *testing.T) { ctx := context.Background() tempDir := t.TempDir() @@ -1438,8 +1406,13 @@ func TestMoveOrDeleteFiles_ExternalFileRenameCalled(t *testing.T) { ctx := context.Background() fms := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{}) - fake := &fakeFSO{} - fms.fileServiceOperator = fake + fakeFSO := &filefakes.FakeFileServiceOperatorInterface{} + + fakeFSO.RenameExternalFileStub = func(ctx context.Context, fileName, tempDir string) error { + return nil + } + + fms.fileServiceOperator = fakeFSO fileName := filepath.Join(t.TempDir(), "ext.conf") fms.fileActions = map[string]*model.FileCache{ @@ -1456,9 +1429,13 @@ func TestMoveOrDeleteFiles_ExternalFileRenameCalled(t *testing.T) { err := fms.moveOrDeleteFiles(ctx, nil) require.NoError(t, err) - assert.True(t, fake.renameExternalCalled) - assert.Equal(t, tempPath, fake.renameSrc) - assert.Equal(t, fileName, fake.renameDst) + + assert.Equal(t, 1, fakeFSO.RenameExternalFileCallCount(), "RenameExternalFile should be called once") + + _, srcArg, dstArg := fakeFSO.RenameExternalFileArgsForCall(0) + + assert.Equal(t, tempPath, srcArg, "RenameExternalFile source argument mismatch") + assert.Equal(t, fileName, dstArg, "RenameExternalFile destination argument mismatch") } func TestDownloadFileContent_MaxBytesLimit(t *testing.T) { @@ -1533,7 +1510,7 @@ func TestIsDomainAllowed_EdgeCases(t *testing.T) { ok = isDomainAllowed("http://sub.example.com/path", []string{"*.example.com"}) assert.True(t, ok) - assert.True(t, isWildcardMatch("example.com", "*.example.com")) - assert.True(t, isWildcardMatch("sub.example.com", "*.example.com")) - assert.False(t, isWildcardMatch("badexample.com", "*.example.com")) + assert.True(t, isMatchesWildcardDomain("example.com", "*.example.com")) + assert.True(t, isMatchesWildcardDomain("sub.example.com", "*.example.com")) + assert.False(t, isMatchesWildcardDomain("badexample.com", "*.example.com")) } diff --git a/internal/file/filefakes/fake_file_service_operator_interface.go b/internal/file/filefakes/fake_file_service_operator_interface.go new file mode 100644 index 000000000..fa98f941a --- /dev/null +++ b/internal/file/filefakes/fake_file_service_operator_interface.go @@ -0,0 +1,664 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package filefakes + +import ( + "context" + "sync" + + v1 "github.com/nginx/agent/v3/api/grpc/mpi/v1" +) + +type FakeFileServiceOperatorInterface struct { + ChunkedFileStub func(context.Context, *v1.File, string, string) error + chunkedFileMutex sync.RWMutex + chunkedFileArgsForCall []struct { + arg1 context.Context + arg2 *v1.File + arg3 string + arg4 string + } + chunkedFileReturns struct { + result1 error + } + chunkedFileReturnsOnCall map[int]struct { + result1 error + } + FileStub func(context.Context, *v1.File, string, string) error + fileMutex sync.RWMutex + fileArgsForCall []struct { + arg1 context.Context + arg2 *v1.File + arg3 string + arg4 string + } + fileReturns struct { + result1 error + } + fileReturnsOnCall map[int]struct { + result1 error + } + IsConnectedStub func() bool + isConnectedMutex sync.RWMutex + isConnectedArgsForCall []struct { + } + isConnectedReturns struct { + result1 bool + } + isConnectedReturnsOnCall map[int]struct { + result1 bool + } + RenameExternalFileStub func(context.Context, string, string) error + renameExternalFileMutex sync.RWMutex + renameExternalFileArgsForCall []struct { + arg1 context.Context + arg2 string + arg3 string + } + renameExternalFileReturns struct { + result1 error + } + renameExternalFileReturnsOnCall map[int]struct { + result1 error + } + RenameFileStub func(context.Context, string, string, string) error + renameFileMutex sync.RWMutex + renameFileArgsForCall []struct { + arg1 context.Context + arg2 string + arg3 string + arg4 string + } + renameFileReturns struct { + result1 error + } + renameFileReturnsOnCall map[int]struct { + result1 error + } + SetIsConnectedStub func(bool) + setIsConnectedMutex sync.RWMutex + setIsConnectedArgsForCall []struct { + arg1 bool + } + UpdateClientStub func(context.Context, v1.FileServiceClient) + updateClientMutex sync.RWMutex + updateClientArgsForCall []struct { + arg1 context.Context + arg2 v1.FileServiceClient + } + UpdateFileStub func(context.Context, string, *v1.File) error + updateFileMutex sync.RWMutex + updateFileArgsForCall []struct { + arg1 context.Context + arg2 string + arg3 *v1.File + } + updateFileReturns struct { + result1 error + } + updateFileReturnsOnCall map[int]struct { + result1 error + } + UpdateOverviewStub func(context.Context, string, []*v1.File, string, int) error + updateOverviewMutex sync.RWMutex + updateOverviewArgsForCall []struct { + arg1 context.Context + arg2 string + arg3 []*v1.File + arg4 string + arg5 int + } + updateOverviewReturns struct { + result1 error + } + updateOverviewReturnsOnCall map[int]struct { + result1 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeFileServiceOperatorInterface) ChunkedFile(arg1 context.Context, arg2 *v1.File, arg3 string, arg4 string) error { + fake.chunkedFileMutex.Lock() + ret, specificReturn := fake.chunkedFileReturnsOnCall[len(fake.chunkedFileArgsForCall)] + fake.chunkedFileArgsForCall = append(fake.chunkedFileArgsForCall, struct { + arg1 context.Context + arg2 *v1.File + arg3 string + arg4 string + }{arg1, arg2, arg3, arg4}) + stub := fake.ChunkedFileStub + fakeReturns := fake.chunkedFileReturns + fake.recordInvocation("ChunkedFile", []interface{}{arg1, arg2, arg3, arg4}) + fake.chunkedFileMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3, arg4) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeFileServiceOperatorInterface) ChunkedFileCallCount() int { + fake.chunkedFileMutex.RLock() + defer fake.chunkedFileMutex.RUnlock() + return len(fake.chunkedFileArgsForCall) +} + +func (fake *FakeFileServiceOperatorInterface) ChunkedFileCalls(stub func(context.Context, *v1.File, string, string) error) { + fake.chunkedFileMutex.Lock() + defer fake.chunkedFileMutex.Unlock() + fake.ChunkedFileStub = stub +} + +func (fake *FakeFileServiceOperatorInterface) ChunkedFileArgsForCall(i int) (context.Context, *v1.File, string, string) { + fake.chunkedFileMutex.RLock() + defer fake.chunkedFileMutex.RUnlock() + argsForCall := fake.chunkedFileArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4 +} + +func (fake *FakeFileServiceOperatorInterface) ChunkedFileReturns(result1 error) { + fake.chunkedFileMutex.Lock() + defer fake.chunkedFileMutex.Unlock() + fake.ChunkedFileStub = nil + fake.chunkedFileReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) ChunkedFileReturnsOnCall(i int, result1 error) { + fake.chunkedFileMutex.Lock() + defer fake.chunkedFileMutex.Unlock() + fake.ChunkedFileStub = nil + if fake.chunkedFileReturnsOnCall == nil { + fake.chunkedFileReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.chunkedFileReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) File(arg1 context.Context, arg2 *v1.File, arg3 string, arg4 string) error { + fake.fileMutex.Lock() + ret, specificReturn := fake.fileReturnsOnCall[len(fake.fileArgsForCall)] + fake.fileArgsForCall = append(fake.fileArgsForCall, struct { + arg1 context.Context + arg2 *v1.File + arg3 string + arg4 string + }{arg1, arg2, arg3, arg4}) + stub := fake.FileStub + fakeReturns := fake.fileReturns + fake.recordInvocation("File", []interface{}{arg1, arg2, arg3, arg4}) + fake.fileMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3, arg4) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeFileServiceOperatorInterface) FileCallCount() int { + fake.fileMutex.RLock() + defer fake.fileMutex.RUnlock() + return len(fake.fileArgsForCall) +} + +func (fake *FakeFileServiceOperatorInterface) FileCalls(stub func(context.Context, *v1.File, string, string) error) { + fake.fileMutex.Lock() + defer fake.fileMutex.Unlock() + fake.FileStub = stub +} + +func (fake *FakeFileServiceOperatorInterface) FileArgsForCall(i int) (context.Context, *v1.File, string, string) { + fake.fileMutex.RLock() + defer fake.fileMutex.RUnlock() + argsForCall := fake.fileArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4 +} + +func (fake *FakeFileServiceOperatorInterface) FileReturns(result1 error) { + fake.fileMutex.Lock() + defer fake.fileMutex.Unlock() + fake.FileStub = nil + fake.fileReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) FileReturnsOnCall(i int, result1 error) { + fake.fileMutex.Lock() + defer fake.fileMutex.Unlock() + fake.FileStub = nil + if fake.fileReturnsOnCall == nil { + fake.fileReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.fileReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) IsConnected() bool { + fake.isConnectedMutex.Lock() + ret, specificReturn := fake.isConnectedReturnsOnCall[len(fake.isConnectedArgsForCall)] + fake.isConnectedArgsForCall = append(fake.isConnectedArgsForCall, struct { + }{}) + stub := fake.IsConnectedStub + fakeReturns := fake.isConnectedReturns + fake.recordInvocation("IsConnected", []interface{}{}) + fake.isConnectedMutex.Unlock() + if stub != nil { + return stub() + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeFileServiceOperatorInterface) IsConnectedCallCount() int { + fake.isConnectedMutex.RLock() + defer fake.isConnectedMutex.RUnlock() + return len(fake.isConnectedArgsForCall) +} + +func (fake *FakeFileServiceOperatorInterface) IsConnectedCalls(stub func() bool) { + fake.isConnectedMutex.Lock() + defer fake.isConnectedMutex.Unlock() + fake.IsConnectedStub = stub +} + +func (fake *FakeFileServiceOperatorInterface) IsConnectedReturns(result1 bool) { + fake.isConnectedMutex.Lock() + defer fake.isConnectedMutex.Unlock() + fake.IsConnectedStub = nil + fake.isConnectedReturns = struct { + result1 bool + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) IsConnectedReturnsOnCall(i int, result1 bool) { + fake.isConnectedMutex.Lock() + defer fake.isConnectedMutex.Unlock() + fake.IsConnectedStub = nil + if fake.isConnectedReturnsOnCall == nil { + fake.isConnectedReturnsOnCall = make(map[int]struct { + result1 bool + }) + } + fake.isConnectedReturnsOnCall[i] = struct { + result1 bool + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) RenameExternalFile(arg1 context.Context, arg2 string, arg3 string) error { + fake.renameExternalFileMutex.Lock() + ret, specificReturn := fake.renameExternalFileReturnsOnCall[len(fake.renameExternalFileArgsForCall)] + fake.renameExternalFileArgsForCall = append(fake.renameExternalFileArgsForCall, struct { + arg1 context.Context + arg2 string + arg3 string + }{arg1, arg2, arg3}) + stub := fake.RenameExternalFileStub + fakeReturns := fake.renameExternalFileReturns + fake.recordInvocation("RenameExternalFile", []interface{}{arg1, arg2, arg3}) + fake.renameExternalFileMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeFileServiceOperatorInterface) RenameExternalFileCallCount() int { + fake.renameExternalFileMutex.RLock() + defer fake.renameExternalFileMutex.RUnlock() + return len(fake.renameExternalFileArgsForCall) +} + +func (fake *FakeFileServiceOperatorInterface) RenameExternalFileCalls(stub func(context.Context, string, string) error) { + fake.renameExternalFileMutex.Lock() + defer fake.renameExternalFileMutex.Unlock() + fake.RenameExternalFileStub = stub +} + +func (fake *FakeFileServiceOperatorInterface) RenameExternalFileArgsForCall(i int) (context.Context, string, string) { + fake.renameExternalFileMutex.RLock() + defer fake.renameExternalFileMutex.RUnlock() + argsForCall := fake.renameExternalFileArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *FakeFileServiceOperatorInterface) RenameExternalFileReturns(result1 error) { + fake.renameExternalFileMutex.Lock() + defer fake.renameExternalFileMutex.Unlock() + fake.RenameExternalFileStub = nil + fake.renameExternalFileReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) RenameExternalFileReturnsOnCall(i int, result1 error) { + fake.renameExternalFileMutex.Lock() + defer fake.renameExternalFileMutex.Unlock() + fake.RenameExternalFileStub = nil + if fake.renameExternalFileReturnsOnCall == nil { + fake.renameExternalFileReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.renameExternalFileReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) RenameFile(arg1 context.Context, arg2 string, arg3 string, arg4 string) error { + fake.renameFileMutex.Lock() + ret, specificReturn := fake.renameFileReturnsOnCall[len(fake.renameFileArgsForCall)] + fake.renameFileArgsForCall = append(fake.renameFileArgsForCall, struct { + arg1 context.Context + arg2 string + arg3 string + arg4 string + }{arg1, arg2, arg3, arg4}) + stub := fake.RenameFileStub + fakeReturns := fake.renameFileReturns + fake.recordInvocation("RenameFile", []interface{}{arg1, arg2, arg3, arg4}) + fake.renameFileMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3, arg4) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeFileServiceOperatorInterface) RenameFileCallCount() int { + fake.renameFileMutex.RLock() + defer fake.renameFileMutex.RUnlock() + return len(fake.renameFileArgsForCall) +} + +func (fake *FakeFileServiceOperatorInterface) RenameFileCalls(stub func(context.Context, string, string, string) error) { + fake.renameFileMutex.Lock() + defer fake.renameFileMutex.Unlock() + fake.RenameFileStub = stub +} + +func (fake *FakeFileServiceOperatorInterface) RenameFileArgsForCall(i int) (context.Context, string, string, string) { + fake.renameFileMutex.RLock() + defer fake.renameFileMutex.RUnlock() + argsForCall := fake.renameFileArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4 +} + +func (fake *FakeFileServiceOperatorInterface) RenameFileReturns(result1 error) { + fake.renameFileMutex.Lock() + defer fake.renameFileMutex.Unlock() + fake.RenameFileStub = nil + fake.renameFileReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) RenameFileReturnsOnCall(i int, result1 error) { + fake.renameFileMutex.Lock() + defer fake.renameFileMutex.Unlock() + fake.RenameFileStub = nil + if fake.renameFileReturnsOnCall == nil { + fake.renameFileReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.renameFileReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) SetIsConnected(arg1 bool) { + fake.setIsConnectedMutex.Lock() + fake.setIsConnectedArgsForCall = append(fake.setIsConnectedArgsForCall, struct { + arg1 bool + }{arg1}) + stub := fake.SetIsConnectedStub + fake.recordInvocation("SetIsConnected", []interface{}{arg1}) + fake.setIsConnectedMutex.Unlock() + if stub != nil { + fake.SetIsConnectedStub(arg1) + } +} + +func (fake *FakeFileServiceOperatorInterface) SetIsConnectedCallCount() int { + fake.setIsConnectedMutex.RLock() + defer fake.setIsConnectedMutex.RUnlock() + return len(fake.setIsConnectedArgsForCall) +} + +func (fake *FakeFileServiceOperatorInterface) SetIsConnectedCalls(stub func(bool)) { + fake.setIsConnectedMutex.Lock() + defer fake.setIsConnectedMutex.Unlock() + fake.SetIsConnectedStub = stub +} + +func (fake *FakeFileServiceOperatorInterface) SetIsConnectedArgsForCall(i int) bool { + fake.setIsConnectedMutex.RLock() + defer fake.setIsConnectedMutex.RUnlock() + argsForCall := fake.setIsConnectedArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeFileServiceOperatorInterface) UpdateClient(arg1 context.Context, arg2 v1.FileServiceClient) { + fake.updateClientMutex.Lock() + fake.updateClientArgsForCall = append(fake.updateClientArgsForCall, struct { + arg1 context.Context + arg2 v1.FileServiceClient + }{arg1, arg2}) + stub := fake.UpdateClientStub + fake.recordInvocation("UpdateClient", []interface{}{arg1, arg2}) + fake.updateClientMutex.Unlock() + if stub != nil { + fake.UpdateClientStub(arg1, arg2) + } +} + +func (fake *FakeFileServiceOperatorInterface) UpdateClientCallCount() int { + fake.updateClientMutex.RLock() + defer fake.updateClientMutex.RUnlock() + return len(fake.updateClientArgsForCall) +} + +func (fake *FakeFileServiceOperatorInterface) UpdateClientCalls(stub func(context.Context, v1.FileServiceClient)) { + fake.updateClientMutex.Lock() + defer fake.updateClientMutex.Unlock() + fake.UpdateClientStub = stub +} + +func (fake *FakeFileServiceOperatorInterface) UpdateClientArgsForCall(i int) (context.Context, v1.FileServiceClient) { + fake.updateClientMutex.RLock() + defer fake.updateClientMutex.RUnlock() + argsForCall := fake.updateClientArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeFileServiceOperatorInterface) UpdateFile(arg1 context.Context, arg2 string, arg3 *v1.File) error { + fake.updateFileMutex.Lock() + ret, specificReturn := fake.updateFileReturnsOnCall[len(fake.updateFileArgsForCall)] + fake.updateFileArgsForCall = append(fake.updateFileArgsForCall, struct { + arg1 context.Context + arg2 string + arg3 *v1.File + }{arg1, arg2, arg3}) + stub := fake.UpdateFileStub + fakeReturns := fake.updateFileReturns + fake.recordInvocation("UpdateFile", []interface{}{arg1, arg2, arg3}) + fake.updateFileMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeFileServiceOperatorInterface) UpdateFileCallCount() int { + fake.updateFileMutex.RLock() + defer fake.updateFileMutex.RUnlock() + return len(fake.updateFileArgsForCall) +} + +func (fake *FakeFileServiceOperatorInterface) UpdateFileCalls(stub func(context.Context, string, *v1.File) error) { + fake.updateFileMutex.Lock() + defer fake.updateFileMutex.Unlock() + fake.UpdateFileStub = stub +} + +func (fake *FakeFileServiceOperatorInterface) UpdateFileArgsForCall(i int) (context.Context, string, *v1.File) { + fake.updateFileMutex.RLock() + defer fake.updateFileMutex.RUnlock() + argsForCall := fake.updateFileArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *FakeFileServiceOperatorInterface) UpdateFileReturns(result1 error) { + fake.updateFileMutex.Lock() + defer fake.updateFileMutex.Unlock() + fake.UpdateFileStub = nil + fake.updateFileReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) UpdateFileReturnsOnCall(i int, result1 error) { + fake.updateFileMutex.Lock() + defer fake.updateFileMutex.Unlock() + fake.UpdateFileStub = nil + if fake.updateFileReturnsOnCall == nil { + fake.updateFileReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.updateFileReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) UpdateOverview(arg1 context.Context, arg2 string, arg3 []*v1.File, arg4 string, arg5 int) error { + var arg3Copy []*v1.File + if arg3 != nil { + arg3Copy = make([]*v1.File, len(arg3)) + copy(arg3Copy, arg3) + } + fake.updateOverviewMutex.Lock() + ret, specificReturn := fake.updateOverviewReturnsOnCall[len(fake.updateOverviewArgsForCall)] + fake.updateOverviewArgsForCall = append(fake.updateOverviewArgsForCall, struct { + arg1 context.Context + arg2 string + arg3 []*v1.File + arg4 string + arg5 int + }{arg1, arg2, arg3Copy, arg4, arg5}) + stub := fake.UpdateOverviewStub + fakeReturns := fake.updateOverviewReturns + fake.recordInvocation("UpdateOverview", []interface{}{arg1, arg2, arg3Copy, arg4, arg5}) + fake.updateOverviewMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3, arg4, arg5) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeFileServiceOperatorInterface) UpdateOverviewCallCount() int { + fake.updateOverviewMutex.RLock() + defer fake.updateOverviewMutex.RUnlock() + return len(fake.updateOverviewArgsForCall) +} + +func (fake *FakeFileServiceOperatorInterface) UpdateOverviewCalls(stub func(context.Context, string, []*v1.File, string, int) error) { + fake.updateOverviewMutex.Lock() + defer fake.updateOverviewMutex.Unlock() + fake.UpdateOverviewStub = stub +} + +func (fake *FakeFileServiceOperatorInterface) UpdateOverviewArgsForCall(i int) (context.Context, string, []*v1.File, string, int) { + fake.updateOverviewMutex.RLock() + defer fake.updateOverviewMutex.RUnlock() + argsForCall := fake.updateOverviewArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4, argsForCall.arg5 +} + +func (fake *FakeFileServiceOperatorInterface) UpdateOverviewReturns(result1 error) { + fake.updateOverviewMutex.Lock() + defer fake.updateOverviewMutex.Unlock() + fake.UpdateOverviewStub = nil + fake.updateOverviewReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) UpdateOverviewReturnsOnCall(i int, result1 error) { + fake.updateOverviewMutex.Lock() + defer fake.updateOverviewMutex.Unlock() + fake.UpdateOverviewStub = nil + if fake.updateOverviewReturnsOnCall == nil { + fake.updateOverviewReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.updateOverviewReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.chunkedFileMutex.RLock() + defer fake.chunkedFileMutex.RUnlock() + fake.fileMutex.RLock() + defer fake.fileMutex.RUnlock() + fake.isConnectedMutex.RLock() + defer fake.isConnectedMutex.RUnlock() + fake.renameExternalFileMutex.RLock() + defer fake.renameExternalFileMutex.RUnlock() + fake.renameFileMutex.RLock() + defer fake.renameFileMutex.RUnlock() + fake.setIsConnectedMutex.RLock() + defer fake.setIsConnectedMutex.RUnlock() + fake.updateClientMutex.RLock() + defer fake.updateClientMutex.RUnlock() + fake.updateFileMutex.RLock() + defer fake.updateFileMutex.RUnlock() + fake.updateOverviewMutex.RLock() + defer fake.updateOverviewMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeFileServiceOperatorInterface) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} From 38913b593b910abfac8df84d93d792abc3cd1350 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Fri, 28 Nov 2025 11:21:54 +0000 Subject: [PATCH 13/14] worked on PR comments --- internal/config/defaults.go | 3 +- internal/file/file_manager_service.go | 23 +--- internal/file/file_manager_service_test.go | 113 +++++++++++++--- internal/file/file_service_operator_test.go | 140 +++++++++++++------- 4 files changed, 192 insertions(+), 87 deletions(-) diff --git a/internal/config/defaults.go b/internal/config/defaults.go index a9141d067..70b32fbd6 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -118,8 +118,7 @@ const ( DefLibDir = "/var/lib/nginx-agent" DefExternalDataSourceProxyUrl = "" - // Default allow external data sources up to 100 MB - DefExternalDataSourceMaxBytes = 100 * 1024 * 1024 + DefExternalDataSourceMaxBytes = 100 * 1024 * 1024 // default 100MB ) func DefaultFeatures() []string { diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index 0cc28e029..b5fbacec6 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -863,26 +863,15 @@ func (fms *FileManagerService) downloadExternalFile(ctx context.Context, fileAct fileName := fileAction.File.GetFileMeta().GetName() fms.externalFileHeaders[fileName] = headers - updateErr := fms.writeContentToTempFile(ctx, contentToWrite, filePath, permission) - - return updateErr -} - -func (fms *FileManagerService) writeContentToTempFile( - ctx context.Context, - content []byte, - path string, - permission string, -) error { writeErr := fms.fileOperator.Write( ctx, - content, - path, + contentToWrite, + filePath, permission, ) if writeErr != nil { - return fmt.Errorf("failed to write downloaded content to temp file %s: %w", path, writeErr) + return fmt.Errorf("failed to write downloaded content to temp file %s: %w", filePath, writeErr) } return nil @@ -966,11 +955,7 @@ func isDomainAllowed(downloadURL string, allowedDomains []string) bool { continue } - if domain == hostname { - return true - } - - if isMatchesWildcardDomain(hostname, domain) { + if domain == hostname || isMatchesWildcardDomain(hostname, domain) { return true } } diff --git a/internal/file/file_manager_service_test.go b/internal/file/file_manager_service_test.go index 89d05a4f1..dd14cdbf0 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -1210,7 +1210,7 @@ func TestFileManagerService_DetermineFileActions_ExternalFile(t *testing.T) { } //nolint:gocognit,revive,govet // cognitive complexity is 22 -func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { +func TestFileManagerService_downloadExternalFiles(t *testing.T) { type tc struct { allowedDomains []string expectContent []byte @@ -1402,7 +1402,7 @@ func TestFileManagerService_downloadExternalFiles_Cases(t *testing.T) { } } -func TestMoveOrDeleteFiles_ExternalFileRenameCalled(t *testing.T) { +func TestFileManagerService_ExternalFileRenameCalled(t *testing.T) { ctx := context.Background() fms := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{}) @@ -1438,7 +1438,7 @@ func TestMoveOrDeleteFiles_ExternalFileRenameCalled(t *testing.T) { assert.Equal(t, fileName, dstArg, "RenameExternalFile destination argument mismatch") } -func TestDownloadFileContent_MaxBytesLimit(t *testing.T) { +func TestFileManagerService_DownloadFileContent_MaxBytesLimit(t *testing.T) { ctx := context.Background() fms := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{}) @@ -1470,7 +1470,7 @@ func TestDownloadFileContent_MaxBytesLimit(t *testing.T) { assert.Equal(t, "etag-1", headers.ETag) } -func TestDownloadFileContent_InvalidProxyURL(t *testing.T) { +func TestFileManagerService_TestDownloadFileContent_InvalidProxyURL(t *testing.T) { ctx := context.Background() fms := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{}) @@ -1494,23 +1494,100 @@ func TestDownloadFileContent_InvalidProxyURL(t *testing.T) { } } -func TestIsDomainAllowed_EdgeCases(t *testing.T) { - ok := isDomainAllowed("http://%", []string{"example.com"}) - assert.False(t, ok) +func TestFileManagerService_IsDomainAllowed(t *testing.T) { + type testCase struct { + name string + url string + allowedDomains []string + expected bool + } - ok = isDomainAllowed("http://", []string{"example.com"}) - assert.False(t, ok) + tests := []testCase{ + { + name: "Invalid URL (Percent)", + url: "http://%", + allowedDomains: []string{"example.com"}, + expected: false, + }, + { + name: "Invalid URL (Empty Host)", + url: "http://", + allowedDomains: []string{"example.com"}, + expected: false, + }, + { + name: "Empty Allowed List", + url: "http://example.com/path", + allowedDomains: []string{""}, + expected: false, + }, + { + name: "Basic Match", + url: "http://example.com/path", + allowedDomains: []string{"example.com"}, + expected: true, + }, + { + name: "Wildcard Subdomain Match", + url: "http://sub.example.com/path", + allowedDomains: []string{"*.example.com"}, + expected: true, + }, + } - ok = isDomainAllowed("http://example.com/path", []string{""}) - assert.False(t, ok) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + actual := isDomainAllowed(tc.url, tc.allowedDomains) + assert.Equal(t, tc.expected, actual, "for URL: %s and domains: %v", tc.url, tc.allowedDomains) + }) + } +} - ok = isDomainAllowed("http://example.com/path", []string{"example.com"}) - assert.True(t, ok) +func TestFileManagerService_IsMatchesWildcardDomain(t *testing.T) { + type testCase struct { + name string + hostname string + pattern string + expected bool + } - ok = isDomainAllowed("http://sub.example.com/path", []string{"*.example.com"}) - assert.True(t, ok) + tests := []testCase{ + { + name: "True Match - Subdomain", + hostname: "sub.example.com", + pattern: "*.example.com", + expected: true, + }, + { + name: "True Match - Exact Base Domain", + hostname: "example.com", + pattern: "*.example.com", + expected: true, + }, + { + name: "False Match - Bad Domain Suffix", + hostname: "badexample.com", + pattern: "*.example.com", + expected: false, + }, + { + name: "False Match - No Wildcard Prefix", + hostname: "test.com", + pattern: "google.com", + expected: false, + }, + { + name: "False Match - Different Suffix", + hostname: "sub.anotherexample.com", + pattern: "*.example.com", + expected: false, + }, + } - assert.True(t, isMatchesWildcardDomain("example.com", "*.example.com")) - assert.True(t, isMatchesWildcardDomain("sub.example.com", "*.example.com")) - assert.False(t, isMatchesWildcardDomain("badexample.com", "*.example.com")) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + actual := isMatchesWildcardDomain(tc.hostname, tc.pattern) + assert.Equal(t, tc.expected, actual, "Hostname: %s, Pattern: %s", tc.hostname, tc.pattern) + }) + } } diff --git a/internal/file/file_service_operator_test.go b/internal/file/file_service_operator_test.go index f176dbe2a..5c73a09fd 100644 --- a/internal/file/file_service_operator_test.go +++ b/internal/file/file_service_operator_test.go @@ -188,66 +188,112 @@ func TestFileManagerService_UpdateFile_LargeFile(t *testing.T) { helpers.RemoveFileWithErrorCheck(t, testFile.Name()) } +//nolint:revive // complexity is 21. func TestFileServiceOperator_RenameExternalFile(t *testing.T) { - tests := []struct { - prepare func(t *testing.T) (src, dst string) - name string - wantErrMsg string - wantErr bool - }{ + type testCase struct { + name string + wantErrMsg string + setupFailurePath string + destinationPath string + expectedDstContent []byte + srcContent []byte + wantErr bool + } + + tests := []testCase{ { - name: "Test 1: success", - prepare: func(t *testing.T) (string, string) { - t.Helper() - tmp := t.TempDir() - src := filepath.Join(tmp, "src.txt") - dst := filepath.Join(tmp, "subdir", "dest.txt") - content := []byte("hello world") - require.NoError(t, os.WriteFile(src, content, 0o600)) - - return src, dst - }, - wantErr: false, + name: "Test 1: success", + srcContent: []byte("hello world"), + setupFailurePath: "", + destinationPath: "subdir/dest.txt", + wantErr: false, + expectedDstContent: []byte("hello world"), }, { - name: "Test 2: mkdirall_fail", - prepare: func(t *testing.T) (string, string) { - t.Helper() - tmp := t.TempDir() - parentFile := filepath.Join(tmp, "not_a_dir") - require.NoError(t, os.WriteFile(parentFile, []byte("block"), 0o600)) - dst := filepath.Join(parentFile, "dest.txt") - src := filepath.Join(tmp, "src.txt") - require.NoError(t, os.WriteFile(src, []byte("content"), 0o600)) - - return src, dst - }, - wantErr: true, - wantErrMsg: "failed to create directories for", + name: "Test 2: mkdirall_fail", + srcContent: []byte("content"), + setupFailurePath: "not_a_dir", + destinationPath: "not_a_dir/dest.txt", + wantErr: true, + wantErrMsg: "failed to create directories for", + expectedDstContent: nil, + }, + { + name: "Test 3: rename_fail (src does not exist)", + srcContent: nil, + setupFailurePath: "", + destinationPath: "subdir/dest.txt", + wantErr: true, + wantErrMsg: "failed to move file", + expectedDstContent: nil, + }, + { + name: "Test 4: No destination specified (empty dst path)", + srcContent: []byte("source content"), + setupFailurePath: "", + destinationPath: "", + wantErr: true, + wantErrMsg: "failed to move file:", + expectedDstContent: nil, }, { - name: "Test 3: rename_fail", - prepare: func(t *testing.T) (string, string) { - t.Helper() - tmp := t.TempDir() - src := filepath.Join(tmp, "does_not_exist.txt") - dst := filepath.Join(tmp, "subdir", "dest.txt") - - return src, dst - }, - wantErr: true, - wantErrMsg: "failed to move file", + name: "Test 5: Restricted directory (simulated permission fail)", + srcContent: []byte("source content"), + setupFailurePath: "", + destinationPath: "restricted_dir/dest.txt", + wantErr: true, + wantErrMsg: "permission denied", + expectedDstContent: nil, + }, + { + name: "Test 6: Two files to the same destination", + srcContent: []byte("source content 1"), + setupFailurePath: "", + destinationPath: "collision_dir/file.txt", + wantErr: false, + expectedDstContent: []byte("source content 2"), }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { + t.Helper() ctx := context.Background() fso := NewFileServiceOperator(types.AgentConfig(), nil, &sync.RWMutex{}) + tmp := t.TempDir() + + src := filepath.Join(tmp, "src.txt") + dst := filepath.Join(tmp, tc.destinationPath) - src, dst := tc.prepare(t) + if tc.setupFailurePath != "" { + parentFile := filepath.Join(tmp, tc.setupFailurePath) + require.NoError(t, os.WriteFile(parentFile, []byte("block"), 0o600)) + } + + if tc.srcContent != nil { + require.NoError(t, os.WriteFile(src, tc.srcContent, 0o600)) + } + + if tc.name == "Test 6: Two files to the same destination" { + src1 := src + dstCollision := dst + require.NoError(t, fso.RenameExternalFile(ctx, src1, dstCollision), "initial rename must succeed") + + src2 := filepath.Join(tmp, "src2.txt") + content2 := []byte("source content 2") + require.NoError(t, os.WriteFile(src2, content2, 0o600)) + + src = src2 + dst = dstCollision + } + + if tc.name == "Test 5: Restricted directory (simulated permission fail)" { + parentDir := filepath.Dir(dst) + require.NoError(t, os.MkdirAll(parentDir, 0o500)) + } err := fso.RenameExternalFile(ctx, src, dst) + if tc.wantErr { require.Error(t, err) if tc.wantErrMsg != "" { @@ -261,12 +307,10 @@ func TestFileServiceOperator_RenameExternalFile(t *testing.T) { dstContent, readErr := os.ReadFile(dst) require.NoError(t, readErr) - if tc.name == "success" { - require.Equal(t, []byte("hello world"), dstContent) - } + require.Equal(t, tc.expectedDstContent, dstContent) _, statErr := os.Stat(src) - require.True(t, os.IsNotExist(statErr)) + require.True(t, os.IsNotExist(statErr), "Source file should not exist after successful rename") }) } } From fb3b6130cf04757a6b04c3544233a8219c5d0ca1 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Tue, 9 Dec 2025 19:07:20 +0000 Subject: [PATCH 14/14] removed duplicate rename function --- api/grpc/mpi/v1/command_grpc.pb.go | 12 +- api/grpc/mpi/v1/files_grpc.pb.go | 16 +- internal/file/file_manager_service.go | 14 +- internal/file/file_manager_service_test.go | 37 ---- internal/file/file_service_operator.go | 28 +-- internal/file/file_service_operator_test.go | 127 ------------- .../fake_file_service_operator_interface.go | 174 +++++++++--------- 7 files changed, 116 insertions(+), 292 deletions(-) diff --git a/api/grpc/mpi/v1/command_grpc.pb.go b/api/grpc/mpi/v1/command_grpc.pb.go index dbf61a337..ba20831d9 100644 --- a/api/grpc/mpi/v1/command_grpc.pb.go +++ b/api/grpc/mpi/v1/command_grpc.pb.go @@ -8,7 +8,7 @@ // Code generated by protoc-gen-go-grpc. DO NOT EDIT. // versions: -// - protoc-gen-go-grpc v1.5.1 +// - protoc-gen-go-grpc v1.6.0 // - protoc (unknown) // source: mpi/v1/command.proto @@ -144,16 +144,16 @@ type CommandServiceServer interface { type UnimplementedCommandServiceServer struct{} func (UnimplementedCommandServiceServer) CreateConnection(context.Context, *CreateConnectionRequest) (*CreateConnectionResponse, error) { - return nil, status.Errorf(codes.Unimplemented, "method CreateConnection not implemented") + return nil, status.Error(codes.Unimplemented, "method CreateConnection not implemented") } func (UnimplementedCommandServiceServer) UpdateDataPlaneStatus(context.Context, *UpdateDataPlaneStatusRequest) (*UpdateDataPlaneStatusResponse, error) { - return nil, status.Errorf(codes.Unimplemented, "method UpdateDataPlaneStatus not implemented") + return nil, status.Error(codes.Unimplemented, "method UpdateDataPlaneStatus not implemented") } func (UnimplementedCommandServiceServer) UpdateDataPlaneHealth(context.Context, *UpdateDataPlaneHealthRequest) (*UpdateDataPlaneHealthResponse, error) { - return nil, status.Errorf(codes.Unimplemented, "method UpdateDataPlaneHealth not implemented") + return nil, status.Error(codes.Unimplemented, "method UpdateDataPlaneHealth not implemented") } func (UnimplementedCommandServiceServer) Subscribe(grpc.BidiStreamingServer[DataPlaneResponse, ManagementPlaneRequest]) error { - return status.Errorf(codes.Unimplemented, "method Subscribe not implemented") + return status.Error(codes.Unimplemented, "method Subscribe not implemented") } func (UnimplementedCommandServiceServer) testEmbeddedByValue() {} @@ -165,7 +165,7 @@ type UnsafeCommandServiceServer interface { } func RegisterCommandServiceServer(s grpc.ServiceRegistrar, srv CommandServiceServer) { - // If the following call pancis, it indicates UnimplementedCommandServiceServer was + // If the following call panics, it indicates UnimplementedCommandServiceServer was // embedded by pointer and is nil. This will cause panics if an // unimplemented method is ever invoked, so we test this at initialization // time to prevent it from happening at runtime later due to I/O. diff --git a/api/grpc/mpi/v1/files_grpc.pb.go b/api/grpc/mpi/v1/files_grpc.pb.go index 69efda491..80ed54f75 100644 --- a/api/grpc/mpi/v1/files_grpc.pb.go +++ b/api/grpc/mpi/v1/files_grpc.pb.go @@ -5,7 +5,7 @@ // Code generated by protoc-gen-go-grpc. DO NOT EDIT. // versions: -// - protoc-gen-go-grpc v1.5.1 +// - protoc-gen-go-grpc v1.6.0 // - protoc (unknown) // source: mpi/v1/files.proto @@ -174,22 +174,22 @@ type FileServiceServer interface { type UnimplementedFileServiceServer struct{} func (UnimplementedFileServiceServer) GetOverview(context.Context, *GetOverviewRequest) (*GetOverviewResponse, error) { - return nil, status.Errorf(codes.Unimplemented, "method GetOverview not implemented") + return nil, status.Error(codes.Unimplemented, "method GetOverview not implemented") } func (UnimplementedFileServiceServer) UpdateOverview(context.Context, *UpdateOverviewRequest) (*UpdateOverviewResponse, error) { - return nil, status.Errorf(codes.Unimplemented, "method UpdateOverview not implemented") + return nil, status.Error(codes.Unimplemented, "method UpdateOverview not implemented") } func (UnimplementedFileServiceServer) GetFile(context.Context, *GetFileRequest) (*GetFileResponse, error) { - return nil, status.Errorf(codes.Unimplemented, "method GetFile not implemented") + return nil, status.Error(codes.Unimplemented, "method GetFile not implemented") } func (UnimplementedFileServiceServer) UpdateFile(context.Context, *UpdateFileRequest) (*UpdateFileResponse, error) { - return nil, status.Errorf(codes.Unimplemented, "method UpdateFile not implemented") + return nil, status.Error(codes.Unimplemented, "method UpdateFile not implemented") } func (UnimplementedFileServiceServer) GetFileStream(*GetFileRequest, grpc.ServerStreamingServer[FileDataChunk]) error { - return status.Errorf(codes.Unimplemented, "method GetFileStream not implemented") + return status.Error(codes.Unimplemented, "method GetFileStream not implemented") } func (UnimplementedFileServiceServer) UpdateFileStream(grpc.ClientStreamingServer[FileDataChunk, UpdateFileResponse]) error { - return status.Errorf(codes.Unimplemented, "method UpdateFileStream not implemented") + return status.Error(codes.Unimplemented, "method UpdateFileStream not implemented") } func (UnimplementedFileServiceServer) testEmbeddedByValue() {} @@ -201,7 +201,7 @@ type UnsafeFileServiceServer interface { } func RegisterFileServiceServer(s grpc.ServiceRegistrar, srv FileServiceServer) { - // If the following call pancis, it indicates UnimplementedFileServiceServer was + // If the following call panics, it indicates UnimplementedFileServiceServer was // embedded by pointer and is nil. This will cause panics if an // unimplemented method is ever invoked, so we test this at initialization // time to prevent it from happening at runtime later due to I/O. diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index b5fbacec6..224ebb142 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -85,8 +85,8 @@ type ( fileToUpdate *mpi.File, ) error SetIsConnected(isConnected bool) - RenameFile(ctx context.Context, hash, fileName, tempDir string) error - RenameExternalFile(ctx context.Context, fileName, tempDir string) error + RenameFile(ctx context.Context, fileName, tempDir string) error + ValidateFileHash(ctx context.Context, fileName, expectedHash string) error UpdateClient(ctx context.Context, fileServiceClient mpi.FileServiceClient) } @@ -646,6 +646,7 @@ func (fms *FileManagerService) downloadUpdatedFilesToTempLocation(ctx context.Co return errGroup.Wait() } +//nolint:revive // cognitive-complexity of 14 max is 12, loop is needed cant be broken up func (fms *FileManagerService) moveOrDeleteFiles(ctx context.Context, actionError error) error { actionsLoop: for _, fileAction := range fms.fileActions { @@ -664,9 +665,14 @@ actionsLoop: continue case model.Add, model.Update: - err = fms.fileServiceOperator.RenameFile(ctx, fileMeta.GetHash(), tempFilePath, fileMeta.GetName()) + err = fms.fileServiceOperator.RenameFile(ctx, tempFilePath, fileMeta.GetName()) + if err != nil { + actionError = err + break actionsLoop + } + err = fms.fileServiceOperator.ValidateFileHash(ctx, fileMeta.GetName(), fileMeta.GetHash()) case model.ExternalFile: - err = fms.fileServiceOperator.RenameExternalFile(ctx, tempFilePath, fileMeta.GetName()) + err = fms.fileServiceOperator.RenameFile(ctx, tempFilePath, fileMeta.GetName()) case model.Unchanged: slog.DebugContext(ctx, "File unchanged") } diff --git a/internal/file/file_manager_service_test.go b/internal/file/file_manager_service_test.go index dd14cdbf0..904b58086 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -21,7 +21,6 @@ import ( "time" "github.com/nginx/agent/v3/internal/config" - "github.com/nginx/agent/v3/internal/file/filefakes" "github.com/nginx/agent/v3/internal/model" "github.com/nginx/agent/v3/pkg/files" @@ -1402,42 +1401,6 @@ func TestFileManagerService_downloadExternalFiles(t *testing.T) { } } -func TestFileManagerService_ExternalFileRenameCalled(t *testing.T) { - ctx := context.Background() - fms := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{}) - - fakeFSO := &filefakes.FakeFileServiceOperatorInterface{} - - fakeFSO.RenameExternalFileStub = func(ctx context.Context, fileName, tempDir string) error { - return nil - } - - fms.fileServiceOperator = fakeFSO - - fileName := filepath.Join(t.TempDir(), "ext.conf") - fms.fileActions = map[string]*model.FileCache{ - fileName: { - File: &mpi.File{FileMeta: &mpi.FileMeta{Name: fileName}}, - Action: model.ExternalFile, - }, - } - - tempPath := tempFilePath(fileName) - reqDir := filepath.Dir(tempPath) - require.NoError(t, os.MkdirAll(reqDir, 0o755)) - require.NoError(t, os.WriteFile(tempPath, []byte("data"), 0o600)) - - err := fms.moveOrDeleteFiles(ctx, nil) - require.NoError(t, err) - - assert.Equal(t, 1, fakeFSO.RenameExternalFileCallCount(), "RenameExternalFile should be called once") - - _, srcArg, dstArg := fakeFSO.RenameExternalFileArgsForCall(0) - - assert.Equal(t, tempPath, srcArg, "RenameExternalFile source argument mismatch") - assert.Equal(t, fileName, dstArg, "RenameExternalFile destination argument mismatch") -} - func TestFileManagerService_DownloadFileContent_MaxBytesLimit(t *testing.T) { ctx := context.Background() fms := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{}) diff --git a/internal/file/file_service_operator.go b/internal/file/file_service_operator.go index 8dba2acf7..35e4731da 100644 --- a/internal/file/file_service_operator.go +++ b/internal/file/file_service_operator.go @@ -110,7 +110,7 @@ func (fso *FileServiceOperator) File( return writeErr } - return fso.validateFileHash(tempFilePath, expectedHash) + return fso.ValidateFileHash(ctx, tempFilePath, expectedHash) } func (fso *FileServiceOperator) UpdateOverview( @@ -260,7 +260,7 @@ func (fso *FileServiceOperator) ChunkedFile( return writeChunkedFileError } - return fso.validateFileHash(tempFilePath, expectedHash) + return fso.ValidateFileHash(ctx, tempFilePath, expectedHash) } func (fso *FileServiceOperator) UpdateFile( @@ -282,26 +282,9 @@ func (fso *FileServiceOperator) UpdateFile( return fso.sendUpdateFileStream(ctx, fileToUpdate, fso.agentConfig.Client.Grpc.FileChunkSize) } -func (fso *FileServiceOperator) RenameExternalFile( - ctx context.Context, source, desination string, -) error { - slog.DebugContext(ctx, fmt.Sprintf("Moving file %s to %s (no hash validation)", source, desination)) - - if err := os.MkdirAll(filepath.Dir(desination), dirPerm); err != nil { - return fmt.Errorf("failed to create directories for %s: %w", desination, err) - } - - moveErr := os.Rename(source, desination) - if moveErr != nil { - return fmt.Errorf("failed to move file: %w", moveErr) - } - - return nil -} - // renameFile, renames (moves) file from tempDir to new location to update file. func (fso *FileServiceOperator) RenameFile( - ctx context.Context, hash, source, desination string, + ctx context.Context, source, desination string, ) error { slog.DebugContext(ctx, fmt.Sprintf("Renaming file %s to %s", source, desination)) @@ -315,10 +298,11 @@ func (fso *FileServiceOperator) RenameFile( return fmt.Errorf("failed to rename file: %w", moveErr) } - return fso.validateFileHash(desination, hash) + return nil } -func (fso *FileServiceOperator) validateFileHash(filePath, expectedHash string) error { +func (fso *FileServiceOperator) ValidateFileHash(ctx context.Context, filePath, expectedHash string) error { + slog.DebugContext(ctx, "Validating file hash for file ", "file_path", filePath) content, err := os.ReadFile(filePath) if err != nil { return err diff --git a/internal/file/file_service_operator_test.go b/internal/file/file_service_operator_test.go index 5c73a09fd..f8206e145 100644 --- a/internal/file/file_service_operator_test.go +++ b/internal/file/file_service_operator_test.go @@ -187,130 +187,3 @@ func TestFileManagerService_UpdateFile_LargeFile(t *testing.T) { helpers.RemoveFileWithErrorCheck(t, testFile.Name()) } - -//nolint:revive // complexity is 21. -func TestFileServiceOperator_RenameExternalFile(t *testing.T) { - type testCase struct { - name string - wantErrMsg string - setupFailurePath string - destinationPath string - expectedDstContent []byte - srcContent []byte - wantErr bool - } - - tests := []testCase{ - { - name: "Test 1: success", - srcContent: []byte("hello world"), - setupFailurePath: "", - destinationPath: "subdir/dest.txt", - wantErr: false, - expectedDstContent: []byte("hello world"), - }, - { - name: "Test 2: mkdirall_fail", - srcContent: []byte("content"), - setupFailurePath: "not_a_dir", - destinationPath: "not_a_dir/dest.txt", - wantErr: true, - wantErrMsg: "failed to create directories for", - expectedDstContent: nil, - }, - { - name: "Test 3: rename_fail (src does not exist)", - srcContent: nil, - setupFailurePath: "", - destinationPath: "subdir/dest.txt", - wantErr: true, - wantErrMsg: "failed to move file", - expectedDstContent: nil, - }, - { - name: "Test 4: No destination specified (empty dst path)", - srcContent: []byte("source content"), - setupFailurePath: "", - destinationPath: "", - wantErr: true, - wantErrMsg: "failed to move file:", - expectedDstContent: nil, - }, - { - name: "Test 5: Restricted directory (simulated permission fail)", - srcContent: []byte("source content"), - setupFailurePath: "", - destinationPath: "restricted_dir/dest.txt", - wantErr: true, - wantErrMsg: "permission denied", - expectedDstContent: nil, - }, - { - name: "Test 6: Two files to the same destination", - srcContent: []byte("source content 1"), - setupFailurePath: "", - destinationPath: "collision_dir/file.txt", - wantErr: false, - expectedDstContent: []byte("source content 2"), - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - t.Helper() - ctx := context.Background() - fso := NewFileServiceOperator(types.AgentConfig(), nil, &sync.RWMutex{}) - tmp := t.TempDir() - - src := filepath.Join(tmp, "src.txt") - dst := filepath.Join(tmp, tc.destinationPath) - - if tc.setupFailurePath != "" { - parentFile := filepath.Join(tmp, tc.setupFailurePath) - require.NoError(t, os.WriteFile(parentFile, []byte("block"), 0o600)) - } - - if tc.srcContent != nil { - require.NoError(t, os.WriteFile(src, tc.srcContent, 0o600)) - } - - if tc.name == "Test 6: Two files to the same destination" { - src1 := src - dstCollision := dst - require.NoError(t, fso.RenameExternalFile(ctx, src1, dstCollision), "initial rename must succeed") - - src2 := filepath.Join(tmp, "src2.txt") - content2 := []byte("source content 2") - require.NoError(t, os.WriteFile(src2, content2, 0o600)) - - src = src2 - dst = dstCollision - } - - if tc.name == "Test 5: Restricted directory (simulated permission fail)" { - parentDir := filepath.Dir(dst) - require.NoError(t, os.MkdirAll(parentDir, 0o500)) - } - - err := fso.RenameExternalFile(ctx, src, dst) - - if tc.wantErr { - require.Error(t, err) - if tc.wantErrMsg != "" { - require.Contains(t, err.Error(), tc.wantErrMsg) - } - - return - } - - require.NoError(t, err) - - dstContent, readErr := os.ReadFile(dst) - require.NoError(t, readErr) - require.Equal(t, tc.expectedDstContent, dstContent) - - _, statErr := os.Stat(src) - require.True(t, os.IsNotExist(statErr), "Source file should not exist after successful rename") - }) - } -} diff --git a/internal/file/filefakes/fake_file_service_operator_interface.go b/internal/file/filefakes/fake_file_service_operator_interface.go index fa98f941a..dc559e41d 100644 --- a/internal/file/filefakes/fake_file_service_operator_interface.go +++ b/internal/file/filefakes/fake_file_service_operator_interface.go @@ -47,26 +47,12 @@ type FakeFileServiceOperatorInterface struct { isConnectedReturnsOnCall map[int]struct { result1 bool } - RenameExternalFileStub func(context.Context, string, string) error - renameExternalFileMutex sync.RWMutex - renameExternalFileArgsForCall []struct { - arg1 context.Context - arg2 string - arg3 string - } - renameExternalFileReturns struct { - result1 error - } - renameExternalFileReturnsOnCall map[int]struct { - result1 error - } - RenameFileStub func(context.Context, string, string, string) error + RenameFileStub func(context.Context, string, string) error renameFileMutex sync.RWMutex renameFileArgsForCall []struct { arg1 context.Context arg2 string arg3 string - arg4 string } renameFileReturns struct { result1 error @@ -113,6 +99,19 @@ type FakeFileServiceOperatorInterface struct { updateOverviewReturnsOnCall map[int]struct { result1 error } + ValidateFileHashStub func(context.Context, string, string) error + validateFileHashMutex sync.RWMutex + validateFileHashArgsForCall []struct { + arg1 context.Context + arg2 string + arg3 string + } + validateFileHashReturns struct { + result1 error + } + validateFileHashReturnsOnCall map[int]struct { + result1 error + } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } @@ -298,84 +297,20 @@ func (fake *FakeFileServiceOperatorInterface) IsConnectedReturnsOnCall(i int, re }{result1} } -func (fake *FakeFileServiceOperatorInterface) RenameExternalFile(arg1 context.Context, arg2 string, arg3 string) error { - fake.renameExternalFileMutex.Lock() - ret, specificReturn := fake.renameExternalFileReturnsOnCall[len(fake.renameExternalFileArgsForCall)] - fake.renameExternalFileArgsForCall = append(fake.renameExternalFileArgsForCall, struct { - arg1 context.Context - arg2 string - arg3 string - }{arg1, arg2, arg3}) - stub := fake.RenameExternalFileStub - fakeReturns := fake.renameExternalFileReturns - fake.recordInvocation("RenameExternalFile", []interface{}{arg1, arg2, arg3}) - fake.renameExternalFileMutex.Unlock() - if stub != nil { - return stub(arg1, arg2, arg3) - } - if specificReturn { - return ret.result1 - } - return fakeReturns.result1 -} - -func (fake *FakeFileServiceOperatorInterface) RenameExternalFileCallCount() int { - fake.renameExternalFileMutex.RLock() - defer fake.renameExternalFileMutex.RUnlock() - return len(fake.renameExternalFileArgsForCall) -} - -func (fake *FakeFileServiceOperatorInterface) RenameExternalFileCalls(stub func(context.Context, string, string) error) { - fake.renameExternalFileMutex.Lock() - defer fake.renameExternalFileMutex.Unlock() - fake.RenameExternalFileStub = stub -} - -func (fake *FakeFileServiceOperatorInterface) RenameExternalFileArgsForCall(i int) (context.Context, string, string) { - fake.renameExternalFileMutex.RLock() - defer fake.renameExternalFileMutex.RUnlock() - argsForCall := fake.renameExternalFileArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 -} - -func (fake *FakeFileServiceOperatorInterface) RenameExternalFileReturns(result1 error) { - fake.renameExternalFileMutex.Lock() - defer fake.renameExternalFileMutex.Unlock() - fake.RenameExternalFileStub = nil - fake.renameExternalFileReturns = struct { - result1 error - }{result1} -} - -func (fake *FakeFileServiceOperatorInterface) RenameExternalFileReturnsOnCall(i int, result1 error) { - fake.renameExternalFileMutex.Lock() - defer fake.renameExternalFileMutex.Unlock() - fake.RenameExternalFileStub = nil - if fake.renameExternalFileReturnsOnCall == nil { - fake.renameExternalFileReturnsOnCall = make(map[int]struct { - result1 error - }) - } - fake.renameExternalFileReturnsOnCall[i] = struct { - result1 error - }{result1} -} - -func (fake *FakeFileServiceOperatorInterface) RenameFile(arg1 context.Context, arg2 string, arg3 string, arg4 string) error { +func (fake *FakeFileServiceOperatorInterface) RenameFile(arg1 context.Context, arg2 string, arg3 string) error { fake.renameFileMutex.Lock() ret, specificReturn := fake.renameFileReturnsOnCall[len(fake.renameFileArgsForCall)] fake.renameFileArgsForCall = append(fake.renameFileArgsForCall, struct { arg1 context.Context arg2 string arg3 string - arg4 string - }{arg1, arg2, arg3, arg4}) + }{arg1, arg2, arg3}) stub := fake.RenameFileStub fakeReturns := fake.renameFileReturns - fake.recordInvocation("RenameFile", []interface{}{arg1, arg2, arg3, arg4}) + fake.recordInvocation("RenameFile", []interface{}{arg1, arg2, arg3}) fake.renameFileMutex.Unlock() if stub != nil { - return stub(arg1, arg2, arg3, arg4) + return stub(arg1, arg2, arg3) } if specificReturn { return ret.result1 @@ -389,17 +324,17 @@ func (fake *FakeFileServiceOperatorInterface) RenameFileCallCount() int { return len(fake.renameFileArgsForCall) } -func (fake *FakeFileServiceOperatorInterface) RenameFileCalls(stub func(context.Context, string, string, string) error) { +func (fake *FakeFileServiceOperatorInterface) RenameFileCalls(stub func(context.Context, string, string) error) { fake.renameFileMutex.Lock() defer fake.renameFileMutex.Unlock() fake.RenameFileStub = stub } -func (fake *FakeFileServiceOperatorInterface) RenameFileArgsForCall(i int) (context.Context, string, string, string) { +func (fake *FakeFileServiceOperatorInterface) RenameFileArgsForCall(i int) (context.Context, string, string) { fake.renameFileMutex.RLock() defer fake.renameFileMutex.RUnlock() argsForCall := fake.renameFileArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4 + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 } func (fake *FakeFileServiceOperatorInterface) RenameFileReturns(result1 error) { @@ -623,6 +558,69 @@ func (fake *FakeFileServiceOperatorInterface) UpdateOverviewReturnsOnCall(i int, }{result1} } +func (fake *FakeFileServiceOperatorInterface) ValidateFileHash(arg1 context.Context, arg2 string, arg3 string) error { + fake.validateFileHashMutex.Lock() + ret, specificReturn := fake.validateFileHashReturnsOnCall[len(fake.validateFileHashArgsForCall)] + fake.validateFileHashArgsForCall = append(fake.validateFileHashArgsForCall, struct { + arg1 context.Context + arg2 string + arg3 string + }{arg1, arg2, arg3}) + stub := fake.ValidateFileHashStub + fakeReturns := fake.validateFileHashReturns + fake.recordInvocation("ValidateFileHash", []interface{}{arg1, arg2, arg3}) + fake.validateFileHashMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeFileServiceOperatorInterface) ValidateFileHashCallCount() int { + fake.validateFileHashMutex.RLock() + defer fake.validateFileHashMutex.RUnlock() + return len(fake.validateFileHashArgsForCall) +} + +func (fake *FakeFileServiceOperatorInterface) ValidateFileHashCalls(stub func(context.Context, string, string) error) { + fake.validateFileHashMutex.Lock() + defer fake.validateFileHashMutex.Unlock() + fake.ValidateFileHashStub = stub +} + +func (fake *FakeFileServiceOperatorInterface) ValidateFileHashArgsForCall(i int) (context.Context, string, string) { + fake.validateFileHashMutex.RLock() + defer fake.validateFileHashMutex.RUnlock() + argsForCall := fake.validateFileHashArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *FakeFileServiceOperatorInterface) ValidateFileHashReturns(result1 error) { + fake.validateFileHashMutex.Lock() + defer fake.validateFileHashMutex.Unlock() + fake.ValidateFileHashStub = nil + fake.validateFileHashReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeFileServiceOperatorInterface) ValidateFileHashReturnsOnCall(i int, result1 error) { + fake.validateFileHashMutex.Lock() + defer fake.validateFileHashMutex.Unlock() + fake.ValidateFileHashStub = nil + if fake.validateFileHashReturnsOnCall == nil { + fake.validateFileHashReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.validateFileHashReturnsOnCall[i] = struct { + result1 error + }{result1} +} + func (fake *FakeFileServiceOperatorInterface) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() @@ -632,8 +630,6 @@ func (fake *FakeFileServiceOperatorInterface) Invocations() map[string][][]inter defer fake.fileMutex.RUnlock() fake.isConnectedMutex.RLock() defer fake.isConnectedMutex.RUnlock() - fake.renameExternalFileMutex.RLock() - defer fake.renameExternalFileMutex.RUnlock() fake.renameFileMutex.RLock() defer fake.renameFileMutex.RUnlock() fake.setIsConnectedMutex.RLock() @@ -644,6 +640,8 @@ func (fake *FakeFileServiceOperatorInterface) Invocations() map[string][][]inter defer fake.updateFileMutex.RUnlock() fake.updateOverviewMutex.RLock() defer fake.updateOverviewMutex.RUnlock() + fake.validateFileHashMutex.RLock() + defer fake.validateFileHashMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} for key, value := range fake.invocations { copiedInvocations[key] = value