From e055610b520e6a717f8f65cbbabc74310f7df650 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 12 Nov 2025 15:29:57 +0000 Subject: [PATCH 01/23] third party config support # Conflicts: # internal/config/config.go # internal/config/types.go # internal/file/file_manager_service.go --- 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 | 11 + internal/file/file_manager_service.go | 288 +++++++++++++++++++++++-- internal/file/file_service_operator.go | 17 ++ internal/model/config.go | 4 + internal/model/file.go | 1 + 9 files changed, 357 insertions(+), 23 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 265e4c05f2..ac4829734c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -158,6 +158,7 @@ func ResolveConfig() (*Config, error) { Labels: resolveLabels(), LibDir: viperInstance.GetString(LibDirPathKey), SyslogServer: resolveSyslogServer(), + ExternalDataSource: resolveExternalDataSource(), } defaultCollector(collector, config) @@ -475,6 +476,7 @@ func registerFlags() { registerCollectorFlags(fs) registerClientFlags(fs) registerDataPlaneFlags(fs) + registerExternalDataSourceFlags(fs) fs.SetNormalizeFunc(normalizeFunc) @@ -489,6 +491,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, @@ -1574,3 +1594,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 a64b68c889..4f515a684b 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1428,6 +1428,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 2b29212fe0..fd913a672e 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -116,6 +116,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 8295c1af78..c12aa3f131 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 ( @@ -143,6 +144,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 c62262fac4..c1bbeafbcc 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -43,6 +43,7 @@ type ( Client *Client `yaml:"client" mapstructure:"client"` Collector *Collector `yaml:"collector" mapstructure:"collector"` Watchers *Watchers `yaml:"watchers" mapstructure:"watchers"` + ExternalDataSource *ExternalDataSource `yaml:"external_data_source" mapstructure:"external_data_source"` SyslogServer *SyslogServer `yaml:"syslog_server" mapstructure:"syslog_server"` Labels map[string]any `yaml:"labels" mapstructure:"labels"` Version string `yaml:"-"` @@ -360,6 +361,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 08fc0f2d2c..11e700dcb8 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" "golang.org/x/sync/errgroup" "google.golang.org/grpc" @@ -40,6 +45,11 @@ const ( executePerm = 0o111 ) +type DownloadHeader struct { + ETag string + LastModified string +} + type ( fileOperator interface { Write(ctx context.Context, fileContent []byte, fileName, filePermissions string) error @@ -74,6 +84,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) } @@ -104,26 +115,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]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), - 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]DownloadHeader), + rollbackManifest: true, + manifestFilePath: agentConfig.LibDir + "/manifest.json", + manifestLock: manifestLock, } } @@ -235,7 +248,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 @@ -392,6 +405,15 @@ func (fms *FileManagerService) DetermineFileActions( continue } + // If it's external, we DON'T care about disk state or hashes here. + // We tag it as ExternalFile and let the downloader handle the rest. + if modifiedFile.File.GetExternalDataSource() != nil || (ok && currentFile.GetExternalDataSource() != nil) { + slog.DebugContext(ctx, "External file detected - flagging for fetch", "file_name", fileName) + modifiedFile.Action = model.ExternalFile + fileDiff[fileName] = modifiedFile + continue + } + // If file currently exists on disk, is being tracked in manifest and file hash is different. // Treat it as a file update. if ok && modifiedFile.File.GetFileMeta().GetHash() != currentFile.GetFileMeta().GetHash() { @@ -617,7 +639,7 @@ func (fms *FileManagerService) executeFileActions(ctx context.Context) (actionEr func (fms *FileManagerService) downloadUpdatedFilesToTempLocation(ctx context.Context) (updateError error) { var downloadFiles []*model.FileCache for _, fileAction := range fms.fileActions { - if fileAction.Action == model.Add || fileAction.Action == model.Update { + if fileAction.Action == model.Add || fileAction.Action == model.Update || fileAction.Action == model.ExternalFile { downloadFiles = append(downloadFiles, fileAction) } } @@ -650,29 +672,36 @@ func (fms *FileManagerService) downloadUpdatedFilesToTempLocation(ctx context.Co 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) { + slog.DebugContext(ctx, "Deleting file", "file", fileMeta.GetName()) + if err = os.Remove(fileMeta.GetName()); err != nil && !os.IsNotExist(err) { actionError = fmt.Errorf("error deleting file: %s error: %w", - fileAction.File.GetFileMeta().GetName(), err) + fileMeta.GetName(), err) break 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()) + 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.RenameFile(ctx, tempFilePath, fileMeta.GetName()) case model.Unchanged: slog.DebugContext(ctx, "File unchanged") } + if err != nil { + actionError = err + break actionsLoop + } } return actionError @@ -828,3 +857,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 DownloadHeader + + 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 DownloadHeader, err error) { + fileName := file.GetFileMeta().GetName() + downloadURL := file.GetExternalDataSource().GetLocation() + externalConfig := fms.agentConfig.ExternalDataSource + + if !isDomainAllowed(downloadURL, externalConfig.AllowedDomain) { + 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, DownloadHeader{}, err + } + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, downloadURL, nil) + if err != nil { + return nil, DownloadHeader{}, 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, DownloadHeader{}, 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, DownloadHeader{}, nil + default: + return nil, DownloadHeader{}, 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, 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)) + + 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 6fce049b39..c989ebfed8 100644 --- a/internal/file/file_service_operator.go +++ b/internal/file/file_service_operator.go @@ -279,6 +279,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 d13c299fda..0973fc102b 100644 --- a/internal/model/config.go +++ b/internal/model/config.go @@ -48,6 +48,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 fc6c5baca1..671a4ff859 100644 --- a/internal/model/file.go +++ b/internal/model/file.go @@ -19,4 +19,5 @@ const ( Update Delete Unchanged + ExternalFile ) From daf36e3f84eb894932149294d68c824f8b8a4235 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 12 Nov 2025 15:43:35 +0000 Subject: [PATCH 02/23] 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 11e700dcb8..081d8696f0 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -880,6 +880,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 0973fc102b..324a5ffd63 100644 --- a/internal/model/config.go +++ b/internal/model/config.go @@ -42,16 +42,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 2b878a4c4a5d0c8a18d8f0029da2073be1ce6d46 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Thu, 13 Nov 2025 12:12:30 +0000 Subject: [PATCH 03/23] 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 | 6 +++--- 5 files changed, 16 insertions(+), 6 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index ac4829734c..1d7bf614ed 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -666,6 +666,11 @@ func registerClientFlags(fs *flag.FlagSet) { DefMaxParallelFileOperations, "Maximum number of file downloads or uploads performed in parallel", ) + fs.Duration( + ClientFileDownloadTimeoutKey, + DefClientFileDownloadTimeout, + "Timeout value in seconds, to downloading file for config apply.", + ) } func registerCommandFlags(fs *flag.FlagSet) { @@ -1154,6 +1159,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 fd913a672e..cfaec2f5e9 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -84,6 +84,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 c12aa3f131..b2b1616184 100644 --- a/internal/config/flags.go +++ b/internal/config/flags.go @@ -50,6 +50,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 c1bbeafbcc..e4e443fb62 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 081d8696f0..0972c4bab5 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -20,7 +20,6 @@ import ( "strconv" "strings" "sync" - "time" "golang.org/x/sync/errgroup" "google.golang.org/grpc" @@ -413,7 +412,7 @@ func (fms *FileManagerService) DetermineFileActions( fileDiff[fileName] = modifiedFile continue } - + // If file currently exists on disk, is being tracked in manifest and file hash is different. // Treat it as a file update. if ok && modifiedFile.File.GetFileMeta().GetHash() != currentFile.GetFileMeta().GetHash() { @@ -466,6 +465,7 @@ func (fms *FileManagerService) DetermineFileActions( return fileDiff, nil } + // UpdateCurrentFilesOnDisk updates the FileManagerService currentFilesOnDisk slice which contains the files // currently on disk func (fms *FileManagerService) UpdateCurrentFilesOnDisk( @@ -1022,7 +1022,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 c3573551d02a1593a64e6eeefaee4cf0012f7e91 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Thu, 13 Nov 2025 15:48:00 +0000 Subject: [PATCH 04/23] 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 c989ebfed8..bdcf5d369c 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), @@ -228,7 +231,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), @@ -391,12 +397,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 } @@ -426,7 +435,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 d1e8a9df2b5919f80cd2b97ac1171090b9404be9 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 19 Nov 2025 07:39:21 +0000 Subject: [PATCH 05/23] 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 1d7bf614ed..44599de689 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1611,15 +1611,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 4f515a684b..293c9971c4 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1629,3 +1629,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 7e616fc3d8b22e7c38e45b12e31b662a4b55fd4a Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 19 Nov 2025 09:54:14 +0000 Subject: [PATCH 06/23] 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 5382950932..00ca45e666 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" @@ -1197,3 +1201,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 1fd2cc6c3199f2e0c635eac640feb9b160cbafaa Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 19 Nov 2025 10:35:37 +0000 Subject: [PATCH 07/23] 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 00ca45e666..8292ffa413 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" @@ -1222,7 +1223,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] @@ -1230,18 +1231,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{ @@ -1249,8 +1251,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 @@ -1259,7 +1261,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", @@ -1277,7 +1279,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"}, @@ -1305,7 +1307,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) @@ -1350,6 +1352,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 ac5bd5e63ef250c9881efe2d806fc0059bb0deeb Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 19 Nov 2025 12:51:39 +0000 Subject: [PATCH 08/23] 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 8292ffa413..cfd6c38009 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -1231,7 +1231,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 @@ -1248,14 +1248,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, @@ -1264,7 +1264,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) }, @@ -1277,7 +1277,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")) @@ -1289,7 +1289,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) }, @@ -1299,6 +1299,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 { @@ -1317,21 +1343,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{ @@ -1349,7 +1397,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 f8206e1455..f176dbe2a5 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 280ed4f689df574856f5588e9313256ce9463be1 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 19 Nov 2025 15:58:04 +0000 Subject: [PATCH 09/23] 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 cfd6c38009..62c0b69ba2 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() @@ -1423,3 +1457,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 e494337e064b5be3219b30b81d6d8462681210bd Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Thu, 20 Nov 2025 11:59:25 +0000 Subject: [PATCH 10/23] 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 0972c4bab5..e7561aa837 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -881,6 +881,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 b16b7388a6..ad752eb105 100644 --- a/test/mock/grpc/mock_management_command_service.go +++ b/test/mock/grpc/mock_management_command_service.go @@ -628,7 +628,8 @@ func addExternalDataSources(filesList []*mpi.File, filesMap map[string]*mpi.File } else { newFile := &mpi.File{ FileMeta: &mpi.FileMeta{ - Name: ed.FilePath, + Name: ed.FilePath, + Permissions: "0644", }, ExternalDataSource: &mpi.ExternalDataSource{ Location: ed.Location, From 28328a41219f8960c751aef5171f71e6e2438ba6 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Mon, 24 Nov 2025 15:59:04 +0000 Subject: [PATCH 11/23] 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 | 79 ++++++++++++---------- internal/file/file_manager_service_test.go | 2 +- 6 files changed, 68 insertions(+), 43 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 44599de689..ca4aa7a63b 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, @@ -669,7 +673,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.", ) } @@ -1624,8 +1628,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 293c9971c4..6b079fd03e 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1430,10 +1430,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, }, } } @@ -1691,7 +1691,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 cfaec2f5e9..7be1593483 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -120,6 +120,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 e09a2a5f30..bda86f4569 100644 --- a/internal/config/testdata/nginx-agent.conf +++ b/internal/config/testdata/nginx-agent.conf @@ -184,3 +184,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 e7561aa837..e904ac3096 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -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]DownloadHeader - 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]DownloadHeader), - 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, } } @@ -465,7 +465,6 @@ func (fms *FileManagerService) DetermineFileActions( return fileDiff, nil } - // UpdateCurrentFilesOnDisk updates the FileManagerService currentFilesOnDisk slice which contains the files // currently on disk func (fms *FileManagerService) UpdateCurrentFilesOnDisk( @@ -656,13 +655,22 @@ func (fms *FileManagerService) downloadUpdatedFilesToTempLocation(ctx context.Co errGroup.Go(func() error { tempFilePath := tempFilePath(fileAction.File.GetFileMeta().GetName()) - slog.DebugContext( - errGroupCtx, - "Downloading file to temp location", - "file", tempFilePath, - ) + switch fileAction.Action { + case model.ExternalFile: + return fms.downloadExternalFile(errGroupCtx, fileAction, tempFilePath) + case model.Add, model.Update: + slog.DebugContext( + errGroupCtx, + "Downloading file to temp location", + "file", tempFilePath, + ) - return fms.fileUpdate(errGroupCtx, fileAction.File, tempFilePath) + return fms.fileUpdate(errGroupCtx, fileAction.File, tempFilePath) + case model.Delete, model.Unchanged: // had to add for linter + return nil + default: + return nil + } }) } @@ -858,10 +866,12 @@ 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 @@ -887,9 +897,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 } @@ -898,12 +908,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 { @@ -922,7 +933,7 @@ func (fms *FileManagerService) downloadFileContent( downloadURL := file.GetExternalDataSource().GetLocation() externalConfig := fms.agentConfig.ExternalDataSource - if !isDomainAllowed(downloadURL, externalConfig.AllowedDomain) { + if !isDomainAllowed(downloadURL, externalConfig.AllowedDomains) { return nil, DownloadHeader{}, fmt.Errorf("download URL %s is not in the allowed domains list", downloadURL) } @@ -953,7 +964,7 @@ 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) + slog.DebugContext(ctx, "File content unchanged (304 Not Modified)", "file_name", fileName) return nil, DownloadHeader{}, nil default: return nil, DownloadHeader{}, fmt.Errorf("download failed with status code %d", resp.StatusCode) @@ -986,16 +997,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 62c0b69ba2..d37a514851 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -1444,7 +1444,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 d77faea418a6a2712820fa44eb33ee2892aef0a4 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Mon, 24 Nov 2025 17:09:52 +0000 Subject: [PATCH 12/23] 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 e904ac3096..18e49d107b 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 @@ -1006,7 +1009,7 @@ func isDomainAllowed(downloadURL string, allowedDomains []string) bool { return true } - if isWildcardMatch(hostname, domain) { + if isMatchesWildcardDomain(hostname, domain) { return true } } @@ -1067,7 +1070,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 d37a514851..f4a3898a70 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() @@ -1462,8 +1430,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{ @@ -1480,9 +1453,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) { @@ -1557,7 +1534,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 0000000000..fa98f941a3 --- /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 7b59e69dd43e3fcf0449aa60d484c69d4748fa46 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Fri, 28 Nov 2025 11:21:54 +0000 Subject: [PATCH 13/23] 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 7be1593483..815dbd5508 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -120,8 +120,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 18e49d107b..f09dd10135 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -902,26 +902,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 @@ -1005,11 +994,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 f4a3898a70..51febdc160 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -1234,7 +1234,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 @@ -1426,7 +1426,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{}) @@ -1462,7 +1462,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{}) @@ -1494,7 +1494,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{}) @@ -1518,23 +1518,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 f176dbe2a5..5c73a09fd0 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 2281b1464738fdc4a1f857f9bda6ef0123327f37 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Tue, 9 Dec 2025 19:07:20 +0000 Subject: [PATCH 14/23] removed duplicate rename function --- internal/file/file_manager_service.go | 5 +- 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 +++++++++--------- 5 files changed, 95 insertions(+), 276 deletions(-) diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index f09dd10135..c2e7bcec7b 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) } @@ -680,6 +680,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 { diff --git a/internal/file/file_manager_service_test.go b/internal/file/file_manager_service_test.go index 51febdc160..2406a52dd2 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" @@ -1426,42 +1425,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 bdcf5d369c..7ab9addc77 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( @@ -263,7 +263,7 @@ func (fso *FileServiceOperator) ChunkedFile( return writeChunkedFileError } - return fso.validateFileHash(tempFilePath, expectedHash) + return fso.ValidateFileHash(ctx, tempFilePath, expectedHash) } func (fso *FileServiceOperator) UpdateFile( @@ -285,26 +285,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)) @@ -318,10 +301,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 5c73a09fd0..f8206e1455 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 fa98f941a3..dc559e41df 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 From 3843ee254b2197f748bceee5301cf6fd7ee9510e Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Thu, 18 Dec 2025 20:46:18 +0000 Subject: [PATCH 15/23] worked on review comment --- internal/file/file_manager_service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index c2e7bcec7b..321f385684 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -686,7 +686,7 @@ actionsLoop: for _, fileAction := range fms.fileActions { var err error fileMeta := fileAction.File.GetFileMeta() - tempFilePath := tempFilePath(fileAction.File.GetFileMeta().GetName()) + tempFilePath := tempFilePath(fileMeta.GetName()) switch fileAction.Action { case model.Delete: slog.DebugContext(ctx, "Deleting file", "file", fileMeta.GetName()) From 480e40255e70d23ff66d1ff9b7885c9915f1b16b Mon Sep 17 00:00:00 2001 From: Donal Hurley Date: Wed, 19 Nov 2025 14:31:00 +0000 Subject: [PATCH 16/23] Ensure only files are in file overview in config apply request (#1395) # Conflicts: # internal/file/file_manager_service.go --- internal/file/file_manager_service.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index 321f385684..b2a166d639 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -410,7 +410,7 @@ func (fms *FileManagerService) DetermineFileActions( // If it's external, we DON'T care about disk state or hashes here. // We tag it as ExternalFile and let the downloader handle the rest. if modifiedFile.File.GetExternalDataSource() != nil || (ok && currentFile.GetExternalDataSource() != nil) { - slog.DebugContext(ctx, "External file detected - flagging for fetch", "file_name", fileName) + slog.DebugContext(ctx, "External file URI detected - flagging for fetch", "file_name", fileName) modifiedFile.Action = model.ExternalFile fileDiff[fileName] = modifiedFile continue @@ -468,6 +468,7 @@ func (fms *FileManagerService) DetermineFileActions( return fileDiff, nil } + // UpdateCurrentFilesOnDisk updates the FileManagerService currentFilesOnDisk slice which contains the files // currently on disk func (fms *FileManagerService) UpdateCurrentFilesOnDisk( From 0171367f4177819bed8d449a271c0cc5427b37b8 Mon Sep 17 00:00:00 2001 From: aphralG <108004222+aphralG@users.noreply.github.com> Date: Thu, 27 Nov 2025 14:42:17 +0000 Subject: [PATCH 17/23] Retry request if no response is sent by management plane (#1381) --- internal/config/config.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/config/config.go b/internal/config/config.go index ca4aa7a63b..defa015829 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -665,6 +665,12 @@ func registerClientFlags(fs *flag.FlagSet) { "Duration to wait for a response before retrying request", ) + fs.Duration( + ClientGRPCResponseTimeoutKey, + DefResponseTimeout, + "Duration to wait for a response before retrying request", + ) + fs.Int( ClientGRPCMaxParallelFileOperationsKey, DefMaxParallelFileOperations, From 16839e8e8d890f765c8d5b2582d33e21c8a5ee48 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 12 Nov 2025 15:29:57 +0000 Subject: [PATCH 18/23] third party config support # Conflicts: # internal/config/config.go # internal/config/types.go # internal/file/file_manager_service.go --- internal/file/file_manager_service.go | 5 ++--- internal/file/file_service_operator.go | 17 +++++++++++++++++ internal/model/config.go | 4 ++++ 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index b2a166d639..56179fc5e3 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -410,12 +410,12 @@ func (fms *FileManagerService) DetermineFileActions( // If it's external, we DON'T care about disk state or hashes here. // We tag it as ExternalFile and let the downloader handle the rest. if modifiedFile.File.GetExternalDataSource() != nil || (ok && currentFile.GetExternalDataSource() != nil) { - slog.DebugContext(ctx, "External file URI detected - flagging for fetch", "file_name", fileName) + slog.DebugContext(ctx, "External file detected - flagging for fetch", "file_name", fileName) modifiedFile.Action = model.ExternalFile fileDiff[fileName] = modifiedFile continue } - + // If file currently exists on disk, is being tracked in manifest and file hash is different. // Treat it as a file update. if ok && modifiedFile.File.GetFileMeta().GetHash() != currentFile.GetFileMeta().GetHash() { @@ -468,7 +468,6 @@ func (fms *FileManagerService) DetermineFileActions( return fileDiff, nil } - // UpdateCurrentFilesOnDisk updates the FileManagerService currentFilesOnDisk slice which contains the files // currently on disk func (fms *FileManagerService) UpdateCurrentFilesOnDisk( diff --git a/internal/file/file_service_operator.go b/internal/file/file_service_operator.go index 7ab9addc77..4dcd2bbe63 100644 --- a/internal/file/file_service_operator.go +++ b/internal/file/file_service_operator.go @@ -285,6 +285,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, source, desination string, diff --git a/internal/model/config.go b/internal/model/config.go index 324a5ffd63..46847af03d 100644 --- a/internal/model/config.go +++ b/internal/model/config.go @@ -52,6 +52,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 From 433da006e9bd1ae5061a490486669a58dfbf9fd6 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Thu, 13 Nov 2025 12:12:30 +0000 Subject: [PATCH 19/23] Added a generic file timeout --- internal/file/file_manager_service.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index 56179fc5e3..7de3a86458 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -20,6 +20,7 @@ import ( "strconv" "strings" "sync" + "time" "golang.org/x/sync/errgroup" "google.golang.org/grpc" From cba25ca60ff6ac2fa85e89a90798655826ec98b1 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Tue, 9 Dec 2025 19:07:20 +0000 Subject: [PATCH 20/23] removed duplicate rename function --- internal/file/file_service_operator.go | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/internal/file/file_service_operator.go b/internal/file/file_service_operator.go index 4dcd2bbe63..7ab9addc77 100644 --- a/internal/file/file_service_operator.go +++ b/internal/file/file_service_operator.go @@ -285,23 +285,6 @@ 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, source, desination string, From 4837339ec850de390a9b398aa4717c3f54a7ae01 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Thu, 18 Dec 2025 22:03:51 +0000 Subject: [PATCH 21/23] linter fix --- internal/config/config_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 6b079fd03e..7fc1a3b61d 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1161,6 +1161,7 @@ func agentConfig() *Config { } } +//nolint:maintidx // createConfig creates a sample Config object for testing purposes. func createConfig() *Config { return &Config{ Log: &Log{ From 3225ff33a312ed82fcb13697df5176cded75e0fe Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Thu, 18 Dec 2025 23:14:03 +0000 Subject: [PATCH 22/23] code fix for rebase --- internal/file/file_manager_service.go | 2 ++ internal/model/config.go | 4 ---- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index 7de3a86458..1ae1d31988 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -48,6 +48,8 @@ const ( executePerm = 0o111 ) +const fileDownloadTimeout = 60 * time.Second + type DownloadHeader struct { ETag string LastModified string diff --git a/internal/model/config.go b/internal/model/config.go index 46847af03d..324a5ffd63 100644 --- a/internal/model/config.go +++ b/internal/model/config.go @@ -52,10 +52,6 @@ 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 From 2e981e0ae38dd47b90218e10b9c4a428167e40f6 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Fri, 19 Dec 2025 00:26:57 +0000 Subject: [PATCH 23/23] fixing rebase errors --- internal/config/config.go | 6 ------ internal/config/types.go | 30 +++++++++++++-------------- internal/file/file_manager_service.go | 7 +++---- 3 files changed, 18 insertions(+), 25 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index defa015829..ca4aa7a63b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -665,12 +665,6 @@ func registerClientFlags(fs *flag.FlagSet) { "Duration to wait for a response before retrying request", ) - fs.Duration( - ClientGRPCResponseTimeoutKey, - DefResponseTimeout, - "Duration to wait for a response before retrying request", - ) - fs.Int( ClientGRPCMaxParallelFileOperationsKey, DefMaxParallelFileOperations, diff --git a/internal/config/types.go b/internal/config/types.go index e4e443fb62..a282c8fd83 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -36,22 +36,22 @@ 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"` + 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"` ExternalDataSource *ExternalDataSource `yaml:"external_data_source" mapstructure:"external_data_source"` - SyslogServer *SyslogServer `yaml:"syslog_server" mapstructure:"syslog_server"` - 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"` + SyslogServer *SyslogServer `yaml:"syslog_server" mapstructure:"syslog_server"` + 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"` } Log struct { diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index 1ae1d31988..fbd4b97294 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -20,7 +20,6 @@ import ( "strconv" "strings" "sync" - "time" "golang.org/x/sync/errgroup" "google.golang.org/grpc" @@ -48,8 +47,6 @@ const ( executePerm = 0o111 ) -const fileDownloadTimeout = 60 * time.Second - type DownloadHeader struct { ETag string LastModified string @@ -416,6 +413,7 @@ func (fms *FileManagerService) DetermineFileActions( slog.DebugContext(ctx, "External file detected - flagging for fetch", "file_name", fileName) modifiedFile.Action = model.ExternalFile fileDiff[fileName] = modifiedFile + continue } @@ -644,7 +642,8 @@ func (fms *FileManagerService) executeFileActions(ctx context.Context) (actionEr func (fms *FileManagerService) downloadUpdatedFilesToTempLocation(ctx context.Context) (updateError error) { var downloadFiles []*model.FileCache for _, fileAction := range fms.fileActions { - if fileAction.Action == model.Add || fileAction.Action == model.Update || fileAction.Action == model.ExternalFile { + if fileAction.Action == model.Add || fileAction.Action == model.Update || + fileAction.Action == model.ExternalFile { downloadFiles = append(downloadFiles, fileAction) } }