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

vet: add check for trailing spaces #7576

Merged
merged 18 commits into from
Sep 12, 2024
Merged

Conversation

purnesh42H
Copy link
Contributor

@purnesh42H purnesh42H commented Aug 30, 2024

RELEASE NOTES: None

@purnesh42H purnesh42H changed the title vet: add trailing spaces check vet: add check for trailing spaces Aug 30, 2024
@purnesh42H
Copy link
Contributor Author

purnesh42H commented Aug 30, 2024

Will fix the existing files before getting this in

./xds/internal/test/e2e/run.sh:6:go test . 
./authz/grpc_authz_server_interceptors_test.go:60:			authzPolicy: `{		
./authz/grpc_authz_server_interceptors_test.go:62:				"allow_rules": 
./authz/grpc_authz_end2end_test.go:81:				"allow_rules": 
./authz/grpc_authz_end2end_test.go:169:							"headers": 
./authz/grpc_authz_end2end_test.go:173:									"values": 
./authz/grpc_authz_end2end_test.go:253:						"request": 
./security/advancedtls/testdata/server_cert_2.pem:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/server_cert_2.pem:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/server_cert_2.pem:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/server_cert_2.pem:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/client_cert_2.pem:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/client_cert_2.pem:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/client_cert_2.pem:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/client_cert_2.pem:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_3.txt:36:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/server_cert_3.txt:39:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/server_cert_3.txt:41:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_3.txt:43:            X509v3 Subject Alternative Name: 
./security/advancedtls/testdata/server_cert_1.txt:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/server_cert_1.txt:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/server_cert_1.txt:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/server_cert_1.txt:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_1.pem:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/server_cert_1.pem:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/server_cert_1.pem:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/server_cert_1.pem:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_2.txt:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/server_cert_2.txt:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/server_cert_2.txt:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/server_cert_2.txt:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_2.txt:91:         
./security/advancedtls/testdata/another_client_cert_1.pem:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/another_client_cert_1.pem:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/another_client_cert_1.pem:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/another_client_cert_1.pem:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_localhost_1.pem:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/server_cert_localhost_1.pem:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/server_cert_localhost_1.pem:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/server_cert_localhost_1.pem:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_localhost_1.pem:61:            X509v3 Subject Alternative Name: 
./security/advancedtls/testdata/client_cert_1.pem:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/client_cert_1.pem:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/client_cert_1.pem:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/client_cert_1.pem:59:            X509v3 Key Usage: 
./examples/features/advancedtls/creds/server_cert_revoked.pem:52:            X509v3 Subject Key Identifier: 
./examples/features/advancedtls/creds/server_cert_revoked.pem:54:            X509v3 Authority Key Identifier: 
./examples/features/advancedtls/creds/server_cert_revoked.pem:56:            X509v3 Basic Constraints: 
./examples/features/advancedtls/creds/server_cert_revoked.pem:58:            X509v3 Key Usage: 
./examples/features/advancedtls/creds/server_cert_revoked.pem:60:            X509v3 Subject Alternative Name: 
./examples/features/advancedtls/creds/client_cert.pem:52:            X509v3 Subject Key Identifier: 
./examples/features/advancedtls/creds/client_cert.pem:54:            X509v3 Authority Key Identifier: 
./examples/features/advancedtls/creds/client_cert.pem:56:            X509v3 Basic Constraints: 
./examples/features/advancedtls/creds/client_cert.pem:58:            X509v3 Key Usage: 
./examples/features/advancedtls/creds/client_cert.pem:60:            X509v3 Subject Alternative Name: 
./examples/features/advancedtls/creds/server_cert.pem:52:            X509v3 Subject Key Identifier: 
./examples/features/advancedtls/creds/server_cert.pem:54:            X509v3 Authority Key Identifier: 
./examples/features/advancedtls/creds/server_cert.pem:56:            X509v3 Basic Constraints: 
./examples/features/advancedtls/creds/server_cert.pem:58:            X509v3 Key Usage: 
./examples/features/advancedtls/creds/server_cert.pem:60:            X509v3 Subject Alternative Name: 
./examples/features/advancedtls/creds/client_cert_revoked.pem:52:            X509v3 Subject Key Identifier: 
./examples/features/advancedtls/creds/client_cert_revoked.pem:54:            X509v3 Authority Key Identifier: 
./examples/features/advancedtls/creds/client_cert_revoked.pem:56:            X509v3 Basic Constraints: 
./examples/features/advancedtls/creds/client_cert_revoked.pem:58:            X509v3 Key Usage: 
./examples/features/advancedtls/creds/client_cert_revoked.pem:60:            X509v3 Subject Alternative Name: 

@purnesh42H purnesh42H assigned dfawley and unassigned dfawley Aug 30, 2024
@purnesh42H purnesh42H added the Type: Meta Github repo, process, etc label Aug 30, 2024
@purnesh42H purnesh42H added this to the 1.67 Release milestone Aug 30, 2024
@purnesh42H purnesh42H self-assigned this Aug 30, 2024
Copy link

codecov bot commented Aug 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.91%. Comparing base (7fb7ac7) to head (72dd06c).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7576      +/-   ##
==========================================
+ Coverage   81.88%   81.91%   +0.03%     
==========================================
  Files         361      361              
  Lines       27813    27813              
==========================================
+ Hits        22775    22784       +9     
+ Misses       3845     3841       -4     
+ Partials     1193     1188       -5     

see 16 files with indirect coverage changes

@dfawley
Copy link
Member

dfawley commented Aug 30, 2024

vet: add check for trailing spaces

Release notes are for users. Only gRPC-Go developers will care about vet. None is appropriate here.

@dfawley dfawley added Type: Testing and removed Type: Meta Github repo, process, etc labels Aug 30, 2024
scripts/vet.sh Outdated
@@ -184,4 +184,11 @@ revive -formatter plain ./... >"${REV_OUT}" || true
# TODO: Remove `|| true` to unskip linter failures once existing issues are fixed.
(noret_grep -v "unused-parameter" "${REV_OUT}" | not grep -v "\.pb\.go:") || true

# Collection of trailing spaces analysis checks
TRAIL_SPACE_OUT="$(mktemp)"
Copy link
Member

Choose a reason for hiding this comment

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

Let's use a pipe instead of creating a file whenever possible, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

scripts/vet.sh Outdated
find . -type f -exec grep -Hn "[[:blank:]]$" {} \; >"${TRAIL_SPACE_OUT}"

# Error for anything other than in .git directory, vendor directory and *.md files.
noret_grep -v "./.git" "${TRAIL_SPACE_OUT}" | noret_grep -v "vendor" | not grep -v ".md"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid scanning these files instead of excluding them later? find has a -prune flag that may be useful.

Copy link
Member

Choose a reason for hiding this comment

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

I dont think we have a vendor directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@purnesh42H
Copy link
Contributor Author

Will fix the existing files before getting this in

./xds/internal/test/e2e/run.sh:6:go test . 
./authz/grpc_authz_server_interceptors_test.go:60:			authzPolicy: `{		
./authz/grpc_authz_server_interceptors_test.go:62:				"allow_rules": 
./authz/grpc_authz_end2end_test.go:81:				"allow_rules": 
./authz/grpc_authz_end2end_test.go:169:							"headers": 
./authz/grpc_authz_end2end_test.go:173:									"values": 
./authz/grpc_authz_end2end_test.go:253:						"request": 
./security/advancedtls/testdata/server_cert_2.pem:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/server_cert_2.pem:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/server_cert_2.pem:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/server_cert_2.pem:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/client_cert_2.pem:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/client_cert_2.pem:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/client_cert_2.pem:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/client_cert_2.pem:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_3.txt:36:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/server_cert_3.txt:39:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/server_cert_3.txt:41:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_3.txt:43:            X509v3 Subject Alternative Name: 
./security/advancedtls/testdata/server_cert_1.txt:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/server_cert_1.txt:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/server_cert_1.txt:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/server_cert_1.txt:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_1.pem:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/server_cert_1.pem:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/server_cert_1.pem:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/server_cert_1.pem:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_2.txt:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/server_cert_2.txt:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/server_cert_2.txt:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/server_cert_2.txt:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_2.txt:91:         
./security/advancedtls/testdata/another_client_cert_1.pem:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/another_client_cert_1.pem:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/another_client_cert_1.pem:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/another_client_cert_1.pem:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_localhost_1.pem:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/server_cert_localhost_1.pem:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/server_cert_localhost_1.pem:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/server_cert_localhost_1.pem:59:            X509v3 Key Usage: 
./security/advancedtls/testdata/server_cert_localhost_1.pem:61:            X509v3 Subject Alternative Name: 
./security/advancedtls/testdata/client_cert_1.pem:52:            X509v3 Subject Key Identifier: 
./security/advancedtls/testdata/client_cert_1.pem:54:            X509v3 Authority Key Identifier: 
./security/advancedtls/testdata/client_cert_1.pem:57:            X509v3 Basic Constraints: 
./security/advancedtls/testdata/client_cert_1.pem:59:            X509v3 Key Usage: 
./examples/features/advancedtls/creds/server_cert_revoked.pem:52:            X509v3 Subject Key Identifier: 
./examples/features/advancedtls/creds/server_cert_revoked.pem:54:            X509v3 Authority Key Identifier: 
./examples/features/advancedtls/creds/server_cert_revoked.pem:56:            X509v3 Basic Constraints: 
./examples/features/advancedtls/creds/server_cert_revoked.pem:58:            X509v3 Key Usage: 
./examples/features/advancedtls/creds/server_cert_revoked.pem:60:            X509v3 Subject Alternative Name: 
./examples/features/advancedtls/creds/client_cert.pem:52:            X509v3 Subject Key Identifier: 
./examples/features/advancedtls/creds/client_cert.pem:54:            X509v3 Authority Key Identifier: 
./examples/features/advancedtls/creds/client_cert.pem:56:            X509v3 Basic Constraints: 
./examples/features/advancedtls/creds/client_cert.pem:58:            X509v3 Key Usage: 
./examples/features/advancedtls/creds/client_cert.pem:60:            X509v3 Subject Alternative Name: 
./examples/features/advancedtls/creds/server_cert.pem:52:            X509v3 Subject Key Identifier: 
./examples/features/advancedtls/creds/server_cert.pem:54:            X509v3 Authority Key Identifier: 
./examples/features/advancedtls/creds/server_cert.pem:56:            X509v3 Basic Constraints: 
./examples/features/advancedtls/creds/server_cert.pem:58:            X509v3 Key Usage: 
./examples/features/advancedtls/creds/server_cert.pem:60:            X509v3 Subject Alternative Name: 
./examples/features/advancedtls/creds/client_cert_revoked.pem:52:            X509v3 Subject Key Identifier: 
./examples/features/advancedtls/creds/client_cert_revoked.pem:54:            X509v3 Authority Key Identifier: 
./examples/features/advancedtls/creds/client_cert_revoked.pem:56:            X509v3 Basic Constraints: 
./examples/features/advancedtls/creds/client_cert_revoked.pem:58:            X509v3 Key Usage: 
./examples/features/advancedtls/creds/client_cert_revoked.pem:60:            X509v3 Subject Alternative Name: 

ALL FIXED in follow up commit

scripts/vet.sh Outdated
@@ -184,4 +184,7 @@ revive -formatter plain ./... >"${REV_OUT}" || true
# TODO: Remove `|| true` to unskip linter failures once existing issues are fixed.
(noret_grep -v "unused-parameter" "${REV_OUT}" | not grep -v "\.pb\.go:") || true

# Error if trailing spaces found in any files excluding files in .git directory and *.md files
$(find . -path ./.git -prune -o -type f ! -name "*.md" -exec grep -Hn "[[:blank:]]$" {} \;) | fail_on_output
Copy link
Member

Choose a reason for hiding this comment

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

Why exclude markdown files? I think trailing whitespaces should always be disallowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIxed

@dfawley dfawley removed their assignment Aug 30, 2024
scripts/vet.sh Outdated
@@ -184,4 +184,7 @@ revive -formatter plain ./... >"${REV_OUT}" || true
# TODO: Remove `|| true` to unskip linter failures once existing issues are fixed.
(noret_grep -v "unused-parameter" "${REV_OUT}" | not grep -v "\.pb\.go:") || true

# Error if trailing spaces found in any files excluding files in .git directory and *.md files
$(find . -path ./.git -prune -o -type f ! -name "*.md" -exec grep -Hn "[[:blank:]]$" {} \;) | fail_on_output
Copy link
Contributor

Choose a reason for hiding this comment

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

Why surround the command in $()? This will cause the output of grep to be executed, which isn't what we want. I believe we can remove the surrounding $()

find . -path ./.git -prune -o -type f ! -name "*.md" -exec grep -Hn "[[:blank:]]$" {} \; | fail_on_output

However, this will not show all the violations because fail_on_output is implemented to exit early with a non-zero exit code if it sees any output. So it closes the read side of the pipe causing errors similar to the following if grep is still writing more results.

find . -path ./.git -prune -o -type f ! -name "*.md" -exec grep -Hn "[[:blank:]]$" {} \; | fail_on_output   
./features/advancedtls/creds/server_cert.pem:52:            X509v3 Subject Key Identifier: 
./features/advancedtls/creds/server_cert.pem:54:            X509v3 Authority Key Identifier: 
./features/advancedtls/creds/server_cert.pem:56:            X509v3 Basic Constraints: 
./features/advancedtls/creds/server_cert.pem:58:            X509v3 Key Usage: 
./features/advancedtls/creds/server_cert.pem:60:            X509v3 Subject Alternative Name: 
find: ‘grep’ terminated by signal 13
find: ‘grep’ terminated by signal 13

A workaround would be to define a version of fail_on_output that reads the entire input before exiting, the following code from an LLM demonstrates this:

fail_on_output() {                                                                                       
  output=$(tee /dev/stderr) # Read the entire output
  if [ -n "$output" ]; then
    return 1
  fi
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah missed it. Good catch. The current command does show the violations though

@arjan-bal arjan-bal assigned purnesh42H and unassigned arjan-bal Sep 2, 2024
scripts/vet.sh Outdated
@@ -184,4 +184,7 @@ revive -formatter plain ./... >"${REV_OUT}" || true
# TODO: Remove `|| true` to unskip linter failures once existing issues are fixed.
(noret_grep -v "unused-parameter" "${REV_OUT}" | not grep -v "\.pb\.go:") || true

# Error if trailing spaces found in any files excluding files in .git directory and *.md files
find . -path ./.git -prune -o -type f -exec grep -Hn "[[:blank:]]$" {} \; | fail_on_output
Copy link
Member

Choose a reason for hiding this comment

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

Does git grep "[[:blank:]]$" | fail_on_output not work? That seems a lot simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah for some reason grep fails fail_on_output even when there are no violations of trailing whitespace. We probably need to modify fail_on_output() function or use something else

Copy link
Member

Choose a reason for hiding this comment

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

I gave it a try on my end, and it seems to be working fine

~/grpc-go pr/purnesh42H/7576*git grep "[[:blank:]]$" | fail_on_output
~/grpc-go pr/purnesh42H/7576*echo $?
0

~/grpc-go pr/purnesh42H/7576*git grep "[[:blank:]]" | fail_on_output
~/grpc-go pr/purnesh42H/7576*echo $?
1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but I don't i see Success at the end when running scripts/vet.sh with this command even though no violations. That's why I am not sure.

Copy link
Member

Choose a reason for hiding this comment

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

See the other places we are doing git grep in here then... E.g.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"git grep '[[:blank:]]$' returns an exit status of 1 when no trailing spaces are found.. Hence, we have to negate it. Let me know if this looks good now

@purnesh42H purnesh42H force-pushed the vet-trailing branch 4 times, most recently from db745d0 to 54a8599 Compare September 6, 2024 17:52
scripts/vet.sh Outdated
@@ -67,6 +67,9 @@ not git grep "\"github.com/golang/protobuf/*" -- "*.go" ':(exclude)reflection/te
# - Ensure all usages of grpc_testing package are renamed when importing.
not git grep "\(import \|^\s*\)\"google.golang.org/grpc/interop/grpc_testing" -- "*.go"

# Ensure that no trailing spaces are found.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why, exactly, but all the section comments start with -; please continue the pattern here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dfawley dfawley assigned purnesh42H and unassigned dfawley Sep 9, 2024
@purnesh42H purnesh42H modified the milestones: 1.67 Release, 1.68 Release Sep 10, 2024
@purnesh42H purnesh42H merged commit b6fde8c into grpc:master Sep 12, 2024
14 checks passed
@purnesh42H purnesh42H added the Area: Tooling Includes anything related to Go builds, modules etc and includes Releases & Github Workflows. label Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Tooling Includes anything related to Go builds, modules etc and includes Releases & Github Workflows. Type: Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants