Merge pull request #561 from silvin-lubecki/fix-crlf-singlefile-app

Fix: A single-file app can have CRLF and random YAML documents order

Author: chris-crone

e2e/commands_test.go

@@ -1,9 +1,11 @@
 package e2e
 
 import (
+	"bytes"
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
+	"path"
 	"path/filepath"
 	"regexp"
 	"strings"
@@ -207,37 +209,80 @@ func TestDetectApp(t *testing.T) {
 	})
 }
 
+func patchEndOfLine(t *testing.T, eol []byte, name, fromDir, toDir string) {
+	buf := golden.Get(t, filepath.Join(fromDir, name))
+	bytes.ReplaceAll(buf, []byte{'\n'}, eol)
+	assert.NilError(t, ioutil.WriteFile(filepath.Join(toDir, name), buf, 0644))
+}
+
 func TestSplitMerge(t *testing.T) {
-	cmd, cleanup := dockerCli.createTestCmd()
-	defer cleanup()
+	testCases := []struct {
+		name       string
+		lineEnding []byte
+	}{
+		{
+			name:       "without-crlf",
+			lineEnding: []byte{'\n'},
+		},
+		{
+			name:       "with-crlf",
+			lineEnding: []byte{'\r', '\n'},
+		},
+	}
+	for _, test := range testCases {
+		t.Run(test.name, func(t *testing.T) {
+			cmd, cleanup := dockerCli.createTestCmd()
+			defer cleanup()
 
-	tmpDir := fs.NewDir(t, "split_merge")
-	defer tmpDir.Remove()
+			tmpDir := fs.NewDir(t, "split_merge", fs.WithDir("my.dockerapp"))
+			defer tmpDir.Remove()
 
-	cmd.Command = dockerCli.Command("app", "merge", "testdata/render/envvariables/my.dockerapp", "--output", tmpDir.Join("remerged.dockerapp"))
-	icmd.RunCmd(cmd).Assert(t, icmd.Success)
+			inputDir := filepath.Join("render", "envvariables", "my.dockerapp")
+			testDataDir := tmpDir.Join("my.dockerapp")
 
-	cmd.Dir = tmpDir.Path()
+			// replace the line ending from the versioned test data files and put them to a temp dir
+			for _, name := range internal.FileNames {
+				patchEndOfLine(t, test.lineEnding, name, inputDir, testDataDir)
+			}
 
-	// test that inspect works on single-file
-	cmd.Command = dockerCli.Command("app", "inspect", "remerged")
-	result := icmd.RunCmd(cmd).Assert(t, icmd.Success)
-	golden.Assert(t, result.Combined(), "envvariables-inspect.golden")
+			// Merge a multi-files app to a single-file app
+			cmd.Command = dockerCli.Command("app", "merge", testDataDir, "--output", tmpDir.Join("remerged.dockerapp"))
+			icmd.RunCmd(cmd).Assert(t, icmd.Success)
 
-	// split it
-	cmd.Command = dockerCli.Command("app", "split", "remerged", "--output", "split.dockerapp")
-	icmd.RunCmd(cmd).Assert(t, icmd.Success)
+			cmd.Dir = tmpDir.Path()
 
-	cmd.Command = dockerCli.Command("app", "inspect", "remerged")
-	result = icmd.RunCmd(cmd).Assert(t, icmd.Success)
-	golden.Assert(t, result.Combined(), "envvariables-inspect.golden")
+			// test that inspect works on single-file
+			cmd.Command = dockerCli.Command("app", "inspect", "remerged")
+			result := icmd.RunCmd(cmd).Assert(t, icmd.Success)
+			golden.Assert(t, result.Combined(), "envvariables-inspect.golden")
 
-	// test inplace
-	cmd.Command = dockerCli.Command("app", "merge", "split")
-	icmd.RunCmd(cmd).Assert(t, icmd.Success)
+			// split it
+			cmd.Command = dockerCli.Command("app", "split", "remerged", "--output", "split.dockerapp")
+			icmd.RunCmd(cmd).Assert(t, icmd.Success)
 
-	cmd.Command = dockerCli.Command("app", "split", "split")
-	icmd.RunCmd(cmd).Assert(t, icmd.Success)
+			// test that inspect still works
+			cmd.Command = dockerCli.Command("app", "inspect", "remerged")
+			result = icmd.RunCmd(cmd).Assert(t, icmd.Success)
+			golden.Assert(t, result.Combined(), "envvariables-inspect.golden")
+
+			// the split app must be the same as the original one
+			manifest := fs.Expected(
+				t,
+				fs.WithMode(0755),
+				fs.WithFile(internal.MetadataFileName, string(golden.Get(t, path.Join(testDataDir, internal.MetadataFileName))), fs.WithMode(0644)),
+				fs.WithFile(internal.ComposeFileName, string(golden.Get(t, path.Join(testDataDir, internal.ComposeFileName))), fs.WithMode(0644)),
+				fs.WithFile(internal.ParametersFileName, string(golden.Get(t, path.Join(testDataDir, internal.ParametersFileName))), fs.WithMode(0644)),
+			)
+			assert.Assert(t, fs.Equal(tmpDir.Join("split.dockerapp"), manifest))
+
+			// test inplace
+			cmd.Command = dockerCli.Command("app", "merge", "split")
+			icmd.RunCmd(cmd).Assert(t, icmd.Success)
+
+			cmd.Command = dockerCli.Command("app", "split", "split")
+			icmd.RunCmd(cmd).Assert(t, icmd.Success)
+		})
+	}
 }
 
 func TestBundle(t *testing.T) {

e2e/testdata/render/singlefile/env.yml

@@ -0,0 +1 @@
+myapp.command3: bam

e2e/testdata/render/singlefile/expected.txt

@@ -0,0 +1,8 @@
+version: "3.4"
+services:
+  test:
+    command:
+    - bash
+    - -c
+    - cat bar bam
+    image: alpine:latest

e2e/testdata/render/singlefile/my.dockerapp

@@ -0,0 +1,16 @@
+version: "3.4"
+services:
+  test:
+    image: alpine:latest
+    command: bash -c "${myapp.command1} ${myapp.command2} ${myapp.command3}"
+---
+myapp:
+  command1: cat
+  command2: foo
+  command3: bar
+---
+version: 0.1.0
+name: myapp
+maintainers:
+  - name: dev
+    email: [email protected]

e2e/testdata/render/singlefile/parameters-0.yml

@@ -0,0 +1,3 @@
+myapp:
+  command2: bar
+  command3: baz

internal/packager/split.go

@@ -45,9 +45,9 @@ func Merge(app *types.App, target io.Writer) error {
 	}
 	for _, data := range [][]byte{
 		app.MetadataRaw(),
-		[]byte(types.SingleFileSeparator),
+		types.YamlSingleFileSeparator(app.HasCRLF()),
 		app.Composes()[0],
-		[]byte(types.SingleFileSeparator),
+		types.YamlSingleFileSeparator(app.HasCRLF()),
 		app.ParametersRaw()[0],
 	} {
 		if _, err := target.Write(data); err != nil {

loader/loader.go

@@ -1,38 +1,100 @@
 package loader
 
 import (
+	"bytes"
 	"io"
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"strings"
 
 	"github.com/docker/app/internal"
+	"github.com/docker/app/specification"
 	"github.com/docker/app/types"
+	"github.com/docker/cli/cli/compose/loader"
+	"github.com/docker/cli/cli/compose/schema"
 	"github.com/docker/docker/pkg/archive"
 	"github.com/pkg/errors"
 )
 
+var (
+	crlf       = []byte{'\r', '\n'}
+	lf         = []byte{'\n'}
+	delimiters = [][]byte{
+		[]byte("\r\n---\r\n"),
+		[]byte("\n---\r\n"),
+		[]byte("\r\n---\n"),
+		[]byte("\n---\n"),
+	}
+)
+
+// useCRLF detects which line break should be used
+func useCRLF(data []byte) bool {
+	nbCrlf := bytes.Count(data, crlf)
+	nbLf := bytes.Count(data, lf)
+	switch {
+	case nbCrlf == nbLf:
+		// document contains only CRLF
+		return true
+	case nbCrlf == 0:
+		// document does not contain any CRLF
+		return false
+	default:
+		// document contains mixed line breaks, so use the OS default
+		return bytes.Equal(defaultLineBreak, crlf)
+	}
+}
+
+// splitSingleFile split a multidocument using all possible document delimiters
+func splitSingleFile(data []byte) [][]byte {
+	parts := [][]byte{data}
+	for _, delimiter := range delimiters {
+		var intermediate [][]byte
+		for _, part := range parts {
+			intermediate = append(intermediate, bytes.Split(part, delimiter)...)
+		}
+		parts = intermediate
+	}
+	return parts
+}
+
 // LoadFromSingleFile loads a docker app from a single-file format (as a reader)
 func LoadFromSingleFile(path string, r io.Reader, ops ...func(*types.App) error) (*types.App, error) {
 	data, err := ioutil.ReadAll(r)
 	if err != nil {
 		return nil, errors.Wrap(err, "error reading single-file")
 	}
-	parts := strings.Split(string(data), types.SingleFileSeparator)
+
+	parts := splitSingleFile(data)
 	if len(parts) != 3 {
 		return nil, errors.Errorf("malformed single-file application: expected 3 documents, got %d", len(parts))
 	}
-	// 0. is metadata
-	metadata := strings.NewReader(parts[0])
-	// 1. is compose
-	compose := strings.NewReader(parts[1])
-	// 2. is parameters
-	parameters := strings.NewReader(parts[2])
+
+	var (
+		metadata io.Reader
+		compose  io.Reader
+		params   io.Reader
+	)
+	for i := 0; i < 3; i++ {
+		parsed, err := loader.ParseYAML(parts[i])
+		if err != nil {
+			return nil, err
+		}
+		if err := specification.Validate(parsed, internal.MetadataVersion); metadata == nil && err == nil {
+			metadata = bytes.NewBuffer(parts[i])
+		} else if err2 := schema.Validate(parsed, schema.Version(parsed)); compose == nil && err2 == nil {
+			compose = bytes.NewBuffer(parts[i])
+		} else if params == nil {
+			params = bytes.NewBuffer(parts[i])
+		} else {
+			return nil, errors.New("malformed single-file application")
+		}
+	}
+
 	appOps := append([]func(*types.App) error{
 		types.WithComposes(compose),
-		types.WithParameters(parameters),
+		types.WithParameters(params),
 		types.Metadata(metadata),
+		types.WithCRLF(useCRLF(data)),
 	}, ops...)
 	return types.NewApp(path, appOps...)
 }

loader/loader_test.go

@@ -18,26 +18,48 @@ import (
 
 const (
 	metadata = `name: my-app
-version: 1.0.0`
-	yaml = `version: "3.1"
-
+version: 1.0.0
+`
+	compose = `version: "3.1"
 services:
   web:
-    image: nginx`
-	parameters = `foo: bar`
+    image: nginx
+`
+	params = `foo: bar
+`
 )
 
 func TestLoadFromSingleFile(t *testing.T) {
-	singlefile := fmt.Sprintf(`%s
----
-%s
----
-%s`, metadata, yaml, parameters)
-	app, err := LoadFromSingleFile("my-app", strings.NewReader(singlefile))
-	assert.NilError(t, err)
-	assert.Assert(t, app != nil)
-	assert.Assert(t, is.Equal(app.Path, "my-app"))
-	assertAppContent(t, app)
+	testCases := []struct {
+		name string
+		file string
+	}{
+		{
+			name: "line-feed",
+			file: fmt.Sprintf("%s\n---\n%s\n---\n%s", metadata, compose, params),
+		},
+		{
+			name: "carriage-return-line-feed",
+			file: fmt.Sprintf("%s\r\n---\r\n%s\r\n---\r\n%s", metadata, compose, params),
+		},
+		{
+			name: "mixed-carriage-return-line-feed",
+			file: fmt.Sprintf("%s\r\n---\r\n%s\r\n---\n%s", metadata, compose, params),
+		},
+		{
+			name: "unordered-documents",
+			file: fmt.Sprintf("%s\n---\n%s\n---\n%s", params, metadata, compose),
+		},
+	}
+	for _, test := range testCases {
+		t.Run(test.name, func(t *testing.T) {
+			app, err := LoadFromSingleFile("my-app", strings.NewReader(test.file))
+			assert.NilError(t, err)
+			assert.Assert(t, app != nil)
+			assert.Assert(t, is.Equal(app.Path, "my-app"))
+			assertAppContent(t, app)
+		})
+	}
 }
 
 func TestLoadFromSingleFileInvalidReader(t *testing.T) {
@@ -46,17 +68,17 @@ func TestLoadFromSingleFileInvalidReader(t *testing.T) {
 }
 
 func TestLoadFromSingleFileMalformed(t *testing.T) {
-	_, err := LoadFromSingleFile("my-app", strings.NewReader(`foo
+	_, err := LoadFromSingleFile("my-app", strings.NewReader(`foo: foo
 ---
-bar`))
+bar: bar`))
 	assert.ErrorContains(t, err, "malformed single-file application")
 }
 
 func TestLoadFromDirectory(t *testing.T) {
 	dir := fs.NewDir(t, "my-app",
 		fs.WithFile(internal.MetadataFileName, metadata),
-		fs.WithFile(internal.ParametersFileName, parameters),
-		fs.WithFile(internal.ComposeFileName, yaml),
+		fs.WithFile(internal.ParametersFileName, params),
+		fs.WithFile(internal.ComposeFileName, compose),
 	)
 	defer dir.Remove()
 	app, err := LoadFromDirectory(dir.Path())
@@ -69,8 +91,8 @@ func TestLoadFromDirectory(t *testing.T) {
 func TestLoadFromDirectoryDeprecatedSettings(t *testing.T) {
 	dir := fs.NewDir(t, "my-app",
 		fs.WithFile(internal.MetadataFileName, metadata),
-		fs.WithFile(internal.DeprecatedSettingsFileName, parameters),
-		fs.WithFile(internal.ComposeFileName, yaml),
+		fs.WithFile(internal.DeprecatedSettingsFileName, params),
+		fs.WithFile(internal.ComposeFileName, compose),
 	)
 	defer dir.Remove()
 	app, err := LoadFromDirectory(dir.Path())
@@ -97,8 +119,8 @@ func createAppTar(t *testing.T) *fs.File {
 	t.Helper()
 	dir := fs.NewDir(t, "my-app",
 		fs.WithFile(internal.MetadataFileName, metadata),
-		fs.WithFile(internal.ParametersFileName, parameters),
-		fs.WithFile(internal.ComposeFileName, yaml),
+		fs.WithFile(internal.ParametersFileName, params),
+		fs.WithFile(internal.ComposeFileName, compose),
 	)
 	defer dir.Remove()
 	r, err := archive.TarWithOptions(dir.Path(), &archive.TarOptions{
@@ -117,9 +139,9 @@ func assertContentIs(t *testing.T, actual []byte, expected string) {
 
 func assertAppContent(t *testing.T, app *types.App) {
 	assert.Assert(t, is.Len(app.ParametersRaw(), 1))
-	assertContentIs(t, app.ParametersRaw()[0], parameters)
+	assertContentIs(t, app.ParametersRaw()[0], params)
 	assert.Assert(t, is.Len(app.Composes(), 1))
-	assertContentIs(t, app.Composes()[0], yaml)
+	assertContentIs(t, app.Composes()[0], compose)
 	assertContentIs(t, app.MetadataRaw(), metadata)
 }
 

loader/loader_unix.go

@@ -0,0 +1,7 @@
+// +build !windows
+
+package loader
+
+var (
+	defaultLineBreak = []byte{'\n'}
+)