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

google-compute-config: Reintroduce fetch-ssh-keys #110784

Merged
merged 2 commits into from Feb 20, 2021

Conversation

talyz
Copy link
Contributor

@talyz talyz commented Jan 25, 2021

Motivation for this change

Reintroduce the fetch-ssh-keys service so that GCE images that work with NixOps can once again be built. Also, reformat the code a bit.

The service was removed in 8857053, likely due to a comment saying it should be removed. It is however still needed for NixOps, at least until the plugin is able to handle google-oslogin.

Things done
  • Built an image and deployed it with NixOps.

  • 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.

Reintroduce the `fetch-ssh-keys` service so that GCE images that work
with NixOps can once again be built. Also, reformat the code a bit.

The service was removed in 8857053,
likely due to a comment saying it should be removed. It was still
needed for images to work with NixOps, however, and probably needed to
be replaced or rewritten rather than removed.
nixos/modules/virtualisation/google-compute-config.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/google-compute-config.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/google-compute-config.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/google-compute-config.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/google-compute-config.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/google-compute-config.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/google-compute-config.nix Outdated Show resolved Hide resolved
@flokli
Copy link
Contributor

flokli commented Feb 10, 2021

@talyz can you address the requested changes?

@flokli
Copy link
Contributor

flokli commented Feb 14, 2021

@talyz Probably too early for this PR, but long term, It'd probably be worth a shot to switch this from yet another bash script to using afterburn, as suggested in #110784 (comment). The package has been merged in #68680,

There's a afterburn-sshkeys@, which takes care of provisioning the authorized keys (didn't test it though). The logic for the host keys still seems to be missing.

@talyz
Copy link
Contributor Author

talyz commented Feb 14, 2021

@flokli Yes, it could be interesting when / if it gains support for managing host keys. For authorized keys, wouldn't oslogin be recommended?

@flokli
Copy link
Contributor

flokli commented Feb 15, 2021

Yes, oslogin certianly is recommended, as that's not run once during startup, but for each connection - but even where oslogin isn't possible, I'd prefer using a somewhat more maintained tool over our own bashscripts ;-)

@AmineChikhaoui
Copy link
Member

Anything pending in this PR ? I'd like to use it once merged to generate some images for Google cloud.
Btw I assume backporting to 20.09 wouldn't be an issue ?

@flokli
Copy link
Contributor

flokli commented Feb 18, 2021

Anything pending in this PR ? I'd like to use it once merged to generate some images for Google cloud.
Btw I assume backporting to 20.09 wouldn't be an issue ?

This looks fine from an overview, and assuming @talyz sufficiently test this on Google Cloud, this is probably fine to merge.

I quickly wondered if we could add coverage for this in the google-oslogin test (and by this make it a more generic google metadata server test), but considering this is not in a module, but just in the profile directly, we can't easily add it there without pulling in other unwanted stuff.

So I'd say if you want to merge this, we can, but we have no way of ensuring it keeps working as long as there is no test ;-)

...check the script with shfmt and shellcheck + some other minor
refactoring.
@talyz
Copy link
Contributor Author

talyz commented Feb 20, 2021

@flokli Yes, I've verified that it works with NixOps. If a normal instance is set up, the service fails I expected:

Feb 20 01:35:12 fetch-instance-ssh-keys[684]: Fetching authorized keys...
Feb 20 01:35:12 fetch-instance-ssh-keys[686]: --2021-02-20 01:35:12--  http://metadata.google.internal/computeMetadata/v1/instance/attributes/sshKeys
Feb 20 01:35:12 fetch-instance-ssh-keys[686]: Resolving metadata.google.internal (metadata.google.internal)... 169.254.169.254
Feb 20 01:35:12 fetch-instance-ssh-keys[686]: Connecting to metadata.google.internal (metadata.google.internal)|169.254.169.254|:80... connected.
Feb 20 01:35:12 fetch-instance-ssh-keys[686]: HTTP request sent, awaiting response... 404 Not Found
Feb 20 01:35:12 fetch-instance-ssh-keys[686]: 2021-02-20 01:35:12 ERROR 404: Not Found.
Feb 20 01:35:12 fetch-instance-ssh-keys.service: Main process exited, code=exited, status=8/n/a
Feb 20 01:35:12 fetch-instance-ssh-keys.service: Failed with result 'exit-code'.
Feb 20 01:35:12 Failed to start Fetch host keys and authorized_keys for root user.

This is of course not fatal, since no other units depend on the service, but the user will see a service failure at instance boot.

@talyz
Copy link
Contributor Author

talyz commented Feb 20, 2021

@flokli Well, the script is so simple that the mock server would likely be more complex and prone to breakage, tbh. Since we're aiming for afterburn to replace the authorized keys fetching (almost the entire script) anyways, we should probably focus on writing tests for it instead of wasting our effort on this stop-gap solution.

@flokli
Copy link
Contributor

flokli commented Feb 20, 2021

Yeah, agreed. I simply wanted to state we don't know it'll keep working:

So I'd say if you want to merge this, we can, but we have no way of ensuring it keeps working as long as there is no test ;-)

Given noone pressed the big green button, I'll do it now.

@flokli flokli merged commit d0be6dc into NixOS:master Feb 20, 2021
@talyz talyz added the 9.needs: port to stable A PR needs a backport to the stable release. label Feb 22, 2021
@talyz talyz deleted the gce-fetch-ssh-keys branch February 22, 2021 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants