You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by andreaturli <gi...@git.apache.org> on 2015/06/16 22:34:45 UTC
[GitHub] incubator-brooklyn pull request: [BROOKLYN-99] add support for Nov...
GitHub user andreaturli opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/694
[BROOKLYN-99] add support for NovaSecurityGroupExtension
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/andreaturli/incubator-brooklyn fix/BROOKLYN-99
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/694.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #694
----
commit a0abd1f20cb10aa6d98e86d60b5f65bd89595c35
Author: Andrea Turli <an...@gmail.com>
Date: 2015-06-16T20:33:41Z
add support for NovaSecurityGroupExtension
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-99] add support for Nov...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/694#discussion_r33178718
--- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java ---
@@ -225,41 +238,47 @@ void addPermissionsToLocation(Iterable<IpPermission> permissions, final String n
* {@link #sharedGroupCache} if no mapping for the shared group's location previously
* existed (e.g. Brooklyn was restarted and rebound to an existing application).
*
+ * Notice that jclouds will attach 2 securityGroups to the node if the locationId is `aws-ec2` so it needs to
+ * look for the uniqueSecurityGroup rather than the shared securityGroup.
+ *
* @param nodeId The id of the node in question
+ * @param locationId The id of the location in question
* @param securityApi The API to use to list security groups
* @return the security group unique to the given node, or null if one could not be determined.
*/
- private SecurityGroup getUniqueSecurityGroupForNodeCachingSharedGroupIfPreviouslyUnknown(
- String nodeId, SecurityGroupExtension securityApi) {
+ private SecurityGroup getUniqueSecurityGroupForNodeCachingSharedGroupIfPreviouslyUnknown(String nodeId, String locationId, SecurityGroupExtension securityApi) {
Set<SecurityGroup> groupsOnNode = securityApi.listSecurityGroupsForNode(nodeId);
- if (groupsOnNode.size() != 2) {
- LOG.warn("Expected to find two security groups on node {} in app {} (one shared, one unique). Found {}: {}",
- new Object[]{nodeId, applicationId, groupsOnNode.size(), groupsOnNode});
- return null;
- }
- String expectedSharedName = getNameForSharedSecurityGroup();
- Iterator<SecurityGroup> it = groupsOnNode.iterator();
- SecurityGroup shared = it.next();
SecurityGroup unique;
- if (shared.getName().endsWith(expectedSharedName)) {
- unique = it.next();
- } else {
- unique = shared;
- shared = it.next();
- }
- if (!shared.getName().endsWith(expectedSharedName)) {
- LOG.warn("Couldn't determine which security group is shared between instances in app {}. Expected={}, found={}",
- new Object[]{applicationId, expectedSharedName, groupsOnNode});
- return null;
- }
- // Shared entry might be missing if Brooklyn has rebound to an application
- SecurityGroup old = sharedGroupCache.asMap().putIfAbsent(shared.getLocation(), shared);
- LOG.info("Loaded unique security group for node {} (in {}): {}",
- new Object[]{nodeId, applicationId, unique});
- if (old == null) {
- LOG.info("Proactively set shared group for app {} to: {}", applicationId, shared);
+ if (locationId.equals("aws-ec2")) {
--- End diff --
It may be nice to extract this AWS-specific functionality into a new method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-99] add support for Nov...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/694#discussion_r33181246
--- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java ---
@@ -225,41 +238,47 @@ void addPermissionsToLocation(Iterable<IpPermission> permissions, final String n
* {@link #sharedGroupCache} if no mapping for the shared group's location previously
* existed (e.g. Brooklyn was restarted and rebound to an existing application).
*
+ * Notice that jclouds will attach 2 securityGroups to the node if the locationId is `aws-ec2` so it needs to
+ * look for the uniqueSecurityGroup rather than the shared securityGroup.
+ *
* @param nodeId The id of the node in question
+ * @param locationId The id of the location in question
* @param securityApi The API to use to list security groups
* @return the security group unique to the given node, or null if one could not be determined.
*/
- private SecurityGroup getUniqueSecurityGroupForNodeCachingSharedGroupIfPreviouslyUnknown(
- String nodeId, SecurityGroupExtension securityApi) {
+ private SecurityGroup getUniqueSecurityGroupForNodeCachingSharedGroupIfPreviouslyUnknown(String nodeId, String locationId, SecurityGroupExtension securityApi) {
Set<SecurityGroup> groupsOnNode = securityApi.listSecurityGroupsForNode(nodeId);
- if (groupsOnNode.size() != 2) {
- LOG.warn("Expected to find two security groups on node {} in app {} (one shared, one unique). Found {}: {}",
- new Object[]{nodeId, applicationId, groupsOnNode.size(), groupsOnNode});
- return null;
- }
- String expectedSharedName = getNameForSharedSecurityGroup();
- Iterator<SecurityGroup> it = groupsOnNode.iterator();
- SecurityGroup shared = it.next();
SecurityGroup unique;
- if (shared.getName().endsWith(expectedSharedName)) {
- unique = it.next();
- } else {
- unique = shared;
- shared = it.next();
- }
- if (!shared.getName().endsWith(expectedSharedName)) {
- LOG.warn("Couldn't determine which security group is shared between instances in app {}. Expected={}, found={}",
- new Object[]{applicationId, expectedSharedName, groupsOnNode});
- return null;
- }
- // Shared entry might be missing if Brooklyn has rebound to an application
- SecurityGroup old = sharedGroupCache.asMap().putIfAbsent(shared.getLocation(), shared);
- LOG.info("Loaded unique security group for node {} (in {}): {}",
- new Object[]{nodeId, applicationId, unique});
- if (old == null) {
- LOG.info("Proactively set shared group for app {} to: {}", applicationId, shared);
+ if (locationId.equals("aws-ec2")) {
+ if (groupsOnNode.size() != 2) {
+ LOG.warn("Expected to find two security groups on node {} in app {} (one shared, one unique). Found {}: {}",
+ new Object[]{nodeId, applicationId, groupsOnNode.size(), groupsOnNode});
+ return null;
+ }
+ String expectedSharedName = getNameForSharedSecurityGroup();
+ Iterator<SecurityGroup> it = groupsOnNode.iterator();
+ SecurityGroup shared = it.next();
+ if (shared.getName().endsWith(expectedSharedName)) {
--- End diff --
This could be simplified a bit using `Iterables.tryFind(Iterable<T> iterable, Predicate<? super T> predicate)`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-99] add support for Nov...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/694#issuecomment-113500700
Thanks @andreaturli !
Could you add a few additional lines of javadoc to `JcloudsLocationSecurityGroupCustomizer.java` that describes what this class does, where it can be used (e.g. for any jclouds that has the security group extension, or is it more limited - I see explicit mention of nova)?
It would also be useful to have an example of how it should be used, e.g. for someone who wants to use security groups in OpenStack.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-99] add support for Nov...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/694
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-99] add support for Nov...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/694#issuecomment-114982160
Official PR image: http://www.eggplante.com/wp-content/uploads/2015/01/Brooklyn-Nine-Nine-Logo1.jpg
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-99] add support for Nov...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/694#issuecomment-114978537
Minor suggestions; LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-99] add support for Nov...
Posted by andreaturli <gi...@git.apache.org>.
Github user andreaturli commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/694#issuecomment-114989275
thanks @mikezaccardo!
I'll try to address your suggestions and re-test asap, unless we want to merge it as-is and address it in a subsequent PR. wdyt @richardcloudsoft ?
Really like the official PR image ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: [BROOKLYN-99] add support for Nov...
Posted by andreaturli <gi...@git.apache.org>.
Github user andreaturli commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/694#issuecomment-114473060
thanks @aledsage!
I've added some javadoc, trying to explain how to use it. HTH
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---