Skip to content
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

Merged
merged 9 commits into from Feb 20, 2019
Merged

Conversation

johanot
Copy link
Contributor

@johanot johanot commented Aug 27, 2018

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
  1. major refactoring of the nixos/kubernetes module.
  2. package upgrade of kubernetes: 1.10.5 -> 1.11.2
  3. manual chapter for kubernetes and release notes

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: kubernetes

Partial log (click to expand)

strip is /nix/store/y4ymnvgxygpq05h03kyzbj572zmh6zla-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/x21r9m4rdzp5pn17l8x144gnfi9hwc0s-kubernetes-1.11.2-man
checking for references to /build in /nix/store/x21r9m4rdzp5pn17l8x144gnfi9hwc0s-kubernetes-1.11.2-man...
shrinking RPATHs of ELF executables and libraries in /nix/store/m9h4y077dnrfkj0axbqq4dxhcqbbds5z-kubernetes-1.11.2-pause
shrinking /nix/store/m9h4y077dnrfkj0axbqq4dxhcqbbds5z-kubernetes-1.11.2-pause/bin/pause
strip is /nix/store/y4ymnvgxygpq05h03kyzbj572zmh6zla-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/m9h4y077dnrfkj0axbqq4dxhcqbbds5z-kubernetes-1.11.2-pause/bin
patching script interpreter paths in /nix/store/m9h4y077dnrfkj0axbqq4dxhcqbbds5z-kubernetes-1.11.2-pause
checking for references to /build in /nix/store/m9h4y077dnrfkj0axbqq4dxhcqbbds5z-kubernetes-1.11.2-pause...
/nix/store/aqd31bgwzwkpwib3ib9hfympzhcj5bx7-kubernetes-1.11.2

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: kubernetes

Partial log (click to expand)

strip is /nix/store/h0lbngpv6ln56hjj59i6l77vxq25flbz-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/iddclpp6ck5g825in0dh30kavj2ddmrc-kubernetes-1.11.2-man
checking for references to /build in /nix/store/iddclpp6ck5g825in0dh30kavj2ddmrc-kubernetes-1.11.2-man...
shrinking RPATHs of ELF executables and libraries in /nix/store/7vzyfsgpw4yjkr53lp9bcyz97s0p2s2f-kubernetes-1.11.2-pause
shrinking /nix/store/7vzyfsgpw4yjkr53lp9bcyz97s0p2s2f-kubernetes-1.11.2-pause/bin/pause
strip is /nix/store/h0lbngpv6ln56hjj59i6l77vxq25flbz-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/7vzyfsgpw4yjkr53lp9bcyz97s0p2s2f-kubernetes-1.11.2-pause/bin
patching script interpreter paths in /nix/store/7vzyfsgpw4yjkr53lp9bcyz97s0p2s2f-kubernetes-1.11.2-pause
checking for references to /build in /nix/store/7vzyfsgpw4yjkr53lp9bcyz97s0p2s2f-kubernetes-1.11.2-pause...
/nix/store/jsrwkfbcaqgy7l0njixm00xv5n7mcxb2-kubernetes-1.11.2

@kalbasit
Copy link
Member

@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!

@steveej
Copy link
Contributor

steveej commented Aug 28, 2018

@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.

@johanot
Copy link
Contributor Author

johanot commented Aug 29, 2018

@steveej Thanks. Yeah, If I were you, I would concentrate the effort on pki.nix, which handles all of the TLS-stuff. Much of what happens in all the cluster component files haven't changed that much. Just me polluting the diff, because I moved a lot of code around. Sorry about that.

@kristoff3r
Copy link
Contributor

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.

@kristoff3r
Copy link
Contributor

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.

@johanot
Copy link
Contributor Author

johanot commented Aug 31, 2018

@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 :)

@kristoff3r
Copy link
Contributor

@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.

@grahamc
Copy link
Member

grahamc commented Sep 2, 2018

cc @srhb for a review please!

@srhb
Copy link
Contributor

srhb commented Sep 2, 2018

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'm very happy about the documentation, especially coupled with the improvements to the roles setup, which now seems robust and secure. Thanks for this!

  • I'm very happy that roles now works better than ever before!

  • I wish we could reduce the number of options. I think it's OK to accept this right now, especially with a push for 18.09, but it should be a priority to try and reduce the number and simplify the module in general in the future (we've already talked about this.)

  • I wish we could rely on mechanisms internal to k8s for bootstrapping. Maybe in the future...

  • I would like some in-code comments about how mkCert works, if possible. :)

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
Copy link
Contributor

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.";
Copy link
Contributor

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.";
Copy link
Contributor

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.";
Copy link
Contributor

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";
Copy link
Contributor

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

nixos/tests/kubernetes/base.nix Outdated Show resolved Hide resolved
@srhb
Copy link
Contributor

srhb commented Sep 2, 2018

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.

@johanot
Copy link
Contributor Author

johanot commented Sep 3, 2018

@srhb Again thank you for the review. I agree with almost all your comments.

I wish we could rely on mechanisms internal to k8s for bootstrapping. Maybe in the future...

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.
Secondly, because most of what is in Kubernetes core already - in terms of security bootstrapping - is based cluster-components running in-cluster as daemonsets. This is not the approach we've chosen so far with the NixOS kubernetes module.

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:
f4a9ba3
I think you'll find that most of the doc-related comments can be resolved based on those changed.
however;

The manual doesn't build now :(

because apparently <xref linkend="sec-kubernetes"/> is not the way to link to a manual section from a module option description. Please help.

@srhb
Copy link
Contributor

srhb commented Sep 3, 2018

The manual doesn't build now :(

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...")

@srhb
Copy link
Contributor

srhb commented Sep 3, 2018

Tangentially, I'm wondering if the documentation should really live in nixos/modules/services/cluster/kubernetes/kubernetes.xml and be referenced in meta.doc from its default.nix. @grahamc ?

@johanot johanot force-pushed the kubernetes-1.11 branch 2 times, most recently from 80819bd to f9f6b9e Compare September 3, 2018 15:07
@johanot
Copy link
Contributor Author

johanot commented Sep 3, 2018

@srhb Squashed some commits, but now the wordy reference looks like this: f9f6b9e#diff-51d78eb73f31123ee0e241af7086c3aaR135 :) Aaand the manual builds again.

@srhb
Copy link
Contributor

srhb commented Sep 3, 2018

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?

@GrahamcOfBorg
Copy link

Failure on aarch64-linux (full log)

Attempted: kubernetes

Partial log (click to expand)

Building Go cmd/dist using /nix/store/29gizw0x73zc6zwgpxsiqcgid7sq57wx-go-bootstrap/share/go.
# _/nix/store/mm8wbc2ybpzy7azlxylv3wsl0c65hj6k-go-1.11/share/go/src/cmd/dist
2018/09/03 13:02:02 Invalid GOARM value. Must be 5, 6, or 7.
builder for '/nix/store/w0i02n2imx2wgifv6nqmhbhgz07fvp24-go-1.11.drv' failed with exit code 2
cannot build derivation '/nix/store/mwfnrw7y2cdqx46z4b5qkcyw81nd71wl-govers-20150109-3b5f175.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/yqxmpdwxzk6z9pzgbz6r86j0cizdkycn-go-bindata-20151023-a0ff256.drv': 2 dependencies couldn't be built
100 23.6M    0 23.6M    0     0  4186k      0 --:--:--  0:00:05 --:--:-- 5586k
unpacking source archive /build/v1.11.2.tar.gz
cannot build derivation '/nix/store/ar0f473jpx7p8zbcnvmxpjk262nd645g-kubernetes-1.11.2.drv': 1 dependencies couldn't be built
error: build of '/nix/store/ar0f473jpx7p8zbcnvmxpjk262nd645g-kubernetes-1.11.2.drv' failed

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: kubernetes

Partial log (click to expand)

strip is /nix/store/h0lbngpv6ln56hjj59i6l77vxq25flbz-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/spmzs6mabcl0dpj1kg5h7dq39dsif33g-kubernetes-1.11.2-man
checking for references to /build in /nix/store/spmzs6mabcl0dpj1kg5h7dq39dsif33g-kubernetes-1.11.2-man...
shrinking RPATHs of ELF executables and libraries in /nix/store/8ycmsi4f5al0x8l0cqlpkz03qkgmbifz-kubernetes-1.11.2-pause
shrinking /nix/store/8ycmsi4f5al0x8l0cqlpkz03qkgmbifz-kubernetes-1.11.2-pause/bin/pause
strip is /nix/store/h0lbngpv6ln56hjj59i6l77vxq25flbz-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/8ycmsi4f5al0x8l0cqlpkz03qkgmbifz-kubernetes-1.11.2-pause/bin
patching script interpreter paths in /nix/store/8ycmsi4f5al0x8l0cqlpkz03qkgmbifz-kubernetes-1.11.2-pause
checking for references to /build in /nix/store/8ycmsi4f5al0x8l0cqlpkz03qkgmbifz-kubernetes-1.11.2-pause...
/nix/store/zax3yglm2x8jnc0a4syxmpjkgc8ddkln-kubernetes-1.11.2

@srhb
Copy link
Contributor

srhb commented Sep 5, 2018

@GrahamcOfBorg build kubernetes

@johanot
Copy link
Contributor Author

johanot commented Feb 20, 2019

@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.

@fpletz
Copy link
Member

fpletz commented Feb 20, 2019

@johanot That was the plan. Definitely before 19.03. :)

@fpletz
Copy link
Member

fpletz commented Feb 20, 2019

Added a fix for the docker/flannel bootstrap issue.

Johan Thomsen and others added 8 commits February 20, 2019 21:08
- 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
…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.
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>
@fpletz
Copy link
Member

fpletz commented Feb 20, 2019

Rebased on top of current master to make @GrahamcOfBorg happy. 😄

@offlinehacker
Copy link
Contributor

After i added Restart="on-failure"; to kubelet it works fine, @fpletz can you add auto restart on failure to kubelet, as this is what you usually want? We also have same on docker.

@offlinehacker
Copy link
Contributor

There's also an issue with kubelet-boostrap failing during tests due docker restarting in between load, do you have any idea how to fix this?

@offlinehacker
Copy link
Contributor

offlinehacker commented Feb 20, 2019

Ok, i made these changes and this seems to fix all race conditions: https://github.com/GateHubNet/nixpkgs/commit/03e3e932f73ebf5b924ed1d8e7c877fefd65f40b

Edit: updated commit url

@offlinehacker
Copy link
Contributor

Also single node tests do not need flannel, but this can be improved later

@fpletz
Copy link
Member

fpletz commented Feb 20, 2019

@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! 🍻

@fpletz fpletz merged commit 2935a67 into NixOS:master Feb 20, 2019
@MasseGuillaume
Copy link
Contributor

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.

<para>
In order to interact with an RBAC-enabled cluster as an administrator, one
needs to have cluster-admin privileges. By default, when easyCerts is
enabled, a cluster-admin kubeconfig file is generated and linked into
<literal>/etc/kubernetes/cluster-admin.kubeconfig</literal> as determined by
<xref linkend="opt-services.kubernetes.pki.etcClusterAdminKubeconfig"/>.
<literal>export KUBECONFIG=/etc/kubernetes/cluster-admin.kubeconfig</literal>
will make kubectl use this kubeconfig to access and authenticate the cluster.
The cluster-admin kubeconfig references an auto-generated keypair owned by
root. Thus, only root on the kubernetes master may obtain cluster-admin
rights by means of this file.
</para>

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
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Makes sense.

@johanot
Copy link
Contributor Author

johanot commented Mar 4, 2019

@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: config.services.kubernetes.pki.certs = { inherit dev; }. Yeah we should perhaps extend the docs to cover how to add extra custom certs.

@MasseGuillaume
Copy link
Contributor

@johanot Thanks I will try this and send a PR to update the documentation. Once again, thanks for your hard work!

@MasseGuillaume
Copy link
Contributor

@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";
    '';
  }
}

@duckfullstop
Copy link
Contributor

duckfullstop commented Apr 4, 2019

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 services.kubernetes.lib.mkKubeConfig myself with my own certificate paths.

Maybe bring kubeconfig generation into the configuration inside services.kubernetes.addonManager.*, ala the other modules? (i.e services.kubernetes.addonManager.keyFile, etc.)

I could be missing something obvious here, so I'm fine with being proved blind!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kubernetes 1.10 -> 1.11 Kubernetes 1.10.5 is unusable, auth is not working