Log a warning when loading bundle manifests with multiple documents (#…

…708)

decodeFileFS reads only the first YAML document in a manifest and
ignores the rest of the documents in that file. This commit makes
it so that it will log a warning when more than one document is
encountered in a single manifest file.

Inspired by this annoying bug: https://gitlab.com/nvidia/kubernetes/gpu-operator/-/merge_requests/254

Signed-off-by: Omer Tuchfeld <otuchfel@redhat.com>

pkg/registry/decode.go

@@ -6,6 +6,7 @@ import (
 	"io"
 	"io/fs"
 
+	"github.com/sirupsen/logrus"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/util/yaml"
 )
@@ -44,7 +45,7 @@ func DecodePackageManifest(reader io.Reader) (manifest *PackageManifest, err err
 	return
 }
 
-func decodeFileFS(root fs.FS, path string, into interface{}) error {
+func decodeFileFS(root fs.FS, path string, into interface{}, log *logrus.Entry) error {
 	fileReader, err := root.Open(path)
 	if err != nil {
 		return fmt.Errorf("unable to read file %s: %s", path, err)
@@ -53,5 +54,16 @@ func decodeFileFS(root fs.FS, path string, into interface{}) error {
 
 	decoder := yaml.NewYAMLOrJSONDecoder(fileReader, 30)
 
-	return decoder.Decode(into)
+	errRet := decoder.Decode(into)
+
+	if errRet == nil {
+		// Look for and warn about extra documents
+		extraDocument := &map[string]interface{}{}
+		err = decoder.Decode(extraDocument)
+		if err == nil && log != nil {
+			log.Warnf("found more than one document inside %s, using only the first one", path)
+		}
+	}
+
+	return errRet
 }

pkg/registry/decode_test.go

@@ -6,6 +6,8 @@ import (
 	"testing"
 	"testing/fstest"
 
+	"github.com/sirupsen/logrus"
+	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -97,17 +99,34 @@ func TestDecodeFileFS(t *testing.T) {
 	}
 
 	root := fstest.MapFS{
-		"foo.yaml": &fstest.MapFile{Data: []byte("bar: baz")},
+		"foo.yaml":   &fstest.MapFile{Data: []byte("bar: baz")},
+		"multi.yaml": &fstest.MapFile{Data: []byte("bar: baz\n---\nfoo: bar")},
 	}
 
+	logger, logHook := test.NewNullLogger()
+	entry := logger.WithFields(nil)
+
 	var nilPtr *foo
-	require.NoError(t, decodeFileFS(root, "foo.yaml", nilPtr))
+	require.NoError(t, decodeFileFS(root, "foo.yaml", nilPtr, entry))
 	require.Nil(t, nilPtr)
+	require.Equal(t, 0, len(logHook.Entries))
+	logHook.Reset()
 
 	ptr := &foo{}
-	require.NoError(t, decodeFileFS(root, "foo.yaml", ptr))
+	require.NoError(t, decodeFileFS(root, "foo.yaml", ptr, entry))
+	require.NotNil(t, ptr)
+	require.Equal(t, "baz", ptr.Bar)
+	require.Equal(t, 0, len(logHook.Entries))
+	logHook.Reset()
+
+	ptr = &foo{}
+	require.NoError(t, decodeFileFS(root, "multi.yaml", ptr, entry))
 	require.NotNil(t, ptr)
 	require.Equal(t, "baz", ptr.Bar)
+	require.Equal(t, 1, len(logHook.Entries))
+	require.Equal(t, logrus.WarnLevel, logHook.LastEntry().Level)
+	require.Equal(t, "found more than one document inside multi.yaml, using only the first one", logHook.LastEntry().Message)
+	logHook.Reset()
 }
 
 func loadFile(t *testing.T, path string) io.Reader {

pkg/registry/parse.go

@@ -73,7 +73,7 @@ func (b *bundleParser) addManifests(manifests fs.FS, bundle *Bundle) error {
 		}
 
 		obj := &unstructured.Unstructured{}
-		if err = decodeFileFS(manifests, name, obj); err != nil {
+		if err = decodeFileFS(manifests, name, obj, b.log); err != nil {
 			b.log.Warnf("failed to decode: %s", err)
 			continue
 		}
@@ -127,23 +127,23 @@ func (b *bundleParser) addMetadata(metadata fs.FS, bundle *Bundle) error {
 		name := f.Name()
 		if af == nil {
 			decoded := AnnotationsFile{}
-			if err = decodeFileFS(metadata, name, &decoded); err == nil {
+			if err = decodeFileFS(metadata, name, &decoded, b.log); err == nil {
 				if decoded != (AnnotationsFile{}) {
 					af = &decoded
 				}
 			}
 		}
 		if df == nil {
 			decoded := DependenciesFile{}
-			if err = decodeFileFS(metadata, name, &decoded); err == nil {
+			if err = decodeFileFS(metadata, name, &decoded, b.log); err == nil {
 				if len(decoded.Dependencies) > 0 {
 					df = &decoded
 				}
 			}
 		}
 		if pf == nil {
 			decoded := PropertiesFile{}
-			if err = decodeFileFS(metadata, name, &decoded); err == nil {
+			if err = decodeFileFS(metadata, name, &decoded, b.log); err == nil {
 				if len(decoded.Properties) > 0 {
 					pf = &decoded
 				}

vendor/modules.txt

@@ -444,6 +444,7 @@ github.com/russross/blackfriday
 # github.com/sirupsen/logrus v1.8.1
 ## explicit
 github.com/sirupsen/logrus
+github.com/sirupsen/logrus/hooks/test
 # github.com/spf13/afero v1.6.0
 ## explicit
 # github.com/spf13/cobra v1.1.3