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

build: prevent go from linking libresolv dynamically. #4394

Merged
merged 8 commits into from
Sep 25, 2023

Conversation

jiceatscion
Copy link
Contributor

@jiceatscion jiceatscion commented Sep 19, 2023

This is default (https://pkg.go.dev/[email protected]#hdr-Name_Resolution). It creates an indirect dependency on a fresh version of glibc. As a result, it prevents our binaries from running on not-so-fesh docker base images.


This change is Reviewable

jiceatscion and others added 3 commits September 19, 2023 15:51
This is default (https://pkg.go.dev/[email protected]#hdr-Name_Resolution).
It creates an indirect dependency on a fresh version of glibc.
As a result, it prevents our binaries from running on not-so-fesh docker
images.
Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @jiceatscion)

a discussion (no related file):
Is this really required?
I think you can just build against an older glibc version that is supported by your target base image.
E.g., you could use https://github.com/aspect-build/gcc-toolchain which uses glibc 2.26.

Which docker base image are you trying to use?


Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @jiceatscion)

a discussion (no related file):

Previously, oncilla (Dominik Roos) wrote…

Is this really required?
I think you can just build against an older glibc version that is supported by your target base image.
E.g., you could use https://github.com/aspect-build/gcc-toolchain which uses glibc 2.26.

Which docker base image are you trying to use?

(I guess the underlying issue is that your locally installed c compiler is using newer glibc version than what is supported)


Copy link
Contributor Author

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @oncilla)

a discussion (no related file):

Previously, oncilla (Dominik Roos) wrote…

(I guess the underlying issue is that your locally installed c compiler is using newer glibc version than what is supported)

This is no more required than switching to sqlite_modernc was. Same reason. Yes, there are other ways to solve the problem: cross build to target older run-times, or build docker images compatible with the build system. The former would make sense if we intended to build for a specific set of platforms. The latter would be more of the right thing to do if we intend to deliver target-agnostic source-code that one can build on and for one's choice's system.

At the moment, I think that target-agnostic is our focus. We (SCION Association) have no near-term plan to include support packages for specific target platforms. So, adding cross-build support right now wouldn't be a great use of my time (but possibly that's something Anapaya would have a vested interest in).

Making docker images compatible with the build system however isn't immediate either. In the meantime, I, for one, cannot run integration tests anymore. A problem that I can solve for myself and everyone else similarly affected by removing the issue's trigger at basically zero cost to anybody. Any particular reason to want libresolv?


Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @jiceatscion)

a discussion (no related file):

(but possibly that's something Anapaya would have a vested interest in).

Not particularly, we build and distribute the packages in our custom way. But internally, we use the linked aspect project to guarantee hermetic builds and do not rely on the host installed compiler.

At the moment, I think that target-agnostic is our focus.

I remember OSX having issues with the Go only DNS resolver.
(https://danp.net/posts/macos-dns-change-in-go-1-20/)
Maybe this has been resolved in the meantime (but I'm also not an OSX user...)


Copy link
Contributor Author

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @oncilla)

a discussion (no related file):

Previously, oncilla (Dominik Roos) wrote…

(but possibly that's something Anapaya would have a vested interest in).

Not particularly, we build and distribute the packages in our custom way. But internally, we use the linked aspect project to guarantee hermetic builds and do not rely on the host installed compiler.

At the moment, I think that target-agnostic is our focus.

I remember OSX having issues with the Go only DNS resolver.
(https://danp.net/posts/macos-dns-change-in-go-1-20/)
Maybe this has been resolved in the meantime (but I'm also not an OSX user...)

That'd be a drag if that had an effect on our own users, but I don't see the use case here. Are there people building SCION for MacOS? If that works, then I guess they won't be using the containers, so indeed my change isn't even useful. Unfortunately there doesn't seem to be a way to put conditional expressions in .bazelrc. However, may be someone building on MacOS could override that line in their own ~/.bazelrc?


Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @jiceatscion)

a discussion (no related file):

Previously, jiceatscion wrote…

That'd be a drag if that had an effect on our own users, but I don't see the use case here. Are there people building SCION for MacOS? If that works, then I guess they won't be using the containers, so indeed my change isn't even useful. Unfortunately there doesn't seem to be a way to put conditional expressions in .bazelrc. However, may be someone building on MacOS could override that line in their own ~/.bazelrc?

There have been in the past. I'm not sure if they are still around. I just brought it up because your mentioning being target agnostic, but AFAIU using Go only supports a subset of the targets


Copy link
Contributor Author

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @oncilla)

a discussion (no related file):

Previously, oncilla (Dominik Roos) wrote…

There have been in the past. I'm not sure if they are still around. I just brought it up because your mentioning being target agnostic, but AFAIU using Go only supports a subset of the targets

Yeah... I wasn't saying super-portable. It's still mostly Linux, I think.


Copy link
Contributor Author

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @oncilla)

a discussion (no related file):

Previously, jiceatscion wrote…

Yeah... I wasn't saying super-portable. It's still mostly Linux, I think.

Good news. I found how one puts platform specific flags in .bazelrc. Problem solved. No MacOS user harmed by this change.


Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @oncilla)

a discussion (no related file):

Previously, jiceatscion wrote…

Good news. I found how one puts platform specific flags in .bazelrc. Problem solved. No MacOS user harmed by this change.

This seems ok to me.

For what it's worth, I don't think DNS is critical in any of the SCION binaries. While some configuration files do ostensibly support host names, these names are only resolved at startup. Functionally, this is mostly equivalent to resolving the names manually at the time of configuration.
I'm not sure the special case for OSX is really necessary, but as you found a very simple solution it doesn't seem to hurt keeping it now.


Copy link
Contributor Author

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf and @oncilla)

a discussion (no related file):

Previously, matzf (Matthias Frei) wrote…

This seems ok to me.

For what it's worth, I don't think DNS is critical in any of the SCION binaries. While some configuration files do ostensibly support host names, these names are only resolved at startup. Functionally, this is mostly equivalent to resolving the names manually at the time of configuration.
I'm not sure the special case for OSX is really necessary, but as you found a very simple solution it doesn't seem to hurt keeping it now.

Thanks! Dominik?


Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @matzf)

a discussion (no related file):

Previously, jiceatscion wrote…

Thanks! Dominik?

It's not the route I would go, but I also don't have strong opinions.
Resolved and :lgtm:


Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)

@matzf
Copy link
Contributor

matzf commented Sep 25, 2023

Danger danger: I'm merging despite failing buildkite run; the failing is only due to the updated buildkite runner stack, which will require some small fixes in our pipeline to work (see #4397). As this change here is also required to make the tests run on the new stack, I'm merging this now.

@matzf matzf merged commit ae14b74 into scionproto:master Sep 25, 2023
3 of 4 checks passed
@jiceatscion jiceatscion deleted the no_libresolv branch September 27, 2023 09:14
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.

3 participants