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.

Sign up for GitHub

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

Jump to bottom

nixos/kubernetes: allow configuring cfssl API server SANs #74094

Merged
merged 1 commit into from
Jan 19, 2020
Merged

nixos/kubernetes: allow configuring cfssl API server SANs #74094

merged 1 commit into from
Jan 19, 2020
+10 −0

Conversation

anmonteiro
Copy link
Member

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:

nixpkgs/nixos/modules/services/cluster/kubernetes/apiserver.nix

Lines 139 to 143 in a3465d9

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)

Sorry, something went wrong.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 25, 2019
@anmonteiro
Copy link
Member Author

Friendly ping @calbrecht @globin

@anmonteiro
Copy link
Member Author

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

jonringer
jonringer reviewed Jan 16, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

you're right, mostly out of my league

nixos/modules/services/cluster/kubernetes/pki.nix
@@ -20,6 +20,7 @@ let
size = 2048;
};
CN = top.masterAddress;
hosts = cfg.cfsslAPIExtraSANs;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@jonringer jonringer requested a review from infinisil January 16, 2020 23:14
@jonringer
Copy link
Contributor

jonringer commented Jan 16, 2020 •
edited
Loading

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

nixos/kubernetes: allow configuring cfssl API server SANs

infinisil
infinisil reviewed Jan 16, 2020
View reviewed changes
nixos/modules/services/cluster/kubernetes/pki.nix
description = ''
Extra x509 Subject Alternative Names to be added to the cfssl API webserver TLS cert.
'';
default = [];
Copy link
Member

Choose a reason for hiding this comment

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

An example would be nice

@anmonteiro
Copy link
Member Author

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.

@ofborg ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 18, 2020
@jonringer
Copy link
Contributor

please squash commits

@anmonteiro
Copy link
Member Author

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
jonringer
jonringer approved these changes Jan 19, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

this seems to correspond with official documentation https://kubernetes.io/docs/tasks/tls/managing-tls-in-a-cluster/#create-a-certificate-signing-request

LGTM

@jonringer
Copy link
Contributor

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

@jonringer jonringer merged commit e2c11ad into NixOS:master Jan 19, 2020
@anmonteiro anmonteiro deleted the patch-1 branch April 14, 2020 04:26
@anmonteiro anmonteiro mentioned this pull request Feb 10, 2021
Closed
10 tasks
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@infinisil infinisil

@jonringer jonringer

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@anmonteiro @jonringer @infinisil @Janik-Haag