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
kubernetes: 1.18.8 -> 1.19.1 #96446
kubernetes: 1.18.8 -> 1.19.1 #96446
Conversation
@GrahamcOfBorg build kubernetes kubectl |
@saschagrunert That was fast 🚀 Thanks! 🎉 Haven't had the slightest chance to test this though. |
Note: Golang v1.15 common name deprecation (https://golang.org/doc/go1.15#commonname , see also: kubernetes/kubernetes#93264) breaks easyCerts and hence the Kubernetes tests are now broken as well. We need to fix easyCerts and we need at least a 20.09 release note about this, since it might break clusters with custom PKI-setups. I know that this is more a Go 1.15 thing than a Kubernetes thing, but I think it would be good to have the k8s tests passing, before merging this. I won't have time over the weekend, but will look into it first thing next week if no one beats me to it. |
I agree 👍 |
@saschagrunert The tests pass for me when I apply this patch: diff --git a/nixos/modules/services/cluster/kubernetes/pki.nix b/nixos/modules/services/cluster/kubernetes/pki.nix
index 4275563f1a3..933ae481e96 100644
--- a/nixos/modules/services/cluster/kubernetes/pki.nix
+++ b/nixos/modules/services/cluster/kubernetes/pki.nix
@@ -20,7 +20,7 @@ let
size = 2048;
};
CN = top.masterAddress;
- hosts = cfg.cfsslAPIExtraSANs;
+ hosts = [top.masterAddress] ++ cfg.cfsslAPIExtraSANs;
});
cfsslAPITokenBaseName = "apitoken.secret";
@@ -228,7 +228,8 @@ in
};
private_key = cert.privateKeyOptions;
request = {
- inherit (cert) CN hosts;
+ hosts = [cert.CN] ++ cert.hosts;
+ inherit (cert) CN;
key = {
algo = "rsa";
size = 2048; Same for you? ... basically, it adds the CN to the list of sans for all issued certificates. Using CN still is ok, as long as any hostname mentioned in the CN-field is also present in the SAN-list. If you want, you can add this patch or something similar to the PR. :) Will you do the honours of condensing some of this info into a release note? |
Nice, thank you for the fix! 🙏 Tested it via nixos-shell that everything works as expected ✔️
Sure, I added the patch to this PR. :) I'm not sure any more if we need a release note for that change. Usually the user should not notice any change, right? Or is there any user action required now? |
I guess it's a matter of taste. I don't know what the official policy is in the community. I do know that some use Kubernetes without easyCerts enabled ( my company included ) and we will be affected by the Golang 1.15 upgrade. Hmm, I don't know. It's more like a Go 1.15 release note I guess. @srhb any opinions? Other than that. This PR looks good to merge for me. :) |
Sorry, I've been away on vacation. If there's any doubt whether users of kubernetes should be aware of this upgrade, I see no reason not to include a release note. It doesn't have to be particularly in-depth, but if we know there's a pitfall here regarding go 1.15 and CN/SANS, let's at least mention it. Other than that, LGTM. Can merge and backport as soon as it has a release note :) |
Signed-off-by: Sascha Grunert <sgrunert@suse.com>
Signed-off-by: Sascha Grunert <sgrunert@suse.com>
Added a release note and bumped Kubernetes to v1.19.1, too. |
@GrahamcOfBorg build kubernetes |
Hmm, that doesn't sound quite right to me. As I understand it, we will break certificates that have been relying on CN instead of SANs for the host name (because of the go 1.15 bump, not this PR specifically.) Whether or not that causes a lot of cluster breakage (it probably does?) depends on the specifics of the cluster setup, but I think that's what the heads-up should be. @johanot Can you clarify this - did I misunderstand the issue? If we can't get more specific -- and I haven't thought about it yet -- maybe something generic like:
And a link to https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.19.md#changes-by-kind-1 I guess. The phrasing from the release notes that worries me is this:
|
@srhb Your comprehension of the issue is correct. The important thing is that users understand that this might cause cluster breakage, depending on how cluster certificates are issued. Users that have re 1) Actually this is not entirely true, because easyCerts users still need to rotate/re-issue their certificates. I don't know if we need to write a specific guide for that? |
Signed-off-by: Sascha Grunert <sgrunert@suse.com>
Sounds good, I updated the release note and linked it in the text. |
20.09: Merge pull request #96446 from saschagrunert/k8s
Motivation for this change
Update k8s to the latest release.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)