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

Remove Snyk automated PRs -- dependabot handles this already #11844

Closed
agilgur5 opened this issue Sep 19, 2023 · 5 comments
Closed

Remove Snyk automated PRs -- dependabot handles this already #11844

agilgur5 opened this issue Sep 19, 2023 · 5 comments
Assignees
Labels
solution/suggested A solution to the bug has been suggested. Someone needs to implement it. type/dependencies PRs and issues specific to updating dependencies type/feature Feature request

Comments

@agilgur5
Copy link
Contributor

Summary

In the previous Contributors meeting, we discussed several improvements to our dependency automation.

One of these that I proposed was to remove automated Snyk PRs.

Use Cases

Pros

  • Dependabot already handles security updates, so we are duplicating tooling and duplicating PRs right now
    • We already have dependabot configured in several ways, including to align with DCO and PR title. Snyk fails both these checks so its automated PRs will currently never pass CI without manual intervention. Example Snyk PR failing checks
    • Old comment from Alex C saying the same thing
  • Snyk is also creating dependency update PRs that are not for any known security issues or CVEs, entirely duplicating dependabot
  • Snyk is currently using individual tokens for some PRs, which are themselves not ideal for security. Example Snyk PRs using individual tokens
    • Not sure how to configure these either
    • Some do use the snyk-bot though, not sure when that occurs.
  • Snyk sometimes gives inaccurate or buggy data in its automated PRs. Example incorrect data
  • Snyk does not preserve PR history or lineage well. Dependabot will try to rebase PRs and will say if it opened a new PR that supersedes an old PR. Snyk does not, and so I have manually commented on several Snyk PRs to notate that they were superseded
    • Closing a PR or issue without a comment is summarily confusing. This is partly due to how it is being used though
  • Many Snyk PRs are summarily closed by maintainers anyway. Examples
    • This adds even more PR noise and confusion, especially as those are closes without comments.
  • We still use Snyk for scanning in CI and do not plan to change that. This is just for Snyk's automated PRs.

Cons

  • I suppose Snyk could potentially raise different PRs from dependabot, but that is not necessarily a good thing
  • @sarabala1979 mentioned that Snyk was a requirement for CNCF graduation
    • If I had to guess, this was that some form of dependency scanning was a requirement. We're not removing Snyk scanning, just automated PRs. We also already have automated PRs via dependabot, so there is no loss in capability either.
    • Anton (me) to ask @crenshaw-dev if he remembers anything about this requirement.

Message from the maintainers:

Love this enhancement proposal? Give it a 👍. We prioritise the proposals with the most 👍.

@agilgur5 agilgur5 added type/feature Feature request type/dependencies PRs and issues specific to updating dependencies labels Sep 19, 2023
@terrytangyuan
Copy link
Member

terrytangyuan commented Sep 19, 2023

I agree we should stop those automated PRs since depdendabot does this already. I believe only @sarabala1979, and I have automated PRs from Snyk. That can be disabled from Snyk from our Snyk account/project settings.

@agilgur5
Copy link
Contributor Author

agilgur5 commented Sep 19, 2023

  • @sarabala1979 mentioned that Snyk was a requirement for CNCF graduation

I couldn't find anything myself about this. The official CNCF graduation criteria do not mention this. They just mention some requirements about best practices and a published security audit. Nothing specific like using Snyk.

I did recall this old issue which mentions "CNCF security engagement", but unfortunately has no real details or links to that (I assume this was from one of the audits? not sure).

  • Anton (me) to ask @crenshaw-dev if he remembers anything about this requirement.

I asked @crenshaw-dev and @alexec (over Slack) about this if they recall anything. Got a response from @crenshaw-dev yesterday (he was OOO for a few weeks) who did not recall such a requirement and thought the same as me re: "some tool for dependency scanning, e.g. Snyk". We also have a very legitimate reason for this (see the whole issue etc) and are not reducing any of our scanning capabilities either, so our security posture does not change.

So I don't think we have any real reasons to not move forward with this, IMO

@terrytangyuan
Copy link
Member

I have updated my personal Snyk account settings to not automatically create PRs. The last PR created by Snyk bot #10798 was a while back so I think we should be fine now.

@agilgur5
Copy link
Contributor Author

@sarabala1979 could you check your personal Snyk account? Once you've changed your settings, feel free to close out this issue.

@sarabala1979
Copy link
Member

Done. Removed Snyk scaning

@agilgur5 agilgur5 added the solution/suggested A solution to the bug has been suggested. Someone needs to implement it. label Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution/suggested A solution to the bug has been suggested. Someone needs to implement it. type/dependencies PRs and issues specific to updating dependencies type/feature Feature request
Projects
None yet
Development

No branches or pull requests

3 participants