Allow specifying interface for default gateway #21875

abbradar · 2017-01-14T02:11:00Z

Motivation for this change

This is needed for particular network configurations. For example, Hetzner provides me with /64 IPv6 network, but requires to set default gateway fe80::1. This won't work if I just set the address because Linux wouldn't know where should it send packets. Specifying the interface explicitly fixes that.

To accomplish this, I turned networking.defaultGateway{,6} into a submodule with options address and gateway. To hold backwards compatibility I introduce new type coercedTo coercedType coerceFunc finalType. It allows one to specify either something of finalType (merged as is), or coercedType (would be converted to finalType during merge using coerceFunc). For example, option:

foo = mkOption {
  type =
    let mySubmodule = {...}: {
      config = {
        value = mkOption {
          type = types.lines;
        };
      };
    };
    in types.coercedTo types.str (x: { value = x; }) (types.submodule mySubmodule);
};

configuration-1.nix:

{
  foo = "value-1";
}

configuration-2.nix:

{
  foo.value = "value2";
}

We'd get resulting foo.value containing both "value1" and "value2".

cc @nbp as the module system author -- this was my first encounter with it and I'm unsure if I've defined coercedTo correctly.

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.

mention-bot · 2017-01-14T02:11:02Z

@abbradar, thanks for your PR! By analyzing the history of the files in this pull request, we identified @wkennington, @tk-ecotelecom and @edolstra to be potential reviewers.

Mic92 · 2017-01-14T09:56:37Z

Could also add an example for ipv6 static address configuration + default gateway to the manual?
Currently it only documents on how to disable ipv6 ...

abbradar · 2017-01-14T10:54:08Z

An example would pretty much duplicate one for IPv4 sans option names and addresses, so is with the gateway. I've, however, added mentions of needed options and a reference to IPv4 section for examples -- do you think that's OK or it's better to describe things in more detail?

Mic92 · 2017-01-14T11:20:40Z

I think at least the device option is not obvious on first spot. Also this might duplicate nixos options documentation, having a copy and paste example would save people a lot of time when installing nixos on a new device. I would also use link-local addresses in the example as this is the main motivation for the device option in the first place.

networking.defaultGateway6 = {
   address = "fe80::1";
   device = "enp3s0";
};

fpletz

👍 but what I would really love to see is a generic networking.routes module.

abbradar · 2017-01-14T16:13:32Z

I added some examples to the documentation.

@fpletz Me too but I don't have enough time to work on it now D: When/if it's done we could just redefine defaultGateway in terms of it.

globin · 2017-01-14T16:51:00Z

You've used interface in the code and device in the docs, I guess you forgot those after renaming, I definitely prefer interface, that would be the option I'd look for intuitively.

I'd like input from @nbp for the coerce... part, but otherwise lgtm!

abbradar · 2017-01-14T16:52:59Z

@globin Indeed, fixed to interface -- thanks!

nbp

Nice addition, I will note that the module system has a way to post-process options with the apply function of option definitions, but as the either type does not check for submodules, this could be a bit complicated.

One good thing is that the coerceTo function should keep the option definition locations, even after they got converted into modules.

I have one concern about this coerceTo function, which is that the error messages can report errors to code that the user did not wrote, which would generate potentially confusing error messages. In which case I would agree that this would be a NixOS issue, if this happens.

In addition to the following comments, I recommend that you add test cases for this new coerceTo type. Including failing cases. (see lib/tests/modules.sh)

nbp · 2017-01-15T19:17:23Z

lib/types.nix

+    coercedTo = coercedType: coerceFunc: finalType: mkOptionType rec {
+      name = "coercedTo";
+      description = "${finalType.description} or ${coercedType.description} coerced to it";
+      check = x: coercedType.check x || finalType.check x;


This does not matter much here, but I would swap the 2 conditions. (see why below)

nbp · 2017-01-15T19:21:19Z

lib/types.nix

@@ -352,6 +352,25 @@ rec {
      functor = (defaultFunctor name) // { wrapped = [ t1 t2 ]; };
    };

+    coercedTo = coercedType: coerceFunc: finalType: mkOptionType rec {
+      name = "coercedTo";
+      description = "${finalType.description} or ${coercedType.description} coerced to it";


These can be quite complex expressions, I am not sure that coerce to it would make sense in all cases.
The fact that some data are coerce is an implementation detail which does not matter from the documentation perspective of the description. I would recommend using the same description as types.either.

nbp · 2017-01-15T19:22:52Z

lib/types.nix

+      check = x: coercedType.check x || finalType.check x;
+      merge = loc: defs:
+        let
+          coerceVal = val: if coercedType.check val


This code will always attempt to coerce. If there is an overlap between the types, I am not sure this would be the expected behavior. Checking for finalType.check before coercedType.check would make more sense to prevent coercing data which already have the final type.

Good catch, I haven't thought that types could overlap.

nbp · 2017-01-15T19:43:25Z

lib/types.nix

@@ -352,6 +352,25 @@ rec {
      functor = (defaultFunctor name) // { wrapped = [ t1 t2 ]; };
    };

+    coercedTo = coercedType: coerceFunc: finalType: mkOptionType rec {


Assert that coercedType does not contain any submodule by checking that getSubmodule is null.

abbradar · 2017-01-15T20:00:19Z

Nice addition, I will note that the module system has a way to post-process options with the apply function of option definitions, but as the either type does not check for submodules, this could be a bit complicated.

apply won't work here because I want to save ability to merge multiple definitions; see my example (pointing that out because I haven't fully understood what you meant about either, sorry if I just repeated you)

I have one concern about this coerceTo function, which is that the error messages can report errors to code that the user did not wrote, which would generate potentially confusing error messages. In which case I would agree that this would be a NixOS issue, if this happens.

You mean assertions to check that types are correct? Yeah, not sure what can we do about that other than carefully writing coercing functions...

I'll address other comments and add tests, thank you for the review!

abbradar · 2017-01-15T20:12:20Z

Hmm, I think we can greatly improve error handling if define check as:

  check = val:
    let coerceResult = builtins.tryEval (finalType.check (coerceFunc val));
    in finalType.check val || (coerceType.check val && coerceResult.success && coerceResult.value);

This should catch nearly all misbehaving coercion functions, presenting them as type errors. It can be a bit expensive however, requiring two coerceFunc calls (in check and in merge later). We can, on the other hand, omit assert from merge. What do you think?

nbp · 2017-01-15T23:19:37Z

For catching misbehaving coercion function, I think you can just add them as part of an assertion in the merge function. A coercion issue would be something for a NixOS module developer, not a new NixOS user.

abbradar · 2017-01-17T09:06:56Z

Changed coercedTo according to @nbp's comments.

globin · 2017-01-19T21:08:14Z

@nbp would you care to review the changes again? If that's fine I'd be happy to merge 👍

nbp

Fix the multiple option declaration issues, and add tests cases in lib/tests/modules.sh.

nbp · 2017-01-19T23:46:13Z

lib/types.nix

+        let
+          coerceVal = val: if finalType.check val
+                           then val
+                           else let


nit: indent these lines properly:

let coerceVal = val: if finalType.check val then val else let

nbp · 2017-01-19T23:47:20Z

lib/types.nix

@@ -352,6 +352,25 @@ rec {
      functor = (defaultFunctor name) // { wrapped = [ t1 t2 ]; };
    };

+    coercedTo = coercedType: coerceFunc: finalType: assert coercedType.getSubModules == null; mkOptionType rec {


nit: move the assert and mkOptionType on new lines.

nbp · 2017-01-20T00:00:42Z

lib/types.nix

+      getSubOptions = finalType.getSubOptions;
+      getSubModules = finalType.getSubModules;
+      substSubModules = m: coercedTo coercedType coerceFunc (finalType.substSubModules m);
+      functor = (defaultFunctor name) // { wrapped = finalType; };


This would not work if multiple options are declared with the same type.
Also, merging multiple coerceTo types or even a coerceTo with a finalType might not be easy, add a failing test case and a nicer error message.

Sorry, I don't fully understand how functor and typeMerge work and what are their purpose -- how are types merged in the module system and where does this occur? I don't want to blindly fix things by example :D

abbradar · 2017-01-20T00:04:51Z

Augh, yes, I forgot about tests! Very sorry, this was not intentional -- I'll take a look at them (and your other comments) ASAP.

nbp · 2017-01-31T23:03:00Z

@abbradar Do you need help for writing and running the test cases?

abbradar · 2017-01-31T23:05:12Z

Test cases should be okay (I haven't tried but think I can handle) however first I want to understand functor and typeMerge -- I'm unsure what purpose they serve in the module system (i.e. when do we need to merge types?).

nbp · 2017-01-31T23:36:26Z

functo and typeMerge are used when we have multiple option declarations which are declaring the same option with potentially different types. This feature was already present for merging options which have submodules, and got generalized recently for solving the desktop/display manager issue by merging enumerated types.

abbradar · 2017-02-01T00:03:21Z

Thanks, I understand what's going on better now. After some reading I have the next question about typeMerge: if say two types t1 and t2 are merged (in unspecified order), which type' typeMerge gets called? IIUC the operation must be commutative so if I want to support merge of different types (coercedTo foo finalType and finalType) I must define coercedTo logic in all the other types too. Is this correct?

nbp · 2017-02-01T22:28:23Z

@abbradar , exactly, the order in which the typeMerge functions are called is not specified which implies that yes, they would have to be commutative and associative. The associativity sounds hard to achieve if you want to consider both sides of the coerceTo type.

I would say, the easiest way forward I would recommend is only make this type mergeable with its identical counter-part, and not with the finalType.

abbradar · 2017-02-01T22:37:47Z

Oh, I have realized only now that type merging of coercedTo seems impossible anyway because we can't "merge" coerceFunc (unless we can solve halting problems :D). Thanks for explanation of this code! I'll make typeMerge return null always and write some tests.

abbradar · 2017-02-01T22:53:34Z

Pushed new version, along with tests. @nbp, please re-review!

nbp

Nice work, thanks!

nbp · 2017-02-01T23:11:10Z

lib/tests/modules.sh

+# Check coerced value.
+checkConfigOutput "\"42\"" config.value ./declare-coerced-value.nix
+checkConfigOutput "\"24\"" config.value ./declare-coerced-value.nix ./define-value-string.nix
+checkConfigError 'The option value .* in .* is not a coercedTo.' config.value ./declare-coerced-value.nix ./define-value-list.nix


Can you open a follow-up issue to fix this error message.

Not sure, what do you mean? This error message is expected because I try to use a list.

I think I understand what you mean -- "is not a coercedTo" is not a helpful message ;). I think we can fix this easily by outputting description instead of type in the error message -- how's this?

That sounds good, and unrelated to the current patch as well, why I suggested to make a follow-up patch ;)

Indeed. Let's merge this and I'll make another PR. Thank you again for your reviews!

abbradar added 0.kind: enhancement 6.topic: nixos labels Jan 14, 2017

fpletz approved these changes Jan 14, 2017

View reviewed changes

abbradar force-pushed the gateway-interface branch from 63aa092 to 29743f7 Compare January 14, 2017 16:12

abbradar force-pushed the gateway-interface branch from 29743f7 to 6be45fa Compare January 14, 2017 16:52

fpletz requested a review from nbp January 14, 2017 19:44

nbp suggested changes Jan 15, 2017

View reviewed changes

abbradar force-pushed the gateway-interface branch from 6be45fa to 873d890 Compare January 17, 2017 09:06

nbp suggested changes Jan 20, 2017

View reviewed changes

abbradar force-pushed the gateway-interface branch from 873d890 to 645387c Compare January 30, 2017 16:59

types library: add coercedTo

5cbcd3a

abbradar added 3 commits February 2, 2017 01:53

module tests: add tests for coercedTo

fcb20b9

network-interfaces service: add defaultGateway{,6}.interface

0cb487e

manual: mention needed options for IPv6

4feb0a9

abbradar force-pushed the gateway-interface branch from 645387c to 4feb0a9 Compare February 1, 2017 22:53

nbp approved these changes Feb 1, 2017

View reviewed changes

fpletz added this to the 17.03 milestone Feb 1, 2017

abbradar merged commit 5247140 into NixOS:master Feb 2, 2017

fpletz mentioned this pull request May 22, 2017

swap: extend randomEncryption to plainOpen and ability to select cipher #25999

Closed

7 tasks

Profpatsch mentioned this pull request May 24, 2017

Put docstrings for types in types.nix #26055

Closed

Allow specifying interface for default gateway #21875

Allow specifying interface for default gateway #21875

Conversation

abbradar commented Jan 14, 2017 • edited

Motivation for this change

Things done

mention-bot commented Jan 14, 2017

Mic92 commented Jan 14, 2017

abbradar commented Jan 14, 2017

Mic92 commented Jan 14, 2017

fpletz left a comment

Choose a reason for hiding this comment

abbradar commented Jan 14, 2017

globin commented Jan 14, 2017

abbradar commented Jan 14, 2017

nbp 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

abbradar commented Jan 15, 2017

abbradar commented Jan 15, 2017 • edited

nbp commented Jan 15, 2017

abbradar commented Jan 17, 2017

globin commented Jan 19, 2017

nbp 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

abbradar commented Jan 20, 2017

nbp commented Jan 31, 2017

abbradar commented Jan 31, 2017 • edited

nbp commented Jan 31, 2017

abbradar commented Feb 1, 2017 • edited

nbp commented Feb 1, 2017

abbradar commented Feb 1, 2017

abbradar commented Feb 1, 2017

nbp 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

abbradar commented Jan 14, 2017 •

edited

abbradar commented Jan 15, 2017 •

edited

abbradar commented Jan 31, 2017 •

edited

abbradar commented Feb 1, 2017 •

edited