nacx requested changes on this pull request.
Thanks @andreaturli! Looks like a great starting point.
Apart from the comments I've made I found it difficult to understand the
duplicated "Create/FindSecurityGroup" classes. For a proper review, could you
please:
* Explain what is each new class, if it is a copy of an existing one and its
purpose and need?
* Explain the main design idea so we have a clear picture of how both
implementations Nova/Neutron will behave together.
* Explain a bit the different execution flows and where the decisions of using
Nova or Neutron are made, so we can properly understand the context around all
these classes and when they are going to take place.
> + bind(new TypeLiteral<SecurityGroupExtension>() {
+ }).toProvider(SecurityGroupExtensionProvider.class);
+
+ }
+
+ @Singleton
+ public static class SecurityGroupExtensionProvider implements
Provider<SecurityGroupExtension> {
+ @Inject(optional = true)
+ @Named("openstack-neutron")
+ protected Supplier<Context> neutronApiContextSupplier;
+
+ private final NeutronSecurityGroupExtension
neutronSecurityGroupExtension;
+ private final NovaSecurityGroupExtension novaSecurityGroupExtension;
+
+ @Inject
+ public SecurityGroupExtensionProvider(NeutronSecurityGroupExtension
neutronSecurityGroupExtension,
Remove the public modifier
> + */
+public class NeutronSecurityGroupExtension implements SecurityGroupExtension {
+
+ @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.NULL;
+
+ private final NovaApi api;
+ private final Supplier<Set<String>> regionIds;
+ private final GroupNamingConvention.Factory namingConvention;
+ private final LoadingCache<RegionAndName, SecurityGroup> groupCreator;
+ private final Supplier<Map<String, Location>> locationIndex;
+
+ @Inject(optional = true)
+ @Named("openstack-neutron")
+ Supplier<Context> neutronContextSupplier;
Make it `private`
> + @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.NULL;
+
+ private final NovaApi api;
+ private final Supplier<Set<String>> regionIds;
+ private final GroupNamingConvention.Factory namingConvention;
+ private final LoadingCache<RegionAndName, SecurityGroup> groupCreator;
+ private final Supplier<Map<String, Location>> locationIndex;
+
+ @Inject(optional = true)
+ @Named("openstack-neutron")
+ Supplier<Context> neutronContextSupplier;
+
+ @Inject
+ public NeutronSecurityGroupExtension(NovaApi api,
Remove the public modifier
> +
+ @Inject(optional = true)
+ @Named("openstack-neutron")
+ Supplier<Context> neutronContextSupplier;
+
+ @Inject
+ public NeutronSecurityGroupExtension(NovaApi api,
+ @Region Supplier<Set<String>>
regionIds,
+ GroupNamingConvention.Factory
namingConvention,
+ LoadingCache<RegionAndName,
SecurityGroup> groupCreator,
+ Supplier<Map<String, Location>>
locationIndex) {
+ this.api = api;
+ this.regionIds = checkNotNull(regionIds, "regionIds");
+ this.namingConvention = checkNotNull(namingConvention,
"namingConvention");
+ this.groupCreator = groupCreator;
+ this.locationIndex = checkNotNull(locationIndex, "locationIndex");
Remove the null checks. They're redundant.
> + Location location = locationIndex.get().get(region);
+ return ImmutableSet.copyOf(transform(filter(allGroups, notNull()),
toSecurityGroup(location)));
+ }
+
+ @Override
+ public SecurityGroup getSecurityGroupById(String id) {
+ RegionAndId regionAndId = RegionAndId.fromSlashEncoded(checkNotNull(id,
"id"));
+ String region = regionAndId.getRegion();
+ String groupId = regionAndId.getId();
+
+ SecurityGroupApi securityGroupApi = getSecurityGroupApi(region);
+
+// final
FluentIterable<org.jclouds.openstack.nova.v2_0.domain.SecurityGroup> allGroups
= securityGroupApi.listSecurityGroups();
+// SecurityGroupInRegion rawGroup = new
SecurityGroupInRegion(securityGroupApi.get(groupId), region, allGroups);
+
+// return groupConverter.apply(rawGroup);
Remove the commented code.
> + @Override
+ public boolean supportsGroupIds() {
+ return true;
+ }
+
+ @Override
+ public boolean supportsPortRangesForGroups() {
+ return false;
+ }
+
+ @Override
+ public boolean supportsExclusionCidrBlocks() {
+ return false;
+ }
+
+ private Function<org.jclouds.openstack.neutron.v2.domain.SecurityGroup,
SecurityGroup> toSecurityGroup(final Location location) {
Move this to its own type.
The private method is a bit awkward. Create a type for this function and use
google assisted inject and a factory interface to populate the Location field
where needed.
> import org.jclouds.logging.Logger;
import org.jclouds.openstack.nova.v2_0.NovaApi;
-import org.jclouds.openstack.nova.v2_0.domain.SecurityGroup;
+import
org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtension;
Unused?
> }
}
- private void authorizeGroupToItselfAndAllIPsToTCPPort(SecurityGroupApi
securityGroupApi,
- SecurityGroup
securityGroup, int port) {
+ private SecurityGroupApi getNeutronSecurityGroupApi(String region) {
+ if (neutronContextSupplier == null)
+ return null;
+ return ((ApiContext<NeutronApi>)
neutronContextSupplier.get()).getApi().getSecurityGroupApi(region);
+ }
+
+ // TODO remove duplications
You should be able to remove this when you have the function in a new type
using assisted inject.
> }
}
+ @Override
+ public void testAScriptExecutionAfterBootWithBasicTemplate() throws
Exception {
+ super.testAScriptExecutionAfterBootWithBasicTemplate();
+ }
Remove this.
> + @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.CONSOLE;
+
+ private Context neutronApiContext;
+
+ public NeutronSecurityGroupExtensionLiveTest() {
+ provider = "openstack-nova";
+
+ Properties overrides = setupProperties();
+ neutronApiContext = ContextBuilder.newBuilder("openstack-neutron")
+ .endpoint(setIfTestSystemPropertyPresent(overrides,
+ "openstack-neutron.endpoint"))
+ .credentials(setIfTestSystemPropertyPresent(overrides,
+ "openstack-neutron.identity"),
+ setIfTestSystemPropertyPresent(overrides,
"openstack-neutron.credential"))
We should be able to use the same Nova properties. It is unlikely to use a
neutron that is behind a different keystone instance. Let's use the same
endpoint and credentials that are configured for Nova.
> + private Context neutronApiContext;
+
+ public NeutronSecurityGroupExtensionLiveTest() {
+ provider = "openstack-nova";
+
+ Properties overrides = setupProperties();
+ neutronApiContext = ContextBuilder.newBuilder("openstack-neutron")
+ .endpoint(setIfTestSystemPropertyPresent(overrides,
+ "openstack-neutron.endpoint"))
+ .credentials(setIfTestSystemPropertyPresent(overrides,
+ "openstack-neutron.identity"),
+ setIfTestSystemPropertyPresent(overrides,
"openstack-neutron.credential"))
+ .modules(ImmutableSet.<Module>of(
+ new SshjSshClientModule(),
+ new SLF4JLoggingModule(),
+ new BouncyCastleCryptoModule())
Do we really need this module?
--
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/pull/1175#pullrequestreview-90106960
