fix --dns and --network conflict #3579

QiWang19 · 2019-07-15T18:02:21Z

Close #3553
This PR makes --dns and --network flag mutually exclusive for podman build and create. Returns conflict error if both flags are set.

Signed-off-by: Qi Wang qiwan@redhat.com

rhatdan · 2019-07-15T19:16:06Z

Should also conflict with
--dns-option=option
--dns-search=domain

Man pages should be updated to say this also.

QiWang19 · 2019-07-15T19:22:58Z

@rhatdan sure, but don't see docker return conflict error with --dns-option and
--dns-search

giuseppe

wouldn't this block --network host? I think it should be restricted to pod or container networks.

giuseppe · 2019-07-16T15:04:58Z

cmd/podman/create.go

@@ -77,6 +77,10 @@ func createInit(c *cliconfig.PodmanCommand) error {
 		logrus.Warn("setting security options with --privileged has no effect")
 	}

+	if (c.IsSet("dns") || c.IsSet("dns-opt") || c.IsSet("dns-search")) && c.IsSet("network") {


you'll need to check --net as well.

mheon · 2019-07-16T15:12:25Z

I think we need to check if --dns works with --net=host now. If it does, we retain that behavior; if not, making it conflict seems fine.

QiWang19 · 2019-07-16T17:34:52Z

@mheon @giuseppe PTAL

TomSweeneyRedHat · 2019-07-16T17:52:43Z

LGTM assuming happy tests

rhatdan · 2019-07-16T18:00:35Z

docs/podman-run.1.md

@@ -212,7 +212,7 @@ Limit write rate (IO per second) to a device (e.g. --device-write-iops=/dev/sda:

 **--dns**=*dns*

-Set custom DNS servers
+Set custom DNS servers. Invalid if using **--dns** with **--network** that is not set to host.


I think this is
Invalid if --network is set to host.
Is it invalid also if --network is set to a different container?
Is it invalid if --network is set to None?

rhatdan · 2019-07-16T18:00:47Z

docs/podman-run.1.md

@@ -224,11 +224,11 @@ The **/etc/resolv.conf** file in the image will be used without changes.

 **--dns-option**=*option*

-Set custom DNS options
+Set custom DNS options. Invalid if using **--dns-option** with **--network** that is not set to host.


I think this is
Invalid if --network is set to host.
Is it invalid also if --network is set to a different container?
Is it invalid if --network is set to None?

rhatdan · 2019-07-16T18:00:56Z

docs/podman-run.1.md


 **--dns-search**=*domain*

-Set custom DNS search domains (Use --dns-search=. if you don't wish to set the search domain)
+Set custom DNS search domains. Invalid if using **--dns-search** and **--network** that is not set to host. (Use --dns-search=. if you don't wish to set the search domain)


I think this is
Invalid if --network is set to host.
Is it invalid also if --network is set to a different container?
Is it invalid if --network is set to None?

rhatdan · 2019-07-16T18:02:02Z

docs/podman-run.1.md

@@ -501,7 +501,7 @@ This works for both background and foreground containers.

 **--network**, **--net**=*mode*

-Set the Network mode for the container:
+Set the Network mode for the container. Invalid if using **--dns**, **--dns-option**, or **--dns-search** with **--network** that is not set to host.


This should be a Node, I think the --net and --network can only be used with without none, host or container:NAME.

rhatdan · 2019-07-16T18:02:37Z

cmd/podman/create.go

+	} else if c.IsSet("net") {
+		setNet = c.String("net")
+	}
+	if (c.IsSet("dns") || c.IsSet("dns-opt") || c.IsSet("dns-search")) && setNet != "" && setNet != "host" {


I think this should be blocked if setNet == NONE, and CONTAINER:?

So it should return error
if (c.IsSet("dns") || c.IsSet("dns-opt") || c.IsSet("dns-search")) && (setNet == "none" || setNet == "host" || strings.HasPrefix("container:")
because it makes no sense to set /etc/resolv.conf if --network=host or --network=none?

DNS configuration options are only disallowed when --net=container:$id is set. Other values (host, bridge, etc) seem fine. --net=none is a possible exception - I don't think it matters, so we can either allow or deny.

If none doesn't matter, I choose to allow none and docker doesn't block none either.

There's another kind of --network 'ns:<path>': path to a network namespace to join, will this be influenced with --dns*?

To only disallow if --net container:id or --network container:id

if (c.IsSet("dns") || c.IsSet("dns-opt") || c.IsSet("dns-search")) && setNet != "" && strings.HasPrefix(setNet, "container:")

Oh, --net=ns: should also disallow DNS options - good catch

Shouldn't we allow dns with ns:? I'd say it is a valid combination

ns attempts to grab /etc/resolv.conf and /etc/hosts from a special path... somewhere in /run, I think? On the assumption that 'ip netns create' made the namespace (it also makes resolv.conf and hosts).

giuseppe · 2019-07-16T18:41:48Z

I think we need to check if --dns works with --net=host now. If it does, we retain that behavior; if not, making it conflict seems fine.

it works with --net host and I think it is an useful combination. You can use the host devices but use a different DNS configuration

mheon · 2019-07-16T19:05:59Z

/approve

mheon · 2019-07-16T19:06:33Z

Alright, given that this works with --dns and --net=host both set we should make sure that we don't regress there.

openshift-ci-robot · 2019-07-16T19:07:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, QiWang19

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:

~~OWNERS~~ [mheon]

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

rhatdan · 2019-07-16T22:22:11Z

Ok fix the docs to explain what is going on, and then we can merge.

rhatdan · 2019-07-16T22:23:06Z

I still think we should fail on --net=none, since resolv.conf makes no sense in this situation. You are basically informing the user that he/she probably made a mistake.

QiWang19 · 2019-07-17T17:44:39Z

podman build also has --network and --dns . Will this also have conflict?

QiWang19 · 2019-07-17T17:51:14Z

The doc said build --network supports container, but I didn't see the code?
https://github.com/containers/libpod/blob/1c02905ec7af9f63a35ee05e9e9ce594c45c4c58/cmd/podman/build.go#L96-L100

mheon · 2019-07-17T17:53:30Z

I view the build thing as a Buildah problem - @TomSweeneyRedHat Mind checking that out to see if things work properly there?

Close containers#3553 This PR makes --dns, --dns-option, --dns-search, and --network not set to host flag mutually exclusive for podman build and create. Returns conflict error if both flags are set. Signed-off-by: Qi Wang <qiwan@redhat.com>

TomSweeneyRedHat · 2019-07-18T15:47:45Z

@mheon too many PR's and too many comments. What do you think needs to be tested on the Buildah side with this?

mheon · 2019-07-18T15:54:18Z

@TomSweeneyRedHat The original issue, #3553 - the --dns flags conflicting with --net when net != host

QiWang19 · 2019-07-18T16:03:14Z

@TomSweeneyRedHat #3553 (comment)
Dan let me check all the commands might have --dns* conflict with --net. I found podman build also have these flags but not sure if they will conflict. If build has this issue, maybe come from buildah?
I let --dns* invalid with --net="none" or --net="container:<id>", dose podman build or build bud support --net="container:"?

TomSweeneyRedHat · 2019-07-18T20:58:33Z

@mheon I played with buildah bud -t myimg --dns = "<MYDNS>" . this afternoon. If I went ahead and did 'buildah from myimg --name myctrand then checked the containers/etc/resolv.confit had the hosts value in there and not "<MYDNS>". However if I added the--dnsoption to the from command alabuildah from myimg --name mctr --dns ""`

I remember having a conversation with @nalind about similar behaviour for another option in the build that was ignored until the from/create container command set it. I believe this is expected, but I'll let him correct me.

So at this point I think we should go forward with this PR once @QiWang19 finishes up her tests and if we need to circle back and touch up the build functionality we can.

rhatdan · 2019-07-19T00:00:12Z

/lgtm
Lets finish up buildah in a separate PR.

QiWang19 · 2019-07-19T19:50:08Z

there's no issue with --dns* flag, they take effect during the build and do not change the /etc/resolv.conf in the final container.
my question is using --dns and --network together in the command

podman build --network="container" doesn't work as described in the doc. I opened an issue for this? I'm not sure it's a doc problem or a bug of the code
and will there be a conflict with buildah bud --dns --network?

$ podman build --dns 4.4.4.4 --network="container" .
Error: unsupported configuration network=container

$ buildah bud --dns 4.4.4.4 --network="container" .
STEP 1: FROM alpine:latest
STEP 2: RUN cat /etc/resolv.conf
search redhat.com usersys.redhat.com
nameserver 4.4.4.4
STEP 3: COMMIT
Getting image source signatures
Copying blob 1bfeebd65323 skipped: already exists

TomSweeneyRedHat · 2019-07-22T14:39:17Z

@QiWang19 please do open a Buildah bug on this. I'm not 100% sure it's a bug, but we can always close it if we find out it is not.

openshift-ci-robot requested review from mheon and TomSweeneyRedHat July 15, 2019 18:02

openshift-ci-robot added the size/S label Jul 15, 2019

QiWang19 changed the title ~~fix --dns and --network confict~~ fix --dns and --network conflict Jul 15, 2019

QiWang19 force-pushed the dns_net branch 2 times, most recently from 00d03d0 to eda7b05 Compare July 15, 2019 18:38

QiWang19 force-pushed the dns_net branch from eda7b05 to fad8b8d Compare July 15, 2019 20:47

openshift-ci-robot added size/M and removed size/S labels Jul 15, 2019

QiWang19 force-pushed the dns_net branch from fad8b8d to f33194d Compare July 15, 2019 20:51

giuseppe reviewed Jul 16, 2019

View reviewed changes

QiWang19 force-pushed the dns_net branch from f33194d to 3494374 Compare July 16, 2019 17:09

rhatdan reviewed Jul 16, 2019

View reviewed changes

mheon added the Release Notes 1.5.0 label Jul 16, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2019

QiWang19 force-pushed the dns_net branch from 3494374 to 84c464c Compare July 17, 2019 17:42

QiWang19 force-pushed the dns_net branch 2 times, most recently from e9df097 to 6ee20fb Compare July 17, 2019 18:08

QiWang19 force-pushed the dns_net branch from 6ee20fb to 3f721f8 Compare July 18, 2019 13:41

openshift-ci-robot assigned rhatdan Jul 19, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2019

openshift-merge-robot merged commit b59abdc into containers:master Jul 19, 2019

QiWang19 deleted the dns_net branch June 26, 2020 15:11

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 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix --dns and --network conflict #3579

fix --dns and --network conflict #3579

QiWang19 commented Jul 15, 2019

rhatdan commented Jul 15, 2019

QiWang19 commented Jul 15, 2019

giuseppe left a comment

giuseppe Jul 16, 2019

mheon commented Jul 16, 2019

QiWang19 commented Jul 16, 2019

TomSweeneyRedHat commented Jul 16, 2019

rhatdan Jul 16, 2019

rhatdan Jul 16, 2019

rhatdan Jul 16, 2019

rhatdan Jul 16, 2019

rhatdan Jul 16, 2019

QiWang19 Jul 16, 2019

mheon Jul 16, 2019

QiWang19 Jul 16, 2019

mheon Jul 16, 2019

giuseppe Jul 16, 2019

mheon Jul 16, 2019

giuseppe commented Jul 16, 2019

mheon commented Jul 16, 2019

mheon commented Jul 16, 2019

openshift-ci-robot commented Jul 16, 2019

rhatdan commented Jul 16, 2019

rhatdan commented Jul 16, 2019

QiWang19 commented Jul 17, 2019

QiWang19 commented Jul 17, 2019

mheon commented Jul 17, 2019

TomSweeneyRedHat commented Jul 18, 2019

mheon commented Jul 18, 2019

QiWang19 commented Jul 18, 2019 •

edited

Loading

TomSweeneyRedHat commented Jul 18, 2019

rhatdan commented Jul 19, 2019

QiWang19 commented Jul 19, 2019

TomSweeneyRedHat commented Jul 22, 2019

fix --dns and --network conflict #3579

fix --dns and --network conflict #3579

Conversation

QiWang19 commented Jul 15, 2019

rhatdan commented Jul 15, 2019

QiWang19 commented Jul 15, 2019

giuseppe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Jul 16, 2019

QiWang19 commented Jul 16, 2019

TomSweeneyRedHat commented Jul 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe commented Jul 16, 2019

mheon commented Jul 16, 2019

mheon commented Jul 16, 2019

openshift-ci-robot commented Jul 16, 2019

rhatdan commented Jul 16, 2019

rhatdan commented Jul 16, 2019

QiWang19 commented Jul 17, 2019

QiWang19 commented Jul 17, 2019

mheon commented Jul 17, 2019

TomSweeneyRedHat commented Jul 18, 2019

mheon commented Jul 18, 2019

QiWang19 commented Jul 18, 2019 • edited Loading

TomSweeneyRedHat commented Jul 18, 2019

rhatdan commented Jul 19, 2019

QiWang19 commented Jul 19, 2019

TomSweeneyRedHat commented Jul 22, 2019

QiWang19 commented Jul 18, 2019 •

edited

Loading