-
-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
nixos/kubernetes: Module refactor #45670
Conversation
Success on aarch64-linux (full log) Attempted: kubernetes Partial log (click to expand)
|
Success on x86_64-linux (full log) Attempted: kubernetes Partial log (click to expand)
|
@johanot thank you for your hard work! I can definitely help review and test this PR, I'll put aside some time this evening to do so! |
@johanot thank you very much for this effort. I will take a look at this tomorrow, I'm especially curious about the TLS key generation and deployment. |
@steveej Thanks. Yeah, If I were you, I would concentrate the effort on |
Thanks for the work! I just tried it out using nixops, and most stuff seems to work well. The only issue so far is that I have to manually restart etcd and flannel after reboots, but I'm not sure if that is an issue with this pull request or something else. |
Actually, it's a bit annoying that KUBECONFIG is hardcoded in the wrapper. It's good for convenience, but it would be nice if the unwrapped kubectl was also in scope. |
@kristoff3r Wait.. What? In the test you mean? AFAIK, I only hard-wrapped kubectl in the NixOS test case, not in the module itself? (hopefully). Thanks for testing, btw :) |
I just figured that out as well, it was just me being confused. |
cc @srhb for a review please! |
I'm afraid I don't have time to do a very thorough review right now unfortunately, and I would appreciate it if someone who actively uses k8s on NixOS right now would chime in -- I no longer do. I will add a few notes (and some that @johanot and I have already talked about, for visibility.)
I'll do a quick line-by-line driveby as well. |
serviceAccountKeyFile = mkOption { | ||
description = '' | ||
Kubernetes apiserver PEM-encoded x509 RSA private or public key file, | ||
used to verify ServiceAccount tokens. By default tls private key file |
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.
ie. tlsKeyFile
? If so, mention it.
@@ -230,556 +126,22 @@ in { | |||
type = types.path; | |||
}; | |||
|
|||
easyCerts = mkOption { | |||
description = "Automatically setup x509 certificates and keys for the entire cluster."; |
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.
Maybe this should refer to the documentation for a quick rundown on how it works?
featureGates = mkOption { | ||
description = "List set of feature gates"; | ||
description = "List set of feature gates."; |
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.
Another "list set"
enable = mkEnableOption "Whether to enable easyCert issuer service."; | ||
|
||
certs = mkOption { | ||
description = "List of certificate specs to feed to cert generator."; |
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.
Can we refer the user to the specs docs here?
}; | ||
|
||
featureGates = mkOption { | ||
description = "List set of feature gates"; |
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.
Another "list set" -- I guess this was already around before your changes
One thing I hadn't thought about before: Would it make sense to add some notes on how to convert an easyCerts cluster to a non-easyCerts-cluster? I'm thinking specifically of how to deal with migrating, in case we change the way bootstrapping is done in the future. We can probably postpone it till then as well, but I'm sure some users would be interested in knowing how to do it. |
@srhb Again thank you for the review. I agree with almost all your comments.
I totally wish that too. However I'm not sure that will ever be possible. First of all because I think the Kubernetes core team is happy with outsourcing most of the bootstrapping tasks. It has become the defacto job of what is sometimes referred to as (term clash warning) "k8s deployments" - i.e. kubeadm, kube-up, kubespray etc. All in all, I think we should keep a close eye at what they are discussing in kubernetes sig-auth for the future of RBAC and TLS-bootstrapping. Also, it might be necessary to align our module closer to kubeadm (or one of the other major k8s toolboxes) in the future; in order to better take advantage of the builtin kubernetes features. Take a look at: The manual doesn't build now :(because apparently |
Oh, right, I've run into this stage restriction before and I'm not sure it's actually easy to solve (I never looked into it much.) I suppose we can settle with wordy references for now ("... in the Kubernetes section of the NixOS manual...") |
Tangentially, I'm wondering if the documentation should really live in |
80819bd
to
f9f6b9e
Compare
@srhb Squashed some commits, but now the wordy reference looks like this: f9f6b9e#diff-51d78eb73f31123ee0e241af7086c3aaR135 :) Aaand the manual builds again. |
Next time, please hold off on squashing till after the review is done. :) With that said, and given a response to my question to Graham about the best location for the main kubernetes docs, I approve this PR. I would still appreciate it if one of the other k8s-savvy people did a more functionality-oriented review. @kalbasit @steveej will you still be able to make time to do that? |
Failure on aarch64-linux (full log) Attempted: kubernetes Partial log (click to expand)
|
Success on x86_64-linux (full log) Attempted: kubernetes Partial log (click to expand)
|
@GrahamcOfBorg build kubernetes |
@calbrecht @fpletz Can you make a separate PR with only the dependency changes? I feel that those are kind of a must-have have for 19.03. |
@johanot That was the plan. Definitely before 19.03. :) |
Added a fix for the docker/flannel bootstrap issue. |
9fa89dc
to
081de33
Compare
- All kubernetes components have been seperated into different files - All TLS-enabled ports have been deprecated and disabled by default - EasyCert option added to support automatic cluster PKI-bootstrap - RBAC has been enforced for all cluster components by default - NixOS kubernetes test cases make use of easyCerts to setup PKI
…lease note for NixOS 19.03
…ger bootstrap - because the kube-addon-manager drops most of its privileges after startup.
+ isolate etcd on the master node by letting it listen only on loopback + enabling kubelet on master and taint master with NoSchedule The reason for the latter is that flannel requires all nodes to be "registered" in the cluster in order to setup the cluster network. This means that the kubelet is needed even at nodes on which we don't plan to schedule anything.
…net.env The current postStart step on flannel causes flannel.service to sometimes hang, even when it's commanded to stop.
…d might fail due to dockerd restarting
Before flannel is ready there is a brief time where docker will be running with a default docker0 bridge. If kubernetes happens to spawn containers before flannel is ready, docker can't be restarted when flannel is ready because some containers are still running on the docker0 bridge with potentially different network addresses. Environment variables in `EnvironmentFile` override those defined via `Environment` in the systemd service config. Co-authored-by: Christian Albrecht <christian.albrecht@mayflower.de>
081de33
to
3a02205
Compare
Rebased on top of current master to make @GrahamcOfBorg happy. 😄 |
After i added |
There's also an issue with |
Ok, i made these changes and this seems to fix all race conditions: https://github.com/GateHubNet/nixpkgs/commit/03e3e932f73ebf5b924ed1d8e7c877fefd65f40b Edit: updated commit url |
Also single node tests do not need flannel, but this can be improved later |
@offlinehacker Awesome! Seems to work on a few of our machines. We've run the tests a couple of times. We've found some other issues while trying to address your comments that we will incorporate in the following PRs. Cherry-picked your commit. Time to merge! 🚀 Thanks to all contributors here! 🍻 |
Hi @johanot, I'm trying to use this PR to set up my developer environment. I saw in the documentation that it's possible to login as the cluster admin with the key that was generated. nixpkgs/nixos/doc/manual/configuration/kubernetes.xml Lines 114 to 125 in 2d20e8c
Would it be possible to add some documentation on how to create cert/key for a user? This is what I got so far: {
environment.systemPackages =
let
kub = config.services.kubernetes;
dev = kub.lib.mkCert {
name = "gui";
CN = "gui";
fields = {
O = "system:masters";
};
privateKeyOwner = "gui";
};
kubeConfig = kub.lib.mkKubeConfig "gui" {
server = kub.apiserverAddress;
certFile = dev.cert;
keyFile = dev.key;
};
kubectl2 = with pkgs; runCommand "wrap-kubectl" { buildInputs = [ makeWrapper ]; } ''
mkdir -p $out/bin
echo "---"
echo ${dev.cert}
echo ${dev.key}
echo "---"
echo "makeWrapper ${pkgs.kubernetes}/bin/kubectl $out/bin/kubectl --set KUBECONFIG "${kubeConfig}""
makeWrapper ${pkgs.kubernetes}/bin/kubectl $out/bin/kubectl2 --set KUBECONFIG "${kubeConfig}"
'';
in [
kubectl2
]
} However dev.cert and dev.key points to non existing files. Is this the correct approach ? |
echo "Node joined succesfully" | ||
'')]; | ||
|
||
# isolate etcd on loopback at the master node |
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.
It would be nice if easyCerts could be used in conjunction with an external etcd cluster. Now this silently disables that possibility without telling the user. We should probably document this behaviour with big fat letters somewhere
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.
@arianvp couldn't you do something like:
services.etcd.enable = pkgs.lib.mkForce false;
services.kubernetes.apiserver.etcd = {
servers = [ "some.other.server.org" ];
keyFile = "/some/key.pem";
certFile = "/some/cert.pem";
caFile = "/some/ca.pem";
};
Or am I missing some details here? (The mkForce
is due to a legacy thing with the flannel module, not easyCerts).
I don't agree with the silently disables phrasing. easyCerts is a feature you can opt-in to (or choose not to), which automates almost every part of the setup of a small basic Kubernetes cluster. It was never meant to satisfy every need, nor was it designed to make cluster customizations easy. It doesn't make customization impossible though - just less automatic. :-) I don't know if the current documentation of easyCerts needs a disclaimer of some sort, but feel free to PR both module and docs.
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.
Thanks! Makes sense.
@MasseGuillaume As far as I can see, you miss the final step of adding your certificate spec to the pki module (ultimately certmgr), which will then issue your certificates. Something along the lines of: |
@johanot Thanks I will try this and send a PR to update the documentation. Once again, thanks for your hard work! |
@johanot I got the following to work: { config, pkgs, ... }:
let
kub = config.services.kubernetes;
devCert = kub.lib.mkCert {
name = "gui";
CN = "kubernetes-cluster-ca";
fields = {
O = "system:masters";
};
privateKeyOwner = "gui";
};
kubeConfig = kub.lib.mkKubeConfig "gui" {
server = kub.apiserverAddress;
certFile = devCert.cert;
keyFile = devCert.key;
};
in
{
services = {
kubernetes = {
roles = ["master" "node"];
easyCerts = true;
masterAddress = "localhost";
pki.certs = { dev = devCert; };
};
}
programs.zsh = {
interactiveShellInit = ''
export KUBECONFIG="${kubeConfig}:$HOME/.kube/config";
'';
}
} |
Would it be possible to implement a better way of configuring kube-addon-manager without having to create kubeconfig ourselves if not using easyCert? I'm talking about pki.nix#238 (unmodified in master) - from what I can see, unless easyCert is in control of certificates, I have no choice but to call Maybe bring kubeconfig generation into the configuration inside I could be missing something obvious here, so I'm fine with being proved blind! |
Big note: Depends on #45567 getting merged for PKI bootstrapping to work as intended, and for tests to run clean.
Motivation for this change
This PR includes
The primary focus of the refactoring has been to enforce RBAC (role-based-access-control) and TLS-encryption across all cluster components. This turned out to be a cumbersome task, especially due to secret bootstrapping and exchange, which is not a trivial task. I ended up picking a solution similar to how kubeadm does cluster bootstrap. For any node to successfully join a cluster, a shared key must be manually (by an admin) copied from the master to the node, before the required node certificates can be signed and issued by the master. This is to ensure that any secrets don't end up in the store.
Secondary focus has been to make it easier to enable and disable individual cluster components independently and to make the use of
services.kubernetes.roles
more intuitive and (most importantly) optional.Unfortunately, upgrade from kube-dns to coredns did not happen as part of this PR. I suspect this will not make it to 18.09, unless somebody finds time to change and test. Similarly, the change from cmdline params to config files will have to wait to a later round.
I hope that this refactoring makes the foundation for further improvements and updates to the module in the future.
fixes #43882, fixes #43395
Thanks to @srhb for much appreciated help and advise during development of this!
@kalbasit @colemickens @steveej Can I ask you guys for review help?
I have a GCE cluster running 1 master and 2 nodes, for which you can get testing/playground access on request.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)