use protobuf content type instead of json for k8s client

[bhavi-koduru]

Is this a bug fix or adding new feature?

What is this PR about? / Why do we need it?
This MR is a part of effort to elevate single eks cluster performance by migrating the EKS components to use protobuf instead of json.

Modify kubeconfig type to use content type application/vnd.kubernetes.protobuf instead of json for performance gain.

This change essentially lets the ebs driver (client) to talk to apiserver using protobuf and falls back to json if protobuf isn't supported. It will only affect the wire format between ebs driver and apiserver.

What testing is done?

[k8s-ci-robot]

Hi @bhavi-koduru. 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-sigs/prow repository.

[AndrewSirenko]

/ok-to-test

[hakuna-matatah]

/lgtm

[AndrewSirenko]

FYI @ConnorJC3 @ElijahQuinones @torredil Seems like this is documented in Kubernetes API Concepts | Alternate Representations of resources

@bhavi-koduru mentioned that this support was enabled in k8s 1.13 so we are safe for all supported K8s versions. (x-ref: kubernetes/client-go#76).

CRDs (sometimes?) are JSON-only. We require snapshot CRDs for snapshot workflows. (not the driver binary) But it seems that snapshot CRDs have protobuf encodings anyway? (see external-snapshotter/client/apis/...types.go)

I may do some research to ensure no noticeable performance regression before approving this PR, but it is good that e2e tests are passing.

Seems like there have been some (old) cases of setting ContentType to protobuf breaking things, but that might have been because those projects did not set the fallback to be JSON via config.AcceptContentTypes?

[ConnorJC3]

If protobuf is so much better than JSON is there a reason it isn't enabled by default in client-go? Are we opting into potentially less tested or less stable code here?

Also, is it even worth taking this risk for the driver binary itself? It rarely communicates with the API server (off the top of my head I can only think of the startup taint removal and pre-stop hooks).

[AndrewSirenko]

If protobuf is so much better than JSON is there a reason it isn't enabled by default in client-go?

Yeah I had same question, turns out it was not enabled by default because not all CRDs support protobuf, and there was concern about protobuf encodings being less stable than JSON encodings back in 2017. At-least that is what this issue rabbit hole mentions: kubernetes/client-go#76

Also, is it even worth taking this risk for the driver binary itself? It rarely communicates with the API server

Good point, @bhavi-koduru is there a world where PRs on the kubernetes-csi sidecars (e.g. external-attacher, resizer, provisioner, snapshotter) makes more sense?

Those are the components of EBS CSI Driver that watch the K8s API Server and then call this plugin's gRPC endpoints to perform EBS volume / EC2 instance actions like creating and mounting volumes.

[mengqiy]

CRDs currently only support json. That's why this change has a fallback after protobuf.
Protobuf encoding is as reliable as (if not more) json encoding. Protobuf is used for all core types between apiserver <-> etcd and apiserver <-> kube-controller-manager, scheduler, kubelet and more core components.

Controller-runtime which is most popular SDK to build controller has adopted protobuf in 2020: kubernetes-sigs/controller-runtime#1149

There is an attempt kubernetes/kubernetes#125314 in upstream to change the default. But it will take some time.

[bhavi-koduru]

is there a world where PRs on the kubernetes-csi sidecars (e.g. external-attacher, resizer, provisioner, snapshotter) makes more sense?

The scope of this task is to get eks/aws components using protobuf. Hence, we are not focusing on upstream components currently.

[torredil]

I think @ConnorJC3 and @AndrewSirenko raise valid concerns here. That said, I'm more inclined to accept this change if kubernetes-csi/external-attacher#585 is accepted, especially given that standard components default to protobuf.

[hakuna-matatah]

If protobuf is so much better than JSON is there a reason it isn't enabled by default in client-go? Are we opting into potentially less tested or less stable code here?

Main reason why it wasn't defaulted is because of this today FMU kubernetes/kubernetes#26130

If it wasn't exposed, defaulting would have happened already.

[torredil]

Thank you for the contribution!
/lgtm

[ElijahQuinones]

/lgtm

[k8s-ci-robot]

@ElijahQuinones: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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-sigs/prow repository.

[ConnorJC3]

/approve

I did some deep digging, and based on what I found this looks to be safe in the versions we support. I would still encourage the submitter to consider contributing to other upstream projects like the sidecars if they're looking for a big improvement howerver.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ConnorJC3

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 [ConnorJC3]

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

[ConnorJC3]

/retest

likely flake