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.

Sign up for GitHub

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

Jump to bottom

Fix Docker API compatibility with network alias #21013

Merged
merged 1 commit into from
Dec 15, 2023
Merged

Fix Docker API compatibility with network alias #21013

merged 1 commit into from
Dec 15, 2023

Conversation

gavinkflam
Copy link
Contributor

@gavinkflam gavinkflam commented Dec 14, 2023 •
edited
Loading

Fixes #17167

I fixed this issue by applying @Luap99's suggestions - #17167 (comment)

This is needed for it to work:

  1. Add the base_hosts_file option to the container spec and config so it can be set for specific containers.
  2. Then make sure the docker compat api sets this field to none to match the docker behavior.

Does this PR introduce a user-facing change?

Fix a bug in Docker API where the entries in /etc/hosts file are copied into the container resulting in incompatibility with network alias.

@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Dec 14, 2023
@gavinkflam gavinkflam mentioned this pull request Dec 14, 2023
Closed
@gavinkflam gavinkflam force-pushed the 17167-compat-base-hosts-file branch 2 times, most recently from 06692b3 to c91004e Compare December 14, 2023 05:57
@gavinkflam gavinkflam changed the title Fix network alias compatibility with Docker API Fix Docker API compatibility with network alias compatibility Dec 14, 2023
@gavinkflam gavinkflam changed the title Fix Docker API compatibility with network alias compatibility Fix Docker API compatibility with network alias Dec 14, 2023
Luap99
Luap99 requested changes Dec 14, 2023
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, code mostly looks good except one naming nit.
I think for the test it would be better to bind mount the hosts file as this could work as rootless as well.

pkg/specgen/specgen.go Outdated Show resolved Hide resolved
test/compose/etc_hosts/setup.sh Outdated Show resolved Hide resolved
test/compose/etc_hosts/docker-compose.yml Outdated Show resolved Hide resolved
@gavinkflam gavinkflam force-pushed the 17167-compat-base-hosts-file branch from c91004e to fe07cca Compare December 15, 2023 04:15
@gavinkflam gavinkflam requested a review from Luap99 December 15, 2023 04:29
@gavinkflam
Fix Docker API compatibility with network alias (containers#17167)
db68764 
* Add BaseHostsFile to container configuration
* Do not copy /etc/hosts file from host when creating a container using Docker API

Signed-off-by: Gavin Lam <[email protected]>
@gavinkflam gavinkflam force-pushed the 17167-compat-base-hosts-file branch from fe07cca to db68764 Compare December 15, 2023 04:31
@gavinkflam
Copy link
Contributor Author

@Luap99 I can submit a subsequent PR adding a --base-hosts-file flag to CLI.

It provides flexibility for users to override hosts file at a container level. Do you think it is a good idea?

@Luap99
Copy link
Member

Luap99 commented Dec 15, 2023

@Luap99 I can submit a subsequent PR adding a --base-hosts-file flag to CLI.

It provides flexibility for users to override hosts file at a container level. Do you think it is a good idea?

Yes sure. I think #13277 asks for this.

Luap99
Luap99 approved these changes Dec 15, 2023
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 15, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gavinkflam, 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 15, 2023
@rhatdan
Copy link
Member

rhatdan commented Dec 15, 2023

/lgtm
Thanks @gavinkflam

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 07834ab into containers:main Dec 15, 2023
91 of 92 checks passed
This was referenced Dec 16, 2023
Merged
Closed
Merged
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Mar 15, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 approved these changes

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: host's /etc/hosts file overrides private network dns settings in podman
3 participants
@gavinkflam @Luap99 @rhatdan