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 iptables option to the daemon #309

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sherry-ummen
Copy link

@sherry-ummen sherry-ummen commented Nov 25, 2020

PR is coming from this discussion here : https://discourse.drone.io/t/docker-parallel-build-failing-anyone/8406/6

Adding iptables option as discussed

@@ -103,6 +103,11 @@ func main() {
Usage: "docker daemon executes in debug mode",
EnvVar: "PLUGIN_DEBUG,DOCKER_LAUNCH_DEBUG",
},
cli.BoolFlag{
Copy link

@ashwilliams1 ashwilliams1 Nov 25, 2020

Choose a reason for hiding this comment

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

change to IPTablesOff since they are on by default, and we want to keep this default value

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -285,6 +290,7 @@ func run(c *cli.Context) error {
Disabled: c.Bool("daemon.off"),
IPv6: c.Bool("daemon.ipv6"),
Debug: c.Bool("daemon.debug"),
IPTables: c.Bool("daemon.iptables"),
Copy link

@ashwilliams1 ashwilliams1 Nov 25, 2020

Choose a reason for hiding this comment

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

rename to IPTablesOff since they are on by default

Copy link
Author

Choose a reason for hiding this comment

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

done

cli.BoolFlag{
Name: "daemon.iptables",
Usage: "docker daemon enable addition of iptables rules",
EnvVar: "PLUGIN_IPTABLES",

Choose a reason for hiding this comment

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

change to PLUGIN_IPTABLES_OFF

Copy link
Author

Choose a reason for hiding this comment

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

done

docker.go Outdated
@@ -370,6 +371,9 @@ func commandDaemon(daemon Daemon) *exec.Cmd {
if daemon.Experimental {
args = append(args, "--experimental")
}
if daemon.IPTables {

Choose a reason for hiding this comment

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

need to verify the flag, but if iptables_off: true then we should disable iptables, which would otherwise be enabled by default

if daemon.IPTablesOff {
  args = append(args, "--iptables=false")
}

Copy link
Author

Choose a reason for hiding this comment

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

done

@sherry-ummen
Copy link
Author

builds are failing not due to these changes . but due to the rate limit issue. if someone could still review and approve this

@tboerger
Copy link

Why the name like iptables off? The flag could be true by default and the flag only gets appended if it's not true.

@@ -103,6 +103,11 @@ func main() {
Usage: "docker daemon executes in debug mode",
EnvVar: "PLUGIN_DEBUG,DOCKER_LAUNCH_DEBUG",
},
cli.BoolTFlag{
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be Bool instead of BoolT. The BoolT parameter defaults to true if unset, however, in this case I think we want to disable to false and only disable IP tables if the user explicitly sets this value to true

@@ -103,6 +103,11 @@ func main() {
Usage: "docker daemon executes in debug mode",
EnvVar: "PLUGIN_DEBUG,DOCKER_LAUNCH_DEBUG",
},
cli.BoolTFlag{
Name: "daemon.iptables",
Usage: "docker daemon enable addition of iptables rules",
Copy link
Member

Choose a reason for hiding this comment

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

also consider changing the description to make it clear that this flag disables iptable rules, instead of enabling

@tphoney
Copy link

tphoney commented Jul 6, 2021

Have you had a look at implementing @bradrydzewski 's suggested changes and rebasing of master.

@andriihrachov
Copy link

Any updates on this? highly needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants