Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean up duplicated code in JcloudsLocationCustomizers #319

Merged
merged 2 commits into from
Oct 31, 2016
Merged

Clean up duplicated code in JcloudsLocationCustomizers #319

merged 2 commits into from
Oct 31, 2016

Conversation

bostko
Copy link
Contributor

@bostko bostko commented Sep 5, 2016 •
edited
Loading

Unite the code from
#276
and #292
OPEN_INBOUND_PORTS_IN_SECURITY_GROUP_EFFECTOR tested after brooklyn restarts. All works fine.

duncangrant
duncangrant reviewed Sep 5, 2016
View reviewed changes
...louds/src/main/java/org/apache/brooklyn/location/jclouds/networking/NetworkingEffectors.java
}
SharedLocationSecurityGroupCustomizer locationSecurityGroupCustomizer = new SharedLocationSecurityGroupCustomizer();
locationSecurityGroupCustomizer.setIpProtocol(ipProtocol.name());
locationSecurityGroupCustomizer.setPortRanges(rawPortRules);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what is going on here. You create a SharedLocationSecurityGroupCustomizer, configure it, and then don't use it? You then create another one below here that you don't configure but you use to customize the location.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't push latest changes.

@duncangrant
Copy link
Contributor

Why are you using the SharedLocationSecurityGroupCustomizer in NetworkingEffectors? The SharedLocationSecurityGroupCustomizer is intended as a cut-down JcloudsLocationSecurityGroupCustomizer that works in yaml. As you are using java could you use JcloudsLocationSecurityGroupCustomizer and then you could avoid modifying SharedLocationSecurityGroupCustomizer?

@bostko bostko force-pushed the cleanup-security-group-customizers branch from 96b16fc to 6a839f4 Compare September 5, 2016 13:11
duncangrant
duncangrant reviewed Sep 5, 2016
View reviewed changes
...a/org/apache/brooklyn/location/jclouds/networking/InboundPortsJcloudsLocationCustomizer.java
@@ -36,6 +36,7 @@
import static org.apache.brooklyn.location.jclouds.networking.NetworkingEffectors.*;

@Beta
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class has only existed a couple of weeks? Can we just deleted it?

@bostko bostko force-pushed the cleanup-security-group-customizers branch 2 times, most recently from 00fdaa2 to 4887673 Compare September 5, 2016 14:57
duncangrant
duncangrant reviewed Sep 5, 2016
View reviewed changes
...a/org/apache/brooklyn/location/jclouds/networking/SharedLocationSecurityGroupCustomizer.java
@@ -74,6 +74,9 @@

private RangeSet<Integer> tcpPortRanges;
private RangeSet<Integer> udpPortRanges;
private Boolean openIcmp;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks useful - AWS let's you specify a protocol - does it just do "ALL"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this fails on Azure - because Azure SGs don't support ICMP - probably should add a comment documenting what will happen in this case

@bostko bostko force-pushed the cleanup-security-group-customizers branch 3 times, most recently from 536d966 to 05b650e Compare September 5, 2016 21:25
@bostko
Copy link
Contributor Author

bostko commented Sep 6, 2016

retest this please

@duncangrant
Copy link
Contributor

@bostko Can you extend the tests in SharedLocationSecurityGroupCustomizerTest to cover the new functionality - otherwise looks good

@bostko bostko force-pushed the cleanup-security-group-customizers branch from 05b650e to b427cf1 Compare September 8, 2016 18:22
@bostko
Copy link
Contributor Author

bostko commented Sep 8, 2016 •
edited
Loading

@duncangrant @aledsage I added a unit test for opening ICMP.

@bostko bostko force-pushed the cleanup-security-group-customizers branch from b427cf1 to dcff2c3 Compare October 3, 2016 11:09
@bostko bostko force-pushed the cleanup-security-group-customizers branch 4 times, most recently from dd48dca to db5b7cd Compare October 18, 2016 13:56
sjcorbett
sjcorbett reviewed Oct 18, 2016
View reviewed changes
utils/common/src/main/java/org/apache/brooklyn/util/net/Networking.java
throw new IllegalArgumentException("portRule shouldn't be empty");
}
if (portRule.equals("-1")) {
result.add(Range.closed(Integer.parseInt("-1"), Integer.parseInt("-1")));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? Strikes me as surprising and I can't see where it's required in these changes.

sjcorbett
sjcorbett reviewed Oct 18, 2016
View reviewed changes
...a/org/apache/brooklyn/location/jclouds/networking/SharedLocationSecurityGroupCustomizer.java
*/
private Boolean openIcmp;

private Collection<SecurityGroup> updatedSecurityGroups;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this and getUpdatedSecurityGroups and add a new method to the class (not sure of the best name for it, maybe simply doCustomize?) that performs the customisation and returns the new security groups. customize(JcloudsLocation, ComputeService, JcloudsMachineLocation) should call this new method and discard the result.

@bostko bostko force-pushed the cleanup-security-group-customizers branch 2 times, most recently from b60db38 to 5369ee0 Compare October 18, 2016 17:41
@sjcorbett
Copy link
Contributor

LGTM. Will wait for Jenkins to complete before merging.

@sjcorbett
Copy link
Contributor

@bostko there are four test failures:

org.apache.brooklyn.location.jclouds.networking.SharedLocationSecurityGroupCustomizerTest.testInboundIcmpAddedToPermissions
org.apache.brooklyn.location.jclouds.networking.SharedLocationSecurityGroupCustomizerTest.testInboundPortsAddedToPermissions
org.apache.brooklyn.location.jclouds.networking.SharedLocationSecurityGroupCustomizerTest.testPermissionsSetFromPortRanges
org.apache.brooklyn.location.jclouds.networking.SharedLocationSecurityGroupCustomizerTest.testUdpPermissionsSetFromPortRanges

The exception is the same in each one:

java.lang.NullPointerException: null
    at org.apache.brooklyn.location.jclouds.networking.SharedLocationSecurityGroupCustomizer.applySecurityGroupCustomizations(SharedLocationSecurityGroupCustomizer.java:192)
    at org.apache.brooklyn.location.jclouds.networking.SharedLocationSecurityGroupCustomizer.customize(SharedLocationSecurityGroupCustomizer.java:152)
    at org.apache.brooklyn.location.jclouds.networking.SharedLocationSecurityGroupCustomizerTest.testUdpPermissionsSetFromPortRanges(SharedLocationSecurityGroupCustomizerTest.java:109)

@bostko bostko force-pushed the cleanup-security-group-customizers branch 2 times, most recently from 58584e9 to 85a4db5 Compare October 19, 2016 14:16
@bostko bostko force-pushed the cleanup-security-group-customizers branch from 85a4db5 to 330965f Compare October 20, 2016 10:31
@neykov
Copy link
Member

neykov commented Oct 31, 2016

Tests are passing now, thanks @duncangrant , @sjcorbett, @bostko. Merging.

@asfgit asfgit merged commit 330965f into apache:master Oct 31, 2016
asfgit pushed a commit that referenced this pull request Oct 31, 2016
@neykov
Closes #319
7cf27bd 
Clean up duplicated code in JcloudsLocationCustomizers

Unite the code from
#276
and #292
OPEN_INBOUND_PORTS_IN_SECURITY_GROUP_EFFECTOR tested after brooklyn restarts. All works fine.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjcorbett sjcorbett sjcorbett left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bostko @duncangrant @sjcorbett @neykov @asfgit