Use 'crc' section for hosts file modification #42

evidolob · 2023-01-13T13:59:07Z

This PR rewrites the way of modifications of hosts file, by adding 'crc' section and perform all modification inside it.

Also it changes library which we use to parse/edit hosts file to https://github.com/areYouLazy/libhosty

Fixes crc-org/crc#2992

Sample:

# Do not remove the following line, or various programs
# that require network functionality will fail.
127.0.0.1        localhost.localdomain localhost
::1              localhost6.localdomain6 localhost6


# Added by CRC
127.0.0.1        entry1
# End of CRC section

cfergeau

Do you have a 'migration plan' in mind to go from /etc/hosts without comments to /etc/hosts with comments? I think after this change the Clean methods will no longer be able to remove crc entries which are outside of the comment block?

I also noticed libhosty's code uses Lock()/Unlock() when modifying the content of the host file (but not in the GetHostsFile* methods??), it would not hurt to do it in our code as well.

Last but not least, this removes the "9 entries per line" code on Windows, which will cause regressions on Windows. libhosty's corresponding issue is areYouLazy/libhosty#12

cfergeau · 2023-01-16T15:39:05Z

pkg/hosts/hosts.go

 		return err
 	}
-	return h.File.Flush()
+
+	line, err := h.findIP(start, end, ip)


maybe you could reuse the getHostsFileLineByIP name? Just a suggestion, both names are fine with me.

pkg/hosts/hosts.go

cfergeau · 2023-01-16T15:45:44Z

pkg/hosts/hosts.go

 				}
 			}
+


Maybe this needs to call hfl.Raw = h.File.RenderHostsFileLine(newLineNum) like what Add is doing? This is not terribly important as SaveHostsFile() is not using it for rendering the file, but would not hurt to be consistent.

pkg/hosts/hosts.go

cfergeau · 2023-01-16T15:49:24Z

pkg/hosts/hosts.go

@@ -154,7 +204,17 @@ func (h *Hosts) Contains(ip, host string) bool {
 		return false
 	}

-	return h.File.Has(ip, host)
+	lines := h.File.GetHostsFileLinesByAddress(ip)


Any reasons this is not restricted to the range owned by crc?

I keep original implementation, just update code with new lib usage, because I can’t find how Contains func used.

Maybe we can delete Contains?

It's used here https://github.com/crc-org/crc/blob/8957ea9f15e953e67b115feed2fb88b1fbae2b57/pkg/crc/adminhelper/hosts.go#L30 to avoid adding multiple times the same hostname. Given the way it's used, I'd restrict it to the comment block.
However, you added support for not adding duplicate entries directly in admin-helper, so this code could be removed from crc, and Contains is no l onger needed.

evidolob · 2023-01-19T13:56:16Z

@gbraad @praveenkumar @cfergeau @anjannath

Migrating plan could be:

During the add/clean/remove, we can check that hosts file contains crc related records outside of crc section,
then we can delete them, if there any.

Basically we need to do that only once, and rest of the time we should modify only crc section, allowing users to have own custom settings in hosts file for crc.

WDYT?

cfergeau · 2023-01-19T14:42:30Z

During the add/clean/remove, we can check that hosts file contains crc related records outside of crc section,
then we can delete them, if there any.

I'd only do this if the hosts file does not have crc's comment block. If it has comment block + entries outside of the crc section, they are likely to have been manually added by the user.
I don't know how to ensure we do this "only once", which would definitely be nice. Otherwise, what I just described would always trigger after the user runs crc cleanup; crc setup; crc start :-/

evidolob · 2023-01-23T10:28:57Z

@cfergeau I think we can check records outside CRC section during Remove, and if there are no CRC section but hosts to delete present, then we can delete them, otherwise we delete only inside CRC section.

So, if user already has crc related hosts changes, and upgrades to new version of CRC, we force him to recreate cluster(with crc delete, which will call Remove func) and there we can delete old records. And during new instance creation we add crc section.

Note: clean will delete all our changes in hosts file, including section comments.

cfergeau · 2023-01-25T16:07:32Z

@cfergeau I think we can check records outside CRC section during Remove, and if there are no CRC section but hosts to delete present, then we can delete them, otherwise we delete only inside CRC section.

So, if user already has crc related hosts changes, and upgrades to new version of CRC, we force him to recreate cluster(with crc delete, which will call Remove func) and there we can delete old records. And during new instance creation we add crc section.

Ah good suggestion, this should work! We'll be missing people doing crc delete; <upgrade crc with new admin-helper>; crc setup let's assume this is not going to be a common case/a big problem.

pkg/hosts/hosts.go

cfergeau

I still need to take a look at the last commit (the "9 entries per line" one)

pkg/hosts/hosts.go

cfergeau · 2023-01-26T10:34:02Z

pkg/hosts/hosts.go

+	h.File.HostsFileLines = newHosts
+
+	// generate raw version of the line
+	hfl.Raw = h.File.RenderHostsFileLine(newLineNum)


I find these line number occurrences in libhosty API a bit awkward, but nothing we can do here :-/

cfergeau · 2023-02-10T16:38:47Z

Now it is the test case below which fails :-/ I've tried to simplify the code to make it easier to follow, but this comes at the cost of less in place modifications, and is more in the form of "let's rewrite everything in the crc section which corresponds to this IP", which may be something you want to avoid? With my changes the test case passes, but I've broken in the process the handling of different IPs in the CRC section :( need to try a bit more work on it.
(broken simpler code is at cfergeau@cb6e4c5)

--- a/pkg/hosts/hosts_test.go
+++ b/pkg/hosts/hosts_test.go
@@ -56,6 +56,24 @@ func TestAddMoreThen9Hosts(t *testing.T) {
 	assert.Equal(t, hostsTemplate+eol()+crcSection("127.0.0.1        entry9", "127.0.0.1        entry1 entry10 entry2 entry3 entry4 entry5 entry6 entry7 entry8")+eol(), string(content))
 }
 
+func TestAddMoreThan18Hosts(t *testing.T) {
+	dir, err := os.MkdirTemp("", "hosts")
+	assert.NoError(t, err)
+	defer os.RemoveAll(dir)
+
+	hostsFile := filepath.Join(dir, "hosts")
+	assert.NoError(t, os.WriteFile(hostsFile, []byte(hostsTemplate), 0600))
+
+	host := hosts(t, hostsFile)
+
+	assert.NoError(t, host.Add("127.0.0.1", []string{"entry0"}))
+	assert.NoError(t, host.Add("127.0.0.1", []string{"entry1", "entry2", "entry3", "entry3", "entry4", "entry5", "entry6", "entry7", "entry8", "entry9", "entry10", "entry11", "entry12", "entry13", "entry14", "entry15", "entry16", "entry17", "entry18", "entry19", "entry20"}))
+
+	content, err := os.ReadFile(hostsFile)
+	assert.NoError(t, err)
+	assert.Equal(t, hostsTemplate+eol()+crcSection("127.0.0.1        entry17 entry18 entry19 entry2 entry20 entry3 entry4 entry5 entry6 entry7 entry8 entry9", "127.0.0.1        entry0 entry1 entry10 entry11 entry12 entry13 entry14 entry15 entry16")+eol(), string(content))
+}
+
 func TestAddMoreThen9HostsInMultipleLines(t *testing.T) {
 	dir, err := os.MkdirTemp("", "hosts")
 	assert.NoError(t, err)

cfergeau · 2023-02-13T13:28:17Z

I spent more time on this code, https://github.com/cfergeau/admin-helper/tree/change-hosts-modification2 keeps the same behaviour as what you did, in place modification of the file as much as possible. I've tried to make Add easier to read/review by moving some of the low-level logic to new helper code. This also fixes the test case I mentioned before.

evidolob · 2023-02-16T10:04:00Z

@cfergeau I've cherry-picked your commits

cfergeau · 2023-02-20T14:10:06Z

For this one, if we wanted to go with a nice history with smaller commit, this could look like

replace goodhosts with libhosty
re-add 9 hosts per line limit
perform clean only if crc section present
add migration code for old hosts file
and possibly a separate commit for not adding duplicate hosts (add this code in a commit similar to "Rework unicity check")

This would gradually add each feature that are important to us, and should make each commit smaller, and easier to understand.
However, with the current history, reordering/merging the commits will cause some significant headaches, so I'm inclined to squash them all in a single commit. What do you think?

evidolob · 2023-02-21T08:01:27Z

@cfergeau I also prefer to squash commit in to single one

evidolob · 2023-05-25T12:36:23Z

@cfergeau I have, finally, time to work on this.

So what you suggest to do with this PR, squash all commits in single one?

gbraad · 2023-11-24T09:54:47Z

@crc-org/crc-team Any thoughts on this?

cfergeau · 2023-11-24T12:54:03Z

@crc-org/crc-team Any thoughts on this?

Better to squash the commits, we need to finally merge this, but this will require careful testing imo.

replace goodhosts with libhosty re-add 9 hosts per line limit perform clean only if crc section present add migration code for old hosts file Co-Authored-By: Christophe Fergeau <[email protected]>

evidolob · 2023-11-27T09:44:30Z

Build has lint error, it should be fixed with #44

gbraad · 2023-11-29T08:39:01Z

but this will require careful testing imo.

Is there something we can do automated. Eventually this needs a test set.
/cc: @jsliacan

cfergeau · 2023-12-01T15:57:44Z

Took another look at this, and it's still looking fine. Tested it with microshift on a mac and it behaved as expected (created multiple routes until a new line is created, removed some of these routes, readded them, then ran crc cleanup).

Preexisting .crc.testing entries in the /etc/hosts file are not removed, but it seems normal (tried with crc-admin-helper clean), and if not, it can be addressed in a follow-up in my opinion.

Preexisting issue, crc delete does not remove entries from /etc/hosts, it should, as the needed entries will be readded when crc start recreates the cluster.

evidolob requested review from gbraad, praveenkumar, cfergeau and anjannath January 13, 2023 13:59

evidolob force-pushed the change-hosts-modification branch from 44d227c to 22266ba Compare January 13, 2023 14:05

cfergeau reviewed Jan 16, 2023

View reviewed changes

evidolob force-pushed the change-hosts-modification branch from bbad7f0 to 29df4d1 Compare January 17, 2023 09:31

cfergeau reviewed Jan 25, 2023

View reviewed changes

pkg/hosts/hosts.go Outdated Show resolved Hide resolved

cfergeau reviewed Jan 25, 2023

View reviewed changes

pkg/hosts/hosts.go Outdated Show resolved Hide resolved

cfergeau reviewed Jan 26, 2023

View reviewed changes

evidolob requested a review from cfergeau February 7, 2023 14:39

gbraad mentioned this pull request Nov 24, 2023

Failed to query DNS from host: lookup api.crc.testing crc-org/crc#3916

Closed

evidolob force-pushed the change-hosts-modification branch 2 times, most recently from ddbda9c to c853707 Compare November 27, 2023 09:19

Use 'crc' section for hosts file modification

6b9e4fa

replace goodhosts with libhosty re-add 9 hosts per line limit perform clean only if crc section present add migration code for old hosts file Co-Authored-By: Christophe Fergeau <[email protected]>

evidolob force-pushed the change-hosts-modification branch from c853707 to 6b9e4fa Compare November 27, 2023 09:24

cfergeau approved these changes Dec 1, 2023

View reviewed changes

cfergeau merged commit 763a6b9 into crc-org:master Dec 1, 2023
1 check failed

evidolob deleted the change-hosts-modification branch December 4, 2023 07:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use 'crc' section for hosts file modification #42

Use 'crc' section for hosts file modification #42

evidolob commented Jan 13, 2023 •

edited

Loading

cfergeau left a comment

cfergeau Jan 16, 2023

cfergeau Jan 16, 2023

cfergeau Jan 16, 2023

evidolob Jan 17, 2023

cfergeau Jan 18, 2023

evidolob commented Jan 19, 2023

cfergeau commented Jan 19, 2023

evidolob commented Jan 23, 2023

cfergeau commented Jan 25, 2023

cfergeau left a comment

cfergeau Jan 26, 2023

cfergeau commented Feb 10, 2023 •

edited

Loading

cfergeau commented Feb 13, 2023

evidolob commented Feb 16, 2023

cfergeau commented Feb 20, 2023

evidolob commented Feb 21, 2023

evidolob commented May 25, 2023

gbraad commented Nov 24, 2023

cfergeau commented Nov 24, 2023

evidolob commented Nov 27, 2023

gbraad commented Nov 29, 2023

cfergeau commented Dec 1, 2023

Use 'crc' section for hosts file modification #42

Use 'crc' section for hosts file modification #42

Conversation

evidolob commented Jan 13, 2023 • edited Loading

cfergeau left a comment

Choose a reason for hiding this comment

cfergeau Jan 16, 2023

Choose a reason for hiding this comment

cfergeau Jan 16, 2023

Choose a reason for hiding this comment

cfergeau Jan 16, 2023

Choose a reason for hiding this comment

evidolob Jan 17, 2023

Choose a reason for hiding this comment

cfergeau Jan 18, 2023

Choose a reason for hiding this comment

evidolob commented Jan 19, 2023

cfergeau commented Jan 19, 2023

evidolob commented Jan 23, 2023

cfergeau commented Jan 25, 2023

cfergeau left a comment

Choose a reason for hiding this comment

cfergeau Jan 26, 2023

Choose a reason for hiding this comment

cfergeau commented Feb 10, 2023 • edited Loading

cfergeau commented Feb 13, 2023

evidolob commented Feb 16, 2023

cfergeau commented Feb 20, 2023

evidolob commented Feb 21, 2023

evidolob commented May 25, 2023

gbraad commented Nov 24, 2023

cfergeau commented Nov 24, 2023

evidolob commented Nov 27, 2023

gbraad commented Nov 29, 2023

cfergeau commented Dec 1, 2023

evidolob commented Jan 13, 2023 •

edited

Loading

cfergeau commented Feb 10, 2023 •

edited

Loading