Skip to content

Commit ecfdf74

Browse files
thaJeztahthaJeztah
authored
Merge pull request #5912 from thaJeztah/refactor_secret_config_create
secret create, config create: refactor, use limit reader, and touch up errors
2 parents 2d74733 + d6d8ca6 commit ecfdf74

File tree

4 files changed

+113
-42
lines changed

4 files changed

+113
-42
lines changed

‎cli/command/config/create.go‎

Lines changed: 50 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -51,17 +51,7 @@ func newConfigCreateCommand(dockerCli command.Cli) *cobra.Command {
5151
func RunConfigCreate(ctx context.Context, dockerCLI command.Cli, options CreateOptions) error {
5252
apiClient := dockerCLI.Client()
5353

54-
var in io.Reader = dockerCLI.In()
55-
if options.File != "-" {
56-
file, err := sequential.Open(options.File)
57-
if err != nil {
58-
return err
59-
}
60-
in = file
61-
defer file.Close()
62-
}
63-
64-
configData, err := io.ReadAll(in)
54+
configData, err := readConfigData(dockerCLI.In(), options.File)
6555
if err != nil {
6656
return errors.Errorf("Error reading content from %q: %v", options.File, err)
6757
}
@@ -83,6 +73,54 @@ func RunConfigCreate(ctx context.Context, dockerCLI command.Cli, options CreateO
8373
return err
8474
}
8575

86-
fmt.Fprintln(dockerCLI.Out(), r.ID)
76+
_, _ = fmt.Fprintln(dockerCLI.Out(), r.ID)
8777
return nil
8878
}
79+
80+
// maxConfigSize is the maximum byte length of the [swarm.ConfigSpec.Data] field,
81+
// as defined by [MaxConfigSize] in SwarmKit.
82+
//
83+
// [MaxConfigSize]: https://pkg.go.dev/github.com/moby/swarmkit/[email protected]/manager/controlapi#MaxConfigSize
84+
const maxConfigSize = 1000 * 1024 // 1000KB
85+
86+
// readConfigData reads the config from either stdin or the given fileName.
87+
//
88+
// It reads up to twice the maximum size of the config ([maxConfigSize]),
89+
// just in case swarm's limit changes; this is only a safeguard to prevent
90+
// reading arbitrary files into memory.
91+
func readConfigData(in io.Reader, fileName string) ([]byte, error) {
92+
switch fileName {
93+
case "-":
94+
data, err := io.ReadAll(io.LimitReader(in, 2*maxConfigSize))
95+
if err != nil {
96+
return nil, fmt.Errorf("error reading from STDIN: %w", err)
97+
}
98+
if len(data) == 0 {
99+
return nil, errors.New("error reading from STDIN: data is empty")
100+
}
101+
return data, nil
102+
case "":
103+
return nil, errors.New("config file is required")
104+
default:
105+
// Open file with [FILE_FLAG_SEQUENTIAL_SCAN] on Windows, which
106+
// prevents Windows from aggressively caching it. We expect this
107+
// file to be only read once. Given that this is expected to be
108+
// a small file, this may not be a significant optimization, so
109+
// we could choose to omit this, and use a regular [os.Open].
110+
//
111+
// [FILE_FLAG_SEQUENTIAL_SCAN]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN
112+
f, err := sequential.Open(fileName)
113+
if err != nil {
114+
return nil, fmt.Errorf("error reading from %s: %w", fileName, err)
115+
}
116+
defer f.Close()
117+
data, err := io.ReadAll(io.LimitReader(f, 2*maxConfigSize))
118+
if err != nil {
119+
return nil, fmt.Errorf("error reading from %s: %w", fileName, err)
120+
}
121+
if len(data) == 0 {
122+
return nil, fmt.Errorf("error reading from %s: data is empty", fileName)
123+
}
124+
return data, nil
125+
}
126+
}

‎cli/command/config/create_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ func TestConfigCreateErrors(t *testing.T) {
5959
}
6060

6161
func TestConfigCreateWithName(t *testing.T) {
62-
name := "foo"
62+
const name = "config-with-name"
6363
var actual []byte
6464
cli := test.NewFakeCli(&fakeClient{
6565
configCreateFunc: func(_ context.Context, spec swarm.ConfigSpec) (types.ConfigCreateResponse, error) {
@@ -87,7 +87,7 @@ func TestConfigCreateWithLabels(t *testing.T) {
8787
"lbl1": "Label-foo",
8888
"lbl2": "Label-bar",
8989
}
90-
name := "foo"
90+
const name = "config-with-labels"
9191

9292
data, err := os.ReadFile(filepath.Join("testdata", configDataFile))
9393
assert.NilError(t, err)
@@ -124,7 +124,7 @@ func TestConfigCreateWithTemplatingDriver(t *testing.T) {
124124
expectedDriver := &swarm.Driver{
125125
Name: "template-driver",
126126
}
127-
name := "foo"
127+
const name = "config-with-template-driver"
128128

129129
cli := test.NewFakeCli(&fakeClient{
130130
configCreateFunc: func(_ context.Context, spec swarm.ConfigSpec) (types.ConfigCreateResponse, error) {

‎cli/command/secret/create.go‎

Lines changed: 55 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,19 @@ func newSecretCreateCommand(dockerCli command.Cli) *cobra.Command {
5252
func runSecretCreate(ctx context.Context, dockerCli command.Cli, options createOptions) error {
5353
client := dockerCli.Client()
5454

55-
if options.driver != "" && options.file != "" {
56-
return errors.Errorf("When using secret driver secret data must be empty")
55+
var secretData []byte
56+
if options.driver != "" {
57+
if options.file != "" {
58+
return errors.Errorf("When using secret driver secret data must be empty")
59+
}
60+
} else {
61+
var err error
62+
secretData, err = readSecretData(dockerCli.In(), options.file)
63+
if err != nil {
64+
return err
65+
}
5766
}
5867

59-
secretData, err := readSecretData(dockerCli.In(), options.file)
60-
if err != nil {
61-
return errors.Errorf("Error reading content from %q: %v", options.file, err)
62-
}
6368
spec := swarm.SecretSpec{
6469
Annotations: swarm.Annotations{
6570
Name: options.name,
@@ -82,26 +87,54 @@ func runSecretCreate(ctx context.Context, dockerCli command.Cli, options createO
8287
return err
8388
}
8489

85-
fmt.Fprintln(dockerCli.Out(), r.ID)
90+
_, _ = fmt.Fprintln(dockerCli.Out(), r.ID)
8691
return nil
8792
}
8893

89-
func readSecretData(in io.ReadCloser, file string) ([]byte, error) {
90-
// Read secret value from external driver
91-
if file == "" {
92-
return nil, nil
93-
}
94-
if file != "-" {
95-
var err error
96-
in, err = sequential.Open(file)
94+
// maxSecretSize is the maximum byte length of the [swarm.SecretSpec.Data] field,
95+
// as defined by [MaxSecretSize] in SwarmKit.
96+
//
97+
// [MaxSecretSize]: https://pkg.go.dev/github.com/moby/swarmkit/[email protected]/api/validation#MaxSecretSize
98+
const maxSecretSize = 500 * 1024 // 500KB
99+
100+
// readSecretData reads the secret from either stdin or the given fileName.
101+
//
102+
// It reads up to twice the maximum size of the secret ([maxSecretSize]),
103+
// just in case swarm's limit changes; this is only a safeguard to prevent
104+
// reading arbitrary files into memory.
105+
func readSecretData(in io.Reader, fileName string) ([]byte, error) {
106+
switch fileName {
107+
case "-":
108+
data, err := io.ReadAll(io.LimitReader(in, 2*maxSecretSize))
97109
if err != nil {
98-
return nil, err
110+
return nil, fmt.Errorf("error reading from STDIN: %w", err)
99111
}
100-
defer in.Close()
101-
}
102-
data, err := io.ReadAll(in)
103-
if err != nil {
104-
return nil, err
112+
if len(data) == 0 {
113+
return nil, errors.New("error reading from STDIN: data is empty")
114+
}
115+
return data, nil
116+
case "":
117+
return nil, errors.New("secret file is required")
118+
default:
119+
// Open file with [FILE_FLAG_SEQUENTIAL_SCAN] on Windows, which
120+
// prevents Windows from aggressively caching it. We expect this
121+
// file to be only read once. Given that this is expected to be
122+
// a small file, this may not be a significant optimization, so
123+
// we could choose to omit this, and use a regular [os.Open].
124+
//
125+
// [FILE_FLAG_SEQUENTIAL_SCAN]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN
126+
f, err := sequential.Open(fileName)
127+
if err != nil {
128+
return nil, fmt.Errorf("error reading from %s: %w", fileName, err)
129+
}
130+
defer f.Close()
131+
data, err := io.ReadAll(io.LimitReader(f, 2*maxSecretSize))
132+
if err != nil {
133+
return nil, fmt.Errorf("error reading from %s: %w", fileName, err)
134+
}
135+
if len(data) == 0 {
136+
return nil, fmt.Errorf("error reading from %s: data is empty", fileName)
137+
}
138+
return data, nil
105139
}
106-
return data, nil
107140
}

‎cli/command/secret/create_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ func TestSecretCreateErrors(t *testing.T) {
5656
}
5757

5858
func TestSecretCreateWithName(t *testing.T) {
59-
name := "foo"
59+
const name = "secret-with-name"
6060
data, err := os.ReadFile(filepath.Join("testdata", secretDataFile))
6161
assert.NilError(t, err)
6262

@@ -89,7 +89,7 @@ func TestSecretCreateWithDriver(t *testing.T) {
8989
expectedDriver := &swarm.Driver{
9090
Name: "secret-driver",
9191
}
92-
name := "foo"
92+
const name = "secret-with-driver"
9393

9494
cli := test.NewFakeCli(&fakeClient{
9595
secretCreateFunc: func(_ context.Context, spec swarm.SecretSpec) (types.SecretCreateResponse, error) {
@@ -118,7 +118,7 @@ func TestSecretCreateWithTemplatingDriver(t *testing.T) {
118118
expectedDriver := &swarm.Driver{
119119
Name: "template-driver",
120120
}
121-
const name = "foo"
121+
const name = "secret-with-template-driver"
122122

123123
cli := test.NewFakeCli(&fakeClient{
124124
secretCreateFunc: func(_ context.Context, spec swarm.SecretSpec) (types.SecretCreateResponse, error) {
@@ -137,7 +137,7 @@ func TestSecretCreateWithTemplatingDriver(t *testing.T) {
137137
})
138138

139139
cmd := newSecretCreateCommand(cli)
140-
cmd.SetArgs([]string{name})
140+
cmd.SetArgs([]string{name, filepath.Join("testdata", secretDataFile)})
141141
assert.Check(t, cmd.Flags().Set("template-driver", expectedDriver.Name))
142142
assert.NilError(t, cmd.Execute())
143143
assert.Check(t, is.Equal("ID-"+name, strings.TrimSpace(cli.OutBuffer().String())))
@@ -148,7 +148,7 @@ func TestSecretCreateWithLabels(t *testing.T) {
148148
"lbl1": "Label-foo",
149149
"lbl2": "Label-bar",
150150
}
151-
const name = "foo"
151+
const name = "secret-with-labels"
152152

153153
cli := test.NewFakeCli(&fakeClient{
154154
secretCreateFunc: func(_ context.Context, spec swarm.SecretSpec) (types.SecretCreateResponse, error) {

0 commit comments

Comments
 (0)