You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by alibazlamit <no...@github.com> on 2017/01/18 10:09:06 UTC
[jclouds/jclouds-labs] JCLOUDS-1208 Implement the
TemplateOptions.inboundPorts in ProfitBricks REST (#344)
@nacx An implementation for [this](https://issues.apache.org/jira/browse/JCLOUDS-1208?filter=-3).
Thanks
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/344
-- Commit Summary --
* JCLOUDS-1208 Implement the TemplateOptions.inboundPorts in ProfitBricks REST
-- File Changes --
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/compute/ProfitBricksComputeServiceAdapter.java (78)
M profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/compute/ProfitBricksComputeServiceLiveTest.java (38)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/344.patch
https://github.com/jclouds/jclouds-labs/pull/344.diff
--
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
Re: [jclouds/jclouds-labs] JCLOUDS-1208 Implement the
TemplateOptions.inboundPorts in ProfitBricks REST (#344)
Posted by Ignasi Barrera <no...@github.com>.
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
Re: [jclouds/jclouds-labs] JCLOUDS-1208 Implement the
TemplateOptions.inboundPorts in ProfitBricks REST (#344)
Posted by Ignasi Barrera <no...@github.com>.
Backported to [2.0.x](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/53439188) too, since it does not introduce backwards incompatible changes.
--
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#issuecomment-275066224
Re: [jclouds/jclouds-labs] JCLOUDS-1208 Implement the
TemplateOptions.inboundPorts in ProfitBricks REST (#344)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @alibazlamit! I'll review the PR in a while. Take care and edit the gist to remove your credentials, and change them in the provider now that they have been exposed!
--
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#issuecomment-273502700
Re: [jclouds/jclouds-labs] JCLOUDS-1208 Implement the
TemplateOptions.inboundPorts in ProfitBricks REST (#344)
Posted by Ignasi Barrera <no...@github.com>.
nacx approved this pull request.
--
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-18360738
Re: [jclouds/jclouds-labs] JCLOUDS-1208 Implement the
TemplateOptions.inboundPorts in ProfitBricks REST (#344)
Posted by alibazlamit <no...@github.com>.
@nacx updated with the changes
--
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#issuecomment-274831128
Re: [jclouds/jclouds-labs] JCLOUDS-1208 Implement the
TemplateOptions.inboundPorts in ProfitBricks REST (#344)
Posted by Ignasi Barrera <no...@github.com>.
Closed #344.
--
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#event-935869001
Re: [jclouds/jclouds-labs] JCLOUDS-1208 Implement the
TemplateOptions.inboundPorts in ProfitBricks REST (#344)
Posted by alibazlamit <no...@github.com>.
@alibazlamit pushed 1 commit.
373c6fa Updated reviews
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/344/files/41666c890673fff3826e7de78931a81e071c62e9..373c6fa7b5c1f468432222d8f1dedb47fcca1ee9
Re: [jclouds/jclouds-labs] JCLOUDS-1208 Implement the
TemplateOptions.inboundPorts in ProfitBricks REST (#344)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> + 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"));
+ }
+ }
I'd just override the `createAndRunAServiceInGroup`, call super and add the validations after that. In the end, it is not a test itself, but the method configures a node with ports 22 and 8080 as inbound ports. Adding the validations after that makes sense, and a failure will produce a proper stacktrace and context.
In the end, we want to test as much as possible in the less time, but having some decent feedback. I think this approach meets both objectives.
--
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
Re: [jclouds/jclouds-labs] JCLOUDS-1208 Implement the
TemplateOptions.inboundPorts in ProfitBricks REST (#344)
Posted by alibazlamit <no...@github.com>.
alibazlamit commented on this pull request.
> + 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"));
+ }
+ }
Do you mean to call the super `testCreateAndRunAService` or the `createAndRunAServiceInGroup`, if i override the `createAndRunAServiceInGroup` i still have to create an additional test since its not a test its just a method?
--
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
Re: [jclouds/jclouds-labs] JCLOUDS-1208 Implement the
TemplateOptions.inboundPorts in ProfitBricks REST (#344)
Posted by Ignasi Barrera <no...@github.com>.
Squashed and pushed to master as [b083f4c2](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/b083f4c2). Thanks @alibazlamit!
--
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#issuecomment-275065256