Merge pull request moby#51381 from thaJeztah/simplify_pkg_security

client/pkg/security: simplify

Author: thaJeztah

client/pkg/security/security_opts.go

@@ -1,8 +1,6 @@
 package security
 
 import (
-	"errors"
-	"fmt"
 	"strings"
 )
 
@@ -19,30 +17,24 @@ type KeyValue struct {
 
 // DecodeOptions decodes a security options string slice to a
 // type-safe [Option].
-func DecodeOptions(opts []string) ([]Option, error) {
-	so := []Option{}
+func DecodeOptions(opts []string) []Option {
+	so := make([]Option, 0, len(opts))
 	for _, opt := range opts {
-		// support output from a < 1.13 docker daemon
-		if !strings.Contains(opt, "=") {
-			so = append(so, Option{Name: opt})
-			continue
-		}
 		secopt := Option{}
 		for _, s := range strings.Split(opt, ",") {
-			k, v, ok := strings.Cut(s, "=")
-			if !ok {
-				return nil, fmt.Errorf("invalid security option %q", s)
-			}
-			if k == "" || v == "" {
-				return nil, errors.New("invalid empty security option")
+			k, v, _ := strings.Cut(s, "=")
+			if k == "" {
+				continue
 			}
 			if k == "name" {
 				secopt.Name = v
 				continue
 			}
 			secopt.Options = append(secopt.Options, KeyValue{Key: k, Value: v})
 		}
-		so = append(so, secopt)
+		if secopt.Name != "" {
+			so = append(so, secopt)
+		}
 	}
-	return so, nil
+	return so
 }

client/pkg/security/security_opts_test.go

@@ -10,10 +10,9 @@ import (
 
 func TestDecode(t *testing.T) {
 	tests := []struct {
-		name    string
-		opts    []string
-		want    []Option
-		wantErr string
+		name string
+		opts []string
+		want []Option
 	}{
 		{
 			name: "empty options",
@@ -25,13 +24,6 @@ func TestDecode(t *testing.T) {
 			opts: nil,
 			want: []Option{},
 		},
-		{
-			name: "legacy format without equals",
-			opts: []string{"apparmor:unconfined"},
-			want: []Option{
-				{Name: "apparmor:unconfined"},
-			},
-		},
 		{
 			name: "single option with name only",
 			opts: []string{"name=apparmor"},
@@ -74,74 +66,64 @@ func TestDecode(t *testing.T) {
 			},
 		},
 		{
-			name: "mixed legacy and new format",
-			opts: []string{
-				"label:disable",
-				"name=apparmor,profile=custom",
-			},
-			want: []Option{
-				{Name: "label:disable"},
-				{
-					Name: "apparmor",
-					Options: []KeyValue{
-						{Key: "profile", Value: "custom"},
-					},
-				},
-			},
-		},
-		{
-			name: "option without name key",
-			opts: []string{"profile=custom,type=container_t"},
+			name: "option with equals in value",
+			opts: []string{"name=selinux,level=s0:c1=c2"},
 			want: []Option{
 				{
+					Name: "selinux",
 					Options: []KeyValue{
-						{Key: "profile", Value: "custom"},
-						{Key: "type", Value: "container_t"},
+						{Key: "level", Value: "s0:c1=c2"},
 					},
 				},
 			},
 		},
 		{
-			name: "option with equals in value",
-			opts: []string{"name=selinux,level=s0:c1=c2"},
+			name: "invalid option without equals in comma-separated list",
+			opts: []string{"name=apparmor,invalid"},
 			want: []Option{
 				{
-					Name: "selinux",
+					Name: "apparmor",
 					Options: []KeyValue{
-						{Key: "level", Value: "s0:c1=c2"},
+						{Key: "invalid"},
 					},
 				},
 			},
 		},
 		{
-			name:    "invalid option without equals in comma-separated list",
-			opts:    []string{"name=apparmor,invalid"},
-			wantErr: `invalid security option "invalid"`,
-		},
-		{
-			name:    "empty key",
-			opts:    []string{"=value"},
-			wantErr: "invalid empty security option",
+			name: "empty key",
+			opts: []string{"=value"},
+			want: []Option{},
 		},
 		{
-			name:    "empty value",
-			opts:    []string{"key="},
-			wantErr: "invalid empty security option",
+			name: "empty value",
+			opts: []string{"key="},
+			want: []Option{},
 		},
 		{
-			name:    "empty key and value",
-			opts:    []string{"="},
-			wantErr: "invalid empty security option",
+			name: "empty key and value",
+			opts: []string{"="},
+			want: []Option{},
 		},
 		{
-			name:    "empty key in middle",
-			opts:    []string{"name=apparmor,=value"},
-			wantErr: "invalid empty security option",
+			name: "empty key in middle",
+			opts: []string{"name=apparmor,=value"},
+			want: []Option{
+				{
+					Name: "apparmor",
+				},
+			},
 		},
 		{
-			name:    "empty value in middle",
-			opts:    []string{"name=apparmor,key="},
-			wantErr: "invalid empty security option",
+			name: "empty value in middle",
+			opts: []string{"name=apparmor,key="},
+			want: []Option{
+				{
+					Name: "apparmor",
+					Options: []KeyValue{
+						{Key: "key", Value: ""},
+					},
+				},
+			},
 		},
 		{
 			name: "complex real-world example",
@@ -178,15 +160,8 @@ func TestDecode(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			got, err := DecodeOptions(tc.opts)
-
-			if tc.wantErr == "" {
-				assert.NilError(t, err)
-				assert.Check(t, cmp.DeepEqual(got, tc.want))
-			} else {
-				assert.Check(t, err != nil, "expected error but got none")
-				assert.ErrorContains(t, err, tc.wantErr)
-			}
+			got := DecodeOptions(tc.opts)
+			assert.Check(t, cmp.DeepEqual(got, tc.want))
 		})
 	}
 }
@@ -196,31 +171,11 @@ func BenchmarkDecode(b *testing.B) {
 		"name=selinux,user=system_u,role=system_r,type=container_t,level=s0:c1.c2",
 		"name=apparmor,profile=/usr/bin/docker",
 		"name=seccomp,profile=builtin",
-		"legacy:format",
-	}
-
-	b.ResetTimer()
-	for i := 0; i < b.N; i++ {
-		_, err := DecodeOptions(opts)
-		if err != nil {
-			b.Fatal(err)
-		}
-	}
-}
-
-func BenchmarkDecodeLegacy(b *testing.B) {
-	opts := []string{
-		"apparmor:unconfined",
-		"label:disable",
-		"seccomp:unconfined",
 	}
 
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		_, err := DecodeOptions(opts)
-		if err != nil {
-			b.Fatal(err)
-		}
+		_ = DecodeOptions(opts)
 	}
 }
 
@@ -232,9 +187,6 @@ func BenchmarkDecodeComplex(b *testing.B) {
 
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		_, err := DecodeOptions(opts)
-		if err != nil {
-			b.Fatal(err)
-		}
+		_ = DecodeOptions(opts)
 	}
 }