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

Add nix-store-gcs-proxy service #79925

Merged
merged 1 commit into from Mar 2, 2020

Conversation

@mrkkrp
Copy link
Member

@mrkkrp mrkkrp commented Feb 12, 2020

Motivation for this change

This adds a NixOS module which provides a service for running nix-store-gcs-proxy automatically.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Feb 12, 2020

cc @andir

Copy link
Member

@andir andir left a comment

How and where does this get credentials from? I think we should add a few more lines of documentation either in the options and/or the nixos documentation.

As far as I know it tries to retrieve those credentials from some environment variable and some well known file system location?

Also commented on the test that should probably be improved to do something.

nixos/tests/nix-store-gcs-proxy.nix Outdated Show resolved Hide resolved
@mrkkrp mrkkrp force-pushed the mrkkrp:mk/add-nix-store-gcs-proxy-service branch from 56e0b18 to 450d3da Feb 12, 2020
@mrkkrp
Copy link
Member Author

@mrkkrp mrkkrp commented Feb 12, 2020

@andir What do you mean by "credentials"? Accessibility of storage bucket is controlled (at least in our case) by service account. If you mean nix signing key, then it is provided like this in the post build hook:

exec nix copy --to http://localhost:3000?secret-key=/run/keys/nix_signing_key \$OUT_PATHS

Added the test to all-tests.nix.

@andir
Copy link
Member

@andir andir commented Feb 12, 2020

@andir What do you mean by "credentials"? Accessibility of storage bucket is controlled (at least in our case) by service account. If you mean nix signing key, then it is provided like this in the post build hook:

exec nix copy --to http://localhost:3000?secret-key=/run/keys/nix_signing_key \$OUT_PATHS

Added the test to all-tests.nix.

Where does that service account come from if I run this on my laptop?

@mrkkrp
Copy link
Member Author

@mrkkrp mrkkrp commented Feb 16, 2020

@andir Apparently you can't really run it without real credentials. If you have credentials you can provide them in one of the ways described here:

https://cloud.google.com/docs/authentication/production

This is why I had to abandon writing "real" tests.

@mrkkrp mrkkrp force-pushed the mrkkrp:mk/add-nix-store-gcs-proxy-service branch from 450d3da to 7f54f38 Feb 16, 2020
@mrkkrp mrkkrp force-pushed the mrkkrp:mk/add-nix-store-gcs-proxy-service branch from 7f54f38 to 4520bed Feb 20, 2020

testScript = ''
start_all()
# the nix-store-gcs-proxy executable needs valid credentials to be

This comment has been minimized.

@andir

andir Feb 20, 2020
Member

Then this test is basically useless? It doesn't even wait for the VM to start. It just starts it and then terminates.

cc @zimbatm Since you authored (most of?) the proxy: Is there something sensible we can actually test here? I would really like the daemon come up and respond to http requests.

This comment has been minimized.

@zimbatm

zimbatm Feb 21, 2020
Member

I guess we could add a /_health end-point like this: tweag/nix-store-gcs-proxy#7 ? Ideally the _health endpoint also checks that it has access to the bucket but in this context it would be hard to emulate that.

This comment has been minimized.

@zimbatm

zimbatm Feb 24, 2020
Member

Let's just drop this test as it's not really useful in its current form.

This comment has been minimized.

@Mic92

Mic92 Feb 25, 2020
Contributor

It could be emulated using something like that https://github.com/fsouza/fake-gcs-server.
For the time being this might be too complicated...

This comment has been minimized.

@zimbatm

zimbatm Feb 26, 2020
Member

Yeah, I think we have been setting the bar of acceptance too high and it's unfair to @mrkkrp

@mweinelt
Copy link
Member

@mweinelt mweinelt commented Feb 20, 2020

Commit message should read "module/nix-store-gcs-proxy: init" per Contributing.

@mrkkrp mrkkrp force-pushed the mrkkrp:mk/add-nix-store-gcs-proxy-service branch 2 times, most recently from d2e16d1 to cb8d40d Feb 21, 2020
@andir
Copy link
Member

@andir andir commented Feb 21, 2020

@mrkkrp
Copy link
Member Author

@mrkkrp mrkkrp commented Feb 24, 2020

Is there anything else I should adjust to get this merged?

@andir
Copy link
Member

@andir andir commented Feb 24, 2020

@mrkkrp
Copy link
Member Author

@mrkkrp mrkkrp commented Feb 24, 2020

@andir Well @zimbatm responded that we could add an endpoint to nix-store-gcs-proxy, that is, change the executable itself. Is this really in scope for this particular PR?

@zimbatm
Copy link
Member

@zimbatm zimbatm commented Feb 24, 2020

Agreed, let's merge this without the test. Not all modules need to be thoroughly tested and if it works that's good enough for me.

@mrkkrp
Copy link
Member Author

@mrkkrp mrkkrp commented Mar 2, 2020

Let's click "merge" then?

@zimbatm
Copy link
Member

@zimbatm zimbatm commented Mar 2, 2020

Please remove the test as discussed above.

@mrkkrp mrkkrp force-pushed the mrkkrp:mk/add-nix-store-gcs-proxy-service branch 2 times, most recently from 2a41ed6 to 6a52c55 Mar 2, 2020
@mrkkrp
Copy link
Member Author

@mrkkrp mrkkrp commented Mar 2, 2020

@zimbatm Done.

Copy link
Member

@andir andir left a comment

Please remove the test if we aren't going to improve on it.

nixos/tests/nix-store-gcs-proxy.nix Outdated Show resolved Hide resolved
@mrkkrp mrkkrp force-pushed the mrkkrp:mk/add-nix-store-gcs-proxy-service branch from 6a52c55 to be8fe89 Mar 2, 2020
@mrkkrp mrkkrp force-pushed the mrkkrp:mk/add-nix-store-gcs-proxy-service branch from be8fe89 to 96b472e Mar 2, 2020
@andir
andir approved these changes Mar 2, 2020
@andir andir merged commit ca5048c into NixOS:master Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.