API: Add ResourceConversion service #1044
Conversation
toddtreece
requested review from
wbrowne,
andresmgot,
xnyo and
ryantxu
and removed request for
a team
| // Raw is the serialized object | ||
| bytes raw = 1; | ||
| // ContentType is the media type of the raw object | ||
| string content_type = 2; |
There was a problem hiding this comment.
i was thinking that content_type would most likely always be application/json, but this would allow additional encodings if needed in the future
|
|
||
| // Objects to convert | ||
| // +listType=atomic | ||
| repeated RawObject objects = 3; |
There was a problem hiding this comment.
@ryantxu the CRD webhook supports multiple objects, so i added support for that. i'm assuming that's used by list requests to reduce the number of round trips
| // otherwise identical (parallel requests, etc). | ||
| // The UID is meant to track the round trip (request/response) between the Kubernetes API server and the webhook, not the user request. | ||
| // It is suitable for correlating log entries between the webhook and apiserver, for either auditing or debugging. | ||
| string uid = 2; |
There was a problem hiding this comment.
uid is also something that is present in CRD conversion webhooks, so this is forwarding that information in case it's useful for debugging
| PluginContext pluginContext = 1; | ||
|
|
||
| // The object kind | ||
| GroupVersionKind kind = 2; |
There was a problem hiding this comment.
it looks like the new conversion request does not specify the source GVK -- is that accurate? Does that mean we need to parse the raw bytes to detect it first? Does k8s know the value before sending?
There was a problem hiding this comment.
@ryantxu the version can be different for every object in the request. maybe a helper could turn this back into a function that handles one object at a time?
@andresmgot admission hooks are used in the k8s write path before the object is sent to storage and conversion is used in the read path, so they are separate concepts. it's possible that many simpler APIs might not have a need to implement custom admission logic, but would have a need to convert between API versions.
this will be used by k8s apiserver conversion workflow or the CRD conversion webhook to convert from the stored version to the version the user requested
i added this context in a comment instead of the description because i thought it would be easier to discover during review, but i think lint rules buried it. multiple should reduce the number of requests during list operations |
| @@ -321,9 +321,6 @@ service AdmissionControl { | |||
|
|
|||
| // Return a modified copy of the request that can be saved or a descriptive error | |||
| rpc MutateAdmission(AdmissionRequest) returns (MutationResponse); | |||
|
|
|||
| // Convert a resource to a new version | |||
| rpc ConvertObject(ConversionRequest) returns (ConversionResponse); | |||
There was a problem hiding this comment.
@andresmgot @wbrowne how should i handle these lint failures?
There was a problem hiding this comment.
These checks aren't required to merge and are primarily there (IIRC) to ensure people are aware/conscious of when breaking changes are being made. We use the FILE category for breaking detection so we know when clients will break. Since these services aren't used yet (even though plugins will have been built with this capability and Grafana is setup to call them), it should be safe. It wouldn't hurt to do some manual validation though.
I'd say if you want to be extra careful then just add the new stuff and just deprecate the other instead if possible:
// Convert a resource to a new version
rpc ConvertObject(ConversionRequest) returns (ConversionResponse) {
option deprecated = true;
};There was a problem hiding this comment.
good call. i'll test this out with grafana and a plugin using the previous SDK version before merging. i'll add the deprecated method if there are any issues
|
@ryantxu @andresmgot i added test coverage, so i think this is ready for a final review |
|
LGTM -- still experimental, so I expect we will iterate a bit more 😃 |
toddtreece
force-pushed
the
toddtreece/resource-conversion
branch
from
4d994c9 to
d191d41
Compare
|
tested with grafana/grafana#91977 |
What this PR does / why we need it:
This adds a
ResourceConversionservice to thepluginv2protobuf and removesConvertObjectfrom theAdmissionControlservice.Admission hooks are used in the write path (create, update, delete) and conversion is used in the read path, so this PR separates the concepts.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
I will add test coverage before this merges if the general approach seems acceptable