"Owned" fallout

Marius Vollmer marius.vollmer at redhat.com
Mon Jun 22 09:35:02 UTC 2015


Stef Walter <stefw at redhat.com> writes:

> On 18.06.2015 03:17, Marius Vollmer wrote:
>
>> Once we have feature negotiation, I think we should not ignore unknown
>> messages anymore.  It is better to know whether or not your peer will
>> understand a specific message and adapt to it.
>
> The entire protocol is based around ignoring stuff it doesn't know
> about. This starts with fields in JSON open messages ... new control
> messages and so on.

Yes, I agree.  'Crashing' because of unknown messages is not useful in
production.  (It might help during development, maybe.)

> For stuff that is defined, the behavior is usually strictly defined, but
> unknown stuff is ignored.
>
> Hence the need for a 'caps' or 'capabilities' field so that the caller
> can enforce that the callee understands some new aspect of a payload or
> protocol.

I am not sure that I agree with the 'enforce' aspect of this.  We want
to work with all kind of version combinations, and if a payload doesn't
have a certain feature, the caller should fall back to an alternative
approach that doesn't need it.

> [...]  It seems to me that this is primarily a bug fix, first and
> foremost. The cockpit.js code should not have thrown a fit when it got
> an unknown message.

I agree.

>>> 2) Add a capabilities property to cockpitchannel and close the channel
>>> if there is no match in cockpit_channel_real_prepare
>>
>> Do we need to have generic logic for detecting incompatibilities once we
>> have feature/capabilities exchange?  In fact, the point is to avoid
>> incompatibilities, and there should ideally never be a reason to close a
>> channel, no?
>
> Not sure I understand. It's the same mechanism.

I think feature negotiation on a high level is preferrable to rejecting
incompatible peers on a low level.

In fact, I would expect us to pretty quickly use the
"close-channel-on-unsupported-cap" feature to implement feature
negotiation anyway: "Let's try with this requirement, and if the bridge
closes the channel, make a note that the feature is not available and
try without."

To prepare for that, should the "close" message send back all supported
capabilities?

>> So, without actually digging into it and verifying the feasibility, what
>> about this:
>>
>> - DBusClient adds { notify_owner: true } to the open command and
>>   understands "owner" messages.  It still aborts on unknown messages.
>>
>> - The bridge only sends "owner" messages when "notify_owner" is true.
>>
>> This ought to fix the immediate problem, no?  (Old bridges will ignore
>> "notify_owner", and old clients wont set it.)
>
> Right, that's what Peter was talking about earlier. Although he was
> suggesting we fix at a lower level. Fixing the dbus-json3 protocol to
> have better behavior about ignoring unknown messages.

I now think we should do both.  The difference is what versions need to
be fixed to get rid of the bug: Ignoring unknown messages needs to be
done in all old versions (RHEL, CentOS, F21), not sending unknown
messages needs only be done in the new versions (F22).

>> With this in place, we can design a more general solution more
>> carefully.  It would require two way feature exchange, I think, and we
>> probably want to do it in a generic way for all payloads.
>
> That's exactly what the "caps" stuff is.

Except is is one-way only.  I think that's a significant difference.

> [...]  We don't need to get more complicated than that, as we already
> have optional/ignored fields in open messages that can be used in
> cases where the feature is requested but not required.

It might be valuable to know whether or not a field was ignored in some
cases, but in that case we can add a "cap" for it, right?


Let me try to summarize the two approaches.

Everyone should ignore unknown messages and other protocol additions
(including the bridge), but we can't rely on that yet because of bugs.

1) A channel is closed when the open message has a "capabilities" (or
   "caps") field that the bridge doesn't understand.

   Protocol additions (such as a new message type) from bridge to
   cockpit.js have a corresponding field in the "open" message, and the
   bridge will use that protocol addition when explicitly requested by
   that field.

   Protocol additions from cockpit.js to the bridge have a corresponding
   capability and cockpit.js only uses them when it has also added the
   corresponding capability to the channel options.

2) 'Somehow', cockpit.js and the bridge exchange their features on the
   first connection.

   Cockpit.js only uses a protocol addition if the bridge has announced
   the corresponding feature.

   The bridge only uses a protocol addition if cockpit.js has annnounced
   the corresponding feature.


IMO, approach 1 has the downside that cockpit.js can only learn the
features of the bridge by trial-and-error: Open a channel with a set of
capabilities, and adjust until it succeeds.

Approach 2 has the downside that the 'somehow' is not trivial and has
overhead.

In fact, the 'somehow' in approach 2 could just be the trial-and-error
from approach 1.  We could have a function

   cockpit.bridge_has_capability(address, payload, feature)

that would try to open a channel with the given payload and capability
and cache the result.  If the error message contains a list of all
supported capabilities, we would learn all of them in one go.


So, I think I am mostly repeating what you have said...


More information about the cockpit-devel mailing list