From 207b2168c731c561a19fa9223460fe3f57d98dfc Mon Sep 17 00:00:00 2001 From: Brian Floersch Date: Mon, 26 Aug 2024 13:43:50 -0400 Subject: [PATCH] Check container symlink when a new source is scheduled (#28712) --- pkg/logs/launchers/file/launcher.go | 4 ++++ pkg/logs/launchers/file/provider/file_provider.go | 6 +++--- .../launchers/file/provider/file_provider_test.go | 8 ++++---- ...rt-lived-container-schedule-87fc0be81e7e160d.yaml | 12 ++++++++++++ 4 files changed, 23 insertions(+), 7 deletions(-) create mode 100644 releasenotes/notes/fix-short-lived-container-schedule-87fc0be81e7e160d.yaml diff --git a/pkg/logs/launchers/file/launcher.go b/pkg/logs/launchers/file/launcher.go index a2c248050547a..3eef1ab04f637 100644 --- a/pkg/logs/launchers/file/launcher.go +++ b/pkg/logs/launchers/file/launcher.go @@ -277,6 +277,10 @@ func (s *Launcher) launchTailers(source *sources.LogSource) { if s.tailers.Count() >= s.tailingLimit { return } + + if fileprovider.ShouldIgnore(s.validatePodContainerID, file) { + continue + } if tailer, isTailed := s.tailers.Get(file.GetScanKey()); isTailed { // the file is already tailed, update the existing tailer's source so that the tailer // uses this new source going forward diff --git a/pkg/logs/launchers/file/provider/file_provider.go b/pkg/logs/launchers/file/provider/file_provider.go index 2271e933e878f..545098dd8866b 100644 --- a/pkg/logs/launchers/file/provider/file_provider.go +++ b/pkg/logs/launchers/file/provider/file_provider.go @@ -132,7 +132,7 @@ func (p *FileProvider) addFilesToTailList(validatePodContainerID bool, inputFile // Add each file one by one up to the limit for j := 0; j < len(inputFiles) && len(filesToTail) < p.filesLimit; j++ { file := inputFiles[j] - if shouldIgnore(validatePodContainerID, file) { + if ShouldIgnore(validatePodContainerID, file) { continue } filesToTail = append(filesToTail, file) @@ -348,7 +348,7 @@ func (p *FileProvider) applyOrdering(files []*tailer.File) { } } -// shouldIgnore resolves symlinks in /var/log/containers in order to use that redirection +// ShouldIgnore resolves symlinks in /var/log/containers in order to use that redirection // to validate that we will be reading a file for the correct container. // // We have to make sure that the file we just detected is tagged with the correct @@ -361,7 +361,7 @@ func (p *FileProvider) applyOrdering(files []*tailer.File) { // See these links for more info: // - https://github.com/kubernetes/kubernetes/issues/58638 // - https://github.com/fabric8io/fluent-plugin-kubernetes_metadata_filter/issues/105 -func shouldIgnore(validatePodContainerID bool, file *tailer.File) bool { +func ShouldIgnore(validatePodContainerID bool, file *tailer.File) bool { // this method needs a source config to detect whether we should ignore that file or not if file == nil || file.Source == nil || file.Source.Config() == nil { return false diff --git a/pkg/logs/launchers/file/provider/file_provider_test.go b/pkg/logs/launchers/file/provider/file_provider_test.go index 8694e64eac757..efa0ad125901d 100644 --- a/pkg/logs/launchers/file/provider/file_provider_test.go +++ b/pkg/logs/launchers/file/provider/file_provider_test.go @@ -679,21 +679,21 @@ func TestContainerIDInContainerLogFile(t *testing.T) { } // we've found a symlink validating that the file we have just scanned is concerning the container we're currently processing for this source - assert.False(shouldIgnore(true, &file), "the file existing in ContainersLogsDir is pointing to the same container, scanned file should be tailed") + assert.False(ShouldIgnore(true, &file), "the file existing in ContainersLogsDir is pointing to the same container, scanned file should be tailed") // now, let's change the container for which we are trying to scan files, // because the symlink is pointing from another container, we should ignore // that log file file.Source.Config().Identifier = "1234123412341234123412341234123412341234123412341234123412341234" - assert.True(shouldIgnore(true, &file), "the file existing in ContainersLogsDir is not pointing to the same container, scanned file should be ignored") + assert.True(ShouldIgnore(true, &file), "the file existing in ContainersLogsDir is not pointing to the same container, scanned file should be ignored") // in this scenario, no link is found in /var/log/containers, thus, we should not ignore the file os.Remove("/tmp/myapp_my-namespace_myapp-abcdefabcdefabcdabcdefabcdefabcdabcdefabcdefabcdabcdefabcdefabcd.log") - assert.False(shouldIgnore(true, &file), "no files existing in ContainersLogsDir, we should not ignore the file we have just scanned") + assert.False(ShouldIgnore(true, &file), "no files existing in ContainersLogsDir, we should not ignore the file we have just scanned") // in this scenario, the file we've found doesn't look like a container ID os.Symlink("/var/log/pods/file-uuid-foo-bar.log", "/tmp/myapp_my-namespace_myapp-thisisnotacontainerIDevenifthisispointingtothecorrectfile.log") - assert.False(shouldIgnore(true, &file), "no container ID found, we don't want to ignore this scanned file") + assert.False(ShouldIgnore(true, &file), "no container ID found, we don't want to ignore this scanned file") } func TestContainerPathsAreCorrectlyIgnored(t *testing.T) { diff --git a/releasenotes/notes/fix-short-lived-container-schedule-87fc0be81e7e160d.yaml b/releasenotes/notes/fix-short-lived-container-schedule-87fc0be81e7e160d.yaml new file mode 100644 index 0000000000000..9f6d6015a8105 --- /dev/null +++ b/releasenotes/notes/fix-short-lived-container-schedule-87fc0be81e7e160d.yaml @@ -0,0 +1,12 @@ +# Each section from every release note are combined when the +# CHANGELOG.rst is rendered. So the text needs to be worded so that +# it does not depend on any information only available in another +# section. This may mean repeating some details, but each section +# must be readable independently of the other. +# +# Each section note must be formatted as reStructuredText. +--- +fixes: + - | + Fixed a rare issue where short-lived containers could cause + logs to be sent with the wrong container ID.