Clean up hostnames when removing project data, closes #831 #1017

andrewfrench · 2018-07-26T18:46:04Z

The Problem/Issue/Bug:

#831: The user's hosts file eventually becomes polluted after having many project hostnames added to it. Currently, ddev makes no effort to remove a hostname entry after it has been added.

How this PR Solves The Problem:

Add the ability to remove a hostname via the ddev hostname command:

ddev hostname --remove my.host 127.0.0.1

Remove hostname entries from the user's hosts file on ddev remove --remove-data. Removing a hostname on ddev remove --remove-data and ddev hostname --remove mimic the output and structure of existing hostname addition code.

User-facing output in ddev hostname has been improved.

Manual Testing Instructions:

Add a hostname via ddev hostname test.host 1.2.3.4 and ensure the hostname has been added to the hosts file
Remove the same hostname via ddev hostname test.host 1.2.3.4 --remove and ensure the hostname has been removed from the hosts file

Test the above steps with and without sudo and the --json-output to inspect output and error logging.

Add a new and unique FQDN or hostname to a ddev project's .ddev/config.yaml
Execute ddev start on the project and ensure the new hostnames have been added to the hosts files
Execute ddev remove --remove-data on the project and ensure the project's hostnames have been removed from the hosts file

Test the above steps with and without DRUD_NONINTERACTIVE to see non-interactive instruction logging.

Automated Testing Overview:

Release/Deployment notes:

rfay · 2018-07-26T18:49:10Z

Quick glance looks fantastic. How about a --all which would only remove those that match our default domain (and maybe that aren't currently active? or could only be executed when nothing is running?)

rickmanelius · 2018-07-26T19:19:10Z

Wow... as a side comment, thank you for such thoroughness on the coverage as well as the testing instructions.

rickmanelius

Preparation

git checkout 2de58d64
make darwin
ddev version => v1.0.0-8-g2de58d64

Testing

Manual Add/Remove
- Add:
  - No sudo: "Could not write hosts file: open /etc/hosts: permission denied"
  - sudo ddev hostname test.host 1.2.3.4
    - Verfied new line "1.2.3.4 test.host" in /etc/hosts
  - sudo ddev hostname test2.host 1.2.3.4
    - Verfied line updated to "1.2.3.4 test.host test2.host" in /etc/hosts
  - sudo ddev hostname test.host 1.2.3.4
    - Receive message "Hostname already exists in hosts file"
- Remove:
  - No sudo: "Could not write hosts file: open /etc/hosts: permission denied"
    - Note this one doesn't appear as an error while the "add" variation is colored red.
  - sudo ddev hostname test2.host 1.2.3.4 --remove
    - Verfied edit line "1.2.3.4 test.host" in /etc/hosts
  - sudo ddev hostname test2.host 1.2.3.4 --remove
    - Verfied no more entries for "1.2.3.4"
Automatic with Site Start/Stop/Remove
- Deleted all *.ddev.local 127.0.0.1 entries in /etc/hosts
- Normal ddev config w/stock D7.59 site
  - Verified base entry to /etc/hosts
- Uncommented the stock additional hostname and FQDN entries in config.yml
  - Verified additional entries: "127.0.0.1 drupal-7.59.ddev.local example.com sub1.example.com somename.ddev.local someothername.ddev.local"
- Created another site and verified it's entry into /etc/hosts
- ddev rm --all
  - No modification to /etc/hosts
- ddev rm --remove-data --all
  - This doesn't seem happy, resulting in "Illegal option combination: --all and --remove-data" with ddev rm
- ddev rm --remove-data
  - Verified the removal of /etc/host entries for the project

Recommendations

We probably want to mention in ddev hostname --help that this has security implications and requires sudo/root access.
I'm not sure why "Illegal option combination: --all and --remove-data" with ddev rm
I'm a fairly avid ddev rm --remove-data user and think that all the sudo requests and worry that all the /etc/hosts sudo requests will become annoying. Also thinking of DDEV-UI as having to get prompted there as well. I'm curious to release this as is and see what the feedback is, but I'm guessing a flag might be less invasive.

andrewfrench · 2018-07-26T20:38:12Z

Agreed, I'll add this here.
I disallowed ddev remove --remove-data --all because that seemed to be an exceptionally destructive command for users that are doing real work with ddev. I'm willing to revisit this, but giving a user the ability to blow away everything seemed dangerous.
I can see that becoming annoying. I specifically put this under the --remove-data option because I'm an avid ddev rm user and didn't want to constantly type my password for sudo, either.

rickmanelius · 2018-07-26T20:41:11Z

Thanks for the follow-ups. Everything you said makes sense and I'm +1

andrewfrench · 2018-07-26T22:27:13Z

@rickmanelius, a failed host name removal is now logged as an error and returns 1, just like a failed addition.

@rfay, I added the --remove-inactive option, which accepts no arguments and actually required a bit more work than I was expecting. Let me know if this behaves as you expected.

rfay · 2018-07-27T00:21:01Z

;TLDR:

it doesn't work without sudo, figure out how to notify the user and don't tell them it did a bunch of stuff when it's going to fail.
Don't remove hostnames that we don't have a reason to guess we put in there (*.ddev.local).
Remember that Windows users may need a little prompting on using this with sudo, which I think most of them probably have now.

First thing I wanted to do was use --fire-bazooka (and thanks for adding that). But:

$ ddev hostname --remove-inactive
Hostname localhost at 127.0.0.1 is active, preserving
Removed hostname randyfay.ddev.local at 127.0.0.1
Removed hostname d8git.ddev.local at 127.0.0.1
Removed hostname one.ddev.local at 127.0.0.1
Removed hostname two.ddev.local at 127.0.0.1
Removed hostname testoverride.ddev.local at 127.0.0.1
Removed hostname a.b.c.ddev.local at 127.0.0.1
Removed hostname coe.ddev.local at 127.0.0.1
Removed hostname typo3-v9-composer-from-instructions.ddev.local at 127.0.0.1
Removed hostname d8composer.ddev.local at 127.0.0.1
Removed hostname peakhrconsulting.com.ddev.local at 127.0.0.1
Removed hostname rfay-drupal8.ddev.local at 127.0.0.1
Removed hostname ddevt3dd18.ddev.local at 127.0.0.1
Removed hostname typo3-v9-composer.ddev.local at 127.0.0.1
Removed hostname satirtraining.ddev.local at 127.0.0.1
Removed hostname junker999.ddev.local at 127.0.0.1
Removed hostname d7git.ddev.local at 127.0.0.1
Removed hostname wordpress.ddev.local at 127.0.0.1
Removed hostname d8incontainer.ddev.local at 127.0.0.1
Removed hostname gchistory.ddev.local at 127.0.0.1
Removed hostname typo3-v8-composer.ddev.local at 127.0.0.1
Removed hostname junk.ddev.local at 127.0.0.1
Removed hostname junk.d8git.com at 127.0.0.1
Removed hostname mysite.com at 127.0.0.1
Removed hostname somesite.yoursite.com at 127.0.0.1
Removed hostname anothersite.yoursite.com at 127.0.0.1
Removed hostname junk.com at 127.0.0.1
Removed hostname drupal-kickstart.ddev.local at 127.0.0.1
Removed hostname drupal8.ddev.local at 127.0.0.1
Removed hostname backdrop.ddev.local at 127.0.0.1
Removed hostname d6git.ddev.local at 127.0.0.1
Removed hostname d7-kickstart.ddev.local at 127.0.0.1
Removed hostname wordpress4.ddev.local at 127.0.0.1
Removed hostname d8-umami.ddev.local at 127.0.0.1
Removed hostname junktypo3.ddev.local at 127.0.0.1
Removed hostname t3git.ddev.local at 127.0.0.1
Removed hostname testpkgdrupal7.ddev.local at 127.0.0.1
Removed hostname testvalidtestsitewordpress.ddev.local at 127.0.0.1
Could not write hosts file: open /etc/hosts: permission denied

Note that it shows many successes, but nothing actually worked (as it tells us at the very end)

I'd hate to have ddev hostname have to sudo itself; maybe it can just check for writeablility before taking action (using goodhosts doing a no-op) and if it can't do it, suggest the correct command?

Also, remember that this is a really serious problem for windows users, so we may have to help them along the way on it.

FInally, really important: If we don't have a reason to believe ddev put the hostname in there, we shouldn't remove it (That's why I was suggesting filtering on ddev.local - or the configured TLD.)

I saw this:

Removed hostname something.com at 127.0.0.1
Removed hostname somethingelse.com at 127.0.0.1

and realized that ddev could have cleaned up some things it absolutely didn't own.

andrewfrench · 2018-07-27T16:34:27Z

Good points @rfay, this will now only remove ddev.local domains (at least until we can get a persistent record of all projects to work from) and attempts a trivial write first to confirm permissions.

rfay

Took this for a spin and people will love it. I had minor, minor request(s) on user prompts, but the code looks good and it's a great improvement. I guess we should make sure we're sure that ddev rm shouldn't remove the hosts entry. On Windows, with capacity for only 10 entries, that might be important. OTOH, we can probably let them fend for themselves with the ddev hostname -A command. Can we add an alias "--fire-bazooka"? (drush registry-rebuild still has a --fire-bazooka to do everything it can think of to do :) )

rfay · 2018-07-27T21:29:34Z

cmd/ddev/cmd/hostname.go

+		// Attempt to write the hosts file first to catch any permissions issues early
+		if err := hosts.Flush(); err != nil {
+			rawResult := make(map[string]interface{})
+			detail := fmt.Sprintf("Could not write hosts file: %v", err)


Let's just prompt them with what to do about it,

Please use "sudo hostname " or execute with administrative privileges.

rfay · 2018-07-27T21:34:24Z

cmd/ddev/cmd/hostname.go

 	},
 }

+// addHostname encapsulates the logic of adding a hostname to the system's hosts file.
+func addHostname(hosts goodhosts.Hosts, ip, hostname string) {


Seems like a relic of long ago that all this has remained in cmd instead of getting refactored into pkg. OK with me, and this is a very, very isolated command.

rfay · 2018-07-27T21:37:02Z

pkg/ddevapp/ddevapp.go

@@ -887,13 +887,18 @@ func (app *DdevApp) Down(removeData bool) error {
 		return fmt.Errorf("failed to remove %s: %v", app.GetName(), err)
 	}

-	// Remove data/database if we need to.
+	// Remove data/database/hostname if we need to.
 	if removeData {


I think it's worth just a small conversation on whether we should just do this on regular rm as well. I guess the argument against is that people hit sudo too often.

rfay · 2018-07-30T19:06:21Z

Still looks great to me. Thanks!

rickmanelius

Hi @andrewfrench. Thanks for your work on this. I confirmed that the bad combination of "Illegal option combination: --all and --remove-data", although I'm still not sure it's an invalid use case. Still, we can handle that in a following PR as that is not necessary here. I can confirm the behavior of a previous review as well as the --remove-inactive removing inactive *.ddev.local. It's worth noting that FQDN entries will not get purged this way, but that's not really possible until we have a metadata storage for project info.

Looks great! Let's get merged in.

andrewfrench added this to the v1.1.0 milestone Jul 26, 2018

andrewfrench self-assigned this Jul 26, 2018

andrewfrench requested review from rfay and rickmanelius July 26, 2018 18:46

rickmanelius approved these changes Jul 26, 2018

View reviewed changes

andrewfrench changed the title ~~Clean up hostnames when removing project data~~ Clean up hostnames when removing project data, closes #831 Jul 27, 2018

rfay approved these changes Jul 27, 2018

View reviewed changes

andrewfrench added 12 commits July 30, 2018 10:02

Add the ability to remove a hostname via ddev hostname.

30a0179

Remove an app's hostnames on ddev remove --remove-data.

1813092

Logging improvements.

14dcef8

Update function comments.

9456281

Fix typo.

60415d3

Fix spacing.

b8fd3f1

Add the ability to remove inactive host names, address comments.

a4b552e

Ignore host names that don't have the ddev.local TLD.

463d390

Try to rewrite the hosts file before modification to catch perms issues.

aceaaa3

Update error message to suggest sudo/admin.

6453f09

Add --fire-bazooka command.

42cf050

Improve logging, remove redundant localhost in preserved list.

683eaae

andrewfrench force-pushed the 2018-07-25_hostname-cleanup branch from bd135db to 683eaae Compare July 30, 2018 16:02

rickmanelius self-requested a review July 30, 2018 17:06

Hide flag name.

e8dbbb7

Satisfy linters.

f96d1cc

rickmanelius approved these changes Jul 30, 2018

View reviewed changes

andrewfrench merged commit 165ba99 into ddev:master Jul 30, 2018

andrewfrench deleted the 2018-07-25_hostname-cleanup branch July 30, 2018 20:57

rickmanelius mentioned this pull request Aug 15, 2018

v1.1.0 Release Checklist Due 2018-08-15 #994

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up hostnames when removing project data, closes #831 #1017

Clean up hostnames when removing project data, closes #831 #1017

andrewfrench commented Jul 26, 2018 •

edited by rickmanelius

rfay commented Jul 26, 2018

rickmanelius commented Jul 26, 2018

rickmanelius left a comment

andrewfrench commented Jul 26, 2018

rickmanelius commented Jul 26, 2018

andrewfrench commented Jul 26, 2018 •

edited

rfay commented Jul 27, 2018 •

edited

andrewfrench commented Jul 27, 2018

rfay left a comment

rfay Jul 27, 2018

rfay Jul 27, 2018

rfay Jul 27, 2018

rfay commented Jul 30, 2018

rickmanelius left a comment

Clean up hostnames when removing project data, closes #831 #1017

Clean up hostnames when removing project data, closes #831 #1017

Conversation

andrewfrench commented Jul 26, 2018 • edited by rickmanelius

The Problem/Issue/Bug:

How this PR Solves The Problem:

Manual Testing Instructions:

Automated Testing Overview:

Related Issue Link(s):

Release/Deployment notes:

rfay commented Jul 26, 2018

rickmanelius commented Jul 26, 2018

rickmanelius left a comment

Choose a reason for hiding this comment

Preparation

Testing

Recommendations

andrewfrench commented Jul 26, 2018

rickmanelius commented Jul 26, 2018

andrewfrench commented Jul 26, 2018 • edited

rfay commented Jul 27, 2018 • edited

andrewfrench commented Jul 27, 2018

rfay left a comment

Choose a reason for hiding this comment

rfay Jul 27, 2018

Choose a reason for hiding this comment

rfay Jul 27, 2018

Choose a reason for hiding this comment

rfay Jul 27, 2018

Choose a reason for hiding this comment

rfay commented Jul 30, 2018

rickmanelius left a comment

Choose a reason for hiding this comment

andrewfrench commented Jul 26, 2018 •

edited by rickmanelius

andrewfrench commented Jul 26, 2018 •

edited

rfay commented Jul 27, 2018 •

edited