kubernetes-sigs / aws-ebs-csi-driver

CSI driver for Amazon EBS https://aws.amazon.com/ebs/
Apache License 2.0
998 stars 800 forks source link

Make govulncheck only run on release branches #2132

Closed AndrewSirenko closed 2 months ago

AndrewSirenko commented 3 months ago

Is this a bug fix or adding new feature?

N/A

What is this PR about? / Why do we need it?

This makes the govulncheck github action only run on PRs merging into release branches:

The risk here is that we suddenly find out about a vulnerability for our release PR, instead of in any pr, which may delay our release by 1-3 hours (due to waiting for CI to re-run). But that should be rare since we are bumping dependencies right before a release anyway.

What testing is done?

CI

AndrewSirenko commented 3 months ago

Thank you for your concern @ConnorJC3, but please hold off on review until it is no longer marked WIP because I have yet to do the testing that I described in PR comment.

What I saw from the docs and relevant stack overflow posts was that the job will fail with a red X, the failure will not block other CI actions for the PR. So there will be a failure shown without blocking merge, but this must be validated.

If a particular step was marked as optional, what you said would be correct.

AndrewSirenko commented 2 months ago

Looks like from GitHub's point of view, maintainers can merge if govulncheck action fails because branch protection rules do not include it as a required check.

However from PROW's POV, tide will block merges because it implicitly sets all actions as required for merge (See #2139).

We may be able to exclude this action in the prow config, which would mean we get best of both worlds (clear red x to signify govulncheck failed, but our ci wouldn't be blocked).

If changing prow config is not desired, we can run this job only on release-branches as to not block new maintainers.


Either way, we can consider updating our default branch protection rules to include most of the other status checks, including tide, if we want to ensure maintainers cannot merge without PROW.

ConnorJC3 commented 2 months ago

My suggestion would be to run it only on the release branch. If we make it optional on Prow, we are going to eventually accidentally merge a release with a known CVE.

There's no circumstance where we would want to intentionally merge a release PR containing known vulnerabilities, so the only potential downside of this solution is false positives. False positives are very rare (I think we've had one over the past year?) and the only reason we're talking about them is recency bias. Additionally, it's trivial to disable the job if that ever happens during a release.

AndrewSirenko commented 2 months ago

/unhold

k8s-ci-robot commented 2 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: torredil

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

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/OWNERS)~~ [torredil] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment