Flow top level `instructions` and `actions` (historic OF1.0 format) should be mutually exclusive

[viniarck]

Historically, most NApps have been using actions, which on OF1.3 context, it'll be converted as OF FlowMod instruction when being serialized, but we've been also supporting actions to avoid breaking APIs and having to change NApps that use this format. There's no issues or harm to still use actions in the old format, and it'll be maintained, but it should be mutually exclusive with instructions, otherwise it can result in OFPT_ERROR:

Example of invalid payload:

{
    "force": false,
    "flows": [
        {
            "priority": 38000,
            "match": {
                "in_port": 1,
                "dl_vlan": 1004
            },
            "instructions": [
                {
                    "instruction_type": "apply_actions",
                    "actions": [
                        {
                            "action_type": "output",
                            "port": 10
                        }
                    ]
                }
            ],
            "actions": [
                {
                    "action_type": "output",
                    "port": 11
                }
            ]
        }
    ]
}

[Alopalao]

I think the problem lies in of_core, specifically here. The final dictionary of instructions ends up like:

            "instructions": [
                {   
                    "instruction_type": "apply_actions", 
                    "actions": [{"port": 11, "action_type": "output"}]
                },
                {
                    "instruction_type": "apply_actions", 
                    "actions": [{"port": 10, "action_type": "output"}]
                }
            ]

I see other examples that multiple actions should be:

            "instructions": [
                {
                    "instruction_type": "apply_actions",
                    "actions": [
                        {"action_type": "output", "port": 10},
                        {"action_type": "output", "port": 1}
                    ]
                }
            ]

Do we want to support instructions with multiple apply_actions? If not, I see that actions list should be ordered, which field from the request should be first, actions should be added to existent instructions last?

[viniarck]

Do we want to support instructions with multiple apply_actions?

That's a good point, based on the spec and OvS man page, for each instruction, it should not repeat the "instruction_type":

Every version of OpenFlow includes actions. OpenFlow 1.1 introduced the higher-level, related concept of instructions. In OpenFlow 1.1 and later, actions within a flow are always encapsulated within an instruction. Each flow has at most one instruction of each kind, which are executed in the following fixed order defined in the OpenFlow specification:

      1. Meter

      2. Apply-Actions

      3. Clear-Actions

      4. Write-Actions

      5. Write-Metadata

      6. Stat-Trigger (not supported by Open vSwitch)

      7. Goto-Table

and probably for each action, we probably shouldn't allow repeated action_type either, but I confirmed in practice, that at least with OvS it didn't crash though, here's an example with multiple output action_type:

 cookie=0x0, duration=1024.068s, table=0, n_packets=0, n_bytes=0, send_flow_rem priority=38000,in_port="s1-eth1",dl_vlan=1004 actions=output:10,output:11

so for the action_type, let's leave it for another iteration and issue, just so there are cases like this one above that it still be great to still keep paring this flow without crashing or discarding it (which is important for consistency check when also detecting alien flows).

In summary

• Let's start validating to accept only a different instruction type per instructions on flow_manager API.
• Let's start validating to accept only either instructions or actions mutually exclusive at the top level on flow_manager API
• On the part that you mentioned on of_core, historically actions used to be supported at the top level due to OF1.0, we still want to maintain this compatibility to avoid breaking dicts and API payloads, but in that part you showed, I believe when actions is set, if you were to add an early return that would also improve it since it wouldn't try to process instructions again. Good catch @Alopalao, I think it'd be worth to create a new issue and address it too there, even though with flow_manager validation that part theoretically wouldn't be hit at least for most of our NApps that send FlowMods.

Let me know if you agree and share the same understanding @Alopalao.

cc'ing also @italovalcy in case he wants to confirm or point out any other requirement.

[Alopalao]

Makes sense to me, I'm on it.