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

[WIP] Enable Vault UI #49082

Closed
wants to merge 5 commits into from
Closed

[WIP] Enable Vault UI #49082

wants to merge 5 commits into from

Conversation

arianvp
Copy link
Member

@arianvp arianvp commented Oct 25, 2018

Motivation for this change

Since version 0.10, Vault ships with an administrator UI. This PR allows us to use that.
This was a bit of task to get working!

The new consul version also packages its UI this way. So when we want to bump consul from the current (pretty old) version, we can use the same groundwork we did here. So that's good news.

Including the assets has barely any impact on the closure size:

these paths will be fetched (13.92 MiB download, 76.72 MiB unpacked):
  /nix/store/5ii0rjvfj6s9260g1mwyrx8bmyqildh9-vault-0.11.2
copying path '/nix/store/5ii0rjvfj6s9260g1mwyrx8bmyqildh9-vault-0.11.2' from 'https://cache.nixos.org'...
/nix/store/5ii0rjvfj6s9260g1mwyrx8bmyqildh9-vault-0.11.2	   83119048


[arian@ryzen:~/Projects/nixpkgs]$ nix path-info -S $(nix-build -A vault)
/nix/store/dwhii30w2xlbs81mpca2kzx8y3byrmrq-vault-0.11.2	   83779736

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.

Enables the vault UI, which ships with vault since version 0.10
Also adds a test that this option actually works
@GrahamcOfBorg GrahamcOfBorg 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/` labels Oct 25, 2018
@arianvp
Copy link
Member Author

arianvp commented Oct 25, 2018

I'm not sure why the build is failing. It gives a "path is not valid" but I can't reproduce that locally

@Mic92
Copy link
Member

Mic92 commented Oct 25, 2018

yarn.nix is a bit large can you try to integrate this into node-packages? Then we can share nix expressions between packages.

@arianvp
Copy link
Member Author

arianvp commented Oct 25, 2018

The problem is that consul and vault use yarn.lock instead of package-lock.json. Which npm2nix isn't compatible with ... But I could try. Maybe I don't need the exact upstream versions for them.

@arianvp
Copy link
Member Author

arianvp commented Oct 25, 2018

Another problem is that some of the dependencies of Vault's UI are not on npm but are on github instead. But I could see if I can try to split that up someway.

edit: apparently npm2nix does support this!

I'll have a look today. Lets hope consul and vault use the same package versions :)

@arianvp
Copy link
Member Author

arianvp commented Oct 25, 2018

Alas @Mic92

I tried adding it to node-packages but I don't think it's a smart idea or feasible. We don't really record version numbers in node-packages at the moment, as we mostly use it to ship "standalone nodejs based applications", not as a set of packages that is used as dependencies for other packages. Both vault and consul need specific versions of npm libraries, not just "any version" so adding it to our global node package set is not a very scalable approach. the node-packages set as currently set up doesn't really seem to be made for this kind of stuff. I think vendoring the needed yarn.nix file with the consul and vault package is the easiest way to go ...

@LnL7
Copy link
Member

LnL7 commented Oct 26, 2018

I think the evaluation error is cause by the way yarn2nix works, it uses import from derivation which isn't allowed on ofborg or hydra.

@arianvp
Copy link
Member Author

arianvp commented Oct 26, 2018 via email

@LnL7
Copy link
Member

LnL7 commented Oct 26, 2018

I thought so too, but maybe not? The error definitely looks like an error caused by restrict-eval (see man nix.conf)

@arianvp arianvp mentioned this pull request Oct 26, 2018
9 tasks
@arianvp
Copy link
Member Author

arianvp commented Oct 26, 2018

Is patterns like:
https://github.com/NixOS/nixpkgs/pull/49082/files#diff-9f45399180b286c35746a0fcafa076cfR25

disallowed in restricted eval mode? I.e. the source of one package is dependent on the source of another package?

@arianvp
Copy link
Member Author

arianvp commented Oct 26, 2018

Ahaaa... It's importJSON'ing package.json.... I see.

@arianvp
Copy link
Member Author

arianvp commented Oct 26, 2018

I filed an issue upstream with yarn2nix nix-community/yarn2nix#83

This method isn't allowed on Hydra. It was only used to
extract metadata from package.json

We remove it, so that the build is pure again.
We should probably upstream this to the yarn2nix repo
as well
@arianvp
Copy link
Member Author

arianvp commented Oct 26, 2018

Apparently consul simply vendors the UI assets in their Repo: #48714 (comment) . This makes building consul a breeze.

I've asked upstream if they would consider doing something similar to vault. This means we wouldn't have to vendor 12k lines of yarn dependencies. hashicorp/vault#5622

@arianvp
Copy link
Member Author

arianvp commented Oct 26, 2018

Unfortunately, they will not change this upstream. So we'll have to go with yarn2nix

@arianvp
Copy link
Member Author

arianvp commented Oct 26, 2018

@Mic92 are there any technical downsides of including this yarn.nix file that I'm not overseeing, like for example evaluation time or other issues? If not, I would prefer to merge this as is, as putting all these dependencies in node-packages seems non-trivial to me at this moment. For the reasons put above in this thread.

@Mic92
Copy link
Member

Mic92 commented Oct 27, 2018

This pull request adds 500 KB generated data to our repository. This will stick in our history and requires additional storage for each channel user. I can tell from experience that also git does not like large generated text files. Also we might be able to fix nix evaluation performance it would be impossible to fix our history.
To still make progress on this pull request I suggest the following approach:

  1. Fork vault on github
  2. Add travis configuration that builds the ui assets and upload them on release tags.
  3. Change the nix build to just take the compiled asset archive instead.

Here is an example project for uploading release assets: https://github.com/Mic92/cntr/blob/master/.travis.yml#L39

@arianvp
Copy link
Member Author

arianvp commented Oct 27, 2018

That sounds like a good idea. I will do that. If it turns out to be too much work. I'm just gonna drop this PR, and we'll just continue building Vault without a UI.

@roberth roberth added the 2.status: work-in-progress This PR isn't done label Oct 28, 2018
@Mic92 Mic92 changed the title Enable Vault UI [WIP] Enable Vault UI Oct 28, 2018
@Mic92 Mic92 removed the 2.status: work-in-progress This PR isn't done label Oct 28, 2018
@zimbatm zimbatm mentioned this pull request Dec 5, 2018
@arianvp
Copy link
Member Author

arianvp commented Dec 5, 2018

I lost interest in getting this merged, as I have no interest in maintaining my own fork of vault with my own CI, given it's quite a security-senstive product. Someone else feel free to pick this up, or use it to build Vault locally with UI enabled :)

@arianvp arianvp closed this Dec 5, 2018
@PsyanticY PsyanticY mentioned this pull request Jul 20, 2019
9 tasks
PsyanticY added a commit to PsyanticY/nixpkgs that referenced this pull request Jul 25, 2019
This binray contain the UI part of HashiCorp Vault that we were not able to build it due to the need to generate a very big yarn file. NixOS#49082
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants