Merge pull request #5912 from thaJeztah/refactor_secret_config_create

secret create, config create: refactor, use limit reader, and touch up errors

cli/command/config/create.go

@@ -51,17 +51,7 @@ func newConfigCreateCommand(dockerCli command.Cli) *cobra.Command {
 func RunConfigCreate(ctx context.Context, dockerCLI command.Cli, options CreateOptions) error {
 	apiClient := dockerCLI.Client()
 
-	var in io.Reader = dockerCLI.In()
+	configData, err := readConfigData(dockerCLI.In(), options.File)
-	if options.File != "-" {
-		file, err := sequential.Open(options.File)
-		if err != nil {
-			return err
-		}
-		in = file
-		defer file.Close()
-	}
-
-	configData, err := io.ReadAll(in)
 	if err != nil {
 		return errors.Errorf("Error reading content from %q: %v", options.File, err)
 	}
@@ -83,6 +73,54 @@ func RunConfigCreate(ctx context.Context, dockerCLI command.Cli, options CreateO
 		return err
 	}
 
-	fmt.Fprintln(dockerCLI.Out(), r.ID)
+	_, _ = fmt.Fprintln(dockerCLI.Out(), r.ID)
 	return nil
 }
+
+// maxConfigSize is the maximum byte length of the [swarm.ConfigSpec.Data] field,
+// as defined by [MaxConfigSize] in SwarmKit.
+//
+// [MaxConfigSize]: https://pkg.go.dev/github.com/moby/swarmkit/v2@v2.0.0-20250103191802-8c1959736554/manager/controlapi#MaxConfigSize
+const maxConfigSize = 1000 * 1024 // 1000KB
+
+// readConfigData reads the config from either stdin or the given fileName.
+//
+// It reads up to twice the maximum size of the config ([maxConfigSize]),
+// just in case swarm's limit changes; this is only a safeguard to prevent
+// reading arbitrary files into memory.
+func readConfigData(in io.Reader, fileName string) ([]byte, error) {
+	switch fileName {
+	case "-":
+		data, err := io.ReadAll(io.LimitReader(in, 2*maxConfigSize))
+		if err != nil {
+			return nil, fmt.Errorf("error reading from STDIN: %w", err)
+		}
+		if len(data) == 0 {
+			return nil, errors.New("error reading from STDIN: data is empty")
+		}
+		return data, nil
+	case "":
+		return nil, errors.New("config file is required")
+	default:
+		// Open file with [FILE_FLAG_SEQUENTIAL_SCAN] on Windows, which
+		// prevents Windows from aggressively caching it. We expect this
+		// file to be only read once. Given that this is expected to be
+		// a small file, this may not be a significant optimization, so
+		// we could choose to omit this, and use a regular [os.Open].
+		//
+		// [FILE_FLAG_SEQUENTIAL_SCAN]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN
+		f, err := sequential.Open(fileName)
+		if err != nil {
+			return nil, fmt.Errorf("error reading from %s: %w", fileName, err)
+		}
+		defer f.Close()
+		data, err := io.ReadAll(io.LimitReader(f, 2*maxConfigSize))
+		if err != nil {
+			return nil, fmt.Errorf("error reading from %s: %w", fileName, err)
+		}
+		if len(data) == 0 {
+			return nil, fmt.Errorf("error reading from %s: data is empty", fileName)
+		}
+		return data, nil
+	}
+}

cli/command/config/create_test.go

@@ -59,7 +59,7 @@ func TestConfigCreateErrors(t *testing.T) {
 }
 
 func TestConfigCreateWithName(t *testing.T) {
-	name := "foo"
+	const name = "config-with-name"
 	var actual []byte
 	cli := test.NewFakeCli(&fakeClient{
 		configCreateFunc: func(_ context.Context, spec swarm.ConfigSpec) (types.ConfigCreateResponse, error) {
@@ -87,7 +87,7 @@ func TestConfigCreateWithLabels(t *testing.T) {
 		"lbl1": "Label-foo",
 		"lbl2": "Label-bar",
 	}
-	name := "foo"
+	const name = "config-with-labels"
 
 	data, err := os.ReadFile(filepath.Join("testdata", configDataFile))
 	assert.NilError(t, err)
@@ -124,7 +124,7 @@ func TestConfigCreateWithTemplatingDriver(t *testing.T) {
 	expectedDriver := &swarm.Driver{
 		Name: "template-driver",
 	}
-	name := "foo"
+	const name = "config-with-template-driver"
 
 	cli := test.NewFakeCli(&fakeClient{
 		configCreateFunc: func(_ context.Context, spec swarm.ConfigSpec) (types.ConfigCreateResponse, error) {

cli/command/secret/create.go

@@ -52,14 +52,19 @@ func newSecretCreateCommand(dockerCli command.Cli) *cobra.Command {
 func runSecretCreate(ctx context.Context, dockerCli command.Cli, options createOptions) error {
 	client := dockerCli.Client()
 
-	if options.driver != "" && options.file != "" {
+	var secretData []byte
-		return errors.Errorf("When using secret driver secret data must be empty")
+	if options.driver != "" {
+		if options.file != "" {
+			return errors.Errorf("When using secret driver secret data must be empty")
+		}
+	} else {
+		var err error
+		secretData, err = readSecretData(dockerCli.In(), options.file)
+		if err != nil {
+			return err
+		}
 	}
 
-	secretData, err := readSecretData(dockerCli.In(), options.file)
-	if err != nil {
-		return errors.Errorf("Error reading content from %q: %v", options.file, err)
-	}
 	spec := swarm.SecretSpec{
 		Annotations: swarm.Annotations{
 			Name:   options.name,
@@ -82,26 +87,54 @@ func runSecretCreate(ctx context.Context, dockerCli command.Cli, options createO
 		return err
 	}
 
-	fmt.Fprintln(dockerCli.Out(), r.ID)
+	_, _ = fmt.Fprintln(dockerCli.Out(), r.ID)
 	return nil
 }
 
-func readSecretData(in io.ReadCloser, file string) ([]byte, error) {
+// maxSecretSize is the maximum byte length of the [swarm.SecretSpec.Data] field,
-	// Read secret value from external driver
+// as defined by [MaxSecretSize] in SwarmKit.
-	if file == "" {
+//
-		return nil, nil
+// [MaxSecretSize]: https://pkg.go.dev/github.com/moby/swarmkit/v2@v2.0.0-20250103191802-8c1959736554/api/validation#MaxSecretSize
-	}
+const maxSecretSize = 500 * 1024 // 500KB
-	if file != "-" {
+
-		var err error
+// readSecretData reads the secret from either stdin or the given fileName.
-		in, err = sequential.Open(file)
+//
+// It reads up to twice the maximum size of the secret ([maxSecretSize]),
+// just in case swarm's limit changes; this is only a safeguard to prevent
+// reading arbitrary files into memory.
+func readSecretData(in io.Reader, fileName string) ([]byte, error) {
+	switch fileName {
+	case "-":
+		data, err := io.ReadAll(io.LimitReader(in, 2*maxSecretSize))
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("error reading from STDIN: %w", err)
 		}
-		defer in.Close()
+		if len(data) == 0 {
+			return nil, errors.New("error reading from STDIN: data is empty")
+		}
+		return data, nil
+	case "":
+		return nil, errors.New("secret file is required")
+	default:
+		// Open file with [FILE_FLAG_SEQUENTIAL_SCAN] on Windows, which
+		// prevents Windows from aggressively caching it. We expect this
+		// file to be only read once. Given that this is expected to be
+		// a small file, this may not be a significant optimization, so
+		// we could choose to omit this, and use a regular [os.Open].
+		//
+		// [FILE_FLAG_SEQUENTIAL_SCAN]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN
+		f, err := sequential.Open(fileName)
+		if err != nil {
+			return nil, fmt.Errorf("error reading from %s: %w", fileName, err)
+		}
+		defer f.Close()
+		data, err := io.ReadAll(io.LimitReader(f, 2*maxSecretSize))
+		if err != nil {
+			return nil, fmt.Errorf("error reading from %s: %w", fileName, err)
+		}
+		if len(data) == 0 {
+			return nil, fmt.Errorf("error reading from %s: data is empty", fileName)
+		}
+		return data, nil
 	}
-	data, err := io.ReadAll(in)
-	if err != nil {
-		return nil, err
-	}
-	return data, nil
 }

cli/command/secret/create_test.go

@@ -56,7 +56,7 @@ func TestSecretCreateErrors(t *testing.T) {
 }
 
 func TestSecretCreateWithName(t *testing.T) {
-	name := "foo"
+	const name = "secret-with-name"
 	data, err := os.ReadFile(filepath.Join("testdata", secretDataFile))
 	assert.NilError(t, err)
 
@@ -89,7 +89,7 @@ func TestSecretCreateWithDriver(t *testing.T) {
 	expectedDriver := &swarm.Driver{
 		Name: "secret-driver",
 	}
-	name := "foo"
+	const name = "secret-with-driver"
 
 	cli := test.NewFakeCli(&fakeClient{
 		secretCreateFunc: func(_ context.Context, spec swarm.SecretSpec) (types.SecretCreateResponse, error) {
@@ -118,7 +118,7 @@ func TestSecretCreateWithTemplatingDriver(t *testing.T) {
 	expectedDriver := &swarm.Driver{
 		Name: "template-driver",
 	}
-	const name = "foo"
+	const name = "secret-with-template-driver"
 
 	cli := test.NewFakeCli(&fakeClient{
 		secretCreateFunc: func(_ context.Context, spec swarm.SecretSpec) (types.SecretCreateResponse, error) {
@@ -137,7 +137,7 @@ func TestSecretCreateWithTemplatingDriver(t *testing.T) {
 	})
 
 	cmd := newSecretCreateCommand(cli)
-	cmd.SetArgs([]string{name})
+	cmd.SetArgs([]string{name, filepath.Join("testdata", secretDataFile)})
 	assert.Check(t, cmd.Flags().Set("template-driver", expectedDriver.Name))
 	assert.NilError(t, cmd.Execute())
 	assert.Check(t, is.Equal("ID-"+name, strings.TrimSpace(cli.OutBuffer().String())))
@@ -148,7 +148,7 @@ func TestSecretCreateWithLabels(t *testing.T) {
 		"lbl1": "Label-foo",
 		"lbl2": "Label-bar",
 	}
-	const name = "foo"
+	const name = "secret-with-labels"
 
 	cli := test.NewFakeCli(&fakeClient{
 		secretCreateFunc: func(_ context.Context, spec swarm.SecretSpec) (types.SecretCreateResponse, error) {