KAFKA-12278; Ensure exposed api versions are consistent within listener scopes

[hachikuji]

With KIP-500, we have more complex requirements on API accessibility. Previously all APIs were accessible on every listener exposed by the broker, but now that is no longer true. For example:

• the controller exposes some APIs which are not accessible on the broker listener (e.g. quorum/registration/heartbeat APIs)
• most of the client APIs are not exposed on the controller (e.g. consumer group apis)
• there are some APIs which are not implemented by the KIP-500 broker (e.g. LeaderAndIsr and UpdateMetadata)
• there are some APIs which are only implemented by the KIP-500 broker (e.g. DecommissionBroker and DescribeQuorum)

All of this means that we need more sophistication in how we expose APIs and keep them consistent with the ApiVersions API. Up to now, we have been working around this using the controllerOnly flag inside ApiKeys, but this is not rich enough to support all of the cases listed above.

In this patch, we address this by problem by introducing a new listeners field to the request schema definitions. This field is an array of strings which indicate the listener types in which the API should be exposed. We currently support the following listener types:

• zkBroker: old broker
• broker: kip-500 broker
• controller: kip-500 controller

For example, the DecommissionBroker API has the following listeners tag:

  "listeners": ["broker", "controller"]

This indicates that the API is only on the KIP-500 broker and controller (both are needed because the request will be sent by clients and forwarded to the controller).

The patch changes the generator so that the listener type definitions are added to ApiMessageType and exposed through convenient helpers. At the same time, we have removed the controllerOnly flag from ApiKeys since now we can identify all controller APIs through the "controller" tag.

The rest of the patch is dedicated to ensuring that the API listener type is properly set and that the APIs accordingly tagged are carried through to the ApiVersionsResponse. We have created a new ApiVersionManager which encapsulates the creation of the ApiVersionsResponse based on the listener type. Additionally, SocketServer is modified to ensure the listener type of received requests before forwarding them to the request handler.

We have also fixed a bug in the handling of the ApiVersionsResponse prior to authentication. Previously a static response was sent, which means that changes to features would not get reflected. This also meant that the logic to ensure that only the intersection of version ranges supported by the controller would get exposed did not work. I think this is important because some clients rely on the initial pre-authenticated ApiVersions response rather than doing a second round after authentication as the Java client does.

One final cleanup note: I have removed the expectation that envelope requests are only allowed on "privileged" listeners. This made sense initially because we expected to use forwarding before the KIP-500 controller was available. That is not the case anymore and we expect the Envelope API to only be exposed on the controller listener. I have nevertheless preserved the existing workarounds to allow verification of the forwarding behavior in integration testing.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[ijuma]

The network package is not meant to access requests, right? Could we make it work by changing the supplier to something more generic?

[hachikuji]

Hmm.. Do you have any ideas? I guess I would say that the dependency is there regardless of how we choose to hide it since we have to go through ChannelBuilders to build the authenticator, and the authenticator does depend on the request APIs.

[cmccabe]

I guess I would say that the dependency is there regardless of how we choose to hide it since we have to go through ChannelBuilders to build the authenticator, and the authenticator does depend on the request APIs.

Yeah, the API versions response is pretty special in the protocol. Fortunately or unfortunately, it's not treated as a generic response. I tried to think of some ways to avoid this dependency but they all ended up being kind of like obfuscation.

Could we make it work by changing the supplier to something more generic?

Maybe I'm misinterpreting the suggestion, but if we choose to make this a Supplier<AbstractResponse>, that's still in the requests package....

[ijuma]

I meant that you could introduce an interface that the response implements. That interface would live in the network package or in the common package.

[cmccabe]

Hmm. Let's revisit this after 2.8.

[hachikuji]

@cmccabe @ijuma This is ready for another look. I have changed scope to listeners and I removed the "raft" listener type.

[cmccabe]

I would really prefer not to have all this ForwardingManager stuff in here. The ControllerServer has been committed to trunk and does handle ENVELOPE_REQUEST so I think this is out of date.

It should be as simple as controllers handle envelope requests, brokers don't, right?

[hachikuji]

The ForwardingManager is used by the broker. The reason we depend on it here is because of the need to intersect the controller api versions.

I agree with you about the envelope handling. I was planning to submit a follow-up to remove this logic from KafkaApis. As you said, only the controller needs to be able to handle the Envelope API. Doing so here, however, would add a significant diff.

[cmccabe]

ok

[cmccabe]

LGTM

[cmccabe]

pushed