nixos/keystone: init at liberty version #20905

nlewo · 2016-12-04T21:56:49Z

Motivation for this change

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
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.

This commit introduces a nixos module for the Openstack Keystone
service. It also provides a optional bootstrap step that creates some
basic initial resources (tenants, endpoints,...).

The provided test starts Keystone by enabling bootstrapping and checks
if user creation works well.

This commit is based on initial works made by domenkozar.

mention-bot · 2016-12-04T21:56:51Z

@nlewo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @joachifm and @offlinehacker to be potential reviewers.

spinus · 2016-12-04T22:34:40Z

nixos/modules/virtualisation/keystone.nix

+    policy_file=${cfg.package}/etc/policy.json
+
+    [database]
+    connection = mysql://${cfg.database.user}:${cfg.database.password}@${cfg.database.host}/${cfg.database.name}


I'm not sure, but maybe would be better to move all this config line as parameter instead of splitting it into smaller parts? For example keystone supports postgresql as well, in such a case user could just paste all the connection string.

In the description field there could be a link to keystone documentation. there is also example field in mkOption so you could provide one or two examples.

And probably this could be a "secret".

spinus · 2016-12-04T22:37:43Z

nixos/modules/module-list.nix

@@ -629,4 +629,5 @@
  ./virtualisation/vmware-guest.nix
  ./virtualisation/xen-dom0.nix
  ./virtualisation/xe-guest-utilities.nix
+  ./virtualisation/keystone.nix


IMHO, I think keystone is not about virtualization at all.
My preference would be to either:

move separate services to categories that match what they do

create sub-category in virtualisation for open stack and keep everything there, i.e.

virtualisation.openstack = { keystone.enable=true; };

My preference is your point 2. Moreover, this is already the openstack nix packages layout.

spinus · 2016-12-04T22:39:14Z

nixos/modules/virtualisation/keystone.nix

+        default = false;
+        type = types.bool;
+        description = ''
+          Bootstrap the Keystone service by creating the service


Would be nice to mention if the bootstrap will be done only first time or each time we deploy. I would expect that if I have bootstrap = true, the bootstrap will be done only one time but I'm not sure what is the behaviour from this description.

spinus · 2016-12-04T22:43:07Z

nixos/modules/virtualisation/keystone.nix

+        type = types.str;
+        default = "localhost";
+        description = ''
+          The address of the public identity endpoint.


Option name says endpoint and description says address. I'm probably biased by when I think about endpoint I have URL/URI in my mind (http://example.org:25/path, tcp://127.0.0.5), when I think of address I have in my mind IP:PORT combination, eventually DNS:PORT. Would be nice to use explain better in the description what values are allowed.

I changed this option a little bit to be more explicit.

spinus · 2016-12-04T22:45:41Z

nixos/modules/virtualisation/keystone.nix

+    # systems!
+    virtualisation.keystone.package = mkDefault pkgs.keystone;
+
+    services.mysql.enable = mkDefault (cfg.database.host == "localhost");


Please don't :-)
This can be done on higher level. This can be surprise (also it was not mentioned in the description), maybe somebody would like to use postgres.

spinus · 2016-12-04T22:46:09Z

nixos/modules/virtualisation/keystone.nix

+      gid = config.ids.gids.keystone;
+    }];
+
+    systemd.services.keystone-all = {


why not just keystone?

The binary name is keystone-all while the binary keystone is the client. For me, it's more clear to use keystone-all since I know this is the keystone server. This is a respond to the "why" and not a respond at the question "what is the best" ;)

spinus · 2016-12-04T22:47:31Z

nixos/modules/virtualisation/keystone.nix

+        path = [ cfg.package pkgs.mysql pkgs.curl pkgs.pythonPackages.keystoneclient pkgs.gawk ];
+        wantedBy = [ "multi-user.target" ];
+        preStart = ''
+          mkdir -m 755 -p /var/lib/keystone


optionally you could have "work-dir" option as some other modules

This could be addressed later without any breaking changes I think.

spinus · 2016-12-04T22:50:13Z

nixos/modules/virtualisation/keystone.nix

+	postStart = optionalString cfg.bootstrap.enable ''
+	    # Wait until the keystone is available for use
+            count=0
+            while ! curl -s  http://localhost:35357/v2.0 > /dev/null 


Probably the port could be configurable (actually I'm not sure actually if it is related to endpointPublic or not). There was no other parameter to specify listening address.

You need "--fail" option

-f, --fail (HTTP) Fail silently (no output at all) on server errors. This is mostly done to bet‐ ter enable scripts etc to better deal with failed attempts. In normal cases when an HTTP server fails to deliver a document, it returns an HTML document stating so (which often also describes why and more). This flag will prevent curl from outputting that and return error 22. This method is not fail-safe and there are occasions where non-successful response codes will slip through, especially when authentication is involved (response codes 401 and 407).

Done. Thanks.

spinus · 2016-12-04T22:54:04Z

nixos/modules/virtualisation/keystone.nix

+          # Set up the keystone's PKI infrastructure
+          ${cfg.package}/bin/keystone-manage --config-file=${keystoneConf} pki_setup --keystone-user keystone --keystone-group keystone
+        '';
+	postStart = optionalString cfg.bootstrap.enable ''


you may consider "set -eu" to make script more robust (it will break script when there is an error or when variable is not set but used)

spinus · 2016-12-04T22:57:13Z

nixos/tests/keystone.nix

@@ -0,0 +1,51 @@
+{ system ? builtins.currentSystem }:


very nice smoketest!

spinus · 2016-12-05T23:21:23Z

@nlewo thanks for making those changes.
The only thing now which I see problematic are the credentials in the nix store rather than in files. Other stuff looks good to me.

This commit introduces a nixos module for the Openstack Keystone service. It also provides a optional bootstrap step that creates some basic initial resources (tenants, endpoints,...). The provided test starts Keystone by enabling bootstrapping and checks if user creation works well. This commit is based on initial works made by domenkozar.

spinus · 2016-12-11T01:42:11Z

nixos/modules/virtualisation/openstack/common.nix

+  # can be stored in the nix store, or not. A pattern is written in
+  # the nix store to represent the secret. The pattern can
+  # then be overwritten with the value of the secret at runtime.
+  mkSecretOption = {name, description ? ""}:


oh boy, you really like those nix store passwords, don't you :-)

Anyway, if files are working I'm ok with it.

We could test if the password contains '/', then use '|' as separator, and so on...

I'm not sure it's worth, even if we fix it here, some values might break the application config files itself (this is probably application specific).

Ah yes, exactly...

spinus · 2016-12-11T01:43:52Z

nixos/modules/virtualisation/openstack/common.nix

+            default = "##${name}##";
+            description = "The pattern that represent the secret.";
+            };
+          storage = mkOption {


if you add

default = "fromFile";

I'll be more than happy.

I guess this is not working. If you set this default, I guess you can not use fromNixStore anymore.
But anyway, the user have to declare these variable, and I think the name is enough explicit to inform the user its password will be store in the nix store or not.

If you set default, yes, you can use other values as well. Just without explicit set, the default is taken, that's it.
If you think that explicit decision here is better, fair enough.

spinus · 2016-12-11T01:45:37Z

nixos/modules/virtualisation/openstack/common.nix

+  # A shell script string help to replace at runtime in a file the
+  # pattern of a secret by its value.
+  replaceSecret = secretOption: filename: ''
+    sed -i "s/${secretOption.pattern}/${getSecret secretOption}/g" ${filename}


This can break if password contains /, but I don't have better idea what to do with it now.

;)
In fact, I would like to have an "easy to use" mode. If someone wants to test the module, or deploy a dev environment, he doesn't have to handle password file and manually manage its password.

spinus · 2016-12-11T01:47:04Z

Thanks for adding "passwords from files" feature.

nlewo · 2016-12-11T07:42:10Z

It is strange there is not a common mechanism to handle these passwords in nixos. In fact, I'm not really fan of the way proposed here but it does the job in the context of this PR. I think we could open a topic to implement a clean and common way to do this. I haven't time on this for now.

Moreover, I tryied complex options definitions and submodule and either option type seems to be fragile, especially with default values.

spinus · 2016-12-11T11:15:32Z

@nlewo It's not strange, the discussion is still going I think (check one of the first issues on github about private nix store entries), on the other front, recently, the security team is forming, all this stuff is slowly happening.

Good stuff anyway, I think this module is very nice start. Probably you need to grab someone now who has merge permissions to review and merge.

nlewo · 2016-12-11T21:15:18Z

@spinus Thanks for your reviews and comments.
If you know one of them, do not hesitate;)
Otherwise, I'll ping them tomorrow on IRC.

nlewo · 2016-12-13T10:24:29Z

@domenkozar do you know who could I ping to review and merge this PR? Maybe @FRidh? (sorry for the spam)

Mic92 · 2016-12-15T10:51:55Z

nixos/modules/virtualisation/openstack/keystone.nix

    policy_file=${cfg.package}/etc/policy.json

    [database]
-    connection = ${cfg.databaseConnection}
+
+    connection = "mysql://${cfg.database.user}:${cfg.database.password.pattern}@${cfg.database.host}/${cfg.database.name}"


is hard coding mysql:// wanted here?

I didn't test it with another database, so I prefer to let it hardcoded for now. If another backend is required and tested, we could easily add it by preserving backward compatibility.

Mic92 · 2016-12-15T10:55:52Z

nixos/tests/keystone.nix

@@ -0,0 +1,79 @@
+{ system ? builtins.currentSystem }:


this test should be referenced in nixos/release.nix, otherwise it won't be automatically run.

I pushed a new commit related to your comment... but I'm really not sure it's correct.

do you want set yourself as maintainer for this test, so you get informed in case it fails?

meta = with pkgs.stdenv.lib.maintainers; { maintainers = [ ...]; };

A secret can be stored in a file. It is written at runtime in the configuration file. Note it is also possible to write them in the nix store for dev purposes.

Mic92 · 2016-12-16T19:53:38Z

Thanks!

nlewo · 2016-12-16T21:17:08Z

@Mic92 @spinus thanks for your reviews!

nlewo mentioned this pull request Dec 4, 2016

Openstack modules #19529

Closed

spinus suggested changes Dec 4, 2016

View reviewed changes

nlewo force-pushed the pr/keystone branch from 7d23568 to d9e83ec Compare December 5, 2016 08:40

nlewo force-pushed the pr/keystone branch from 5ce66d5 to 61d1839 Compare December 10, 2016 22:29

spinus approved these changes Dec 11, 2016

View reviewed changes

Mic92 approved these changes Dec 15, 2016

View reviewed changes

Mic92 reviewed Dec 15, 2016

View reviewed changes

nlewo added 2 commits December 15, 2016 20:58

nixos/keystone: secrets can be read from files

54c0bf6

A secret can be stored in a file. It is written at runtime in the configuration file. Note it is also possible to write them in the nix store for dev purposes.

nixos/keystone: add test in release.nix

cdd4b0f

nlewo force-pushed the pr/keystone branch from 564c571 to cdd4b0f Compare December 15, 2016 19:59

Mic92 merged commit 086e5d0 into NixOS:master Dec 16, 2016

nixos/keystone: init at liberty version #20905

nixos/keystone: init at liberty version #20905

Conversation

nlewo commented Dec 4, 2016

Motivation for this change

Things done

mention-bot commented Dec 4, 2016

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

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

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

spinus commented Dec 5, 2016

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

Choose a reason for hiding this comment

spinus commented Dec 11, 2016

nlewo commented Dec 11, 2016

spinus commented Dec 11, 2016

nlewo commented Dec 11, 2016

nlewo commented Dec 13, 2016

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

Mic92 commented Dec 16, 2016

nlewo commented Dec 16, 2016