Skip to content

Commit

Permalink
openvpn service: source up/down scripts
Browse files Browse the repository at this point in the history 
source the up/down scripts instead of executing them to avoid loosing
access to special variables like $1
  • Loading branch information
@fadenb @grahamc
fadenb authored and grahamc committed Apr 25, 2017
1 parent b6714f5 commit 50ad243
Showing 1 changed file with 13 additions and 3 deletions.
16 changes: 13 additions & 3 deletions nixos/modules/services/networking/openvpn.nix
View file
Expand Up @@ -28,16 +28,26 @@ let
fi
done
${cfg.up}
${optionalString cfg.updateResolvConf
"${pkgs.update-resolv-conf}/libexec/openvpn/update-resolv-conf"}
${optionalString (cfg.up != "") "source ${userSuppliedUpScript}"}
'';

downScript = ''
#! /bin/sh
export PATH=${path}
${optionalString cfg.updateResolvConf
"${pkgs.update-resolv-conf}/libexec/openvpn/update-resolv-conf"}
${optionalString (cfg.down != "") "source ${userSuppliedDownScript}"}
'';

userSuppliedUpScript = pkgs.writeScript "openvpn-${name}-userSuppliedUpScript" ''
${cfg.up}
'';

userSuppliedDownScript = pkgs.writeScript "openvpn-${name}-userSuppliedDownScript" ''
${cfg.down}
'';

Expand Down Expand Up @@ -133,15 +143,15 @@ in
default = "";
type = types.lines;
description = ''
Shell commands executed when the instance is starting.
Shell script sourced by NixOS generated script when the instance is starting.
'';
};

down = mkOption {
default = "";
type = types.lines;
description = ''
Shell commands executed when the instance is shutting down.
Shell script sourced by NixOS generated script when the instance is shutting down.
'';
};

Expand Down

7 comments on commit 50ad243

@peti
Copy link
Member

@peti peti commented on 50ad243 Apr 25, 2017

Choose a reason for hiding this comment

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

@fadenb, @grahamc, I don't understand the purpose of this modification. The up and down commands were embedded in-place into the up and down scripts, i.e. these commands were never executed as separate scripts. Now, after this change, they are written into separate files but then sourced. This seems like the exact same thing we had before, only more complicated.

Am I missing something?

@fadenb
Copy link
Contributor Author

@fadenb fadenb commented on 50ad243 Apr 25, 2017

Choose a reason for hiding this comment

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

@peti: Hmm, perhaps you are right. It might be possible that I used up/down differently than intended.
I was using them like up = "/path/to/some/externally/managed/script.sh". The script I called tried to use $1 $2 ... to get MTU, IP and so on from OpenVPN and failed. I propably just could have used it like up = "source /path/to/some/externally/managed/script.sh or try to embed my whole script.

@grahamc
Copy link
Member

@grahamc grahamc commented on 50ad243 Apr 25, 2017 via email

Choose a reason for hiding this comment

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

@copumpkin
Copy link
Member

Choose a reason for hiding this comment

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

@fadenb I think your options in the old system would have been readFile /path/to/some/externally/managed/script.sh or "source ${/path/to/some/externally/managed/script.sh}". I'd probably just switch it back, because most people probably pass in literal scripts rather than paths to them, and this change modifies the behavior in a breaking manner.

@peti
Copy link
Member

@peti peti commented on 50ad243 Apr 26, 2017

Choose a reason for hiding this comment

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

Yes, please revert.

@grahamc
Copy link
Member

@grahamc grahamc commented on 50ad243 Apr 26, 2017 via email

Choose a reason for hiding this comment

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

@layus
Copy link
Member

@layus layus commented on 50ad243 Apr 27, 2017

Choose a reason for hiding this comment

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

@fadenb It would be useful to emit a warning if this option is a path. This would avoid other getting confused on this option again. Also add the content of 50ad243#commitcomment-21902824 to the documentation of these options.

Please sign in to comment.