nixos/kubernetes: improvements #41884

johanot · 2018-06-12T10:42:00Z

Motivation for this change

Wanted to do some minor fixups and cleanups of the module.

APIServer Addresses
Most notably I have changed the apiserver address options to conform better with the kube-apiserver cmdline args. kube-apiserver has "bindAddress" which selects which interfaces to listen on, and the "advertiseAddress" which is what other parties are expected to use to contact the apiserver. Ref: https://kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/

Thus, I renamed "address" to "bindAddress" and put in default="0.0.0.0" - also default as per kubernetes docs.

As well I removed hardcoding of the implementation bind address, which was done already by #41018. Unfortunately this PR caused the kubernetes tests to break, since the "address" options was used for multiple purposes.

Configurable images for addons
It is now possible to change the docker images seeded for KubeDNS and Kubernetes Dashboard. Unfortunately, it is not possible to spawn containers by reference to the imageDigest, due to image digests changing after docker load: moby/moby#22011. Thanks especially to @srhb for help on this one.

Custom directory for CNI config
You can now specify cni.configDir and override the existing behavior of autogenerating CNI-config files at build time. Not only Kubernetes use CNI, and therefore this is useful if you have other ways of assembling your superset of CNI config files.

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)

/kubernetes/dns.nix and /kubernetes/rbac.nix

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/)
Fits CONTRIBUTING.md.

srhb · 2018-06-12T10:51:54Z

I have no real opinion on the cni config parts, so I will leave that for someone else to comment on. I can imagine that it might be useful.

I am in favor of the rest of the changes. I think it is best to remove the options, as was done, to eliminate confusion from the earlier mismatches.

I also confirm that the tests build and the address changes fix them.

Please note that ofborg has some comments that need to be addressed: https://gist.github.com/GrahamcOfBorg/6a730ee2359ad80fa119bb69b71da1d3

johanot · 2018-06-12T11:32:25Z

ofborg is happy now. @steveej Perhaps you'd like to review this?

srhb · 2018-06-12T12:15:11Z

@GrahamcOfBorg test kubernetes.dns.singlenode kubernetes.dns.multinode kubernetes.rbac.singlenode kubernetes.rbac.multinode

GrahamcOfBorg · 2018-06-12T12:15:30Z

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: tests.kubernetes.dns.singlenode, tests.kubernetes.dns.multinode, tests.kubernetes.rbac.singlenode, tests.kubernetes.rbac.multinode

Partial log (click to expand)


Cannot nix-instantiate `tests.kubernetes.dns.multinode' because:
error: attribute 'multinode' in selection path 'tests.kubernetes.dns.multinode' not found

Cannot nix-instantiate `tests.kubernetes.rbac.singlenode' because:
error: attribute 'singlenode' in selection path 'tests.kubernetes.rbac.singlenode' not found

Cannot nix-instantiate `tests.kubernetes.rbac.multinode' because:
error: attribute 'multinode' in selection path 'tests.kubernetes.rbac.multinode' not found

GrahamcOfBorg · 2018-06-12T12:28:24Z

Failure on x86_64-linux (full log)

Attempted: tests.kubernetes.dns.singlenode, tests.kubernetes.dns.multinode, tests.kubernetes.rbac.singlenode, tests.kubernetes.rbac.multinode

Partial log (click to expand)

machine1# [  616.304952] etcd[738]: read-only range request "key:\"/registry/services/endpoints/kube-system/kube-controller-manager\" " took too long (334.078493ms) to execute
machine1: exit status 1
machine1: output: ;; connection timed out; no servers could be reached
error: command `host redis.default.svc.cluster.local' did not succeed (exit code 1)
command `host redis.default.svc.cluster.local' did not succeed (exit code 1)
cleaning up
killing machine1 (pid 593)
vde_switch: EOF on stdin, cleaning up and exiting
builder for '/nix/store/gjjj6qjx8f47d0lz8pbk9w3iaj2xgqvc-vm-test-run-kubernetes-dns-singlenode.drv' failed with exit code 255
error: build of '/nix/store/gjjj6qjx8f47d0lz8pbk9w3iaj2xgqvc-vm-test-run-kubernetes-dns-singlenode.drv', '/nix/store/pd93zr4gkkwq7dwkihpny1vkvlf23bdp-vm-test-run-kubernetes-rbac-multinode.drv' failed

srhb · 2018-06-12T12:29:03Z

Oh well, I guess the tests are a bit too large to run on ofborg. Again, they do pass locally.

steveej

One remark about the bindAddress/advertiseAddress combination as a comment.

In theory having a configurable CNI directory might come in handy for testing; although because I can't think of a use-case right now where this is better than changing the verbatim config in the expressions. Could you name one such example?
While we are discussing CNI, how do we handle --cni-bin-dir?

steveej · 2018-06-12T12:22:27Z

nixos/modules/services/cluster/kubernetes/default.nix

@@ -265,7 +266,7 @@ in {
          to members of the cluster. This address must be reachable by the rest
          of the cluster.
        '';
-        default = null;
+        default = cfg.apiserver.bindAddress;


I wonder what happens if "0.0.0.0" is advertised. Does that make sense to be the default?

I thought about that myself, however I think it is the correct way to go about it, since this is what the apiserver falls back to itself, according to the docs: https://kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/ .

--advertise-address ip The IP address on which to advertise the apiserver to members of the cluster. This address must be reachable by the rest of the cluster. If blank, the --bind-address will be used. If --bind-address is unspecified, the host's default interface will be used.

... but perhaps it's better to let the apiserver itself to the fallback through 1. bind-address, 2. host-address. Thus making it default = null and implementing with optionalString cfg.advertiseAddress --advertise-address cfg.advertiseAddress. Then however, we would need to not use cfg.advertiseAddress in the kubelet config.

.. a third option is therefore, don't set any default value for the advertiseAddress and require it to be configured? This is the best alternative I can think of.

Of course all of this boils down to: How plug'n'play do we want the module to be?

Since advertising a service at 0.0.0.0 is practically impossible I was curious about what IP is advertised to cluster members if this is specified or as your quote says if fallen back to.

Someone had already done the code digging: kubernetes/kubernetes#58686 (comment)

If not set or unspecified it will default to the secure external interface

0.0.0.0 is considered unspecified

I don't know if we can easily reproduce secure.DefaultExternalAddress() which is part of the fallback mechanism. To me it seems sensible to leave the default at null and thereby let the kube-apiserver figure it out, while providing the option to set it if needed. Do you see any difficulty with that option?

Of course someone already digged into that! 👍 :)

I'm ok with that solution, only problem I see is this line, which uses the advertiseAddress: https://github.com/johanot/nixpkgs/blob/k8s-improvements/nixos/modules/services/cluster/kubernetes/default.nix#L76

If we let kube-apiserver decide advertiseAddress at runtime, we cannot sanely set a default value of the kubeconfig server option either ? Can we ?

Would if cfg.apiserver.advertiseAddress != null then cfg.apiserver.advertiseAddress else "127.0.0.1
work? It seems to be a sane assumption that the kubeconfig is generated for the clusternode itself. If the user needs anything different then she needs to fill in address information manually anyway.

mkKubeConfigOptions is used for all kubelets in the cluster. For any non-master node in a multi-node cluster, 127.0.0.1 will not work. I think however that it's good enough to set as default value. At least it will work for a singlenode cluster and for components co-located with the apiserver. Will add the change to the PR now.

Reminds me that, we better put some work into deprecating those insecure ports... Support for them might be completely gone in v1.11.. Well... Next PR.

johanot · 2018-06-12T13:17:28Z

@steveej Regarding the CNI changes: The usecase at my dayjob is that we run kube-router, which (currently) requires write-access to the CNI-config as it uses the standard host-local CNI plugin and updates CIDR assignments etc. directly in the CNI-config file. For this reason, I want my CNI config to live in /run and not be a derivation.

To make it a bit more general. The number of available CNI plugins goes up over time, and I don't think CNI should be limited in such way that it can only be controlled via the k8s module. I think CNI and k8s are two seperate things - like k8s and docker. So in the future I think we should work towards decoupling kubernetes <-> CNI <-> container-runtime (docker). Many different modules might want access to the same CNI-config and binaries, so I think it should be a free choice how to manage those.

I don't like the current handling of the CNI-bin either. In my opinion, the config option should just be a path. A user could then choose whether the bin target should be a derivation or something else.

coretemp · 2018-06-12T15:13:54Z

nixos/modules/services/cluster/kubernetes/default.nix

+      (pkgs.buildEnv {
+        name = "kubernetes-cni-config";
+        paths = imap (i: entry:
+          pkgs.writeTextDir "${toString (10+i)}-${entry.type}.conf" (builtins.toJSON entry)


Can we add something like autogenerated_by_NixOS_module to the filename? (Also fine for a separate PR)

I don't know what other modules do when writing files to the store? Is it standard practice to write such header in all NixOS module generated files? In any case, yes, let's consider it for a separate PR :)

I don't think it's standard practice, but I do think it's a good idea regardless of what is currently being done in nixpkgs (nixpkgs is not some kind of designed engineering perfection, so in a sense it doesn't really matter what other modules do). You could be first to do things right! Imagine that!

Separate PR is fine.

The use-case of something like this would be to ideally even include a clickable link in the generated files themselves too such that if you open them you can immediately jump to the defining source in case you want to make modifications.

(I don't speak on behalf of the NixOS organisation, so whether or not you listen to me is completely optional. The only effect is that the number of future users of this module would likely be one less if it diverges too far from our our goals. )

coretemp · 2018-06-12T15:16:44Z

nixos/modules/services/cluster/kubernetes/dns.nix

+      description = "Docker image to seed for the kube-dns main container.";
+      type = types.attrs;
+      default = {
+        imageName = "k8s.gcr.io/k8s-dns-kube-dns-amd64";


Does k8s run on ARM? Probably out of scope for this PR otherwise.

Haven't tested on other archs. At least now you can configure another image íf you want to :) Before this change, the image details were hardcoded.

- Added option 'cni.configDir' to allow for having CNI config outside of nix-store Existing behavior (writing verbatim CNI conf-files to nix-store) is still available. - Removed unused option 'apiserver.publicAddress' and changed 'apiserver.address' to 'bindAddress' This conforms better to k8s docs and removes existing --bind-address hardcoding to 0.0.0.0 - Fixed c/p mistake in apiserver systemd unit description - Updated 18.09 release notes to reflect changes to existing options And fixed some typos from previous PR - Make docker images for Kubernetes Dashboard and kube-dns configurable

steveej

After addressing the other reviewer's comments this should be good to go. Thanks for the work to improve our k8s experience 👍

srhb · 2018-06-13T08:48:03Z

Cool, I'll merge once @coretemp chimes in.

coretemp · 2018-06-13T12:29:45Z

It's certainly good enough to be merged.

GrahamcOfBorg added 6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) labels Jun 12, 2018

johanot force-pushed the k8s-improvements branch from a0b1298 to bcb948c Compare June 12, 2018 11:10

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jun 12, 2018

srhb self-assigned this Jun 12, 2018

steveej reviewed Jun 12, 2018

View reviewed changes

johanot mentioned this pull request Jun 12, 2018

kubernetes: 1.10.3 -> 1.10.4 #41888

Merged

8 tasks

coretemp reviewed Jun 12, 2018

View reviewed changes

johanot force-pushed the k8s-improvements branch from bcb948c to 8d7ea96 Compare June 12, 2018 20:49

steveej approved these changes Jun 13, 2018

View reviewed changes

globin approved these changes Jun 13, 2018

View reviewed changes

coretemp approved these changes Jun 13, 2018

View reviewed changes

srhb merged commit 2ebadc4 into NixOS:master Jun 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/kubernetes: improvements #41884

nixos/kubernetes: improvements #41884

johanot commented Jun 12, 2018

srhb commented Jun 12, 2018

johanot commented Jun 12, 2018

srhb commented Jun 12, 2018

GrahamcOfBorg commented Jun 12, 2018

GrahamcOfBorg commented Jun 12, 2018

srhb commented Jun 12, 2018

steveej left a comment

steveej Jun 12, 2018

johanot Jun 12, 2018

steveej Jun 12, 2018

johanot Jun 12, 2018

steveej Jun 12, 2018

johanot Jun 12, 2018

johanot Jun 12, 2018

johanot commented Jun 12, 2018

coretemp Jun 12, 2018

johanot Jun 13, 2018

coretemp Jun 13, 2018

coretemp Jun 12, 2018

johanot Jun 12, 2018

steveej left a comment

srhb commented Jun 13, 2018

coretemp commented Jun 13, 2018

nixos/kubernetes: improvements #41884

nixos/kubernetes: improvements #41884

Conversation

johanot commented Jun 12, 2018

Motivation for this change

Things done

srhb commented Jun 12, 2018

johanot commented Jun 12, 2018

srhb commented Jun 12, 2018

GrahamcOfBorg commented Jun 12, 2018

GrahamcOfBorg commented Jun 12, 2018

srhb commented Jun 12, 2018

steveej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanot commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveej left a comment

Choose a reason for hiding this comment

srhb commented Jun 13, 2018

coretemp commented Jun 13, 2018