KAFKA-9768: Fix handling of rest.advertised.listener config

[C0urante]

Jira

The rest.advertised.listener config is currently broken as setting it to http when listeners are configured for both https and http will cause the framework to choose whichever of the two listeners is listed first. The changes here attempt to fix this by checking not only that ServerConnector::getName begins with the specified protocol, but also that that protocol is immediately followed by an underscore, which the framework uses as a delimiter between the protocol and the remainder of the connector name.

An existing unit test for the RestServer::advertisedUrl method has been expanded to include a case that fails with the framework in its current state and passes with the changes in this PR.

See

kafka/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java

Line 181 in ee0ef09

connector.setName(String.format("%s_%s%d", PROTOCOL_HTTPS, hostname, port));

and

kafka/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java

Line 186 in ee0ef09

connector.setName(String.format("%s_%s%d", PROTOCOL_HTTP, hostname, port));

for how the framework sets the name for the ServerConnector instances it creates.

Committer Checklist (excluded from commit message)

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

[C0urante]

@rhauch @kkonstantine would you mind taking a look at this when you have a chance?

[kkonstantine]

The fix seems adequate as a bugfix, although the name is not set by Jetty.

The name is a convention which is set by us in the same class (see Connector#setName). That's where the underscore is used as a delimiter. Unless I'm missing some log message, the connector name probably doesn't even need to contain the hostname and the port. In fact for the admin server, the connector name is just Admin.

We could consider setting the name to something that we'd match exactly and in a unique way with a mapping or enum to the protocol.
For example names Secured, Plain, Admin for https, http only and admin.

[C0urante]

Yes @kkonstantine, I'd updated the description of the PR a few hours before you left your comment linking to the LOC in the RestServer class that determined the name for the Jetty Connector instances.

I don't see a significant advantage to adding an enum or changing how the Connector instances are named as long as we have a guarantee of correctness and a guard against regression, which we do in the form of unit tests for the advertised URL logic.

It would probably have been cleaner to implement this logic initially with an enum or some other strict mapping, but it'd probably introduce more risk than it'd be worth to do that now for what is intended to be a small bug fix.

[kkonstantine]

As I mentioned, this fix looks sufficient to me to fix the bug and get backported at the moment.

The ideal solution, without going into implementation details, would be a way to have an exact match from protocol to connector, given that the connector name can be anything. Again, not suggested for this PR.

@C0urante have you checked whether this issue has a configuration workaround (whether the order of listeners matters) and whether it's a regression (and since when) or it's been like that since the ssl endpoint was introduced?

[C0urante]

The ideal solution, without going into implementation details, would be a way to have an exact match from protocol to connector, given that the connector name can be anything. Again, not suggested for this PR.

Understood, I'm still skeptical that it'd be beneficial to perform this refactoring any time soon without any functional changes to the Connect REST server to warrant them. But if someone else wants to take on that work and they can do it without breaking anything, it'd certainly make the code base easier to read.

have you checked whether this issue has a configuration workaround (whether the order of listeners matters)

Yep, the order of listeners can be used as a workaround. Added to the ticket.

and whether it's a regression (and since when) or it's been like that since the ssl endpoint was introduced?

It's been like that since the SSL endpoint was introduced; the blame is a little misleading since @wicknicks did some work on the REST server to add the admin listener, but the bug was present before that work as well. Also added to the ticket, along with a specific version number (1.1.0).

[C0urante]

Thanks for the discussion, @kkonstantine and @rhauch. I've added a Javadoc to the findConnector method that outlines its use, and an internal comment to that method explaining how the server connector names are used, how they're formatted, and that the logic for the method would need to be changed if the formatting for the server connector names were ever altered.

How does this look?

[kkonstantine]

ok to test

[rhauch]

One more nit, just to clarify the intention in the test.

[C0urante]

Thanks @rhauch, I've accepted the suggestion.

[kkonstantine]

This LGTM now.

[kkonstantine]

retest this please

[kkonstantine]

Only a couple streams integration test failures in every jdk.
These are unrelated to this PR, so I'm merging.
Thanks for the fix @C0urante !