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

libpod,netavark: correctly set /etc/resolv.conf for custom dns server and make --dns functional #16297

Merged
merged 4 commits into from
Jan 23, 2023

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Oct 26, 2022

Following PR ensures two things

This also ensures docker parity since Podman populates container's /etc/resolv.conf with custom DNS servers
( specified via --dns or dns_server in containers.conf ) even when container is connected to a network where dns_enabled is true. Current behavior does not matches with docker, hence following commit ensures that podman only populates custom DNS server when container is not connected to any network where DNS is enabled and for the cases where dns_enabled is true
the resolution for custom DNS server will happen via ( aardvark-dns ).

Reference: https://docs.docker.com/config/containers/container-networking/#dns-services

container: `--dns` and `dns_server` behavior for containers connected to network matches with docker now when netavark is used

Closes: #16172
Closes: RFC in BZ#2128675

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 26, 2022
@flouthoc flouthoc changed the title libpod,netavark: correctly set /etc/resolv.conf for custom dns server when using netavark/aardvark-dns libpod,netavark: correctly set /etc/resolv.conf for custom dns server and make --dns functional Oct 26, 2022
@flouthoc flouthoc requested a review from Luap99 October 26, 2022 07:24
@flouthoc
Copy link
Collaborator Author

Following PR needs netavark and aardvark-dns from main, @cevich any clue how can I get netavark and aardvark-dns from main so I can make CI green and get this PR in ?

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.

How does this behave with --dns=none? Does it still work, it should ignore /etc/resolv.conf setup.

libpod/container_internal_common.go Outdated Show resolved Hide resolved
@flouthoc
Copy link
Collaborator Author

How does this behave with --dns=none? Does it still work, it should ignore /etc/resolv.conf setup.

In case of --dns=none no /etc/resolv.conf is created inside the container.

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
I think we have skip the test until me make new nv/av releases and get them into the CI VMs.

@flouthoc
Copy link
Collaborator Author

Is it possible to only pull nv/av from main only for this PR so I can verify entire CI ? @cevich @baude any hints ?

@flouthoc
Copy link
Collaborator Author

LGTM I think we have skip the test until me make new nv/av releases and get them into the CI VMs.

@Luap99 Problem with skipping is that I will also have to skip some older nameserver tests as well since now nameservers are being returned from netavark but CI is using old netavark. However I am okay to skip those tests if everybody is ok with it.

@Luap99
Copy link
Member

Luap99 commented Oct 26, 2022

Lets wait then, there is no rush to get this in, the next feature release is 4.4 which I assume is at least over a month away.

@vrothberg vrothberg changed the title libpod,netavark: correctly set /etc/resolv.conf for custom dns server and make --dns functional WIP: libpod,netavark: correctly set /etc/resolv.conf for custom dns server and make --dns functional Nov 9, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2022
@flouthoc
Copy link
Collaborator Author

I think CI should pass if new netavark/aardvark are already in CI.

@flouthoc flouthoc changed the title WIP: libpod,netavark: correctly set /etc/resolv.conf for custom dns server and make --dns functional libpod,netavark: correctly set /etc/resolv.conf for custom dns server and make --dns functional Nov 15, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 15, 2022
@TomSweeneyRedHat
Copy link
Member

Just leaving a reminder that we promised to have this upstream for QE testing on November 28. @flouthoc , if that's not likely to happen, that's fine, but we'll just need to adjust expectations.

@flouthoc
Copy link
Collaborator Author

Just leaving a reminder that we promised to have this upstream for QE testing on November 28. @flouthoc , if that's not likely to happen, that's fine, but we'll just need to adjust expectations.

@TomSweeneyRedHat Recently there was change of requirements for original RFE so this PR does not covers the epic, this is the old implementation before requirements were changed so this PR might not be applicable for epic anymore but still needed for docker compat.

Scope of new RFE is bigger and requirement is still under consideration, work on that part has not been started yet so definitely it will not make it on nov28. But I am not aware when requirements will be finalized but @baude can help better here.

Overall this PR is still needed for docker compatibility so I think this will be still merged.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2022
@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@mheon
Copy link
Member

mheon commented Jan 17, 2023

What's going on here - is this abandoned in favor of a newer PR?

@Luap99
Copy link
Member

Luap99 commented Jan 17, 2023

This is still needed AFAIK

@github-actions github-actions bot removed the stale-pr label Jan 18, 2023
@flouthoc
Copy link
Collaborator Author

@mheon This is still needed but needs newer versions of nv/av in CI so waiting for that.

@Luap99
Copy link
Member

Luap99 commented Jan 18, 2023

when you rebase and make sure test passes with newer nv/av I am fine with merging this with the test skipped like the other PR.

Aardvark-dns and netavark now accepts custom DNS servers for containers
via new config field `dns_servers`. New field allows containers to use
custom resolvers instead of host's default resolvers.

Following commit instruments libpod to pass these custom DNS servers set
via `--dns` or central config to the network stack.

Depends-on:
* Common: containers/common#1189
* Netavark: containers/netavark#452
* Aardvark-dns: containers/aardvark-dns#240

Signed-off-by: Aditya R <arajan@redhat.com>
…server

After containers/netavark#452 `netavark` is
incharge of deciding `custom_dns_servers` if any so lets honor that and
libpod should not set these manually.

This also ensures docker parity
Podman populates container's `/etc/resolv.conf` with custom DNS servers ( specified via `--dns` or `dns_server` in containers.conf )
even when container is connected to a network where `dns_enabled` is `true`.

Current behavior does not matches with docker, hence following commit ensures that podman only populates custom DNS server when container is not connected to any network where DNS is enabled and for the cases where `dns_enabled` is `true`
the resolution for custom DNS server will happen via ( `aardvark-dns` or `dnsname` ).

Reference: https://docs.docker.com/config/containers/container-networking/#dns-services
Closes: containers#16172

Signed-off-by: Aditya R <arajan@redhat.com>
Set search domain irrespective of nameservers.

Signed-off-by: Aditya R <arajan@redhat.com>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2023
@flouthoc
Copy link
Collaborator Author

Failing tests seems unrelated.

@flouthoc flouthoc force-pushed the netavark-custom-dns branch 4 times, most recently from db19977 to 47bbbb2 Compare January 23, 2023 11:58
@flouthoc
Copy link
Collaborator Author

I think this should become green now.

Signed-off-by: Aditya R <arajan@redhat.com>
@flouthoc
Copy link
Collaborator Author

@Luap99 @containers/podman-maintainers PTAL

@flouthoc flouthoc requested a review from Luap99 January 23, 2023 16:08
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
Copy link
Contributor

openshift-ci bot commented Jan 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

@flouthoc flouthoc requested a review from mheon January 23, 2023 16:32
@mheon
Copy link
Member

mheon commented Jan 23, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2023
@openshift-merge-robot openshift-merge-robot merged commit 1a90189 into containers:main Jan 23, 2023
@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 Sep 14, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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
Development

Successfully merging this pull request may close these issues.

Podman's --dns + --network is different than docker's --dns + --network
5 participants