Change the shape of update nic to be valid PUT #1288
Conversation
| }, | ||
| "primary": { | ||
| "description": "Make a secondary interface the instance's primary interface.\n\nIf applied to a secondary interface, that interface will become the primary on the next reboot of the instance. Note that this may have implications for routing between instances, as the new primary interface will be on a distinct subnet from the previous primary interface.\n\nNote that this can only be used to select a new primary interface for an instance. Requests to change the primary interface into a secondary will return an error.", | ||
| "default": false, |
There was a problem hiding this comment.
@bnaecker is is true that the default is false? E.g. submitting without a value will try to make it not the primary interface? Just wondering if a PUT was issued for the primary interface to rename it if the default would cause the request to fail by trying to make it not the primary.
There was a problem hiding this comment.
It's true that the default is false. The value is ignored if the target interface is the primary. I.e., whether you've got true or false there, and that was specified by the client or left as the default, it's never touched if we're updating the primary interface.
There was a problem hiding this comment.
I realize now I never updated that docstring. It's not true that it returns an error, it's just ignored. It's not the best behavior, but generating an error condition was trickier than I thought in the database query. Would you mind changing that from will return an error to will be ignored? Or maybe: Other parameters will be updated, but requests to change a primary interface into a secondary will be ignored. Not sure about the wordsmithing here.
| } | ||
|
|
||
| impl From<params::NetworkInterfaceUpdate> for NetworkInterfaceUpdate { | ||
| fn from(params: params::NetworkInterfaceUpdate) -> Self { | ||
| let make_primary = if params.make_primary { Some(true) } else { None }; | ||
| let primary = if params.primary { Some(true) } else { None }; |
There was a problem hiding this comment.
I guess this probably predates this change, but I think having optional booleans with default value "false" kind of sucks because a lot of client code treats falsey (like null or undefined or otherwise missing) as false. I'd be all for making these required.
There was a problem hiding this comment.
Can you explain a bit more why that's a problem? A value of false is effectively ignored in the implementation, since (1) if the interface is a secondary, then that'd be a no-op, and (2) setting the primary to a secondary isn't supported, one has to set a secondary to the new primary. It seems like the client sending a value of false or sending nothing both have the intended behavior in that case.
There was a problem hiding this comment.
It's a non-specific problem, really. Given that this is only an input parameter, and we control the server, and we're using a language that doesn't make it easy to confuse falsey with false, it's unlikely we'll run into a specific problem with this case.
If you're asking "how could this ever be a problem": imagine we use this pattern in a view struct and a customer uses a client for a language that makes it easy to conflate false with falsey (certainly JavaScript is one of these). They fetch the object that's missing some_property, and they act as though it's false (because they wrote if (!thing.some_property) { /* do something that assumes the property is false */ }, when it's really true (because the default value is true).
This is a lot less likely with an input param, but it's not impossible: suppose we (or a customer for that matter) winds up writing a mock API server or a proxy server that serves the same API and we use one of these languages for it. We can have the same problem there.
I think this case is probably fine. It just feels safer to me to say that we won't use this pattern anywhere than to rely on us only using it safely (e.g., only on inputs).
There was a problem hiding this comment.
I see, thanks for the explanation. I was focused on this particular API, but yes, I agree in general it might be better to just avoid this type of interface.
Non-blocking for the demo
I'm working on oxidecomputer/console#933 and I noticed that the PUT endpoint for networks have
make_primaryinstead ofprimarylike the GET endpoint has. While I understand it's invalid to submitfalsefor the interface that's already primary and therefore this param is technically functioning as described, it deviates from typical PUT request params by changing the shape of the object being re-posted. Ideally we could take a GET object alter a value and repost it as a PUT.Having to change the shape of the object before updating means downstream work for clients which would be nice to avoid.