Improvements to CRD webhook conversion KEP #2484

mbohlool · 2018-08-07T01:24:55Z

A follow up to #2420 to address @lavalamp comments. @sttts @deads2k @erictune please let me know if you have any comment on the KEP and I can address them here or in a new PR.

/assign @lavalamp
/sig api-machinery

sttts · 2018-08-07T05:21:21Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

  // These three fields should not be set for first item in Versions list
  Schema *JSONSchemaProp
  Subresources *CustomResourceSubresources
  AdditionalPrinterColumns []CustomResourceColumnDefinition
 }

 Type CustomResourceConversion struct {
-  // Conversion strategy, either "nop” or "webhook”. If webhook is set, Webhook field is required.
+  // Conversion strategy, either "None” or "Webhook”. If Webhook is set, Webhook field is required.


third "webhook" is lower-case

sttts · 2018-08-07T05:23:39Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

@@ -225,7 +230,7 @@ error on server: conversion from stored version v1 to requested version v2 for s
 ```


-For operations that need more than one conversion (e.g. LIST), no partial result will be returned. Instead the whole operation will fail the same way with detailed error messages. To help debugging these kind of operations, the UID of the first failing conversion will also be included in the error message. 
+For operations that need more than one conversion (e.g. LIST), no partial result will be returned. Instead the whole operation will fail the same way with detailed error messages. Also the webhook will recieve the whole list to convert instead of items one by one.


receive

This sounds strange now to have LIST as example for multiple calls and for one call for a list.

fixed. Where do we have LIST as an example of multiple calls? It needs multiple conversion but it will be done with a single http call.

k8s-ci-robot · 2018-08-07T07:02:22Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp

If they are not already assigned, you can assign the PR to them by writing /assign @lavalamp in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

contributors/design-proposals/api-machinery/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mbohlool · 2018-08-24T04:02:45Z

@sttts PTAL
@lavalamp ping

sttts · 2018-08-24T08:23:48Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

-  // Optional, and correspond to the first version in the versions list  
+
+
+// These fields are optional and correspond to the first version in the versions list


sttts · 2018-08-24T08:24:40Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

@@ -178,20 +178,27 @@ type ConversionRequest struct {
  UID types.UID
  // The version to convert given object to. E.g. "stable.example.com/v1"
  APIVersion string
-  // Object is the CRD object to be converted.
+  // Object is the CRD object or list of objects to be converted.


nit: list of CRD objects

This is the custom CustomResourceList type or a generic List?

Object will not be a CRD object, right? It will be of the type described in the CRD?

Also +1 to being explicit about the list type

nit: CR not CRD. Will fix that.

The list is a UnstructuredList (not sure what CustomResourceList is but I assume that is what you meant).

lavalamp · 2018-08-24T16:39:08Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

  Object runtime.RawExtension
+  // IsList indicates that this is a List operation and Object contains a list of items.
+  IsList bool


Why is this necessary?

Another way to represent would be to have List []runtime.RawExtension and require that exactly one of object or list be set.

List has its own meta data. Do we care let conversion webhook to modify that? right now the webhook is responsible for any change to that meta data (specifically changing the APIVersion). If we pass a list of objects, then the caller(APIServer) not the webhook is responsible for any list metadata changes.

I don't see why the actual list object should be exposed to the conversion hook. Conversion is by list item by definition. @lavalamp's slice suggestion would make the interface obvious. We could even drop the Object field and just have an Objects instead.

+1 for unifying the interface to something like objects []runtime.RawExtension, treating single and multi-item conversion requests consistently.

lavalamp · 2018-08-24T16:39:29Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

-  // ConvertedObject is the converted version of request.Object.
-  ConvertedObject runtime.RawExtension
+  // ConvertedObject is the converted version of request.Object if the Result field is nil.
+  // +optional


Probably worth being explicit about what to do for lists.

lavalamp · 2018-08-24T16:41:56Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

@@ -225,7 +232,7 @@ error on server: conversion from stored version v1 to requested version v2 for s
 ```


-For operations that need more than one conversion (e.g. LIST), no partial result will be returned. Instead the whole operation will fail the same way with detailed error messages. To help debugging these kind of operations, the UID of the first failing conversion will also be included in the error message. 
+For operations that need more than one conversion (e.g. LIST), no partial result will be returned. Instead the whole operation will fail the same way with detailed error message. Also the webhook will receive the whole list to convert instead of items one by one.


Is it actually easy to always group items in a list like this, given our stack?

I think we shouldn't promise the entire list in one call. We will probably wish to call it in multiple batches, to avoid causing the webhook to need super high memory usage if for no other reason.

refer to the other comment about list metadata. We pass a list item and implementation is strightforward right now. Even with current design and implementation, we can divide List object (UnstructuredList) into smaller valid List items and send them to webhook. We promise here that we send an UnstructuredList to the webhook. The webhook is not aware of the actual user request and we do not promise we send that whole user requested objects in one call.

lavalamp · 2018-08-24T16:44:25Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

 }
 ```

-If the conversion is failed, the webhook should fail the HTTP request with a proper error code and message that will be used to create a status error for the original API caller.
+If the conversion is failed, the webhook is prefered to fill up the Result field of the response with a proper error code and message instead of returning failure HTTP codes.


Perhaps: "Instead of returning failure HTTP status codes, the preferred way for a webhook to indicate a conversion failure is to fill the result field of the response with an error code and message. Non-200 status return codes will be taken to mean there was a problem with the webhook rather than a problem with the object to be converted."

Fixed that in the implementation and will fix it here too.

mbohlool · 2018-10-02T23:47:05Z

@lavalamp @sttts PTAL.

justaugustus · 2018-10-13T03:18:33Z

/kind kep

k8s-ci-robot · 2018-10-28T19:15:49Z

@mbohlool: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mattfarina · 2018-11-09T14:32:08Z

@lavalamp @deads2k what's the next step on this one?

liggitt · 2018-11-09T14:34:56Z

obsoleted by #2737, I thought

justaugustus · 2018-11-20T04:46:53Z

REMINDER: KEPs are moving to k/enhancements on November 30. Please attempt to merge this KEP before then to signal consensus.
For more details on this change, review this thread.

Any questions regarding this move should be directed to that thread and not asked on GitHub.

justaugustus · 2018-12-01T08:03:50Z

KEPs have moved to k/enhancements.
This PR will be closed and any additional changes to this KEP should be submitted to k/enhancements.
For more details on this change, review this thread.

Any questions regarding this move should be directed to that thread and not asked on GitHub.
/close

k8s-ci-robot · 2018-12-01T08:03:51Z

@justaugustus: Closed this PR.

In response to this:

KEPs have moved to k/enhancements.
This PR will be closed and any additional changes to this KEP should be submitted to k/enhancements.
For more details on this change, review this thread.

Any questions regarding this move should be directed to that thread and not asked on GitHub.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot assigned lavalamp Aug 7, 2018

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Aug 7, 2018

k8s-ci-robot requested review from deads2k and lavalamp August 7, 2018 01:25

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/design Categorizes issue or PR as related to design. labels Aug 7, 2018

sttts reviewed Aug 7, 2018

View reviewed changes

mbohlool force-pushed the crd_conversion_kep branch from d9ef19d to ee1c967 Compare August 7, 2018 07:02

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 7, 2018

mbohlool force-pushed the crd_conversion_kep branch from ee1c967 to c46ce6e Compare August 24, 2018 04:02

sttts reviewed Aug 24, 2018

View reviewed changes

lavalamp reviewed Aug 24, 2018

View reviewed changes

Improvements to CRD webhook conversion KEP

0c1b0a8

mbohlool force-pushed the crd_conversion_kep branch from c46ce6e to 0c1b0a8 Compare October 2, 2018 23:46

k8s-ci-robot added the kind/kep label Oct 13, 2018

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2018

k8s-ci-robot closed this Dec 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to CRD webhook conversion KEP #2484

Improvements to CRD webhook conversion KEP #2484

mbohlool commented Aug 7, 2018

sttts Aug 7, 2018

sttts Aug 24, 2018

sttts Aug 7, 2018

mbohlool Aug 24, 2018

k8s-ci-robot commented Aug 7, 2018

mbohlool commented Aug 24, 2018

sttts Aug 24, 2018

sttts Aug 24, 2018

sttts Aug 24, 2018

lavalamp Aug 24, 2018

mbohlool Aug 28, 2018

lavalamp Aug 24, 2018

mbohlool Aug 28, 2018

sttts Aug 28, 2018

liggitt Aug 28, 2018

lavalamp Aug 24, 2018

lavalamp Aug 24, 2018

mbohlool Aug 28, 2018

lavalamp Aug 24, 2018

mbohlool Aug 28, 2018

mbohlool commented Oct 2, 2018

justaugustus commented Oct 13, 2018

k8s-ci-robot commented Oct 28, 2018

mattfarina commented Nov 9, 2018

liggitt commented Nov 9, 2018

justaugustus commented Nov 20, 2018

justaugustus commented Dec 1, 2018

k8s-ci-robot commented Dec 1, 2018

		// Optional, and correspond to the first version in the versions list


		// These fields are optional and correspond to the first version in the versions list

Improvements to CRD webhook conversion KEP #2484

Improvements to CRD webhook conversion KEP #2484

Conversation

mbohlool commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 7, 2018

mbohlool commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbohlool commented Oct 2, 2018

justaugustus commented Oct 13, 2018

k8s-ci-robot commented Oct 28, 2018

mattfarina commented Nov 9, 2018

liggitt commented Nov 9, 2018

justaugustus commented Nov 20, 2018

justaugustus commented Dec 1, 2018

k8s-ci-robot commented Dec 1, 2018