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: allow configuring cfssl API server SANs #74094

Merged
merged 1 commit into from Jan 19, 2020

Conversation

@anmonteiro
Copy link
Member

anmonteiro commented Nov 25, 2019

Motivation for this change

services.kubernetes.easyCerts is really useful to bootstrap a Kubernetes cluster. A common use case in Kubernetes clusters is to have a DNS entry for the master so that nodes can connect to a master that may change address (e.g. in an autoscaling group).

Right now this is not easily possible because the master also runs the cfssl API server but doesn't allow registering Subject Alternative Names in its TLS cert.

This PR proposes allowing configuring the extra SANs in this certificate similarly to what's available in the API server:

extraSANs = mkOption {
description = "Extra x509 Subject Alternative Names to be added to the kubernetes apiserver tls cert.";
default = [];
type = listOf str;
};

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nix-review --run "nix-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @johanot @calbrecht (apologies if spammy, this is my first contribution to nixpkgs and you both were listed as the top committers in pki.nix)

@anmonteiro
Copy link
Member Author

anmonteiro commented Jan 9, 2020

Friendly ping @calbrecht @globin

@anmonteiro
Copy link
Member Author

anmonteiro commented Jan 16, 2020

Hey @jonringer, pinging you on this one for a review, feel free to ignore if it's too out of your depth.

Copy link
Contributor

jonringer left a comment

you're right, mostly out of my league

@@ -20,6 +20,7 @@ let
size = 2048;
};
CN = top.masterAddress;
hosts = cfg.cfsslAPIExtraSANs;

This comment has been minimized.

@jonringer

jonringer Jan 16, 2020 Contributor

would be nice to have a tests which demonstrates that this gets correctly used

@jonringer jonringer requested a review from Infinisil Jan 16, 2020
@jonringer
Copy link
Contributor

jonringer commented Jan 16, 2020

also, i think the commit/PR name should be:

nixos/kubernetes: allow configuring cfssl API server SANs 
description = ''
Extra x509 Subject Alternative Names to be added to the cfssl API webserver TLS cert.
'';
default = [];

This comment has been minimized.

@Infinisil

Infinisil Jan 16, 2020 Member

An example would be nice

@anmonteiro anmonteiro force-pushed the anmonteiro:patch-1 branch from c9b6fe0 to 273b1fc Jan 18, 2020
@anmonteiro
Copy link
Member Author

anmonteiro commented Jan 18, 2020

Thanks for the initial review. I did the following things:

  • rebased on current master
  • changed the commit message as per @jonringer's review
  • added an example as per @Infinisil 's suggestion

I can look into adding a test next, but it might take me a while to do that.

@jonringer
Copy link
Contributor

jonringer commented Jan 18, 2020

please squash commits

@anmonteiro anmonteiro force-pushed the anmonteiro:patch-1 branch from 273b1fc to 96e5619 Jan 18, 2020
@anmonteiro
Copy link
Member Author

anmonteiro commented Jan 18, 2020

Done

@anmonteiro anmonteiro changed the title Kubernetes cfssl: Allow configuring cfssl API server SANs nixos/kubernetes: allow configuring cfssl API server SANs Jan 18, 2020
Copy link
Contributor

jonringer left a comment

@jonringer
Copy link
Contributor

jonringer commented Jan 19, 2020

@GrahamcOfBorg test kubernetes.dns
@GrahamcOfBorg test kubernetes.rbac

@jonringer jonringer merged commit e2c11ad into NixOS:master Jan 19, 2020
17 checks passed
17 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
tests.kubernetes.dns on aarch64-linux Success
Details
tests.kubernetes.dns on x86_64-linux Success
Details
tests.kubernetes.rbac on aarch64-linux Success
Details
tests.kubernetes.rbac on x86_64-linux Success
Details
@anmonteiro anmonteiro deleted the anmonteiro:patch-1 branch Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.