Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear the secrets from request for klog print in logGRPC() #1462

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions pkg/gce-pd-csi-driver/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"errors"
"fmt"
"reflect"

csi "github.com/container-storage-interface/spec/lib/go/csi"
"google.golang.org/grpc"
Expand Down Expand Up @@ -59,14 +60,30 @@ func NewNodeServiceCapability(cap csi.NodeServiceCapability_RPC_Type) *csi.NodeS
}
}

// Reflect magic below simply clears Secrets map from request.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to document all the assumptions about how secret fields are named in the csi spec.

func clearSecrets(req interface{}) interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsafrane how do you feel about putting this into csi-lib-utils as an alternative to protosanitizer so that it is not specific to a csi driver?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, but with a huge warning that only field explicitly named Secrets is cleared.

v := reflect.ValueOf(&req).Elem()
e := reflect.New(v.Elem().Type()).Elem()
e.Set(v.Elem())
f := reflect.Indirect(e).FieldByName("Secrets")
if f.IsValid() && f.CanSet() && f.Kind() == reflect.Map {
f.Set(reflect.MakeMap(f.Type()))
v.Set(e)
}
mpatlasov marked this conversation as resolved.
Show resolved Hide resolved
return req
}

func logGRPC(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
if info.FullMethod == ProbeCSIFullMethod {
return handler(ctx, req)
}
// Note that secrets are not included in any RPC message. In the past protosanitizer and other log
// Note that secrets may be included in some RPC messages
// (https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/issues/1372),
// but the driver ignores them. In the past protosanitizer and other log
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you able to do a benchmark test to show that the reflect method does not signficantly increase CPU usage?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @msau42 , I added a test to benchmark logGRPC. For 1M iterations, the reflect method increases execution time by only 1%:

# Before applying patch with `clearSecrets(req)`
$ go test -c -o /tmp/test ./pkg/gce-pd-csi-driver/ && time /tmp/test -test.run=TestLogGRPC -test.count=1000000
PASS
real	0m20.632s

# After applying patch with `clearSecrets(req)`
$ go test -c -o /tmp/test ./pkg/gce-pd-csi-driver/ && time /tmp/test -test.run=TestLogGRPC -test.count=1000000
PASS
real	0m20.750s

// stripping was shown to cause a significant increase of CPU usage (see
// https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/issues/356#issuecomment-550529004).
klog.V(4).Infof("%s called with request: %s", info.FullMethod, req)
// That is why we use hand-crafted clearSecrets() below rather than protosanitizer.
klog.V(4).Infof("%s called with request: %s", info.FullMethod, clearSecrets(req))
resp, err := handler(ctx, req)
if err != nil {
klog.Errorf("%s returned with error: %v", info.FullMethod, err.Error())
Expand Down
53 changes: 53 additions & 0 deletions pkg/gce-pd-csi-driver/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,14 @@ limitations under the License.
package gceGCEDriver

import (
"context"
"fmt"
"reflect"
"testing"

csi "github.com/container-storage-interface/spec/lib/go/csi"
"github.com/google/go-cmp/cmp"
"google.golang.org/grpc"
"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common"
)

Expand Down Expand Up @@ -723,3 +726,53 @@ func TestValidateStoragePoolZones(t *testing.T) {
}
}
}

func TestClearSecrets(t *testing.T) {
vc := &csi.VolumeCapability{
AccessType: &csi.VolumeCapability_Mount{
Mount: &csi.VolumeCapability_MountVolume{},
},
AccessMode: &csi.VolumeCapability_AccessMode{
Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
},
}

req := &csi.NodeExpandVolumeRequest{
VolumePath: "/path",
VolumeCapability: vc,
Secrets: map[string]string{
"key": "value",
},
}

clearedReq := &csi.NodeExpandVolumeRequest{
VolumePath: "/path",
VolumeCapability: vc,
Secrets: map[string]string{},
}

if !reflect.DeepEqual(clearSecrets(req), clearedReq) {
t.Fatalf("Unexpected change: %v vs. %v", clearSecrets(req), clearedReq)
}
}

func TestLogGRPC(t *testing.T) {
info := &grpc.UnaryServerInfo{
FullMethod: "foo",
}

req := struct {
Secrets map[string]string
}{map[string]string{
"password": "pass",
}}

handler := func(ctx context.Context, req any) (any, error) {
return nil, nil
}

_, err := logGRPC(nil, req, info, handler)
if err != nil {
t.Fatalf("logGRPC returns error %v", err)
}
}