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 nsswitch.conf to the orderer and peer docker images #1150

Merged
merged 2 commits into from
May 22, 2020

Conversation

silveraid
Copy link
Contributor

Go's netgo implementation currently does not respect hostname overrides
defined in /etc/hosts if the /etc/nsswitch.conf does not exists.

Signed-off-by: Frank Felhoffer frank.felhoffer@securekey.com

Type of change

  • Bug fix

Description

Golang does not use the content of the /etc/hosts file at all if there is no /etc/nsswitch.conf with some minimal configuration in place. Usually there is one, but at some point of time Alpine Linux decided to remove this file from their distribution and ever since the applications written in Go placed into an alpine based docker image struggle with this.

Users who are using the /etc/hosts file to override hostnames are unable to use the new images unless they add the nsswitch.conf on their own, either by mounting it into the image or creating they own images.

Additional details

Endless threads on the Internet suggests the applied solution:
golang/go#22846
golang/go#35305

The pull request has been tested by rebuilding the images and taking the look if the /etc/nsswitch.conf is now present.

Related issues

N/A

@silveraid silveraid requested a review from a team as a code owner April 22, 2020 20:17
Copy link
Contributor

@sykesm sykesm left a comment

Choose a reason for hiding this comment

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

I don't really have an opinion about whether or not the file should be added to the base image. That said, the change being suggested addresses an issue by side effect. The change is subtle and I can assure you that without a comment or any sort of check to ensure it remains, it will likely get removed.

I also don't think that a conditional shell test is needed when adding a file to a base. If the expected contents are known, the file should simply be created. If the contents are unknown, this change may not help at all if the base image creates an nsswitch.conf with hosts: dns and drops files.

Finally, in master and release-2.x, I believe the peer and orderer are built by default as dynamically linked executables with cgo enabled. Is there a reason why forcing the cgo resolver via environment variable isn't a viable option to use a deployment time?

@silveraid
Copy link
Contributor Author

Yes, so I added the fix the way it is

RUN [ ! -e /etc/nsswitch.conf ] && echo 'hosts: files dns' > /etc/nsswitch.conf

because by some reason people consider this as the current de-facto work-around for Alpine-based containers, this even includes the official go image itself as well ;-)

https://github.com/docker-library/golang/blob/a4deea14ce3306822bb9352ccf124af8c0eea257/1.13/alpine3.10/Dockerfile#L9

I will update my patch with the official comment, I omitted it to not to break the look and feel of the Dockerfile itself.

I will give it a go to the cgo resolver to see how it goes with that.

Go's netgo implementation currently does not respect hostname overrides
defined in /etc/hosts if the /etc/nsswitch.conf does not exists.

Signed-off-by: Frank Felhoffer <frank.felhoffer@securekey.com>
@mastersingh24
Copy link
Contributor

While this change seems ok, we've been trying to greatly simply the images and really the images are provided as a convenience mechanism for people to quickly get started.

@denyeart
Copy link
Contributor

If this is the de facto solution as evidenced by the inclusion in the official Go image, seems reasonable to include in the Fabric images. I'll approve but wait for other opinions before merging.

denyeart
denyeart previously approved these changes Apr 29, 2020
images/peer/Dockerfile Outdated Show resolved Hide resolved
I agree

Signed-off-by: Frank Felhoffer <frank.felhoffer@securekey.com>
Co-authored-by: Gari Singh <gari.r.singh@gmail.com>
@mastersingh24 mastersingh24 merged commit bdfe1af into hyperledger:master May 22, 2020
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.

4 participants