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
go.mod: upgrade to the highest patch versions #1210
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused here. You say (in the PR description):
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
A patch version introducing new issues? 🤔
We just need to decide what to do:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The agreement from the last guild meeting (according to the notes) was this:
@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.
@gbbr users retain full control via the
replacedirective 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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?
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