You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Richard Downer <no...@github.com> on 2013/11/07 16:13:04 UTC
[jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort (#33)
The inboundPort settings of the first node in the group dictated the firewall configuration. Subsequent nodes added to the group had their inboundPort settings ignored.
GCE firewalls specify their "target" (VM instances) by means of tags - if a targetTag on a firewall matches the tag on an instance, the firewall's rules are allowed for the instance. This commit applies a tag for each requested inboundPort to new instances. Then, a firewall is created for each tag (if one does not already exist) which has 'allow' rules for the port.
You can merge this Pull Request by running:
git pull https://github.com/richardcloudsoft/jclouds-labs JCLOUDS-367
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs/pull/33
-- Commit Summary --
* JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
-- File Changes --
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineService.java (37)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineServiceAdapter.java (40)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/config/GoogleComputeEngineServiceContextModule.java (3)
A google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/functions/FirewallTagNamingConvention.java (64)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/functions/InstanceInZoneToNodeMetadata.java (15)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/strategy/CreateNodesWithGroupEncodedIntoNameThenAddToSet.java (79)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/domain/Firewall.java (6)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/33.patch
https://github.com/jclouds/jclouds-labs/pull/33.diff
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #84](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/84/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33#issuecomment-28069826
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
Committed to [1.6.x](https://git-wip-us.apache.org/repos/asf?p=jclouds-labs.git;a=shortlog;h=refs/heads/1.6.x)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33#issuecomment-28192330
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> + */
> +package org.jclouds.googlecomputeengine.compute.functions;
> +
> +import com.google.common.base.Predicate;
> +import org.jclouds.compute.functions.GroupNamingConvention;
> +
> +import javax.inject.Inject;
> +
> +/**
> + * The convention for naming instance tags that firewall rules recognise.
> + *
> + * @author richardcloudsoft
> + */
> +public class FirewallTagNamingConvention {
> +
> + public static class Factory {
> is more like the existing org.jclouds.compute.functions.GroupNamingConvention
Thanks for pointing out the precedent! Fine to leave as-is, then...and learnt something new, too!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7534607
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Richard Downer <no...@github.com>.
> @@ -266,6 +266,12 @@ public static IPProtocol fromValue(String protocol) {
> private final IPProtocol ipProtocol;
> private final RangeSet<Integer> ports;
>
> + /* Some handy shortcuts */
> + public static Rule permitTcpRule(Integer start, Integer end) { return Rule.builder().IPProtocol(Firewall.Rule.IPProtocol.TCP).addPortRange(start, end).build(); }
> + public static Rule permitTcpRule(Integer port) { return Rule.builder().IPProtocol(Firewall.Rule.IPProtocol.TCP).addPort(port).build(); }
> + public static Rule permitUdpRule(Integer start, Integer end) { return Rule.builder().IPProtocol(Firewall.Rule.IPProtocol.UDP).addPortRange(start, end).build(); }
> + public static Rule permitUdpRule(Integer port) { return Rule.builder().IPProtocol(Firewall.Rule.IPProtocol.UDP).addPort(port).build(); }
Since they construct new instances, I thought to place them close to the top of the class (where you might see `public static Thing create(...)` factory methods). Any strong feelings about this either way?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7524380
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Richard Downer <no...@github.com>.
> + */
> +package org.jclouds.googlecomputeengine.compute.functions;
> +
> +import com.google.common.base.Predicate;
> +import org.jclouds.compute.functions.GroupNamingConvention;
> +
> +import javax.inject.Inject;
> +
> +/**
> + * The convention for naming instance tags that firewall rules recognise.
> + *
> + * @author richardcloudsoft
> + */
> +public class FirewallTagNamingConvention {
> +
> + public static class Factory {
This class is more like the existing org.jclouds.compute.functions.GroupNamingConvention, which uses the Factory pattern. Also, this class participates in dependency injection, which Builders do not.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7526918
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> @@ -151,14 +161,14 @@ public GoogleComputeEngineServiceAdapter(GoogleComputeEngineApi api,
> retry(new Predicate<AtomicReference<Instance>>() {
> @Override
> public boolean apply(AtomicReference<Instance> input) {
> - input.set(api.getInstanceApiForProject(userProject.get()).getInZone(template.getLocation().getId(),
> + input.set(instanceApi.getInZone(zone,
> name));
> return input.get() != null;
> }
> }, operationCompleteCheckTimeout, operationCompleteCheckInterval, MILLISECONDS).apply(instance);
>
> if (options.getTags().size() > 0) {
`!options.getTags().isEmpty()`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512434
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> + */
> +public class FirewallTagNamingConvention {
> +
> + public static class Factory {
> +
> + private final GroupNamingConvention.Factory namingConvention;
> +
> + @Inject
> + public Factory(GroupNamingConvention.Factory namingConvention) {
> + this.namingConvention = namingConvention;
> + }
> +
> + public FirewallTagNamingConvention get(String groupName) {
> + return new FirewallTagNamingConvention(namingConvention.create().sharedNameForGroup(groupName));
> + }
> + }
jclouds seems to use a Builder pattern preferentially?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512649
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
>
> - retry(operationDonePredicate, operationCompleteCheckTimeout, operationCompleteCheckInterval,
> - MILLISECONDS).apply(operation);
> -
> - checkState(!operation.get().getHttpError().isPresent(), "Could not create firewall, operation failed" + operation);
> + checkState(!operation.get().getHttpError().isPresent(),"Could not create firewall, operation failed" + operation);
Add name of firewall that couldn't be created to error message? And note that now we might exit with _some_ firewalls already created...is that the intention?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512856
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
>
> - retry(operationDonePredicate, operationCompleteCheckTimeout, operationCompleteCheckInterval,
> - MILLISECONDS).apply(operation);
> -
> - checkState(!operation.get().getHttpError().isPresent(), "Could not create firewall, operation failed" + operation);
> + checkState(!operation.get().getHttpError().isPresent(),"Could not create firewall, operation failed" + operation);
> before this change, if a firewall creation failed, there would be an orphaned network instance created.
OK, then let's leave as-is and address the orphaned resource problem separate, as needed. Thanks for explaining!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7534679
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> @@ -266,6 +266,12 @@ public static IPProtocol fromValue(String protocol) {
> private final IPProtocol ipProtocol;
> private final RangeSet<Integer> ports;
>
> + /* Some handy shortcuts */
> + public static Rule permitTcpRule(Integer start, Integer end) { return Rule.builder().IPProtocol(Firewall.Rule.IPProtocol.TCP).addPortRange(start, end).build(); }
> + public static Rule permitTcpRule(Integer port) { return Rule.builder().IPProtocol(Firewall.Rule.IPProtocol.TCP).addPort(port).build(); }
> + public static Rule permitUdpRule(Integer start, Integer end) { return Rule.builder().IPProtocol(Firewall.Rule.IPProtocol.UDP).addPortRange(start, end).build(); }
> + public static Rule permitUdpRule(Integer port) { return Rule.builder().IPProtocol(Firewall.Rule.IPProtocol.UDP).addPort(port).build(); }
Since these are utility methods, move them to the end of the class?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512891
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Richard Downer <no...@github.com>.
> return input.get() != null;
> }
> }, operationCompleteCheckTimeout, operationCompleteCheckInterval, MILLISECONDS).apply(instance);
> }
>
> - InstanceInZone instanceInZone = new InstanceInZone(instance.get(), template.getLocation().getId());
> + // Add tags for security groups
> + final FirewallTagNamingConvention naming = firewallTagNamingConvention.get(group);
> + Set<String> tags = FluentIterable.from(Ints.asList(options.getInboundPorts()))
> + .transform(new Function<Integer, String>(){
> + @Override
> + public String apply(Integer input) {
> + return input != null
The Guava `Function` interface has an `@Nullable` annotation on the parameter to `apply()`. As a habit, whenever writing a `Function` implementation, I always check that the input is non-null, even if it's unlikely to be the case. Some smart IDEs see the nullable annotation and show warnings if it is absent. That's my convention however, and I can remove this if you'd prefer.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7524336
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> @@ -116,6 +123,8 @@ public GoogleComputeEngineServiceAdapter(GoogleComputeEngineApi api,
>
> checkNotNull(template, "template");
>
> + final InstanceApi instanceApi = api.getInstanceApiForProject(userProject.get());
> +
Get `instanceApi` down where it's used?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512390
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Richard Downer <no...@github.com>.
Thanks @demobox, I think I've gone through all of your review comments, with the exception of the failure-cleanup question. Any more comments at this stage?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33#issuecomment-28069473
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Richard Downer <no...@github.com>.
>
> - retry(operationDonePredicate, operationCompleteCheckTimeout, operationCompleteCheckInterval,
> - MILLISECONDS).apply(operation);
> -
> - checkState(!operation.get().getHttpError().isPresent(), "Could not create firewall, operation failed" + operation);
> + checkState(!operation.get().getHttpError().isPresent(),"Could not create firewall, operation failed" + operation);
The `operation` object is included in the message, so there will be information about the firewall ID. Good question about the cleanup though. Even before this change, if a firewall creation failed, there would be an orphaned network instance created. `GoogleComputeEngineService.cleanUpIncidentalResourcesOfDeadNodes()` would tidy this up but I'm uncertain if jclouds would call it this chain of events?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7528469
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> I can't reproduce this new test failure
Let's see what happens in the next DEV@cloud run...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33#issuecomment-28013287
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
Closed #33.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
>
> - if (operation.get().getHttpError().isPresent()) {
> - HttpResponse response = operation.get().getHttpError().get();
> - logger.warn("delete orphaned firewall failed. Http Error Code: " + response.getStatusCode() +
> - " HttpError: " + response.getMessage());
> + if (operation.get().getHttpError().isPresent()) {
> + HttpResponse response = operation.get().getHttpError().get();
> + logger.warn("delete orphaned firewall failed. Http Error Code: " + response.getStatusCode() +
And name and other identifying information here, so the user knows _which_ firewall couldn't be deleted?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512325
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> @@ -108,6 +114,7 @@ public GoogleComputeEngineServiceAdapter(GoogleComputeEngineApi api,
> operationCompleteCheckInterval, TimeUnit.MILLISECONDS);
> this.zones = checkNotNull(zones, "zones");
> this.hardwareMap = checkNotNull(hardwareMap, "hardwareMap");
> + this.firewallTagNamingConvention = firewallTagNamingConvention;
Add null check?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512363
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #82](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/82/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33#issuecomment-27974025
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> return input.get() != null;
> }
> }, operationCompleteCheckTimeout, operationCompleteCheckInterval, MILLISECONDS).apply(instance);
> }
>
> - InstanceInZone instanceInZone = new InstanceInZone(instance.get(), template.getLocation().getId());
> + // Add tags for security groups
> + final FirewallTagNamingConvention naming = firewallTagNamingConvention.get(group);
> + Set<String> tags = FluentIterable.from(Ints.asList(options.getInboundPorts()))
> + .transform(new Function<Integer, String>(){
> + @Override
> + public String apply(Integer input) {
> + return input != null
Can `options.getInboundPorts()` contain `null` entries?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512517
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
+1 - looks good to me! Thanks for the changes, @richardcloudsoft. Squash-n-rebase, please?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33#issuecomment-28086420
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> @@ -95,7 +100,8 @@ public GoogleComputeEngineServiceAdapter(GoogleComputeEngineApi api,
> @Named(OPERATION_COMPLETE_INTERVAL) Long operationCompleteCheckInterval,
> @Named(OPERATION_COMPLETE_TIMEOUT) Long operationCompleteCheckTimeout,
> @Memoized Supplier<Map<URI, ? extends Location>> zones,
> - @Memoized Supplier<Map<URI, ? extends Hardware>> hardwareMap) {
> + @Memoized Supplier<Map<URI, ? extends Hardware>> hardwareMap,
> + FirewallTagNamingConvention.Factory firewallTagNamingConvention) {
`@Memoized` or not?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512375
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Richard Downer <no...@github.com>.
Squash-n-rebase done, @demobox. Thanks for reviewing :smiley:
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33#issuecomment-28186914
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> return input.get() != null;
> }
> }, operationCompleteCheckTimeout, operationCompleteCheckInterval, MILLISECONDS).apply(instance);
> }
>
> - InstanceInZone instanceInZone = new InstanceInZone(instance.get(), template.getLocation().getId());
> + // Add tags for security groups
> + final FirewallTagNamingConvention naming = firewallTagNamingConvention.get(group);
> + Set<String> tags = FluentIterable.from(Ints.asList(options.getInboundPorts()))
> + .transform(new Function<Integer, String>(){
> + @Override
> + public String apply(Integer input) {
> + return input != null
Makes sense - fine with keeping it here. Thanks for explaining!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7534579
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds-labs #568](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/568/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33#issuecomment-27974822
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> @@ -166,14 +176,28 @@ public boolean apply(AtomicReference<Instance> input) {
> retry(new Predicate<AtomicReference<Instance>>() {
> @Override
> public boolean apply(AtomicReference<Instance> input) {
> - input.set(api.getInstanceApiForProject(userProject.get()).getInZone(template.getLocation().getId(),
> - name));
> + input.set(instanceApi.getInZone(zone,
> + name));
Indent wrong, and spurious wrapping?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512447
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> for (Integer port : templateOptions.getInboundPorts()) {
> - tcpRule.addPort(port);
> - udpRule.addPort(port);
> + String name = naming.name(port);
> + ImmutableSet<Firewall.Rule> rules = ImmutableSet.of(Firewall.Rule.permitTcpRule(port), Firewall.Rule.permitUdpRule(port));
> + FirewallOptions firewallOptions = new FirewallOptions()
Wait until inside the `if (firewall == null)` bit before building the options?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512823
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> @@ -266,6 +266,12 @@ public static IPProtocol fromValue(String protocol) {
> private final IPProtocol ipProtocol;
> private final RangeSet<Integer> ports;
>
> + /* Some handy shortcuts */
> + public static Rule permitTcpRule(Integer start, Integer end) { return Rule.builder().IPProtocol(Firewall.Rule.IPProtocol.TCP).addPortRange(start, end).build(); }
> + public static Rule permitTcpRule(Integer port) { return Rule.builder().IPProtocol(Firewall.Rule.IPProtocol.TCP).addPort(port).build(); }
> + public static Rule permitUdpRule(Integer start, Integer end) { return Rule.builder().IPProtocol(Firewall.Rule.IPProtocol.UDP).addPortRange(start, end).build(); }
> + public static Rule permitUdpRule(Integer port) { return Rule.builder().IPProtocol(Firewall.Rule.IPProtocol.UDP).addPort(port).build(); }
> Any strong feelings about this either way?
Not really - I normally don't put methods between fields and the constructor, but I'm OK to keep as-is.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7534701
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> for (Integer port : templateOptions.getInboundPorts()) {
> - tcpRule.addPort(port);
> - udpRule.addPort(port);
> + String name = naming.name(port);
> + ImmutableSet<Firewall.Rule> rules = ImmutableSet.of(Firewall.Rule.permitTcpRule(port), Firewall.Rule.permitUdpRule(port));
> + FirewallOptions firewallOptions = new FirewallOptions()
> + .name(name)
> + .network(network.getSelfLink())
> + .allowedRules(rules)
> + .sourceTags(templateOptions.getTags())
> + .sourceRanges(of(DEFAULT_INTERNAL_NETWORK_RANGE, EXTERIOR_RANGE))
> + .targetTags(ImmutableSet.of(name));
> +
> + Firewall firewall = api.getFirewallApiForProject(projectName).get(firewallOptions.getName());
Extract `api.getFirewallApiForProject(projectName)` as variable?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512769
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> this.toPortableNodeStatus = toPortableNodeStatus;
> this.nodeNamingConvention = namingConvention.createWithoutPrefix();
> this.images = images;
> this.hardwares = hardwares;
> this.locations = locations;
> + this.firewallTagNamingConvention = firewallTagNamingConvention;
null check?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512699
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #85](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/85/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33#issuecomment-28187335
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> @@ -119,6 +121,7 @@ protected void configure() {
>
> install(new LocationsFromComputeServiceAdapterModule<InstanceInZone, MachineTypeInZone, Image, Zone>() {});
>
> + this.bind(FirewallTagNamingConvention.Factory.class).in(Scopes.SINGLETON);
Do we need `this` here? We're not using it above..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512579
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #83](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/83/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33#issuecomment-27989047
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> + */
> +package org.jclouds.googlecomputeengine.compute.functions;
> +
> +import com.google.common.base.Predicate;
> +import org.jclouds.compute.functions.GroupNamingConvention;
> +
> +import javax.inject.Inject;
> +
> +/**
> + * The convention for naming instance tags that firewall rules recognise.
> + *
> + * @author richardcloudsoft
> + */
> +public class FirewallTagNamingConvention {
> +
> + public static class Factory {
jclouds seems to use a Builder pattern preferentially?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512661
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Andrew Phillips <no...@github.com>.
> @@ -93,6 +97,7 @@ protected CreateNodesWithGroupEncodedIntoNameThenAddToSet(
> this.operationCompleteCheckTimeout = checkNotNull(operationCompleteCheckTimeout,
> "operation completed check timeout");
> this.operationDonePredicate = operationDonePredicate;
> + this.firewallTagNamingConvention = firewallTagNamingConvention;
null check?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33/files#r7512714
Re: [jclouds-labs] JCLOUDS-367: GCE nodes n>1 ignoring inboundPort
(#33)
Posted by Richard Downer <no...@github.com>.
I can't reproduce this new test failure (GoogleComputeEngineImageToImageTest.testWellFormedImageName) and its unrelated to my recent changes... a CloudBees glitch?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/33#issuecomment-27989660