search

fluxcd / flux

Successor: https://github.com/fluxcd/flux2
https://fluxcd.io
Apache License 2.0
6.9k stars 1.08k forks source link

Update kubernetes go client (@ 0.22.0), other go mods #3537

Closed kingdonb closed 3 years ago

kingdonb commented 3 years ago

This PR combines #3515 and #3512, merging them correctly (which they would have generated a conflict otherwise) and also should complete the Kubernetes CLI upgrade, from 1.17.17 to 1.21.3, and now to 1.22.0.

which are both about go module updates.

The upgrade to Kubernetes Client 1.21.3 required some changes to the code, I have to admit I don't know much about Kubernetes context, and I've basically added Context.TODO() calls in every location that required a context, ...

Since taking advantage of any context for a feature reason would have represented doing some feature work for Flux v1, I did not do that. We can consider taking advantage of context to fix bugs in the future, but I don't know when that will happen or what form that effort would have to take. It's not work I have planned myself.

I think it's fine to leave all of these Context.TODO() stubs around forever, I'd be happy to be told by someone that knows more if that's OK (or even if that's not strictly true).

I will keep this as a draft for now until it shows tests passing and until I have verified there are no more stragglers to upgrade before we call it for the 1.24.0 Flux milestone release.

kingdonb commented 3 years ago

Tests failed because:

Error: ../../../go/pkg/mod/k8s.io/client-go@v0.22.0/plugin/pkg/client/auth/exec/metrics.go:21:2: package io/fs is not in GOROOT (/opt/hostedtoolcache/go/1.15.15/x64/src/io/fs)

This PR updated the toolchain to Go 1.16.x but CI was not yet upgraded. Adding a commit for that, CI should pass after.

kingdonb commented 3 years ago

~I think we should also rebase and merge #3514, after this one is merged.~ #3514 was merged ahead of this.

Then let's consider adding these three that have been waiting for merge for a while:

Those are all bug fixes that I think we have reasonably well understood.

The main driver for this release is that there are critical CVEs reported from scans of our base image, it would be good to push 1.24.0 even sooner if possible. We can also do this without the three bugfixes above, but I see no reason to leave them behind any longer, I think they can all be merged, and hopefully it will not be necessary to do another release soon again.

I will remember also to document that #3015 is actually fixed in this upcoming 1.24.0 release (not in 1.23.2, as was initially planned and announced.) There is also #3536 which I think will not be a controversial change at all to merge.

I'll get those merged in order, write up a CHANGELOG if there are no objections soon, and for now this is the plan for the next Flux V1 MINOR release, 1.24.0. I am hoping to cut the final build including all these by Friday.

If there are other things to add, let's have them out ASAP. I think this will most likely be everything for the next Flux v1 release, excepting housekeeping and any needed docs updates.

(The next release for me after that will probably be Helm Operator, if there are any Snyk scan results showing CVEs there too.) I think that will leave only one Flux v1 PR still in the list unmerged, with nothing else remotely urgent or majorly important at all being left behind. 👍

kingdonb commented 3 years ago

I should give some credit for this release to @renovate-bot which made it very easy to corral all the pending updates and separate them out, testing one at a time, merging only when the tests are green, and holding back or rebasing updates that could not be completed successfully without causing errors.

https://github.com/kingdonb/flux/pulls

There are some PRs for go.mod updates that were left behind there, I don't think there's anything particularly urgent or important from that list. I will continue to monitor for updates there, and bring them into Flux v1 as opportunities present themselves. Anyway, I don't think anything from that list should still be considered for this next release.

kingdonb commented 3 years ago

The commits I proposed to merge above, all combined with this PR, should be roughly equal to what is in 0294896a

I kept that altogether-merged branch out of every PR to better preserve the ability for individuals to perform reviews one at a time as needed, but for anyone that wants to build their own image and test it all together, that's the commit to test (kingdonb/flux:master @ 0294896aee2c644a039b02e23307a7088f434974)

kingdonb commented 3 years ago

This change is closed in favor of #3538

After discussing our support policy and the potential impact of upgrading too far, I understand the relevant policies now. That discussion took place at today's CNCF Flux Dev meeting, for any who might be interested in the details of that conversation.