This is really the sorta thing we should discuss on cockpit-devel. I'll CC the email.
For anyone following along, there was an addition to the dbus-json3 channel in the cockpit transport protocol that broke connecting from earlier versions cockpit.js to later versions of cockpit-bridge.
On 18.06.2015 03:17, Marius Vollmer wrote:
Peter Volpe pvolpe@redhat.com writes:
As Stef brought up on IRC we already have channel options for optional behavior so my plan is
- Add a permissive_client boolean with a default value of false to
the dbus channel options here: https://github.com/cockpit-project/cockpit/blob/master/src/bridge/cockpitdbu.... Only when that is true will we send the owned message.
Hmm, I was thinking that we add a flag explicitly for the "owner" protocol addition.
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.
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.
For the "owner" message, this doesn't make a big difference, but for other protocol additions, the bridge or the JS might want to choose between alternatives based on the capabilities of the peer.
Right exactly. This case seems not to require that the callee understands about 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.
- 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.
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.
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.
Features would be static and per payload for a given bridge executable and a given cockpit.js file.
For example, the bridge might announce "can-notify-owner" for the dbus-json3 payload so that the client knows whether it will actually get the notifications or not. (And the client announces "want-notify-owner" to tell the bridge to actually send them.)
What about inventing a new payload type just for feature exchange? Old bridges without features will reject that payload, which we can interpret that as "no features".
I think that's exactly what the "caps" stuff is. Except for to keep it simple, it's just one way. The caller tells the callee what it requires ... and the callee closes the channel if it doesn't understand one of the requirements.
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.
Stef
Stef Walter stefw@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.
- 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...
On 22.06.2015 05:35, Marius Vollmer wrote:
Stef Walter stefw@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.)
<snip>
We talked about this further....
The plan for Cockpit has always been that by and large a compatible bridge and javascript code run together. You can see that in the way we componentize, in the way we load the code from the destination system, etc.
This relieves most of the complexity of protocol feature detection and negotiation from most of the code.
The exception to this is dashboard code. The "capabilities" logic in cockpit-bridge was meant to help with these few corner cases.
But ... it's buggy. Lets treat this as bugs, update Cockpit in the places where this buggy code exists, and move on.
We don't need to add further layers of complexity because we encountered these bugs. We're still in early stages of making this all work, so bugs aren't totally unexpected. Of course, it's important to double check that they're actually fixed this time around.
Stef
cockpit-devel@lists.fedorahosted.org
-
Marius Vollmer -
Stef Walter