Fix: resolve a set of NPEs due to security group activation
Change-Id: Idfd93c80f710e85370f9054d93b9b7e96c77fd49
diff --git a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingFloatingIpHandler.java b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingFloatingIpHandler.java
index 21324bf..f0c0550 100644
--- a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingFloatingIpHandler.java
+++ b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingFloatingIpHandler.java
@@ -860,6 +860,11 @@
case OPENSTACK_PORT_PRE_REMOVE:
InstancePort instPort =
instancePortService.instancePort(event.port().getId());
+
+ if (instPort == null) {
+ break;
+ }
+
NetFloatingIP fip =
associatedFloatingIp(instPort, osRouterAdminService.floatingIps());
diff --git a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingIcmpHandler.java b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingIcmpHandler.java
index 17d9922..d1ab9d7 100644
--- a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingIcmpHandler.java
+++ b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingIcmpHandler.java
@@ -53,7 +53,6 @@
import org.onosproject.store.service.Serializer;
import org.onosproject.store.service.StorageService;
import org.openstack4j.model.network.ExternalGateway;
-import org.openstack4j.model.network.IP;
import org.openstack4j.model.network.Port;
import org.openstack4j.model.network.Router;
import org.openstack4j.model.network.RouterInterface;
@@ -272,12 +271,8 @@
checkNotNull(srcIp);
Port osPort = osNetworkService.port(instance.portId());
- IP fixedIp = osPort.getFixedIps().stream()
- .filter(ip -> IpAddress.valueOf(ip.getIpAddress()).equals(srcIp))
- .findAny().orElse(null);
- checkNotNull(fixedIp);
-
- return osNetworkService.subnet(fixedIp.getSubnetId());
+ return osNetworkService.subnets(osPort.getNetworkId())
+ .stream().findAny().orElse(null);
}
private boolean isForSubnetGateway(IpAddress dstIp, Subnet srcSubnet) {
diff --git a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupHandler.java b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupHandler.java
index d0241d6..01711e9 100644
--- a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupHandler.java
+++ b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupHandler.java
@@ -89,6 +89,9 @@
import static org.onosproject.openstacknetworking.api.Constants.PRIORITY_CT_DROP_RULE;
import static org.onosproject.openstacknetworking.api.Constants.PRIORITY_CT_HOOK_RULE;
import static org.onosproject.openstacknetworking.api.Constants.PRIORITY_CT_RULE;
+import static org.onosproject.openstacknetworking.util.RulePopulatorUtil.computeCtMaskFlag;
+import static org.onosproject.openstacknetworking.util.RulePopulatorUtil.computeCtStateFlag;
+import static org.onosproject.openstacknetworking.util.RulePopulatorUtil.niciraConnTrackTreatmentBuilder;
import static org.onosproject.openstacknode.api.OpenstackNode.NodeType.COMPUTE;
import static org.slf4j.LoggerFactory.getLogger;
@@ -139,9 +142,14 @@
@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
protected ClusterService clusterService;
- private final InstancePortListener instancePortListener = new InternalInstancePortListener();
- private final OpenstackNetworkListener portListener = new InternalOpenstackPortListener();
- private final OpenstackSecurityGroupListener securityGroupListener = new InternalSecurityGroupListener();
+ private final InstancePortListener instancePortListener =
+ new InternalInstancePortListener();
+ private final OpenstackNetworkListener osNetworkListener =
+ new InternalOpenstackNetworkListener();
+ private final OpenstackNetworkListener osPortListener =
+ new InternalOpenstackPortListener();
+ private final OpenstackSecurityGroupListener securityGroupListener =
+ new InternalSecurityGroupListener();
private final OpenstackNodeListener osNodeListener = new InternalNodeListener();
private ApplicationId appId;
private NodeId localNodeId;
@@ -174,7 +182,8 @@
localNodeId = clusterService.getLocalNode().id();
instancePortService.addListener(instancePortListener);
securityGroupService.addListener(securityGroupListener);
- osNetService.addListener(portListener);
+ osNetService.addListener(osPortListener);
+ osNetService.addListener(osNetworkListener);
configService.registerProperties(getClass());
osNodeService.addListener(osNodeListener);
@@ -185,7 +194,8 @@
protected void deactivate() {
instancePortService.removeListener(instancePortListener);
securityGroupService.removeListener(securityGroupListener);
- osNetService.removeListener(portListener);
+ osNetService.removeListener(osNetworkListener);
+ osNetService.removeListener(osPortListener);
configService.unregisterProperties(getClass(), false);
osNodeService.removeListener(osNodeListener);
eventExecutor.shutdown();
@@ -215,38 +225,41 @@
private void initializeConnTrackTable(DeviceId deviceId, boolean install) {
//table=1,ip,ct_state=-trk, actions=ct(table:2)
- long ctState = RulePopulatorUtil.computeCtStateFlag(false, false, false);
- long ctMask = RulePopulatorUtil.computeCtMaskFlag(true, false, false);
+ long ctState = computeCtStateFlag(false, false, false);
+ long ctMask = computeCtMaskFlag(true, false, false);
setConnTrackRule(deviceId, ctState, ctMask, CT_NO_COMMIT, (short) GOTO_CONNTRACK_TABLE,
ACTION_NONE, PRIORITY_CT_HOOK_RULE, install);
//table=2,ip,nw_dst=10.10.0.2,ct_state=+trk+est,action=goto_table:3
- ctState = RulePopulatorUtil.computeCtStateFlag(true, false, true);
- ctMask = RulePopulatorUtil.computeCtMaskFlag(true, false, true);
+ ctState = computeCtStateFlag(true, false, true);
+ ctMask = computeCtMaskFlag(true, false, true);
setConnTrackRule(deviceId, ctState, ctMask, CT_NO_COMMIT, CT_NO_RECIRC,
GOTO_JUMP_TABLE, PRIORITY_CT_RULE, install);
//table=2,ip,nw_dst=10.10.0.2,ct_state=+trk+new,action=drop
- ctState = RulePopulatorUtil.computeCtStateFlag(true, true, false);
- ctMask = RulePopulatorUtil.computeCtMaskFlag(true, true, false);
+ ctState = computeCtStateFlag(true, true, false);
+ ctMask = computeCtMaskFlag(true, true, false);
setConnTrackRule(deviceId, ctState, ctMask, CT_NO_COMMIT, CT_NO_RECIRC,
ACTION_DROP, PRIORITY_CT_DROP_RULE, install);
}
- private void setSecurityGroupRules(InstancePort instPort, Port port, boolean install) {
+ private void setSecurityGroupRules(InstancePort instPort,
+ Port port, boolean install) {
port.getSecurityGroups().forEach(sgId -> {
SecurityGroup sg = securityGroupService.securityGroup(sgId);
if (sg == null) {
log.error("Security Group Not Found : {}", sgId);
return;
}
- sg.getRules().forEach(sgRule -> updateSecurityGroupRule(instPort, port, sgRule, install));
+ sg.getRules().forEach(sgRule ->
+ updateSecurityGroupRule(instPort, port, sgRule, install));
final String action = install ? "Installed " : "Removed ";
log.debug(action + "security group rule ID : " + sgId);
});
}
- private void updateSecurityGroupRule(InstancePort instPort, Port port, SecurityGroupRule sgRule, boolean install) {
+ private void updateSecurityGroupRule(InstancePort instPort, Port port,
+ SecurityGroupRule sgRule, boolean install) {
if (instPort == null || port == null || sgRule == null) {
return;
@@ -254,23 +267,33 @@
if (sgRule.getRemoteGroupId() != null && !sgRule.getRemoteGroupId().isEmpty()) {
getRemoteInstPorts(port.getTenantId(), sgRule.getRemoteGroupId())
- .forEach(rInstPort -> {
- populateSecurityGroupRule(sgRule, instPort, rInstPort.ipAddress().toIpPrefix(), install);
- populateSecurityGroupRule(sgRule, rInstPort, instPort.ipAddress().toIpPrefix(), install);
+ .forEach(rInstPort -> {
+ populateSecurityGroupRule(sgRule, instPort,
+ rInstPort.ipAddress().toIpPrefix(), install);
+ populateSecurityGroupRule(sgRule, rInstPort,
+ instPort.ipAddress().toIpPrefix(), install);
- SecurityGroupRule rSgRule = new NeutronSecurityGroupRule.SecurityGroupRuleConcreteBuilder()
- .from(sgRule)
- .direction(sgRule.getDirection().toUpperCase().equals(EGRESS) ? INGRESS : EGRESS).build();
- populateSecurityGroupRule(rSgRule, instPort, rInstPort.ipAddress().toIpPrefix(), install);
- populateSecurityGroupRule(rSgRule, rInstPort, instPort.ipAddress().toIpPrefix(), install);
- });
+ SecurityGroupRule rSgRule =
+ new NeutronSecurityGroupRule
+ .SecurityGroupRuleConcreteBuilder()
+ .from(sgRule)
+ .direction(sgRule.getDirection().toUpperCase()
+ .equals(EGRESS) ? INGRESS : EGRESS)
+ .build();
+ populateSecurityGroupRule(rSgRule, instPort,
+ rInstPort.ipAddress().toIpPrefix(), install);
+ populateSecurityGroupRule(rSgRule, rInstPort,
+ instPort.ipAddress().toIpPrefix(), install);
+ });
} else {
- populateSecurityGroupRule(sgRule, instPort, sgRule.getRemoteIpPrefix() == null ? IP_PREFIX_ANY :
+ populateSecurityGroupRule(sgRule, instPort,
+ sgRule.getRemoteIpPrefix() == null ? IP_PREFIX_ANY :
IpPrefix.valueOf(sgRule.getRemoteIpPrefix()), install);
}
}
- private void populateSecurityGroupRule(SecurityGroupRule sgRule, InstancePort instPort,
+ private void populateSecurityGroupRule(SecurityGroupRule sgRule,
+ InstancePort instPort,
IpPrefix remoteIp, boolean install) {
Set<TrafficSelector> selectors = buildSelectors(sgRule,
Ip4Address.valueOf(instPort.ipAddress().toInetAddress()), remoteIp);
@@ -307,8 +330,8 @@
int commit, short recircTable,
int action, int priority, boolean install) {
- ExtensionSelector esCtSate = RulePopulatorUtil.buildCtExtensionSelector(driverService, deviceId,
- ctState, ctMask);
+ ExtensionSelector esCtSate = RulePopulatorUtil
+ .buildCtExtensionSelector(driverService, deviceId, ctState, ctMask);
TrafficSelector selector = DefaultTrafficSelector.builder()
.extension(esCtSate, deviceId)
.matchEthType(Ethernet.TYPE_IPV4)
@@ -318,7 +341,7 @@
if (commit == CT_COMMIT || recircTable > 0) {
RulePopulatorUtil.NiriraConnTrackTreatmentBuilder natTreatmentBuilder =
- RulePopulatorUtil.niciraConnTrackTreatmentBuilder(driverService, deviceId);
+ niciraConnTrackTreatmentBuilder(driverService, deviceId);
natTreatmentBuilder.natAction(false);
if (commit == CT_COMMIT) {
natTreatmentBuilder.commit(true);
@@ -388,30 +411,31 @@
Set<TrafficSelector> selectorSet = Sets.newHashSet();
TrafficSelector.Builder sBuilder = DefaultTrafficSelector.builder();
- buildMatchs(sBuilder, sgRule, vmIp, remoteIp);
+ buildMatches(sBuilder, sgRule, vmIp, remoteIp);
if (sgRule.getPortRangeMax() != null && sgRule.getPortRangeMin() != null &&
sgRule.getPortRangeMin() < sgRule.getPortRangeMax()) {
- Map<TpPort, TpPort> portRangeMatchMap = buildPortRangeMatches(sgRule.getPortRangeMin(),
+ Map<TpPort, TpPort> portRangeMatchMap =
+ buildPortRangeMatches(sgRule.getPortRangeMin(),
sgRule.getPortRangeMax());
portRangeMatchMap.entrySet().forEach(entry -> {
- if (sgRule.getProtocol().toUpperCase().equals(PROTO_TCP)) {
- if (sgRule.getDirection().toUpperCase().equals(EGRESS)) {
- sBuilder.matchTcpSrcMasked(entry.getKey(), entry.getValue());
- } else {
- sBuilder.matchTcpDstMasked(entry.getKey(), entry.getValue());
- }
- } else if (sgRule.getProtocol().toUpperCase().equals(PROTO_UDP)) {
- if (sgRule.getDirection().toUpperCase().equals(EGRESS)) {
- sBuilder.matchUdpSrcMasked(entry.getKey(), entry.getValue());
- } else {
- sBuilder.matchUdpDstMasked(entry.getKey(), entry.getValue());
- }
- }
+ if (sgRule.getProtocol().toUpperCase().equals(PROTO_TCP)) {
+ if (sgRule.getDirection().toUpperCase().equals(EGRESS)) {
+ sBuilder.matchTcpSrcMasked(entry.getKey(), entry.getValue());
+ } else {
+ sBuilder.matchTcpDstMasked(entry.getKey(), entry.getValue());
+ }
+ } else if (sgRule.getProtocol().toUpperCase().equals(PROTO_UDP)) {
+ if (sgRule.getDirection().toUpperCase().equals(EGRESS)) {
+ sBuilder.matchUdpSrcMasked(entry.getKey(), entry.getValue());
+ } else {
+ sBuilder.matchUdpDstMasked(entry.getKey(), entry.getValue());
+ }
+ }
- selectorSet.add(sBuilder.build());
- }
+ selectorSet.add(sBuilder.build());
+ }
);
} else {
selectorSet.add(sBuilder.build());
@@ -420,8 +444,9 @@
return selectorSet;
}
- private void buildMatchs(TrafficSelector.Builder sBuilder, SecurityGroupRule sgRule,
- Ip4Address vmIp, IpPrefix remoteIp) {
+ private void buildMatches(TrafficSelector.Builder sBuilder,
+ SecurityGroupRule sgRule,
+ Ip4Address vmIp, IpPrefix remoteIp) {
buildMatchEthType(sBuilder, sgRule.getEtherType());
buildMatchDirection(sBuilder, sgRule.getDirection(), vmIp);
buildMatchProto(sBuilder, sgRule.getProtocol());
@@ -453,8 +478,10 @@
}
}
- private void buildMatchRemoteIp(TrafficSelector.Builder sBuilder, IpPrefix remoteIpPrefix, String direction) {
- if (remoteIpPrefix != null && !remoteIpPrefix.getIp4Prefix().equals(IP_PREFIX_ANY)) {
+ private void buildMatchRemoteIp(TrafficSelector.Builder sBuilder,
+ IpPrefix remoteIpPrefix, String direction) {
+ if (remoteIpPrefix != null &&
+ !remoteIpPrefix.getIp4Prefix().equals(IP_PREFIX_ANY)) {
if (direction.toUpperCase().equals(EGRESS)) {
sBuilder.matchIPDst(remoteIpPrefix);
} else {
@@ -480,7 +507,8 @@
}
}
- private void buildMatchPort(TrafficSelector.Builder sBuilder, String protocol, String direction,
+ private void buildMatchPort(TrafficSelector.Builder sBuilder,
+ String protocol, String direction,
int portMin, int portMax) {
if (portMin > 0 && portMax > 0 && portMin == portMax) {
if (protocol.toUpperCase().equals(PROTO_TCP)) {
@@ -503,7 +531,8 @@
if (useSecurityGroup) {
osNodeService.completeNodes(OpenstackNode.NodeType.COMPUTE)
- .forEach(node -> osFlowRuleService.setUpTableMissEntry(node.intgBridge(), ACL_TABLE));
+ .forEach(node -> osFlowRuleService
+ .setUpTableMissEntry(node.intgBridge(), ACL_TABLE));
securityGroupService.securityGroups().forEach(securityGroup ->
securityGroup.getRules().forEach(this::securityGroupRuleAdded));
osNodeService.nodes().stream()
@@ -511,7 +540,8 @@
.forEach(node -> initializeConnTrackTable(node .intgBridge(), true));
} else {
osNodeService.completeNodes(OpenstackNode.NodeType.COMPUTE)
- .forEach(node -> osFlowRuleService.connectTables(node.intgBridge(), ACL_TABLE, JUMP_TABLE));
+ .forEach(node -> osFlowRuleService
+ .connectTables(node.intgBridge(), ACL_TABLE, JUMP_TABLE));
securityGroupService.securityGroups().forEach(securityGroup ->
securityGroup.getRules().forEach(this::securityGroupRuleRemoved));
osNodeService.nodes().stream()
@@ -519,12 +549,14 @@
.forEach(node -> initializeConnTrackTable(node.intgBridge(), false));
}
- log.info("Reset security group info " + (useSecurityGroup ? " with " : " without") + " Security Group");
+ log.info("Reset security group info " +
+ (useSecurityGroup ? " with " : " without") + " Security Group");
}
private void securityGroupRuleAdded(SecurityGroupRule sgRule) {
osNetService.ports().stream()
- .filter(port -> port.getSecurityGroups().contains(sgRule.getSecurityGroupId()))
+ .filter(port -> port.getSecurityGroups()
+ .contains(sgRule.getSecurityGroupId()))
.forEach(port -> {
updateSecurityGroupRule(
instancePortService.instancePort(port.getId()),
@@ -536,7 +568,8 @@
private void securityGroupRuleRemoved(SecurityGroupRule sgRule) {
osNetService.ports().stream()
- .filter(port -> port.getSecurityGroups().contains(sgRule.getSecurityGroupId()))
+ .filter(port -> port.getSecurityGroups()
+ .contains(sgRule.getSecurityGroupId()))
.forEach(port -> {
updateSecurityGroupRule(
instancePortService.instancePort(port.getId()),
@@ -614,7 +647,8 @@
int maskEnd = binHigher(binStrMinPadded, mask);
log.debug("start : {} port/mask = {} / {} ", start, getMask(mask), maskStart);
- portMaskMap.put(TpPort.tpPort(maskStart), TpPort.tpPort(Integer.parseInt(getMask(mask), 16)));
+ portMaskMap.put(TpPort.tpPort(maskStart), TpPort.tpPort(
+ Integer.parseInt(Objects.requireNonNull(getMask(mask)), 16)));
start = maskEnd + 1;
if (start > portMax) {
@@ -651,16 +685,6 @@
true);
});
break;
- case OPENSTACK_INSTANCE_PORT_VANISHED:
- log.debug("Instance port vanished MAC:{} IP:{}",
- instPort.macAddress(),
- instPort.ipAddress());
- eventExecutor.execute(() -> {
- setSecurityGroupRules(instPort,
- osNetService.port(event.subject().portId()),
- false);
- });
- break;
default:
break;
}
@@ -674,6 +698,42 @@
if (event.port() == null || Strings.isNullOrEmpty(event.port().getId())) {
return false;
}
+
+ InstancePort instPort = instancePortService.instancePort(event.port().getId());
+
+ if (instPort == null) {
+ return false;
+ }
+
+ return useSecurityGroup && mastershipService.isLocalMaster(instPort.deviceId());
+ }
+
+ @Override
+ public void event(OpenstackNetworkEvent event) {
+ log.debug("openstack port event received {}", event);
+ Port osPort = event.port();
+ InstancePort instPort = instancePortService.instancePort(osPort.getId());
+
+ switch (event.type()) {
+ case OPENSTACK_PORT_PRE_REMOVE:
+ eventExecutor.execute(() -> {
+ setSecurityGroupRules(instPort, osPort, false);
+ });
+ break;
+ default:
+ // do nothing for the other events
+ break;
+ }
+ }
+ }
+
+ private class InternalOpenstackNetworkListener implements OpenstackNetworkListener {
+
+ @Override
+ public boolean isRelevant(OpenstackNetworkEvent event) {
+ if (event.port() == null || Strings.isNullOrEmpty(event.port().getId())) {
+ return false;
+ }
if (event.securityGroupId() == null ||
securityGroupService.securityGroup(event.securityGroupId()) == null) {
return false;
@@ -681,10 +741,7 @@
if (instancePortService.instancePort(event.port().getId()) == null) {
return false;
}
- if (!useSecurityGroup) {
- return false;
- }
- return true;
+ return useSecurityGroup;
}
@Override
@@ -724,10 +781,7 @@
@Override
public boolean isRelevant(OpenstackSecurityGroupEvent event) {
- if (!useSecurityGroup) {
- return false;
- }
- return true;
+ return useSecurityGroup;
}
@Override
@@ -781,10 +835,12 @@
try {
if (useSecurityGroup) {
initializeConnTrackTable(osNode.intgBridge(), true);
- log.warn("SG table initialization : {} is done", osNode.intgBridge());
+ log.warn("SG table initialization : {} is done",
+ osNode.intgBridge());
}
} catch (IllegalArgumentException e) {
- log.error("ACL table initialization error : {}", e.getMessage());
+ log.error("ACL table initialization error : {}",
+ e.getMessage());
}
});
break;
@@ -797,4 +853,4 @@
}
}
}
-}
+}
\ No newline at end of file
diff --git a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSwitchingArpHandler.java b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSwitchingArpHandler.java
index 59241cb..dceb7ca 100644
--- a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSwitchingArpHandler.java
+++ b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSwitchingArpHandler.java
@@ -651,6 +651,11 @@
}
private void setDefaultArpRule(OpenstackNode osNode, boolean install) {
+
+ if (getArpMode() == null) {
+ return;
+ }
+
switch (getArpMode()) {
case ARP_PROXY_MODE:
setDefaultArpRuleForProxyMode(osNode, install);