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
