Skip to content

Commit 13f6d1f

Browse files
Akshay2191Akshay2191
committed
worked on review comments
1 parent 3777ce0 commit 13f6d1f

File tree

4 files changed

+85
-70
lines changed

4 files changed

+85
-70
lines changed

‎internal/config/config_test.go‎

Lines changed: 32 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1633,69 +1633,60 @@ func TestValidateLabel(t *testing.T) {
16331633

16341634
func TestValidateAllowedDomains(t *testing.T) {
16351635
tests := []struct {
1636-
name string
1637-
domains []string
1638-
wantErr bool
1636+
expectedErr error
1637+
name string
1638+
domains []string
16391639
}{
16401640
{
1641-
name: "Test 1: Success: Empty slice",
1642-
domains: []string{},
1643-
wantErr: false,
1641+
name: "Test 1: Success: Empty slice",
1642+
domains: []string{},
1643+
expectedErr: nil,
16441644
},
16451645
{
1646-
name: "Test 2: Success: Nil slice",
1647-
domains: nil,
1648-
wantErr: false,
1646+
name: "Test 2: Success: Nil slice",
1647+
domains: nil,
1648+
expectedErr: nil,
16491649
},
16501650
{
1651-
name: "Test 3: Success: Valid domains",
1652-
domains: []string{"example.com", "api.nginx.com", "sub.domain.io"},
1653-
wantErr: false,
1651+
name: "Test 3: Success: Valid domains",
1652+
domains: []string{"example.com", "api.nginx.com", "sub.domain.io"},
1653+
expectedErr: nil,
16541654
},
16551655
{
1656-
name: "Test 4: Failure: Domain contains space",
1657-
domains: []string{"valid.com", "bad domain.com"},
1658-
wantErr: true,
1656+
name: "Test 4: Failure: Domain contains space",
1657+
domains: []string{"valid.com", "bad domain.com"},
1658+
expectedErr: errors.New("invalid domain found in allowed_domains"),
16591659
},
16601660
{
1661-
name: "Test 5: Failure: Empty string domain",
1662-
domains: []string{"valid.com", ""},
1663-
wantErr: true,
1661+
name: "Test 5: Failure: Empty string domain",
1662+
domains: []string{"valid.com", ""},
1663+
expectedErr: errors.New("invalid domain found in allowed_domains"),
16641664
},
16651665
{
1666-
name: "Test 6: Failure: Domain contains forward slash /",
1667-
domains: []string{"domain.com/path"},
1668-
wantErr: true,
1666+
name: "Test 6: Failure: Domain contains forward slash /",
1667+
domains: []string{"domain.com/path"},
1668+
expectedErr: errors.New("invalid domain found in allowed_domains"),
16691669
},
16701670
{
1671-
name: "Test 7: Failure: Domain contains backward slash \\",
1672-
domains: []string{"domain.com\\path"},
1673-
wantErr: true,
1671+
name: "Test 7: Failure: Domain contains backward slash \\",
1672+
domains: []string{"domain.com\\path"},
1673+
expectedErr: errors.New("invalid domain found in allowed_domains"),
16741674
},
16751675
{
1676-
name: "Test 8: Failure: Mixed valid and invalid (first is invalid)",
1677-
domains: []string{" only.com", "good.com"},
1678-
wantErr: true,
1676+
name: "Test 8: Failure: Mixed valid and invalid (first is invalid)",
1677+
domains: []string{" only.com", "good.com"},
1678+
expectedErr: errors.New("invalid domain found in allowed_domains"),
16791679
},
16801680
}
16811681

16821682
for _, tt := range tests {
16831683
t.Run(tt.name, func(t *testing.T) {
1684-
var logBuffer bytes.Buffer
1685-
logHandler := slog.NewTextHandler(&logBuffer, &slog.HandlerOptions{Level: slog.LevelError})
1686-
1687-
originalLogger := slog.Default()
1688-
slog.SetDefault(slog.New(logHandler))
1689-
defer slog.SetDefault(originalLogger)
1690-
1691-
actualErr := validateAllowedDomains(tt.domains)
1692-
1693-
if tt.wantErr {
1694-
require.Error(t, actualErr, "Expected an error but got nil.")
1695-
assert.Contains(t, logBuffer.String(), "domain specified in allowed_domains is invalid",
1696-
"Expected the error log message to be present in the output.")
1684+
err := validateAllowedDomains(tt.domains)
1685+
if tt.expectedErr == nil {
1686+
assert.NoError(t, err)
16971687
} else {
1698-
assert.NoError(t, actualErr, "Did not expect an error but got one: %v", actualErr)
1688+
require.Error(t, err)
1689+
assert.EqualError(t, err, tt.expectedErr.Error())
16991690
}
17001691
})
17011692
}

‎internal/file/external_file_operator.go‎

Lines changed: 22 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import (
2424
)
2525

2626
type ExternalFileOperator struct {
27-
fms *FileManagerService
27+
fileManagerService *FileManagerService
2828
}
2929

3030
const (
@@ -35,7 +35,7 @@ const (
3535
)
3636

3737
func NewExternalFileOperator(fms *FileManagerService) *ExternalFileOperator {
38-
return &ExternalFileOperator{fms: fms}
38+
return &ExternalFileOperator{fileManagerService: fms}
3939
}
4040

4141
func (efo *ExternalFileOperator) DownloadExternalFile(ctx context.Context, fileAction *model.FileCache,
@@ -69,7 +69,7 @@ func (efo *ExternalFileOperator) DownloadExternalFile(ctx context.Context, fileA
6969
// persist headers if present
7070
if headers.ETag != "" || headers.LastModified != "" {
7171
fileName := fileAction.File.GetFileMeta().GetName()
72-
efo.fms.externalFileHeaders[fileName] = headers
72+
efo.fileManagerService.externalFileHeaders[fileName] = headers
7373
}
7474

7575
return nil
@@ -82,9 +82,9 @@ func (efo *ExternalFileOperator) DownloadExternalFile(ctx context.Context, fileA
8282
return fmt.Errorf("downloaded file validation failed for %s: %w", fileName, err)
8383
}
8484

85-
efo.fms.externalFileHeaders[fileName] = headers
85+
efo.fileManagerService.externalFileHeaders[fileName] = headers
8686

87-
writeErr := efo.fms.fileOperator.Write(
87+
writeErr := efo.fileManagerService.fileOperator.Write(
8888
ctx,
8989
contentToWrite,
9090
filePath,
@@ -104,22 +104,22 @@ func (efo *ExternalFileOperator) downloadFileContent(ctx context.Context, file *
104104
) {
105105
fileName := file.GetFileMeta().GetName()
106106
downloadURL := file.GetExternalDataSource().GetLocation()
107-
externalConfig := efo.fms.agentConfig.ExternalDataSource
107+
externalConfig := efo.fileManagerService.agentConfig.ExternalDataSource
108108

109109
if !efo.isDomainAllowed(downloadURL, externalConfig.AllowedDomains) {
110110
return nil, DownloadHeader{}, fmt.Errorf("download URL %s is not in the allowed domains list", downloadURL)
111111
}
112112

113-
u, err := url.Parse(downloadURL)
113+
pasrsedURl, err := url.Parse(downloadURL)
114114
if err != nil {
115115
return nil, DownloadHeader{}, fmt.Errorf("failed to parse URL: %w", err)
116116
}
117117

118-
originalScheme := u.Scheme
118+
originalScheme := pasrsedURl.Scheme
119119
if originalScheme != httpScheme && originalScheme != httpsScheme {
120-
u.Scheme = httpsScheme
120+
pasrsedURl.Scheme = httpsScheme
121121
}
122-
networkURL := u.String()
122+
networkURL := pasrsedURl.String()
123123

124124
httpClient, err := efo.setupHTTPClient(ctx, externalConfig.ProxyURL.URL)
125125
if err != nil {
@@ -154,22 +154,22 @@ func (efo *ExternalFileOperator) downloadFileContent(ctx context.Context, file *
154154
case http.StatusNotModified:
155155
slog.DebugContext(ctx, "File content unchanged (304 Not Modified)", "file_name", fileName)
156156
// return empty content but preserve headers if present
157-
h := DownloadHeader{
157+
header := DownloadHeader{
158158
ETag: resp.Header.Get("ETag"),
159159
LastModified: resp.Header.Get("Last-Modified"),
160160
}
161161

162-
return nil, h, nil
162+
return nil, header, nil
163163
default:
164164
const maxErrBody = 4096
165165
var bodyMsg string
166166

167167
limited := io.LimitReader(resp.Body, maxErrBody)
168-
b, readErr := io.ReadAll(limited)
168+
body, readErr := io.ReadAll(limited)
169169
if readErr != nil {
170170
slog.DebugContext(ctx, "Failed to read error response body", "error", readErr, "status", resp.StatusCode)
171171
} else {
172-
bodyMsg = strings.TrimSpace(string(b))
172+
bodyMsg = strings.TrimSpace(string(body))
173173
}
174174

175175
if bodyMsg != "" {
@@ -181,8 +181,8 @@ func (efo *ExternalFileOperator) downloadFileContent(ctx context.Context, file *
181181
}
182182

183183
reader := io.Reader(resp.Body)
184-
if efo.fms.agentConfig.ExternalDataSource.MaxBytes > 0 {
185-
reader = io.LimitReader(resp.Body, efo.fms.agentConfig.ExternalDataSource.MaxBytes)
184+
if efo.fileManagerService.agentConfig.ExternalDataSource.MaxBytes > 0 {
185+
reader = io.LimitReader(resp.Body, efo.fileManagerService.agentConfig.ExternalDataSource.MaxBytes)
186186
}
187187

188188
content, err = io.ReadAll(reader)
@@ -196,13 +196,13 @@ func (efo *ExternalFileOperator) downloadFileContent(ctx context.Context, file *
196196
}
197197

198198
func (efo *ExternalFileOperator) isDomainAllowed(downloadURL string, allowedDomains []string) bool {
199-
u, err := url.Parse(downloadURL)
199+
parsedURL, err := url.Parse(downloadURL)
200200
if err != nil {
201201
slog.Debug("Failed to parse download URL for domain check", "url", downloadURL, "error", err)
202202
return false
203203
}
204204

205-
hostname := u.Hostname()
205+
hostname := parsedURL.Hostname()
206206
if hostname == "" {
207207
return false
208208
}
@@ -241,7 +241,7 @@ func (efo *ExternalFileOperator) setupHTTPClient(ctx context.Context, proxyURLSt
241241

242242
httpClient := &http.Client{
243243
Transport: transport,
244-
Timeout: efo.fms.agentConfig.Client.FileDownloadTimeout,
244+
Timeout: efo.fileManagerService.agentConfig.Client.FileDownloadTimeout,
245245
}
246246

247247
return httpClient, nil
@@ -250,7 +250,7 @@ func (efo *ExternalFileOperator) setupHTTPClient(ctx context.Context, proxyURLSt
250250
func (efo *ExternalFileOperator) addConditionalHeaders(ctx context.Context, req *http.Request, fileName string) {
251251
slog.DebugContext(ctx, "Proxy configured; adding headers to GET request.")
252252

253-
manifestFiles, _, manifestFileErr := efo.fms.manifestFile()
253+
manifestFiles, _, manifestFileErr := efo.fileManagerService.manifestFile()
254254

255255
if manifestFileErr != nil && !errors.Is(manifestFileErr, os.ErrNotExist) {
256256
slog.WarnContext(ctx, "Error reading manifest file for headers", "error", manifestFileErr)
@@ -299,12 +299,11 @@ func (efo *ExternalFileOperator) validateDownloadedFile(content []byte, fileName
299299
detected := mimetype.Detect(sniff).String()
300300
ext := strings.ToLower(filepath.Ext(fileName)) // includes leading dot or empty
301301

302-
// extension <-> detected MIME compatibility
303302
if err := efo.checkExtCompatibility(ext, detected); err != nil {
304303
return err
305304
}
306305

307-
allowed := efo.fms.agentConfig.ExternalDataSource.AllowedFileTypes
306+
allowed := efo.fileManagerService.agentConfig.ExternalDataSource.AllowedFileTypes
308307
if len(allowed) == 0 {
309308
return nil
310309
}
@@ -340,7 +339,7 @@ func (efo *ExternalFileOperator) checkExtCompatibility(ext, detected string) err
340339
//
341340
//nolint:revive // simple logic
342341
func (efo *ExternalFileOperator) allowedByConfig(detected, ext string) bool {
343-
for _, t := range efo.fms.agentConfig.ExternalDataSource.AllowedFileTypes {
342+
for _, t := range efo.fileManagerService.agentConfig.ExternalDataSource.AllowedFileTypes {
344343
tt := strings.ToLower(strings.TrimSpace(t))
345344
if tt == "" {
346345
continue

‎internal/file/file_manager_service.go‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ type FileManagerService struct {
125125
func NewFileManagerService(fileServiceClient mpi.FileServiceClient, agentConfig *config.Config,
126126
manifestLock *sync.RWMutex,
127127
) *FileManagerService {
128-
fms := &FileManagerService{
128+
fileManagerService := &FileManagerService{
129129
agentConfig: agentConfig,
130130
fileOperator: NewFileOperator(manifestLock),
131131
fileServiceOperator: NewFileServiceOperator(agentConfig, fileServiceClient, manifestLock),
@@ -139,9 +139,9 @@ func NewFileManagerService(fileServiceClient mpi.FileServiceClient, agentConfig
139139
}
140140

141141
// initialize the external file operator with a reference to the FileManagerService
142-
fms.externalFileOperator = NewExternalFileOperator(fms)
142+
fileManagerService.externalFileOperator = NewExternalFileOperator(fileManagerService)
143143

144-
return fms
144+
return fileManagerService
145145
}
146146

147147
func (fms *FileManagerService) ResetClient(ctx context.Context, fileServiceClient mpi.FileServiceClient) {
@@ -409,10 +409,10 @@ func (fms *FileManagerService) DetermineFileActions(
409409
continue
410410
}
411411

412-
// If it's external, we DON'T care about disk state or hashes here.
412+
// If it's external, we don't care about disk state or hashes here.
413413
// We tag it as ExternalFile and let the downloader handle the rest.
414414
if modifiedFile.File.GetExternalDataSource() != nil || (ok && currentFile.GetExternalDataSource() != nil) {
415-
slog.DebugContext(ctx, "External file detected - flagging for fetch", "file_name", fileName)
415+
slog.DebugContext(ctx, "External file requires downloading", "file_name", fileName)
416416
modifiedFile.Action = model.ExternalFile
417417
fileDiff[fileName] = modifiedFile
418418

@@ -664,7 +664,6 @@ func (fms *FileManagerService) downloadUpdatedFilesToTempLocation(ctx context.Co
664664

665665
switch fileAction.Action {
666666
case model.ExternalFile:
667-
// delegate to the new operator
668667
return fms.externalFileOperator.DownloadExternalFile(errGroupCtx, fileAction, tempFilePath)
669668
case model.Add, model.Update:
670669
slog.DebugContext(

‎internal/file/file_manager_service_test.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -684,6 +684,18 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
684684
addErr := os.WriteFile(addTestFile.Name(), addFileContent, 0o600)
685685
require.NoError(t, addErr)
686686

687+
externalFileName := filepath.Join(tempDir, "external.conf")
688+
modifiedExternalFiles := map[string]*model.FileCache{
689+
externalFileName: {
690+
File: &mpi.File{
691+
FileMeta: &mpi.FileMeta{
692+
Name: externalFileName,
693+
},
694+
ExternalDataSource: &mpi.ExternalDataSource{Location: "http://example.com/file"},
695+
},
696+
},
697+
}
698+
687699
tests := []struct {
688700
expectedError error
689701
modifiedFiles map[string]*model.FileCache
@@ -823,6 +835,20 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
823835
tempDir,
824836
),
825837
},
838+
{
839+
name: "Test 5: External file becomes ExternalFile",
840+
allowedDirs: []string{tempDir},
841+
expectedError: nil,
842+
modifiedFiles: modifiedExternalFiles,
843+
currentFiles: make(map[string]*mpi.File),
844+
expectedCache: map[string]*model.FileCache{
845+
externalFileName: {
846+
File: modifiedExternalFiles[externalFileName].File,
847+
Action: model.ExternalFile,
848+
},
849+
},
850+
expectedContent: nil,
851+
},
826852
}
827853

828854
for _, test := range tests {

0 commit comments

Comments
 (0)