You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ygy <gi...@git.apache.org> on 2015/06/22 09:07:10 UTC
[GitHub] incubator-brooklyn pull request: BROOKLYN-137 stopIptables and ope...
GitHub user ygy opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/707
BROOKLYN-137 stopIptables and openIptables don't work on CentOS7
Implemented to retain full backwards compatibility before the release
- Adding preliminary support for firewalld
- Starting/stopping/restrting of firewalld service
- Adding rules via firewalld's direct interface
- Tests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ygy/incubator-brooklyn feature/firewall-cmd-support
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/707.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 #707
----
commit d7232a51a93a102fb9e1f38ec0aa55c6247c07a4
Author: Yavor Yanchev <ya...@yanchev.com>
Date: 2015-06-22T07:03:08Z
BROOKLYN-137 stopIptables and openIptables don't work on CentOS7
- Adding preliminary support for firewalld
- Starting/stopping/restrting of firewalld service
- Adding rules via firewalld's direct interface
- Tests
----
---
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: BROOKLYN-137 stopIptables and ope...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/707#discussion_r33020040
--- Diff: utils/common/src/main/java/brooklyn/util/ssh/IptablesCommands.java ---
@@ -89,6 +89,38 @@ public static String iptablesServiceStatus() {
return iptablesService("status");
}
+ @Beta // implementation not portable across distros
+ public static String firewalldService(String cmd) {
+ return sudo(alternatives(
+ BashCommands.ifExecutableElse1("systemctl", "systemctl " + cmd + " firewalld"),
+ "/usr/bin/systemctl " + cmd + " firewalld"));
--- End diff --
Does this mean that `/usr/bin` might not be on the `PATH`? In what cases?
---
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: BROOKLYN-137 stopIptables and ope...
Posted by ygy <gi...@git.apache.org>.
Github user ygy commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/707#issuecomment-114064705
Thanks for the feedback.
@bostko: adding rules with firewall-cmd requires root privileges. it follows the implementation of addIptablesRule
@andreaturli @tbouron: actually, the initial implementation was just as your proposal. i've discussed it with @bostko already.
I've decided to go with the current one, because we are days before a major release. I didn't wanted to change the interface or introduce big changes which will require comprehensive testing. It will be good to have firewalld support in 0.7 so we can get Riak running on CentOS 7 too.
If you agree, I can get it implemented with proper abstraction post 0.7 release?
---
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: BROOKLYN-137 stopIptables and ope...
Posted by ygy <gi...@git.apache.org>.
Github user ygy commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/707#discussion_r33030496
--- Diff: utils/common/src/main/java/brooklyn/util/ssh/IptablesCommands.java ---
@@ -89,6 +89,38 @@ public static String iptablesServiceStatus() {
return iptablesService("status");
}
+ @Beta // implementation not portable across distros
+ public static String firewalldService(String cmd) {
+ return sudo(alternatives(
+ BashCommands.ifExecutableElse1("systemctl", "systemctl " + cmd + " firewalld"),
+ "/usr/bin/systemctl " + cmd + " firewalld"));
--- End diff --
It's just as a precaution in case someone changed or removed environment variables for non-interactive shells
---
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: BROOKLYN-137 stopIptables and ope...
Posted by andreaturli <gi...@git.apache.org>.
Github user andreaturli commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/707#issuecomment-114047478
thanks @tbouron it makes a lot of sense!
---
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: BROOKLYN-137 stopIptables and ope...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/707#issuecomment-114043722
Looks good. @ygy Can you remove sudo from the firewall-cmd related methods in IptablesCommands.
This will improve the readability of `IptablesCommandsFirewalldTest`
---
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: BROOKLYN-137 stopIptables and ope...
Posted by andreaturli <gi...@git.apache.org>.
Github user andreaturli commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/707#issuecomment-114045925
@ygy thanks for that! great start!
Do you think it is possible to refactor a bit your implementation? It would be nice to rename `IptablesCommands` a more generic `FirewallCommands` interface, which is able to use the correct implementation (`iptables` vs `firewalld` and possibly `windows firewall`) based on the OsFamily to make it more portable?
If so, we should probably deprecate `IptablesCommands` instead of removing it immediately.
---
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: BROOKLYN-137 stopIptables and ope...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/707#issuecomment-114046588
@andreaturli +1. Actually, this PR can also convert that: https://issues.apache.org/jira/browse/BROOKLYN-151
---
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: BROOKLYN-137 stopIptables and ope...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/707#issuecomment-114410733
LGTM. +1 for @andreaturli's suggestions which can be addressed in a future PR.
---
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: BROOKLYN-137 stopIptables and ope...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/707#discussion_r32920648
--- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
@@ -364,7 +364,15 @@ public boolean isWindows(NodeMetadata node, ConfigBag config) {
? (OsFamily.WINDOWS == os.getFamily())
: (OsFamily.WINDOWS == confFamily);
}
-
+
+ public boolean isNodeFirewalldEnabled(NodeMetadata node) {
+ String OS = node.getOperatingSystem().getFamily().toString();
+ String version = node.getOperatingSystem().getVersion();
+ return node.getOperatingSystem().getVersion().startsWith("7") &&
+ (node.getOperatingSystem().getFamily().equals(OsFamily.RHEL) ||
+ node.getOperatingSystem().getFamily().equals(OsFamily.CENTOS));
--- End diff --
The check is not future proof. Instead of checking fixed OS versions is it possible to check for the existence of firewall-cmd instead? This will allow the blueprint to work on future versions of the OS without any 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: BROOKLYN-137 stopIptables and ope...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/707
---
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: BROOKLYN-137 stopIptables and ope...
Posted by ygy <gi...@git.apache.org>.
Github user ygy commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/707#discussion_r32935680
--- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
@@ -365,6 +365,16 @@ public boolean isWindows(NodeMetadata node, ConfigBag config) {
: (OsFamily.WINDOWS == confFamily);
}
+ public boolean isLocationFirewalldEnabled(SshMachineLocation location) {
+ int result = location.execCommands("checking if firewalld is active",
+ ImmutableList.of(IptablesCommands.firewalldServiceIsActive()));
+ if (result == 0) {
+ return true;
+ }
+
--- End diff --
@neykov here is addressed your previous comment regarding the firewall-cmd existence
---
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.
---