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

fix: implement limit on number of hosts per line in /etc/hosts because Safari has problems with long lines, fixes #2817 #4805

Merged
merged 7 commits into from Nov 1, 2023

Conversation

rfay
Copy link
Member

@rfay rfay commented Apr 5, 2023

The Issue

I thought we were already limiting the number of hosts per line in /etc/hosts, but it turns out it required extra code.

See https://github.com/goodhosts/hostsfile

How This PR Solves The Issue

Minor change to goodhosts/hostsfile usage to implement 9-hosts-per-line limit

We may want to wait on this because of non-critical

Manual Testing Instructions

for item in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15; do 
  sudo ddev hostname a$item.ddev.site 127.0.0.1
done

Then inspect hosts file. This needs to be done on macOS, Linux, and traditional Windows.

Automated Testing Overview

We don't test this behavior I don't think because requires sudo

Related Issue Link(s)

Release/Deployment Notes

@rfay rfay requested a review from a team as a code owner April 5, 2023 14:47
@josefglatz
Copy link
Contributor

josefglatz commented Apr 6, 2023

Tested locally with real world /etc/hosts file

here are the results:

  1. ⁇ Broadcast host line was moved to first line (was before on penultimate line)
  2. ⁇ IPv6 line ::1 localhost was moved to 2nd line (was before on last line)
  3. ❌ Executing the following test command resulting in a broadcast line 255.255.255.255 with a2.ddev.site 🤨
  4. ❤️ All other entries/lines were adopted correct from my POV

Test command:

for item in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15; do 
  sudo ddev hostname a$item.ddev.site 127.0.0.1
done

Resulting /etc/hosts under macOS Ventura (project related entries where masked due to privacy)

image


Other infos

What problems does occur without this fix?

macOS Safari can not handle too long lines in /etc/hosts and will not read domains on the end of the line (e.g. 127.0.0.0.1 entry with ~100 domain names)

Apple Bug Report?

I already created a bug report. If I get any response I let you know here in this PR/issue :-)

@rfay
Copy link
Member Author

rfay commented Apr 6, 2023

Did it really change "broadcast" to "boardcast" ?

I do see this in goodhosts issue queue:

Please link your safari issue with Apple here.

@rfay
Copy link
Member Author

rfay commented Apr 6, 2023

For test purposes, here's a plain vanilla macOS starter version of /etc/hosts:

##
# Host Database
#
# localhost is used to configure the loopback interface
# when the system is booting.  Do not change this entry.
##
127.0.0.1	localhost
255.255.255.255	broadcasthost
::1             localhost

@josefglatz
Copy link
Contributor

josefglatz commented Apr 6, 2023

Did it really change "broadcast" to "boardcast" ?

Sorry, was just a typo. See the screenshot in my comment.

@josefglatz
Copy link
Contributor

I have reported the problem at Apple: Feedback-ID FB12100456

@rfay
Copy link
Member Author

rfay commented Apr 6, 2023

@rfay
Copy link
Member Author

rfay commented Apr 6, 2023

Please test with the updated build here. After running the suggested test against a stock macOS hosts file I see

255.255.255.255 broadcasthost
::1 localhost

127.0.0.1 a1.ddev.site a10.ddev.site a11.ddev.site a12.ddev.site a13.ddev.site a14.ddev.site a15.ddev.site a2.ddev.site
127.0.0.1 a3.ddev.site a4.ddev.site a5.ddev.site a6.ddev.site a7.ddev.site a8.ddev.site a9.ddev.site localhost

Default hosts file is at https://raw.githubusercontent.com/rfay/goodhostsbug/main/hosts

After changing the example to remove:

for item in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15; do    sudo ddev hostname -r a$item.ddev.site 127.0.0.1; done

I see this:

255.255.255.255 broadcasthost
::1 localhost

127.0.0.1 localhost

I don't like that the comments have been removed, see

@rfay
Copy link
Member Author

rfay commented Apr 6, 2023

@josefglatz if you have trouble again, please provide the starting version of your /etc/hosts so I can experiment with it as well. Thanks!

@rfay
Copy link
Member Author

rfay commented Apr 13, 2023

I'm not sure this is going to be an acceptable approach @josefglatz because of the rearranging of the file and loss of comments. Maybe we'll have to wait until something happens with

However, I'd love it if you could test it anyway...

@rfay rfay marked this pull request as draft April 13, 2023 16:59
@rfay rfay changed the title Implement limit on number of hosts per line in /etc/hosts Implement limit on number of hosts per line in /etc/hosts because Safari has problems with long lines Apr 26, 2023
@rfay
Copy link
Member Author

rfay commented Apr 28, 2023

No response from @josefglatz on this at this point, and I'm not very happy about the idea idea of completely rewriting hosts file. Closing for now, happy to reopen and to discuss. I'm following the issue,

@rfay rfay closed this Apr 28, 2023
@josefglatz
Copy link
Contributor

I tested a little bit. But I can't figure out how to adopt this to make this more bulletproof.

Should we add a note about this in the DDEV documentation @rfay ?

@rfay
Copy link
Member Author

rfay commented May 8, 2023

You didn't respond on the PR, would have loved to have your input on that.

I think there's code in there to warn windows users, but there's no way to warn safari users.

A docs PR would be fine.

@rfay
Copy link
Member Author

rfay commented Sep 29, 2023

Although

is being worked on at this point, I'm not sure that

will get attention, but it might!

@rfay rfay changed the title Implement limit on number of hosts per line in /etc/hosts because Safari has problems with long lines Implement limit on number of hosts per line in /etc/hosts because Safari has problems with long lines, fixes #2817 Oct 27, 2023
@rfay
Copy link
Member Author

rfay commented Oct 28, 2023

Reopened, as

@rfay rfay changed the title Implement limit on number of hosts per line in /etc/hosts because Safari has problems with long lines, fixes #2817 fix: implement limit on number of hosts per line in /etc/hosts because Safari has problems with long lines, fixes #2817 Oct 28, 2023
@github-actions github-actions bot added bugfix dependencies Pull requests that update a dependency file labels Oct 28, 2023
@github-actions
Copy link

github-actions bot commented Oct 28, 2023

@ddev ddev deleted a comment from github-actions bot Oct 29, 2023
@rfay
Copy link
Member Author

rfay commented Oct 29, 2023

@josefglatz Please take a look at this again, I think it's working great with the excellent fixes to goodhosts/hostsfile, thanks! Artifacts for testing in #4805 (comment)

@rfay rfay marked this pull request as ready for review October 29, 2023 19:02
@rfay rfay requested a review from a team as a code owner October 29, 2023 19:02
@rfay rfay requested a review from stasadev October 31, 2023 22:39
Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

Tested it on Linux:

$ cat /etc/hosts
# Host addresses
127.0.0.1 localhost
127.0.1.1 my-machine-name
::1 localhost ip6-localhost ip6-loopback
ff02::1 ip6-allnodes
ff02::2 ip6-allrouters

$ for item in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15; do sudo ddev hostname a$item.ddev.site 127.0.0.1; done
$ cat /etc/hosts
# Host addresses
127.0.0.1 localhost a1.ddev.site a2.ddev.site a3.ddev.site a4.ddev.site a5.ddev.site a6.ddev.site a7.ddev.site
127.0.0.1 a8.ddev.site a9.ddev.site a10.ddev.site a11.ddev.site a12.ddev.site a13.ddev.site a14.ddev.site a15.ddev.site
127.0.1.1 my-machine-name
::1 localhost ip6-localhost ip6-loopback
ff02::1 ip6-allnodes
ff02::2 ip6-allrouters

$ for item in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15; do sudo ddev hostname -r a$item.ddev.site 127.0.0.1; done
$ cat /etc/hosts
# Host addresses
127.0.0.1 localhost
127.0.1.1 my-machine-name
::1 localhost ip6-localhost ip6-loopback
ff02::1 ip6-allnodes
ff02::2 ip6-allrouters

@rfay rfay merged commit faff807 into ddev:master Nov 1, 2023
27 checks passed
@rfay rfay deleted the 20230405_hosts_limit branch November 1, 2023 17:07
@rfay
Copy link
Member Author

rfay commented Nov 1, 2023

@rfay
Copy link
Member Author

rfay commented Nov 1, 2023

I also did a prerelease that can be used for testing this, https://github.com/ddev/ddev/releases/tag/v1.22.5-alpha1

@josefglatz
Copy link
Contributor

Tested the feature successfully. Sorry for the delay @rfay

@rfay
Copy link
Member Author

rfay commented Nov 21, 2023

Yay, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants