Skip to content

IptablesCommands support #622

Merged
merged 1 commit into from Mar 25, 2013

3 participants

@andreaturli
  • pulled CommonCommands up to brooklyn/core
  • added IptablesCommands to brooklyn/core
  • deprecated brooklyn-software-base/CommonCommands
  • added initial support to map security group to iptables in JcloudsLocation

  • tested positively on MySqlLiveEc2Test

@buildhive

Brooklyn Central » brooklyn #317 FAILURE
Looks like there's a problem with this pull request
(what's this?)

@aledsage aledsage commented on an outdated diff Mar 22, 2013
...a/brooklyn/entity/basic/lifecycle/CommonCommands.java
- private static Object getFlag(Map flags, String flagName, Object defaultValue) {
- Object found = flags.get(flagName);
- return found == null ? defaultValue : found;
- }
-
- /**
- * Returns the command that installs Java 1.6.
- *
- * @return the command that install Java 1.6.
- */
- public static String installJava6() {
- return installPackage(MutableMap.of("apt", "openjdk-6-jdk","yum", "java-1.6.0-openjdk-devel"), null);
- }
+/**
+ *
+ * @deprecated since 0.5.0-M3 (added in 0.5.0-M2). Please use {@link brooklyn.util.ssh.CommonCommands}
@aledsage
Brooklyn Central member
aledsage added a note Mar 22, 2013

Was this really only added in 0.5.0-M2?

@aledsage
Brooklyn Central member
aledsage added a note Mar 22, 2013

It was definitely in 0.4 as well: https://github.com/brooklyncentral/brooklyn/blob/0.4.0/software/base/src/main/java/brooklyn/entity/basic/lifecycle/CommonCommands.java

So just say @deprecated since 0.5.0. Please use {@link brooklyn.util.ssh.CommonCommands}`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aledsage aledsage commented on the diff Mar 22, 2013
core/src/main/java/brooklyn/util/ssh/CommonCommands.java
+import static java.lang.String.format;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+
+import brooklyn.entity.drivers.downloads.DownloadResolverManager;
+import brooklyn.util.MutableMap;
+
+import com.google.common.collect.ImmutableList;
+
+public class CommonCommands {
@aledsage
Brooklyn Central member
aledsage added a note Mar 22, 2013

did you use git mv software/base/src/main/java/brooklyn/entity/basic/lifecycle/CommonCommands.java core/src/main/java/brooklyn/util/ssh/CommonCommands.java? That (sometimes) preserves the history of the file, and makes the pull request diff show that only the changes rather than a delete + create.

@andreaturli
andreaturli added a note Mar 22, 2013

No, I've simply recreated CommonCommands at brooklyn.util.ssh.

I will try to remember this tip next time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aledsage aledsage commented on an outdated diff Mar 22, 2013
...src/main/java/brooklyn/util/ssh/IptablesCommands.java
@@ -0,0 +1,75 @@
+package brooklyn.util.ssh;
+
+import static brooklyn.util.ssh.CommonCommands.chain;
+import static brooklyn.util.ssh.CommonCommands.exists;
+import static brooklyn.util.ssh.CommonCommands.sudo;
+
+import java.util.LinkedList;
+import java.util.List;
+
+public class IptablesCommands {
+
+ public enum Chain {
@aledsage
Brooklyn Central member
aledsage added a note Mar 22, 2013

Is your indentation set to 2 spaces? 4 is the norm for brooklyn (though some 2 or 3 has snuck in).

@aledsage
Brooklyn Central member
aledsage added a note Mar 22, 2013

Is there a mix of tabs and spaces, by any chance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aledsage aledsage commented on an outdated diff Mar 22, 2013
...src/main/java/brooklyn/util/ssh/IptablesCommands.java
+import java.util.List;
+
+public class IptablesCommands {
+
+ public enum Chain {
+ INPUT, FORWARD, OUTPUT
+ }
+
+ public enum Policy {
+ ACCEPT, REJECT, DROP, LOG
+ }
+
+ public enum Protocol {
+ TCP("tcp"), UDP("udp"), ALL("all");
+
+ String protocol;
@aledsage
Brooklyn Central member
aledsage added a note Mar 22, 2013

best to make this final.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aledsage aledsage and 1 other commented on an outdated diff Mar 22, 2013
...n/java/brooklyn/location/jclouds/JcloudsLocation.java
@@ -353,6 +360,20 @@ public JcloudsSshMachineLocation obtain(Map<?,?> flags) throws NoMachinesAvailab
}
+ private void mapSecurityGroupRuleToIpTables(ComputeService computeService,
+ NodeMetadata node, LoginCredentials credentials, Iterable<Integer> ports) {
+
+ for (Integer port : ports) {
+ String insertIptableRule = IptablesCommands.insertIptablesRule(Chain.INPUT, "eth0", Protocol.TCP, port, Policy.ACCEPT);
+ Statement statement = Statements.newStatementList(exec(insertIptableRule));
+ ExecResponse response =
+ computeService.runScriptOnNode(node.getId(), statement, overrideLoginCredentials(credentials).runAsRoot(false));
+ if (response.getExitStatus() != 0)
+ throw new RuntimeException(String.format(
@aledsage
Brooklyn Central member
aledsage added a note Mar 22, 2013

I think this will throw an exception if iptables wasn't installed on the box, which is not the intent.

The command generated uses CommonCommands.exists, which does something like (which mycmd && mycmd) - so that returns -1 if mycmd is not on the path.

@andreaturli
andreaturli added a note Mar 22, 2013

Good point. I think I need to study a bit more, because iptables is a kernel module and it is supposed to be there always (maybe worth checking the loaded module?)

So rather than 'exists' we could always add an iptables rule even if it not strictly needed in all the distros.

@aledsage wdyt?

@andreaturli
andreaturli added a note Mar 25, 2013

The netfilter/iptable project is designed in this way:
The kernel modules named ip_tables, ip6_tables, arp_tables (the underscore is part of the name) and ebtables are some of the significant users of the Netfilter hook system. They provide a table-based system for defining firewall rules that can filter or transform packets. The tables can be administered through the user-space tools iptables, ip6tables, arptables and ebtables, respectively.

So I think we can ensure iptables is installed on the vm, and run the insert/append rule to it.

@aledsage
Brooklyn Central member
aledsage added a note Mar 25, 2013

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aledsage
Brooklyn Central member

Looks good. Comments are mostly minor.

@buildhive

Brooklyn Central » brooklyn #327 SUCCESS
This pull request looks good
(what's this?)

@aledsage aledsage commented on the diff Mar 25, 2013
...test/java/brooklyn/util/ssh/IptablesCommandsTest.java
@@ -0,0 +1,67 @@
+package brooklyn.util.ssh;
+
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import brooklyn.util.ssh.IptablesCommands.Chain;
+import brooklyn.util.ssh.IptablesCommands.Policy;
+import brooklyn.util.ssh.IptablesCommands.Protocol;
+
+public class IptablesCommandsTest {
+
+ private static final String cleanUptptablesRules = "((which iptables || " +
+ "((which apt-get && echo apt-get exists, doing update && export DEBIAN_FRONTEND=noninteractive && " +
@aledsage
Brooklyn Central member
aledsage added a note Mar 25, 2013

wow, that's scary looking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aledsage
Brooklyn Central member

Thanks Andrea; merging.

@aledsage aledsage merged commit e903ff1 into brooklyncentral:master Mar 25, 2013
@ahgittin ahgittin pushed a commit that referenced this pull request Feb 1, 2016
@sjcorbett sjcorbett This closes #622 0be3b72
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.