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

tailscale: init at 0.97-0 [backport 19.09] #82831

Merged
merged 5 commits into from Mar 27, 2020

Conversation

danderson
Copy link
Contributor

Motivation for this change

Tailscale is a new package+nixos module, merged recently in master. I would like to backport it to 19.09, selfishly so that I can install Tailscale on my NixOS servers.

It's not clear if this is okay or not. I backported the pppd module I created into 19.09 late last year, and back then the reasoning was: it's a "leaf" config, so it only risks breaking something if you explicitly decide to enable it. Therefore the risk was low to other 19.09 users.

I think the same reasoning applies here: the tailscale package is a "leaf" package that creates binaries (not libs), and the nixos module does nothing unless you explicitly opt-in. So, this should be completely safe to backport.

The only policy I could find was in the NixOS manual, where it says RMs decide what bugs & features should be backported. So, @disassembler and @lheckemann, I think this is your decision.

There are 4 commits in this PR: 3 cherrypicks, and 1 19.09-only change.

  1. Cherrypick c5733e7 to fix buildGo113Module (otherwise it can't build anything)
  2. Cherrypick 6e055c9 (tailscale pkg+module init)
  3. Cherrypick 1e59307 (pkg version bump to fix a severe bug)
  4. Change the pkg to use buildGo113Module instead of buildGoModule, because 19.09 defaults to Go 1.12 and Tailscale requires >=1.13.

Of note: even though buildGo113Module exists in 19.09, this would be the 1st and only package that uses it in 19.09 (that's why it was broken without anyone noticing).

So, um, there. That's all the information on this change. Hopefully that helps you decide if this is acceptable to backport or not. I would like to document some general guidelines for this for future maintainers (because nobody was sure if this is okay or not), so I anxiously await your feedback :)

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.

marsam and others added 3 commits March 18, 2020 00:30
Since Go 1.13, `GOSUMDB` defaults to "sum.golang.org", to consult the
checksum database of the main module's go.sum.

We already use the default behavior when building `go-modules`, but Go
tries to consult the checksum database again when building the module,
and fails because since it requires `cacert` and `git` which are not
propagated when building the package.

(cherry picked from commit c5733e7)
Signed-off-by: Martin Baillie <martin@baillie.email>
(cherry picked from commit 6e055c9)
Fixes a severe bug with subnet routing.

Signed-off-by: David Anderson <dave@natulte.net>
(cherry picked from commit f61f686)
@danderson
Copy link
Contributor Author

Ouch, mass rebuild. That's because of c5733e7 , triggering the rebuild of anything that's go+modules. It should be a no-op for anything on Go 1.12 (except hash mutations, of course), but that's a much bigger change than I bargained for when I sent this PR.

tailscale can't work without that backport, so I think that makes me lean towards rejecting this PR, and just waiting for tailscale in 20.03. That's a big rebuild for 19.09 late in its life.

But you're welcome to talk me out of it ;)

Copy link
Member

@KamilaBorowska KamilaBorowska left a comment

Choose a reason for hiding this comment

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

This looks reasonable. The rebuild is unfortunate, but seems harmless as GOSUMDB doesn't exist in Go 1.12[1].

References

  1. Go 1.13 Release Notes, Modules Environment Variables

@Profpatsch
Copy link
Member

Same comment as in #82827 (review) (I think you can even squash the version update commit), then LGTM

@danderson
Copy link
Contributor Author

Done.

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Approving as I did on IRC.

The tag points to the same commit hash, so the binary
is unchanged.

Signed-off-by: David Anderson <dave@natulte.net>
(cherry picked from commit 3fa813e)
Tailscale does not support Go 1.12.

Signed-off-by: David Anderson <dave@natulte.net>
@danderson
Copy link
Contributor Author

Updated to use mostly cherrypicks from master. The one change that's not a cherrypick is specific to 19.09: 609a3da makes the derivation use Go 1.13 explicitly, as it's required by tailscale and not the default on 19.09.

The cherry-pick of 0e1cf19 still causes a rebuild of all Go code. The rebuild is a no-op because Go 1.12 doesn't know about the sumdb environment variable that got added, but that change alters hashes and so, yay rebuild.

@worldofpeace
Copy link
Contributor

@GrahamcOfBorg build antibody

@worldofpeace
Copy link
Contributor

testing it's no op change there.

@danderson
Copy link
Contributor Author

Minor diff in the antibody binary:

--- /home/dave/antibody.before	2020-03-23 15:06:44.702861189 -0700
+++ /home/dave/antibody.after	2020-03-23 15:06:37.322806354 -0700
@@ -40,12 +40,12 @@
 00000270  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
 *
 00000f40  00 00 00 00 00 00 00 00  00 00 00 00 04 00 00 00  |................|
-00000f50  53 00 00 00 04 00 00 00  47 6f 00 00 70 32 4c 53  |S.......Go..p2LS|
-00000f60  4c 38 46 56 75 56 6e 4c  6a 6d 73 6d 63 6c 77 6b  |L8FVuVnLjmsmclwk|
+00000f50  53 00 00 00 04 00 00 00  47 6f 00 00 36 69 4e 33  |S.......Go..6iN3|
+00000f60  79 4a 43 35 38 33 5a 34  7a 51 32 77 71 77 37 76  |yJC583Z4zQ2wqw7v|
 00000f70  2f 6e 31 79 63 72 73 4b  66 32 5f 68 34 63 6a 5f  |/n1ycrsKf2_h4cj_|
 00000f80  56 6a 30 6d 74 2f 75 63  4f 53 42 37 77 66 71 76  |Vj0mt/ucOSB7wfqv|
-00000f90  38 57 42 67 6e 70 43 55  53 4f 2f 38 75 61 6b 5a  |8WBgnpCUSO/8uakZ|
-00000fa0  6c 4d 75 65 57 54 50 6d  4b 47 4e 62 2d 74 48 00  |lMueWTPmKGNb-tH.|
+00000f90  38 57 42 67 6e 70 43 55  53 4f 2f 78 50 70 57 52  |8WBgnpCUSO/xPpWR|
+00000fa0  7a 35 51 6b 34 37 6a 64  36 44 77 45 5f 61 53 00  |z5Qk47jd6DwE_aS.|
 00000fb0  2f 6e 69 78 2f 73 74 6f  72 65 2f 77 78 31 76 6b  |/nix/store/wx1vk|
 00000fc0  37 35 62 70 64 72 36 35  67 36 78 77 78 62 6a 34  |75bpdr65g6xwxbj4|
 00000fd0  72 77 30 70 6b 30 34 76  35 6a 33 2d 67 6c 69 62  |rw0pk04v5j3-glib|

Looks pretty minor, but can't identify the diff more precisely without further disasm.

@danderson
Copy link
Contributor Author

Readelf says all the changed bytes are in the .note.go.buildid ELF section, which is Go's own deterministic build ID that hashes its inputs. I suspect it's taking in the env as a hash input, which would explain why it changed and nothing else - but digging further.

Either way, the actual code+data of the binary is unchanged.

@worldofpeace
Copy link
Contributor

worldofpeace commented Mar 23, 2020

@danderson I was wondering this exactly if that somehow will change build reproducibility if we changed the configure environment. And when you mentioned something about hashes changing I naively thought "well hopefully not modSha256", but reading the code changes I believe that sort of change would not be possible.

@danderson
Copy link
Contributor Author

I'm looking through the code of Go 1.12 that generates the build ID. It cares about a lot of factors, but afaict the environment variables is not one of them...

Clearly one of the inputs to the Go build ID is changing, but I can't figure out which one. I've sent a distress call on twitter (https://twitter.com/dave_universetf/status/1242222407810572288) because I have no idea where to go from here to identify the cause. Will keep looking.

On the bright side though: this change is a no-op in practice, there is no change in the generated machine code. I still want to get a definitive answer on why the build ID changed though.

@danderson
Copy link
Contributor Author

Oh actually, does buildGoModule use a deterministic rootdir path when building? The build path is hashed into the build ID, so if the derivation hash changes (which it did, because buildGoModule's derivation hash changed, because of the env var change), then it would hash in a different build path into the build ID.

That would explain the "build ID only" diff: produced exactly the same code, but built from a different path?

@worldofpeace I'm too new to Nix to be sure of my reasoning here. Does this seem plausible, or would you expect buildGoModule to eradicate that harmless non-determinism? (harmless because it only happens if the derivation hash changes, but the produced binaries are identical)

@worldofpeace
Copy link
Contributor

Oh actually, does buildGoModule use a deterministic rootdir path when building? The build path is hashed into the build ID, so if the derivation hash changes (which it did, because buildGoModule's derivation hash changed, because of the env var change), then it would hash in a different build path into the build ID.

That would explain the "build ID only" diff: produced exactly the same code, but built from a different path?

@worldofpeace I'm too new to Nix to be sure of my reasoning here. Does this seem plausible, or would you expect buildGoModule to eradicate that harmless non-determinism? (harmless because it only happens if the derivation hash changes, but the produced binaries are identical)

@kalbasit Authored buildGoModule, so he would know. This is the buildPhase

buildPhase = args.buildPhase or ''

@worldofpeace
Copy link
Contributor

I do think that could be a plausible thing.

@danderson
Copy link
Contributor Author

I just dived into the Go compiler to try and figure out what goes into build ID, and failed. There's um, a lot of code that goes into computing build ID, and it's intertwined with the build caching logic. I'm going to need an adult in the Go world to help me further (fortunately I might have access to one...).

@Profpatsch
Copy link
Member

Are you afraid that some packages might break because of the binary metadata change? Sounds like a reasonably small chance to me.

Can code actually use / branch on that metadata?

@worldofpeace
Copy link
Contributor

Are you afraid that some packages might break because of the binary metadata change? Sounds like a reasonably small chance to me.

Can code actually use / branch on that metadata?

I personally don't see a problem here, I thought this was just a curiosity investigation. This will all rebuild anyways because the deriver was changed.

@danderson
Copy link
Contributor Author

Technically, Go code can read the build ID, and could branch on it. I can't think of any reasonable justification for doing that though.

This is mostly a curiosity investigation, yes. Unfortunately it's stalled because all my Go team experts are overloaded due to *waves generally at everything*, so I'm not going to pursue this further right now. I've made a note in my nix contrib todo that I want to investigate making buildGoModule more strictly deterministic, even in the face of changing build roots (which is the current suspect cause).

But if you're okay with the rebuild resulting in different build IDs being burned into the binaries, I'm good with merging this as-is.

@grahamc grahamc merged commit 64a3ccb into NixOS:release-19.09 Mar 27, 2020
@danderson danderson deleted the tailscale-19.09 branch March 27, 2020 18:14
@danderson danderson restored the tailscale-19.09 branch November 4, 2020 03:50
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

6 participants