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

[krancour]

The first argument to ctrl.NewManager(...) is a normal *rest.Config from client-go.

That type is composed of the rest.ContentConfig type among others.

rest.ContentConfig has fields Accept ContentTypes and ContentType that are documented as follows:

type ContentConfig struct {
	// AcceptContentTypes specifies the types the client will accept and is optional.
	// If not set, ContentType will be used to define the Accept header
	AcceptContentTypes string
	// ContentType specifies the wire format used to communicate with the server.
	// This value will be set as the Accept header on requests made to the server, and
	// as the default content type on any object sent to the server. If not set,
	// "application/json" is used.
	ContentType string

	//...
}

Controller runtime does not seem to honor these rules. When neither ContentType nor AcceptContentTypes are explicitly set, JSON is used for CR types (since that is all that is possible), but protobufs are used for any type that is built-in to k8s.

I cannot find any documentation or even an issue explaining this. The change occurred in #1149, but was made without explanation.

So the only documentation anyone has to go on is what rest.Config says and Controller Runtime seems to defy those rules.

Would it be possible for this behavior be explicitly documented somewhere along with the rationale for doing so (so that people know what they're getting themselves into if they override it)?

Many apologies in advance if this is documented somewhere and I have failed to find it.

[vincepri]

Controller Runtime uses the dynamic client to operate on types (CRDs or core types). Core types are currently handled through protobuf, given that we have the protos available and it's usually quicker to serialize/deserialize; note that protobuf isn't supported for CRD resources.

What are you trying to accomplish?

[krancour]

Thanks for the response @vincepri. You're echoing my understanding.

I first noticed this when working on a proxy that could only handle application/json.

I know how to solve for that. I just opened this issue to call attention to surprising behavior that doesn't seem to be documented anywhere.

[vincepri]

If you set the ContentType, this

controller-runtime/pkg/client/apiutil/apimachinery.go

Lines 203 to 210 in 56a973c

	// TODO(FillZpp): In the long run, we want to check discovery or something to make sure that this is actually true.
	if cfg.ContentType == "" && !isUnstructured {
	protobufSchemeLock.RLock()
	if protobufScheme.Recognizes(gvk) {
	cfg.ContentType = runtime.ContentTypeProtobuf
	}
	protobufSchemeLock.RUnlock()
	}

should go through and ignore protobuf, if that's not the case we might need a reproducible test that shows the bug and correct it.

cc @FillZpp

[vincepri]

Re-reading your original description, seems like it was expected that by default only application/json is used, which isn't true for controller runtime. We should probably adjust our documentation to reflect that core types are treated differently if no ContentType is set

[vincepri]

/kind documentation

[vincepri]

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

[FillZpp]

@krancour The documentation you showed is the doc of client-go, which means if controller-runtime does not set ContentType, it will use application/json for all types. But actually controller-runtime uses protobuf for built-in types by default, which could extremely improve performance and reduce the memory cost of serialize/deserialize.

So I will open a PR to add some documentation for this. Thanks for reporting!
/assign