jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] JCLOUDS-1208 Implement the TemplateOptions.inboundPorts in ProfitBricks REST (#344)
Date Wed, 18 Jan 2017 15:28:12 GMT
nacx requested changes on this pull request.

Reviewed. Thanks @alibazlamit! Looks simpler than I thought :)

>        trackables.waitUntilRequestCompleted(nic);
       waitNICUntilAvailable.apply(NicRef.create(dataCenterId, server.id(), nic.id()));
       waitDcUntilAvailable.apply(dataCenterId);
       waitServerUntilAvailable.apply(ServerRef.create(dataCenterId, server.id()));
 
+      for (int inboundPort : inboundPorts) {
+         FirewallRule rule = api.firewallApi().create(
+                 FirewallRule.Request.creatingBuilder()
+                 .dataCenterId(dataCenterId)
+                 .serverId(server.id())
+                 .nicId(nic.id())
+                 .name(server.properties().name() + " jclouds-firewall")
+                 .protocol(FirewallRule.Protocol.TCP)
+                 .portRangeStart(inboundPort)
+                 .portRangeEnd(inboundPort)
+                 .build()
+         );
+         trackables.waitUntilRequestCompleted(rule);
+
+      }

Better use the [ComputeServiceUtils. getPortRangesFromList](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/util/ComputeServiceUtils.java#L265-L281)
to compute the ranges and create the minimum set of rules.

> @@ -276,20 +274,42 @@ public boolean apply(Lan input) {
          }
       }
 
+      List<FirewallRule> firewallrules = new ArrayList<FirewallRule>();

Unused?

>        Nic nic = api.nicApi().create(Nic.Request.creatingBuilder()
               .dataCenterId(dataCenterId)
               .name("jclouds" + name)
               .dhcp(Boolean.TRUE)
               .lan(lanId)
-              .firewallActive(Boolean.FALSE)
+              .firewallActive(firewallActive)

Just `.firewallActive(inboundPorts.length > 0)` and remove the previous conditional.

>     @Override
    protected Iterable<Module> setupModules() {
       ImmutableSet.Builder<Module> modules = ImmutableSet.builder();
       modules.addAll(super.setupModules());
-      modules.add(new ProfitBricksRateLimitModule());

Why is this removed?

> +         int matches = 0;
+         client.getSecurityGroupExtension();
+         NodeMetadata node = getOnlyElement(client.createNodesInGroup(group + "inbound",
1, template));
+         DataCenterAndId datacenterAndId = DataCenterAndId.fromSlashEncoded(node.getId());
+         ProfitBricksApi pbApi = client.getContext().unwrapApi(ProfitBricksApi.class);
+         Server server = pbApi.serverApi().getServer(datacenterAndId.getDataCenter(), datacenterAndId.getId(),
new DepthOptions().depth(5));
+         for (FirewallRule rule : server.entities().nics().items().get(0).entities().firewallrules().items())
{
+            if (rule.properties().portRangeStart() == 80 || rule.properties().portRangeStart()
== 22 || rule.properties().portRangeStart() == 443) {
+               matches++;
+            }
+         }
+         Assert.assertEquals(3, matches);
+      } finally {
+         client.destroyNodesMatching(inGroup(group + "inbound"));
+      }
+   }

The `createAndRunAServiceInGroup`method of the parent class already configures a couple inbound
ports for some tests. You could override that method, call super and just add the firewall
rule validations after. Then you could remove this additional test.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/344#pullrequestreview-17246072
Mime
View raw message