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

Add govulncheck script to expose go vulnerabilities in a PR #101

Closed
wants to merge 1 commit into from

Conversation

ArkaSaha30
Copy link
Member

@ArkaSaha30 ArkaSaha30 commented Sep 1, 2023

This PR will add a govulncheck script to check for go vulnerabilities when a PR is raised. It will be triggered as a presubmit from test-infra/config/jobs/kubernetes/sig-security/govulncheck-presubmit.yaml for every PR opened for go module changes.

Fixes: #99
Parent Issue: #95

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 1, 2023
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 1, 2023
@PushkarJ
Copy link
Member

PushkarJ commented Sep 1, 2023

/assign @liggitt @PushkarJ

Comment on lines 18 to 29
export WORKDIR=${ARTIFACTS:-$TMPDIR}
export PATH=$PATH:$GOPATH/bin
mkdir -p "${WORKDIR}"
pushd "$WORKDIR"
go install golang.org/x/vuln/cmd/govulncheck@latest
popd

govulncheck -scan module ./... > "${WORKDIR}/head.txt"
git reset --hard HEAD
git checkout -b base "${PULL_BASE_SHA}"
govulncheck -scan module ./... > "${WORKDIR}/pr-base.txt"
diff -s -u --ignore-all-space "${WORKDIR}"/pr-base.txt "${WORKDIR}"/head.txt || true
Copy link
Member

Choose a reason for hiding this comment

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

@ArkaSaha30 were you able to test this locally with prow to see if the directories line up correctly with where govulncheck needs to run?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried with pj-on-kind, looks like it is trying to run in the current directory instead of k8s.io/kubernetes. Working on fixing it, thank you

Copy link
Member Author

@ArkaSaha30 ArkaSaha30 Sep 5, 2023

Choose a reason for hiding this comment

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

Instead of changing this script I have fixed the path in check-govulncheck-results presubmit:

-          cd sig-security-tooling/govulncheck/hack/ && ./govulncheck-presubmit.sh
+          cd ../kubernetes
+          ../sig-security/sig-security-tooling/govulncheck/hack/govulncheck-presubmit.sh

And the govulncheck is running successfully(used #120341 as the PR to run the presubmit):

/logs/artifacts /home/prow/go/src/k8s.io/kubernetes
go: downloading golang.org/x/vuln v1.0.1
go: downloading golang.org/x/mod v0.12.0
go: downloading golang.org/x/tools v0.12.1-0.20230815132531-74c255bcf846
go: downloading golang.org/x/sync v0.3.0
go: downloading golang.org/x/sys v0.11.0
/home/prow/go/src/k8s.io/kubernetes
HEAD is now at 6e95fa1942e Merge commit '123da8cc1fed775a68f97fd4beb032f2b45a0200'
Switched to a new branch 'base'
--- /logs/artifacts/pr-base.txt	2023-09-05 15:17:04.307034006 +0000
+++ /logs/artifacts/head.txt	2023-09-05 15:16:47.373852012 +0000
@@ -1,4 +1,4 @@
-Scanning your code and 2056 packages across 219 dependent modules for known vulnerabilities...
+Scanning your code and 2088 packages across 221 dependent modules for known vulnerabilities...
 
 No vulnerabilities found.
 

Copy link
Member Author

Choose a reason for hiding this comment

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

Can I go ahead with the above change?

Copy link
Member

Choose a reason for hiding this comment

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

Yes go for it. Thanks for testing this 🙌🏼

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ArkaSaha30
Once this PR has been reviewed and has the lgtm label, please ask for approval from pushkarj. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: ArkaSaha30 <arkasaha30@gmail.com>
Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

[SIG Testing Hat On]

Typically we run a tool like this with a script in the repo under test (IE kubernetes/kubernetes).

That way changes to the script can be safely made without suddenly failing all PRs, the new changes will only take affect once the PR with the changes passes running those tests on the repo under test because it is the same repo with the script.

Additionally, it is best practice to install a specific version of a linter etc for the same reason, rather than @latest. In k/k it should be versioned in hack/tools/go.mod

@liggitt
Copy link
Member

liggitt commented Sep 7, 2023

+1 to the verify script living in k/k and versioning the govulncheck lib

@PushkarJ
Copy link
Member

PushkarJ commented Dec 8, 2023

Completed via kubernetes/kubernetes#120562

/close

@k8s-ci-robot
Copy link
Contributor

@PushkarJ: Closed this PR.

In response to this:

Completed via kubernetes/kubernetes#120562

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[govulncheck] Pre-submit Prow Job for govulncheck
5 participants