-
Notifications
You must be signed in to change notification settings - Fork 4
feat: introduce node compatibility status API #338
Conversation
} | ||
|
||
message CompatibilityStatus { | ||
string state = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use an enum here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use enums in protobufs currently because of their interop with json-schemas and because we need enums to be stored as string in the database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused... this shouldn't be a problem.
The grpc-gateway tooling that transforms Protobuf messages to its JSON counterpart can alter the enums to their string representation. As such, via the REST API, users would provide a string, and would also be stored as a string in the database.
Have you ever tried actually using enums? Perhaps the wrong Protobuf JSON marshaller was being used, but I can assure you, this is supported (as long as enums_as_ints
is disabled):
https://grpc-ecosystem.github.io/grpc-gateway/docs/mapping/customizing_openapi_output/#omitting-the-default-value-of-enums
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grpc-gateway supports it yes. Does protojson supports something like it?
The current structure of the code is to use json-schema for all such validation and the protobuf layer is pretty thin. We can introduce enum but then the same enums have to be declared in the json-schema as well. I don't feel strongly either way. I've resisted enums in protobufs because they can't be converted to strings, something Kong may do since it doesn't define a protobuf contract itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's supported by protojson:
https://github.com/protocolbuffers/protobuf-go/blob/v1.28.1/encoding/protojson/encode_test.go#L268-L277
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts about the second part of my comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to use enums, as there shouldn't be any reason not to use them as they can be transformed to/from strings as expected. For fields like this, it just makes sense & is proper API contract design (even in the OpenAPI world).
Regarding the JSON schema, well, the enums should be declared there anyway if they're fixed values. The protoc-go tooling would automatically generate a slice of the enums, so it's very easy to automatically throw those into wherever needed (especially that we're auto-genning the JSON schemas from Go code anyway).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. I'm still hesitant because Kong can change a field from enum (internally in the schema) to a string and that is not a backward-incompatible change. Koko won't be able to do that once it uses enum. Let's try with this specific field since it is a Koko-only field.
I've a question. Is it possible to change the string value of an enum in protojson?
Marshalling the following:
json.Marshaller.Marshal(&v1.Node{
CompatibilityStatus: &v1.CompatibilityStatus{
State: v1.CompatibilityState_COMPATIBILITY_STATE_FULLY_COMPATIBLE,
},
})
outputs:
{"compatibility_status":{"state":"COMPATIBILITY_STATE_FULLY_COMPATIBLE"}}
Is there a way to instead output (and read back) "FULLY_COMPATIBLE" instead? The enum is named as above because we enforce a buf linting rule for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming generated there is part of the protoc Golang codegen process. Sadly, it's not possible to alter the naming within the generated code via a configurable option (technically you can do it if you were to hack it in, but I'd frown against that).
This is known by the community & tracked here. Such capability is supported in gogo/protobuf, but I would personally avoid that library (lots of other OSS projects have moved off it as well).
The biggest gripe with the enum naming when marshaled to/from JSON is how it's outputted & consumed over the REST API (as how it's stored in the DB isn't really a problem). With that said, it is technically possible to re-write the JSON request/response there, but there's no built-in tooling to specifically re-write Protobuf enums. I've never done that specifically, but I have had to re-write Protobuf messages to comply with non-JSON API consumers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the spec to use an enum, please take another look.
a745743
to
6cd4b3f
Compare
6cd4b3f
to
05368c8
Compare
05368c8
to
b3b2ef9
Compare
CP modifies the configuration based on the version of DP. These modifications need to be tracked and exposed to the end-user to inform the user of (potentially adverse) configuration mutations This patch contains the user-facing API as well as internal API for this, Specifically, - User facing "Node" resource gets a new CompatibilityStatus message. The user can consult this object to determine compatibility status and issues (if any) associated with the node. The "state" field will be an enum that will be defined in code in a subsequent patch. The currently planned values are "FULLY-COMPATIBLE" and "PARTIALLY-COMPATIBLE". - Compatibility for each node is tracked as a separate resource internally. CP (ws.Manager) is responsible for tracking compatibility status of each node in the database (via the relay service). Foreign key relationships ensure easy clean up of NodeStatus resources. The (internal) API will be responsible of merging Node and NodeStatus resouces and presenting it as a unified single Node resource to the end user. The separation is done for performance reasons: Node resource sees frequent u pdates (once every 30s) while NodeStatus is expected to see far fewer updates which may be very large (128 possible changes with 128 resources each can result in > 7000 kilobytes of data)
b3b2ef9
to
ac1b56b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
CP modifies the configuration based on the version of DP. These
modifications need to be tracked and exposed to the end-user to inform
the user of (potentially adverse) configuration mutations
This patch contains the user-facing API as well as internal API for
this, Specifically,
The user can consult this object to determine compatibility status and
issues (if any) associated with the node. The "state" field will be an
enum that will be defined in code in a subsequent patch. The currently
planned values are "FULLY-COMPATIBLE" and "PARTIALLY-COMPATIBLE".
internally. CP (ws.Manager) is responsible for tracking compatibility
status of each node in the database (via the relay service). Foreign
key relationships ensure easy clean up of NodeStatus resources. The
(internal) API will be responsible of merging Node and NodeStatus resource and
presenting it as a unified single Node resource to the end user.
The separation is done for performance reasons: Node resource sees frequent
updates (once every 30s) while NodeStatus is expected to see far fewer updates
which may be very large (128 possible changes with 128 resources each can result
in > 7000 kilobytes of data)