doc: Remove REQUIRED for boolean values #265
Conversation
|
Who is making decisions about CSI messages by looking at gRPC message wire format? Yes, it may be difficult for a receiver to know whether a sender actually attempted to send a default value, or simply didn't set a value for some scalar field. If that is the problem that we're trying to solve, then instead of removing REQUIRED we could use the Well Known Type cousins of the scalar types (they're basically common message wrappers around scalars). Is this proving to be a problem in practice? |
|
Decisions are made reading the CSI specs. If the specs say that a field is REQUIRED, then it is common sense that the field will be present in the message. So the right thing to do would be to have a check in the CSI plugin to ensure that the field is present and fail if it's not. I would say this has been an issue for at least one person (me), as I created an issue on csi-sanity and removed the check from my code before realizing this was how gRPC works, and that we could not know if the field was present when the call was made. So I wanted to avoid this issue for anybody else. After all, given the way gRPC works, saying REQUIRED on a |
|
So the argument is "because REQUIRED isn't verifiable for boolean fields in
gRPC messages, the spec should avoid using it for such" ?
…On Tue, Aug 21, 2018 at 5:30 AM Gorka Eguileor ***@***.***> wrote:
Decisions are made reading the CSI specs. If the specs say that a field is
*REQUIRED*, then it is common sense that the field *will be present in
the message*. So the right thing to do would be to have a check in the
CSI plugin to ensure that the field is present and fail if it's not.
I would say this has been an issue for at least one person (me), as I
created an issue on csi-sanity
<kubernetes-csi/csi-test#86> and removed the
check from my code
<Akrog/ember-csi@dfc8600>
before realizing this was how gRPC works, and that we could not know if the
field was present when the call was made.
So I wanted to avoid this issue for anybody else. After all, given the way
gRPC works, saying *REQUIRED* on a boolean field means literally *nothing*.
So why would we want to say something in the spec that isn't true?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#265 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACPVLDn0q7JQbJbvT4dEbiKDDvgoAmWPks5uS9NJgaJpZM4WDmVf>
.
|
|
I can try to phrase it a little bit better:
|
|
Being able to differentiate between REQUIRED and OPTIONAL is useful. Maybe we can add a generic comment at the top of the doc that says for REQUIRED scalar fields they will be defaulted if not specified per proto3. This is more of a problem for OPTIONAL fields that are scalar. But, we took a look at the existing OPTIONAL fields and only found one in |
This seems like an acceptable compromise. We can make developers aware of this implementation detail while keeping the spec more agnostic from the underlying transport protocol. |
spec.md
Outdated
| @@ -16,6 +16,8 @@ The key words "unspecified", "undefined", and "implementation-defined" are to be | |||
| An implementation is not compliant if it fails to satisfy one or more of the MUST, REQUIRED, or SHALL requirements for the protocols it implements. | |||
| An implementation is compliant if it satisfies all the MUST, REQUIRED, and SHALL requirements for the protocols it implements. | |||
|
|
|||
| Scalar fields, even REQUIRED ones, will be defaulted if not specified, and any field set to the defaul value will not be serialized over the wire as per [proto3](https://developers.google.com/protocol-buffers/docs/proto3#default). | |||
There was a problem hiding this comment.
Wording looks good. But can we move this to a section right after the Size Limits section?
There was a problem hiding this comment.
Sure, it does makes more sense to be in the Required vs Optional section.
GRPC proto3 docs states that all scalar fields have a default value, and that default values for scalars will not be transferred over the wire. This makes it impossible to know if a message had a field set in the first place, so even if a field is REQUIRED in the spec and it is set on send, it may not be present on the message. This is something that developers must be aware when writing plugins, as they should not be check for the presence of scalar REQUIRED fields on the message, as they'll only be present when the value is not the default. In this patch we add a comment at the beginning of the document for those who are not aware of this implementation detail of proto3.
|
LGTM |
Setting REQUIRED on boolean values is misleading, as it may cause CSI
implementers to think that the field must be present in the GRPC payload
and that they should fail if it's not there, which is incorrect.
As mentioned in the GRPC Proto3 docs: "note that if a scalar message
field is set to its default, the value will not be serialized on the wire."
This means that boolean fields will not be sent over the wire when their
value is set to false. Making it impossible to know whether the GRPC
message had the field set in the first place.
This patch updates the comments to reflect the default value and removes
the misleading REQUIRED notation.