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

Jump to bottom

amazon-image: amazon-init script checks for nix expressions in userdata correctly #50763

Closed
wants to merge 1 commit into from
Closed

amazon-image: amazon-init script checks for nix expressions in userdata correctly #50763

wants to merge 1 commit into from

Conversation

PsyanticY
Copy link
Contributor

@PsyanticY PsyanticY commented Nov 19, 2018 •
edited

A comment in amzon-init says that it will check for valid nix expressions but it do not do that. when lunching a nixos image from aws and typing whatever in the user-data field the amazon-init unit will copy that to configuration.nix and then it will fail when it run nixos-rebuild switch .

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@PsyanticY PsyanticY force-pushed the amazon-image branch 2 times, most recently from eabe459 to e734b44 Compare November 22, 2018 14:48
@PsyanticY
Copy link
Contributor Author

@matthewbauer Any thoughts on this ^^

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-december/1711/11

veprbl
veprbl reviewed Jan 20, 2019
View reviewed changes
nixos/modules/virtualisation/amazon-init.nix Outdated
echo "setting configuration from EC2 user data"
cp "$userData" /etc/nixos/configuration.nix
else
echo "user data does not appear to be a Nix expression; ignoring"
Copy link
Member

Choose a reason for hiding this comment

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

You should probably change wording of this to indicate that the expression failed to evaluate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@veprbl thanks for the review. Would Failed to evaluate user data with nix-instantiate; ignoring be a more suitable message.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, maybe, evaluate wasn't a right word here. How about "user data is not a valid Nix expression; ignoring"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@veprbl yeah thats wayy better.

@veprbl
Copy link
Member

veprbl commented Jan 20, 2019

cc @shlevy @copumpkin @edolstra as latest contributors to amazon-init.nix

@veprbl
Copy link
Member

veprbl commented Jan 21, 2019

One other comment I have is that after your change, the else branch of

if sed '/^\(#\|SSH_HOST_.*\)/d' < "$userData" | grep -q '\S'; then

doesn't have a warning and, more importantly, it doesn't terminate with exit.

@edolstra
Copy link
Member

I don't see why this is necessary. The Nix expression will be checked eventually anyway by nixos-rebuild...

@PsyanticY
Copy link
Contributor Author

PsyanticY commented Jan 22, 2019 •
edited

@veprbl Added the else clause as advised.
@edolstra It is definitely not necessary but it can prevent copying wrong expressions to /etc/nixos/configuration.nix
Provding this is whatever as user data results in this

[root@ip-10-100-46-187:~]# cat /etc/nixos/configuration.nix 
this is whatever
[root@ip-10-100-46-187:~]#

@edolstra
Copy link
Member

Well, I don't see why it matters whether it fails in amazon-init or in nixos-rebuild. Either way your configuration doesn't work. Also, nix-instantiate --eval is a very shallow test (especially on NixOS modules) - it doesn't actually guarantee that the system configuration works.

@PsyanticY
Copy link
Contributor Author

@edolstra mmmm, yeah. Closing then.

@PsyanticY PsyanticY closed this Jan 22, 2019
@PsyanticY PsyanticY deleted the amazon-image branch July 15, 2019 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veprbl veprbl veprbl left review comments

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@PsyanticY @nixos-discourse @veprbl @edolstra @GrahamcOfBorg