Skip to content

Commit c25bb76

Browse files
aphralGdhurley
aphralG
and
dhurley
authored
Update Manifest file during a ConfigContextUpdate (#1075)
--------- Co-authored-by: Donal Hurley <d.hurley@f5.com>
1 parent f21a747 commit c25bb76

File tree

9 files changed

+252
-48
lines changed

9 files changed

+252
-48
lines changed

‎internal/config/config.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ func checkCollectorConfiguration(collector *Collector, config *Config) {
139139
if isOTelExporterConfigured(collector) && config.IsGrpcClientConfigured() && config.IsAuthConfigured() &&
140140
config.IsTLSConfigured() {
141141
slog.Info("No collector configuration found in NGINX Agent config, command server configuration found." +
142-
"Using default collector configuration")
142+
" Using default collector configuration")
143143
defaultCollector(collector, config)
144144
}
145145
}

‎internal/file/file_manager_service.go‎

Lines changed: 87 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ type (
6161
Rollback(ctx context.Context, instanceID string) error
6262
UpdateFile(ctx context.Context, instanceID string, fileToUpdate *mpi.File) error
6363
ClearCache()
64-
UpdateCurrentFilesOnDisk(updateFiles map[string]*mpi.File)
64+
UpdateCurrentFilesOnDisk(ctx context.Context, updateFiles map[string]*mpi.File, referenced bool) error
6565
DetermineFileActions(currentFiles map[string]*mpi.File, modifiedFiles map[string]*model.FileCache) (
6666
map[string]*model.FileCache, map[string][]byte, error)
6767
IsConnected() bool
@@ -325,8 +325,7 @@ func (fms *FileManagerService) ConfigApply(ctx context.Context,
325325
}
326326
fileOverviewFiles := files.ConvertToMapOfFiles(fileOverview.GetFiles())
327327
// Update map of current files on disk
328-
fms.UpdateCurrentFilesOnDisk(fileOverviewFiles)
329-
manifestFileErr := fms.UpdateManifestFile(fileOverviewFiles)
328+
manifestFileErr := fms.UpdateCurrentFilesOnDisk(ctx, fileOverviewFiles, false)
330329
if manifestFileErr != nil {
331330
return model.RollbackRequired, manifestFileErr
332331
}
@@ -376,7 +375,7 @@ func (fms *FileManagerService) Rollback(ctx context.Context, instanceID string)
376375
}
377376

378377
if areFilesUpdated {
379-
manifestFileErr := fms.UpdateManifestFile(fms.currentFilesOnDisk)
378+
manifestFileErr := fms.UpdateManifestFile(fms.currentFilesOnDisk, true)
380379
if manifestFileErr != nil {
381380
return manifestFileErr
382381
}
@@ -469,31 +468,40 @@ func (fms *FileManagerService) checkAllowedDirectory(checkFiles []*mpi.File) err
469468

470469
// DetermineFileActions compares two sets of files to determine the file action for each file. Returns a map of files
471470
// that have changed and a map of the contents for each updated and deleted file. Key to both maps is file path
472-
// nolint: revive,cyclop
473-
func (fms *FileManagerService) DetermineFileActions(currentFiles map[string]*mpi.File,
474-
modifiedFiles map[string]*model.FileCache) (map[string]*model.FileCache, map[string][]byte, error,
471+
// nolint: revive,cyclop,gocognit
472+
func (fms *FileManagerService) DetermineFileActions(
473+
currentFiles map[string]*mpi.File,
474+
modifiedFiles map[string]*model.FileCache,
475+
) (
476+
map[string]*model.FileCache,
477+
map[string][]byte,
478+
error,
475479
) {
476480
fms.filesMutex.Lock()
477481
defer fms.filesMutex.Unlock()
478482

479483
fileDiff := make(map[string]*model.FileCache) // Files that have changed, key is file name
480484
fileContents := make(map[string][]byte) // contents of the file, key is file name
481485

482-
manifestFiles, manifestFileErr := fms.manifestFile(currentFiles)
486+
manifestFiles, filesMap, manifestFileErr := fms.manifestFile()
483487

484-
if manifestFileErr != nil && manifestFiles == nil {
485-
return nil, nil, manifestFileErr
488+
if manifestFileErr != nil {
489+
if errors.Is(manifestFileErr, os.ErrNotExist) {
490+
filesMap = currentFiles
491+
} else {
492+
return nil, nil, manifestFileErr
493+
}
486494
}
487495
// if file is in manifestFiles but not in modified files, file has been deleted
488496
// copy contents, set file action
489-
for fileName, manifestFile := range manifestFiles {
497+
for fileName, manifestFile := range filesMap {
490498
_, exists := modifiedFiles[fileName]
491499

492500
if !exists {
493501
// Read file contents before marking it deleted
494502
fileContent, readErr := os.ReadFile(fileName)
495503
if readErr != nil {
496-
return nil, nil, fmt.Errorf("error reading file %s, error: %w", fileName, readErr)
504+
return nil, nil, fmt.Errorf("error reading file %s: %w", fileName, readErr)
497505
}
498506
fileContents[fileName] = fileContent
499507

@@ -506,7 +514,7 @@ func (fms *FileManagerService) DetermineFileActions(currentFiles map[string]*mpi
506514

507515
for _, modifiedFile := range modifiedFiles {
508516
fileName := modifiedFile.File.GetFileMeta().GetName()
509-
currentFile, ok := manifestFiles[modifiedFile.File.GetFileMeta().GetName()]
517+
currentFile, ok := filesMap[modifiedFile.File.GetFileMeta().GetName()]
510518
// default to unchanged action
511519
modifiedFile.Action = model.Unchanged
512520

@@ -519,6 +527,8 @@ func (fms *FileManagerService) DetermineFileActions(currentFiles map[string]*mpi
519527
if !ok {
520528
modifiedFile.Action = model.Add
521529
fileDiff[modifiedFile.File.GetFileMeta().GetName()] = modifiedFile
530+
531+
continue
522532
// if file currently exists and file hash is different, file has been updated
523533
// copy contents, set file action
524534
} else if modifiedFile.File.GetFileMeta().GetHash() != currentFile.GetFileMeta().GetHash() {
@@ -530,14 +540,36 @@ func (fms *FileManagerService) DetermineFileActions(currentFiles map[string]*mpi
530540
fileContents[fileName] = fileContent
531541
fileDiff[modifiedFile.File.GetFileMeta().GetName()] = modifiedFile
532542
}
543+
544+
// If the file is unreferenced we check if the file has been updated since the last time
545+
// Also check if the unreferenced file still exists
546+
if manifestFiles[modifiedFile.File.GetFileMeta().GetName()] != nil &&
547+
!manifestFiles[modifiedFile.File.GetFileMeta().GetName()].ManifestFileMeta.Referenced {
548+
if fileStats, err := os.Stat(modifiedFile.File.GetFileMeta().GetName()); errors.Is(err, os.ErrNotExist) {
549+
modifiedFile.Action = model.Add
550+
fileDiff[modifiedFile.File.GetFileMeta().GetName()] = modifiedFile
551+
} else if timestamppb.New(fileStats.ModTime()) != modifiedFile.File.GetFileMeta().GetModifiedTime() {
552+
fileContent, readErr := os.ReadFile(fileName)
553+
if readErr != nil {
554+
return nil, nil, fmt.Errorf("error reading file %s, error: %w", fileName, readErr)
555+
}
556+
modifiedFile.Action = model.Update
557+
fileContents[fileName] = fileContent
558+
fileDiff[modifiedFile.File.GetFileMeta().GetName()] = modifiedFile
559+
}
560+
}
533561
}
534562

535563
return fileDiff, fileContents, nil
536564
}
537565

538566
// UpdateCurrentFilesOnDisk updates the FileManagerService currentFilesOnDisk slice which contains the files
539567
// currently on disk
540-
func (fms *FileManagerService) UpdateCurrentFilesOnDisk(currentFiles map[string]*mpi.File) {
568+
func (fms *FileManagerService) UpdateCurrentFilesOnDisk(
569+
ctx context.Context,
570+
currentFiles map[string]*mpi.File,
571+
referenced bool,
572+
) error {
541573
fms.filesMutex.Lock()
542574
defer fms.filesMutex.Unlock()
543575

@@ -546,11 +578,37 @@ func (fms *FileManagerService) UpdateCurrentFilesOnDisk(currentFiles map[string]
546578
for _, currentFile := range currentFiles {
547579
fms.currentFilesOnDisk[currentFile.GetFileMeta().GetName()] = currentFile
548580
}
581+
582+
err := fms.UpdateManifestFile(currentFiles, referenced)
583+
if err != nil {
584+
return fmt.Errorf("failed to update manifest file: %w", err)
585+
}
586+
587+
return nil
549588
}
550589

551-
func (fms *FileManagerService) UpdateManifestFile(currentFiles map[string]*mpi.File) (err error) {
552-
manifestFiles := fms.convertToManifestFileMap(currentFiles)
553-
manifestJSON, err := json.MarshalIndent(manifestFiles, "", " ")
590+
// seems to be a control flag, avoid control coupling
591+
// nolint: revive
592+
func (fms *FileManagerService) UpdateManifestFile(currentFiles map[string]*mpi.File, referenced bool) (err error) {
593+
currentManifestFiles, _, readError := fms.manifestFile()
594+
if readError != nil && !errors.Is(readError, os.ErrNotExist) {
595+
return fmt.Errorf("unable to read manifest file: %w", readError)
596+
}
597+
598+
manifestFiles := fms.convertToManifestFileMap(currentFiles, referenced)
599+
600+
// During a config apply every file is set to unreferenced
601+
// When a new NGINX config context is detected
602+
// we update the files in the manifest by setting the referenced bool to true
603+
if currentManifestFiles != nil && referenced {
604+
for manifestFileName, manifestFile := range manifestFiles {
605+
currentManifestFiles[manifestFileName] = manifestFile
606+
}
607+
} else {
608+
currentManifestFiles = manifestFiles
609+
}
610+
611+
manifestJSON, err := json.MarshalIndent(currentManifestFiles, "", " ")
554612
if err != nil {
555613
return fmt.Errorf("unable to marshal manifest file json: %w", err)
556614
}
@@ -575,50 +633,52 @@ func (fms *FileManagerService) UpdateManifestFile(currentFiles map[string]*mpi.F
575633
return nil
576634
}
577635

578-
func (fms *FileManagerService) manifestFile(currentFiles map[string]*mpi.File) (map[string]*mpi.File, error) {
636+
func (fms *FileManagerService) manifestFile() (map[string]*model.ManifestFile, map[string]*mpi.File, error) {
579637
if _, err := os.Stat(manifestFilePath); err != nil {
580-
return currentFiles, err // Return current files if manifest directory still doesn't exist
638+
return nil, nil, err
581639
}
582640

583641
file, err := os.ReadFile(manifestFilePath)
584642
if err != nil {
585-
return nil, fmt.Errorf("failed to read manifest file: %w", err)
643+
return nil, nil, fmt.Errorf("failed to read manifest file: %w", err)
586644
}
587645

588646
var manifestFiles map[string]*model.ManifestFile
589647

590648
err = json.Unmarshal(file, &manifestFiles)
591649
if err != nil {
592-
return nil, fmt.Errorf("failed to parse manifest file: %w", err)
650+
return nil, nil, fmt.Errorf("failed to parse manifest file: %w", err)
593651
}
594652

595653
fileMap := fms.convertToFileMap(manifestFiles)
596654

597-
return fileMap, nil
655+
return manifestFiles, fileMap, nil
598656
}
599657

600658
func (fms *FileManagerService) convertToManifestFileMap(
601659
currentFiles map[string]*mpi.File,
660+
referenced bool,
602661
) map[string]*model.ManifestFile {
603662
manifestFileMap := make(map[string]*model.ManifestFile)
604663

605664
for name, currentFile := range currentFiles {
606665
if currentFile == nil || currentFile.GetFileMeta() == nil {
607666
continue
608667
}
609-
manifestFile := fms.convertToManifestFile(currentFile)
668+
manifestFile := fms.convertToManifestFile(currentFile, referenced)
610669
manifestFileMap[name] = manifestFile
611670
}
612671

613672
return manifestFileMap
614673
}
615674

616-
func (fms *FileManagerService) convertToManifestFile(file *mpi.File) *model.ManifestFile {
675+
func (fms *FileManagerService) convertToManifestFile(file *mpi.File, referenced bool) *model.ManifestFile {
617676
return &model.ManifestFile{
618677
ManifestFileMeta: &model.ManifestFileMeta{
619-
Name: file.GetFileMeta().GetName(),
620-
Size: file.GetFileMeta().GetSize(),
621-
Hash: file.GetFileMeta().GetHash(),
678+
Name: file.GetFileMeta().GetName(),
679+
Size: file.GetFileMeta().GetSize(),
680+
Hash: file.GetFileMeta().GetHash(),
681+
Referenced: referenced,
622682
},
623683
}
624684
}

‎internal/file/file_manager_service_test.go‎

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package file
77

88
import (
99
"context"
10+
"encoding/json"
1011
"fmt"
1112
"os"
1213
"path/filepath"
@@ -223,7 +224,8 @@ func TestFileManagerService_ConfigApply_Update(t *testing.T) {
223224
agentConfig := types.AgentConfig()
224225
agentConfig.AllowedDirectories = []string{tempDir}
225226
fileManagerService := NewFileManagerService(fakeFileServiceClient, agentConfig)
226-
fileManagerService.UpdateCurrentFilesOnDisk(filesOnDisk)
227+
err := fileManagerService.UpdateCurrentFilesOnDisk(ctx, filesOnDisk, false)
228+
require.NoError(t, err)
227229

228230
request := protos.CreateConfigApplyRequest(overview)
229231

@@ -269,7 +271,8 @@ func TestFileManagerService_ConfigApply_Delete(t *testing.T) {
269271
agentConfig := types.AgentConfig()
270272
agentConfig.AllowedDirectories = []string{tempDir}
271273
fileManagerService := NewFileManagerService(fakeFileServiceClient, agentConfig)
272-
fileManagerService.UpdateCurrentFilesOnDisk(filesOnDisk)
274+
err := fileManagerService.UpdateCurrentFilesOnDisk(ctx, filesOnDisk, false)
275+
require.NoError(t, err)
273276

274277
request := protos.CreateConfigApplyRequest(overview)
275278

@@ -286,7 +289,18 @@ func TestFileManagerService_ConfigApply_Delete(t *testing.T) {
286289
require.NoError(t, err)
287290
assert.NoFileExists(t, tempFile.Name())
288291
assert.Equal(t, fileManagerService.rollbackFileContents[tempFile.Name()], fileContent)
289-
assert.Equal(t, fileManagerService.fileActions[tempFile.Name()].File, filesOnDisk[tempFile.Name()])
292+
assert.Equal(t,
293+
fileManagerService.fileActions[tempFile.Name()].File.GetFileMeta().GetName(),
294+
filesOnDisk[tempFile.Name()].GetFileMeta().GetName(),
295+
)
296+
assert.Equal(t,
297+
fileManagerService.fileActions[tempFile.Name()].File.GetFileMeta().GetHash(),
298+
filesOnDisk[tempFile.Name()].GetFileMeta().GetHash(),
299+
)
300+
assert.Equal(t,
301+
fileManagerService.fileActions[tempFile.Name()].File.GetFileMeta().GetSize(),
302+
filesOnDisk[tempFile.Name()].GetFileMeta().GetSize(),
303+
)
290304
assert.Equal(t, model.OK, writeStatus)
291305
}
292306

@@ -484,10 +498,6 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
484498
unmanagedErr := os.WriteFile(unmanagedFile.Name(), unmanagedFileContent, 0o600)
485499
require.NoError(t, unmanagedErr)
486500

487-
manifestDirPath = tempDir
488-
manifestFilePath = manifestDirPath + "/manifest.json"
489-
helpers.CreateFileWithErrorCheck(t, manifestDirPath, "manifest.json")
490-
491501
tests := []struct {
492502
expectedError error
493503
modifiedFiles map[string]*model.FileCache
@@ -597,9 +607,14 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
597607

598608
for _, test := range tests {
599609
t.Run(test.name, func(tt *testing.T) {
610+
// Delete manifest file if it already exists
611+
manifestFile := CreateTestManifestFile(t, tempDir, test.currentFiles)
612+
defer manifestFile.Close()
613+
manifestDirPath = tempDir
614+
manifestFilePath = manifestFile.Name()
615+
t.Logf("path: %s", manifestFilePath)
600616
fakeFileServiceClient := &v1fakes.FakeFileServiceClient{}
601617
fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig())
602-
err = fileManagerService.UpdateManifestFile(test.currentFiles)
603618
require.NoError(tt, err)
604619
diff, contents, fileActionErr := fileManagerService.DetermineFileActions(test.currentFiles,
605620
test.modifiedFiles)
@@ -610,6 +625,22 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
610625
}
611626
}
612627

628+
func CreateTestManifestFile(t testing.TB, tempDir string, currentFiles map[string]*mpi.File) *os.File {
629+
t.Helper()
630+
fakeFileServiceClient := &v1fakes.FakeFileServiceClient{}
631+
fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig())
632+
manifestFiles := fileManagerService.convertToManifestFileMap(currentFiles, true)
633+
manifestJSON, err := json.MarshalIndent(manifestFiles, "", " ")
634+
require.NoError(t, err)
635+
file, err := os.CreateTemp(tempDir, "manifest.json")
636+
require.NoError(t, err)
637+
638+
_, err = file.Write(manifestJSON)
639+
require.NoError(t, err)
640+
641+
return file
642+
}
643+
613644
func TestFileManagerService_fileActions(t *testing.T) {
614645
ctx := context.Background()
615646
tempDir := t.TempDir()

‎internal/file/file_plugin.go‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,14 @@ func (fp *FilePlugin) handleConfigApplySuccess(ctx context.Context, msg *bus.Mes
136136
}
137137

138138
fp.fileManagerService.ClearCache()
139+
updateError := fp.fileManagerService.UpdateCurrentFilesOnDisk(
140+
ctx,
141+
files.ConvertToMapOfFiles(successMessage.ConfigContext.Files),
142+
true,
143+
)
144+
if updateError != nil {
145+
slog.ErrorContext(ctx, "Unable to update current files on disk", "error", updateError)
146+
}
139147
fp.messagePipe.Process(ctx, &bus.Message{Topic: bus.DataPlaneResponseTopic, Data: successMessage.DataPlaneResponse})
140148
}
141149

@@ -171,6 +179,7 @@ func (fp *FilePlugin) handleConfigApplyFailedRequest(ctx context.Context, msg *b
171179
}
172180

173181
func (fp *FilePlugin) handleConfigApplyRequest(ctx context.Context, msg *bus.Message) {
182+
slog.DebugContext(ctx, "File plugin received config apply request message")
174183
var response *mpi.DataPlaneResponse
175184
correlationID := logger.GetCorrelationID(ctx)
176185

@@ -197,6 +206,7 @@ func (fp *FilePlugin) handleConfigApplyRequest(ctx context.Context, msg *bus.Mes
197206

198207
switch writeStatus {
199208
case model.NoChange:
209+
slog.DebugContext(ctx, "No changes required for config apply request")
200210
dpResponse := fp.createDataPlaneResponse(
201211
correlationID,
202212
mpi.CommandResponse_COMMAND_STATUS_OK,
@@ -280,6 +290,7 @@ func (fp *FilePlugin) handleConfigApplyRequest(ctx context.Context, msg *bus.Mes
280290

281291
return
282292
case model.OK:
293+
slog.DebugContext(ctx, "Changes required for config apply request")
283294
// Send WriteConfigSuccessfulTopic with Correlation and Instance ID for use by resource plugin
284295
data := &model.ConfigApplyMessage{
285296
CorrelationID: correlationID,
@@ -298,7 +309,14 @@ func (fp *FilePlugin) handleNginxConfigUpdate(ctx context.Context, msg *bus.Mess
298309
return
299310
}
300311

301-
fp.fileManagerService.UpdateCurrentFilesOnDisk(files.ConvertToMapOfFiles(nginxConfigContext.Files))
312+
updateError := fp.fileManagerService.UpdateCurrentFilesOnDisk(
313+
ctx,
314+
files.ConvertToMapOfFiles(nginxConfigContext.Files),
315+
true,
316+
)
317+
if updateError != nil {
318+
slog.ErrorContext(ctx, "Unable to update current files on disk", "error", updateError)
319+
}
302320

303321
err := fp.fileManagerService.UpdateOverview(ctx, nginxConfigContext.InstanceID, nginxConfigContext.Files, 0)
304322
if err != nil {

0 commit comments

Comments
 (0)