go.mod: upgrade to the highest patch versions #1210
Conversation
| github.com/armon/go-radix v1.0.0 // indirect | ||
| github.com/aws/aws-sdk-go v1.34.28 | ||
| github.com/aws/aws-sdk-go v1.34.34 |
There was a problem hiding this comment.
I'm a bit confused here. You say (in the PR description):
[...] to avoid introducing bugged versions when a user dependency gets updated due to a dd-trace-go upgrade.
Do you mean to say in situations where there is no avoiding the upgrade of a dependency? I'll assume so, because otherwise we want to avoid dd-trace-go causing upgrades at all costs and we should be keeping the minimum supported versions here.
Lastly, I just want to say that in some situations, some people don't want the latest patch version. We have specific situations like this ourselves internally - where patch versions introduce issues so we're not upgrading. This is of course an exceptional situation, but I just want to bring awareness to it.
There was a problem hiding this comment.
Do you mean to say in situations where there is no avoiding the upgrade of a dependency? I'll assume so, because otherwise we want to avoid dd-trace-go causing upgrades at all costs and we should be keeping the minimum supported versions here.
I talk about the case where the user dependency gets updated due to dd-trace-go.
If we take the example of what happened in dd-go:
- dd-go was using sqlx v1.2.0
- dd-trace-go was using sqlx v1.3.0 (bugged)
- so dd-go resulted in using sqlx v1.3.0, whose bugs were fortunately caught by their integration tests.
Lastly, I just want to say that in some situations, some people don't want the latest patch version. We have specific situations like this ourselves internally - where patch versions introduce issues so we're not upgrading. This is of course an exceptional situation, but I just want to bring awareness to it.
A patch version introducing new issues? 🤔
We just need to decide what to do:
- Keeping the lowest even if they are bugged (ie. they have higher patch release versions available) and let the user upgrade itself. The issue I see with this with the previous example is that maybe it won't be caught by any test and lead to run time/production issues pointing.
- Or like in this PR where we upgrade to the highest patch release (every time we release?).
There was a problem hiding this comment.
@Julio-Guerra regarding the sqlx v1.3.0, just to re-iterate here so it's not lost in internal Slack, I intended to downgrade all dependencies to their lowest working, non-vulnerable versions in #1188, but I missed that sqlx v1.2.0 was the lowest listed on pkg.go.dev and still passes our tests. So the fact that it was set to v1.3.0 was just a mistake on my end, and isn't directly related to this issue.
There was a problem hiding this comment.
@Julio-Guerra I think we need to always keep the lowest possible supported version, regardless of its state (bugged or not), and let the user have full control of which version they want to use.
@nsrip-dd thanks for following up. Perhaps a short script can be written to do this.
There was a problem hiding this comment.
Didn't we agree that we should have in our go.mod file the highest non vulnerable versions (of the lowest supported one)? If that's the case, then the same logic applies to patch versions for the same reasons.
There was a problem hiding this comment.
@DataDog/profiling-go, @DataDog/tracing-go, @DataDog/appsec-go: let's get straight to the point: the current go.mod update rule is stated at https://github.com/DataDog/dd-trace-go/blob/julio.guerra/go-mod-patch-updates/CONTRIBUTING.md?plain=1#L43 and says "prefer the minimum secure versions of any modules rather than the latest versions".
What I propose is adapt this rule so that any new dependency added to go.mod should also be upgraded to its highest patch releases [of its minimum secure version]. The following dd-trace-go releases could then keep the existing go.mod file dependencies the way they are regardless of new patch versions. But newly added dependencies should be again updated to their highest patch versions of their "the minimum secure versions".
There was a problem hiding this comment.
The agreement from the last guild meeting (according to the notes) was this:
We’ll try to use minimal versions for all dependencies that compiles against contrib and doesn’t pop up vulnerability warnings on GitHub.
@Julio-Guerra I would say we should treat critical bugs the same way as vulnerabilities: I.e. we should upgrade to the next higher version of the dependency that fixes the bug.
I think we need to always keep the lowest possible supported version, regardless of its state (bugged or not), and let the user have full control of which version they want to use.
@gbbr users retain full control via the replace directive in any case.
Further I think we should not assume responsibility for implicitly upgrading modules that are also depended on by user applications. This behavior is inherent to the Go module ecosystem and users need to verify the indirect impacts of any module upgrade, not just dd-trace-go.
Regardless of what we decide, I think we need to write up our exact policy somewhere and agree to it. If somebody wants to take a shot a this, let me know. If not I can put it on my plate, but I won't be able to deliver it until next week or later (we have R&D week next week). Edit: Our CONTRIBUTING.md file has our policy. We probably just need to clarify the situation if a version has "critical" bugs (e.g. those breaking dd-go).
There was a problem hiding this comment.
(Julio) Didn't we agree that we should have in our go.mod file the highest non vulnerable versions (of the lowest supported one)? If that's the case, then the same logic applies to patch versions for the same reasons.
The same logic doesn't apply for patch versions. Patch versions fix bugs that are not necessarily vulnerabilities. Vulnerabilities have a special status in the wider software community that garden-variety bugs don't have. It is not our responsibility to force an upgrade to fix any random bug.
(Felix) @Julio-Guerra I would say we should treat critical bugs the same way as vulnerabilities: I.e. we should upgrade to the next higher version of the dependency that fixes the bug.
I think this is OK. We don't want to cause an upgrade that totally breaks a user application. However we need to be careful how we define "critical". Are they ones that break dd-trace-go? Ones that just flat don't work for some reason? Ones that are retracted?
(Felix) Further I think we should not assume responsibility for implicitly upgrading modules that are also depended on by user applications. This behavior is inherent to the Go module ecosystem and users need to verify the indirect impacts of any module upgrade, not just dd-trace-go.
Yes, as far as I'm concerned, the goal is to allow the broadest range of versions to the user, with the least likelihood of forcing an update, while ensuring if we do force an update it does not introduce a vulnerability. I think this policy achieves that and fits well with Go's "minimum version selection" principle
|
Closing this as we decided to only have in the go.mod file the lowest non vulnerable version of our dependencies for now. |
Result of running
go1.14 get -u=patch ./… && go1.14 mod tidyin order to provide the highest available patch versions to avoid introducing bugged versions when a user dependency gets updated due to a dd-trace-go upgrade.