Merge pull request #4 from pohly/older-csi

support also CSI <= 0.3.0

Author: k8s-ci-robot

Gopkg.lock

@@ -24,10 +24,10 @@ import (
 	"reflect"
 	"strings"
 
-	"github.com/container-storage-interface/spec/lib/go/csi"
 	"github.com/golang/protobuf/descriptor"
 	"github.com/golang/protobuf/proto"
 	protobuf "github.com/golang/protobuf/protoc-gen-go/descriptor"
+	protobufdescriptor "github.com/golang/protobuf/protoc-gen-go/descriptor"
 )
 
 // StripSecrets returns a wrapper around the original CSI gRPC message
@@ -36,15 +36,27 @@ import (
 // Instead of the secret value(s), the string "***stripped***" is
 // included in the result.
 //
+// StripSecrets relies on an extension in CSI 1.0 and thus can only
+// be used for messages based on that or a more recent spec!
+//
 // StripSecrets itself is fast and therefore it is cheap to pass the
 // result to logging functions which may or may not end up serializing
 // the parameter depending on the current log level.
 func StripSecrets(msg interface{}) fmt.Stringer {
-	return &stripSecrets{msg}
+	return &stripSecrets{msg, isCSI1Secret}
+}
+
+// StripSecretsCSI03 is like StripSecrets, except that it works
+// for messages based on CSI 0.3 and older. It does not work
+// for CSI 1.0, use StripSecrets for that.
+func StripSecretsCSI03(msg interface{}) fmt.Stringer {
+	return &stripSecrets{msg, isCSI03Secret}
 }
 
 type stripSecrets struct {
 	msg interface{}
+
+	isSecretField func(field *protobuf.FieldDescriptorProto) bool
 }
 
 func (s *stripSecrets) String() string {
@@ -60,7 +72,7 @@ func (s *stripSecrets) String() string {
 	}
 
 	// Now remove secrets from the generic representation of the message.
-	strip(parsed, s.msg)
+	s.strip(parsed, s.msg)
 
 	// Re-encoded the stripped representation and return that.
 	b, err = json.Marshal(parsed)
@@ -70,7 +82,7 @@ func (s *stripSecrets) String() string {
 	return string(b)
 }
 
-func strip(parsed interface{}, msg interface{}) {
+func (s *stripSecrets) strip(parsed interface{}, msg interface{}) {
 	protobufMsg, ok := msg.(descriptor.Message)
 	if !ok {
 		// Not a protobuf message, so we are done.
@@ -93,8 +105,7 @@ func strip(parsed interface{}, msg interface{}) {
 	fields := md.GetField()
 	if fields != nil {
 		for _, field := range fields {
-			ex, err := proto.GetExtension(field.Options, csi.E_CsiSecret)
-			if err == nil && ex != nil && *ex.(*bool) {
+			if s.isSecretField(field) {
 				// Overwrite only if already set.
 				if _, ok := parsedFields[field.GetName()]; ok {
 					parsedFields[field.GetName()] = "***stripped***"
@@ -126,13 +137,41 @@ func strip(parsed interface{}, msg interface{}) {
 				if slice, ok := entry.([]interface{}); ok {
 					// Array of values, like VolumeCapabilities in CreateVolumeRequest.
 					for _, entry := range slice {
-						strip(entry, i)
+						s.strip(entry, i)
 					}
 				} else {
 					// Single value.
-					strip(entry, i)
+					s.strip(entry, i)
 				}
 			}
 		}
 	}
 }
+
+// isCSI1Secret uses the csi.E_CsiSecret extension from CSI 1.0 to
+// determine whether a field contains secrets.
+func isCSI1Secret(field *protobuf.FieldDescriptorProto) bool {
+	ex, err := proto.GetExtension(field.Options, e_CsiSecret)
+	return err == nil && ex != nil && *ex.(*bool)
+}
+
+// Copied from the CSI 1.0 spec (https://github.com/container-storage-interface/spec/blob/37e74064635d27c8e33537c863b37ccb1182d4f8/lib/go/csi/csi.pb.go#L4520-L4527)
+// to avoid a package dependency that would prevent usage of this package
+// in repos using an older version of the spec.
+//
+// Future revision of the CSI spec must not change this extensions, otherwise
+// they will break filtering in binaries based on the 1.0 version of the spec.
+var e_CsiSecret = &proto.ExtensionDesc{
+	ExtendedType:  (*protobufdescriptor.FieldOptions)(nil),
+	ExtensionType: (*bool)(nil),
+	Field:         1059,
+	Name:          "csi.v1.csi_secret",
+	Tag:           "varint,1059,opt,name=csi_secret,json=csiSecret",
+	Filename:      "github.com/container-storage-interface/spec/csi.proto",
+}
+
+// isCSI03Secret relies on the naming convention in CSI <= 0.3
+// to determine whether a field contains secrets.
+func isCSI03Secret(field *protobuf.FieldDescriptorProto) bool {
+	return strings.HasSuffix(field.GetName(), "_secrets")
+}

protosanitizer/protosanitizer.go

@@ -20,14 +20,54 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/container-storage-interface/spec/lib/go/csi"
+	"github.com/golang/protobuf/proto"
+	csi03 "github.com/kubernetes-csi/csi-lib-utils/protosanitizer/test/csi03"
+	csi "github.com/kubernetes-csi/csi-lib-utils/protosanitizer/test/csi10"
 	"github.com/kubernetes-csi/csi-lib-utils/protosanitizer/test/csitest"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestStripSecrets(t *testing.T) {
 	secretName := "secret-abc"
 	secretValue := "123"
+
+	// CSI 0.3.0.
+	createVolumeCSI03 := &csi03.CreateVolumeRequest{
+		AccessibilityRequirements: &csi03.TopologyRequirement{
+			Requisite: []*csi03.Topology{
+				&csi03.Topology{
+					Segments: map[string]string{
+						"foo": "bar",
+						"x":   "y",
+					},
+				},
+				&csi03.Topology{
+					Segments: map[string]string{
+						"a": "b",
+					},
+				},
+			},
+		},
+		Name: "foo",
+		VolumeCapabilities: []*csi03.VolumeCapability{
+			&csi03.VolumeCapability{
+				AccessType: &csi03.VolumeCapability_Mount{
+					Mount: &csi03.VolumeCapability_MountVolume{
+						FsType: "ext4",
+					},
+				},
+			},
+		},
+		CapacityRange: &csi03.CapacityRange{
+			RequiredBytes: 1024,
+		},
+		ControllerCreateSecrets: map[string]string{
+			secretName:   secretValue,
+			"secret-xyz": "987",
+		},
+	}
+
+	// Current spec.
 	createVolume := &csi.CreateVolumeRequest{
 		AccessibilityRequirements: &csi.TopologyRequirement{
 			Requisite: []*csi.Topology{
@@ -63,9 +103,50 @@ func TestStripSecrets(t *testing.T) {
 		},
 	}
 
-	cases := []struct {
+	// Revised spec with more secret fields.
+	createVolumeFuture := &csitest.CreateVolumeRequest{
+		CapacityRange: &csitest.CapacityRange{
+			RequiredBytes: 1024,
+		},
+		MaybeSecretMap: map[int64]*csitest.VolumeCapability{
+			1: &csitest.VolumeCapability{ArraySecret: "aaa"},
+			2: &csitest.VolumeCapability{ArraySecret: "bbb"},
+		},
+		Name:         "foo",
+		NewSecretInt: 42,
+		Seecreets: map[string]string{
+			secretName:   secretValue,
+			"secret-xyz": "987",
+		},
+		VolumeCapabilities: []*csitest.VolumeCapability{
+			&csitest.VolumeCapability{
+				AccessType: &csitest.VolumeCapability_Mount{
+					Mount: &csitest.VolumeCapability_MountVolume{
+						FsType: "ext4",
+					},
+				},
+				ArraySecret: "knock knock",
+			},
+			&csitest.VolumeCapability{
+				ArraySecret: "Who's there?",
+			},
+		},
+		VolumeContentSource: &csitest.VolumeContentSource{
+			Type: &csitest.VolumeContentSource_Volume{
+				Volume: &csitest.VolumeContentSource_VolumeSource{
+					VolumeId:         "abc",
+					OneofSecretField: "hello",
+				},
+			},
+			NestedSecretField: "world",
+		},
+	}
+
+	type testcase struct {
 		original, stripped interface{}
-	}{
+	}
+
+	cases := []testcase{
 		{nil, "null"},
 		{1, "1"},
 		{"hello world", `"hello world"`},
@@ -98,44 +179,9 @@ func TestStripSecrets(t *testing.T) {
 			AccessibilityRequirements: &csi.TopologyRequirement{},
 		}, `{"accessibility_requirements":{},"capacity_range":{"limit_bytes":1024,"required_bytes":1024},"name":"test-volume","parameters":{"param1":"param1","param2":"param2"},"secrets":"***stripped***","volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4","mount_flags":["flag1","flag2","flag3"]}},"access_mode":{"mode":5}}],"volume_content_source":{"Type":null}}`},
 		{createVolume, `{"accessibility_requirements":{"requisite":[{"segments":{"foo":"bar","x":"y"}},{"segments":{"a":"b"}}]},"capacity_range":{"required_bytes":1024},"name":"foo","secrets":"***stripped***","volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4"}}}]}`},
+		{createVolumeCSI03, `{"accessibility_requirements":{"requisite":[{"segments":{"foo":"bar","x":"y"}},{"segments":{"a":"b"}}]},"capacity_range":{"required_bytes":1024},"controller_create_secrets":"***stripped***","name":"foo","volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4"}}}]}`},
 		{&csitest.CreateVolumeRequest{}, `{}`},
-		{&csitest.CreateVolumeRequest{
-			CapacityRange: &csitest.CapacityRange{
-				RequiredBytes: 1024,
-			},
-			MaybeSecretMap: map[int64]*csitest.VolumeCapability{
-				1: &csitest.VolumeCapability{ArraySecret: "aaa"},
-				2: &csitest.VolumeCapability{ArraySecret: "bbb"},
-			},
-			Name:         "foo",
-			NewSecretInt: 42,
-			Seecreets: map[string]string{
-				secretName:   secretValue,
-				"secret-xyz": "987",
-			},
-			VolumeCapabilities: []*csitest.VolumeCapability{
-				&csitest.VolumeCapability{
-					AccessType: &csitest.VolumeCapability_Mount{
-						Mount: &csitest.VolumeCapability_MountVolume{
-							FsType: "ext4",
-						},
-					},
-					ArraySecret: "knock knock",
-				},
-				&csitest.VolumeCapability{
-					ArraySecret: "Who's there?",
-				},
-			},
-			VolumeContentSource: &csitest.VolumeContentSource{
-				Type: &csitest.VolumeContentSource_Volume{
-					Volume: &csitest.VolumeContentSource_VolumeSource{
-						VolumeId:         "abc",
-						OneofSecretField: "hello",
-					},
-				},
-				NestedSecretField: "world",
-			},
-		},
+		{createVolumeFuture,
 			// Secrets are *not* removed from all fields yet. This will have to be fixed one way or another
 			// before the CSI spec can start using secrets there (currently it doesn't).
 			// The test is still useful because it shows that also complicated fields get serialized.
@@ -144,11 +190,29 @@ func TestStripSecrets(t *testing.T) {
 		},
 	}
 
+	// Message from revised spec as received by a sidecar based on the current spec.
+	// The XXX_unrecognized field contains secrets and must not get logged.
+	unknownFields := &csi.CreateVolumeRequest{}
+	data, err := proto.Marshal(createVolumeFuture)
+	if assert.NoError(t, err, "marshall future message") &&
+		assert.NoError(t, proto.Unmarshal(data, unknownFields), "unmarshal with unknown fields") {
+		cases = append(cases, testcase{unknownFields,
+			`{"capacity_range":{"required_bytes":1024},"name":"foo","secrets":"***stripped***","volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4"}}},{"AccessType":null}],"volume_content_source":{"Type":{"Volume":{"volume_id":"abc"}}}}`,
+		})
+	}
+
 	for _, c := range cases {
 		before := fmt.Sprint(c.original)
-		stripped := StripSecrets(c.original)
+		var stripped fmt.Stringer
+		if _, ok := c.original.(*csi03.CreateVolumeRequest); ok {
+			stripped = StripSecretsCSI03(c.original)
+		} else {
+			stripped = StripSecrets(c.original)
+		}
 		if assert.Equal(t, c.stripped, fmt.Sprintf("%s", stripped), "unexpected result for fmt s of %s", c.original) {
-			assert.Equal(t, c.stripped, fmt.Sprintf("%v", stripped), "unexpected result for fmt v of %s", c.original)
+			if assert.Equal(t, c.stripped, fmt.Sprintf("%v", stripped), "unexpected result for fmt v of %s", c.original) {
+				assert.Equal(t, c.stripped, fmt.Sprintf("%+v", stripped), "unexpected result for fmt +v of %s", c.original)
+			}
 		}
 		assert.Equal(t, before, fmt.Sprint(c.original), "original value modified")
 	}