Whirr
  1. Whirr
  2. WHIRR-371

Allow defining additional firewall rules

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.6.0
    • Fix Version/s: 0.7.0
    • Component/s: core
    • Labels:
      None

      Description

      Users should have more control over the firewall rules, by adding one or more properties to the whirr properties file

      1. WHIRR-371.patch
        12 kB
        Karel Vervaeke
      2. WHIRR-371.patch
        12 kB
        Karel Vervaeke
      3. WHIRR-371.patch
        12 kB
        Karel Vervaeke
      4. WHIRR-371.patch
        4 kB
        Karel Vervaeke

        Issue Links

          Activity

          Hide
          Karel Vervaeke added a comment -

          Sample configuration:
          whirr.firewall.rules=tcp/10101,tcp/-10102,udp/8650,80

          meaning:
          allow tcp traffic on 10101
          disallow tcp traffic on 10102 (overriding any clusteractionhandler's action)
          allow udp traffic on 8650
          allow tcp traffic on port 80 (tcp/ is the default and can be omitted)

          Bonus points for allowing to define service-specific rules, e.g.
          whirr.firewall.rules.hbase-regionserver=10102

          Note that it's possible to have conflicting rules, e.g.
          whirr.instance-templates='1 serviceA+serviceB'
          whirr.firewall.rules.serviceA=10102
          whirr.firewall.rules.serviceB=-10102
          I suggest we log a warning and open the port in this case.

          Show
          Karel Vervaeke added a comment - Sample configuration: whirr.firewall.rules=tcp/10101,tcp/-10102,udp/8650,80 meaning: allow tcp traffic on 10101 disallow tcp traffic on 10102 (overriding any clusteractionhandler's action) allow udp traffic on 8650 allow tcp traffic on port 80 (tcp/ is the default and can be omitted) Bonus points for allowing to define service-specific rules, e.g. whirr.firewall.rules.hbase-regionserver=10102 Note that it's possible to have conflicting rules, e.g. whirr.instance-templates='1 serviceA+serviceB' whirr.firewall.rules.serviceA=10102 whirr.firewall.rules.serviceB=-10102 I suggest we log a warning and open the port in this case.
          Hide
          Karel Vervaeke added a comment -

          Loosely related, since the related issues would allow the user to create additional listening sockets.

          Show
          Karel Vervaeke added a comment - Loosely related, since the related issues would allow the user to create additional listening sockets.
          Hide
          Tom White added a comment -

          This would be a useful addition. BTW it's sort of related to WHIRR-336.

          Show
          Tom White added a comment - This would be a useful addition. BTW it's sort of related to WHIRR-336 .
          Hide
          Karel Vervaeke added a comment -

          Uploaded a patch which enables using
          whirr.firewall.rules and
          whirr.firewall.rules.<rolename>

          The values of these properties need to be a comma separated list of integers.

          I didn't implement any of the negative-portnumber stuff or the TCP/UDP separation. That was just a brain fart.
          I think it would be better to stay away from implementing a more complete firewall configuration system.

          Show
          Karel Vervaeke added a comment - Uploaded a patch which enables using whirr.firewall.rules and whirr.firewall.rules.<rolename> The values of these properties need to be a comma separated list of integers. I didn't implement any of the negative-portnumber stuff or the TCP/UDP separation. That was just a brain fart. I think it would be better to stay away from implementing a more complete firewall configuration system.
          Hide
          Karel Vervaeke added a comment -

          whirr.firewall.rules=X and whirr.firewall.rules.<rolename>=X result in the same thing right now, but the distinction should become more useful in combination with WHIRR-336.

          Show
          Karel Vervaeke added a comment - whirr.firewall.rules=X and whirr.firewall.rules.<rolename>=X result in the same thing right now, but the distinction should become more useful in combination with WHIRR-336 .
          Hide
          Andrei Savu added a comment -

          Karel can you regenerate the patch? I believe you have create the diff by comparing the branches in the reverse order. One final thing you need to do is to update the configuration guide page in src/side/xdoc/configuration-guide.xml to explain the new option.

          Show
          Andrei Savu added a comment - Karel can you regenerate the patch? I believe you have create the diff by comparing the branches in the reverse order. One final thing you need to do is to update the configuration guide page in src/side/xdoc/configuration-guide.xml to explain the new option.
          Hide
          Karel Vervaeke added a comment -

          Oops. Uploaded new patch.
          This isn't just the reverse of the original patch though. Writing the documentation made me review my changes and eventually led to a quasi-rewrite.

          Show
          Karel Vervaeke added a comment - Oops. Uploaded new patch. This isn't just the reverse of the original patch though. Writing the documentation made me review my changes and eventually led to a quasi-rewrite.
          Hide
          Karel Vervaeke added a comment -

          Again, with grant for ASF inclusion.

          Show
          Karel Vervaeke added a comment - Again, with grant for ASF inclusion.
          Hide
          Adrian Cole added a comment -

          Thanks for starting on this, Karel.

          I like that we are scoping rules to roles. This is going to be important, especially as we have services like HBase that need to inherit rules from services like Zookeeper. If we can have composability, it would be a plus.

          I'd recommend a couple things.

          1. let's focus on rules themselves, noting not all rules are ingress, nor TCP
          For example, I've noticed people want to specify rules relative to a group as opposed to just cidr. For example, this issue addressed a concern from BackType who want to scope rules to a source group (or role) and port range: http://code.google.com/p/jclouds/issues/detail?id=606 In jclouds, there is a IpPermission class that might be helpful when modeling with a fluent class IpPermissions that may be helpful as well.

          2. I like the simple syntax of the properties. It would be nice to use something simple to presume TCP and also ingress, but also parse if the rule is egress or another protocol.

          3. A role will have multiple rules associated with it, so we should consider the impact of this on serialization into properties, etc. similar to the comma separating cidr.

          4. We should figure out how or whether to address composability

          I have to run, but I hope this is helpful!

          -A

          Show
          Adrian Cole added a comment - Thanks for starting on this, Karel. I like that we are scoping rules to roles. This is going to be important, especially as we have services like HBase that need to inherit rules from services like Zookeeper. If we can have composability, it would be a plus. I'd recommend a couple things. 1. let's focus on rules themselves, noting not all rules are ingress, nor TCP For example, I've noticed people want to specify rules relative to a group as opposed to just cidr. For example, this issue addressed a concern from BackType who want to scope rules to a source group (or role) and port range: http://code.google.com/p/jclouds/issues/detail?id=606 In jclouds, there is a IpPermission class that might be helpful when modeling with a fluent class IpPermissions that may be helpful as well. 2. I like the simple syntax of the properties. It would be nice to use something simple to presume TCP and also ingress, but also parse if the rule is egress or another protocol. 3. A role will have multiple rules associated with it, so we should consider the impact of this on serialization into properties, etc. similar to the comma separating cidr. 4. We should figure out how or whether to address composability I have to run, but I hope this is helpful! -A
          Hide
          Andrei Savu added a comment -

          +1 for committing this patch as it is. I believe we should use this feature to open the ports needed for testing and keep them closed by default in all services - except SSH. For 0.7.0 we should look into adding firewall support for cloudservers (e.g. start with all ports closed except SSH in the install phase and push final firewall rules afterConfigure).

          Show
          Andrei Savu added a comment - +1 for committing this patch as it is. I believe we should use this feature to open the ports needed for testing and keep them closed by default in all services - except SSH. For 0.7.0 we should look into adding firewall support for cloudservers (e.g. start with all ports closed except SSH in the install phase and push final firewall rules afterConfigure).
          Hide
          Adrian Cole added a comment -

          only thing is that in the patch there seems to be an inconsistency where hyphens are used rather than dots:

          ex. whirr.firewall-rules-serviceA should be whirr.firewall-rules.serviceA (inside testFirewallRules() + also the documentation)

          otherwise +1

          I'm keen getting this in, as we will need it for WHIRR-385

          Show
          Adrian Cole added a comment - only thing is that in the patch there seems to be an inconsistency where hyphens are used rather than dots: ex. whirr.firewall-rules-serviceA should be whirr.firewall-rules.serviceA (inside testFirewallRules() + also the documentation) otherwise +1 I'm keen getting this in, as we will need it for WHIRR-385
          Hide
          Karel Vervaeke added a comment -

          Fixed the hyphen-vs-dot confusion. Settled for hyphens (whirr.firewall-rules-serviceA)

          Show
          Karel Vervaeke added a comment - Fixed the hyphen-vs-dot confusion. Settled for hyphens (whirr.firewall-rules-serviceA)
          Hide
          Karel Vervaeke added a comment -

          People seem to agree with applying this as-is.

          Show
          Karel Vervaeke added a comment - People seem to agree with applying this as-is.
          Hide
          Karel Vervaeke added a comment -

          Committed

          Show
          Karel Vervaeke added a comment - Committed
          Hide
          Adrian Cole added a comment -

          I don't find the whirr.firewall-rules-serviceA syntax intuitive compared to whirr.firewall-rules.serviceA It is possible we can commit a change to address this?

          Show
          Adrian Cole added a comment - I don't find the whirr.firewall-rules-serviceA syntax intuitive compared to whirr.firewall-rules.serviceA It is possible we can commit a change to address this?
          Hide
          Tom White added a comment -

          I think whirr.firewall-rules.serviceA would be better.

          Show
          Tom White added a comment - I think whirr.firewall-rules.serviceA would be better.
          Hide
          Karel Vervaeke added a comment -

          Changed to whirr.firewall-rules.serviceA in r1173204

          Show
          Karel Vervaeke added a comment - Changed to whirr.firewall-rules.serviceA in r1173204
          Hide
          Adrian Cole added a comment -

          thanks, Karel!

          Show
          Adrian Cole added a comment - thanks, Karel!

            People

            • Assignee:
              Karel Vervaeke
              Reporter:
              Karel Vervaeke
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development