⚠️ Use application/vnd.kubernetes.protobuf as content-type if possible #1149

FillZpp · 2020-09-02T06:48:02Z

Signed-off-by: Siyu Wang FillZpp.pub@gmail.com

Use application/vnd.kubernetes.protobuf as content-type if it is possible, currently for those native resources defined in client-go.

k8s-ci-robot · 2020-09-02T06:48:10Z

Welcome @FillZpp!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-09-02T06:48:10Z

Hi @FillZpp. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

FillZpp · 2020-09-02T06:49:41Z

/assign @alvaroaleman

alvaroaleman · 2020-09-02T12:54:51Z

pkg/client/apiutil/apimachinery.go

@@ -93,5 +102,8 @@ func createRestConfig(gvk schema.GroupVersionKind, baseConfig *rest.Config) *res
 	if cfg.UserAgent == "" {
 		cfg.UserAgent = rest.DefaultKubernetesUserAgent()
 	}
+	if cfg.ContentType == "" && localScheme.Recognizes(gvk) {


So you copy the client-go scheme so this is only used for built-in types? Are you sure protobuf works for all built-in types on all kube server versions? Does upstream have any docs on this?

Do you know what happens if we request protobuf for a type that doesn't support it, is there maybe some way to negotiate?

So you copy the client-go scheme so this is only used for built-in types?

Yes, this is only used for built-in resources. For custom resources, CRDs can not support Protocol Buffers but Aggregated API can. You can see it in this table.

Are you sure protobuf works for all built-in types on all kube server versions? Does upstream have any docs on this?

All built-in resources are guaranteed to implement Protocal Buffers:

This design doc for contributors says protobuf schema for each API group and version in k8s.io/api will be generated from the objects in that API group and version.

hack/update-generated-protobuf.sh in Kubernetes repo will find all packages contains +k8s:protobuf-gen=package and generate protobuf schema for them. And all API definition package must contain a doc.go which has these comments:

// +k8s:deepcopy-gen=package // +k8s:protobuf-gen=packag

missed this comment. I think this should probably be a bit less magic, since this ignores aggregated api servers or any future improvements to CRDs. This is probably fine in the short run, but in the long run I think we want to check discovery or something to make sure that this is actually true.

Understand, but currently there is no way to check if a resource type supports protobuf or not. I have added a TODO to describe this.

alvaroaleman · 2020-09-14T17:59:09Z

pkg/client/apiutil/apimachinery.go

@@ -93,5 +102,8 @@ func createRestConfig(gvk schema.GroupVersionKind, baseConfig *rest.Config) *res
 	if cfg.UserAgent == "" {
 		cfg.UserAgent = rest.DefaultKubernetesUserAgent()
 	}
+	if cfg.ContentType == "" && localScheme.Recognizes(gvk) {
+		cfg.ContentType = "application/vnd.kubernetes.protobuf"


Nit: Can you use runtime.ContentTypeProtobuf here?

alvaroaleman

/approve
/hold
/assign @DirectXMan12

alvaroaleman · 2020-09-14T17:59:46Z

/ok-to-test

DirectXMan12 · 2020-10-13T20:01:05Z

pkg/client/apiutil/apimachinery.go

+)
+
+func init() {
+	_ = clientgoscheme.AddToScheme(localScheme)


this should not be using some sort of magic private scheme -- we should be using the user's passed-in scheme. That way, if the user also defines protobuf support for their types (e.g. aggregated api servers, if we get proto support in the future for CRDs, etc) it'll just work.

EDIT: missed the comment above, responding up there.

Nit: Can we panic here if this fails?

DirectXMan12 · 2020-10-13T20:22:06Z

pkg/client/client_cache.go

+	_, isUnstructuredList := obj.(*unstructured.UnstructuredList)
+	isUnstructured = isUnstructured || isUnstructuredList
+
+	resourceByType := c.structuredResourceByType


move this so that it's under the lock

alvaroaleman

Some nits, other than that it looks good. Also this change is breaking due to the signature change of RESTClientForGVK.

alvaroaleman · 2020-10-15T15:58:04Z

pkg/client/apiutil/apimachinery.go

+)
+
+func init() {
+	_ = clientgoscheme.AddToScheme(localScheme)


Nit: Can we panic here if this fails?

alvaroaleman · 2020-10-15T15:58:45Z

pkg/client/apiutil/apimachinery.go

 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/restmapper"
 )

+var (
+	localScheme = runtime.NewScheme()


Nit: Can we rename to "protobufScheme"? The current name is a bit too generic IMHO

alvaroaleman

/lgtm
leaving hold cancel to @DirectXMan12

alvaroaleman · 2020-10-19T13:10:50Z

and thanks for your work, this is a great change!

vincepri

LGTM
/milestone v0.7.x

FillZpp · 2020-10-23T03:03:38Z

PTAL @DirectXMan12 when you have time

alvaroaleman · 2020-10-23T13:34:46Z

Retitling because this PR includes a breaking change in RESTClientForGVK
/retitle ⚠️ Use application/vnd.kubernetes.protobuf as content-type if possible

vincepri · 2020-11-20T17:20:06Z

Are we ok unholding this change? @alvaroaleman @DirectXMan12

alvaroaleman · 2020-11-20T17:33:14Z

pkg/client/apiutil/apimachinery.go

 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/restmapper"
 )

+var (
+	protobufScheme = runtime.NewScheme()


Actually one nit: Can we add a setter for this so projects that have additional apis that do support protobuf (metrics-server, all the openshift apis that are served via its aggregated apiserver) can add themselves here?

I am also fine doing a quick follow-up for that and merging this PR as-is

Actually one nit: Can we add a setter for this so projects that have additional apis that do support protobuf (metrics-server, all the openshift apis that are served via its aggregated apiserver) can add themselves here?

Sure. How about provide a function in pkg/client/apiutil/apimachinery.go?

func AdditionalProtobufScheme(builder runtime.SchemeBuilder) error { return builder.AddToScheme(protobufScheme) }

Sounds good to me. Could you also add a small godoc comment explaining it?

How about AddToProtobufScheme to make the experience familiar with the other AddToScheme methods?

I have add this func and a RWMutex to avoid race modification of protobufScheme.
PTAL @alvaroaleman @vincepri

Signed-off-by: Siyu Wang <FillZpp.pub@gmail.com>

k8s-ci-robot · 2020-11-24T03:34:43Z

@FillZpp: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-controller-runtime-apidiff-master	`7c26bc0`	link	`/test pull-controller-runtime-apidiff-master`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

alvaroaleman

/lgtm

vincepri

/approve

@alvaroaleman good to unhold?

k8s-ci-robot · 2020-11-30T04:02:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, FillZpp, vincepri

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:

~~OWNERS~~ [alvaroaleman,vincepri]

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

alvaroaleman · 2020-11-30T13:41:00Z

/hold cancel

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 2, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 2, 2020

k8s-ci-robot requested review from alvaroaleman and DirectXMan12 September 2, 2020 06:48

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 2, 2020

k8s-ci-robot assigned alvaroaleman Sep 2, 2020

alvaroaleman reviewed Sep 2, 2020

View reviewed changes

alvaroaleman reviewed Sep 14, 2020

View reviewed changes

alvaroaleman approved these changes Sep 14, 2020

View reviewed changes

k8s-ci-robot assigned DirectXMan12 Sep 14, 2020

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 14, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 14, 2020

FillZpp force-pushed the use-protobuf-for-native-resources branch 2 times, most recently from 9077fb6 to a8946c3 Compare September 15, 2020 04:59

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 Sep 15, 2020

FillZpp force-pushed the use-protobuf-for-native-resources branch from a8946c3 to 5a22a5f Compare September 15, 2020 08:21

DirectXMan12 suggested changes Oct 13, 2020

View reviewed changes

FillZpp force-pushed the use-protobuf-for-native-resources branch 4 times, most recently from 9176b46 to 3abb8c6 Compare October 14, 2020 12:50

alvaroaleman reviewed Oct 15, 2020

View reviewed changes

FillZpp force-pushed the use-protobuf-for-native-resources branch from 3abb8c6 to 00b4b34 Compare October 19, 2020 10:07

alvaroaleman approved these changes Oct 19, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2020

vincepri reviewed Oct 19, 2020

View reviewed changes

k8s-ci-robot added this to the v0.7.x milestone Oct 19, 2020

k8s-ci-robot changed the title ~~✨ Use application/vnd.kubernetes.protobuf as content-type if possible~~ ⚠️ Use application/vnd.kubernetes.protobuf as content-type if possible Oct 23, 2020

alvaroaleman reviewed Nov 20, 2020

View reviewed changes

Use application/vnd.kubernetes.protobuf as content-type if possible

7c26bc0

Signed-off-by: Siyu Wang <FillZpp.pub@gmail.com>

FillZpp force-pushed the use-protobuf-for-native-resources branch from 00b4b34 to 7c26bc0 Compare November 24, 2020 03:33

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 24, 2020

alvaroaleman approved these changes Nov 24, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 24, 2020

vincepri approved these changes Nov 30, 2020

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2020

k8s-ci-robot merged commit e2261d9 into kubernetes-sigs:master Nov 30, 2020

timebertt mentioned this pull request Dec 21, 2020

☂️-Issue for upgrading to k/*@v0.19 and controller-runtime@v0.7 gardener/gardener#3109

Closed

timebertt mentioned this pull request Jan 12, 2021

Use protobuf in gardener components gardener/gardener#2836

Closed

FillZpp mentioned this pull request Aug 22, 2021

Use proto only if we have proto types #1638

Closed

FillZpp mentioned this pull request Nov 16, 2021

REQUEST: New membership for FillZpp kubernetes/org#3100

Closed

7 tasks

FillZpp mentioned this pull request Dec 22, 2021

🌱 Add FillZpp as a reviewer #1753

Merged

FillZpp mentioned this pull request Aug 2, 2022

#1045 specifies the kruise-manager client accept protobuf first openkruise/kruise#1045

Closed

krancour mentioned this pull request Jul 17, 2023

Clarify that in the default case, an empty ContentType is json for everything, and protobuf for core #2408

Closed

⚠️ Use application/vnd.kubernetes.protobuf as content-type if possible #1149

⚠️ Use application/vnd.kubernetes.protobuf as content-type if possible #1149

Conversation

FillZpp commented Sep 2, 2020

k8s-ci-robot commented Sep 2, 2020

k8s-ci-robot commented Sep 2, 2020

FillZpp commented Sep 2, 2020

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

alvaroaleman left a comment

Choose a reason for hiding this comment

alvaroaleman commented Sep 14, 2020

DirectXMan12 Oct 13, 2020 • edited

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

alvaroaleman left a 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

alvaroaleman left a comment

Choose a reason for hiding this comment

alvaroaleman commented Oct 19, 2020

vincepri left a comment

Choose a reason for hiding this comment

FillZpp commented Oct 23, 2020

alvaroaleman commented Oct 23, 2020

vincepri commented Nov 20, 2020

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

k8s-ci-robot commented Nov 24, 2020

alvaroaleman left a comment

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 30, 2020

alvaroaleman commented Nov 30, 2020

DirectXMan12 Oct 13, 2020 •

edited