Use proto only if we have proto types #1638

justinsb · 2021-08-20T14:46:29Z

✨ WIP: Use proto only if we have proto definitions

Even if the apiserver supports proto for a type, if we don't know the
proto schema definition we can't use it.

Therefore, detect if the type implements proto.Message when deciding
whether to use proto encoding.

k8s-ci-robot · 2021-08-20T14:46:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: justinsb
To complete the pull request process, please assign vincepri after the PR has been reviewed.
You can assign the PR to them by writing /assign @vincepri in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

coderanger · 2021-08-20T19:06:58Z

pkg/client/apiutil/apimachinery.go

+	if gvkType.Kind() == reflect.Struct {
+		gvkType = reflect.PtrTo(gvkType)
+	}
+	return gvkType.Implements(protoMessageType)


Is this slow enough that we should cache it? I know reflect operations can get pretty cranky sometimes.

It did involve a linear scan, so I added memoization - thanks!

FillZpp · 2021-08-21T05:57:51Z

pkg/client/apiutil/apimachinery.go

+	if gvkType.Kind() == reflect.Struct {
+		gvkType = reflect.PtrTo(gvkType)
+	}
+	return gvkType.Implements(protoMessageType)


I'm afraid the gvk type has proto definition in scheme doesn't mean the API Server could accept its requests via Protobuf. Actually for a gvk type defined by CRD, API Server can only accept JSON requests even if we implement its proto types in project.

Do you know if there's a way to discover if the apiserver supports proto (for a given kind)? I think we need destination-type-is-proto and apiserver-supports-proto-for-this-kind, because if someone queries a PodList into an UnstructuredList we still want to not use proto (?).

BTW the reason that I'm looking at this code is that I'd like to eliminate most of the imports of the kubernetes API types, for controllers that generally just use unstructured.Unstructured.

You can see this PR Use application/vnd.kubernetes.protobuf as content-type if possible I merged before, which brings Protobuf support into controller-runtime.

// Currently only enabled for built-in resources which are guaranteed to implement Protocol Buffers. // For custom resources, CRDs can not support Protocol Buffers but Aggregated API can. // See doc: https://kubernetes.io/docs/concepts/extend-kubernetes/api-extension/custom-resources/#advanced-features-and-flexibility

So if you want to extend the no-built-in types to use Protobuf, you have to find out whether they are defined by CRD or Aggregated API, which is hard to check. Since all types created via kubebuilder/operator-sdk are in CRD definition, I think the built-in types are enough for most ppl using controller-runtime.

WDYT @alvaroaleman

Good point, I guess it is possible (and something I would love to try out!) to use proto to define CRDs locally.

I tweaked the logic therefore, there's a list of built-in types, and we have a unit test to make sure that the core kubernetes types are recognized appropriately. This should detect if there's a new built-in library added upstream, and I added a comment so that it's obvious how to add the newly-added built-in group.

This also means that we can continue to support AddToProtobufScheme.

alvaroaleman · 2021-08-22T14:34:04Z

@justinsb what problem does this PR solve? I agree it is currently a bit hard to discover how to register your api for proto usage, however the only case where this could be wanted is an extension api server that supports proto. To my knowledge, there aren't a lot of them, so this is a bit of a niche problem.

justinsb · 2021-11-13T22:37:21Z

Thanks for the reviews and comment @alvaroaleman and @coderanger . The intention here is actually the other way round; I want to eliminate all dependencies on kubernetes/scheme. Merely importing that package pulls in all the k8s API types, and that adds up to a lot more memory usage than is really needed (>500 entries in the scheme, instead of around a dozen that are really needed in a typical controller). It also causes the binary size to be much larger, though that is a secondary concern for me.

This is by no means the last place we need this, but ... it's a journey!

We should not try to use proto if the scheme does not have the protobuf representation in the go type; we won't be able to deserialize it.

k8s-ci-robot · 2021-11-13T22:41:52Z

@justinsb: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-controller-runtime-apidiff-master	`9f1aac0`	link	false	`/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.

k8s-triage-robot · 2022-02-11T23:04:12Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-03-14T00:03:26Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2022-04-13T00:06:44Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2022-04-13T00:06:53Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 20, 2021

k8s-ci-robot requested review from gerred and shawn-hurley August 20, 2021 14:46

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 20, 2021

justinsb force-pushed the use_proto_if_proto branch 2 times, most recently from 513e6cc to 8067a1d Compare August 20, 2021 15:39

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 20, 2021

justinsb force-pushed the use_proto_if_proto branch from 8067a1d to 5a35d58 Compare August 20, 2021 15:44

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 20, 2021

coderanger reviewed Aug 20, 2021

View reviewed changes

FillZpp reviewed Aug 21, 2021

View reviewed changes

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

justinsb force-pushed the use_proto_if_proto branch from 5a35d58 to 0776b2c Compare November 13, 2021 21:30

justinsb changed the title ~~WIP: Use proto if we have proto types~~ Use proto only if we have proto types Nov 13, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2021

justinsb force-pushed the use_proto_if_proto branch 2 times, most recently from 602bb3c to 0780d4f Compare November 13, 2021 22:34

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2021

Use proto only if we have proto types

9f1aac0

We should not try to use proto if the scheme does not have the protobuf representation in the go type; we won't be able to deserialize it.

justinsb force-pushed the use_proto_if_proto branch from 0780d4f to 9f1aac0 Compare November 13, 2021 22:40

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

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 11, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 14, 2022

k8s-ci-robot closed this Apr 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use proto only if we have proto types #1638

Use proto only if we have proto types #1638

justinsb commented Aug 20, 2021

k8s-ci-robot commented Aug 20, 2021

coderanger Aug 20, 2021

justinsb Nov 13, 2021

FillZpp Aug 21, 2021

justinsb Aug 21, 2021 •

edited

Loading

FillZpp Aug 22, 2021

justinsb Nov 13, 2021

alvaroaleman commented Aug 22, 2021

justinsb commented Nov 13, 2021 •

edited

Loading

k8s-ci-robot commented Nov 13, 2021 •

edited

Loading

k8s-triage-robot commented Feb 11, 2022

k8s-triage-robot commented Mar 14, 2022

k8s-triage-robot commented Apr 13, 2022

k8s-ci-robot commented Apr 13, 2022

Use proto only if we have proto types #1638

Use proto only if we have proto types #1638

Conversation

justinsb commented Aug 20, 2021

k8s-ci-robot commented Aug 20, 2021

coderanger Aug 20, 2021

Choose a reason for hiding this comment

justinsb Nov 13, 2021

Choose a reason for hiding this comment

FillZpp Aug 21, 2021

Choose a reason for hiding this comment

justinsb Aug 21, 2021 • edited Loading

Choose a reason for hiding this comment

FillZpp Aug 22, 2021

Choose a reason for hiding this comment

justinsb Nov 13, 2021

Choose a reason for hiding this comment

alvaroaleman commented Aug 22, 2021

justinsb commented Nov 13, 2021 • edited Loading

k8s-ci-robot commented Nov 13, 2021 • edited Loading

k8s-triage-robot commented Feb 11, 2022

k8s-triage-robot commented Mar 14, 2022

k8s-triage-robot commented Apr 13, 2022

k8s-ci-robot commented Apr 13, 2022

justinsb Aug 21, 2021 •

edited

Loading

justinsb commented Nov 13, 2021 •

edited

Loading

k8s-ci-robot commented Nov 13, 2021 •

edited

Loading