[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openvswitch-discuss
Subject:    [ovs-discuss] ovs-conntrack action ordering?
From:       joestringer () nicira ! com (Joe Stringer)
Date:       2015-06-25 10:23:51
Message-ID: CANr6G5w8huEGRwsfQ85bsnQKzaT9J5CO1_m8RUm5HQ-BzCBLnA () mail ! gmail ! com
[Download RAW message or body]

Good example. I agree that the ovs conntrack support should consider
the ordering this way. Setting conn_mark/conn_label is also relevant
to this, as they are currently done via commit_odp_actions(). So they
cannot be done before conntrack() action. This was already how we were
thinking about these commands, and perhaps their ordering should also
be enforced by OpenFlow.

Right, the commit call is what I was looking for. I'll roll this
change into the series soon and start testing with it, see what comes
up. Perhaps the datapath flow translation like this would be a good
test case for the kernel module testsuite too.

Thanks,
Joe

On 25 June 2015 at 11:50, John Hurley <john.hurley at netronome.com> wrote:
> Hi Joe,
>
> Thanks for the response.
>
> Agree with your MPLS points. I was just using the MPLS example as a
> hypothetical situation as I noticed it called the action commit function so
> it could be used to modify IP address fields before Conntrack that were not
> be modified in other ways.
>
> I was more looking clarification that the ordering of the rule actions is
> important in ovs-conntrack so that if we do an IP address set field followed
> by a CT() then the updated IP field should pass into nf_conntrack rather
> than it being the original tuples (as is now).
>
> To me, the former makes more sense here. For example, if the OVS was
> implementing some form of NAT along with connection tracking then using the
> input tuples and not updated fields means we could not track flows
> bidirectionally.
>
> I will take from your response that OVS-Conntrack should consider the
> ordering in this way?
>
> I 'think' the fix for this in the code is quite simple. Just move the action
> commit call from the compose_recirculate_action__() in ofproto-dpif-xlate.c
> to before the netlink message generation in compose_conntrack_action() in
> the same file.  That way previous actions will be pushed before the
> conntrack action.  The commit code would also need to be added to
> compose_recirculation_action() to ensure the actions are committed if a
> recirculation action occurs outside of conntrack.
>
> Thanks,
> John
>
>
>
>
> On Thu, Jun 25, 2015 at 10:05 AM, Joe Stringer <joestringer at nicira.com>
> wrote:
>>
>> Hi John,
>>
>> Thanks for reporting this. It sounds like a bug. As an OpenFlow user,
>> It would be surprising for the order of these actions in the action
>> list to be rearranged like you report. For many fields this doesn't
>> matter (eg, if you only write IP then write TCP port), however for
>> conntrack, the order is more important - when you change the IP
>> address affects how the connection is tracked.  So the flow generation
>> for the first rule looks wrong from that perspective.
>>
>> The core problem here is that while processing the actions list, many
>> of the "set_field" operations are deferred until the end of the
>> processing, whereas conntrack action is not. I think we need to come
>> up with some way to ensure that the flow changes are committed before
>> adding the conntrack action to the datapath actions list. I suspect
>> much of the code is already there for this, it just needs to be hooked
>> up correctly.
>>
>> For the second flow, in some ways I'm inclined to go with the same
>> approach - Commit the MPLS before committing the conntrack action.
>> Also, I don't know what you're expecting the connection tracker to do
>> when it receives MPLS packets. I doubt that the connection tracker
>> would understand how to track MPLS packets or associate them with
>> connections. I'm not sure what behaviour you're getting today, but I
>> would expect all such packets to come back with conn_state=+trk+inv
>> (ie, invalid). What does it even mean to connection-track MPLS
>> packets?
>>
>> On 24 June 2015 at 18:12, John Hurley <john.hurley at netronome.com> wrote:
>> > Hi,
>> >
>> > I have been playing about with ovs-conntrack and noticed an issue that
>> > could
>> > be a bug. Either that or my understanding is incorrect and would
>> > appreciate
>> > clarification.
>> >
>> > When we add a rule with a ct(recirc) action I notice that the call to
>> > Conntrack is always the first action in the kernel rule that is created.
>> > In
>> > ofproto-dpif-xlate.c a call to compose_conntrack_action() will append a
>> > nl_msg for this action, then call recirculate which uses
>> > commit_odp_actions() to add messages for previous actions before adding
>> > its
>> > own recirc action to kernel rule action list.  However, I have also
>> > noticed
>> > that when a push mpls action is added that this commit_odp_actions()
>> > function is also called which can affect the tuples sent to
>> > nf_conntrack.
>> >
>> > e.g. adding the rule:
>> > ovs-ofctl add-flow br0
>> > "conn_state=-trk,action=set_field:1.2.3.4->nw_dst,ct(recirc)"
>> >
>> > will result in a kernel rule with actions of order 'ct, set dst ip,
>> > recirc'
>> > and the sk_buff sent to nf_conntrack_in will have the source and
>> > destination
>> > IP addresses of the matching packet.
>> >
>> >
>> > adding the rule:
>> > ovs-ofctl add-flow br0
>> >
>> > "conn_state=-trk,action=set_field:1.2.3.4->nw_dst,push_mpls:0x8847,ct(recirc)"
>> >
>> > will have actions ordered 'set dst ip, ct, push mpls, recirc' and the
>> > destination IP sent into nf_conntrack will be 1.2.3.4
>> >
>> > Can you clarify if this discrepancy is a bug? Also, can you clarify what
>> > is
>> > the correct way ovs should support Conntrack in an action list? For
>> > example,
>> > in the first rule, is it correct that Conntrack should be applied on the
>> > input packet tuples or should the set field be applied before Conntrack
>> > as
>> > it appears before it in the ovs-ofctl rule?
>> >
>> > Thanks,
>> > John
>> >
>> >
>> >
>> > _______________________________________________
>> > discuss mailing list
>> > discuss at openvswitch.org
>> > http://openvswitch.org/mailman/listinfo/discuss
>> >
>
>

[prev in list] [next in list] [prev in thread] [next in thread]