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 buildinfo flag #6286

Closed
wants to merge 6 commits into from
Closed

Conversation

alexbozhenko
Copy link
Contributor

@alexbozhenko alexbozhenko commented Dec 19, 2024

Use https://pkg.go.dev/runtime/debug#BuildInfo to see all the knobs that were used to build the binary, including the Go toolchain version, https://go.dev/doc/godebug values, etc...

This information is already available through
go version -m path/to/binary
But it will be more convenient to have it available without Go installed(which is most likely the case in production)

Test plan:

go build .
./nats-server -buildinfo
./nats-server --buildinfo

Return:

# ./nats-server --buildinfo
go      go1.23.4
path    github.com/nats-io/nats-server/v2
mod     github.com/nats-io/nats-server/v2       (devel)
dep     github.com/klauspost/compress   v1.17.11        h1:In6xLpyWOi1+C7tXUUWv2ot1QvBjxevKAaI6IXrJmUc=
dep     github.com/minio/highwayhash    v1.0.3  h1:kbnuUMoHYyVl7szWjSxJnxw11k2U709jqFPPmIUyD6Q=
dep     github.com/nats-io/jwt/v2       v2.7.3  h1:6bNPK+FXgBeAqdj4cYQ0F8ViHRbi7woQLq4W29nUAzE=
dep     github.com/nats-io/nkeys        v0.4.9  h1:qe9Faq2Gxwi6RZnZMXfmGMZkg3afLLOtrU+gDZJ35b0=
dep     github.com/nats-io/nuid v1.0.1  h1:5iA8DT8V7q8WK2EScv2padNa/rTESc1KdnPw4TC2paw=
dep     go.uber.org/automaxprocs        v1.6.0  h1:O3y2/QNTOdbF+e/dpXNNW7Rx2hZ4sTIPyybbxyNqTUs=
dep     golang.org/x/crypto     v0.31.0 h1:ihbySMvVjLAeSH1IbfcRTkD/iNscyz8rGzjF/E5hV6U=
dep     golang.org/x/sys        v0.28.0 h1:Fksou7UEQUWlKvIdsqzJmUmCX3cZuD2+P3XyyzwMhlA=
dep     golang.org/x/time       v0.8.0  h1:9i3RxcPv3PZnitoVGMPDKZSq1xW1gK1Xy3ArNOGZfEg=
build   -buildmode=exe
build   -compiler=gc
build   DefaultGODEBUG=asynctimerchan=1,gotypesalias=0,httpservecontentkeepheaders=1,tls3des=1,tlskyber=0,x509keypairleaf=0,x509negativeserial=1
build   CGO_ENABLED=1
build   CGO_CFLAGS=
build   CGO_CPPFLAGS=
build   CGO_CXXFLAGS=
build   CGO_LDFLAGS=
build   GOARCH=amd64
build   GOOS=linux
build   GOAMD64=v1
build   vcs=git
build   vcs.revision=fedf08be59ab96a74906dd2a3f3c67fa3b286795
build   vcs.time=2024-12-26T22:27:24Z
build   vcs.modified=true

Signed-off-by: Alex Bozhenko [email protected]

@alexbozhenko alexbozhenko requested a review from a team as a code owner December 19, 2024 23:29
Copy link
Member

@bruth bruth left a comment

Choose a reason for hiding this comment

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

IMO it is a pretty niche desire for wanting to read the build info if you do not have Go installed. That said, I suppose there is no harm in adding a CLI option as long as it does not introducing breaking changes.

server/opts.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
@alexbozhenko alexbozhenko marked this pull request as draft December 20, 2024 20:37
Signed-off-by: Alex Bozhenko <[email protected]>
@alexbozhenko alexbozhenko marked this pull request as ready for review December 27, 2024 00:39
@alexbozhenko alexbozhenko requested a review from bruth December 27, 2024 00:42
server/server.go Outdated Show resolved Hide resolved
Copy link
Member

@bruth bruth left a comment

Choose a reason for hiding this comment

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

LGTM

@ripienaar
Copy link
Contributor

Like @bruth not really sure of the use case here. We'd get better milage from publishing a SBOM in the industry standard format.

@alexbozhenko
Copy link
Contributor Author

alexbozhenko commented Dec 27, 2024

@ripienaar: the use case is to find out what kind of binary I am dealing with.
Even though I know "version" and "Git" from the output that is printed when the server starts, I do not have visibility into all the other knobs that may be tuned, and knowing just git commit sha is not enough to, e.g. reproduce the binary.
I am not sure about SBOM... Looks like it is concerned with dependency info?

@ripienaar
Copy link
Contributor

I understand why you would want it @alexbozhenko - but this is not something anyone else have asked for. So as a developer I can see the use case, but then I'd just run go version -m nats-server and get my answer.

What I wouldn't do is expose developer specific information as flags to end users who would only find it adds more UI clutter and surprising or confusing output. So my question really is: As a end-user, how is this useful?

@alexbozhenko
Copy link
Contributor Author

alexbozhenko commented Dec 27, 2024

@ripienaar
Here is an example of an issue the user faced:
#6241
After we fix binaries.nats.dev, a user may want to verify that the binary that was downloaded is armv7...
It is easy to imagine scenarios where having easy access to the specifics of the build would be useful.

go version -m doesn't work without jumping through hoops, e.g. with a container. But this works and it is nice:

# docker run -it nats:latest --version
nats-server: v2.10.24

Other tools expose additional info about how binary was built too:
image
image

@ripienaar
Copy link
Contributor

The linked user issue is a case of the binary not being downloaded at all - how would the flag have helped there?

curl -sf https://binaries.nats.dev/nats-io/nats-server/[email protected] | sh
...
Error downloading, got 500 response from server

Again, I understand this makes the information easy to find - I am wondering what the end user utility of this information is, the CLI flags to ants-server is end user interface, not developer user interface so we need to frame these kinds of additions in the light of end users

@alexbozhenko
Copy link
Contributor Author

alexbozhenko commented Dec 27, 2024

Here is how I thought the flag can be beneficial for the user in this case:

  1. (once it is fixed) user downloads curl -sf https://binaries.nats.dev/nats-io/nats-server/[email protected] | sh
  2. The user runs nats-server --buildinfo to check that it is indeed an ARMv7 binary

Or, let's imagine we decided to tune some other flag(existing or some future one). Let's say we start to build with GOAMD64=v4. And let's say such binary crashes for some users. If we make it easier to figure out the reason - that would be great. That's why I would like to have buildinfo exposed for the users.

Answering the question "What kind of binary I am dealing with" is a valid user concern. And our users are developers, so they won't consider this info clutter...
In addition to being useful for the users, it also would be useful for us - the support team, supporting paying customers.

Copy link
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

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

Added some comments, also think might be too niche but could be convenient when trying to get on the same page with users when reproducing issues to get the build info from the Go version used to build it just in case instead of the Go version that might be available locally or when do not have one at hand.

server/opts.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@ripienaar
Copy link
Contributor

Here is how I thought the flag can be beneficial for the user in this case:

  1. (once it is fixed) user downloads curl -sf https://binaries.nats.dev/nats-io/nats-server/[email protected] | sh
  2. The user runs nats-server --buildinfo to check that it is indeed an ARMv7 binary

Or, let's imagine we decided to tune some other flag(existing or some future one). Let's say we start to build with GOAMD64=v4. And let's say such binary crashes for some users. If we make it easier to figure out the reason - that would be great. That's why I would like to have buildinfo exposed for the users.

Answering the question "What kind of binary I am dealing with" is a valid user concern. And our users are developers, so they won't consider this info clutter... In addition to being useful for the users, it also would be useful for us - the support team, supporting paying customers.

It's either going to fail to run if its the wrong architecture or in some cases like some ARM combos you can just check it in the case that it would run:

~ $ file /usr/bin/nats-server
/usr/bin/nats-server: ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), statically linked, Go BuildID=fOeCzTD4TNmbYbIIzz2c/oD-ysM25TgDdDGwD0JwT/uRlUS4nmuvKtXW5cMzA4/pSUltOa7AgJKAdmrnUrK, stripped

It seems very contrived to have to add this imo, super niche need, minimal utility to end users. As we only support binaries released officially, none of these buildinfo checks would tell you if this is a supported version / build or not - verifying signatures, checksums against GitHub etc, those are the only ways.

@alexbozhenko alexbozhenko requested a review from wallyqs December 27, 2024 21:30
@alexbozhenko
Copy link
Contributor Author

@ripienaar I updated to hide the helpfrom the users, is it a good compromise? It may be useful for us in support, so could we plase have it there? Users won't see it, and won't be confused...

@ripienaar
Copy link
Contributor

Hiding the flag is better.

I think with 3 out of 3 reviewers voicing concern about the validity of the feature there may be an opportunity to take step back and see if this can be done in a more Idiomatic way. We don’t tend to expose useful info only on the command line

@wallyqs
Copy link
Member

wallyqs commented Dec 27, 2024

maybe this would be better suited to report via varz? there is some buildinfo in that struct too

Copy link
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

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

LGTM

@wallyqs
Copy link
Member

wallyqs commented Dec 28, 2024

Another option would be to have something like a nats-server --debug version instead of adding a new flag, or we could have nats-server version -o json like the other tools above since already possible to call either nats-server version or nats-server --version.

@ripienaar
Copy link
Contributor

varz would be better than command line but I think doing that could be very undesirable in some embedded uses, people might be embedding sensitive information into their binaries using linker flags that will then become exposed

we should hear from the support team if this is really needed for their purposes @jarretlavallee

@alexbozhenko
Copy link
Contributor Author

alexbozhenko commented Dec 28, 2024

It seems very contrived to have to add this imo, super niche need, minimal utility to end users.

Users may want to see how the binary was built, why this is not a valid use case?
File utility does not tell that this is an armv7 binary:

# file nats-server-v2.10.23-linux-arm7/nats-server
nats-server-v2.10.23-linux-arm7/nats-server: ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV), statically linked, Go BuildID=7wTsSUPSvxOlzSxeWQae/KeLmkr5FXBN0b70VaGDv/RpLkgf7Zu61EX0gl147Y/5waZ3bhqH5ySTYMbjNcr, not stripped

But buildinfo does, as well as all other knobs(and knobs that Go team will add in the future)

varz would be better than the command line

Why? With varz I can not do e.g. this: docker run -it nats:latest --buildinfo.

We removed it from the usage text so that the flag will be hidden. So your concern about "cluttering the UI" is addressed. Now it seems like you just do not want me to add it...

We can do nats-server --debug version if you think this is better. In that case, the kind of output the user may expect there will be very explicit. We can also make it output JSON, making it more idiomatic(other tools do that)

we should hear from the support team

I am on the support team as well, and I thought it is going to be useful.
Before opening a PR I discussed it with @neilalexander, and he told he would be ok to add it.

This PR is literally 20 lines, I do not know what we argue about.

@ripienaar
Copy link
Contributor

When adding information to the NATS Server we have worked hard to do so in a way that supports our eco system of tools - this means adding it to the network queryable endpoints. This is there to support tools like nats, support efforts like nats audit so we can gather this in bulk and later build reporting tools that can proactively alert users of issues, tools like Control Plane and our efforts around things like fleet management.

This is not something we take lightly, we've invested many months of development time in these efforts and its the central enabler for many tools and features. It's how we do things, it's what is idiomatic in our eco system.

Every bit of new code is code that requires maintenance and considerations around security exposure and more. So it's important that we add things that are really needed and that we consider them from all angles like their utility and security.

So to your questions:

  • You are right I do not want to add this to the CLI, simply because that is just now how we do things. We put metadata on the network where it's accessible. We already expose some build info adding more in the same way would be the correct approach if doing that satisfies the criteria of it being needed and being secure. We want to support the efforts of various teams in building tooling so doing this in an idiomatic way is best. Why would I support a way that is specifically counter to our general established approach?
  • You said this is so utilities in docker isnt needed for this and so the cli flag make sense. However the basic way that docker works just makes this unnecessary- you will automatically run the right architecture for your particular setup unless the containers are corrupt. And if you are using docker you can use standard docker utilities to view the architecture and OS of your container. And the basic reality is in most cases you simply can't run the wrong architecture binary.
  • As before multiple reviewers here have said this feature is probably not useful. I have asked other via DM and they too do not think this feature is useful, so its worth asking the question of Why?
  • Yes you are on the support team, however the insight of team members who have been on the team for years and have extensive experience in the issues WE run into are valuable. Especially given that there seems little support for this feature. Asking for input from others is not a problem.

It's valuable to bring up these kinds of feature requests in meetings like the Field Ops discussions where background information can be obtained and methodologies shared so that implementations are done in line with our expectations, a bit of discussing ahead of time will yield much better outcomes for the time you invest in PRs

As you say its 20 lines of code, it would be much more valuable if implemented in a way in line with how the project approach these kinds of features and thus supporting our wider eco system at the same time.

@Jarema
Copy link
Member

Jarema commented Dec 30, 2024

Just to ad my two cents - I feel that if that went through ADR proposal followed up by a short discussion we would reach the consensus quicker. Let's chat about it.

@alexbozhenko
Copy link
Contributor Author

Thanks, @ripienaar. Ok, closing.
Will try to bring up adding it to /varz in the oss meeting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants