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 new etchosts package #998

Merged
merged 5 commits into from
Apr 21, 2022
Merged

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Apr 11, 2022

Add a new libnetwork/etchosts package to manage reading/writing hosts files.
This package exports two functions New() and Add(). See the godoc comments
on the functions.

Both podman and buildah should use this function to make sure files are
generated identical.

libnetwork/util: add GetNetworkIPs()

Add helper function to get the first ipv4 and ipv6 from the network
status. Since this is stored as a map, calling this function multiple
times is not deterministic. The caller (Podman) should store this result
to make sure it will keep using the same ips.

add base_hosts_file field to containers.conf

base_hosts_file can be used to overwrite the default base host file
/etc/hosts which is used to copy hosts entries from this file into the
containers /etc/hosts file. As special value "image" can be used to copy
the entries from the image hosts file or an empty string "" to not use a
base file at all.

Ref containers/podman#13277
Ref containers/podman#13748

add host_containers_internal_ip to containers.conf

Set the ip for the host.containers.internal entry in the containers /etc/hosts
file. This can be set to "none" to disable adding this entry. By default it
will automatically choose the host ip.

Also add a function to get the correct host.containers.internal ip. This
should be used by podman and buildah and then passed to the New()
function.

Ref containers/podman#13224

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Luap99 Luap99 changed the title add new etchosts package WIP: add new etchosts package Apr 11, 2022
@mheon
Copy link
Member

mheon commented Apr 11, 2022

Empty string for none for base_hosts_file feels strange - "" feels like it should be default, and we should have a "none" sigil for no base file?

Will review the rest after lunch

@Luap99
Copy link
Member Author

Luap99 commented Apr 11, 2022

@mheon No rush. I want to work on the podman/buildah changes now before we merge this to make sure I have not missed something or need to tweak the interfaces a bit.

@rhatdan
Copy link
Member

rhatdan commented Apr 11, 2022

:LGTM other then a few nits.

@Luap99 Luap99 force-pushed the etchosts branch 6 times, most recently from 330f607 to b94788f Compare April 19, 2022 09:29
@Luap99 Luap99 changed the title WIP: add new etchosts package add new etchosts package Apr 19, 2022
@Luap99
Copy link
Member Author

Luap99 commented Apr 19, 2022

Podman PR: containers/podman#13918
Buildah PR: containers/buildah#3917

I think I still have to change and add some tests in podman/buildah but this is ready. @mheon PTAL

@Luap99
Copy link
Member Author

Luap99 commented Apr 20, 2022

@mheon @rhatdan PTAL

@rhatdan
Copy link
Member

rhatdan commented Apr 20, 2022

LGTM

@Luap99
Copy link
Member Author

Luap99 commented Apr 21, 2022

@vrothberg PTAL

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Other than the docs nit, LGTM.

Nice work!

pkg/config/containers.conf Outdated Show resolved Hide resolved
Luap99 added 5 commits April 21, 2022 11:39
Add a new libnetwork/etchosts package to manage reading/writing hosts files.
This package exports four functions New(), Add(), AddIfExists() and Remove().
See the godoc comments on the functions.

Both podman and buildah should use this functions to make sure files are
generated identical.

Signed-off-by: Paul Holzinger <[email protected]>
base_hosts_file can be used to overwrite the default base host file
/etc/hosts which is used to copy hosts entries from this file into the
containers /etc/hosts file. As special value "image" can be used to copy
the entries from the image hosts file or "none" to not use a base file
at all. IF the value is empty we should use /etc/hosts as default.

Ref containers/podman#13277
Ref containers/podman#13748

Signed-off-by: Paul Holzinger <[email protected]>
Set the ip for the host.containers.internal entry in the containers /etc/hosts
file. This can be set to "none" to disable adding this entry. By default it
will automatically choose the host ip.

Also add a function to get the correct host.containers.internal ip. This
should be used by podman and buildah and then passed to the New()
function.

Ref containers/podman#13224

Signed-off-by: Paul Holzinger <[email protected]>
Add function to get all host entries from a given network status.

Signed-off-by: Paul Holzinger <[email protected]>
Add helper function to convert the base_hosts_file config value to a
actual path. It is important to use securejoin to make sure that
containers cannot point to a file on the hosts via a symlink.

Signed-off-by: Paul Holzinger <[email protected]>
@vrothberg
Copy link
Member

/lgtm
/hold

@Luap99
Copy link
Member Author

Luap99 commented Apr 21, 2022

/hold cancel

@openshift-merge-robot openshift-merge-robot merged commit 7309411 into containers:main Apr 21, 2022
@Luap99 Luap99 deleted the etchosts branch April 21, 2022 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants