You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by bostko <gi...@git.apache.org> on 2016/07/28 00:13:40 UTC

[GitHub] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

GitHub user bostko opened a pull request:

    https://github.com/apache/brooklyn-server/pull/276

    Effector for opening inbound ports in security group

    Caveat: the effector will return error when called before machine location is provisioned

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/bostko/brooklyn-server security_group_effector

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/brooklyn-server/pull/276.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 #276
    
----
commit ac7683ee29155fd78c9cb7fd183689ee31837e83
Author: Valentin Aitken <bo...@gmail.com>
Date:   2016-07-28T00:13:18Z

    Effector for opening inbound ports in security group
    
    Caveat: the effector will return error when called before machine location is provisioned

----


---
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] brooklyn-server pull request #276: (For Review) Effector for opening inbound...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r72585871
  
    --- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/NetworkingEffectors.java ---
    @@ -0,0 +1,80 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.location.jclouds.networking;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Preconditions;
    +import com.google.common.base.Predicates;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Range;
    +import com.google.common.reflect.TypeToken;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.effector.EffectorBody;
    +import org.apache.brooklyn.core.effector.Effectors;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.apache.brooklyn.util.collections.MutableList;
    +import org.apache.brooklyn.util.core.BrooklynNetworkUtils;
    +import org.apache.brooklyn.util.core.config.ConfigBag;
    +import org.apache.brooklyn.util.net.Cidr;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +
    +import java.util.List;
    +
    +public class NetworkingEffectors {
    +    public static final ConfigKey<List<String>> INBOUND_PORTS_EFFECTOR = ConfigKeys.newConfigKey(new TypeToken<List<String>>() {}, "inbound.ports.effector",
    +            "Ports to open from the effector", ImmutableList.<String>of());
    +
    +    public static Effector<Void> openPortsInSecurityGroupEffector() {
    +        return Effectors.effector(Void.class, "openPortsInSecurityGroup")
    +                .parameter(INBOUND_PORTS_EFFECTOR)
    +                .description("Open ports in Cloud Security Group")
    +                .impl(new OpenPortsInSecurityGroupBody())
    +                .build();
    +    }
    +
    +    private static class OpenPortsInSecurityGroupBody extends EffectorBody<Void> {
    +        @Override
    +        public Void call(ConfigBag parameters) {
    +            List<String> rawPortRules = parameters.get(INBOUND_PORTS_EFFECTOR);
    +            Preconditions.checkNotNull(rawPortRules, "ports cannot be null");
    +            MutableList.Builder<IpPermission> ipPermissionsBuilder = MutableList.builder();
    +            for (Range<Integer> portRule : BrooklynNetworkUtils.portRulesToRanges(rawPortRules).asRanges()) {
    +                ipPermissionsBuilder.add(
    +                        IpPermission.builder()
    +                                .ipProtocol(IpProtocol.TCP)
    --- End diff --
    
    We want this to support both UPD and TCP, so the effector to indicate which is required.


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75850603
  
    --- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/NetworkingEffectors.java ---
    @@ -0,0 +1,90 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.location.jclouds.networking;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Preconditions;
    +import com.google.common.base.Predicates;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Range;
    +import com.google.common.reflect.TypeToken;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.effector.EffectorBody;
    +import org.apache.brooklyn.core.effector.Effectors;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.apache.brooklyn.util.collections.MutableList;
    +import org.apache.brooklyn.util.core.BrooklynNetworkUtils;
    +import org.apache.brooklyn.util.core.config.ConfigBag;
    +import org.apache.brooklyn.util.net.Cidr;
    +import org.apache.brooklyn.util.net.Networking;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +
    +import java.util.List;
    +
    +public class NetworkingEffectors {
    +    // Intentionally not use CloudLocationConfig.INBOUND_PORTS to make richer syntax and rename it to differ it from the first in a ConfigBag
    +    public static final ConfigKey<List<String>> INBOUND_PORTS_LIST = ConfigKeys.newConfigKey(new TypeToken<List<String>>() {}, "inbound.ports.list",
    +            "Ports to open from the effector", ImmutableList.<String>of());
    +    public static final ConfigKey<IpProtocol> INBOUND_PORTS_LIST_PROTOCOL = ConfigKeys.newConfigKey(new TypeToken<IpProtocol>() {}, "inbound.ports.list.protocol",
    +            "Protocol for ports to open. Possible values: TCP, UDP, ICMP, ALL.", IpProtocol.TCP);
    +
    +    public static Effector<Iterable<IpPermission>> openPortsInSecurityGroupEffector() {
    +        return Effectors.effector(EffectorResult.class, "openPortsInSecurityGroup")
    +                .parameter(INBOUND_PORTS_LIST)
    +                .parameter(INBOUND_PORTS_LIST_PROTOCOL)
    +                .description("Open ports in Cloud Security Group. If called before machine location is provisioned, it will fail.")
    +                .impl((EffectorBody)new OpenPortsInSecurityGroupBody())
    +                .build();
    +    }
    +
    +    private interface EffectorResult extends Iterable<IpPermission> {}
    +
    +    private static class OpenPortsInSecurityGroupBody extends EffectorBody<Iterable<IpPermission>> {
    +        @Override
    +        public Iterable<IpPermission> call(ConfigBag parameters) {
    +            List<String> rawPortRules = parameters.get(INBOUND_PORTS_LIST);
    +            IpProtocol ipProtocol = parameters.get(INBOUND_PORTS_LIST_PROTOCOL);
    +            Preconditions.checkNotNull(ipProtocol);
    +            Preconditions.checkNotNull(rawPortRules, "ports cannot be null");
    --- End diff --
    
    @neykov this should be fixed 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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75852467
  
    --- Diff: software/base/src/test/java/org/apache/brooklyn/entity/software/base/location/NetworkingEffectorsLiveTests.java ---
    @@ -0,0 +1,107 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.entity.software.base.location;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Predicate;
    +import com.google.common.base.Predicates;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.common.collect.Iterables;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.entity.EntitySpec;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.api.mgmt.Task;
    +import org.apache.brooklyn.core.mgmt.internal.EffectorUtils;
    +import org.apache.brooklyn.core.test.BrooklynAppLiveTestSupport;
    +import org.apache.brooklyn.entity.software.base.EmptySoftwareProcess;
    +import org.apache.brooklyn.entity.software.base.SoftwareProcess;
    +import org.apache.brooklyn.location.jclouds.JcloudsLocation;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.jclouds.compute.ComputeService;
    +import org.jclouds.compute.domain.SecurityGroup;
    +import org.jclouds.compute.extensions.SecurityGroupExtension;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +import org.testng.annotations.Test;
    +
    +import java.util.Map;
    +import java.util.Set;
    +
    +import static org.apache.brooklyn.test.Asserts.*;
    +import static org.jclouds.net.domain.IpProtocol.*;
    +
    +public abstract class NetworkingEffectorsLiveTests extends BrooklynAppLiveTestSupport {
    +    @Test
    +    public void testPassSecurityGroupParameters() {
    +        EmptySoftwareProcess emptySoftwareProcess = app.createAndManageChild(EntitySpec.create(EmptySoftwareProcess.class)
    +                .configure(SoftwareProcess.ADD_OPEN_PORTS_EFFECTOR, true));
    +
    +        JcloudsLocation jcloudsLocation = (JcloudsLocation)mgmt.getLocationRegistry().getLocationManaged(getLocationSpec(), getLocationProperties());
    +        app.start(ImmutableList.of(jcloudsLocation));
    +
    +        Optional<Location> jcloudsMachineLocation = Iterables.tryFind(emptySoftwareProcess.getLocations(), Predicates.instanceOf(JcloudsMachineLocation.class));
    +        if (!jcloudsMachineLocation.isPresent()) {
    +            throw new IllegalArgumentException("Tried to execute open ports effector on an entity with no JcloudsMachineLocation");
    +        }
    +        ComputeService computeService = ((JcloudsMachineLocation)jcloudsMachineLocation.get()).getParent().getComputeService();
    +        String nodeId = ((JcloudsMachineLocation)jcloudsMachineLocation.get()).getNode().getId();
    +        final SecurityGroupExtension securityApi = computeService.getSecurityGroupExtension().get();
    +
    +        Effector<Iterable<IpPermission>> openPortsInSecurityGroup = (Effector<Iterable<IpPermission>>)EffectorUtils.findEffectorDeclared(emptySoftwareProcess, "openPortsInSecurityGroup").get();
    +        Task<Iterable<IpPermission>> task = EffectorUtils.invokeEffectorAsync(emptySoftwareProcess, openPortsInSecurityGroup, ImmutableMap.of("inbound.ports.list", "234,324,550-1050"));
    --- End diff --
    
    Fixed


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75678644
  
    --- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/SoftwareProcess.java ---
    @@ -147,6 +147,9 @@
         @SetFromFlag("runDir")
         AttributeSensorAndConfigKey<String,String> RUN_DIR = BrooklynConfigKeys.RUN_DIR;
     
    +    ConfigKey<Boolean> ADD_OPEN_PORTS_EFFECTOR = ConfigKeys.newBooleanConfigKey("effector.add.openPorts",
    --- End diff --
    
    I think it's better if we use `AddEffector` interceptor instead.


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75678389
  
    --- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/SoftwareProcess.java ---
    @@ -147,6 +147,9 @@
         @SetFromFlag("runDir")
         AttributeSensorAndConfigKey<String,String> RUN_DIR = BrooklynConfigKeys.RUN_DIR;
     
    +    ConfigKey<Boolean> ADD_OPEN_PORTS_EFFECTOR = ConfigKeys.newBooleanConfigKey("effector.add.openPorts",
    --- End diff --
    
    You are using "inbound" and "open" interchangeably. Better stick to one of them for consistency.


---
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] brooklyn-server issue #276: (For Review) Effector for opening inbound ports ...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:

    https://github.com/apache/brooklyn-server/pull/276
  
    I've merged #298 which has your first commit. Can you rebase against master please?


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75093209
  
    --- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/NetworkingEffectors.java ---
    @@ -0,0 +1,88 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.location.jclouds.networking;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Preconditions;
    +import com.google.common.base.Predicates;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Range;
    +import com.google.common.reflect.TypeToken;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.effector.EffectorBody;
    +import org.apache.brooklyn.core.effector.Effectors;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.apache.brooklyn.util.collections.MutableList;
    +import org.apache.brooklyn.util.core.BrooklynNetworkUtils;
    +import org.apache.brooklyn.util.core.config.ConfigBag;
    +import org.apache.brooklyn.util.net.Cidr;
    +import org.apache.brooklyn.util.net.Networking;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +
    +import java.util.List;
    +
    +public class NetworkingEffectors {
    +    // Intentionally not use CloudLocationConfig.INBOUND_PORTS to make richer syntax and rename it to differ it from the first in a ConfigBag
    +    public static final ConfigKey<List<String>> INBOUND_PORTS_LIST = ConfigKeys.newConfigKey(new TypeToken<List<String>>() {}, "inbound.ports.list",
    +            "Ports to open from the effector", ImmutableList.<String>of());
    +    public static final ConfigKey<IpProtocol> INBOUND_PORTS_LIST_PROTOCOL = ConfigKeys.newConfigKey(new TypeToken<IpProtocol>() {}, "inbound.ports.list.protocol",
    +            "Protocol for ports to open. Possible values: TCP, UDP, ICMP, ALL.", IpProtocol.TCP);
    +
    +    public static Effector<Iterable<IpPermission>> openPortsInSecurityGroupEffector() {
    +        return (Effector<Iterable<IpPermission>>)Effectors.effector(Iterable.class, "openPortsInSecurityGroup")
    --- End diff --
    
    Or even better, overloading `Effectors.effector` to take a TypeToken :-)


---
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] brooklyn-server issue #276: Effector for opening inbound ports in security g...

Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the issue:

    https://github.com/apache/brooklyn-server/pull/276
  
    Manually tested successfully with the following YAML:
    
    ```
    name: Simple Netcat Server & Effector
    location: aws-ec2:eu-west-1
    services:
    - type: org.apache.brooklyn.entity.software.base.VanillaSoftwareProcess
      name: Simple Netcat Server
      launch.command: |
        echo hello | nc -l 4321 &
        echo $! > $PID_FILE
      brooklyn.config:
        effector.add.openPorts: true
    ```
    
    I'm getting a 404 from Jenkins when I try to look a the build failure - I think Jenkins only keeps the logs for so long. It also builds locally - @bostko can you do a force push on your branch to re-trigger the jenkins build


---
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] brooklyn-server issue #276: Effector for opening inbound ports in security g...

Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on the issue:

    https://github.com/apache/brooklyn-server/pull/276
  
    retest this please


---
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] brooklyn-server issue #276: Effector for opening inbound ports in security g...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:

    https://github.com/apache/brooklyn-server/pull/276
  
    @bostko looks good to me. I'll merge now. Thanks for the amazingly fast update to add `@Beta`, even late at night!
    
    What are your thoughts on `openIptables`? It could well be that the port(s) will still be locked down in the iptables rules. Do you think that would be a separate effector (or script or whatever) to open those?


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75734999
  
    --- Diff: software/base/pom.xml ---
    @@ -116,7 +116,6 @@
                 <groupId>org.apache.brooklyn</groupId>
                 <artifactId>brooklyn-locations-jclouds</artifactId>
                 <version>${project.version}</version>
    -            <scope>test</scope>
    --- End diff --
    
    I need a non-test dependency on `brooklyn-locations-jclouds`. Tests also need code from `brooklyn-locations-jclouds` so adding a simple dependency with default compile scope is enough I think?
    @neykov 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] brooklyn-server pull request #276: (For Review) Effector for opening inbound...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r72586637
  
    --- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/SoftwareProcess.java ---
    @@ -147,6 +147,8 @@
         @SetFromFlag("runDir")
         AttributeSensorAndConfigKey<String,String> RUN_DIR = BrooklynConfigKeys.RUN_DIR;
     
    +    ConfigKey<Boolean> ADD_OPEN_PORTS_EFFECTOR = ConfigKeys.newBooleanConfigKey("effector.add.openPorts", "Flag which adds effector for opening ports through Cloud security groups");
    --- End diff --
    
    I prefer giving an explicit default, rather than null.
    
    (But I do see you handle null in SoftwareProcessImpl.init, so the code does work).


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75676700
  
    --- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/NetworkingEffectors.java ---
    @@ -0,0 +1,90 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.location.jclouds.networking;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Preconditions;
    +import com.google.common.base.Predicates;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Range;
    +import com.google.common.reflect.TypeToken;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.effector.EffectorBody;
    +import org.apache.brooklyn.core.effector.Effectors;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.apache.brooklyn.util.collections.MutableList;
    +import org.apache.brooklyn.util.core.BrooklynNetworkUtils;
    +import org.apache.brooklyn.util.core.config.ConfigBag;
    +import org.apache.brooklyn.util.net.Cidr;
    +import org.apache.brooklyn.util.net.Networking;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +
    +import java.util.List;
    +
    +public class NetworkingEffectors {
    +    // Intentionally not use CloudLocationConfig.INBOUND_PORTS to make richer syntax and rename it to differ it from the first in a ConfigBag
    +    public static final ConfigKey<List<String>> INBOUND_PORTS_LIST = ConfigKeys.newConfigKey(new TypeToken<List<String>>() {}, "inbound.ports.list",
    +            "Ports to open from the effector", ImmutableList.<String>of());
    +    public static final ConfigKey<IpProtocol> INBOUND_PORTS_LIST_PROTOCOL = ConfigKeys.newConfigKey(new TypeToken<IpProtocol>() {}, "inbound.ports.list.protocol",
    +            "Protocol for ports to open. Possible values: TCP, UDP, ICMP, ALL.", IpProtocol.TCP);
    +
    +    public static Effector<Iterable<IpPermission>> openPortsInSecurityGroupEffector() {
    +        return Effectors.effector(EffectorResult.class, "openPortsInSecurityGroup")
    +                .parameter(INBOUND_PORTS_LIST)
    +                .parameter(INBOUND_PORTS_LIST_PROTOCOL)
    +                .description("Open ports in Cloud Security Group. If called before machine location is provisioned, it will fail.")
    +                .impl((EffectorBody)new OpenPortsInSecurityGroupBody())
    +                .build();
    +    }
    +
    +    private interface EffectorResult extends Iterable<IpPermission> {}
    +
    +    private static class OpenPortsInSecurityGroupBody extends EffectorBody<Iterable<IpPermission>> {
    +        @Override
    +        public Iterable<IpPermission> call(ConfigBag parameters) {
    +            List<String> rawPortRules = parameters.get(INBOUND_PORTS_LIST);
    +            IpProtocol ipProtocol = parameters.get(INBOUND_PORTS_LIST_PROTOCOL);
    +            Preconditions.checkNotNull(ipProtocol);
    +            Preconditions.checkNotNull(rawPortRules, "ports cannot be null");
    --- End diff --
    
    use the parameter name `inbound.ports.list`


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75860988
  
    --- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/NetworkingEffectors.java ---
    @@ -0,0 +1,103 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.location.jclouds.networking;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Preconditions;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Range;
    +import com.google.common.reflect.TypeToken;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.effector.EffectorBody;
    +import org.apache.brooklyn.core.effector.Effectors;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.apache.brooklyn.util.collections.MutableList;
    +import org.apache.brooklyn.util.core.config.ConfigBag;
    +import org.apache.brooklyn.util.net.Cidr;
    +import org.apache.brooklyn.util.net.Networking;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +
    +import java.util.List;
    +
    +import static com.google.common.base.Predicates.instanceOf;
    +import static com.google.common.collect.Iterables.tryFind;
    +import static org.apache.brooklyn.core.location.Locations.getLocationsCheckingAncestors;
    +
    +public class NetworkingEffectors {
    +    // Intentionally not use CloudLocationConfig.INBOUND_PORTS to make richer syntax and rename it to differ it from the first in a ConfigBag
    +    public static final ConfigKey<List<String>> INBOUND_PORTS_LIST = ConfigKeys.newConfigKey(new TypeToken<List<String>>() {}, "inbound.ports.list",
    +            "Ports to open from the effector", ImmutableList.<String>of());
    +    public static final ConfigKey<IpProtocol> INBOUND_PORTS_LIST_PROTOCOL = ConfigKeys.newConfigKey(new TypeToken<IpProtocol>() {}, "inbound.ports.list.protocol",
    +            "Protocol for ports to open. Possible values: TCP, UDP, ICMP, ALL.", IpProtocol.TCP);
    +
    +    public static final ConfigKey<JcloudsMachineLocation> JCLOUDS_MACHINE_LOCATIN = ConfigKeys.newConfigKey(JcloudsMachineLocation.class, "jcloudsMachineLocation");
    +
    +    @SuppressWarnings("unchecked")
    +    public static final Effector<Iterable<IpPermission>> OPEN_INBOUND_PORTS_IN_SECURITY_GROUP_EFFECTOR = (Effector<Iterable<IpPermission>>)(Effector<?>)Effectors.effector(Iterable.class, "openPortsInSecurityGroup")
    +                .parameter(INBOUND_PORTS_LIST)
    +                .parameter(INBOUND_PORTS_LIST_PROTOCOL)
    +                .description("Open ports in Cloud Security Group. If called before machine location is provisioned, it will fail.")
    +                .impl(new OpenPortsInSecurityGroupBody())
    +                .build();
    +
    +    @SuppressWarnings("rawtypes")
    +    private static class OpenPortsInSecurityGroupBody extends EffectorBody<Iterable> {
    +        @Override
    +        public Iterable<IpPermission> call(ConfigBag parameters) {
    +            List<String> rawPortRules = parameters.get(INBOUND_PORTS_LIST);
    +            IpProtocol ipProtocol = parameters.get(INBOUND_PORTS_LIST_PROTOCOL);
    +            JcloudsMachineLocation jcloudsMachineLocation = parameters.get(JCLOUDS_MACHINE_LOCATIN);
    +            Preconditions.checkNotNull(ipProtocol, INBOUND_PORTS_LIST_PROTOCOL.getName() + " cannot be null");
    +            Preconditions.checkNotNull(rawPortRules, INBOUND_PORTS_LIST.getName() + " cannot be null");
    +            MutableList.Builder<IpPermission> ipPermissionsBuilder = MutableList.builder();
    +            for (Range<Integer> portRule : Networking.portRulesToRanges(rawPortRules).asRanges()) {
    +                ipPermissionsBuilder.add(
    +                        IpPermission.builder()
    +                                .ipProtocol(ipProtocol)
    +                                .fromPort(portRule.lowerEndpoint())
    +                                .toPort(portRule.upperEndpoint())
    +                                .cidrBlock(Cidr.UNIVERSAL.toString())
    +                                .build());
    +            }
    +            JcloudsLocationSecurityGroupCustomizer customizer = JcloudsLocationSecurityGroupCustomizer.getInstance(entity());
    +
    +            if (jcloudsMachineLocation == null) {
    +                Optional<Location> jcloudsMachineLocationOptional = tryFind(
    +                        (Iterable<Location>) getLocationsCheckingAncestors(null, entity()),
    +                        instanceOf(JcloudsMachineLocation.class));
    +                if (!jcloudsMachineLocationOptional.isPresent()) {
    +                    throw new IllegalArgumentException("Tried to execute open ports effector on an entity with no JcloudsMachineLocation");
    +                } else {
    +                    jcloudsMachineLocation = (JcloudsMachineLocation)jcloudsMachineLocationOptional.get();
    +                }
    +            }
    +            Iterable<IpPermission> ipPermissionsToAdd = ipPermissionsBuilder.build();
    +            customizer.addPermissionsToLocation(jcloudsMachineLocation, ipPermissionsToAdd);
    +            return ipPermissionsToAdd;
    +        }
    +    }
    +
    +    public static void main(String[] args) {
    --- End diff --
    
    Suggest we delete this `main` method.


---
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] brooklyn-server issue #276: Effector for opening inbound ports in security g...

Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the issue:

    https://github.com/apache/brooklyn-server/pull/276
  
    In terms of user prospective I think it should be optional and may be within the same effector.


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75679060
  
    --- Diff: software/base/src/test/java/org/apache/brooklyn/entity/software/base/location/NetworkingEffectorsLiveTests.java ---
    @@ -0,0 +1,107 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.entity.software.base.location;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Predicate;
    +import com.google.common.base.Predicates;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.common.collect.Iterables;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.entity.EntitySpec;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.api.mgmt.Task;
    +import org.apache.brooklyn.core.mgmt.internal.EffectorUtils;
    +import org.apache.brooklyn.core.test.BrooklynAppLiveTestSupport;
    +import org.apache.brooklyn.entity.software.base.EmptySoftwareProcess;
    +import org.apache.brooklyn.entity.software.base.SoftwareProcess;
    +import org.apache.brooklyn.location.jclouds.JcloudsLocation;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.jclouds.compute.ComputeService;
    +import org.jclouds.compute.domain.SecurityGroup;
    +import org.jclouds.compute.extensions.SecurityGroupExtension;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +import org.testng.annotations.Test;
    +
    +import java.util.Map;
    +import java.util.Set;
    +
    +import static org.apache.brooklyn.test.Asserts.*;
    +import static org.jclouds.net.domain.IpProtocol.*;
    +
    +public abstract class NetworkingEffectorsLiveTests extends BrooklynAppLiveTestSupport {
    +    @Test
    +    public void testPassSecurityGroupParameters() {
    +        EmptySoftwareProcess emptySoftwareProcess = app.createAndManageChild(EntitySpec.create(EmptySoftwareProcess.class)
    +                .configure(SoftwareProcess.ADD_OPEN_PORTS_EFFECTOR, true));
    +
    +        JcloudsLocation jcloudsLocation = (JcloudsLocation)mgmt.getLocationRegistry().getLocationManaged(getLocationSpec(), getLocationProperties());
    +        app.start(ImmutableList.of(jcloudsLocation));
    +
    +        Optional<Location> jcloudsMachineLocation = Iterables.tryFind(emptySoftwareProcess.getLocations(), Predicates.instanceOf(JcloudsMachineLocation.class));
    +        if (!jcloudsMachineLocation.isPresent()) {
    +            throw new IllegalArgumentException("Tried to execute open ports effector on an entity with no JcloudsMachineLocation");
    +        }
    +        ComputeService computeService = ((JcloudsMachineLocation)jcloudsMachineLocation.get()).getParent().getComputeService();
    +        String nodeId = ((JcloudsMachineLocation)jcloudsMachineLocation.get()).getNode().getId();
    +        final SecurityGroupExtension securityApi = computeService.getSecurityGroupExtension().get();
    +
    +        Effector<Iterable<IpPermission>> openPortsInSecurityGroup = (Effector<Iterable<IpPermission>>)EffectorUtils.findEffectorDeclared(emptySoftwareProcess, "openPortsInSecurityGroup").get();
    +        Task<Iterable<IpPermission>> task = EffectorUtils.invokeEffectorAsync(emptySoftwareProcess, openPortsInSecurityGroup, ImmutableMap.of("inbound.ports.list", "234,324,550-1050"));
    --- End diff --
    
    Could use the Java config key instead of name.


---
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] brooklyn-server pull request #276: (For Review) Effector for opening inbound...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r72585710
  
    --- Diff: core/src/main/java/org/apache/brooklyn/util/core/BrooklynNetworkUtils.java ---
    @@ -39,4 +43,22 @@ public static InetAddress getLocalhostInetAddress() {
                     Networking.getLocalHost()), InetAddress.class);
         }
     
    +    // TODO it does not add adjacent intervals: {[22, 22], [23, 23]} is not merged to {[22, 23]}
    +    public static RangeSet<Integer> portRulesToRanges(Collection<String> portRules) {
    +        RangeSet<Integer> result = TreeRangeSet.create();
    +        for (String portRule : portRules) {
    +            if (portRule.contains("-")) {
    +                String[] fromTo = portRule.split("-");
    +                assert fromTo.length == 2;
    --- End diff --
    
    Don't use `assert` for validating user input. Instead we'd want to throw a nicer `IllegalArgumentException` to tell the caller that they passed in the wrong input.
    
    Only use `assert` when it's about the internal invariants of a class (e.g. validating args in private methods, etc).


---
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] brooklyn-server pull request #276: (For Review) Effector for opening inbound...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r72586500
  
    --- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/NetworkingEffectors.java ---
    @@ -0,0 +1,80 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.location.jclouds.networking;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Preconditions;
    +import com.google.common.base.Predicates;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Range;
    +import com.google.common.reflect.TypeToken;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.effector.EffectorBody;
    +import org.apache.brooklyn.core.effector.Effectors;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.apache.brooklyn.util.collections.MutableList;
    +import org.apache.brooklyn.util.core.BrooklynNetworkUtils;
    +import org.apache.brooklyn.util.core.config.ConfigBag;
    +import org.apache.brooklyn.util.net.Cidr;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +
    +import java.util.List;
    +
    +public class NetworkingEffectors {
    +    public static final ConfigKey<List<String>> INBOUND_PORTS_EFFECTOR = ConfigKeys.newConfigKey(new TypeToken<List<String>>() {}, "inbound.ports.effector",
    --- End diff --
    
    "inbound.ports.effector" seems like a strange name. This isn't an effector, I believe: it's a parameter for the effector, yes?


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75681798
  
    --- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/NetworkingEffectors.java ---
    @@ -0,0 +1,90 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.location.jclouds.networking;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Preconditions;
    +import com.google.common.base.Predicates;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Range;
    +import com.google.common.reflect.TypeToken;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.effector.EffectorBody;
    +import org.apache.brooklyn.core.effector.Effectors;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.apache.brooklyn.util.collections.MutableList;
    +import org.apache.brooklyn.util.core.BrooklynNetworkUtils;
    +import org.apache.brooklyn.util.core.config.ConfigBag;
    +import org.apache.brooklyn.util.net.Cidr;
    +import org.apache.brooklyn.util.net.Networking;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +
    +import java.util.List;
    +
    +public class NetworkingEffectors {
    +    // Intentionally not use CloudLocationConfig.INBOUND_PORTS to make richer syntax and rename it to differ it from the first in a ConfigBag
    +    public static final ConfigKey<List<String>> INBOUND_PORTS_LIST = ConfigKeys.newConfigKey(new TypeToken<List<String>>() {}, "inbound.ports.list",
    +            "Ports to open from the effector", ImmutableList.<String>of());
    +    public static final ConfigKey<IpProtocol> INBOUND_PORTS_LIST_PROTOCOL = ConfigKeys.newConfigKey(new TypeToken<IpProtocol>() {}, "inbound.ports.list.protocol",
    +            "Protocol for ports to open. Possible values: TCP, UDP, ICMP, ALL.", IpProtocol.TCP);
    +
    +    public static Effector<Iterable<IpPermission>> openPortsInSecurityGroupEffector() {
    --- End diff --
    
    Turn this into a constant to make it easier to call the effector in Java.


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75677727
  
    --- Diff: software/base/pom.xml ---
    @@ -116,7 +116,6 @@
                 <groupId>org.apache.brooklyn</groupId>
                 <artifactId>brooklyn-locations-jclouds</artifactId>
                 <version>${project.version}</version>
    -            <scope>test</scope>
    --- End diff --
    
    Why remove it from here, but leave all following dependencies the same. If you need a (non-test) dependency on `brooklyn-locations-jclouds` add a new one instead.


---
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] brooklyn-server pull request #276: (For Review) Effector for opening inbound...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r72586282
  
    --- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/NetworkingEffectors.java ---
    @@ -0,0 +1,80 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.location.jclouds.networking;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Preconditions;
    +import com.google.common.base.Predicates;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Range;
    +import com.google.common.reflect.TypeToken;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.effector.EffectorBody;
    +import org.apache.brooklyn.core.effector.Effectors;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.apache.brooklyn.util.collections.MutableList;
    +import org.apache.brooklyn.util.core.BrooklynNetworkUtils;
    +import org.apache.brooklyn.util.core.config.ConfigBag;
    +import org.apache.brooklyn.util.net.Cidr;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +
    +import java.util.List;
    +
    +public class NetworkingEffectors {
    +    public static final ConfigKey<List<String>> INBOUND_PORTS_EFFECTOR = ConfigKeys.newConfigKey(new TypeToken<List<String>>() {}, "inbound.ports.effector",
    +            "Ports to open from the effector", ImmutableList.<String>of());
    +
    +    public static Effector<Void> openPortsInSecurityGroupEffector() {
    +        return Effectors.effector(Void.class, "openPortsInSecurityGroup")
    +                .parameter(INBOUND_PORTS_EFFECTOR)
    +                .description("Open ports in Cloud Security Group")
    +                .impl(new OpenPortsInSecurityGroupBody())
    +                .build();
    +    }
    +
    +    private static class OpenPortsInSecurityGroupBody extends EffectorBody<Void> {
    --- End diff --
    
    We'd also like this to work in other clouds, such as downsteram projects that use vCloudDirector via the brooklyn-networking `SubnetTier`. I need to think about that more, for how we structure that.



---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/brooklyn-server/pull/276


---
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] brooklyn-server issue #276: Effector for opening inbound ports in security g...

Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on the issue:

    https://github.com/apache/brooklyn-server/pull/276
  
    (above was a command to jenkins, not to the submitter!)
    (but it looks like it didn't work anyway)


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75677407
  
    --- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/NetworkingEffectors.java ---
    @@ -0,0 +1,90 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.location.jclouds.networking;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Preconditions;
    +import com.google.common.base.Predicates;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Range;
    +import com.google.common.reflect.TypeToken;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.effector.EffectorBody;
    +import org.apache.brooklyn.core.effector.Effectors;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.apache.brooklyn.util.collections.MutableList;
    +import org.apache.brooklyn.util.core.BrooklynNetworkUtils;
    +import org.apache.brooklyn.util.core.config.ConfigBag;
    +import org.apache.brooklyn.util.net.Cidr;
    +import org.apache.brooklyn.util.net.Networking;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +
    +import java.util.List;
    +
    +public class NetworkingEffectors {
    +    // Intentionally not use CloudLocationConfig.INBOUND_PORTS to make richer syntax and rename it to differ it from the first in a ConfigBag
    +    public static final ConfigKey<List<String>> INBOUND_PORTS_LIST = ConfigKeys.newConfigKey(new TypeToken<List<String>>() {}, "inbound.ports.list",
    +            "Ports to open from the effector", ImmutableList.<String>of());
    +    public static final ConfigKey<IpProtocol> INBOUND_PORTS_LIST_PROTOCOL = ConfigKeys.newConfigKey(new TypeToken<IpProtocol>() {}, "inbound.ports.list.protocol",
    +            "Protocol for ports to open. Possible values: TCP, UDP, ICMP, ALL.", IpProtocol.TCP);
    +
    +    public static Effector<Iterable<IpPermission>> openPortsInSecurityGroupEffector() {
    +        return Effectors.effector(EffectorResult.class, "openPortsInSecurityGroup")
    +                .parameter(INBOUND_PORTS_LIST)
    +                .parameter(INBOUND_PORTS_LIST_PROTOCOL)
    +                .description("Open ports in Cloud Security Group. If called before machine location is provisioned, it will fail.")
    +                .impl((EffectorBody)new OpenPortsInSecurityGroupBody())
    +                .build();
    +    }
    +
    +    private interface EffectorResult extends Iterable<IpPermission> {}
    +
    +    private static class OpenPortsInSecurityGroupBody extends EffectorBody<Iterable<IpPermission>> {
    +        @Override
    +        public Iterable<IpPermission> call(ConfigBag parameters) {
    +            List<String> rawPortRules = parameters.get(INBOUND_PORTS_LIST);
    +            IpProtocol ipProtocol = parameters.get(INBOUND_PORTS_LIST_PROTOCOL);
    +            Preconditions.checkNotNull(ipProtocol);
    +            Preconditions.checkNotNull(rawPortRules, "ports cannot be null");
    +            MutableList.Builder<IpPermission> ipPermissionsBuilder = MutableList.builder();
    +            for (Range<Integer> portRule : Networking.portRulesToRanges(rawPortRules).asRanges()) {
    +                ipPermissionsBuilder.add(
    +                        IpPermission.builder()
    +                                .ipProtocol(ipProtocol)
    +                                .fromPort(portRule.lowerEndpoint())
    +                                .toPort(portRule.upperEndpoint())
    +                                .cidrBlock(Cidr.UNIVERSAL.toString())
    +                                .build());
    +            }
    +            JcloudsLocationSecurityGroupCustomizer customizer = JcloudsLocationSecurityGroupCustomizer.getInstance(entity());
    +            Optional<Location> jcloudsMachineLocation = Iterables.tryFind(entity().getLocations(), Predicates.instanceOf(JcloudsMachineLocation.class));
    --- End diff --
    
    Use `Locations.getLocationsCheckingAncestors(locations, entity);` instead - the location could be declared on parent entities (SameServerEntity).


---
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] brooklyn-server pull request #276: (For Review) Effector for opening inbound...

Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r72585644
  
    --- Diff: core/src/main/java/org/apache/brooklyn/util/core/BrooklynNetworkUtils.java ---
    @@ -39,4 +43,22 @@ public static InetAddress getLocalhostInetAddress() {
                     Networking.getLocalHost()), InetAddress.class);
         }
     
    +    // TODO it does not add adjacent intervals: {[22, 22], [23, 23]} is not merged to {[22, 23]}
    +    public static RangeSet<Integer> portRulesToRanges(Collection<String> portRules) {
    +        RangeSet<Integer> result = TreeRangeSet.create();
    +        for (String portRule : portRules) {
    +            if (portRule.contains("-")) {
    +                String[] fromTo = portRule.split("-");
    +                assert fromTo.length == 2;
    --- End diff --
    
    Assertions are disabled by default, use `Preconditions.checkState`


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75856867
  
    --- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/SoftwareProcess.java ---
    @@ -147,6 +147,9 @@
         @SetFromFlag("runDir")
         AttributeSensorAndConfigKey<String,String> RUN_DIR = BrooklynConfigKeys.RUN_DIR;
     
    +    ConfigKey<Boolean> ADD_OPEN_PORTS_EFFECTOR = ConfigKeys.newBooleanConfigKey("effector.add.openPorts",
    --- End diff --
    
    My idea was that the class `AddEffector` is a generic way to attach effectors to entities in yaml. Then no need for an ad-hoc flag to attach the effector.
    Looking at the implementation though it's not straightforward to use it in YAML currently so ignore.


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75974892
  
    --- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/NetworkingEffectors.java ---
    @@ -0,0 +1,100 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.location.jclouds.networking;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Preconditions;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Range;
    +import com.google.common.reflect.TypeToken;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.effector.EffectorBody;
    +import org.apache.brooklyn.core.effector.Effectors;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.apache.brooklyn.util.collections.MutableList;
    +import org.apache.brooklyn.util.core.config.ConfigBag;
    +import org.apache.brooklyn.util.net.Cidr;
    +import org.apache.brooklyn.util.net.Networking;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +
    +import java.util.List;
    +
    +import static com.google.common.base.Predicates.instanceOf;
    +import static com.google.common.collect.Iterables.tryFind;
    +import static org.apache.brooklyn.core.location.Locations.getLocationsCheckingAncestors;
    +
    +public class NetworkingEffectors {
    --- End diff --
    
    Worth including `@beta`, I think.
    
    I was imagining that we could tell it to open certain TCP ports and certain UDP ports in a single effector call. But I see that your approach works (one can make multiple effector calls), and is probably simpler than having a map from protocol to a list of port ranges (or whatever scheme one comes up with). So I'm happy with this.


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75852452
  
    --- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/SoftwareProcess.java ---
    @@ -147,6 +147,9 @@
         @SetFromFlag("runDir")
         AttributeSensorAndConfigKey<String,String> RUN_DIR = BrooklynConfigKeys.RUN_DIR;
     
    +    ConfigKey<Boolean> ADD_OPEN_PORTS_EFFECTOR = ConfigKeys.newBooleanConfigKey("effector.add.openPorts",
    --- End diff --
    
    Why to use AddEffector? Isn't it interchangeable with  `Effectors.effector`?


---
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] brooklyn-server issue #276: Effector for opening inbound ports in security g...

Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the issue:

    https://github.com/apache/brooklyn-server/pull/276
  
    I added a new commit with `InboundPortsJcloudsLocationCustomizer`:
    This is the yaml I tested with:
    ```yaml
    location:
      AWS Oregon:
        hardwareId: t1.micro
        customizers:
        - $brooklyn:object:
            type: org.apache.brooklyn.location.jclouds.networking.InboundPortsJcloudsLocationCustomizer
            object.fields: {inboundPortsList: "23,55", inboundPortsListProtocol: UDP}
        - $brooklyn:object:
            type: org.apache.brooklyn.location.jclouds.networking.InboundPortsJcloudsLocationCustomizer
            object.fields: {inboundPortsList: "200-300,500", inboundPortsListProtocol: TCP}
    services:
    - type: org.apache.brooklyn.entity.software.base.EmptySoftwareProcess
    ```


---
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] brooklyn-server pull request #276: (For Review) Effector for opening inbound...

Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75093029
  
    --- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/NetworkingEffectors.java ---
    @@ -0,0 +1,88 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.location.jclouds.networking;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Preconditions;
    +import com.google.common.base.Predicates;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Range;
    +import com.google.common.reflect.TypeToken;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.effector.EffectorBody;
    +import org.apache.brooklyn.core.effector.Effectors;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.apache.brooklyn.util.collections.MutableList;
    +import org.apache.brooklyn.util.core.BrooklynNetworkUtils;
    +import org.apache.brooklyn.util.core.config.ConfigBag;
    +import org.apache.brooklyn.util.net.Cidr;
    +import org.apache.brooklyn.util.net.Networking;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +
    +import java.util.List;
    +
    +public class NetworkingEffectors {
    +    // Intentionally not use CloudLocationConfig.INBOUND_PORTS to make richer syntax and rename it to differ it from the first in a ConfigBag
    +    public static final ConfigKey<List<String>> INBOUND_PORTS_LIST = ConfigKeys.newConfigKey(new TypeToken<List<String>>() {}, "inbound.ports.list",
    +            "Ports to open from the effector", ImmutableList.<String>of());
    +    public static final ConfigKey<IpProtocol> INBOUND_PORTS_LIST_PROTOCOL = ConfigKeys.newConfigKey(new TypeToken<IpProtocol>() {}, "inbound.ports.list.protocol",
    +            "Protocol for ports to open. Possible values: TCP, UDP, ICMP, ALL.", IpProtocol.TCP);
    +
    +    public static Effector<Iterable<IpPermission>> openPortsInSecurityGroupEffector() {
    +        return (Effector<Iterable<IpPermission>>)Effectors.effector(Iterable.class, "openPortsInSecurityGroup")
    --- End diff --
    
    This fails to build in IntelliJ, replacing with `return (Effector<Iterable<IpPermission>>)Effectors.effector((new TypeToken<Iterable<IpPermission>>(){}).getRawType(), "openPortsInSecurityGroup")` would keep IDEs happy, but it builds in Maven, so no strong feelings


---
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] brooklyn-server issue #276: Effector for opening inbound ports in security g...

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:

    https://github.com/apache/brooklyn-server/pull/276
  
    LGTM, minor comments only. 


---
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] brooklyn-server issue #276: (For Review) Effector for opening inbound ports ...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:

    https://github.com/apache/brooklyn-server/pull/276
  
    It would be good to have test(s). Have you thought about how to write those?


---
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] brooklyn-server pull request #276: Effector for opening inbound ports in sec...

Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/276#discussion_r75850750
  
    --- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/NetworkingEffectors.java ---
    @@ -0,0 +1,90 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.brooklyn.location.jclouds.networking;
    +
    +import com.google.common.base.Optional;
    +import com.google.common.base.Preconditions;
    +import com.google.common.base.Predicates;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Range;
    +import com.google.common.reflect.TypeToken;
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.location.Location;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.effector.EffectorBody;
    +import org.apache.brooklyn.core.effector.Effectors;
    +import org.apache.brooklyn.location.jclouds.JcloudsMachineLocation;
    +import org.apache.brooklyn.util.collections.MutableList;
    +import org.apache.brooklyn.util.core.BrooklynNetworkUtils;
    +import org.apache.brooklyn.util.core.config.ConfigBag;
    +import org.apache.brooklyn.util.net.Cidr;
    +import org.apache.brooklyn.util.net.Networking;
    +import org.jclouds.net.domain.IpPermission;
    +import org.jclouds.net.domain.IpProtocol;
    +
    +import java.util.List;
    +
    +public class NetworkingEffectors {
    +    // Intentionally not use CloudLocationConfig.INBOUND_PORTS to make richer syntax and rename it to differ it from the first in a ConfigBag
    +    public static final ConfigKey<List<String>> INBOUND_PORTS_LIST = ConfigKeys.newConfigKey(new TypeToken<List<String>>() {}, "inbound.ports.list",
    +            "Ports to open from the effector", ImmutableList.<String>of());
    +    public static final ConfigKey<IpProtocol> INBOUND_PORTS_LIST_PROTOCOL = ConfigKeys.newConfigKey(new TypeToken<IpProtocol>() {}, "inbound.ports.list.protocol",
    +            "Protocol for ports to open. Possible values: TCP, UDP, ICMP, ALL.", IpProtocol.TCP);
    +
    +    public static Effector<Iterable<IpPermission>> openPortsInSecurityGroupEffector() {
    +        return Effectors.effector(EffectorResult.class, "openPortsInSecurityGroup")
    +                .parameter(INBOUND_PORTS_LIST)
    +                .parameter(INBOUND_PORTS_LIST_PROTOCOL)
    +                .description("Open ports in Cloud Security Group. If called before machine location is provisioned, it will fail.")
    +                .impl((EffectorBody)new OpenPortsInSecurityGroupBody())
    +                .build();
    +    }
    +
    +    private interface EffectorResult extends Iterable<IpPermission> {}
    +
    +    private static class OpenPortsInSecurityGroupBody extends EffectorBody<Iterable<IpPermission>> {
    +        @Override
    +        public Iterable<IpPermission> call(ConfigBag parameters) {
    +            List<String> rawPortRules = parameters.get(INBOUND_PORTS_LIST);
    +            IpProtocol ipProtocol = parameters.get(INBOUND_PORTS_LIST_PROTOCOL);
    +            Preconditions.checkNotNull(ipProtocol);
    +            Preconditions.checkNotNull(rawPortRules, "ports cannot be null");
    +            MutableList.Builder<IpPermission> ipPermissionsBuilder = MutableList.builder();
    +            for (Range<Integer> portRule : Networking.portRulesToRanges(rawPortRules).asRanges()) {
    +                ipPermissionsBuilder.add(
    +                        IpPermission.builder()
    +                                .ipProtocol(ipProtocol)
    +                                .fromPort(portRule.lowerEndpoint())
    +                                .toPort(portRule.upperEndpoint())
    +                                .cidrBlock(Cidr.UNIVERSAL.toString())
    +                                .build());
    +            }
    +            JcloudsLocationSecurityGroupCustomizer customizer = JcloudsLocationSecurityGroupCustomizer.getInstance(entity());
    +            Optional<Location> jcloudsMachineLocation = Iterables.tryFind(entity().getLocations(), Predicates.instanceOf(JcloudsMachineLocation.class));
    --- End diff --
    
    wdyt about using `tryFind((Iterable<Location>) getLocationsCheckingAncestors(null, entity()),                       instanceOf(JcloudsMachineLocation.class))`


---
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.
---