diff --git a/pkg/server/diagnostics/BUILD.bazel b/pkg/server/diagnostics/BUILD.bazel index ee85b48cf52b..46bbcf999ead 100644 --- a/pkg/server/diagnostics/BUILD.bazel +++ b/pkg/server/diagnostics/BUILD.bazel @@ -55,12 +55,13 @@ go_test( size = "medium", srcs = [ "main_test.go", + "reporter_test.go", "update_checker_test.go", ], args = ["-test.timeout=295s"], - tags = ["no-remote"], + embed = [":diagnostics"], + tags = ["no-remote-exec"], deps = [ - ":diagnostics", "//pkg/base", "//pkg/build", "//pkg/security/securityassets", @@ -71,6 +72,7 @@ go_test( "//pkg/util/cloudinfo", "//pkg/util/leaktest", "//pkg/util/log", + "@com_github_mitchellh_reflectwalk//:reflectwalk", "@com_github_stretchr_testify//require", ], ) diff --git a/pkg/server/diagnostics/reporter.go b/pkg/server/diagnostics/reporter.go index 62b68124f18c..607a8b5c8106 100644 --- a/pkg/server/diagnostics/reporter.go +++ b/pkg/server/diagnostics/reporter.go @@ -320,6 +320,10 @@ func (r *Reporter) populateSQLInfo(uptime int64, sql *diagnosticspb.SQLInstanceI sql.Uptime = uptime } +// collectSchemaInfo is the "old" way of collecting schema information, and it +// redacted all `*string` type fields in the table descriptors but not `string` +// type fields. Check out `schematelemetry` package for a better data source for +// collecting redacted schema information. func (r *Reporter) collectSchemaInfo(ctx context.Context) ([]descpb.TableDescriptor, error) { startKey := keys.MakeSQLCodec(r.TenantID).TablePrefix(keys.DescriptorTableID) endKey := startKey.PrefixEnd() @@ -442,7 +446,7 @@ func anonymizeZoneConfig(dst *zonepb.ZoneConfig, src zonepb.ZoneConfig, secret s type stringRedactor struct{} func (stringRedactor) Primitive(v reflect.Value) error { - if v.Kind() == reflect.String && v.String() != "" { + if v.Kind() == reflect.String && v.String() != "" && v.CanSet() { v.Set(reflect.ValueOf("_").Convert(v.Type())) } return nil diff --git a/pkg/server/diagnostics/reporter_test.go b/pkg/server/diagnostics/reporter_test.go new file mode 100644 index 000000000000..1540ae1da35d --- /dev/null +++ b/pkg/server/diagnostics/reporter_test.go @@ -0,0 +1,49 @@ +// Copyright 2021 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package diagnostics + +import ( + "testing" + + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/mitchellh/reflectwalk" + "github.com/stretchr/testify/require" +) + +// TestStringRedactor_Primitive tests that fields of type `*string` will be +// correctly redacted to "_", and all other field types ( including `string`) +// will be unchanged. +func TestStringRedactor_Primitive(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + type Foo struct { + A string + B *string + C map[string]string + } + + string1 := "string 1" + string2 := "string 2" + foo := Foo{ + A: string1, + B: &string2, + C: map[string]string{"3": "string 3"}, + } + + require.NoError(t, reflectwalk.Walk(foo, stringRedactor{})) + require.Equal(t, "string 1", string1) + require.Equal(t, "string 1", foo.A) + require.Equal(t, "_", string2) + require.Equal(t, "_", *foo.B) + require.Equal(t, "string 3", foo.C["3"]) +}