RFC 153 - Remove allowlists from Dependabot configs #153
Conversation
ChrisBAshton
force-pushed
the
revert-rfc-126
branch
from
85db880 to
e74dfc5
Compare
There was a problem hiding this comment.
This RFC makes me very happy.
We've had several Rails upgrades that have proven more difficult than expected as multiple dependencies needed upgrading. Doing these updates incrementally would've spread this work over a longer period of time and focussed on a single gem at a time.
|
Thank you for raising this, I've had many occasions where there has been a vulnerability due to a package used by another package but dependabot never picked it up! 🏅 |
|
Thanks for looking into this and raising this - awesome stuff 👍 As someone who merges a heck of a lot of dependabot PRs, this proposal does make me a little concerned as it doesn’t really feel like we stay on top of the ones we have? Rather than having this unusual “Revert RFC 126” would be able to give this a new title and indicate that it supersedes RFC 126 (and update that RFC to indicate it was superseded). Revert sounds like a step backwards and doesn’t remove remove that the RFC existed and was expected. I’d suggest something like "Treat dependabot dependencies equally" ? It sounds like the main motivation to change the behaviour is due to the security issue? I’m assuming we’d not be considering changing our configuration if it wasn’t for this as the other things listed in problems seem to be successes or unknowns. Not too big a deal as it doesn’t affect the proposal but does read a bit strange. Have we contacted GitHub at all to see what they recommend? It’s a bit of a pain as it used to do security updates despite the file configuration and then that seemed to change sometime earlier this year (e.g. alphagov/whitehall#6451). I somewhat wonder can you have two sets of configuration for a package-ecosystem - one that is an allow list and one that is security updates only? A few of further questions I have are:
|
|
Thanks for the review, @kevindew - responses below:
Yes, it will be harder to keep on top of these PRs in the short term: a sacrifice worth making to ensure we receive security updates, in my opinion. Teams have agency to work towards retiring repositories, removing dependencies, or even telling Dependabot to ignore major versions (which would not block security PRs), if they so wish. They can do so via 'ignore' lists (as hinted at in the RFC) or via GitHub comments. Meanwhile, Platform Reliability are hoping to introduce some more automation in future to help teams deal with the load (which we expect to increase by around 36% in the short term). Anecdotally, the updated configs might even make certain upgrades easier to perform.
Fair point. I'll rename, and update the other RFC 👍
Yes, absolutely. For context, this RFC came out of us trying to find ways to re-enable security updates, and concluding that the configs we arrived at in RFC-126 were preventing us from doing that. Given that these configs came in via an RFC, it feels important that any major change or removal of configs should also be subject to an RFC. We also recently spotted the intent to 'review the RFC after 6 months', so decided to perform a full review.
That idea was already attempted in the spike linked in the RFC (which is also where we reached out to GitHub for support).
I was imagining Platform Reliability would do it. We're quite well versed in raising PRs en-masse, and given there's no testing or deployment stage needed, it shouldn't be too much effort. We'd also merge the PRs ourselves, taking approval of this RFC as consent to make changes to the repos.
By "problem", do you mean 'creates too many PRs for devs to handle', or 'unexpectedly breaks Dependabot'? ± I'm hoping 'next major change' == 'automatically merge certain PRs', but that discussion is out of scope for this RFC.
I would hope teams will point to this RFC as a reason to not use an allow list, but you're right, I haven't written a hard rule here. I'd say it's up to teams discretion, but I can't really think of a situation where we wouldn't need security updates applied, so maybe it should have a section stating "teams MUST NOT use allow lists in Dependabot configs". What do you think? |
Yup, it's quite a blow that the security update ones stopped working as it's a major use case. Also seems a bit frustrating having to tinker around with the ignore config if we get a bit overwhelmed.
Sure, perhaps lead the problems section with this to state that the principal problem is the lack of security updates since dependabot's behaviour changed
Argh, sorry I missed that. Thats frustrating - awesome to get the @issyl0 support though ❤️ . Did we also try having a wildcard for allow of * and see if later options overrode it (though that feels a total hack so probably not wise).
Great stuff, may be worth having this in the consequences?
I meant the former, though I'd not really characterise it as too many PRs too handle, since dependabot stops opening new ones more that that just linger.
Yeah, I'm skeptical we'll get to that point myself but like you say out-of-scope. I generally tend to find ones that just pass aren't a big deal though, more the failed build ones seem to linger for ages.
Yup, that sounds like something good to state in consequences. Perhaps go for SHOULD NOT? It wouldn't seem that wrong to me for a gem to not be monitoring a security issue in a dependency for instance. |
When I can help I always will try to! 😍 It was an interesting thing to figure out, albeit not the answer you really wanted! |
- Rename the RFC (and filename) to better describe the proposal - Frontload the 'security' issue in the Problem section - Clarify the expected consequences of the RFC
|
@kevindew I've pushed up some commits in response to your comments 👍 |
ChrisBAshton
changed the title
This makes the configuration consistent with the rest of GOV.UK, taking into account [RFC-153](alphagov/govuk-rfcs#153).
This makes the configuration consistent with the rest of GOV.UK, taking into account [RFC-153](alphagov/govuk-rfcs#153).
In [RFC-153](alphagov/govuk-rfcs#153), we proposed and agreed to remove 'allow lists' from all [GOV.UK](http://gov.uk/) Dependabot configs, in order to re-enable security updates. Trello card: https://trello.com/c/DuA0q9Ck/2966-remove-allow-lists-from-dependabot-configs-2
In [RFC-153](alphagov/govuk-rfcs#153), we proposed and agreed to remove 'allow lists' from all [GOV.UK](http://gov.uk/) Dependabot configs, in order to re-enable security updates. Trello card: https://trello.com/c/DuA0q9Ck/2966-remove-allow-lists-from-dependabot-configs-2
Read the rendered version ⭐ 💅
Please comment by end of day 15th August.
Trello: https://trello.com/c/nqliwwxV/2952-investigate-how-to-fix-dependabot-not-raising-security-prs