Whirr
  1. Whirr
  2. WHIRR-275

Improve firewall API for services

    Details

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

      Description

      The API that services use to configure firewall settings is pretty verbose. It would be nice to improve it.

      1. WHIRR-275.patch
        16 kB
        Tom White
      2. WHIRR-275.patch
        15 kB
        Tom White
      3. WHIRR-275.patch
        14 kB
        Tom White
      4. WHIRR-275.patch
        13 kB
        Tom White

        Activity

        Hide
        Tom White added a comment -

        Here's an early patch which shows what a more fluent interface looks like.

        Show
        Tom White added a comment - Here's an early patch which shows what a more fluent interface looks like.
        Hide
        Tom White added a comment -

        Here's a more complete patch which passes ZK and Hadoop integration tests. FirewallSettings is now deprecated - other services can be moved to use FirewallManager later.

        Show
        Tom White added a comment - Here's a more complete patch which passes ZK and Hadoop integration tests. FirewallSettings is now deprecated - other services can be moved to use FirewallManager later.
        Hide
        Andrei Savu added a comment -

        Looks good. I've just noticed that all the firewall settings are useless on CloudServers. We should do something about that issue or at least put a notice on the website.

        Show
        Andrei Savu added a comment - Looks good. I've just noticed that all the firewall settings are useless on CloudServers. We should do something about that issue or at least put a notice on the website.
        Hide
        Tom White added a comment -

        Good point - I've opened WHIRR-276 for the iptables fix. Meanwhile we should add documentation about this limitation.

        Show
        Tom White added a comment - Good point - I've opened WHIRR-276 for the iptables fix. Meanwhile we should add documentation about this limitation.
        Hide
        Patrick Hunt added a comment -

        lgtm, a few comments:

        1) seems like it would be good to add javadoc for addRule - all the special cases (for example if source is not specified)

        2) javadoc on ports([]) mentioning that the arg replaces, rather than adds, to the port list.

        3) should we considered using guava? Makes collection handling in particular much easier. then we could standardize around using collections rather than varargs (could have a single addRule that takes a set say)

        Show
        Patrick Hunt added a comment - lgtm, a few comments: 1) seems like it would be good to add javadoc for addRule - all the special cases (for example if source is not specified) 2) javadoc on ports([]) mentioning that the arg replaces, rather than adds, to the port list. 3) should we considered using guava? Makes collection handling in particular much easier. then we could standardize around using collections rather than varargs (could have a single addRule that takes a set say)
        Hide
        Tom White added a comment -

        Thanks for taking a look Patrick. Here's a new patch that fixes 1) and 2). For 3) I opted to use varargs since it makes it easy to call in the handlers. E.g.

            event.getFirewallManager().addRules(
                Rule.create()
                  .destination(namenode)
                  .ports(NAMENODE_WEB_UI_PORT, JOBTRACKER_WEB_UI_PORT),
                Rule.create()
                  .source(namenode.getPublicAddress().getHostAddress())
                  .destination(namenode)
                  .ports(NAMENODE_PORT, JOBTRACKER_PORT)
            );
        

        We could overload with a method that takes a collection too if needed.

        Show
        Tom White added a comment - Thanks for taking a look Patrick. Here's a new patch that fixes 1) and 2). For 3) I opted to use varargs since it makes it easy to call in the handlers. E.g. event.getFirewallManager().addRules( Rule.create() .destination(namenode) .ports(NAMENODE_WEB_UI_PORT, JOBTRACKER_WEB_UI_PORT), Rule.create() .source(namenode.getPublicAddress().getHostAddress()) .destination(namenode) .ports(NAMENODE_PORT, JOBTRACKER_PORT) ); We could overload with a method that takes a collection too if needed.
        Hide
        Patrick Hunt added a comment -

        changes for 1/2 look good.

        However on 3, I think it's more natural to have a collection, no? Say you have quite a few rules, or perhaps you want to call one or more methods to create those rules, you'd want to collect them up in.. a collection, and pass that to addRules. Optionally you could just call addRule(rule) multiple times. My suggestion re guava was related to this - it has nice ways to create sets by doing something like "...addRules(ImmutableSet.of(rule1, rule2, rule3))

        Show
        Patrick Hunt added a comment - changes for 1/2 look good. However on 3, I think it's more natural to have a collection, no? Say you have quite a few rules, or perhaps you want to call one or more methods to create those rules, you'd want to collect them up in.. a collection, and pass that to addRules. Optionally you could just call addRule(rule) multiple times. My suggestion re guava was related to this - it has nice ways to create sets by doing something like "...addRules(ImmutableSet.of(rule1, rule2, rule3))
        Hide
        Tom White added a comment -

        Added an overloaded method to take a set.

        Show
        Tom White added a comment - Added an overloaded method to take a set.
        Hide
        Patrick Hunt added a comment -

        +1 looks good.

        Show
        Patrick Hunt added a comment - +1 looks good.
        Hide
        Tom White added a comment -

        I've just committed this.

        Show
        Tom White added a comment - I've just committed this.

          People

          • Assignee:
            Tom White
            Reporter:
            Tom White
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development