Skip to content

infra: remove govulncheck script and GHA workflow - #434

Open
vitorfloriano wants to merge 1 commit into
kubernetes-sigs:mainfrom
vitorfloriano:fix-govulncheck-script
Open

infra: remove govulncheck script and GHA workflow#434
vitorfloriano wants to merge 1 commit into
kubernetes-sigs:mainfrom
vitorfloriano:fix-govulncheck-script

Conversation

@vitorfloriano

@vitorfloriano vitorfloriano commented Aug 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This PR refactors how govulncheck is used in the repo.

The govulncheck target was renamed to verify-govulncheck and the make target calls govulncheck using the default scan (symbol instead of package level), which performs reachability analysis and lessens the ocurrences of false positives.

The hack script and GitHub Actions workflow were removed and a periodic ProwJob should follow up.

We're also bumping govulncheck to v1.7.0.

Related Issue

Fixes #433

Type of Change

/kind bug
/kind cleanup
/kind failing-test

Testing

Ran make verify-govulncheck locally and it downloaded the binary and detected the vulnerabilities, as expected.

@kubernetes-prow kubernetes-prow Bot added kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. labels Aug 19, 2026
@netlify

netlify Bot commented Aug 19, 2026 •
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for node-readiness-controller canceled.

Name Link
🔨 Latest commit 89cc0a8
🔍 Latest deploy log https://app.netlify.com/projects/node-readiness-controller/deploys/6a88f74d335c090008ff0716

@kubernetes-prow

kubernetes-prow Bot commented Aug 19, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vitorfloriano
Once this PR has been reviewed and has the lgtm label, please assign tallclair for approval. For more information see the Code Review Process.

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

Details 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

@kubernetes-prow
kubernetes-prow Bot requested a review from tallclair August 19, 2026 18:12
@kubernetes-prow kubernetes-prow Bot added 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. labels Aug 19, 2026
@vitorfloriano

vitorfloriano commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

/cc @ajaysundark @AvineshTripathi

@vitorfloriano

vitorfloriano commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

govulncheck ran successfully (but didn't fail, despite the vulnerabilites found): https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_node-readiness-controller/434/pull-node-readiness-controller-verify-all/2090139930821922816#1:build-log.txt%3A69-585

@ajaysundark

ajaysundark commented Aug 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for looking into this, @vitorfloriano !

Should it fail if/when vulnerabilities are found?

I think it's one of those things that doesn't fit a presubmit best. Ideally for a vulnerability detector, we should move this into a periodic, that fails separately and can alert us. Potentially even having an "exemption" list so that we can 'silent' some failures if we need to. And our release process to check this periodic for green and address any patches if needed.

@vitorfloriano

vitorfloriano commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

I think it's one of those things that doesn't fit a presubmit best. Ideally for a vulnerability detector, we should move this into a periodic, that fails separately and can alert us. Potentially even having an "exemption" list so that we can 'silent' some failures if we need to. And our release process to check this periodic for green and address any patches if needed.

Agreed.

So, in this case, I guess I should pivot the PR to:

  • remove hack/verify-govulncheck.sh
  • remove hack/verify-govulncheck.sh from verify-all.sh
  • remove .github/workflows/govulncheck.yml
  • refactor make verify-govulncheck to run govulncheck -scan package ./... on the main branch (not base vs PR branch)

In a follow-up:

  • Add a periodic that runs make verify-govulncheck daily (or whatever cadence we decide upon)

WDYT @ajaysundark?

@ajaysundark

ajaysundark commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

I think it's one of those things that doesn't fit a presubmit best. Ideally for a vulnerability detector, we should move this into a periodic, that fails separately and can alert us. Potentially even having an "exemption" list so that we can 'silent' some failures if we need to. And our release process to check this periodic for green and address any patches if needed.

Agreed.

So, in this case, I guess I should pivot the PR to:

  • remove hack/verify-govulncheck.sh
  • remove hack/verify-govulncheck.sh from verify-all.sh
  • remove .github/workflows/govulncheck.yml
  • refactor make verify-govulncheck to run govulncheck -scan package ./... on the main branch (not base vs PR branch)

In a follow-up:

  • Add a periodic that runs make verify-govulncheck daily (or whatever cadence we decide upon)

WDYT @ajaysundark?

Sgtm. @Priyankasaggu11929 do you have any thoughts on tying this with periodics than presubmits?

@Priyankasaggu11929

Priyankasaggu11929 commented Aug 20, 2026

Copy link
Copy Markdown
Member

i think running as a periodics is good for our required signal right now

@vitorfloriano
vitorfloriano force-pushed the fix-govulncheck-script branch from 701007c to 361059a Compare August 21, 2026 19:46
@kubernetes-prow kubernetes-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 21, 2026
@vitorfloriano
vitorfloriano marked this pull request as draft August 21, 2026 19:53
@kubernetes-prow kubernetes-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 21, 2026
@vitorfloriano
vitorfloriano force-pushed the fix-govulncheck-script branch from 361059a to e711a19 Compare August 22, 2026 01:03
@vitorfloriano
infra: remove govulncheck script and gha workflow
89cc0a8 
Govulncheck is now called directly against main.

A follow-up must add a periodic prowjob to
run this check, instead of a presubmit.
@vitorfloriano
vitorfloriano force-pushed the fix-govulncheck-script branch from e711a19 to 89cc0a8 Compare August 22, 2026 01:11
@vitorfloriano vitorfloriano changed the title infra: refactor govulncheck target and hack script. infra: remove govulncheck script and GHA workflow Aug 22, 2026
@vitorfloriano
vitorfloriano marked this pull request as ready for review August 22, 2026 01:17
@kubernetes-prow kubernetes-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 22, 2026
@vitorfloriano

vitorfloriano commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

I made the necessary changes and will add the periodic next.

/hold

@kubernetes-prow kubernetes-prow Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 22, 2026
@vitorfloriano vitorfloriano mentioned this pull request Aug 22, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haircommander haircommander Awaiting requested review from haircommander
@tallclair tallclair Awaiting requested review from tallclair
@ajaysundark ajaysundark Awaiting requested review from ajaysundark
@AvineshTripathi AvineshTripathi Awaiting requested review from AvineshTripathi
@mrunalp mrunalp Awaiting requested review from mrunalp
@SergeyKanzhelev SergeyKanzhelev Awaiting requested review from SergeyKanzhelev

Assignees

No one assigned

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] command govulncheck not found in CI

3 participants

@vitorfloriano @ajaysundark @Priyankasaggu11929