Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRD Conversion API Changes #67795

Merged
merged 4 commits into from Nov 1, 2018

Conversation

@mbohlool
Copy link
Member

mbohlool commented Aug 24, 2018

This is the API changes section of Custom Resource Conversion feature (kubernetes/community#2420) extracted from main implementation PR #67006 for the purpose of a separate API review.

@deads2k @lavalamp @sttts @kubernetes/sig-api-machinery-api-reviews

NONE

@k8s-ci-robot k8s-ci-robot requested review from justinsb and sttts Aug 24, 2018

@mbohlool mbohlool changed the title Crd Conversion Api Changes Crd Conversion API Changes Aug 24, 2018

@mbohlool mbohlool changed the title Crd Conversion API Changes CRD Conversion API Changes Aug 24, 2018

@mbohlool mbohlool referenced this pull request Aug 24, 2018

Merged

CRD webhook conversion #67006

@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented Aug 26, 2018

/retest

@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented Aug 26, 2018

@sttts @deads2k Please take a look

@dims

This comment has been minimized.

Copy link
Member

dims commented Aug 27, 2018

/sig architecture

we need sign off from them too, right?

// - `Webhook`: API Server will call to an external webhook to do the conversion. Additional information is needed for this option.
Strategy ConversionStrategyType

// Additional information for external conversion if strategy is set to external

This comment has been minimized.

Copy link
@sttts

sttts Aug 27, 2018

Contributor

is set to "Webhook"

// - `Webhook`: API Server will call to an external webhook to do the conversion. Additional information is needed for this option.
Strategy ConversionStrategyType `json:"strategy" protobuf:"bytes,1,name=strategy"`

// Additional information for external conversion if strategy is set to external

This comment has been minimized.

Copy link
@sttts

sttts Aug 27, 2018

Contributor

is set to "Webhook"

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 27, 2018

Member

If strategy is Webhook, must contain instructions for how to call the webhook.

// ConvertedObject is the converted version of request.Object if the Result is nil.
// +optional
ConvertedObject runtime.RawExtension `json:"convertedObject" protobuf:"bytes,2,name=convertedObject"`
// Result contains extra details into why a conversion request was failed. If it is not nil, it means

This comment has been minimized.

Copy link
@sttts

sttts Aug 27, 2018

Contributor

it's not a pointer. It can't be nil.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 27, 2018

Member

please make it a pointer & optional

This comment has been minimized.

Copy link
@mbohlool

mbohlool Aug 28, 2018

Author Member

runtime.RawExtension can be nil (having nil value). The optional runtime.RawExtension is not a pointer in other APIs (at least the ones I checked like WatchEvent or AdmissionRequest)

@lavalamp
Copy link
Member

lavalamp left a comment

Some API suggestions.


const (
// NopConverter is a converter that only sets apiversion of the CR and leave everything else unchanged.
NopConverter ConversionStrategyType = "None"

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 27, 2018

Member

RenameConverter? "Nop" is maybe a little obscure, and anyway, no abbreviations in the API with few exceptions.

NoChangeConverter? RepackageConverter? NullConverter?

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 27, 2018

Member

ah I see the name isn't going to be in the docs, so it matters less. In that case, let's make it match "None" even though NoneConverter sounds a bit weird?


// CustomResourceConversion describes how to convert different versions of a CR.
type CustomResourceConversion struct {
// Strategy specifies the conversion strategy. Allowed values are:

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 27, 2018

Member

strategy -- match json field name.

// - `Webhook`: API Server will call to an external webhook to do the conversion. Additional information is needed for this option.
Strategy ConversionStrategyType `json:"strategy" protobuf:"bytes,1,name=strategy"`

// Additional information for external conversion if strategy is set to external

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 27, 2018

Member

If strategy is Webhook, must contain instructions for how to call the webhook.

}

// CustomResourceConversionWebhook describes how to call a conversion webhook.
type CustomResourceConversionWebhook struct {

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 27, 2018

Member

What is the purpose of this layer of indirection?

This comment has been minimized.

Copy link
@mbohlool

mbohlool Aug 29, 2018

Author Member

removed

Response *ConversionResponse `json:"response,omitempty" protobuf:"bytes,2,opt,name=response"`
}

// ConversionRequest describes a conversion request parameters.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 27, 2018

Member

s/a/the/

// +optional
UID types.UID `json:"uid,omitempty" protobuf:"bytes,1,opt,name=uid"`
// The version to convert given object to. E.g. "stable.example.com/v1"
APIVersion string `json:"apiVersion" protobuf:"bytes,2,name=apiVersion"`

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 27, 2018

Member

Please separate it out into separate group and kind fields--friends don't make friends parse strings ;)

Please name it so it's clear it's the desired version, not the version of the thing that is being sent.

This comment has been minimized.

Copy link
@mbohlool

mbohlool Aug 28, 2018

Author Member

There is no need to parse the string (as an example, look at my example converter webhook in the main PR test/images folder). I find it more clear to use the whole APIVersion in switch cases of the conversion.

This comment has been minimized.

Copy link
@mbohlool

This comment has been minimized.

Copy link
@mbohlool

mbohlool Aug 29, 2018

Author Member

Also the field in the CR is APIVersion in the same form (e.g. stable.example.com/v1). If we separate it out then our friends need to merge strings which is not horrible but why? :)

Probably looking at these two code samples make it easier to argue that APIVersion make more sense:

https://github.com/mbohlool/kubernetes/blob/b31503af5365263544ad36e73abbd268085d65c5/test/images/crd-conversion-webhook/converter/example_converter.go#L40

https://github.com/mbohlool/kubernetes/blob/b31503af5365263544ad36e73abbd268085d65c5/test/images/crd-conversion-webhook/converter/framework.go#L102

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 30, 2018

Member

OK, no need to split it if it is always used together (I still think in an absolute sense it's better for it to be passed around as a pair, but fixing only one spot is maybe worse than leaving it like this).

However I still think it should be named more clearly, like "DesiredAPIVersion", "DestinationAPIVersion", or something like that. ("Target" is not good as it's ambiguous.)

// Object is the CRD object to be converted.
Object runtime.RawExtension `json:"object" protobuf:"bytes,3,name=object"`
// IsList indicates that this is a List operation and Object contains a list of items.
IsList bool `json:"isList" protobuf:"varint,4,name=isList"`

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 27, 2018

Member

I haven't looked back at the KEP, so maybe you answered this there, but why not just make Object into Objects []runtime.RawExtension? Then the webhook always gets a list, and apiserver can pack as many or few objects into the list as it wants.

This comment has been minimized.

Copy link
@mbohlool

mbohlool Aug 28, 2018

Author Member

I have a more detailed answer for this in the main PR, but briefly, this will allow the webhook to change list metas.

This comment has been minimized.

Copy link
@liggitt

liggitt Aug 28, 2018

Member

why do we want the conversion webhook to inspect/change anything under listmeta? I can't think of a reason conversion of individual objects should care about resourceVersion or continue tokens for the containing list

This comment has been minimized.

Copy link
@mbohlool

mbohlool Aug 29, 2018

Author Member

Agreed. Will make this a array.

// ConvertedObject is the converted version of request.Object if the Result is nil.
// +optional
ConvertedObject runtime.RawExtension `json:"convertedObject" protobuf:"bytes,2,name=convertedObject"`
// Result contains extra details into why a conversion request was failed. If it is not nil, it means

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 27, 2018

Member

please make it a pointer & optional

@mbohlool mbohlool force-pushed the mbohlool:crd_conversion_api_changes branch from 1f6a9a2 to d337746 Aug 29, 2018

@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented Aug 29, 2018

@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented Aug 29, 2018

/retest

2 similar comments
@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented Aug 29, 2018

/retest

@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented Aug 30, 2018

/retest

UID types.UID `json:"uid,omitempty" protobuf:"bytes,1,opt,name=uid"`
// ConvertedObjects is the list of converted version of request.Objects if the Result is successful otherwise empty.
ConvertedObjects []runtime.RawExtension `json:"convertedObjects" protobuf:"bytes,2,rep,name=convertedObjects"`
// Result contains the result of conversion with extra details if the conversion failed. Result.Status determines if

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 30, 2018

Member

Is this field optional? What should it be set to for successful conversions? Unsuccessful ones? What will happen with the reason / message fields?

This comment has been minimized.

Copy link
@mbohlool

mbohlool Aug 30, 2018

Author Member

Added comment to explain that.

// otherwise identical (parallel requests, requests when earlier requests did not modify etc)
// The UID is meant to track the round trip (request/response) between the KAS and the WebHook, not the user request.
// It is suitable for correlating log entries between the webhook and apiserver, for either auditing or debugging.
// +optional

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 30, 2018

Member

Is this actually optional?

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Aug 30, 2018

This LGTM with the few minor changes I mentioned.

@mbohlool mbohlool force-pushed the mbohlool:crd_conversion_api_changes branch 2 times, most recently from 2422154 to 1a8aae1 Aug 30, 2018

@mbohlool mbohlool force-pushed the mbohlool:crd_conversion_api_changes branch 2 times, most recently from 5ae3346 to f77fd9f Oct 31, 2018

Mehdy Bohlool added some commits Sep 3, 2018

@mbohlool mbohlool force-pushed the mbohlool:crd_conversion_api_changes branch from f77fd9f to 0e2024f Oct 31, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Oct 31, 2018

/approve
/lgtm

looks like verify script is failing on generated file

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Oct 31, 2018

cc @sttts @lavalamp for k8s.io/apiextensions-apiserver approval

@mbohlool mbohlool force-pushed the mbohlool:crd_conversion_api_changes branch from 0e2024f to 96241de Oct 31, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 31, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Oct 31, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 31, 2018

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Oct 31, 2018

/approve

We should get the api types here owned by the api reviewers/approvers.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 31, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, liggitt, mbohlool

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:

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

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Oct 31, 2018

We should get the api types here owned by the api reviewers/approvers.

#70453

Mehdy Bohlool

@mbohlool mbohlool force-pushed the mbohlool:crd_conversion_api_changes branch from 96241de to e27096c Oct 31, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 31, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 31, 2018

New changes are detected. LGTM label has been removed.

@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented Nov 1, 2018

/test pull-kubernetes-e2e-gce-100-performance

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Nov 1, 2018

set an empty release note, make sure we include release notes in the implementation PR

@k8s-ci-robot k8s-ci-robot merged commit 11706d3 into kubernetes:master Nov 1, 2018

18 checks passed

cla/linuxfoundation mbohlool authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@liggitt liggitt referenced this pull request Jan 24, 2019

Open

Extensibility features - Path to GA #73185

0 of 12 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.