[ONOS-7493] Segment routing may uninstall forwarding with no next id
Change-Id: Id78b9ed94633b7e96fdeebe185e28d6de386e3ec
diff --git a/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java b/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java
index 796891d..b4ce830 100644
--- a/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java
+++ b/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java
@@ -122,8 +122,7 @@
tableEntryBuilder
.forTable(piTableId)
- .withMatchKey(piMatchKey)
- .withAction(piTableAction);
+ .withMatchKey(piMatchKey);
if (piTableAction != null) {
tableEntryBuilder.withAction(piTableAction);
diff --git a/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricTreatmentInterpreter.java b/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricTreatmentInterpreter.java
index badb38c..6346302 100644
--- a/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricTreatmentInterpreter.java
+++ b/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricTreatmentInterpreter.java
@@ -22,6 +22,7 @@
import org.onlab.packet.VlanId;
import org.onlab.util.ImmutableByteSequence;
import org.onosproject.net.PortNumber;
+import org.onosproject.net.flow.DefaultTrafficTreatment;
import org.onosproject.net.flow.TrafficTreatment;
import org.onosproject.net.flow.instructions.Instruction;
import org.onosproject.net.flow.instructions.Instructions;
@@ -126,6 +127,10 @@
public static PiAction mapForwardingTreatment(TrafficTreatment treatment)
throws PiInterpreterException {
+ // Empty treatment, generate table entry with no action
+ if (treatment.equals(DefaultTrafficTreatment.emptyTreatment())) {
+ return null;
+ }
List<Instruction> insts = treatment.allInstructions();
OutputInstruction outInst = insts.stream()
.filter(inst -> inst.type() == OUTPUT)
diff --git a/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/pipeliner/FabricForwardingPipeliner.java b/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/pipeliner/FabricForwardingPipeliner.java
index e72042f..5424a49 100644
--- a/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/pipeliner/FabricForwardingPipeliner.java
+++ b/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/pipeliner/FabricForwardingPipeliner.java
@@ -169,12 +169,6 @@
checkNotNull(vlanIdCriterion, "VlanId criterion should not be null");
checkNotNull(ethDstCriterion, "EthDst criterion should not be null");
- if (fwd.nextId() == null) {
- log.warn("Forwarding objective for L2 unicast should contains next id");
- resultBuilder.setError(ObjectiveError.BADPARAMS);
- return;
- }
-
VlanId vlanId = vlanIdCriterion.vlanId();
MacAddress ethDst = ethDstCriterion.mac();
@@ -182,7 +176,11 @@
.matchVlanId(vlanId)
.matchEthDst(ethDst)
.build();
- TrafficTreatment treatment = buildSetNextIdTreatment(fwd.nextId());
+ TrafficTreatment treatment = fwd.treatment();
+ if (fwd.nextId() != null) {
+ treatment = buildSetNextIdTreatment(fwd.nextId());
+ }
+
FlowRule flowRule = DefaultFlowRule.builder()
.withSelector(selector)
.withTreatment(treatment)
@@ -200,18 +198,16 @@
ForwardingObjective fwd,
PipelinerTranslationResult.Builder resultBuilder) {
checkNotNull(vlanIdCriterion, "VlanId criterion should not be null");
- if (fwd.nextId() == null) {
- log.warn("Forwarding objective for L2 broadcast should contains next id");
- resultBuilder.setError(ObjectiveError.BADPARAMS);
- return;
- }
VlanId vlanId = vlanIdCriterion.vlanId();
TrafficSelector selector = DefaultTrafficSelector.builder()
.matchVlanId(vlanId)
.build();
- TrafficTreatment treatment = buildSetNextIdTreatment(fwd.nextId());
+ TrafficTreatment treatment = fwd.treatment();
+ if (fwd.nextId() != null) {
+ treatment = buildSetNextIdTreatment(fwd.nextId());
+ }
FlowRule flowRule = DefaultFlowRule.builder()
.withSelector(selector)
.withTreatment(treatment)
@@ -228,16 +224,13 @@
private void processIpv4UnicastRule(IPCriterion ipDstCriterion, ForwardingObjective fwd,
PipelinerTranslationResult.Builder resultBuilder) {
checkNotNull(ipDstCriterion, "IP dst criterion should not be null");
- if (fwd.nextId() == null) {
- log.warn("Forwarding objective for IPv4 unicast should contains next id");
- resultBuilder.setError(ObjectiveError.BADPARAMS);
- return;
- }
TrafficSelector selector = DefaultTrafficSelector.builder()
.matchIPDst(ipDstCriterion.ip())
.build();
-
- TrafficTreatment treatment = buildSetNextIdTreatment(fwd.nextId());
+ TrafficTreatment treatment = fwd.treatment();
+ if (fwd.nextId() != null) {
+ treatment = buildSetNextIdTreatment(fwd.nextId());
+ }
FlowRule flowRule = DefaultFlowRule.builder()
.withSelector(selector)
.withTreatment(treatment)
@@ -254,25 +247,25 @@
private void processMplsRule(MplsCriterion mplsCriterion, ForwardingObjective fwd,
PipelinerTranslationResult.Builder resultBuilder) {
checkNotNull(mplsCriterion, "Mpls criterion should not be null");
- if (fwd.nextId() == null) {
- log.warn("Forwarding objective for MPLS should contains next id");
- resultBuilder.setError(ObjectiveError.BADPARAMS);
- return;
+ TrafficTreatment treatment;
+
+ treatment = fwd.treatment();
+ if (fwd.nextId() != null) {
+ PiActionParam nextIdParam = new PiActionParam(FabricConstants.ACT_PRM_NEXT_ID_ID,
+ ImmutableByteSequence.copyFrom(fwd.nextId().byteValue()));
+ PiAction nextIdAction = PiAction.builder()
+ .withId(FabricConstants.ACT_FORWARDING_POP_MPLS_AND_NEXT_ID)
+ .withParameter(nextIdParam)
+ .build();
+ treatment = DefaultTrafficTreatment.builder()
+ .piTableAction(nextIdAction)
+ .build();
}
+
TrafficSelector selector = DefaultTrafficSelector.builder()
.add(mplsCriterion)
.build();
- PiActionParam nextIdParam = new PiActionParam(FabricConstants.ACT_PRM_NEXT_ID_ID,
- ImmutableByteSequence.copyFrom(fwd.nextId().byteValue()));
- PiAction nextIdAction = PiAction.builder()
- .withId(FabricConstants.ACT_FORWARDING_POP_MPLS_AND_NEXT_ID)
- .withParameter(nextIdParam)
- .build();
- TrafficTreatment treatment = DefaultTrafficTreatment.builder()
- .piTableAction(nextIdAction)
- .build();
-
FlowRule flowRule = DefaultFlowRule.builder()
.withSelector(selector)
.withTreatment(treatment)
@@ -286,6 +279,13 @@
resultBuilder.addFlowRule(flowRule);
}
+ /**
+ * Builds treatment with set_next_id action, returns empty treatment
+ * if next id is null.
+ *
+ * @param nextId the next id for action
+ * @return treatment with set_next_id action; empty treatment if next id is null
+ */
private static TrafficTreatment buildSetNextIdTreatment(Integer nextId) {
PiActionParam nextIdParam = new PiActionParam(FabricConstants.ACT_PRM_NEXT_ID_ID,
ImmutableByteSequence.copyFrom(nextId.byteValue()));
diff --git a/pipelines/fabric/src/test/java/org/onosproject/pipelines/fabric/FabricInterpreterTest.java b/pipelines/fabric/src/test/java/org/onosproject/pipelines/fabric/FabricInterpreterTest.java
index f0d27f35..8d97f20 100644
--- a/pipelines/fabric/src/test/java/org/onosproject/pipelines/fabric/FabricInterpreterTest.java
+++ b/pipelines/fabric/src/test/java/org/onosproject/pipelines/fabric/FabricInterpreterTest.java
@@ -30,6 +30,7 @@
import org.onosproject.net.pi.runtime.PiActionParam;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
/**
* Test for fabric interpreter.
@@ -125,6 +126,17 @@
assertEquals(expectedAction, mappedAction);
}
+ /**
+ * Map empty treatment for forwarding block to nop action.
+ */
+ @Test
+ public void testEmptyForwardingTreatment() throws Exception {
+ TrafficTreatment treatment = DefaultTrafficTreatment.emptyTreatment();
+ PiAction mappedAction = interpreter.mapTreatment(treatment,
+ FabricConstants.TBL_UNICAST_V4_ID);
+ assertNull(mappedAction);
+ }
+
/* Next control block */
/**
diff --git a/pipelines/fabric/src/test/java/org/onosproject/pipelines/fabric/pipeliner/FabricForwardingPipelineTest.java b/pipelines/fabric/src/test/java/org/onosproject/pipelines/fabric/pipeliner/FabricForwardingPipelineTest.java
index e65e30b..d29beaa 100644
--- a/pipelines/fabric/src/test/java/org/onosproject/pipelines/fabric/pipeliner/FabricForwardingPipelineTest.java
+++ b/pipelines/fabric/src/test/java/org/onosproject/pipelines/fabric/pipeliner/FabricForwardingPipelineTest.java
@@ -169,6 +169,18 @@
}
@Test
+ public void testIPv4UnicastWithNoNextId() {
+ TrafficSelector selector = DefaultTrafficSelector.builder()
+ .matchEthType(Ethernet.TYPE_IPV4)
+ .matchIPDst(IPV4_UNICAST_ADDR)
+ .build();
+ TrafficSelector expectedSelector = DefaultTrafficSelector.builder()
+ .matchIPDst(IPV4_UNICAST_ADDR)
+ .build();
+ testSpecificForward(FabricConstants.TBL_UNICAST_V4_ID, expectedSelector, selector, null);
+ }
+
+ @Test
@Ignore
public void testIPv4Multicast() {
TrafficSelector selector = DefaultTrafficSelector.builder()
@@ -234,15 +246,21 @@
private void testSpecificForward(PiTableId expectedTableId, TrafficSelector expectedSelector,
TrafficSelector selector, Integer nextId) {
- PiActionParam nextIdParam = new PiActionParam(FabricConstants.ACT_PRM_NEXT_ID_ID,
- ImmutableByteSequence.copyFrom(nextId.byteValue()));
- PiAction setNextIdAction = PiAction.builder()
- .withId(FabricConstants.ACT_FORWARDING_SET_NEXT_ID_ID)
- .withParameter(nextIdParam)
- .build();
- TrafficTreatment setNextIdTreatment = DefaultTrafficTreatment.builder()
- .piTableAction(setNextIdAction)
- .build();
+ TrafficTreatment setNextIdTreatment;
+ if (nextId == null) {
+ // Ref: RoutingRulePopulator.java->revokeIpRuleForRouter
+ setNextIdTreatment = DefaultTrafficTreatment.builder().build();
+ } else {
+ PiActionParam nextIdParam = new PiActionParam(FabricConstants.ACT_PRM_NEXT_ID_ID,
+ ImmutableByteSequence.copyFrom(nextId.byteValue()));
+ PiAction setNextIdAction = PiAction.builder()
+ .withId(FabricConstants.ACT_FORWARDING_SET_NEXT_ID_ID)
+ .withParameter(nextIdParam)
+ .build();
+ setNextIdTreatment = DefaultTrafficTreatment.builder()
+ .piTableAction(setNextIdAction)
+ .build();
+ }
testSpecificForward(expectedTableId, expectedSelector, selector, nextId, setNextIdTreatment);
@@ -250,16 +268,19 @@
private void testSpecificForward(PiTableId expectedTableId, TrafficSelector expectedSelector,
TrafficSelector selector, Integer nextId, TrafficTreatment treatment) {
- ForwardingObjective fwd = DefaultForwardingObjective.builder()
+ ForwardingObjective.Builder fwd = DefaultForwardingObjective.builder()
.withSelector(selector)
.withPriority(PRIORITY)
.fromApp(APP_ID)
.makePermanent()
- .withFlag(ForwardingObjective.Flag.SPECIFIC)
- .nextStep(nextId)
- .add();
+ .withTreatment(treatment)
+ .withFlag(ForwardingObjective.Flag.SPECIFIC);
- PipelinerTranslationResult result = pipeliner.pipelinerForward.forward(fwd);
+ if (nextId != null) {
+ fwd.nextStep(nextId);
+ }
+
+ PipelinerTranslationResult result = pipeliner.pipelinerForward.forward(fwd.add());
List<FlowRule> flowRulesInstalled = (List<FlowRule>) result.flowRules();
List<GroupDescription> groupsInstalled = (List<GroupDescription>) result.groups();