Skip to content

Commit

Permalink
vet: run staticcheck for all sub modules (#7155)
Browse files Browse the repository at this point in the history
  • Loading branch information
@arvindbr8
arvindbr8 authored Apr 23, 2024
1 parent d75b5e2 commit cb9c22d
Show file tree
Hide file tree
Showing 8 changed files with 60 additions and 54 deletions.
3 changes: 2 additions & 1 deletion channelz/internal/protoconv/sockopt_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ package protoconv
import (
"time"

channelzpb "google.golang.org/grpc/channelz/grpc_channelz_v1"
"google.golang.org/grpc/grpclog"
"google.golang.org/grpc/internal/channelz"

channelzpb "google.golang.org/grpc/channelz/grpc_channelz_v1"
"google.golang.org/protobuf/types/known/anypb"
"google.golang.org/protobuf/types/known/durationpb"
)
Expand Down
1 change: 1 addition & 0 deletions examples/features/customloadbalancer/client/customroundrobin/customroundrobin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*
*/

// Package customroundrobin provides an example for the custom roundrobin balancer.
package customroundrobin

import (
Expand Down
23 changes: 10 additions & 13 deletions gcp/observability/observability_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import (
"google.golang.org/grpc/internal/leakcheck"
"google.golang.org/grpc/internal/stubserver"
"google.golang.org/grpc/internal/testutils"
"google.golang.org/grpc/metadata"

testgrpc "google.golang.org/grpc/interop/grpc_testing"
testpb "google.golang.org/grpc/interop/grpc_testing"
Expand All @@ -64,21 +63,16 @@ func init() {
}

var (
defaultTestTimeout = 10 * time.Second
testHeaderMetadata = metadata.MD{"header": []string{"HeADer"}}
testTrailerMetadata = metadata.MD{"trailer": []string{"TrAileR"}}
testOkPayload = []byte{72, 101, 108, 108, 111, 32, 87, 111, 114, 108, 100}
testErrorPayload = []byte{77, 97, 114, 116, 104, 97}
testErrorMessage = "test case injected error"
infinitySizeBytes int32 = 1024 * 1024 * 1024
defaultRequestCount = 24
defaultTestTimeout = 10 * time.Second
testOkPayload = []byte{72, 101, 108, 108, 111, 32, 87, 111, 114, 108, 100}
defaultRequestCount = 24
)

const (
TypeOpenCensusViewDistribution string = "distribution"
TypeOpenCensusViewCount = "count"
TypeOpenCensusViewSum = "sum"
TypeOpenCensusViewLastValue = "last_value"
TypeOpenCensusViewDistribution = "distribution"
TypeOpenCensusViewCount = "count"
TypeOpenCensusViewSum = "sum"
TypeOpenCensusViewLastValue = "last_value"
)

type fakeOpenCensusExporter struct {
Expand Down Expand Up @@ -504,6 +498,9 @@ func (s) TestCustomTagsTracingMetrics(t *testing.T) {
}`

cleanup, err := createTmpConfigInFileSystem(configJSON)
if err != nil {
t.Fatalf("failed to create config in file system: %v", err)
}
defer cleanup()

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
Expand Down
71 changes: 40 additions & 31 deletions scripts/vet.sh
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
#!/bin/bash

set -ex # Exit on error; debugging enabled.
set -o pipefail # Fail a pipe if any sub-command fails.
set -ex # Exit on error; debugging enabled.
set -o pipefail # Fail a pipe if any sub-command fails.

source "$(dirname $0)/vet-common.sh"

# Check to make sure it's safe to modify the user's git repo.
git status --porcelain | fail_on_output

# noret_grep will return 0 if zero or more lines were selected, and >1 if an
# error occurred. Suppresses grep's return code of 1 when there are no matches
# (for eg, empty file).
noret_grep() {
grep "$@" || [[ $? == 1 ]]
}

# Undo any edits made by this script.
cleanup() {
git reset --hard HEAD
Expand Down Expand Up @@ -40,34 +47,31 @@ not grep 'func Test[^(]' *_test.go
not grep 'func Test[^(]' test/*.go

# - Check for typos in test function names
git grep 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test'
git grep 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example'

# - Do not import x/net/context.
not git grep -l 'x/net/context' -- "*.go"
git grep --quiet 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test'
git grep --quiet 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example'

# - Do not use time.After except in tests. It has the potential to leak the
# timer since there is no way to stop it early.
git grep -l 'time.After(' -- "*.go" | not grep -v '_test.go\|test_utils\|testutils'
git grep --quiet -l 'time.After(' -- "*.go" | not grep -v '_test.go\|test_utils\|testutils'

# - Do not import math/rand for real library code. Use internal/grpcrand for
# thread safety.
git grep -l '"math/rand"' -- "*.go" 2>&1 | not grep -v '^examples\|^interop/stress\|grpcrand\|^benchmark\|wrr_test'
git grep --quiet -l '"math/rand"' -- "*.go" 2>&1 | not grep -v '^examples\|^interop/stress\|grpcrand\|^benchmark\|wrr_test'

# - Do not use "interface{}"; use "any" instead.
git grep -l 'interface{}' -- "*.go" 2>&1 | not grep -v '\.pb\.go\|protoc-gen-go-grpc\|grpc_testing_not_regenerate'
git grep --quiet -l 'interface{}' -- "*.go" 2>&1 | not grep -v '\.pb\.go\|protoc-gen-go-grpc\|grpc_testing_not_regenerate'

# - Do not call grpclog directly. Use grpclog.Component instead.
git grep -l -e 'grpclog.I' --or -e 'grpclog.W' --or -e 'grpclog.E' --or -e 'grpclog.F' --or -e 'grpclog.V' -- "*.go" | not grep -v '^grpclog/component.go\|^internal/grpctest/tlogger_test.go'
git grep --quiet -l -e 'grpclog.I' --or -e 'grpclog.W' --or -e 'grpclog.E' --or -e 'grpclog.F' --or -e 'grpclog.V' -- "*.go" | not grep -v '^grpclog/component.go\|^internal/grpctest/tlogger_test.go'

# - Ensure that the deprecated protobuf dependency is not used.
not git grep "\"github.com/golang/protobuf/*" -- "*.go" ':(exclude)reflection/test/grpc_testing_not_regenerate/*'
not git grep --quiet "\"github.com/golang/protobuf/*" -- "*.go" ':(exclude)reflection/test/grpc_testing_not_regenerate/*'

# - Ensure all usages of grpc_testing package are renamed when importing.
not git grep "\(import \|^\s*\)\"google.golang.org/grpc/interop/grpc_testing" -- "*.go"
not git grep --quiet "\(import \|^\s*\)\"google.golang.org/grpc/interop/grpc_testing" -- "*.go"

# - Ensure all xds proto imports are renamed to *pb or *grpc.
git grep '"github.com/envoyproxy/go-control-plane/envoy' -- '*.go' ':(exclude)*.pb.go' | not grep -v 'pb "\|grpc "'
git grep --quiet '"github.com/envoyproxy/go-control-plane/envoy' -- '*.go' ':(exclude)*.pb.go' | not grep -v 'pb "\|grpc "'

misspell -error .

Expand All @@ -83,21 +87,19 @@ for MOD_FILE in $(find . -name 'go.mod'); do
go mod tidy -compat=1.19
git status --porcelain 2>&1 | fail_on_output || \
(git status; git --no-pager diff; exit 1)
popd
done

# - Collection of static analysis checks
SC_OUT="$(mktemp)"
staticcheck -go 1.19 -checks 'all' ./... >"${SC_OUT}" || true

# - Collection of static analysis checks
SC_OUT="$(mktemp)"
staticcheck -go 1.19 -checks 'all' ./... > "${SC_OUT}" || true
# Error for anything other than checks that need exclusions.
noret_grep -v "(ST1000)" "${SC_OUT}" | noret_grep -v "(SA1019)" | noret_grep -v "(ST1003)" | noret_grep -v "(ST1019)\|\(other import of\)" | not grep -v "(SA4000)"

# Error for anything other than checks that need exclusions.
grep -v "(ST1000)" "${SC_OUT}" | grep -v "(SA1019)" | grep -v "(ST1003)" | not grep -v "(ST1019)\|\(other import of\)"
# Exclude underscore checks for generated code.
noret_grep "(ST1003)" "${SC_OUT}" | not grep -v '\(.pb.go:\)\|\(code_string_test.go:\)\|\(grpc_testing_not_regenerate\)'

# Exclude underscore checks for generated code.
grep "(ST1003)" "${SC_OUT}" | not grep -v '\(.pb.go:\)\|\(code_string_test.go:\)\|\(grpc_testing_not_regenerate\)'

# Error for duplicate imports not including grpc protos.
grep "(ST1019)\|\(other import of\)" "${SC_OUT}" | not grep -Fv 'XXXXX PleaseIgnoreUnused
# Error for duplicate imports not including grpc protos.
noret_grep "(ST1019)\|\(other import of\)" "${SC_OUT}" | not grep -Fv 'XXXXX PleaseIgnoreUnused
channelz/grpc_channelz_v1"
go-control-plane/envoy
grpclb/grpc_lb_v1"
Expand All @@ -106,16 +108,21 @@ interop/grpc_testing"
orca/v3"
proto/grpc_gcp"
proto/grpc_lookup_v1"
examples/features/proto/echo"
reflection/grpc_reflection_v1"
reflection/grpc_reflection_v1alpha"
XXXXX PleaseIgnoreUnused'

# Error for any package comments not in generated code.
grep "(ST1000)" "${SC_OUT}" | not grep -v "\.pb\.go:"
# Error for any package comments not in generated code.
noret_grep "(ST1000)" "${SC_OUT}" | not grep -v "\.pb\.go:"

# Ignore a false positive when operands have side affectes.
# TODO(https://github.com/dominikh/go-tools/issues/54): Remove this once the issue is fixed in staticcheck.
noret_grep "(SA4000)" "${SC_OUT}" | not grep -ev "crl.go:\d*:\d*: identical expressions on the left and right side of the '||' operator (SA4000)"

# Only ignore the following deprecated types/fields/functions and exclude
# generated code.
grep "(SA1019)" "${SC_OUT}" | not grep -Fv 'XXXXX PleaseIgnoreUnused
# Only ignore the following deprecated types/fields/functions and exclude
# generated code.
noret_grep "(SA1019)" "${SC_OUT}" | not grep -Fv 'XXXXX PleaseIgnoreUnused
XXXXX Protobuf related deprecation errors:
"github.com/golang/protobuf
.pb.go:
Expand Down Expand Up @@ -154,5 +161,7 @@ GetSuffixMatch
GetTlsCertificateCertificateProviderInstance
GetValidationContextCertificateProviderInstance
XXXXX PleaseIgnoreUnused'
popd
done

echo SUCCESS
3 changes: 1 addition & 2 deletions security/advancedtls/crl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,6 @@ qsSIp8gfxSyzkJP+Ngkm2DdLjlJQCZ9R0MZP9Xj4
t.Fatalf("parseRevocationList(dummyCrlFile) failed: %v", err)
}
crlExt := &CRL{certList: crl}
var crlIssuer pkix.Name = crl.Issuer

var revocationTests = []struct {
desc string
Expand Down Expand Up @@ -290,7 +289,7 @@ qsSIp8gfxSyzkJP+Ngkm2DdLjlJQCZ9R0MZP9Xj4
{
desc: "Single unrevoked Issuer",
in: x509.Certificate{
Issuer: crlIssuer,
Issuer: crl.Issuer,
SerialNumber: big.NewInt(2),
CRLDistributionPoints: []string{"test"},
},
Expand Down
5 changes: 2 additions & 3 deletions security/authorization/engine/engine.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* limitations under the License.
*/

// Package engine provides a CEL-based authorization engine for gRPC.
package engine

import (
Expand All @@ -28,7 +29,6 @@ import (
"github.com/google/cel-go/interpreter"
expr "google.golang.org/genproto/googleapis/api/expr/v1alpha1"
"google.golang.org/grpc/grpclog"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/peer"
"google.golang.org/protobuf/proto"
)
Expand Down Expand Up @@ -71,7 +71,6 @@ func (activation activationImpl) Parent() interpreter.Activation {

// AuthorizationArgs is the input of the CEL-based authorization engine.
type AuthorizationArgs struct {
md metadata.MD
peerInfo *peer.Peer
fullMethod string
}
Expand Down Expand Up @@ -211,7 +210,7 @@ func exprToProgram(condition *expr.Expr, env *cel.Env) (cel.Program, error) {
return nil, iss.Err()
}
// Check that the expression will evaluate to a boolean.
if !proto.Equal(ast.ResultType(), decls.Bool) {
if ot, _ := cel.TypeToExprType(ast.OutputType()); !proto.Equal(ot, decls.Bool) {
return nil, fmt.Errorf("expected boolean condition")
}
// Build the program plan.
Expand Down
2 changes: 1 addition & 1 deletion security/authorization/engine/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func compileCel(env *cel.Env, expr string) (*cel.Ast, error) {
return nil, iss.Err()
}
// Check the result type is a Boolean.
if !proto.Equal(checked.ResultType(), decls.Bool) {
if ot, _ := cel.TypeToExprType(checked.OutputType()); !proto.Equal(ot, decls.Bool) {
return nil, errors.New("failed to compile CEL string: get non-bool value")
}
return checked, nil
Expand Down
6 changes: 3 additions & 3 deletions security/authorization/engine/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ import (

func (s) TestStringConvert(t *testing.T) {
declarations := []*expr.Decl{
decls.NewIdent("request.url_path", decls.String, nil),
decls.NewIdent("request.host", decls.String, nil),
decls.NewIdent("connection.uri_san_peer_certificate", decls.String, nil),
decls.NewConst("request.url_path", decls.String, nil),
decls.NewConst("request.host", decls.String, nil),
decls.NewConst("connection.uri_san_peer_certificate", decls.String, nil),
}
env, err := cel.NewEnv()
if err != nil {
Expand Down

0 comments on commit cb9c22d

Please sign in to comment.