Skip to content

Commit

Permalink
Merge pull request kubernetes-csi#4 from pohly/older-csi
Browse files Browse the repository at this point in the history 
support also CSI <= 0.3.0
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Dec 6, 2018
2 parents 1628ab5 + 1e77671 commit 5853414
Show file tree
Hide file tree
Showing 6 changed files with 5,148 additions and 260 deletions.
13 changes: 4 additions & 9 deletions Gopkg.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

55 changes: 47 additions & 8 deletions protosanitizer/protosanitizer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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)
Expand All @@ -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.
Expand All @@ -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***"
Expand Down Expand Up @@ -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")
}
148 changes: 106 additions & 42 deletions protosanitizer/protosanitizer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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"`},
Expand Down Expand Up @@ -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.
Expand All @@ -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")
}
Expand Down
Loading

0 comments on commit 5853414

Please sign in to comment.