Update controller-runtime to 0.17.3 #232
Conversation
go.mod
Outdated
| k8s.io/api => k8s.io/api v0.29.2 // Replaced so that 'go get -u' works. Remove/bump when upgrading. | ||
| k8s.io/client-go => k8s.io/client-go v0.29.2 // Replaced so that 'go get -u' works. Remove/bump when upgrading. | ||
| k8s.io/kubectl => k8s.io/kubectl v0.28.0 // Replaced so that 'go get -u' works. Remove/bump when upgrading. | ||
| sigs.k8s.io/kustomize/api => sigs.k8s.io/kustomize/api v0.15.0 // Replaced so that 'go get -u' works. Remove/bump when upgrading. | ||
| sigs.k8s.io/kustomize/kyaml => sigs.k8s.io/kustomize/kyaml v0.15.0 // Replaced so that 'go get -u' works. Remove/bump when upgrading. |
There was a problem hiding this comment.
@dhaiducek if I remember right, you added these? I'd like to remove any of them if possible... is it as simple as removing it here, running a tidy, and then running some test somewhere involving go test -u? (that last bit is where I get lost)
There was a problem hiding this comment.
Yeah, they were set so that Go could programmatically find an unblocked upgrade path with go get -u. During an upgrade, they should be removed entirely and replacements added back on an as-needed basis with the highest possible version.
go.mod
Outdated
| ) | ||
|
|
||
| replace ( | ||
| github.com/imdario/mergo => github.com/imdario/mergo v0.3.16 // Replaced so that 'go get -u' works. Remove/bump when upgrading. | ||
| golang.org/x/crypto => golang.org/x/crypto v0.14.0 // CVE-2021-43565 | ||
| golang.org/x/crypto => golang.org/x/crypto v0.16.0 // CVE-2021-43565 |
There was a problem hiding this comment.
This might be a good time to clean up some of these replace directives for CVEs. I think that if you remove them here, and check the version it would choose (maybe in the go.sum after running a tidy), we can see if that would still be affected by this CVE. I expect some of them might have been updated in our direct dependencies, and so we don't need the replace anymore.
If this ends up being a big task, we can split it out into another issue.
Signed-off-by: Jeffrey Luo <jeluo@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JeffeyL, mprahl 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:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 58ae6e3
into
open-cluster-management-io:main
| golang.org/x/crypto => golang.org/x/crypto v0.14.0 // CVE-2021-43565 | ||
| golang.org/x/net => golang.org/x/net v0.17.0 // CVE-2023-39325 | ||
| golang.org/x/text => golang.org/x/text v0.13.0 // CVE-2022-32149 | ||
| gopkg.in/yaml.v2 => gopkg.in/yaml.v2 v2.4.0 // CVE-2022-3064 |
There was a problem hiding this comment.
I have concerns that removing this block re-introduced affected versions of these packages in go.sum. Is updating the indirects sufficient to resolve the CVEs?
There was a problem hiding this comment.
We might be ok - from what I'm reading in https://go.dev/ref/mod#go-sum-files, I think that the <module> <version>/go.mod entries that are now in the go.sum file and refer to older (possibly affected) versions are not actually being used. But I'm going to research this some more because it would be really nice to get rid of some of these directives
There was a problem hiding this comment.
Before approving the PR, I researched this and my understanding is that what is actually used is in go.mod but go.sum has all the entries of the Go modules that the minimal version selection algorithm looked at to come up with the go.mod result.
There was a problem hiding this comment.
Ok, I think go list -m <module> gives a less ambiguous answer to what version of the dependency is being used, and shows that these are good:
❯ go list -m golang.org/x/crypto
golang.org/x/crypto v0.21.0
❯ go list -m golang.org/x/net
golang.org/x/net v0.22.0
❯ go list -m golang.org/x/text
golang.org/x/text v0.14.0
❯ go list -m gopkg.in/yaml.v2
gopkg.in/yaml.v2 v2.4.0
There was a problem hiding this comment.
Nice--if that's the case, then we can stop using replace for CVEs!
Changes required to update controller-runtime to 0.17.
Ref: https://issues.redhat.com/browse/ACM-10835