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

added ip forwarding flag #115

Merged
merged 4 commits into from
Apr 12, 2014
Merged

added ip forwarding flag #115

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a682b91
added ip forwarding flag
ninnemana Apr 8, 2014
8315df3
i suck at ruby
ninnemana Apr 8, 2014
45b53ad
swapped some logic
ninnemana Apr 8, 2014
4261bf4
switched to more conventional way and renamed to ipv4_forward to avoi…
ninnemana Apr 9, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ image_cmd_timeout | image LWRP default cmd_timeout seconds | Fixnum | 300
init_type | Init type for docker ("runit", "systemd", "sysv", or "upstart") | String | auto-detected (see attributes/default.rb)
install_dir | Installation directory for docker binary | String | auto-detected (see attributes/default.rb)
install_type | Installation type for docker ("binary", "package" or "source") | String | "package"
ip_forward | Sysctl set net.ipv4.ip_forward to 1 | Boolean | true
Copy link
Contributor

Choose a reason for hiding this comment

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

What about IPv6 net.ipv6.conf.all.forwarding? I might be tempted to rename this to ipv4_forward. Or maybe not. I'm not sure. It should also control the behavior in at least the systemd service file, as it also sets ipv4.ip_forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll make this modification.

logfile | Set custom DOCKER_LOGFILE | String | nil
options | Additional options to pass to docker. These could be flags like "-api-enable-cors". | String | nil
pidfile | Set custom DOCKER_PIDFILE | String | nil
Expand Down
3 changes: 3 additions & 0 deletions attributes/default.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@

default['docker']['version'] = nil

# IP forwarding
default['docker']['ip_forward'] = true

# Binary attributes
default['docker']['binary']['version'] = node['docker']['version'] || 'latest'
default['docker']['binary']['url'] = "http://get.docker.io/builds/#{node['kernel']['name']}/#{node['docker']['arch']}/docker-#{node['docker']['binary']['version']}"
Expand Down
8 changes: 5 additions & 3 deletions recipes/default.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
include_recipe 'apt'
package 'apt-transport-https'
package 'bsdtar'
sysctl_param 'net.ipv4.ip_forward' do
value 1
only_if { node['platform'] == 'debian' }
unless node['docker']['ip_forward'] == true
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this does the opposite of what I'd expect this attribute to do: setting ip_forward to true would not enable IP forwarding.

In any event, for conditionals surrounding a single resource, Chef convention is to use only_if and not_if inside the block, just like the only_if that's already inside sysctl_param. The fix would be changing this to: only_if { node['platform'] == 'debian' && node['docker']['ip_forward'] }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right, I wasn't sure if multiple conditions in the only_if were supported.

sysctl_param 'net.ipv4.ip_forward' do
value 1
only_if { node['platform'] == 'debian' }
end
end
end

Expand Down