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

Fixes incorrect 'PublicIp a.b.c.d is not valid' in log when multiple gateways are present #59

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixes incorrect 'PublicIp a.b.c.d is not valid' in log when multiple gateways are present #59

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 11 additions & 30 deletions vcloud-director/src/main/java/brooklyn/networking/vclouddirector/NatService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,37 +549,17 @@ private boolean includesPublicIp(final String publicIp, List<SubnetParticipation
return false;
}

private void checkPublicIps(Iterable<String> publicIps, List<SubnetParticipationType> subnetParticipations) {
private boolean checkPublicIps(Iterable<String> publicIps, List<SubnetParticipationType> subnetParticipations) {
Copy link
Member

Choose a reason for hiding this comment

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

The naming convention of checkPublicIps follows the guava contention. The semantics elsewhere are that it will throw an exception if the check fails, rather than returning a boolean.

for (String publicIp : publicIps) {
checkPublicIp(publicIp, subnetParticipations);
}
}

private void checkPublicIp(final String publicIp, List<SubnetParticipationType> subnetParticipations) {
boolean found = includesPublicIp(publicIp, subnetParticipations);
if (!found) {
StringBuilder builder = new StringBuilder();
builder.append("PublicIp '" + publicIp + "' is not valid. Public IP must fall in the following ranges: ");
if (subnetParticipations == null) {
for (SubnetParticipationType subnetParticipation : subnetParticipations) {
IpRangesType range = subnetParticipation.getIpRanges();
if (range != null && range.getIpRange() != null) {
for (IpRangeType ipRangeType : range.getIpRange()) {
builder.append(ipRangeType.getStartAddress());
builder.append(" - ");
builder.append(ipRangeType.getEndAddress());
builder.append(", ");
}
} else {
builder.append("<no ip range>, ");
}
}
} else {
builder.append("<no subnet participants>");
if (!checkPublicIp(publicIp, subnetParticipations)) {
return false;
}
LOG.error(builder.toString()+" (rethrowing)");
throw new IllegalArgumentException(builder.toString());
}
return true;
}

private boolean checkPublicIp(final String publicIp, List<SubnetParticipationType> subnetParticipations) {
return includesPublicIp(publicIp, subnetParticipations);
Copy link
Member

Choose a reason for hiding this comment

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

Is the problem not just that the log message is misleading, e.g. change this line to:

builder.append("PublicIp '" + publicIp + "' is not valid. Public IP must fall in one of the following ranges: ");

}

private static long ipToLong(InetAddress ip) {
Expand Down Expand Up @@ -615,8 +595,9 @@ protected EdgeGateway getEdgeGatewayForPublicIp(Iterable<String> publicIps) thro
subnetParticipations.addAll(gatewayInterfaceType.getSubnetParticipation());
}
try {
checkPublicIps(publicIps, subnetParticipations);
return edgeGateway;
if (checkPublicIps(publicIps, subnetParticipations)) {
return edgeGateway;
}
} catch (IllegalArgumentException e) {
errs.put(edgeGateway, e.getMessage());
}
Expand Down