Add dynamic option for vault

[ProbablyRusty]

• Have you followed the guidelines for contributing?
• Have you checked that there aren't other open pull requests for the same formula update/change?
• Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
• Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

---

• Add option “with-dynamic” to vault, in order to optionally build with CGO_ENABLED
• This is a common use case for VPN users on Mac OS X
• See:
  DNS resolution issues when using vault hashicorp/vault#1159

hashicorp/vault#712

[mistydemeo]

Is there also a reason to prefer the static build? Should we always build it one way instead of offering an option?

[ProbablyRusty]

HashiCorp distributes the static build as their downloadable binary, so I feel we should respect that as a default, but I could be convinced otherwise.

[mistydemeo]

I'd love to get a comment from one of the developers on that.

[ProbablyRusty]

Perhaps you'd like to weigh in, @jefferai?

[jefferai]

I'm not opposed to this per se but want to mention a few things:

1. We don't support dynamic builds so it shouldn't be the default and should probably print a warning
2. This doesn't seem to be a common problem; it's likely DNS-setup/VPN software specific (there have been many ways that OSX can configure name information in various versions and different VPN clients/servers used different methods)
3. We never heard back from users on either mentioned ticket so I'm not even sure this helps those that do experience a problem, my suggestions there were just guess work

[ProbablyRusty]

@jefferai Out of band, I'll follow up with you separately or on the referenced tickets, but quickly to point (3): CGO_ENABLED=1 does indeed help with this, and to (2): I have more context on this now and there are also some non-VPN cases in which it is an issue as well. Like I said, I'll follow up elsewhere. Thanks for opining here. Very helpful.

@mistydemeo Looks like the option (non-default) approach in my PR is the right one. Let me augment this PR with a new commit which adds a warning message as per (1).

[jefferai]

@consultantRR I am aware of non-VPN cases where this helps. I just know that the VPN case is dependent on specifics.

[ProbablyRusty]

Indeed, I should have said "some VPN users on Mac OS X" and not "VPN users on Mac OS X" in the commit message.

[MikeMcQuaid]

We never heard back from users on either mentioned ticket so I'm not even sure this helps those that do experience a problem, my suggestions there were just guess work
We don't support dynamic builds so it shouldn't be the default and should probably print a warning

Given these two things I'm 👎 on adding this option until we have more information here. I don't want us to add options unsupported by upstream (because then we implicitly support them instead) and I don't want to add options that aren't going to be widely used to definitely fix a specific problem.

Don't need to close this, can leave it open for now.

[jefferai]

@MikeMcQuaid @consultantRR is one of the people we never heard from so here is saying it did help, just didn't say anything on the previous ticket

Also let me just clarify "unsupported" in this context: our unit tests run on static builds but we also run the race detector which requires dynamic builds. We don't know of any breakages; it's really in the context of platform-specific cgo things (default DNS resolver, possibly cert handling depending on how that has shaken out, which syscalls are available) where if the user experiences an issue with dynamic builds we will try to help but may fall back on "Go issue" and report upstream if the issue is not something we can replicate with the static build. So it's not like we abandon those users.

As I said before I'm fine with it, I just think a warning is a good idea, for instance notifying the user that using the dynamic option changes DNS resolution so if the user experiences issues they may want to try the official/static build. I should note that this isn't really Vault specific behavior anyways; any Go program will behave this way. So I'm just being overly cautious here rather than aware of specific problems.

[ProbablyRusty]

@jefferai Thanks, I agree with everything you wrote. (By the way, the cert handling issues you mention are, in my experience, no longer a problem. I believe it's only DNS resolution that remains a potential problem in some use cases with static binaries on OS X.)

@MikeMcQuaid As @jefferai correctly notes, this is essentially a longstanding Go issue. Users who want to use a tool like Vault on OS X in certain situations (split DNS over OpenVPN & Bonjour/mdns, for example) do sometimes need to build the tool dynamically. It would be nice, in my opinion, if they could easily do so with Homebrew. (Not as a default, but as an option, as I have proposed.)

How's this for a proposed warning message?

Previously, I proposed:

Be aware that HashiCorp does not support dynamic builds.

How about:

The official HashiCorp release of Vault is built (and tested) as a static binary. Be aware that building Vault dynamically (with CGO_ENABLED=1) changes DNS resolution. Specifically, a dynamic build will utilize native OS X DNS resolution (inspectable with scutil --dns) rather than the DNS resolvers defined in /etc/resolv.conf. Other behavioral differences are possible as well. If any issues occur, it is highly recommend to first try the official static binary prior to any further troubleshooting.

Let me know if that works ok, and settles the prior concerns. If so, I'll commit the new warning, and hopefully we can move ahead with merging this and #7246.

[jefferai]

If it's easy to turn into a reusable message I'd remove the HC/Vault mention and just make it generic so other Go packages could reuse it (maybe with some simplification first).

[ProbablyRusty]

Cool, how about:

Be aware that building Go binaries dynamically (with CGO_ENABLED=1) on macOS changes DNS resolution. Specifically, a dynamic build will utilize native macOS DNS resolution (inspectable with scutil --dns) rather than the DNS resolvers defined in /etc/resolv.conf. Other behavioral differences are possible as well. If any issues occur, it is highly recommended to first test a static binary prior to any further troubleshooting.

[MikeMcQuaid]

Homebrew doesn't really want to be printing out such large warnings. It sounds to me like the dynamic build makes more sense for macOS but only if we decide to do so without warnings. If we add warnings we should just go for an option where we have (unsupported by upstream) in the option text instead of caveats or opoo etc.

[MikeMcQuaid]

target = build.with?("dynamic) ? "dev-dynamic" : "dev" and stick it immediately above system "make", target

[jefferai]

Do you build from source regardless of dynamic flag or not?

[ProbablyRusty]

If no build options are specified, then a prebuilt bottle is installed, I believe.

[jefferai]

I'd really prefer if it's not marked as unsupported. Don't really want to scare off users.

[jefferai]

Does HB provide any information (env flags, build flags, whatevs) to compilers about what's being built and/or version? Or is it relatively easy to do so?

[MikeMcQuaid]

If no build options are specified, then a prebuilt bottle is installed, I believe.

Indeed.

Does HB provide any information (env flags, build flags, whatevs) to compilers about what's being built and/or version? Or is it relatively easy to do so?

Yes but are there more specific things you're looking for?

[jefferai]

I mainly would like a way to know when a user reports an issue whether the build is static or dynamic. I can get values at compile time (that's how we get the git rev for instance) but I'm not sure other than keying on CGO_ENABLED whether I can easily discover this at compile time.

This would actually be useful beyond HB, so maybe the best thing here is to defer this for the moment. I can look at getting something in for 0.6.3 to either detect it on my side or so that HB can pass in some identifying value at compile time.

[ProbablyRusty]

This PR is simply making the dev-dynamic target which is already in the official Makefile:

dev-dynamic: generate
	@CGO_ENABLED=1 BUILD_TAGS='$(BUILD_TAGS)' VAULT_DEV_BUILD=1 sh -c "'$(CURDIR)/scripts/build.sh'"

Could be handled there, I suppose.

[jefferai]

Sure, like I said I will try to solve it generally. :-)

[jefferai]

For 0.6.3 I've added output that will indicate whether cgo is enabled when the binary is run. My preference would be to wait until then (a couple of weeks) and then add this option in with the 0.6.3 formula.

[ProbablyRusty]

I like that plan, @jefferai. Maybe that output implementation could be shared with the Nomad team for future consistency with #7246? @diptanu?

[MikeMcQuaid]

Sounds good to me too 👍

[ProbablyRusty]

Vault 0.6.3 is released, and I have updated this PR accordingly. I think the Do Not Merge tag can now be removed, no?

[MikeMcQuaid]

Please leave this as-is, they'll be updated when we pull.

[MikeMcQuaid]

@BrewTestBot test this please

[dunn]

Having trouble applying the patch; could you squash this down to one commit? Thanks!

[ProbablyRusty]

Done. Thanks!

[dunn]

Thanks! Merged as 3a9a2ab.

[dunn]

Crap, sorry about the typo in the commit message.