You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by andreaturli <gi...@git.apache.org> on 2014/07/10 17:37:06 UTC
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
GitHub user andreaturli opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/62
fix/BROOKLYN-36
fix saveIptablesRules
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/andreaturli/incubator-brooklyn fix/BROOKLYN-36
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/62.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #62
----
commit 0524a08e9295c8acc66c050707a0c1d885036036
Author: Andrea Turli <an...@gmail.com>
Date: 2014-07-10T15:35:10Z
fix saveIptablesRules
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#issuecomment-49287829
@andreaturli can you test empirical that changes survive a reboot, when using `service iptables save`.
Then I'd be happy to merge.
You could also consider writing a few sentences about using `iptables-persistent` (borrowing from + referencing http://www.thomas-krenn.com/en/wiki/Saving_Iptables_Firewall_Rules_Permanently), and see if you can contribute that to improve the ubuntu community site docs:
https://help.ubuntu.com/community/IptablesHowTo#Solution_.233_iptables-persistent
That should also be a good way to get feedback as to whether you are right!
Note this will only work on Ubuntu version >= Lucid (i.e. >= 10.04).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#discussion_r14807785
--- Diff: utils/common/src/main/java/brooklyn/util/ssh/IptablesCommands.java ---
@@ -93,7 +93,7 @@ public static String iptablesServiceStatus() {
*/
public static String saveIptablesRules() {
return BashCommands.alternatives(
- BashCommands.ifExecutableElse1("iptables-save", sudo("iptables-save")),
+ BashCommands.ifExecutableElse1("apt-get", BashCommands.installPackage("iptables-persistent")),
--- End diff --
does `apt-get`ting this package really cause the rules to be saved immediately? i think you want to second command to be run irrespective of the result of the former. perhaps also try running `iptables-save` as presumably that worked somewhere, and it might work somewhere that `service iptables save` and `/sbin/service iptables save` doesn't work ... if i've understood correctly? /cc @grkvlt
i'd suggest (with `import static BashCommands.*` here, for brevity):
chain(
ok(install("iptables-persistent")),
alternatives(
iptablesService("save"),
"iptables-save"
))
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by andreaturli <gi...@git.apache.org>.
Github user andreaturli commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#issuecomment-54496052
I've tested, squashed the commits and rebased my branch. I think we could close this issue
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#issuecomment-54076989
sounds like you're right, `iptables-save` is not very helpful here; instead we need a
`alternatives(sudo("service iptables save"), installPackage("iptables-persistent"))`
the commands above should be sufficient to have the iptables changes persisted. i don't think we have a use case for a manual `restoreIptablesRules`; but probably worth a comment in the text pointing at this issue in case we need more functionality in future, or discover that our thinking right now is wrong.
AFAIK we don't have any major use cases so would be good to do this small change now, test it, and close this issue.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by andreaturli <gi...@git.apache.org>.
Github user andreaturli commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#discussion_r14809842
--- Diff: utils/common/src/main/java/brooklyn/util/ssh/IptablesCommands.java ---
@@ -93,7 +93,7 @@ public static String iptablesServiceStatus() {
*/
public static String saveIptablesRules() {
return BashCommands.alternatives(
- BashCommands.ifExecutableElse1("iptables-save", sudo("iptables-save")),
+ BashCommands.ifExecutableElse1("apt-get", BashCommands.installPackage("iptables-persistent")),
--- End diff --
Thanks @ahgittin for your comments.
My idea is slightly different: I wanted to make the `saveIptablesRules` more portable across distros.
For rhel/centos the suggested way is to use `service iptables save` while for debian/ubuntu it is available a package called `iptables-persistent` which install a service on the box that save the iptables rules any time they change, afaik.
Instead `iptables-save` seems misused here as it is generally useful when you want to export iptables rules to a file using i.e.: `iptables-save > /etc/sysconfig/iptables` and then `iptables-restore < /etc/sysconfig/iptables` but this doesn't look helpful here. wdyt?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by andreaturli <gi...@git.apache.org>.
Github user andreaturli commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#issuecomment-48999747
@aledsage I think you are right the documentation is a bit confusing. I've manually installed `iptables-persistent` and on ubuntu 12.04 it starts also a service which I assume it is able to persist changes as they happen, but I can't find a good source for it more official than http://www.thomas-krenn.com/en/wiki/Saving_Iptables_Firewall_Rules_Permanently
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#discussion_r17469549
--- Diff: utils/common/src/test/java/brooklyn/util/ssh/IptablesCommandsTest.java ---
@@ -27,43 +27,56 @@
public class IptablesCommandsTest {
- private static final String cleanUptptablesRules = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -F ); else sudo -E -n -S -- /sbin/iptables -F; fi )";
+ private static final String cleanUptptablesRules = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -F ); else sudo -E -n -S -- /sbin/iptables -F; fi )";
- public static final String insertIptablesRule = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -I INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT ); "
- + "else sudo -E -n -S -- /sbin/iptables -I INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT; fi )";
- public static final String appendIptablesRule = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -A INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT ); "
- + "else sudo -E -n -S -- /sbin/iptables -A INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT; fi )";
- public static final String insertIptablesRuleAll = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -I INPUT -p tcp --dport 3306 -j ACCEPT ); "
- + "else sudo -E -n -S -- /sbin/iptables -I INPUT -p tcp --dport 3306 -j ACCEPT; fi )";
- public static final String appendIptablesRuleAll = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -A INPUT -p tcp --dport 3306 -j ACCEPT ); "
- + "else sudo -E -n -S -- /sbin/iptables -A INPUT -p tcp --dport 3306 -j ACCEPT; fi )";
-
- @Test
- public void testCleanUpIptablesRules() {
- Assert.assertEquals(IptablesCommands.cleanUpIptablesRules(), cleanUptptablesRules);
- }
+ public static final String insertIptablesRule = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -I INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT ); "
+ + "else sudo -E -n -S -- /sbin/iptables -I INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT; fi )";
+ private static final String appendIptablesRule = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -A INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT ); "
+ + "else sudo -E -n -S -- /sbin/iptables -A INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT; fi )";
+ private static final String insertIptablesRuleAll = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -I INPUT -p tcp --dport 3306 -j ACCEPT ); "
+ + "else sudo -E -n -S -- /sbin/iptables -I INPUT -p tcp --dport 3306 -j ACCEPT; fi )";
+ private static final String appendIptablesRuleAll = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -A INPUT -p tcp --dport 3306 -j ACCEPT ); "
+ + "else sudo -E -n -S -- /sbin/iptables -A INPUT -p tcp --dport 3306 -j ACCEPT; fi )";
+ private static final String saveIptablesRules = "( ( if test \"$UID\" -eq 0; then ( service iptables save ); else sudo -E -n -S -- service iptables save; fi ) || " +
--- End diff --
To save the iptables rules, will installing `iptables-persistent` really do that? I thought that would just restore the iptables on restart based on the contents of either `/etc/iptables/rules.v4` or `/etc/iptables/rules.v6`. Would we not also need to do `dpkg-reconfigure iptables-persistent` or `iptables-save >/etc/iptables/rules.v4`?
But empirical evidence (and ideally live tests!) trumps everything else.
See http://unix.stackexchange.com/questions/125833/why-isnt-the-iptables-persistent-service-saving-my-changes
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#discussion_r17469109
--- Diff: utils/common/src/main/java/brooklyn/util/ssh/IptablesCommands.java ---
@@ -87,15 +89,20 @@ public static String iptablesServiceStatus() {
}
/**
- * Returns the command that saves on disk iptables rules, to make them resilient to reboot.
+ * Returns the command that saves iptables rules on file.
+ *
+ * @return Returns the command that saves iptables rules on file.
--- End diff --
Personal preference: no need to repeat the same words for the method description + return description. And the return description doesn't need to start with the word "Returns".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#issuecomment-59788780
@andreaturli would be good to have a live test for this, on the different distros.
No need to block or ignore this PR any longer though! I'll merge this now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#issuecomment-51540186
@andreaturli can you update this pls?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/62
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#discussion_r17469190
--- Diff: utils/common/src/main/java/brooklyn/util/ssh/IptablesCommands.java ---
@@ -87,15 +89,20 @@ public static String iptablesServiceStatus() {
}
/**
- * Returns the command that saves on disk iptables rules, to make them resilient to reboot.
+ * Returns the command that saves iptables rules on file.
+ *
+ * @return Returns the command that saves iptables rules on file.
*
- * @return Returns the command that saves on disk iptables rules.
*/
public static String saveIptablesRules() {
- return BashCommands.alternatives(
- BashCommands.ifExecutableElse1("iptables-save", sudo("iptables-save")),
- iptablesService("save"));
- }
+ return alternatives(sudo("service iptables save"), installPackage("iptables-persistent"));
+ }
+ /*
--- End diff --
Strange to have this commented out code here. Either delete, or if it's worth keeping then it needs to go inside the relevant method, and it needs a comment to say why it could be useful in the future and why it's not being used now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by andreaturli <gi...@git.apache.org>.
Github user andreaturli commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#issuecomment-51606179
@aledsage you are right about the fact that `iptables-persistent` needs to be called any time a new ruls is added.
Also, notice that for debian-derivatives there is no `service iptables save`
So in askUbuntu I found a good reference [this](http://askubuntu.com/questions/119393/how-to-save-rules-of-the-iptables) where iptables-persistent seems to be the easiest solution (instead of touching `/etc/network/interfaces`)
For RHEL derivatives we could instead use `service iptables save`
With that in mind I think we need to add a `restoreIptablesRules` that should work with `saveIptablesRules` to achieve the iptables rules persistency. Wdyt @ahgittin and @aledsage ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#issuecomment-48922208
@andreaturli agree that the previous code calling `iptables-save` wasn't right - I don't see how that ever could have achieved it.
However, I also can't find reference to your `iptables-persistent` solution. At [1] it simply says "Install and use the iptables-persistent package.". At [2] and [3] they both say to do `iptables-save > /etc/iptables/rules.v4` (or on some older systems to write to `/etc/iptables/rules`), and to do that every time the rules change.
Can you point us at the URL you are using that describes `iptables-persistent` behaviour as you outlined above?
[1] https://help.ubuntu.com/community/IptablesHowTo#Configuration_on_startup
[1] http://stackoverflow.com/questions/9330694/how-to-permanently-update-iptables
[2] http://www.thomas-krenn.com/en/wiki/Saving_Iptables_Firewall_Rules_Permanently
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#discussion_r17469260
--- Diff: utils/common/src/main/java/brooklyn/util/ssh/IptablesCommands.java ---
@@ -87,15 +89,20 @@ public static String iptablesServiceStatus() {
}
/**
- * Returns the command that saves on disk iptables rules, to make them resilient to reboot.
+ * Returns the command that saves iptables rules on file.
+ *
+ * @return Returns the command that saves iptables rules on file.
*
- * @return Returns the command that saves on disk iptables rules.
*/
public static String saveIptablesRules() {
- return BashCommands.alternatives(
- BashCommands.ifExecutableElse1("iptables-save", sudo("iptables-save")),
- iptablesService("save"));
- }
+ return alternatives(sudo("service iptables save"), installPackage("iptables-persistent"));
--- End diff --
Do you still need to do `service iptables save` after installing `iptables-persistent`? I presume installing the package will not automatically have saved the rules?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: fix/BROOKLYN-36
Posted by andreaturli <gi...@git.apache.org>.
Github user andreaturli commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/62#discussion_r17569005
--- Diff: utils/common/src/test/java/brooklyn/util/ssh/IptablesCommandsTest.java ---
@@ -27,43 +27,56 @@
public class IptablesCommandsTest {
- private static final String cleanUptptablesRules = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -F ); else sudo -E -n -S -- /sbin/iptables -F; fi )";
+ private static final String cleanUptptablesRules = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -F ); else sudo -E -n -S -- /sbin/iptables -F; fi )";
- public static final String insertIptablesRule = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -I INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT ); "
- + "else sudo -E -n -S -- /sbin/iptables -I INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT; fi )";
- public static final String appendIptablesRule = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -A INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT ); "
- + "else sudo -E -n -S -- /sbin/iptables -A INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT; fi )";
- public static final String insertIptablesRuleAll = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -I INPUT -p tcp --dport 3306 -j ACCEPT ); "
- + "else sudo -E -n -S -- /sbin/iptables -I INPUT -p tcp --dport 3306 -j ACCEPT; fi )";
- public static final String appendIptablesRuleAll = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -A INPUT -p tcp --dport 3306 -j ACCEPT ); "
- + "else sudo -E -n -S -- /sbin/iptables -A INPUT -p tcp --dport 3306 -j ACCEPT; fi )";
-
- @Test
- public void testCleanUpIptablesRules() {
- Assert.assertEquals(IptablesCommands.cleanUpIptablesRules(), cleanUptptablesRules);
- }
+ public static final String insertIptablesRule = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -I INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT ); "
+ + "else sudo -E -n -S -- /sbin/iptables -I INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT; fi )";
+ private static final String appendIptablesRule = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -A INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT ); "
+ + "else sudo -E -n -S -- /sbin/iptables -A INPUT -i eth0 -p tcp --dport 3306 -j ACCEPT; fi )";
+ private static final String insertIptablesRuleAll = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -I INPUT -p tcp --dport 3306 -j ACCEPT ); "
+ + "else sudo -E -n -S -- /sbin/iptables -I INPUT -p tcp --dport 3306 -j ACCEPT; fi )";
+ private static final String appendIptablesRuleAll = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -A INPUT -p tcp --dport 3306 -j ACCEPT ); "
+ + "else sudo -E -n -S -- /sbin/iptables -A INPUT -p tcp --dport 3306 -j ACCEPT; fi )";
+ private static final String saveIptablesRules = "( ( if test \"$UID\" -eq 0; then ( service iptables save ); else sudo -E -n -S -- service iptables save; fi ) || " +
--- End diff --
`iptables-persistent` saves the current ipv4 and ipv6 firewall rules during the installation.
So `saveIptablesRules` need to invoke `sudo /etc/init.d/iptables-persistent save` as explained [here](http://askubuntu.com/questions/119393/how-to-save-rules-of-the-iptables) to save rules using `iptables-persistent`
Notice also that as we enable `DEBIAN_FRONTEND=noninteractive` for apt-get install, the problem described [here](http://askubuntu.com/questions/339790/how-can-i-prevent-apt-get-aptitude-from-showing-dialogs-during-installation) is solved.
I've tested the command on an empirical test on a centOS 6 server and a Debian 7 server
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---